fix(utils): backward-compatible protobuf field.is_repeated (#1011)#1049
fix(utils): backward-compatible protobuf field.is_repeated (#1011)#1049jacksjp wants to merge 2 commits intoa2aproject:mainfrom
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 introduces a _is_field_repeated helper function in proto_utils.py to handle the transition from the deprecated field.label to the newer field.is_repeated API across different Protobuf versions. The feedback suggests updating the helper function's docstring to use the full FieldDescriptor.LABEL_REPEATED reference for consistency with the code and the repository's idiomatic practices.
| """Return True if *field* is a repeated field. | ||
|
|
||
| Uses ``field.is_repeated`` (protobuf ≥ 4.0) when available, and falls back | ||
| to ``field.label == LABEL_REPEATED`` for older versions. |
There was a problem hiding this comment.
The docstring mentions LABEL_REPEATED, but the implementation uses FieldDescriptor.LABEL_REPEATED. It's better to be consistent with the code to avoid confusion, especially since LABEL_REPEATED is not imported into the local namespace. Accessing descriptor constants via the FieldDescriptor class is the preferred idiomatic approach in this repository.
| to ``field.label == LABEL_REPEATED`` for older versions. | |
| to field.label == FieldDescriptor.LABEL_REPEATED for older versions. |
References
- When checking for repeated fields in Protobuf, access descriptor constants via the FieldDescriptor class (e.g., FieldDescriptor.LABEL_REPEATED) rather than directly from the field instance.
🧪 Code Coverage (vs
|
| Base | PR | Delta | |
|---|---|---|---|
| src/a2a/utils/proto_utils.py | 90.87% | 97.70% | 🟢 +6.83% |
| Total | 93.01% | 93.18% | 🟢 +0.18% |
Generated by coverage-comment.yml
- Fix _is_field_repeated docstring to use FieldDescriptor.LABEL_REPEATED (consistent with implementation and repository idiom) per code review comment - Add tests for _is_field_repeated legacy fallback path (field.label) - Add tests for parse_params edge cases: unknown keys, empty repeated values, and non-string repeated values - Add test for REQUIRED has_presence message field missing validation - Add test for map entry field recursive validation - Add tests for validation_errors_to_bad_request, bad_request_to_validation_errors, and roundtrip conversion Coverage for src/a2a/utils/proto_utils.py: 85.4% -> 97.7% Closes a2aproject#1011
jacksjp
force-pushed
the
fix/proto-utils-protobuf-with-fallback
branch
from
f88baa2 to
69a8139
Compare
|
Unfortunately,
|
2 participants
Summary
Fixes #1011
FieldDescriptor.is_repeated was introduced in protobuf 4.0, while field.label was removed in protobuf 7.0. The existing code used field.label == LABEL_REPEATED everywhere, which broke with protobuf ≥ 7.0.
Changes
Added _is_field_repeated(field) helper in proto_utils.py that uses field.is_repeated when available (protobuf ≥ 4.0), falling back to field.label == LABEL_REPEATED for older versions.
Replaced all three field.label == FieldDescriptor.LABEL_REPEATED call sites in parse_params, _check_required_field_violation, and _recurse_validation with the new helper.
Added a module-level debug log indicating which API path is active at import time.
Removed the three TODO(#1011) comments that tracked this work.
Testing
Existing tests in test_proto_utils.py cover the affected code paths and continue to pass.
Compatibility
Works with protobuf < 4.0 (uses field.label)
Works with protobuf 4.x–6.x (uses field.is_repeated)
Works with protobuf ≥ 7.0 (uses field.is_repeated; field.label no longer available)