Skip to content

fix: add logic to check for IAM RPC methods within the service before adding mixin to avoid generating duplicate methods#7929

Open
shivanee-p wants to merge 16 commits intomainfrom
shivaneep-iam-duplicate-mixins
Open

fix: add logic to check for IAM RPC methods within the service before adding mixin to avoid generating duplicate methods#7929
shivanee-p wants to merge 16 commits intomainfrom
shivaneep-iam-duplicate-mixins

Conversation

@shivanee-p
Copy link
Copy Markdown
Contributor

@shivanee-p shivanee-p commented Mar 31, 2026

This PR resolves issues where a service's native RPC methods are implemented even if they match the mixin methods with the same name. As a result, we end up with duplicate methods. We have implemented logic to prioritize native RPC methods over mixin implementations for IAM, Locations, and LRO (Long Running Operations).

Generator Schema (proto.ts):

  • Introduced OperationsMixinConfig to track which methods (getOperation, cancelOperation, deleteOperation, listOperations) conflict with a service's native RPC methods.
  • Optimized the logical order in augmentService so that longRunningOperationsMixinFlags are populated after the LongRunningOperationsMixin is evaluated.
  • Defined and calculated mixin flags (iamPolicyMixinFlags, locationMixinFlags, and longRunningOperationsMixinFlags) to prevent duplicate native method definitions.
  • Added assertions in test/unit/proto.ts to test the flags under conflicting method scenarios.

Templates (_iam.njk, _locations.njk, _operations.njk):

  • Wrapped the generation of mixin methods (CJS & ESM) in conditional statements checking their corresponding mixin flags.

Tests (gapic_$service_$version.ts.njk):

  • Updated test generation templates (CJS & ESM) to conditionally generate tests for mixin methods only when those methods are actually active in the generated client.
  • Regenerated baselines

Fixes #7659 🦕

@shivanee-p shivanee-p changed the title fix: add logic to check for IAM RPC methods within the service before… fix: add logic to check for IAM RPC methods within the service before adding mixin to avoid generating duplicate methods Mar 31, 2026
… adding mixin to avoid generating duplicate methods
…e before adding mixin to avoid generating duplicate methods"

This reverts commit 5759a4f.
@shivanee-p shivanee-p force-pushed the shivaneep-iam-duplicate-mixins branch from e5d9ada to b3ee7c3 Compare March 31, 2026 23:01
@shivanee-p shivanee-p marked this pull request as ready for review April 1, 2026 00:22
@shivanee-p shivanee-p requested a review from a team as a code owner April 1, 2026 00:22
Copy link
Copy Markdown
Contributor

@sofisl sofisl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok this is awesome! A few questions/comments generally:

  1. We also support an LRO mixin. Could you make sure that's not duplicated as well? https://docs.google.com/document/d/1YAeaLHghuvEJJtcjWFv1OaMZJgkRQG-FSVqlJJN5YgI/edit?resourcekey=0-KMHBem3k-xSFVxqUQ1QV1g&tab=t.0#heading=h.ad847mq3hi3j
  2. Do we want to drill down by the actual rpc name? Like, say an API declares a mixin AND directly declares a method. Should we assume that if one rpc exists, we just give precedence to that and exclude the mixin entirely? or we do we do it function by function? Or, maybe we just say more generally, don't produce duplicative function names (and don't do it function by function). I'm not really sure what path we'd want to take, my opinion is that maybe we just say we have no duplicative functions for any mixin if it's declared, without looking specifically function by function name (like in your comparison, I'm wondering what if there's a capitalization issue or a just some slight discrpancy in the hard-coding of the function name). we should just get a list of all functions in the mixin and make sure none of these are also declared in the proto.
  3. The actual function of how you are excluding functions in the templates makes sense and looks good! I just think we want to clean up how we're deciding what to add (specifically in proto.ts file)

@shivanee-p shivanee-p requested review from a team and sofisl April 23, 2026 18:58
@shivanee-p shivanee-p added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 23, 2026
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 23, 2026
@shivanee-p shivanee-p added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 24, 2026
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 24, 2026
@shivanee-p shivanee-p added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 24, 2026
@yoshi-kokoro yoshi-kokoro removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Apr 24, 2026
@shivanee-p
Copy link
Copy Markdown
Contributor Author

  1. Do we want to drill down by the actual rpc name? Like, say an API declares a mixin AND directly declares a method. Should we assume that if one rpc exists, we just give precedence to that and exclude the mixin entirely? or we do we do it function by function? Or, maybe we just say more generally, don't produce duplicative function names (and don't do it function by function). I'm not really sure what path we'd want to take, my opinion is that maybe we just say we have no duplicative functions for any mixin if it's declared, without looking specifically function by function name (like in your comparison, I'm wondering what if there's a capitalization issue or a just some slight discrpancy in the hard-coding of the function name). we should just get a list of all functions in the mixin and make sure none of these are also declared in the proto.

Unfortunately, we need to drill down by the specific RPC name. The AIP guidelines state that if for instance only one of the get IAM method is declared within the service, the other 2 mixin methods (set() and test()) still need to be rendered using the mixin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Generator: give precendence to protos if mixin is declared

3 participants