fix(api-core): use truthiness check in setup_request_id to support proto-plus messages

[hebaalazzeh]

Overview

Updates setup_request_id in google.api_core.gapic_v1.requests to use a truthiness check (if not getattr(...)) instead of an identity check (if getattr(...) is None:) for non-protobuf objects when is_proto3_optional=True.

Why is this change necessary?

• Fixes broken UUID auto-population on proto-plus messages: In generated Google Cloud Python libraries, request objects are primarily instances of proto.Message (from the proto-plus library). When an optional string field (is_proto3_optional=True) is unset, getattr(request, "request_id", None) returns the protobuf default string value: "" (empty string), not None.
• getattr(...) is None always evaluates to False: Because "" is None is False, setup_request_id silently failed to auto-populate UUIDs on all unset proto-plus messages across generated client libraries.
• Why unit tests didn't catch it: Existing unit tests used a plain Python MockRequest class where missing attributes return None, masking real-world proto.Message behavior.
• Aligns with generator compatibility layer: Using if not getattr(...) ensures unset string fields (not "" $\rightarrow$ True) are correctly populated with UUID4 tokens, restoring 100% test pass rates in downstream integration suites (showcase_v1beta1).

Summary of Changes

• google/api_core/gapic_v1/requests.py: Changed if getattr(request, field_name, None) is None: to if not getattr(request, field_name, None): in the except (AttributeError, ValueError): block for is_proto3_optional=True.
• tests/unit/gapic/test_requests.py: Removed test assertions expecting explicit empty strings ("") to be preserved without auto-population.

[gemini-code-assist]

Code Review

This pull request updates the setup_request_id function to use a truthiness check instead of an identity check (is None) when handling proto-plus messages or other objects, ensuring empty string request IDs are auto-populated with a UUID. The feedback suggests extending this truthiness check to the dictionary handling block for consistency, and updating the removed test cases to assert that empty strings are now correctly populated with a UUID rather than deleting them.

[gemini-code-assist]

While updating the fallback for proto-plus/other objects to use a truthiness check (if not getattr(...)), the dictionary handling block at the beginning of setup_request_id (which is outside this diff) was not updated. It likely still uses if request.get(field_name) is None: when is_proto3_optional is True.

To ensure consistent behavior between dictionaries and objects (especially since the corresponding dictionary test case ({"request_id": ""}, True, "") was removed), please also update the dictionary handling block to use a truthiness check:

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

[gemini-code-assist]

Instead of removing the test case (MockRequest(request_id=""), True, ""), it should be updated to expect "uuid" (i.e., (MockRequest(request_id=""), True, "uuid")) to explicitly verify that an empty string is now correctly auto-populated with a UUID when is_proto3_optional is True.

[gemini-code-assist]

Instead of removing the test case ({"request_id": ""}, True, ""), it should be updated to expect "uuid" (i.e., ({"request_id": ""}, True, "uuid")) to explicitly verify that an empty string in a dictionary is now correctly auto-populated with a UUID when is_proto3_optional is True (once the dictionary handling in setup_request_id is also updated).

[parthea]

Gemini suggested this refactor because of the following issues:

1. AIP-4235 Violation (see link below): Shifting to a truthiness check if not getattr(...) on objects with is_proto3_optional=True breaks the explicit presence contract. Any explicit empty string "" provided by a user will be silently overwritten by a generated UUID. We need to use HasField to check if the value was set to empty string by the user. We need to use HasField in the proto-plus handling also: https://protobuf.dev/programming-guides/field_presence/#using-the-generated-code

From https://google.aip.dev/client-libraries/4235#expected-generator-and-client-library-behavior,

The field must be automatically populated if and only if one of the following conditions holds:

The field supports explicit presence, and has not been set by the user

2. High Redundancy: The token generation block str(uuid.uuid4()) is duplicated 5 separate times throughout the function.

3. Overly Complex Branching: Dictionaries, pure protobufs, proto-plus messages, and custom objects are routed through separate, deeply nested conditional blocks, which makes maintenance error-prone.

import uuid
from typing import Any, Union

def setup_request_id(
    request: Union[Any, dict, None],
    field_name: str,
    is_proto3_optional: bool,
) -> None:
    """Populate a UUID4 field in the request if it is not already set.

    Ensures request idempotency by automatically generating a unique
    identifier (such as `request_id`) for requests supporting it.
    """
    if request is None:
        return

    # 1. Evaluate whether the field is considered "unset" and needs population
    should_populate = False

    if isinstance(request, dict):
        if is_proto3_optional:
            # AIP-4235: Only populate if completely missing or strictly None
            should_populate = field_name not in request or request[field_name] is None
        else:
            # Populate if the field is missing or falsy (e.g. empty string)
            should_populate = not request.get(field_name)
    else:
        # Check for proto-plus wrapper (which has an underlying ._pb message)
        is_proto_plus = hasattr(request, "_pb") and hasattr(request._pb, "HasField")

        if is_proto3_optional:
            if is_proto_plus:
                try:
                    # Ask the underlying protobuf if the field has explicit presence
                    should_populate = not request._pb.HasField(field_name)
                except ValueError:
                    # Fallback for non-optional fields or non-presence primitives
                    should_populate = getattr(request, field_name, None) is None
            else:
                try:
                    # Pure protobuf messages
                    should_populate = not request.HasField(field_name)
                except (AttributeError, ValueError):
                    # Standard Python objects / Mock requests: Only populate if strictly None
                    should_populate = getattr(request, field_name, None) is None
        else:
            # If not proto3 optional, populate on any falsy value (None or empty string)
            should_populate = not getattr(request, field_name, None)

    # 2. Consolidate mutation to a single, clean DRY block
    if should_populate:
        generated_id = str(uuid.uuid4())
        if isinstance(request, dict):
            request[field_name] = generated_id
        else:
            setattr(request, field_name, generated_id)

