Repository navigation
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 55 files
Shadow auto-approve: would not auto-approve because issues were found.
View guided diff | Turn on auto-fix | Re-trigger cubic
Generated wrap serializers wrote required and explicitly set nullable fields back as None even when include, exclude, exclude_unset or exclude_defaults left them out of the dump. Skip a declared field that is absent from Pydantic's output under both its API name and its Python name, unless the dump uses exclude_none, which request bodies rely on to keep sending null. Also read fields whose API name is the empty string (multipart file content) under that name: the serializer fell back to the Python name, so by-alias dumps lost the value.
9ddfe10 to
7eca0ea
Compare
Generator Snapshot Testing
Snapshot results: https://github.com/speakeasy-api/openapi-generation-snapshots/issues/136#issuecomment-6371548832 Snapshot results are summarized above. Additional run context is linked for maintainers with access. |
There was a problem hiding this comment.
0 issues found across 52 reviewed files. 1 file intentionally excluded from review.
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Fixes generated Python model serializers to honour include/exclude/exclude_unset/exclude_defaults by skipping absent fields unless exclude_none, and corrects empty-alias handling. Bounded template/generated-code bug fix with new tests; request-body dumps remain unchanged.
Tip: Get a Slack DM when cubic finishes reviewing your PRs. Connect Slack
View guided diff | Turn on auto-fix | Re-trigger cubic
Why
Every generated Python model with optional or nullable fields has a wrap
model_serializer. It rebuilds the output field by field from Pydantic's own dump and writes required fields, and nullable fields that were explicitly set, even when Pydantic left them out. A field that the caller filtered out therefore came back asNone:The same happened with
include,exclude_unsetandexclude_defaults, for required fields, const fields and nullable fields with anulldefault, including in nested models.The write-back is needed in one case: request bodies are dumped with
exclude_none=True, and must still sendnullfor required fields and explicitly set nullable fields.What changed
infoand skips a declared field that is absent from Pydantic's dump under both its API name and its Python name, unless the dump usesexclude_none. Fields that Pydantic filters out of a dump stay out, except when the dump usesexclude_none. Both spellings are checked because a nested model's serializer also runs on its own earlier output, which is keyed by API name.marshal_jsonalways dumps withexclude_none=True, where the new skip never runs, and it is the only dump the SDK runtime performs.alias="". The serializer computed the API name asf.alias or n, so it looked forcontentwhile Pydantic's by-alias output holds"". This was already broken: the value was lost and written back asNone({'content': None, 'fileName': 'a', ...}), and with the skip above the field would have disappeared entirely. The API name is nowf.alias if f.alias is not None else n, so the value is kept under""inmodel_dump(),model_dump(by_alias=True)andmarshal_json. Multipart requests are serialized from field metadata, not from these dumps, and the multipart upload tests pass unchanged.infoparameter, the two skip lines and the new API-name line;gen.lockchanges.Known limitations
exclude_none=Truewithinclude,exclude,exclude_unsetorexclude_defaultscan still write excluded required fields back asNone. The serializer cannot tell a field dropped for beingNonefrom one dropped by another filter, and request bodies rely on the former.display_namenext todisplayName).Test coverage
New
model-dump-filtersprimary fragment:FilteredFields(required renamed field, required and optional nullable fields, a defaulted field, a nullable field with anulldefault, a const field, a nested copy and an array of copies),FilteredFieldsWithAdditionalProperties, and an error response whose body nestsFilteredFields.test_model_dump_filters_additional.pycovers:include,exclude,exclude_unsetandexclude_defaults, top level and nested;exclude_nonelimitation above;model_dump()/model_dump_json()of nested models andTypeAdapter(List[...])of model lists keeping API names;nulladditional property, an additional property under a field's Python name, and a top-levelNonebody;excludehonoured by the nested model;BinaryString.content) keeping its value, and staying out when excluded.Stack
preserveUnknownFieldsmodelSerializationMethodsTesting
TARGET=primary make test-pythonv2: 696 passed (17 new), including the multipart upload tests.TARGET=secondary make test-pythonv2: 264 passed, 1 skipped.TARGET=tertiary make test-pythonv2: 214 passed, 1 skipped.TARGET=quaternary make test-pythonv2: 247 passed, 1 skipped.TARGET=review make build-pythonv2: integrity validation passes; the diff contains only theinfoparameter, the two skip lines and the API-name line in each of the 66 wrap serializers, plusgen.lock.git diff --checkpasses. No TypeScript changed.Public-safety check
.claude/skills/public-repo-communication/SKILL.md).git diff --check.