Repository navigation
feat(python): add forward-compatible model serialization - #133
Draft
ThomasRooney wants to merge 22 commits into
Draft
ThomasRooney wants to merge 22 commits into
ThomasRooney wants to merge 22 commits into
Conversation
Generator Snapshot Testing
Snapshot results: https://github.com/speakeasy-api/openapi-generation-snapshots/issues/128#issuecomment-6357571860 Snapshot results are summarized above. Additional run context is linked for maintainers with access. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Get a Slack DM when cubic finishes reviewing your PRs. Connect Slack
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
No issues found across 28 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Adds an opt-in Python generator flag (default false) so opted-in models retain undeclared fields through validation and serialization; default output is unchanged and new echo-test variants/CI cover the enabled and disabled paths.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 2 unresolved issues from previous reviews.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 2 unresolved issues from previous reviews.
View guided diff | Turn on auto-fix | Re-trigger cubic
ThomasRooney
changed the title
There was a problem hiding this comment.
0 issues found across 14 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 2 unresolved issues from previous reviews.
View guided diff | Turn on auto-fix | Re-trigger cubic
Exercise all four model serialization configurations in CI.
There was a problem hiding this comment.
0 issues found across 14 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 2 unresolved issues from previous reviews.
View guided diff | Turn on auto-fix | Re-trigger cubic
2ynn
self-requested a review
October 22, 2026 17:27
2ynn
reviewed
Oct 22, 2026
2ynn
marked this pull request as draft
October 22, 2026 22:53
Contributor
|
@ThomasRooney The commits I pushed solve the following issues: With both options off
Tests and CI
Each commit stands alone, so any of them can be dropped. Known limitations
|
There was a problem hiding this comment.
All reported issues were addressed across 37 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
View guided diff | Turn on auto-fix | Re-trigger cubic
Const fields, including union discriminators, are populated by their default, so the exclude_unset default of to_dict and to_json dropped them. A discriminated union value then failed re-validation, and a const beside optional fields was emitted as None. When modelSerializationMethods is enabled, models with const fields now mark them as set after initialisation, so the helpers emit the same consts as the request payload. Optional consts whose value is null are not marked, so they stay out of request payloads and helper output unless the caller sets them.
Enabling modelSerializationMethods appended a trailing underscore to every name matching a helper, not only to model fields. A toJson operation group became sdk.to_json_ in code and usage snippets, and globals or server variables named like a helper were renamed in SDK constructor arguments. Sub-SDK classes and the SDK constructor have no helper methods, so only model fields, including those of the generated globals model, keep the trailing underscore. Usage snippets name globals with the same constructor argument names.
The generated model serializer always wrote declared fields under their API names, so to_dict(use_api_names=False) and to_json(use_api_names=False) returned API names for any model with optional or nullable fields. When modelSerializationMethods is enabled, the serializer now writes field names when by_alias is explicitly False. model_dump() without by_alias, by_alias=True and request serialization are unchanged.
main installs the same patched PHP in the test-generator job, so this branch no longer needs its own copy of the step.
With preserveUnknownFields, Unset inherited extra="allow", so an invalid dict on an OptionalNullable field validated to Unset(...) carrying that dict and no longer compared equal to UNSET.
The generated model serializer looked a field up under its API name,
fell back to its Python name, and removed both keys. With
preserveUnknownFields or modelSerializationMethods enabled this:
- dropped an unknown field named like a declared field's Python name,
because serialized.pop(n) ran as the eager default of
serialized.pop(k);
- took that unknown field as the declared field's value, written under
the API name, when exclude_none or exclude_defaults had dropped the
declared field;
- wrote None for required fields Pydantic excluded because they held
their default, so to_dict(exclude_defaults=True) emitted
{"kind": None} for consts;
- returned None for nullable fields from to_dict(exclude_none=True) and
to_json(exclude_none=True), because the serializer puts explicit nulls
back after Pydantic drops them for request serialization.
When either option is enabled, the serializer now receives the
serialization info and reads each field only from the key the dump
used, by_alias deciding between the API and Python name. With
modelSerializationMethods, fields dropped by exclude_defaults stay out,
and to_dict and to_json dump with a serialization context so fields
dropped by exclude_none stay out too. Request payloads still send
explicit nulls.
With preserveUnknownFields, response models keep fields the server returns but the spec does not declare, and model equality includes them. Generated tests compared whole response models with ==, so they failed whenever the server sent such fields. Compare declared fields exactly and undeclared fields only where the expected value has them. Additional properties of models that declare additionalProperties are still compared exactly. The helper ships in tests/unknown_fields.py, generated whenever tests are generated with preserveUnknownFields.
Run the unknown-field cases from a shared uber fragment instead of four dedicated variants with their own spec: - secondary enables preserveUnknownFields and keeps whole request bodies, so top-level unknown fields can be sent and echoed. - tertiary enables modelSerializationMethods. - quaternary enables both with flattened requests, covering to_dict and to_json on responses that carry unknown fields. Every other variant keeps both options off. The four unknown-fields variants, their CI jobs and the separate spec are removed. Generated models holding unrecognized enum values are also dumped with warnings raised as errors, not only a bare TypeAdapter. The echo operation is excluded from javav2, whose generated union members repeat the same interface when the parent schema has input and output variants. The discriminator enums of the fragment are named schemas, so Go does not give the NumberChoice kind the type name Kind used by an existing enum.
Share the unknown-field cases between variants, each paired with the fields left when unknown fields are dropped: - primary (both options off) checks unknown fields are dropped from models and request bodies. - secondary (preserveUnknownFields) echoes them as before. - tertiary (modelSerializationMethods) checks to_dict and to_json drop unknown fields, keep consts, reserve helper names only on models and honour exclude_none. - quaternary (both) round trips every case through to_dict and to_json, compares them with literal expected values instead of model_dump with the same options, sends flattened requests asynchronously, and covers unknown fields named like an excluded field and additional properties in the assertion helper. The serialization-helper group is excluded from ruby, where its accessor would shadow Object#to_json.
The model serializer put back every field the dump had excluded. As a result, to_dict() returned unset nullable fields whose default is null, although it leaves every other unset defaulted field out, and dumps with exclude_unset, include or exclude wrote None for the fields they excluded. Only request serialization needs fields put back: it dumps with exclude_none and still sends explicit nulls on nullable fields. When modelSerializationMethods is enabled, excluded fields now stay out of every dump except exclude_none dumps outside to_dict and to_json.
The JSON serializer of UnrecognizedStr and UnrecognizedInt accepted any value. While serializing a union, Pydantic can try it for values that validated as another member, so an open enum followed by a number or boolean branch sent 1.5 as 1 and true as 1. This applied to every SDK with such a union, with or without the new model options. Each serializer now declines values that are not its own type, so the union serializes them with the matching member. Unrecognized enum values are still returned as UnrecognizedStr and UnrecognizedInt in Python mode and as plain strings and integers in JSON. The test fragment declares its enums as named schemas. Inline, the integer enum generated a type named One in other targets, replacing the existing One model.
Flattened operation arguments only accept declared fields, so undeclared top-level request fields need flattenRequests: false. Nested models passed as arguments keep their undeclared fields. State this in the option description. Note at the model serializer that Pydantic writes extras after declared fields, so an extra under a field's emitted key replaces that field's value. The template comment is not emitted into generated code.
The tertiary TypeScript configuration combines flat responses, no index modules and the legacy naming scheme. With that combination, an operation whose success and error bodies are both inline gives both the same generated name, and generation fails with a duplicate import. Remove echoUnknownFields only from the tertiary TypeScript spec, so the other TypeScript variants and every other target keep the operation. Delete this overlay once the generator names the two bodies apart.
2ynn
force-pushed
the
thomas/python-preserve-unknown-fields
branch
from
October 23, 2026 04:31
deff206 to
c904549
Compare
There was a problem hiding this comment.
0 issues found across 4 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would require human review. Adds opt-in Python serialization/unknown-field preservation with tests and an open-enum serializer fix; requires human review because the dismissal of the model_post_init collision claim relies on a reserved-keyword list not visible in the diff.
View guided diff | Turn on auto-fix | Re-trigger cubic
4 tasks done
Contributor
|
@ThomasRooney I split the work into a stack of four PRs for easier review:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Python SDKs need undeclared fields to survive nested JSON request and response serialization, and model helpers must serialize future enum values without a downstream base-model patch.
What changed
python.preserveUnknownFields(defaultfalse) to retain undeclared fields in generated models and error bodies. Explicitly typed additional properties still validate; the option deliberately permits extras on closed schemas.python.modelSerializationMethods(defaultfalse) forto_dictandto_jsonhelpers, including field aliases and Pydantic filtering options. Reserve the helper names only when enabled.Configuration schema: speakeasy-api/sdk-gen-config#136
Testing
git diff --checkpasses.Public-safety check
git diff --check.