fix(server): improve Swagger /docs example annotations for A2A routes#1092
fix(server): improve Swagger /docs example annotations for A2A routes#1092martimfasantos wants to merge 6 commits into
Conversation
- Use add_a2a_routes_to_fastapi() in the sample instead of app.routes.extend() - Annotate enum fields with the first non-UNSPECIFIED value as example - Default string fields to "" (proto3 unset) and REQUIRED strings to field.name, driven by google.api.field_behavior annotations - Default boolean fields to false to avoid return_immediately: true - Wrap non-REQUIRED message fields as oneOf [$ref, null] with example: null so Swagger doesn't pre-fill optional configs that contain required sub-fields - Add array-level example by propagating the item example, preventing Swagger from generating one array entry per oneOf branch - Add top-level example to oneOf schemas using only the first variant
![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 refactors FastAPI route registration in the hello world agent sample and enhances OpenAPI/Swagger schema generation for Protobuf messages in _proto_schema.py by adding support for field examples, nullable optional message fields, and oneof variant examples. It also adds corresponding unit tests. Feedback on the changes highlights two issues: first, a bug where optional repeated message fields return early with a nullable schema instead of being wrapped in an array schema, along with a recommendation to use FieldDescriptor.LABEL_REPEATED instead of field.is_repeated; second, a potential type constraint violation when generating examples for oneof fields by assuming the first field is always a string, which should instead be dynamically determined based on the field's actual type.
🧪 Code Coverage (vs
|
| Base | PR | Delta | |
|---|---|---|---|
| src/a2a/server/routes/_proto_schema.py | 94.23% | 95.83% | 🟢 +1.60% |
| Total | 93.07% | 93.10% | 🟢 +0.02% |
Generated by coverage-comment.yml
…dynamic oneof example - Guard nullable wrapping with `not field.is_repeated` so repeated non-REQUIRED message fields fall through to array wrapping instead of returning a nullable oneOf schema - Derive the oneof variant example dynamically from the field's schema rather than assuming the first field is always a string type - Add test covering repeated optional message fields are wrapped as arrays
Add tests for the non-string-scalar and message-typed first-oneof-variant paths in message_schema's example generation. No real A2A proto exercises these branches (Part's first variant is a string), so they were the source of the coverage drop flagged on the PR.
|
|
||
| from typing import Any | ||
|
|
||
| from google.api import field_behavior_pb2 as _fb |
There was a problem hiding this comment.
nit: I'd just do as fb:
| from google.api import field_behavior_pb2 as _fb | |
| from google.api import field_behavior_pb2 as fb |
The entire _proto_schema.py is already marked as internal and also it's not an established practice in the repo.
| assert 'oneOf' not in schema | ||
|
|
||
|
|
||
| def test_field_schema_repeated_optional_message_is_array_not_nullable(): |
There was a problem hiding this comment.
Can we use Task.history here instead of mocking? Based on the test description it should suit it: repeated and non-required.
| except KeyError: | ||
| return False |
There was a problem hiding this comment.
I feel like this is redundant as field_behavior is repeated so Extensions[field_behavior] should just give an empty list when it's not there. However dropping this fails tests, which I believe are not real failures as they use mocks.
Let's try to avoid mocks for such tests, if we don't have a certain structure across current protobufs I'd just skip testing this. We should just make sure we invoke building the entire schema in tests so that in case of changes we don't support it fails right there.
2 participants
Summary
add_a2a_routes_to_fastapi()in the sample instead ofapp.routes.extend()— the feature introduced in feat(server): restore FastAPI /docs visibility for A2A routes #1024 was not reflected in the sample_proto_schema.py), driven bygoogle.api.field_behaviorproto annotations and proto type information rather than hardcoded field-name listsSchema example improvements
"user") causing-32602UNSPECIFIEDvalue (e.g.ROLE_USER)"string"causing task-lookup failures ontask_id""(proto3 empty = field unset)REQUIRED"string"causing validation failures onmessage_idfield.name(non-empty, self-describing)truecausingreturn_immediately: true→SUBMITTEDfalseREQUIREDoneOf: [$ref, null]withexample: nullREQUIRED$ref(not nullable)oneOfbranch[item_example](single pre-filled entry)Optionality is derived from
google.api.field_behavior = REQUIREDannotations already present on the A2A proto fields — no hardcoded field-name exceptions.Checklist
CONTRIBUTINGGuideruff formatandruff checkcleanContinues #1024 🦕