[parthea]

Since is_proto3_optional is True, we may still need this test case to follow the AIP

From https://google.aip.dev/client-libraries/4235#expected-generator-and-client-library-behavior,

The field must be automatically populated if and only if one of the following conditions holds:

The field supports explicit presence, and has not been set by the user

[daniel-sanche]

do we have to lose this typing? Can it really be anything?

[hebaalazzeh]

In Python, proto-plus message classes (proto.Message) are wrappers around underlying protobuf messages (._pb) and do not inherit from google.protobuf.message.Message.

Because of this, if we keep request: Union[google.protobuf.message.Message, dict, None], static type checkers (like mypy) will report type incompatibilities whenever a proto-plus request object is passed into setup_request_id.

Using Any here allows the function to accept:

1. proto-plus message wrappers (proto.Message)
2. Pure protobuf messages (google.protobuf.message.Message)
3. Dictionaries (dict)
4. Custom/Mock request objects

This avoids needing a hard runtime dependency/import on proto-plus just for type hinting while ensuring static type checkers don't fail when proto-plus requests are passed.

[daniel-sanche]

Can you explain this field a bit more? I'm not sure what exactly this means in this context, but it seems important

[daniel-sanche]

Gemini suggested this as a docstring, does it seem accurate?

       is_proto3_optional (bool): Whether the field is declared as `optional`
            in the proto schema (`proto3 optional`). Enforces proto presence
            semantics across message objects and dictionaries:
            - If True, explicit empty strings ("") are preserved and only unset
              fields (or missing/None dict keys) are auto-populated.
            - If False, any empty or unset string is replaced with a generated UUID.

(In hingsight, I wish we gave this a better name, like "preserve_empty_strings". But probably not worth the potential breaking change)

[hebaalazzeh]

In protobuf 3 (proto3), fields originally did not support explicit presence—there was no way to distinguish whether a user explicitly set a field to its default value (like "") or left it unset.

When a field is marked with optional in proto3 (optional string request_id = 2;), it enables explicit presence tracking. In GAPIC and api-core, is_proto3_optional indicates whether the target field (field_name) was defined with explicit presence in the proto schema.

Why this is important for setup_request_id (AIP-4235 Compliance):
According to AIP-4235 (Idempotency / Request ID):

"The field must be automatically populated if and only if one of the following conditions holds:
The field supports explicit presence, and has not been set by the user."

When is_proto3_optional = True:

• Unset field (user never passed request_id): We check not request._pb.HasField(field_name) $\rightarrow$ auto-populate a UUID.
• Explicitly set to empty string (user passed request_id=""): HasField returns True $\rightarrow$ preserve the user's explicit empty string "" (do NOT overwrite with a UUID).

When is_proto3_optional = False (no explicit presence):

• We fall back to a truthiness check (not getattr(...) / not request.get(...)), where any falsy value (None or "") is treated as unset and auto-populated with a UUID.

[daniel-sanche]

more comments would be helpful here. There are a lot of nested cases, it's hard to follow

Maybe this should even be broken into multiple helpers

[hebaalazzeh]

added more comments!

[daniel-sanche]

nit: it looks like is_proto_plus already implies pb_msg is not None

[hebaalazzeh]

Done. Resolved by unwrapping the underlying _pb directly with getattr(request, "_pb", request) and removing the redundant intermediate checks.

[daniel-sanche]

proto-plus objects contain a pure-proto instance, so case 2a and 2b seem redundant. Looking at the code, it seems to be the same logic, just using different variables? Can we just unwrap the proto-plus object, and use the same parsing logic?

Something like:

if is_proto3_optional:
      # extract the protobuf from proto-plus if wrapped
      pure_pb: google.protobuf.message.Message = getattr(request, "_pb", request)
    try:
        should_populate = not pure_pb.HasField(field_name)
    except ValueError:
        should_populate = getattr(pure_pb, field_name, None) is None
else:
    should_populate = not getattr(request, field_name, None)

[hebaalazzeh]

Done. Updated to unwrap pure_pb: google.protobuf.message.Message = getattr(request, "_pb", request) and consolidated the parsing logic into a single presence-checking block.

[daniel-sanche]

nit: this may be more readable if cast to bool:

should_populate = not bool(getattr(request, field_name, False))

[hebaalazzeh]

Done. Cast to bool as suggested.

[daniel-sanche]

I'd leave out the meta-commentary about this being a DRY block, and focus on what the block is supposed to accomplish.

Maybe something like:
# If the field was found to be empty, set random id

[hebaalazzeh]

Done. Updated the comment to # If the field was found to be empty, set random id.

[daniel-sanche]

probably no need to mention mock/dict-like objects, since those aren't covered by our type annotations, so we don't formally support them

[hebaalazzeh]

Done. Updated the comment to # Case 2: Object request (proto-plus wrapper or pure protobuf message).

[daniel-sanche]

nit: have you considered using magic mock to test some of these? Custom classes with getattr, setattr and hasattr implementations shouldn't be needed

[hebaalazzeh]

Removed all unnecessary contains implementations from the mock classes. Kept the lightweight class structures so attribute access and HasField presence behavior remain explicit and deterministic without MagicMock auto-generating truthy attributes.

[daniel-sanche]

very small nit: it feels a bit unbalanced that we have Case 1a and 1b, but then Case 2 and 2b

(also, "Proto request" feels more descriptive than "Object request")