From 807ceca1dcd684f90e36f4d2b59b89c7c470ae99 Mon Sep 17 00:00:00 2001 From: Viacheslav Rostovtsev Date: Tue, 12 Aug 2025 10:53:34 -0700 Subject: [PATCH 1/4] fix: no longer generate cross-package references for mixin elements --- gapic-generator/lib/gapic/formatting_utils.rb | 7 ++- gapic-generator/lib/gapic/model/mixins.rb | 26 ++++++++++ .../test/gapic/mixins/mixins_test.rb | 16 ++++++ shared/gem_defaults.rb | 4 ++ shared/input/testing_desc.bin | Bin 187393 -> 196827 bytes .../proto_docs/google/iam/v1/policy.rb | 2 +- .../proto_docs/google/iam/v1/policy.rb | 2 +- .../lib/google/cloud/location/locations_pb.rb | 48 ++++++++++++++++++ .../cloud/location/locations_services_pb.rb | 47 +++++++++++++++++ .../testing/lib/testing/mixins/mixins_pb.rb | 3 +- .../proto_docs/testing/mixins/mixins.rb | 7 +++ shared/protos/testing/mixins/mixins.proto | 6 +++ .../testing/mixins/testing_service.yaml | 3 ++ 13 files changed, 167 insertions(+), 4 deletions(-) create mode 100644 shared/output/gapic/templates/testing/lib/google/cloud/location/locations_pb.rb create mode 100644 shared/output/gapic/templates/testing/lib/google/cloud/location/locations_services_pb.rb diff --git a/gapic-generator/lib/gapic/formatting_utils.rb b/gapic-generator/lib/gapic/formatting_utils.rb index 4afaf6eb7..b6627829b 100644 --- a/gapic-generator/lib/gapic/formatting_utils.rb +++ b/gapic-generator/lib/gapic/formatting_utils.rb @@ -116,7 +116,12 @@ def escape_line_braces line def format_line_xrefs api, line, disable_xrefs, transport while (m = @xref_detector.match line) entity = api.lookup m[:addr] - return line if entity.nil? + is_mixin_field_addr = Gapic::Model::Mixins.mixin_message_field_address?( + m[:addr], + gem_name: api.configuration.fetch(:gem, nil)&.fetch(:name, "") + ) + return line if entity.nil? || is_mixin_field_addr + text = m[:text] yard_link = disable_xrefs ? text : yard_link_for_entity(entity, text, transport) return line if yard_link.nil? diff --git a/gapic-generator/lib/gapic/model/mixins.rb b/gapic-generator/lib/gapic/model/mixins.rb index 1183b4833..e4062dd40 100644 --- a/gapic-generator/lib/gapic/model/mixins.rb +++ b/gapic-generator/lib/gapic/model/mixins.rb @@ -162,6 +162,25 @@ def self.mixin_service_address? service_address, gem_name: nil MIXIN_GEM_NAMES.include?(service_address) && gem_name != MIXIN_GEM_NAMES[service_address] end + ## + # Returns true if the given service address is a mixin. + # This just checks the service against a (hard-coded) set of known mixins. + # If `gem_name` is provided, objects from the package of the service + # that corresponds to that gem_name are not considered mixins. + # + # @param message_field_address [String,Array] The address (either array + # or dot-delimited) of the message or field to check. + # @param gem_name [String] The name of the gem. + # @return [boolean] + # + def self.mixin_message_field_address? message_field_address, gem_name: nil + message_field_address = message_field_address.join "." unless message_field_address.is_a? String + # NB: messages are checked against package, not service + service_address = MIXIN_GEM_NAMES.keys.find { |sn| message_field_address.start_with? MIXIN_PACKAGE_NAMES[sn] } + + !service_address.nil? && gem_name != MIXIN_GEM_NAMES[service_address] + end + private # @return [Enumerable] Names of all services that are specified @@ -179,6 +198,13 @@ def services_in_config }.freeze private_constant :MIXIN_GEM_NAMES + MIXIN_PACKAGE_NAMES = { + LOCATIONS_SERVICE => "google.cloud.location", + IAM_SERVICE => "google.iam.v1", + LRO_SERVICE => "google.longrunning.operations" + }.freeze + private_constant :MIXIN_PACKAGE_NAMES + # Since mixins are scope-limited to a couple of services, it is easier to # have these in lookup tables than to construct a ServicePresenter diff --git a/gapic-generator/test/gapic/mixins/mixins_test.rb b/gapic-generator/test/gapic/mixins/mixins_test.rb index 04d3bda8d..a4c17dbd3 100644 --- a/gapic-generator/test/gapic/mixins/mixins_test.rb +++ b/gapic-generator/test/gapic/mixins/mixins_test.rb @@ -45,6 +45,22 @@ def test_mixin_service_address_checker refute Gapic::Model::Mixins.mixin_service_address? "testing.mixins.ServiceWithLoc" end + def test_mixin_message_field_address_checker + # sic. no `s` at the end, this is a message named `Location`, not service named `Locations` + assert Gapic::Model::Mixins.mixin_message_field_address? "google.cloud.location.Location" + assert Gapic::Model::Mixins.mixin_message_field_address? "google.cloud.location.Location.metadata" + + refute Gapic::Model::Mixins.mixin_message_field_address? "google.cloud.location.Location", + gem_name: "google-cloud-location" + refute Gapic::Model::Mixins.mixin_message_field_address? "google.cloud.location.Location.metadata", + gem_name: "google-cloud-location" + + assert Gapic::Model::Mixins.mixin_message_field_address? ["google", "iam", "v1", "Policy"] + assert Gapic::Model::Mixins.mixin_message_field_address? ["google", "iam", "v1", "Policy", "bindings"] + + refute Gapic::Model::Mixins.mixin_message_field_address? "testing.mixins.Request" + end + # Test the `Garbage` library, which does NOT have mixins specified # in its service.yaml (or service.yaml at all) def test_garbage_mixins diff --git a/shared/gem_defaults.rb b/shared/gem_defaults.rb index 96a3bbb75..71f8f39af 100644 --- a/shared/gem_defaults.rb +++ b/shared/gem_defaults.rb @@ -156,6 +156,10 @@ def gem_defaults "testing/mixins/mixins.proto", "testing/routing_headers/routing_headers.proto", "testing/nonstandard_lro_grpc/nonstandard_lro_grpc.proto", + # `locations.proto` is included here because it is often + # included in the real world libraries + # as part of `files_to_generate` due to protoc limitations + "google/cloud/location/locations.proto", ], grpc_service_config: [ "../shared/protos/testing/grpc_service_config/grpc_service_config.json", diff --git a/shared/input/testing_desc.bin b/shared/input/testing_desc.bin index 7512734b97b10c2229dc2afb6f627a70b628bb2a..3a2d65e3637fccb22ca8a73bcaf372bdf1a27cce 100644 GIT binary patch delta 7259 zcmeHLPi!1l8K3uNyxn=5w9Uk^*AplEcI^h&v1em9P$j9|Zk$plibG?YMs;MRw%Ds?)!Cl<8NQFyfA)}+ zrbyD{_TbO)*Km`Bh+qY4$|-3Q>F*%AxMus*a;sHV3uwUnz^5jy+OOL+T6V1lR|_1} zOZGWS==BJyA)p!wRO51t*{HI>ESrJ3ynEo6qC@R5(jf4Y6^u0bR64h9W%2zxr8nLi zdL%7I-E#w1JF+LF@b4evU;N9?LelFq$;)( zFfWV4jn9$ZlqnkNnNQcvRkmQi!L(z9@b-0=??I$~B8z0c#$K*Az_o^USZi zHJ@oyL`}Hyvxv$gnaP~7C(afj^DqNhVd@K1R7Z&R5))ACU!eSB-^C;?a6bTuIg&}1 zzG0VROe9fVENgjkplthf$K1M6Gpj7yBT#2z-4#%2GbH7hE6nk;SYSK(2yDh>G5W)z6TOm=gZrG@5Kq{zxV&93Ub|`w1ckJ80AP z-9Iu|Lhd&j;HCQ5eqcKXN0ul&|MtD+U*5%MwBF+8MDb0|$W%g>e`_hP<=T4ir*&op zesT0x`|@cYB9e)$A#i9<@3a4oG4m{f8ri(*u>I~!Vdm|7fNo0bYolnP0~kv~TE5-W z#@B`igY?-?OTE#Bj9kpD+r9xe`;R4KEePu2zh=|2Z{V32HG>*9Aew|r%#kOcOxeOHIYP@C{cJSQB>Kgnrh@J_{`^U+B!z8WP z%qd5~G9|$&bLm4oaHb?#lPP|VG!Kv-5WWYra53r1<5x)3^L^;y(p93QBn;dPz=Y|w-?0oHKpaI_zm!05xhBZ&ks zho6}x2RTYCDVp+>GMXZXh$_TieEQo$teO<~kfs>;%d$R9;bQFt&ul>gQs9KIz&Z#3 z7^y??mIweC+HfKP0HgS93;+RuqH;O`fRqS;P`QvOfu1wci%7TLKZx~aVmIID2wV?k zxn0)VJv>fzoTfABL2`vC3O9s&4r_V|5;B8a>$?pv@l5w-&F5yg-a0DJ*p3513LXe! z)UHsoUUzKET!9!PSl|ox@D8q~F-(nmZEEqaAbS;Mgs> z)vl&>VgNeKNmu~4%u{&OL@vp5XMFiti;%C!ZU++p{z42NBv@=Ab~}&&{sOq&EWnpF z<(jmC^yh%yPIvcM9Tf2H^us$o>Tf0?C&tHp<68Pj@*V0KbhMfpCo8T(hk; zS~a)mrs-|b6&BoNtVZo>-C?a~u5S?fRfuftb(8DQ($|`WR>F;yOV-5BaD7dz>(|B370+h1va>~Z7?|(ej9+YH zG}x+hWH7{}HD-D%Thy|HXhJ7#W3&KI<1M$)0>vYP*#gCV%v6uWpm=03vjgO&00lw? z=k;|u+?l+@UOU)qg?3D(TC=Yp`v6B@-DF3}#*y}A+^w=uI%)(jPX6{yEG8yh{ z1t+MupH=z>MM4EX<3XsBqogcA!d+j-L;AIc^M8{5J1~hPYHpe~ytf@jEwgS4=HS`E z)9b3e$!er+ofAsu2s9O?t zOTy0gS5jwnOTyylj-1pj2}PONEeX3NVYeiVia>aTbxT72dHvyU*83Fc-I6f=DBmp! fyCq@#!LM5q@-IIB!;XS&A7d5wjOO^AhoL1+8+gG~NR Z+r1^3uJvu-xs%D26~DqYN0_E*0RU Date: Tue, 12 Aug 2025 11:06:20 -0700 Subject: [PATCH 2/4] fixup --- shared/input/testing_desc.bin | Bin 196827 -> 196827 bytes shared/protos/testing/mixins/mixins.proto | 3 +-- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/shared/input/testing_desc.bin b/shared/input/testing_desc.bin index 3a2d65e3637fccb22ca8a73bcaf372bdf1a27cce..6e78832e0d65c365e2d6304b5b4d7d39e0dbd51b 100644 GIT binary patch delta 50 vcmccJ$aA}qr=f*$3sa~Vqv7`L2brRnfb>QQCf1b*{&XOJ(e^DznAB|m%R3RF delta 50 vcmccJ$aA}qr=f*$3sa~VqtW*52brRnfb>QQCf1b*{&XOJ(e^DznAB|m%bO9U diff --git a/shared/protos/testing/mixins/mixins.proto b/shared/protos/testing/mixins/mixins.proto index 3424b6ebf..ee55ec027 100644 --- a/shared/protos/testing/mixins/mixins.proto +++ b/shared/protos/testing/mixins/mixins.proto @@ -47,5 +47,4 @@ message Response { // This comment tests a mixin documentation reference: // [google.cloud.location.Location.metadata][google.cloud.location.Location.metadata]. message ReferencedInServiceYaml { - -} \ No newline at end of file +} From c3e8c9a96140c1dab612d4069c2d60699a6dd204 Mon Sep 17 00:00:00 2001 From: Viacheslav Rostovtsev Date: Tue, 12 Aug 2025 17:58:25 -0700 Subject: [PATCH 3/4] fixup test --- gapic-generator/test/test_helper.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/gapic-generator/test/test_helper.rb b/gapic-generator/test/test_helper.rb index 81ff14b05..6217ba59c 100644 --- a/gapic-generator/test/test_helper.rb +++ b/gapic-generator/test/test_helper.rb @@ -344,6 +344,10 @@ def fix_namespace name def fix_service_name name @service_mapping[name] || name end + + def configuration + {} + end end # A fake request builder From c3db4c78198faa12be1c22056f513cb6a061df02 Mon Sep 17 00:00:00 2001 From: Viacheslav Rostovtsev Date: Tue, 12 Aug 2025 18:02:54 -0700 Subject: [PATCH 4/4] test: add testcase --- gapic-generator/test/gapic/mixins/mixins_test.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/gapic-generator/test/gapic/mixins/mixins_test.rb b/gapic-generator/test/gapic/mixins/mixins_test.rb index a4c17dbd3..e5dd6c6ad 100644 --- a/gapic-generator/test/gapic/mixins/mixins_test.rb +++ b/gapic-generator/test/gapic/mixins/mixins_test.rb @@ -50,6 +50,9 @@ def test_mixin_message_field_address_checker assert Gapic::Model::Mixins.mixin_message_field_address? "google.cloud.location.Location" assert Gapic::Model::Mixins.mixin_message_field_address? "google.cloud.location.Location.metadata" + assert Gapic::Model::Mixins.mixin_message_field_address? "google.cloud.location.Location", + gem_name: "google-cloud-something-else" + refute Gapic::Model::Mixins.mixin_message_field_address? "google.cloud.location.Location", gem_name: "google-cloud-location" refute Gapic::Model::Mixins.mixin_message_field_address? "google.cloud.location.Location.metadata",