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 6 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 6 commits intomainfrom
shivaneep-iam-duplicate-mixins

Conversation

@shivanee-p
Copy link
Copy Markdown
Contributor

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

Adds unit tests and creates baselines for testing code generation

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)

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

2 participants