Skip to content

chore: refactor setting of auto-populated field in request#17004

Draft
parthea wants to merge 4 commits intorepro-508597813from
refactor-autopopulated-fields
Draft

chore: refactor setting of auto-populated field in request#17004
parthea wants to merge 4 commits intorepro-508597813from
refactor-autopopulated-fields

Conversation

@parthea
Copy link
Copy Markdown
Contributor

@parthea parthea commented May 8, 2026

This PR depends on #16944. Please review #16944 first.

This PR addresses the feedback in #16944 (comment)

Copy link
Copy Markdown
Contributor

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

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 auto-population of UUID4 fields by centralizing the logic into a new _setup_request_id static method within the generated clients and updating shared macros to utilize it. Feedback indicates that the current implementation of _setup_request_id lacks proper support for dictionary and pure protobuf message types, which will lead to runtime errors. Additionally, the Jinja template condition for generating this method is incorrectly defined, causing it to be included as dead code even when no fields require auto-population.

Comment on lines +466 to +471
if is_proto3_optional:
if field_name not in request:
setattr(request, field_name, str(uuid.uuid4()))
else:
if not getattr(request, field_name):
setattr(request, field_name, str(uuid.uuid4()))
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.

high

The implementation of _setup_request_id has two significant issues:

  1. Correctness (Dict support): The docstring states that request can be a dict, but setattr and getattr will raise an AttributeError when called on a dictionary.
  2. Correctness (Pure Protobuf support): For pure protobuf messages (non-proto-plus), the check field_name not in request will raise a TypeError because protobuf messages are not iterable.

To support both dictionary and message types (including pure protobuf and proto-plus), the logic should be more robust.

        if isinstance(request, dict):
            if is_proto3_optional:
                if field_name not in request:
                    request[field_name] = str(uuid.uuid4())
            elif not request.get(field_name):
                request[field_name] = str(uuid.uuid4())
            return

        if is_proto3_optional:
            try:
                # Pure protobuf messages
                if not request.HasField(field_name):
                    setattr(request, field_name, str(uuid.uuid4()))
            except (AttributeError, ValueError):
                # Proto-plus messages or other objects
                if field_name not in request:
                    setattr(request, field_name, str(uuid.uuid4()))
        else:
            if not getattr(request, field_name):
                setattr(request, field_name, str(uuid.uuid4()))

Comment on lines +240 to +245
if is_proto3_optional:
if field_name not in request:
setattr(request, field_name, str(uuid.uuid4()))
else:
if not getattr(request, field_name):
setattr(request, field_name, str(uuid.uuid4()))
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.

high

The implementation of _setup_request_id does not correctly handle the dict type mentioned in the docstring (as setattr/getattr will fail), and it will raise a TypeError for pure protobuf messages when using the in operator on the message object. The logic should be updated to handle all supported request types.

        if isinstance(request, dict):
            if is_proto3_optional:
                if field_name not in request:
                    request[field_name] = str(uuid.uuid4())
            elif not request.get(field_name):
                request[field_name] = str(uuid.uuid4())
            return

        if is_proto3_optional:
            try:
                # Pure protobuf messages
                if not request.HasField(field_name):
                    setattr(request, field_name, str(uuid.uuid4()))
            except (AttributeError, ValueError):
                # Proto-plus messages or other objects
                if field_name not in request:
                    setattr(request, field_name, str(uuid.uuid4()))
        else:
            if not getattr(request, field_name):
                setattr(request, field_name, str(uuid.uuid4()))

@parthea parthea force-pushed the refactor-autopopulated-fields branch from f2f692b to 0223a0d Compare May 8, 2026 21:20
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.

1 participant