Generate Pydantic models from Destinations API spec, validate on release - #1225
Generate Pydantic models from Destinations API spec, validate on release#1225Regan-Koopmans wants to merge 3 commits into
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. |
| ] | ||
|
|
||
|
|
||
| class DestinationPatchRequest1(BaseModel): |
There was a problem hiding this comment.
I am worried that names like DestinationPatchRequest1, DestinationPatchRequest2, DestinationPatchRequest3, etc are not descriptive enough and could present a maintenance burden in the future. Let's give naming some extra attention to ensure that the name of the type is descriptive and ideally self-documenting. Using DestinationPatchRequestN as an example, I cannot clearly understand why they all exist.
|
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.
| from planet.clients.base import _BaseClient | ||
| from planet.exceptions import APIError, ClientError | ||
| from planet.http import Session | ||
| from ..api_models.destinations import Destination, DestinationsResponse |
| "geojson", | ||
| "httpx>=0.28.0", | ||
| "jsonschema", | ||
| "pydantic>=2.0", |
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: