fix(api-core): address PR review comments for requests.py and test_requests - #17795
fix(api-core): address PR review comments for requests.py and test_requests#17795hebaalazzeh wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies setup_request_id to check if a field is explicitly None rather than falsy (such as an empty string) when handling proto-plus or other objects, preventing an empty string from being overwritten by a new UUID. Corresponding test cases for empty strings were added, unused mock methods were removed, and re.match was updated to re.fullmatch in the tests. The reviewer suggests extending the test coverage by adding a test case for MockProtoRequest with an empty string to ensure standard protobuf messages are also verified.
| # MockRequest cases | ||
| (MockRequest(), True, "uuid"), | ||
| (MockRequest(request_id="already_set"), True, "already_set"), | ||
| (MockRequest(request_id=""), True, ""), |
There was a problem hiding this comment.
While adding a test case for MockRequest with an explicit empty string when is_proto3_optional=True is great, we should also add a corresponding test case for MockProtoRequest to ensure standard protobuf messages (which implement HasField) are also covered and behave correctly under this scenario. Please consider adding (MockProtoRequest(request_id=""), True, "") and the corresponding ID "proto3_optional_explicit_empty_proto" to the ids list.
1 participant
Description
This PR addresses code review feedback regarding edge cases and test coverage in
requests.pyandtest_requests.py:requests.py: Updated fallback logic for proto-plus messages/other objects whenis_proto3_optional=Truefromif not getattr(...)toif getattr(...) is None. This distinguishes missing fields from fields explicitly set to empty values (e.g.,""), matching the dictionary handling behavior.test_requests.py: Removed the unused__contains__mock helper methods fromMockRequestandMockValueErrorRequestsince the helper uses theinoperator only for dictionaries.test_requests.py: Added parameterized test cases foris_proto3_optional=Truewith explicit empty string values""to verify they are preserved and not overwritten.test_requests.py: Changed test assertions to usere.fullmatchinstead ofre.matchto guarantee strict UUID format validation.