Generate Pydantic models from Destinations API spec, validate on release - #1225
Conversation
26efe67 to
5caafeb
Compare
- Generate Pydantic models from the Destinations API OpenAPI spec (planet/api_models/destinations.py) - Use Destination/DestinationsResponse models as return types in DestinationsClient - Add pre-release model validation in tests/drift/validate_models.py - Add nox sessions: generate_models, validate_models - Wire validate_models into the publish-pypi CI workflow - Scope everything to the Destinations API for now; other APIs noted as TODO
5caafeb to
25fd5f0
Compare
The generated models set extra='forbid', so any field Planet added to the Destinations API turned a working call into a ValidationError in a shipped SDK. Responses now allow unknown fields and preserve them through model_dump, so the CLI reports what the API returned rather than a filtered copy. The drift check could never pass: the committed models had been reformatted with yapf after generation, so a byte-comparison against fresh codegen output always failed. It also could not run at all -- tests/conftest.py imports respx and setup.cfg injects --cov, neither present in the validate_models extra. Since this gates PyPI releases, both were release blockers. Model shape is now controlled entirely by codegen flags rather than by editing generated files. --strict-nullable matters most: the spec is OpenAPI 3.0.3 and marks Destination.archived as required and nullable, and codegen was silently dropping the nullability. --target-python-version and an exact codegen pin make output reproducible, so a tool release or a different interpreter cannot fail the gate. The command line lives in one module imported by both the nox session and the drift test, which previously duplicated it and could drift apart. Also fixes a yapf exclude that matched nothing (a bare directory is not an fnmatch for files inside it, which is how the generated file got reformatted in the first place), a dead None-check that silently dropped `default unset` output, and six sync docstrings still promising dict. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Hey @asonnenschein, let me know if you want to have a sync call on this to discuss approach. |
|
Hi Regan, This would be a breaking change for the Destinations clients but it raises the issue of untyped dicts (throughout the API) as responses... This is a long-standing critique of mine that also includes a lack of a "request" object (that could be used by both sync/async dispatch, rather than duplicating all request params and docs for each client) and a broader more/useful "response" object, that for example, allows reading raw JSON or as a typed object. This would also support non-body content, such as rate-limiting headers, etc. I've suggested we could migrate to this approach, without breaking existing clients, by adding a new dispatcher type and defining request/response objects that could be reused under the hood by existing client implementations. One discussion we had was "is there value in just having users generate clients from openapi specializations?". I think there's something to consider there (as it allows other language users to build on our APIs) unless we provide some value/convenience over generated code. Finally, while there's certainly value in having a stronger typed response, I think validation of requests is more valuable than validation of responses - our services should ensure the contract and validity of their responses. |
|
I agree with @ischneider's point about this being a breaking change, but otherwise I think this is a big step in the right direction! Perhaps we can refactor the approach here so that the default response objects are still JSON, and typed response object are opt-in? |
There was a problem hiding this comment.
🟡 Changes recommended
Critical packaging omissions could make released wheels fail to import, and CLI tests do not verify model serialization.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds generated Pydantic models for the Destinations API, typed client responses, CLI serialization, and release-time model drift validation.
Changes:
- Adds generated models and Nox regeneration/validation sessions.
- Updates clients, CLI, tests, and upgrade documentation.
- Adds dependencies and release workflow validation.
File summaries
| File | Summary | Findings |
|---|---|---|
tests/integration/test_destinations_cli.py |
Updates CLI fixtures and assertions. | Moderate (1 vote): Add parsed-output assertions covering _links, pl:ref, serialization, and defaults. |
tests/integration/test_destinations_api.py |
Verifies typed API responses. | — |
tests/drift/validate_models.py |
Detects model drift against the live spec. | — |
tests/drift/codegen_config.py |
Defines shared code-generation settings. | — |
pyproject.toml |
Adds Pydantic and validation dependencies. | Critical (1 vote): The package list omits planet.api_models, so published wheels omit the models and imports fail. |
planet/sync/destinations.py |
Adds typed synchronous responses. | — |
planet/clients/destinations.py |
Validates asynchronous responses into models. | Critical (2 votes): The new planet.api_models package is omitted from the built package, causing installed imports to fail. |
planet/cli/destinations.py |
Serializes Pydantic responses. | — |
planet/api_models/destinations.py |
Adds generated Destination models. | — |
planet/api_models/__init__.py |
Marks the models directory as a package. | — |
noxfile.py |
Adds generation and validation sessions. | — |
docs/get-started/upgrading-v3.md |
Documents the response-model API change. | — |
.github/workflows/publish-pypi.yml |
Validates models before release. | — |
Review details
Suppressed comments (1)
planet/cli/destinations.py:58
- The destination CLI tests only assert
exit_code, so they do not verify the new model-to-JSON contract introduced here. A regression could emit Python field names (field_links,pl_ref), fail to serialize datetimes/enums, or change default-field handling while all these tests still pass; assert representative parsed output (including_linksandpl:ref) for at least the list/get path and one mutation.
echo_json(
response.model_dump(mode='json',
by_alias=True,
exclude_unset=True),
- Files reviewed: 12/13 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Clients return raw dict (matches pattern of all other API clients). Pydantic models remain available in planet.api_models for callers that want typed responses via Destination.model_validate(...). - CLI drops model_dump(); passes the dict directly to echo_json. - Add planet.api_models to setup.cfg packages list so the module is included in built wheels. - Fix DestinationPatchRequest1/2/3 naming: strip the pure-constraint anyOf block from the spec before codegen so the generator emits a single flat DestinationPatchRequest instead of numbered variants. - Codegen pipeline now fetches and patches the spec in-process rather than passing --url directly; both nox sessions and the drift test use the same fetch_and_patch_spec() helper.
Our changes unnecessarily altered Dict -> Dict[str, Any] annotations and rewrote docstrings that were already correct on the branch. Restore both files to match main exactly, keeping only the Pydantic import removal.
Clients now return plain dicts; the upgrading guide section describing attribute access and model_dump() no longer applies.
|
Hi @ischneider, @asonnenschein. I have reverted changes to the Destinations client code. The changes are now backwards compatible for existing users, but the models are still available for users who would like to validate against them ( via |
Requests and responses want opposite handling of unknown fields, so the global `--extra-fields allow` was wrong for half the models. Codegen now runs without that override and honours the spec, so the schemas marked `additionalProperties: false` generate as `extra='forbid'`. A typo'd key now fails client side instead of on the round trip. Response schemas need the opposite. A shipped SDK must not raise when Planet adds a field, so every schema reachable from a response body is patched to `additionalProperties: true` before codegen. Reachability is computed from the spec, not hardcoded, so it carries to other APIs. The two sets overlap: AmazonS3Params and its siblings are echoed back in responses, so tolerance wins and they stay `allow`. Only the *PatchParams and the top-level request bodies are request-only, and those get `forbid`. Model validation moves out of the release workflow and into test.yml, so drift is caught on the PR that causes it rather than gating a release on a live API call.
|
Can we change the models import path from |
…n constants Models move from planet.api_models to planet.types. pydantic moves out of the core dependency list into a `models` extra, so `pip install planet` no longer pulls in pydantic-core and its compiled Rust extension. Typed models are `pip install planet[models]`. Nothing outside planet/types imports pydantic -- clients still return plain dicts -- and a unit test walks the AST of every module under planet/ to keep it that way. planet/types/__init__.py raises a pointed ImportError naming the extra rather than letting a bare ModuleNotFoundError surface. The codegen script moves from tests/drift/ to scripts/, since it generates production code rather than test fixtures. Spec URLs, output paths, the generated-file header and the codegen target version split out into scripts/codegen_constants.py; adding an API is now one line there. The drift test reaches the script through a conftest that puts scripts/ on sys.path. The validate_models extra folds into dev. Regenerated output is byte-identical: the refactor changed no models.
| """Pre-release drift detection: regenerate Pydantic models and diff against committed files. | ||
|
|
||
| How it works: | ||
| - datamodel-codegen fetches the live OpenAPI spec and generates models into a temp file. |
There was a problem hiding this comment.
datamodel-codegen fetches the live OpenAPI spec ...
Is this still true? IIRC fetch_and_patch_spec fetches the spec and codegen reads it from a temp file.
There was a problem hiding this comment.
Not anymore. Updated: the spec is fetched and patched into a temp file, and codegen reads that file.
|
Note - this MR doesn't include a |
| import json | ||
| import tempfile |
There was a problem hiding this comment.
These imports should be at the top of the file.
Rename scripts/codegen_config.py to scripts/type_gen.py, scripts/codegen_constants.py to scripts/constants.py, and tests/unit/test_api_models.py to tests/unit/test_types.py. Hoist noxfile imports, use flake8 --extend-exclude, pin the codegen sessions to Python 3.12, and use MODELS_DIR for output paths.
asonnenschein
left a comment
asonnenschein
left a comment
There was a problem hiding this comment.
LGTM! I think we've given this MR appropriate technical due diligence, great job @Regan-Koopmans!
I have introduced typing for the Destination API client methods using Pydantic models. I generated these models from the live Destinations API spec using
datamodel-code-generator. I have added a helpfulnoxcommand to conveniently regenerate these models going forward:Having this in place means that we can validate our models against the production API. I have added a
noxcommand to do just this:I have configured this to run in the CI for every release, which guarantees that our code does not diverge from the API (at least at the time of release). This will not detect new endpoints in the Destinations API that we have not implemented yet (unless they introduce new DTOs). This could be useful but it is out of scope for this PR.
PR Checklist: