Skip to content

chore(generator): delegate request-id setup to public request#17739

Open
hebaalazzeh wants to merge 51 commits into
mainfrom
feat/gapic-generator-centralization-request-id
Open

chore(generator): delegate request-id setup to public request#17739
hebaalazzeh wants to merge 51 commits into
mainfrom
feat/gapic-generator-centralization-request-id

Conversation

@hebaalazzeh

@hebaalazzeh hebaalazzeh commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Updates generator templates and goldens to import public requests from google-api-core gapic_v1 and call setup_request_id helper. Removes duplicate setup_request_id test logic from generated client unit tests.

Temporarily pointing to this PR then will revert once it is merged/released:

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the request ID setup logic in the generated client templates by delegating it to google.api_core.gapic_v1.method_helpers.setup_request_id instead of maintaining an inline implementation. As a result, the redundant unit tests for _setup_request_id have been removed from both the templates and the golden integration tests. There are no review comments, so I have no feedback to provide.

@hebaalazzeh
hebaalazzeh changed the base branch from main to feat/gapic-centralization-api-core-request-id July 16, 2026 15:20
@hebaalazzeh hebaalazzeh self-assigned this Jul 16, 2026
@hebaalazzeh hebaalazzeh reopened this Jul 16, 2026
@hebaalazzeh
hebaalazzeh force-pushed the feat/gapic-generator-centralization-request-id branch from f037044 to f25d768 Compare July 16, 2026 16:10
@hebaalazzeh
hebaalazzeh force-pushed the feat/gapic-centralization-api-core-request-id branch from a0bff77 to 613b392 Compare July 16, 2026 16:18
@hebaalazzeh
hebaalazzeh force-pushed the feat/gapic-generator-centralization-request-id branch from f25d768 to 71b2f4e Compare July 16, 2026 16:20
@hebaalazzeh
hebaalazzeh force-pushed the feat/gapic-centralization-api-core-request-id branch 3 times, most recently from e02ee4b to db040bb Compare July 16, 2026 16:28
@daniel-sanche daniel-sanche added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 16, 2026
@hebaalazzeh
hebaalazzeh force-pushed the feat/gapic-generator-centralization-request-id branch from 71b2f4e to a8cc8d8 Compare July 16, 2026 18:11
@daniel-sanche

Copy link
Copy Markdown
Contributor

This implementation makes sense if we plan to bump up the api_core requirement Before merging, we need to:

  1. get the changes in feat(api_core): add request-id auto-population logic to gapic_v1 public helpers #17738 released
  2. update the setup.py to reference the new api_core version floor
  3. regenerate goldens

There's also a chance we will want to keep a fallback implementation to keep supporting old api_core versions

@hebaalazzeh hebaalazzeh changed the title feat: delegate request-id setup to public method_helpers chore(generator): delegate request-id setup to public request Jul 16, 2026
…lic helpers

Introduces method_helpers module containing setup_request_id helper. Exposes the module as public in gapic_v1.
@hebaalazzeh
hebaalazzeh force-pushed the feat/gapic-centralization-api-core-request-id branch from 90602cc to 76ac456 Compare July 16, 2026 23:21
…lic helpers

Introduces method_helpers module containing setup_request_id helper. Exposes the module as public in gapic_v1.
Updates generator templates and goldens to import public method_helpers from google-api-core gapic_v1 and call setup_request_id helper. Removes duplicate setup_request_id test logic from generated client unit tests.
@hebaalazzeh
hebaalazzeh force-pushed the feat/gapic-centralization-api-core-request-id branch from 8e31d83 to 20adea6 Compare July 16, 2026 23:42
@hebaalazzeh
hebaalazzeh force-pushed the feat/gapic-generator-centralization-request-id branch from 20e4bea to a8105ff Compare July 21, 2026 20:23
@hebaalazzeh
hebaalazzeh force-pushed the feat/gapic-generator-centralization-request-id branch from 4659080 to e243bbd Compare July 21, 2026 20:38
Comment thread packages/gapic-generator/gapic/generator/generator.py Outdated
@hebaalazzeh
hebaalazzeh force-pushed the feat/gapic-generator-centralization-request-id branch from cd6b66e to e052b61 Compare July 21, 2026 22:16
@hebaalazzeh
hebaalazzeh force-pushed the feat/gapic-generator-centralization-request-id branch from 57a8810 to 6f07478 Compare July 21, 2026 23:13
@hebaalazzeh

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new compatibility module template _compat.py.j2 to provide fallbacks for older versions of google-api-core, and refactors the generator, client templates, and tests to use it. The review feedback highlights three main issues: a missing test__compat.py.j2 template for the newly added golden test file, an optimization opportunity in _compat.py.j2 to check if request is None before generating a UUID, and a mismatch between the template and the generated golden file where a less precise check in the golden file could incorrectly overwrite valid falsy values.

Comment thread packages/gapic-generator/gapic/generator/generator.py
Comment thread packages/gapic-generator/gapic/generator/generator.py Outdated
{% if method_settings and method_settings.auto_populated_fields %}
{% set has_auto_populated_fields.value = true %}
{% endif %}
{% endfor %}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think I commented this on a different PR, but can you clarify what has_auto_populated_fields is doing? Both for me, and as a comment for future maintainers

Comment thread packages/gapic-generator/tests/unit/generator/test_generator.py Outdated
"grpcio-tools",
)
session.install("-e", ".")
session.install("-e", "../google-api-core")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is probably fine, but is this required? Shouldn't the tests pass against the released api_core too?

Note that it will still use a pinned version on 3.10 (in line below this)

get_universe_domain,
)
except ImportError:
# TODO(https://github.com/googleapis/google-cloud-python/issues/17813): Remove these fallbacks when google-api-core >= 2.18.0 is the minimum required version.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The current api-core version is 2.33.0, so I assume these versions are incorrect?

# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
"""A compatibility module for older versions of google-api-core."""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a reason we're not seeing this file generated for all goldens?

extras = "[async_rest]"

session.install("-e", f"{tmp_dir}{extras}", "-r", constraints_path)
session.install("-e", f"{tmp_dir}{extras}", "--no-build-isolation", "-r", constraints_path)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why was --no-build-isolation included?

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

Labels

do not merge Indicates a pull request not ready for merge, due to either quality or timing.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants