chore: refactor setting of auto-populated field in request#17004
chore: refactor setting of auto-populated field in request#17004parthea wants to merge 4 commits intorepro-508597813from
Conversation
There was a problem hiding this comment.
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.
| 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())) |
There was a problem hiding this comment.
The implementation of _setup_request_id has two significant issues:
- Correctness (Dict support): The docstring states that
requestcan be adict, butsetattrandgetattrwill raise anAttributeErrorwhen called on a dictionary. - Correctness (Pure Protobuf support): For pure protobuf messages (non-proto-plus), the check
field_name not in requestwill raise aTypeErrorbecause 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()))
| 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())) |
There was a problem hiding this comment.
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()))
f2f692b to
0223a0d
Compare
This PR depends on #16944. Please review #16944 first.
This PR addresses the feedback in #16944 (comment)