From 25fd5f0f6c33954a45f73b81974f7154cad8596e Mon Sep 17 00:00:00 2001 From: Regan-Koopmans Date: Wed, 9 Sep 2026 13:33:50 +0200 Subject: [PATCH 01/10] Pydantic models and drift detection for Destinations API - 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 --- .github/workflows/publish-pypi.yml | 6 +- noxfile.py | 82 ++++- planet/api_models/__init__.py | 0 planet/api_models/destinations.py | 344 +++++++++++++++++++++ planet/cli/destinations.py | 16 +- planet/clients/destinations.py | 56 ++-- planet/sync/destinations.py | 22 +- pyproject.toml | 5 + tests/drift/validate_models.py | 92 ++++++ tests/integration/test_destinations_api.py | 31 +- tests/integration/test_destinations_cli.py | 50 ++- 11 files changed, 637 insertions(+), 67 deletions(-) create mode 100644 planet/api_models/__init__.py create mode 100644 planet/api_models/destinations.py create mode 100644 tests/drift/validate_models.py diff --git a/.github/workflows/publish-pypi.yml b/.github/workflows/publish-pypi.yml index 3aa701868..e45e53a08 100644 --- a/.github/workflows/publish-pypi.yml +++ b/.github/workflows/publish-pypi.yml @@ -24,9 +24,13 @@ jobs: restore-keys: | ${{ runner.os }}-pip - - name: Build, verify, and upload to PyPI + - name: Validate models against live API specs run: | pip install --upgrade nox + nox -s validate_models + + - name: Build, verify, and upload to PyPI + run: | nox -s build publish_pypi env: TWINE_PASSWORD: ${{ secrets.PYPI_API_TOKEN }} diff --git a/noxfile.py b/noxfile.py index 1dc50a244..9f0c64cea 100644 --- a/noxfile.py +++ b/noxfile.py @@ -9,6 +9,8 @@ nox.options.sessions = ['lint', 'analyze', 'test', 'coverage', 'docs'] source_files = ("planet", "examples", "tests", "setup.py", "noxfile.py") +# Generated code — excluded from linting and formatting checks +generated_dirs = ("planet/api_models", ) BUILD_DIRS = ['build', 'dist'] @@ -17,7 +19,11 @@ def analyze(session): session.install(".[lint]") - session.run("mypy", "--ignore-missing", "planet") + session.run("mypy", + "--ignore-missing", + "--exclude", + "|".join(generated_dirs), + "planet") @nox.session @@ -63,8 +69,9 @@ def test(session): def lint(session): session.install("-e", ".[lint]") - session.run("flake8", *source_files) - session.run('yapf', '--diff', '-r', *source_files) + exclude = ",".join(generated_dirs) + session.run("flake8", f"--exclude={exclude}", *source_files) + session.run('yapf', '--diff', '-r', f'--exclude={exclude}', *source_files) @nox.session @@ -114,6 +121,75 @@ def examples(session): session.run('pytest', '--no-cov', 'examples/', '-s', *options) +@nox.session +def generate_models(session): + """Re-generate Pydantic models for the Destinations API in planet/api_models/. + + Requires datamodel-code-generator to be available on PATH: + uv tool install 'datamodel-code-generator[http]' + + Run after a known API spec change to refresh the models, then re-run + validate_models to confirm compatibility. + """ + # TODO: extend to other APIs as Pydantic models are adopted: + # "subscriptions": "https://api.planet.com/subscriptions/v1/spec", + # "orders": "https://api.planet.com/compute/ops/spec", + # "data": "https://api.planet.com/data/v1/spec", + specs = { + "destinations": "https://api.planet.com/destinations/v1/spec", + } + + header = ("# flake8: noqa\n" + "# fmt: off\n" + "# Generated code — do not edit manually.\n" + "# To regenerate, run:\n" + "# nox -s generate_models\n" + "# Requires: uv tool install 'datamodel-code-generator[http]'") + + common_args = [ + "--output-model-type", + "pydantic_v2.BaseModel", + "--custom-file-header", + header, + "--formatters", + "builtin", + ] + + for name, url in specs.items(): + session.run( + "datamodel-codegen", + "--url", + url, + "--input-file-type", + "openapi", + "--output", + f"planet/api_models/{name}.py", + *common_args, + external=True, + ) + + +@nox.session +def validate_models(session): + """Validate committed Pydantic models match the live API specs. + + Fetches live OpenAPI specs from Planet's API and compares against committed + snapshots. Fails if any spec has changed. No API key required. + + To refresh snapshots after a deliberate API change, run: + nox -s generate_models + Intended as a pre-release gate; not included in the default nox session list. + """ + session.install("-e", ".[validate_models]") + session.run( + "pytest", + "tests/drift/validate_models.py", + "-v", + "--no-cov", + "--tb=short", + ) + + @nox.session def build(session): """Build package""" diff --git a/planet/api_models/__init__.py b/planet/api_models/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/planet/api_models/destinations.py b/planet/api_models/destinations.py new file mode 100644 index 000000000..11aac712f --- /dev/null +++ b/planet/api_models/destinations.py @@ -0,0 +1,344 @@ +# flake8: noqa +# fmt: off +# Generated code — do not edit manually. +# To regenerate, run: +# nox -s generate_models +# Requires: uv tool install 'datamodel-code-generator[http]' + +from enum import Enum + +from typing import Annotated +from pydantic import AwareDatetime, BaseModel, ConfigDict, Field, RootModel, StringConstraints + + +class AmazonS3Params(BaseModel): + model_config = ConfigDict(extra='forbid', ) + aws_access_key_id: str = Field( + ..., + description='AWS access key ID for authentication with Amazon S3.') + aws_region: str = Field( + ..., description='The AWS region where the S3 bucket is located.') + aws_secret_access_key: str = Field( + ..., + description='AWS secret access key for authentication with Amazon S3.') + bucket: str = Field( + ..., + description= + 'The name of the Amazon S3 bucket where data will be delivered.', + ) + explicit_sse: bool | None = Field( + False, + description='Enable explicit server-side encryption headers for SSE-S3.' + ) + + +class AmazonS3PatchParams(BaseModel): + model_config = ConfigDict(extra='forbid', ) + aws_access_key_id: str = Field( + ..., + description='AWS access key ID for authentication with Amazon S3.') + aws_secret_access_key: str = Field( + ..., + description='AWS secret access key for authentication with Amazon S3.') + explicit_sse: bool | None = Field( + False, + description='Enable explicit server-side encryption headers for SSE-S3.' + ) + + +class AzureCloudStorageParams(BaseModel): + model_config = ConfigDict(extra='forbid', ) + account: str = Field( + ..., + description= + 'The name of the Azure Storage account where data will be delivered.', + ) + container: str = Field( + ..., + description= + 'The name of the Azure Blob Storage container within the account.', + ) + sas_token: str = Field( + ..., + description= + 'Shared Access Signature (SAS) token for authentication with Azure Storage.', + ) + storage_endpoint_suffix: str | None = Field( + None, + description= + 'The storage endpoint suffix for the Azure Storage service (optional).', + ) + + +class AzureCloudStoragePatchParams(BaseModel): + model_config = ConfigDict(extra='forbid', ) + sas_token: str = Field( + ..., + description= + 'Shared Access Signature (SAS) token for authentication with Azure Storage.', + ) + + +class DefaultDestinationRequest(BaseModel): + model_config = ConfigDict(extra='forbid', ) + destination_id: str = Field( + ..., description='The ID of the default destination.') + + +class DestinationType(Enum): + google_cloud_storage = 'google_cloud_storage' + amazon_s3 = 'amazon_s3' + azure_blob_storage = 'azure_blob_storage' + oracle_cloud_storage = 'oracle_cloud_storage' + s3_compatible = 's3_compatible' + + +class Error(BaseModel): + code: int + message: str + + +class GoogleCloudStorageParams(BaseModel): + model_config = ConfigDict(extra='forbid', ) + bucket: str = Field( + ..., + description= + 'The name of the Google Cloud Storage bucket where data will be delivered.', + ) + credentials: str = Field( + ..., + description= + "Base64-encoded service account JSON credentials for Google Cloud Storage access.\n\nTo encode the credentials: `cat service-account.json | base64 | tr -d '\\n'`\n", + ) + + +class GoogleCloudStoragePatchParams(BaseModel): + model_config = ConfigDict(extra='forbid', ) + credentials: str = Field( + ..., + description= + "Base64-encoded service account JSON credentials for Google Cloud Storage access.\n\nTo encode the credentials: `cat service-account.json | base64 | tr -d '\\n'`\n", + ) + + +class Links(BaseModel): + field_self: str = Field( + ..., + alias='_self', + description='RFC 3986 URI representing the location of this object.', + ) + + +class OracleCloudStorageParams(BaseModel): + model_config = ConfigDict(extra='forbid', ) + bucket: str = Field( + ..., + description= + 'The name of the Oracle Cloud Storage bucket where data will be delivered.', + ) + customer_access_key_id: str = Field( + ..., + description= + 'Customer access key ID for authentication with Oracle Cloud Storage.', + ) + customer_secret_key: str = Field( + ..., + description= + 'Customer secret key for authentication with Oracle Cloud Storage.', + ) + namespace: str = Field( + ..., + description= + 'The Oracle Object Storage namespace that contains the bucket.') + region: str = Field( + ..., + description='The Oracle Cloud region where the bucket is located.') + + +class OracleCloudStoragePatchParams(BaseModel): + model_config = ConfigDict(extra='forbid', ) + customer_access_key_id: str = Field( + ..., + description= + 'Customer access key ID for authentication with Oracle Cloud Storage.', + ) + customer_secret_key: str = Field( + ..., + description= + 'Customer secret key for authentication with Oracle Cloud Storage.', + ) + + +class Ownership(BaseModel): + is_owner: bool = Field( + ..., description='True if the user is the creator of the destination.') + owner_id: int = Field( + ..., description='The ID of the user who created the destination.') + + +class Permissions(BaseModel): + can_write: bool = Field( + ..., + description='True if the user can write to the destination (patch).') + + +class S3CompatibleParams(BaseModel): + model_config = ConfigDict(extra='forbid', ) + access_key_id: str = Field( + ..., + description= + 'Access key ID for authentication with the S3-compatible service.', + ) + bucket: str = Field( + ..., + description= + 'The name of the S3-compatible bucket where data will be delivered.', + ) + endpoint: str = Field( + ..., + description='The URL endpoint for the S3-compatible storage service.') + region: str = Field( + ..., + description= + 'The region identifier for the S3-compatible storage service.') + secret_access_key: str = Field( + ..., + description= + 'Secret access key for authentication with the S3-compatible service.', + ) + use_path_style: bool | None = Field( + False, + description= + 'Use path-style URL addressing with the bucket name in the URL path.', + ) + + +class S3CompatiblePatchParams(BaseModel): + model_config = ConfigDict(extra='forbid', ) + access_key_id: str = Field( + ..., + description= + 'Access key ID for authentication with the S3-compatible service.', + ) + secret_access_key: str = Field( + ..., + description= + 'Secret access key for authentication with the S3-compatible service.', + ) + use_path_style: bool | None = Field( + False, + description= + 'Use path-style URL addressing with the bucket name in the URL path.', + ) + + +class DestinationParameters(RootModel[GoogleCloudStorageParams + | AmazonS3Params + | AzureCloudStorageParams + | OracleCloudStorageParams + | S3CompatibleParams]): + root: (GoogleCloudStorageParams + | AmazonS3Params + | AzureCloudStorageParams + | OracleCloudStorageParams + | S3CompatibleParams) = Field( + ..., description='Parameters for the given Destination type.') + + +class DestinationPatchParameters(RootModel[GoogleCloudStoragePatchParams + | AmazonS3PatchParams + | AzureCloudStoragePatchParams + | OracleCloudStoragePatchParams + | S3CompatiblePatchParams]): + root: (GoogleCloudStoragePatchParams + | AmazonS3PatchParams + | AzureCloudStoragePatchParams + | OracleCloudStoragePatchParams + | S3CompatiblePatchParams) = Field( + ..., + description='Patch parameters for the given Destination type.') + + +class DestinationPatchRequest1(BaseModel): + model_config = ConfigDict(extra='forbid', ) + archive: bool | None = Field( + None, + description='True to archive the destination, false to unarchive.') + name: Annotated[ + str, StringConstraints(min_length=3, max_length=63)] | None = Field( + None, description='A string to uniquely identify a Destination.') + parameters: DestinationPatchParameters + + +class DestinationPatchRequest2(BaseModel): + model_config = ConfigDict(extra='forbid', ) + archive: bool = Field( + ..., + description='True to archive the destination, false to unarchive.') + name: Annotated[ + str, StringConstraints(min_length=3, max_length=63)] | None = Field( + None, description='A string to uniquely identify a Destination.') + parameters: DestinationPatchParameters | None = None + + +class DestinationPatchRequest3(BaseModel): + model_config = ConfigDict(extra='forbid', ) + archive: bool | None = Field( + None, + description='True to archive the destination, false to unarchive.') + name: Annotated[ + str, StringConstraints(min_length=3, max_length=63)] = Field( + ..., description='A string to uniquely identify a Destination.') + parameters: DestinationPatchParameters | None = None + + +class DestinationPatchRequest(RootModel[DestinationPatchRequest1 + | DestinationPatchRequest2 + | DestinationPatchRequest3]): + root: ( + DestinationPatchRequest1 | DestinationPatchRequest2 + | DestinationPatchRequest3 + ) = Field( + ..., + description= + 'A DestinationPatchRequest is an object describing how to update a Destination.', + title='Destination patch request') + + +class DestinationRequest(BaseModel): + model_config = ConfigDict(extra='forbid', ) + name: Annotated[ + str, StringConstraints(min_length=3, max_length=63)] | None = Field( + None, description='A name given to this Destination.') + parameters: DestinationParameters + type: DestinationType + + +class Destination(BaseModel): + field_links: Links = Field(..., alias='_links') + archived: AwareDatetime | None = Field( + None, description='Timestamp when the Destination was archived.') + created: AwareDatetime = Field( + ..., description='Timestamp when the Destination was created.') + default: bool | None = Field( + None, + description= + 'True if this is the default destination for the organization.') + id: str = Field(..., + description='A string to uniquely identify a Destination.') + name: str = Field(..., description='A name given to this Destination.') + ownership: Ownership + parameters: DestinationParameters + permissions: Permissions + pl_ref: str = Field(..., + alias='pl:ref', + description='A reference for the destination.') + type: DestinationType + updated: AwareDatetime = Field( + ..., description='Timestamp when the Destination was last updated.') + + +class DestinationsResponse(BaseModel): + field_links: Links = Field(..., alias='_links') + destinations: list[Destination] = Field( + ..., description='Array of Destinations.') diff --git a/planet/cli/destinations.py b/planet/cli/destinations.py index ed3a25131..5c37e706f 100644 --- a/planet/cli/destinations.py +++ b/planet/cli/destinations.py @@ -31,7 +31,7 @@ async def _patch_destination(ctx, destination_id, data, pretty): async with destinations_client(ctx) as cl: try: response = await cl.patch_destination(destination_id, data) - echo_json(response, pretty) + echo_json(response.model_dump(mode='json', by_alias=True), pretty) except Exception as e: raise ClickException(f"Failed to patch destination: {e}") @@ -48,7 +48,7 @@ async def _list_destinations(ctx, is_owner, can_write, is_default) - echo_json(response, pretty) + echo_json(response.model_dump(mode='json', by_alias=True), pretty) except Exception as e: raise ClickException(f"Failed to list destinations: {e}") @@ -57,7 +57,7 @@ async def _get_destination(ctx, destination_id, pretty): async with destinations_client(ctx) as cl: try: response = await cl.get_destination(destination_id) - echo_json(response, pretty) + echo_json(response.model_dump(mode='json', by_alias=True), pretty) except Exception as e: raise ClickException(f"Failed to get destination: {e}") @@ -66,7 +66,7 @@ async def _create_destination(ctx, data, pretty): async with destinations_client(ctx) as cl: try: response = await cl.create_destination(data) - echo_json(response, pretty) + echo_json(response.model_dump(mode='json', by_alias=True), pretty) except Exception as e: raise ClickException(f"Failed to create destination: {e}") @@ -75,7 +75,7 @@ async def _set_default_destination(ctx, destination_id, pretty): async with destinations_client(ctx) as cl: try: response = await cl.set_default_destination(destination_id) - echo_json(response, pretty) + echo_json(response.model_dump(mode='json', by_alias=True), pretty) except Exception as e: raise ClickException(f"Failed to set default destination: {e}") @@ -84,7 +84,9 @@ async def _unset_default_destination(ctx, pretty): async with destinations_client(ctx) as cl: try: response = await cl.unset_default_destination() - echo_json(response, pretty) + if response is not None: + echo_json(response.model_dump(mode='json', by_alias=True), + pretty) except Exception as e: raise ClickException(f"Failed to unset default destination: {e}") @@ -93,7 +95,7 @@ async def _get_default_destination(ctx, pretty): async with destinations_client(ctx) as cl: try: response = await cl.get_default_destination() - echo_json(response, pretty) + echo_json(response.model_dump(mode='json', by_alias=True), pretty) except Exception as e: raise ClickException(f"Failed to get default destination: {e}") diff --git a/planet/clients/destinations.py b/planet/clients/destinations.py index 1d1f0dad0..7fdd046d5 100644 --- a/planet/clients/destinations.py +++ b/planet/clients/destinations.py @@ -13,19 +13,18 @@ # the License. import logging -from typing import Any, Dict, Optional, TypeVar +from typing import Any, Dict, Optional from planet.clients.base import _BaseClient from planet.exceptions import APIError, ClientError from planet.http import Session +from ..api_models.destinations import Destination, DestinationsResponse from ..constants import PLANET_BASE_URL BASE_URL = f'{PLANET_BASE_URL}/destinations/v1/' LOGGER = logging.getLogger() -T = TypeVar("T") - DEFAULT_DESTINATION_REF = "pl:destinations/default" @@ -57,11 +56,12 @@ def __init__(self, """ super().__init__(session, base_url or BASE_URL) - async def list_destinations(self, - archived: Optional[bool] = None, - is_owner: Optional[bool] = None, - can_write: Optional[bool] = None, - is_default: Optional[bool] = None) -> Dict: + async def list_destinations( + self, + archived: Optional[bool] = None, + is_owner: Optional[bool] = None, + can_write: Optional[bool] = None, + is_default: Optional[bool] = None) -> DestinationsResponse: """ List all destinations. By default, all non-archived destinations in the requesting user's org are returned. @@ -72,7 +72,7 @@ async def list_destinations(self, is_default (bool): If True, include only the default destination. Returns: - dict: A dictionary containing the list of destinations inside the 'destinations' key. + DestinationsResponse: The list of destinations. Raises: APIError: If the API returns an error response. @@ -97,10 +97,9 @@ async def list_destinations(self, except ClientError: # pragma: no cover raise else: - dest_response = response.json() - return dest_response + return DestinationsResponse.model_validate(response.json()) - async def get_destination(self, destination_id: str) -> Dict: + async def get_destination(self, destination_id: str) -> Destination: """ Get a specific destination by its ID. @@ -108,7 +107,7 @@ async def get_destination(self, destination_id: str) -> Dict: destination_id (str): The ID of the destination to retrieve. Returns: - dict: A dictionary containing the destination details. + Destination: The destination details. Raises: APIError: If the API returns an error response. @@ -122,12 +121,11 @@ async def get_destination(self, destination_id: str) -> Dict: except ClientError: # pragma: no cover raise else: - dest = response.json() - return dest + return Destination.model_validate(response.json()) async def patch_destination(self, destination_id: str, - request: Dict[str, Any]) -> Dict: + request: Dict[str, Any]) -> Destination: """ Update a specific destination by its ID. @@ -136,7 +134,7 @@ async def patch_destination(self, request (dict): Destination content to update, only attributes to update are required. Returns: - dict: A dictionary containing the updated destination details. + Destination: The updated destination details. Raises: APIError: If the API returns an error response. @@ -152,10 +150,9 @@ async def patch_destination(self, except ClientError: # pragma: no cover raise else: - dest = response.json() - return dest + return Destination.model_validate(response.json()) - async def create_destination(self, request: Dict[str, Any]) -> Dict: + async def create_destination(self, request: Dict[str, Any]) -> Destination: """ Create a new destination. @@ -163,7 +160,7 @@ async def create_destination(self, request: Dict[str, Any]) -> Dict: request (dict): Destination content to create, all attributes are required. Returns: - dict: A dictionary containing the created destination details. + Destination: The created destination details. Raises: APIError: If the API returns an error response. @@ -178,10 +175,10 @@ async def create_destination(self, request: Dict[str, Any]) -> Dict: except ClientError: # pragma: no cover raise else: - dest = response.json() - return dest + return Destination.model_validate(response.json()) - async def set_default_destination(self, destination_id: str) -> Dict: + async def set_default_destination(self, + destination_id: str) -> Destination: """ Set an existing destination as the default destination. Default destinations are globally available to all members of an organization. An organization can have zero or one default destination at any time. @@ -191,7 +188,7 @@ async def set_default_destination(self, destination_id: str) -> Dict: destination_id (str): The ID of the destination to set as default. Returns: - dict: A dictionary containing the default destination details. + Destination: The default destination details. Raises: APIError: If the API returns an error response. @@ -208,7 +205,7 @@ async def set_default_destination(self, destination_id: str) -> Dict: except ClientError: # pragma: no cover raise else: - return response.json() + return Destination.model_validate(response.json()) async def unset_default_destination(self) -> None: """ @@ -230,13 +227,13 @@ async def unset_default_destination(self) -> None: except ClientError: # pragma: no cover raise - async def get_default_destination(self) -> Dict: + async def get_default_destination(self) -> Destination: """ Get the current default destination. The default destination is globally available to all members of an organization. Returns: - dict: A dictionary containing the default destination details. + Destination: The default destination details. Raises: APIError: If the API returns an error response. @@ -250,5 +247,4 @@ async def get_default_destination(self) -> Dict: except ClientError: # pragma: no cover raise else: - dest = response.json() - return dest + return Destination.model_validate(response.json()) diff --git a/planet/sync/destinations.py b/planet/sync/destinations.py index a95b2f977..b688c792c 100644 --- a/planet/sync/destinations.py +++ b/planet/sync/destinations.py @@ -14,6 +14,7 @@ from typing import Any, Dict, Optional from planet.clients.destinations import DestinationsClient +from planet.api_models.destinations import Destination, DestinationsResponse from planet.http import Session @@ -31,11 +32,12 @@ def __init__(self, session: Session, base_url: Optional[str] = None): self._client = DestinationsClient(session, base_url) - def list_destinations(self, - archived: Optional[bool] = None, - is_owner: Optional[bool] = None, - can_write: Optional[bool] = None, - is_default: Optional[bool] = None) -> Dict: + def list_destinations( + self, + archived: Optional[bool] = None, + is_owner: Optional[bool] = None, + can_write: Optional[bool] = None, + is_default: Optional[bool] = None) -> DestinationsResponse: """ List all destinations. By default, all non-archived destinations in the requesting user's org are returned. @@ -58,7 +60,7 @@ def list_destinations(self, can_write, is_default)) - def get_destination(self, destination_id: str) -> Dict: + def get_destination(self, destination_id: str) -> Destination: """ Get a specific destination by its ID. @@ -76,7 +78,7 @@ def get_destination(self, destination_id: str) -> Dict: self._client.get_destination(destination_id)) def patch_destination(self, destination_ref: str, - request: Dict[str, Any]) -> Dict: + request: Dict[str, Any]) -> Destination: """ Update a specific destination by its ref. @@ -94,7 +96,7 @@ def patch_destination(self, destination_ref: str, return self._client._call_sync( self._client.patch_destination(destination_ref, request)) - def create_destination(self, request: Dict[str, Any]) -> Dict: + def create_destination(self, request: Dict[str, Any]) -> Destination: """ Create a new destination. @@ -111,7 +113,7 @@ def create_destination(self, request: Dict[str, Any]) -> Dict: return self._client._call_sync( self._client.create_destination(request)) - def set_default_destination(self, destination_id: str) -> Dict: + def set_default_destination(self, destination_id: str) -> Destination: """ Set an existing destination as the default destination. Default destinations are globally available to all members of an organization. An organization can have zero or one default destination at any time. @@ -145,7 +147,7 @@ def unset_default_destination(self) -> None: return self._client._call_sync( self._client.unset_default_destination()) - def get_default_destination(self) -> Dict: + def get_default_destination(self) -> Destination: """ Get the current default destination. The default destination is globally available to all members of an organization. diff --git a/pyproject.toml b/pyproject.toml index 04837eca5..c1f6ae592 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -11,6 +11,7 @@ dependencies = [ "geojson", "httpx>=0.28.0", "jsonschema", + "pydantic>=2.0", "pyjwt>=2.1", "tqdm>=4.56", "typing-extensions", @@ -41,6 +42,10 @@ test = [ "respx>=0.22.0", "coverage[toml]" ] +validate_models = [ + "pytest==8.3.3", + "datamodel-code-generator[http]>=0.25", +] lint = [ "flake8", "mypy", diff --git a/tests/drift/validate_models.py b/tests/drift/validate_models.py new file mode 100644 index 000000000..232625e03 --- /dev/null +++ b/tests/drift/validate_models.py @@ -0,0 +1,92 @@ +# Copyright 2024 Planet Labs PBC. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may not +# use this file except in compliance with the License. You may obtain a copy of +# the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations under +# the License. +"""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. + - The output is compared against the committed file in planet/api_models/. + - The test fails if they differ, indicating the spec has changed. + +When a test fails: + 1. Review what changed in the spec. + 2. Regenerate the committed models: + nox -s generate_models + 3. Update the client code if the API change requires it. + 4. Commit the updated models. +""" +import pathlib +import subprocess +import tempfile + +import pytest + +REPO_ROOT = pathlib.Path(__file__).parent.parent.parent +MODELS_DIR = REPO_ROOT / "planet" / "api_models" + +HEADER = ("# flake8: noqa\n" + "# fmt: off\n" + "# Generated code — do not edit manually.\n" + "# To regenerate, run:\n" + "# nox -s generate_models\n" + "# Requires: uv tool install 'datamodel-code-generator[http]'") + +SPECS = { + "destinations": "https://api.planet.com/destinations/v1/spec", +} + + +def _regenerate(url: str, output: pathlib.Path) -> None: + result = subprocess.run( + [ + "datamodel-codegen", + "--url", + url, + "--input-file-type", + "openapi", + "--output", + str(output), + "--output-model-type", + "pydantic_v2.BaseModel", + "--custom-file-header", + HEADER, + "--formatters", + "builtin", + ], + capture_output=True, + text=True, + ) + if result.returncode != 0: + pytest.fail(f"datamodel-codegen failed:\n{result.stderr}") + + +@pytest.mark.parametrize("name,url", SPECS.items()) +def test_models_match_spec(name, url): + committed = MODELS_DIR / f"{name}.py" + + with tempfile.NamedTemporaryFile(suffix=".py", delete=False) as tmp: + tmp_path = pathlib.Path(tmp.name) + + try: + _regenerate(url, tmp_path) + + generated = tmp_path.read_text() + current = committed.read_text() + + if generated != current: + pytest.fail( + f"planet/api_models/{name}.py is out of date with the live spec.\n" + f"Run `nox -s generate_models` to regenerate, then commit the result." + ) + finally: + tmp_path.unlink(missing_ok=True) diff --git a/tests/integration/test_destinations_api.py b/tests/integration/test_destinations_api.py index af702b223..d395edf46 100644 --- a/tests/integration/test_destinations_api.py +++ b/tests/integration/test_destinations_api.py @@ -20,6 +20,7 @@ from planet import DestinationsClient, Session from planet.auth import Auth from planet.sync.destinations import DestinationsAPI +from planet.api_models.destinations import Destination, DestinationsResponse pytestmark = pytest.mark.anyio @@ -107,8 +108,11 @@ def construct_list_response(destinations): async def test_list_destinations(): mock_response(TEST_URL, construct_list_response(DEST_LIST)) + expected = DestinationsResponse.model_validate( + construct_list_response(DEST_LIST)) + def assertf(resp): - assert resp == construct_list_response(DEST_LIST) + assert resp == expected assertf(await cl_async.list_destinations()) assertf(cl_sync.list_destinations()) @@ -119,8 +123,11 @@ async def test_list_destinations_filtering(): mock_response(f"{TEST_URL}?archived=false&is_owner=true", construct_list_response([DEST_1])) + expected = DestinationsResponse.model_validate( + construct_list_response([DEST_1])) + def assertf(resp): - assert resp == construct_list_response([DEST_1]) + assert resp == expected assertf(await cl_async.list_destinations(archived=False, is_owner=True)) assertf(cl_sync.list_destinations(archived=False, is_owner=True)) @@ -132,8 +139,10 @@ async def test_get_destination(): url = f"{TEST_URL}/{id}" mock_response(url, DEST_1) + expected = Destination.model_validate(DEST_1) + def assertf(resp): - assert resp == DEST_1 + assert resp == expected assertf(await cl_async.get_destination(id)) assertf(cl_sync.get_destination(id)) @@ -146,8 +155,10 @@ async def test_create_destination(): method="post", status_code=HTTPStatus.CREATED) + expected = Destination.model_validate(DEST_1) + def assertf(resp): - assert resp == DEST_1 + assert resp == expected assertf(await cl_async.create_destination(DEST_1_REQ_PAYLOAD)) assertf(cl_sync.create_destination(DEST_1_REQ_PAYLOAD)) @@ -159,8 +170,10 @@ async def test_patch_destination(): url = f"{TEST_URL}/{id}" mock_response(url, DEST_2, method="patch") + expected = Destination.model_validate(DEST_2) + def assertf(resp): - assert resp == DEST_2 + assert resp == expected assertf(await cl_async.patch_destination(id, DEST_2_PATCH_PAYLOAD)) assertf(cl_sync.patch_destination(id, DEST_2_PATCH_PAYLOAD)) @@ -187,8 +200,10 @@ async def test_set_default_destination(): url = f"{TEST_URL}/default" mock_response(url, DEST_1, method="put") + expected = Destination.model_validate(DEST_1) + def assertf(resp): - assert resp == DEST_1 + assert resp == expected assertf(await cl_async.set_default_destination(id)) assertf(cl_sync.set_default_destination(id)) @@ -216,8 +231,10 @@ async def test_get_default_destination(): url = f"{TEST_URL}/default" mock_response(url, DEST_1) + expected = Destination.model_validate(DEST_1) + def assertf(resp): - assert resp == DEST_1 + assert resp == expected assertf(await cl_async.get_default_destination()) assertf(cl_sync.get_default_destination()) diff --git a/tests/integration/test_destinations_cli.py b/tests/integration/test_destinations_cli.py index f975989b8..76c137aae 100644 --- a/tests/integration/test_destinations_cli.py +++ b/tests/integration/test_destinations_cli.py @@ -22,6 +22,38 @@ TEST_DESTINATIONS_URL = 'https://api.planet.com/destinations/v1' +DEST = { + "id": "fake-dest-id", + "name": "Fake Destination", + "type": "amazon_s3", + "parameters": { + "bucket": "my-bucket", + "aws_region": "us-west-2", + "aws_access_key_id": "key", + "aws_secret_access_key": "secret" + }, + "created": "2024-01-01T00:00:00Z", + "updated": "2024-01-01T00:00:00Z", + "pl:ref": "pl:destinations/fake-dest-id", + "_links": { + "_self": "https://api.planet.com/destinations/v1/fake-dest-id" + }, + "archived": None, + "permissions": { + "can_write": True + }, + "ownership": { + "is_owner": True, "owner_id": 1 + } +} + +DEST_LIST = { + "destinations": [DEST], + "_links": { + "_self": "https://api.planet.com/destinations/v1" + } +} + @pytest.fixture def invoke(): @@ -37,7 +69,7 @@ def _invoke(extra_args, runner=None): @respx.mock def test_destinations_cli_archive(invoke): url = f"{TEST_DESTINATIONS_URL}/fake-dest-id" - respx.patch(url).return_value = httpx.Response(HTTPStatus.OK, json={}) + respx.patch(url).return_value = httpx.Response(HTTPStatus.OK, json=DEST) result = invoke(['archive', 'fake-dest-id']) assert result.exit_code == 0 @@ -46,7 +78,7 @@ def test_destinations_cli_archive(invoke): @respx.mock def test_destinations_cli_create(invoke): respx.post(TEST_DESTINATIONS_URL).return_value = httpx.Response( - HTTPStatus.ACCEPTED, json={}) + HTTPStatus.ACCEPTED, json=DEST) # azure result = invoke([ @@ -139,7 +171,7 @@ def test_destinations_cli_create(invoke): @respx.mock def test_destinations_cli_get(invoke): url = f"{TEST_DESTINATIONS_URL}/fake-dest-id" - respx.get(url).return_value = httpx.Response(HTTPStatus.OK, json={}) + respx.get(url).return_value = httpx.Response(HTTPStatus.OK, json=DEST) result = invoke(['get', 'fake-dest-id']) assert result.exit_code == 0 @@ -148,7 +180,7 @@ def test_destinations_cli_get(invoke): @respx.mock def test_destinations_cli_rename(invoke): url = f"{TEST_DESTINATIONS_URL}/fake-dest-id" - respx.patch(url).return_value = httpx.Response(HTTPStatus.OK, json={}) + respx.patch(url).return_value = httpx.Response(HTTPStatus.OK, json=DEST) result = invoke(['rename', 'fake-dest-id', 'new-name']) assert result.exit_code == 0 @@ -157,7 +189,7 @@ def test_destinations_cli_rename(invoke): @respx.mock def test_destinations_cli_unarchive(invoke): url = f"{TEST_DESTINATIONS_URL}/fake-dest-id" - respx.patch(url).return_value = httpx.Response(HTTPStatus.OK, json={}) + respx.patch(url).return_value = httpx.Response(HTTPStatus.OK, json=DEST) result = invoke(['unarchive', 'fake-dest-id']) assert result.exit_code == 0 @@ -166,7 +198,7 @@ def test_destinations_cli_unarchive(invoke): @respx.mock def test_destinations_cli_list(invoke): respx.get(TEST_DESTINATIONS_URL).return_value = httpx.Response( - HTTPStatus.OK, json={}) + HTTPStatus.OK, json=DEST_LIST) result = invoke(['list']) assert result.exit_code == 0 @@ -203,7 +235,7 @@ def test_destinations_cli_list(invoke): def test_destinations_cli_update(invoke): url = f"{TEST_DESTINATIONS_URL}/fake-dest-id" respx.patch(url).return_value = httpx.Response(HTTPStatus.ACCEPTED, - json={}) + json=DEST) # azure result = invoke( @@ -262,7 +294,7 @@ def test_destinations_cli_update(invoke): @respx.mock def test_destinations_cli_default_set(invoke): url = f"{TEST_DESTINATIONS_URL}/default" - respx.put(url).return_value = httpx.Response(HTTPStatus.OK, json={}) + respx.put(url).return_value = httpx.Response(HTTPStatus.OK, json=DEST) result = invoke(['default', 'set', 'fake-dest-id']) assert result.exit_code == 0 @@ -285,7 +317,7 @@ def test_destinations_cli_default_set_bad_request(invoke): @respx.mock def test_destinations_cli_default_get(invoke): url = f"{TEST_DESTINATIONS_URL}/default" - respx.get(url).return_value = httpx.Response(HTTPStatus.OK, json={}) + respx.get(url).return_value = httpx.Response(HTTPStatus.OK, json=DEST) result = invoke(['default', 'get']) assert result.exit_code == 0 From d9e7a4cfe49c86a1fe5b18235cf39639ae3845de Mon Sep 17 00:00:00 2001 From: Regan-Koopmans Date: Fri, 11 Sep 2026 10:04:23 +0200 Subject: [PATCH 02/10] Make Destinations models tolerant and the drift gate actually runnable 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) --- docs/get-started/upgrading-v3.md | 18 + noxfile.py | 68 ++-- planet/api_models/destinations.py | 592 ++++++++++++++++++------------ planet/cli/destinations.py | 42 ++- planet/sync/destinations.py | 12 +- pyproject.toml | 4 +- tests/drift/codegen_config.py | 78 ++++ tests/drift/validate_models.py | 45 +-- 8 files changed, 534 insertions(+), 325 deletions(-) create mode 100644 tests/drift/codegen_config.py diff --git a/docs/get-started/upgrading-v3.md b/docs/get-started/upgrading-v3.md index a8fc6c760..93fce1207 100644 --- a/docs/get-started/upgrading-v3.md +++ b/docs/get-started/upgrading-v3.md @@ -97,4 +97,22 @@ should be preferred to the use of Planet API keys. * Deprecated `planet.subscription_request.clip_tool()` method for defining custom clip AOIs with requests to create subscriptions. Subscriptions API no longer supports custom clip AOIs; instead users can opt-in to clip to their subscription source geometry by including kwarg `clip_to_source=True` when constructing requests via `planet.subscription_request.build_request()`. See [PR #1169](https://github.com/planetlabs/planet-client-python/pull/1169) for implementation details. * Renamed `planet.cli.subscriptions.request_pv()` to `planet.cli.subscriptions.request_source()`, and removed `var_type` positional argument from the signature. This change, in effect renames the CLI argument `planet subscriptions request-pv` to `planet subscriptions request-source`. Also renamed `planet.subscription_request.planetary_variable_source()` to `planet.subscription_request.subscription_source()`. Source type positional arguments are removed from these methods in favor of `source_id`. See [PR #1170](https://github.com/planetlabs/planet-client-python/pull/1170) for implementation details. +* The Destinations API methods on `DestinationsClient` and `DestinationsAPI` now return Pydantic models generated from Planet's OpenAPI spec instead of plain dictionaries. `list_destinations()` returns a `DestinationsResponse`; `get_destination()`, `create_destination()`, `patch_destination()`, `set_default_destination()` and `get_default_destination()` return a `Destination`. Attribute access replaces subscript access: + + ```python + # Version 2 + resp = pl.destinations.list_destinations() + for d in resp["destinations"]: + print(d["id"]) + + # Version 3 + resp = pl.destinations.list_destinations() + for d in resp.destinations: + print(d.id) + ``` + + Call `.model_dump(mode="json", by_alias=True)` on any of these to recover a + JSON-compatible dictionary. The models allow unknown fields, so destinations + returned by a newer version of the API still parse. + ---- diff --git a/noxfile.py b/noxfile.py index 9f0c64cea..40b8414b5 100644 --- a/noxfile.py +++ b/noxfile.py @@ -1,5 +1,6 @@ from pathlib import Path import shutil +import sys import nox @@ -69,9 +70,13 @@ def test(session): def lint(session): session.install("-e", ".[lint]") - exclude = ",".join(generated_dirs) - session.run("flake8", f"--exclude={exclude}", *source_files) - session.run('yapf', '--diff', '-r', f'--exclude={exclude}', *source_files) + session.run("flake8", + f"--exclude={','.join(generated_dirs)}", + *source_files) + # yapf --exclude is a repeatable flag taking one fnmatch pattern; a bare + # directory name matches nothing, so the trailing /* is required. + yapf_excludes = [f"--exclude={d}/*" for d in generated_dirs] + session.run('yapf', '--diff', '-r', *yapf_excludes, *source_files) @nox.session @@ -125,48 +130,21 @@ def examples(session): def generate_models(session): """Re-generate Pydantic models for the Destinations API in planet/api_models/. - Requires datamodel-code-generator to be available on PATH: - uv tool install 'datamodel-code-generator[http]' + Uses the same pinned datamodel-code-generator as `nox -s validate_models`, + so the committed output is byte-identical to what the drift check + regenerates. Do not reformat the result. Run after a known API spec change to refresh the models, then re-run validate_models to confirm compatibility. """ - # TODO: extend to other APIs as Pydantic models are adopted: - # "subscriptions": "https://api.planet.com/subscriptions/v1/spec", - # "orders": "https://api.planet.com/compute/ops/spec", - # "data": "https://api.planet.com/data/v1/spec", - specs = { - "destinations": "https://api.planet.com/destinations/v1/spec", - } - - header = ("# flake8: noqa\n" - "# fmt: off\n" - "# Generated code — do not edit manually.\n" - "# To regenerate, run:\n" - "# nox -s generate_models\n" - "# Requires: uv tool install 'datamodel-code-generator[http]'") - - common_args = [ - "--output-model-type", - "pydantic_v2.BaseModel", - "--custom-file-header", - header, - "--formatters", - "builtin", - ] - - for name, url in specs.items(): - session.run( - "datamodel-codegen", - "--url", - url, - "--input-file-type", - "openapi", - "--output", - f"planet/api_models/{name}.py", - *common_args, - external=True, - ) + session.install("-e", ".[validate_models]") + + sys.path.insert(0, str(Path(__file__).parent / "tests" / "drift")) + import codegen_config + + for name, url in codegen_config.SPECS.items(): + output = Path("planet/api_models") / f"{name}.py" + session.run(*codegen_config.codegen_argv(url, output)) @nox.session @@ -184,8 +162,14 @@ def validate_models(session): session.run( "pytest", "tests/drift/validate_models.py", + # Stop conftest discovery below tests/, whose conftest imports the + # full test-suite dependencies that this extra deliberately omits. + "--confcutdir=tests/drift", + # setup.cfg addopts injects --cov, but this extra deliberately omits + # pytest-cov; clear addopts rather than pull in the full test deps. + "-o", + "addopts=", "-v", - "--no-cov", "--tb=short", ) diff --git a/planet/api_models/destinations.py b/planet/api_models/destinations.py index 11aac712f..b2685db22 100644 --- a/planet/api_models/destinations.py +++ b/planet/api_models/destinations.py @@ -1,88 +1,110 @@ # flake8: noqa # fmt: off # Generated code — do not edit manually. +# Reformatting this file will break `nox -s validate_models`. # To regenerate, run: # nox -s generate_models -# Requires: uv tool install 'datamodel-code-generator[http]' -from enum import Enum +from __future__ import annotations +from enum import Enum from typing import Annotated -from pydantic import AwareDatetime, BaseModel, ConfigDict, Field, RootModel, StringConstraints + +from pydantic import AwareDatetime, BaseModel, ConfigDict, Field, RootModel class AmazonS3Params(BaseModel): - model_config = ConfigDict(extra='forbid', ) - aws_access_key_id: str = Field( - ..., - description='AWS access key ID for authentication with Amazon S3.') - aws_region: str = Field( - ..., description='The AWS region where the S3 bucket is located.') - aws_secret_access_key: str = Field( - ..., - description='AWS secret access key for authentication with Amazon S3.') - bucket: str = Field( - ..., - description= - 'The name of the Amazon S3 bucket where data will be delivered.', - ) - explicit_sse: bool | None = Field( - False, - description='Enable explicit server-side encryption headers for SSE-S3.' + model_config = ConfigDict( + extra='allow', ) + aws_access_key_id: Annotated[ + str, Field(description='AWS access key ID for authentication with Amazon S3.') + ] + aws_region: Annotated[ + str, Field(description='The AWS region where the S3 bucket is located.') + ] + aws_secret_access_key: Annotated[ + str, + Field(description='AWS secret access key for authentication with Amazon S3.'), + ] + bucket: Annotated[ + str, + Field( + description='The name of the Amazon S3 bucket where data will be delivered.' + ), + ] + explicit_sse: Annotated[ + bool, + Field(description='Enable explicit server-side encryption headers for SSE-S3.'), + ] = False class AmazonS3PatchParams(BaseModel): - model_config = ConfigDict(extra='forbid', ) - aws_access_key_id: str = Field( - ..., - description='AWS access key ID for authentication with Amazon S3.') - aws_secret_access_key: str = Field( - ..., - description='AWS secret access key for authentication with Amazon S3.') - explicit_sse: bool | None = Field( - False, - description='Enable explicit server-side encryption headers for SSE-S3.' + model_config = ConfigDict( + extra='allow', ) + aws_access_key_id: Annotated[ + str, Field(description='AWS access key ID for authentication with Amazon S3.') + ] + aws_secret_access_key: Annotated[ + str, + Field(description='AWS secret access key for authentication with Amazon S3.'), + ] + explicit_sse: Annotated[ + bool, + Field(description='Enable explicit server-side encryption headers for SSE-S3.'), + ] = False class AzureCloudStorageParams(BaseModel): - model_config = ConfigDict(extra='forbid', ) - account: str = Field( - ..., - description= - 'The name of the Azure Storage account where data will be delivered.', - ) - container: str = Field( - ..., - description= - 'The name of the Azure Blob Storage container within the account.', - ) - sas_token: str = Field( - ..., - description= - 'Shared Access Signature (SAS) token for authentication with Azure Storage.', - ) - storage_endpoint_suffix: str | None = Field( - None, - description= - 'The storage endpoint suffix for the Azure Storage service (optional).', + model_config = ConfigDict( + extra='allow', ) + account: Annotated[ + str, + Field( + description='The name of the Azure Storage account where data will be delivered.' + ), + ] + container: Annotated[ + str, + Field( + description='The name of the Azure Blob Storage container within the account.' + ), + ] + sas_token: Annotated[ + str, + Field( + description='Shared Access Signature (SAS) token for authentication with Azure Storage.' + ), + ] + storage_endpoint_suffix: Annotated[ + str | None, + Field( + description='The storage endpoint suffix for the Azure Storage service (optional).' + ), + ] = None class AzureCloudStoragePatchParams(BaseModel): - model_config = ConfigDict(extra='forbid', ) - sas_token: str = Field( - ..., - description= - 'Shared Access Signature (SAS) token for authentication with Azure Storage.', + model_config = ConfigDict( + extra='allow', ) + sas_token: Annotated[ + str, + Field( + description='Shared Access Signature (SAS) token for authentication with Azure Storage.' + ), + ] class DefaultDestinationRequest(BaseModel): - model_config = ConfigDict(extra='forbid', ) - destination_id: str = Field( - ..., description='The ID of the default destination.') + model_config = ConfigDict( + extra='allow', + ) + destination_id: Annotated[ + str, Field(description='The ID of the default destination.') + ] class DestinationType(Enum): @@ -94,251 +116,349 @@ class DestinationType(Enum): class Error(BaseModel): + model_config = ConfigDict( + extra='allow', + ) code: int message: str class GoogleCloudStorageParams(BaseModel): - model_config = ConfigDict(extra='forbid', ) - bucket: str = Field( - ..., - description= - 'The name of the Google Cloud Storage bucket where data will be delivered.', - ) - credentials: str = Field( - ..., - description= - "Base64-encoded service account JSON credentials for Google Cloud Storage access.\n\nTo encode the credentials: `cat service-account.json | base64 | tr -d '\\n'`\n", + model_config = ConfigDict( + extra='allow', ) + bucket: Annotated[ + str, + Field( + description='The name of the Google Cloud Storage bucket where data will be delivered.' + ), + ] + credentials: Annotated[ + str, + Field( + description="Base64-encoded service account JSON credentials for Google Cloud Storage access.\n\nTo encode the credentials: `cat service-account.json | base64 | tr -d '\\n'`\n" + ), + ] class GoogleCloudStoragePatchParams(BaseModel): - model_config = ConfigDict(extra='forbid', ) - credentials: str = Field( - ..., - description= - "Base64-encoded service account JSON credentials for Google Cloud Storage access.\n\nTo encode the credentials: `cat service-account.json | base64 | tr -d '\\n'`\n", + model_config = ConfigDict( + extra='allow', ) + credentials: Annotated[ + str, + Field( + description="Base64-encoded service account JSON credentials for Google Cloud Storage access.\n\nTo encode the credentials: `cat service-account.json | base64 | tr -d '\\n'`\n" + ), + ] class Links(BaseModel): - field_self: str = Field( - ..., - alias='_self', - description='RFC 3986 URI representing the location of this object.', + model_config = ConfigDict( + extra='allow', ) + field_self: Annotated[ + str, + Field( + alias='_self', + description='RFC 3986 URI representing the location of this object.', + ), + ] class OracleCloudStorageParams(BaseModel): - model_config = ConfigDict(extra='forbid', ) - bucket: str = Field( - ..., - description= - 'The name of the Oracle Cloud Storage bucket where data will be delivered.', - ) - customer_access_key_id: str = Field( - ..., - description= - 'Customer access key ID for authentication with Oracle Cloud Storage.', + model_config = ConfigDict( + extra='allow', ) - customer_secret_key: str = Field( - ..., - description= - 'Customer secret key for authentication with Oracle Cloud Storage.', - ) - namespace: str = Field( - ..., - description= - 'The Oracle Object Storage namespace that contains the bucket.') - region: str = Field( - ..., - description='The Oracle Cloud region where the bucket is located.') + bucket: Annotated[ + str, + Field( + description='The name of the Oracle Cloud Storage bucket where data will be delivered.' + ), + ] + customer_access_key_id: Annotated[ + str, + Field( + description='Customer access key ID for authentication with Oracle Cloud Storage.' + ), + ] + customer_secret_key: Annotated[ + str, + Field( + description='Customer secret key for authentication with Oracle Cloud Storage.' + ), + ] + namespace: Annotated[ + str, + Field( + description='The Oracle Object Storage namespace that contains the bucket.' + ), + ] + region: Annotated[ + str, Field(description='The Oracle Cloud region where the bucket is located.') + ] class OracleCloudStoragePatchParams(BaseModel): - model_config = ConfigDict(extra='forbid', ) - customer_access_key_id: str = Field( - ..., - description= - 'Customer access key ID for authentication with Oracle Cloud Storage.', - ) - customer_secret_key: str = Field( - ..., - description= - 'Customer secret key for authentication with Oracle Cloud Storage.', + model_config = ConfigDict( + extra='allow', ) + customer_access_key_id: Annotated[ + str, + Field( + description='Customer access key ID for authentication with Oracle Cloud Storage.' + ), + ] + customer_secret_key: Annotated[ + str, + Field( + description='Customer secret key for authentication with Oracle Cloud Storage.' + ), + ] class Ownership(BaseModel): - is_owner: bool = Field( - ..., description='True if the user is the creator of the destination.') - owner_id: int = Field( - ..., description='The ID of the user who created the destination.') + model_config = ConfigDict( + extra='allow', + ) + is_owner: Annotated[ + bool, Field(description='True if the user is the creator of the destination.') + ] + owner_id: Annotated[ + int, Field(description='The ID of the user who created the destination.') + ] class Permissions(BaseModel): - can_write: bool = Field( - ..., - description='True if the user can write to the destination (patch).') + model_config = ConfigDict( + extra='allow', + ) + can_write: Annotated[ + bool, + Field(description='True if the user can write to the destination (patch).'), + ] class S3CompatibleParams(BaseModel): - model_config = ConfigDict(extra='forbid', ) - access_key_id: str = Field( - ..., - description= - 'Access key ID for authentication with the S3-compatible service.', - ) - bucket: str = Field( - ..., - description= - 'The name of the S3-compatible bucket where data will be delivered.', - ) - endpoint: str = Field( - ..., - description='The URL endpoint for the S3-compatible storage service.') - region: str = Field( - ..., - description= - 'The region identifier for the S3-compatible storage service.') - secret_access_key: str = Field( - ..., - description= - 'Secret access key for authentication with the S3-compatible service.', - ) - use_path_style: bool | None = Field( - False, - description= - 'Use path-style URL addressing with the bucket name in the URL path.', + model_config = ConfigDict( + extra='allow', ) + access_key_id: Annotated[ + str, + Field( + description='Access key ID for authentication with the S3-compatible service.' + ), + ] + bucket: Annotated[ + str, + Field( + description='The name of the S3-compatible bucket where data will be delivered.' + ), + ] + endpoint: Annotated[ + str, + Field(description='The URL endpoint for the S3-compatible storage service.'), + ] + region: Annotated[ + str, + Field( + description='The region identifier for the S3-compatible storage service.' + ), + ] + secret_access_key: Annotated[ + str, + Field( + description='Secret access key for authentication with the S3-compatible service.' + ), + ] + use_path_style: Annotated[ + bool, + Field( + description='Use path-style URL addressing with the bucket name in the URL path.' + ), + ] = False class S3CompatiblePatchParams(BaseModel): - model_config = ConfigDict(extra='forbid', ) - access_key_id: str = Field( - ..., - description= - 'Access key ID for authentication with the S3-compatible service.', + model_config = ConfigDict( + extra='allow', ) - secret_access_key: str = Field( - ..., - description= - 'Secret access key for authentication with the S3-compatible service.', - ) - use_path_style: bool | None = Field( - False, - description= - 'Use path-style URL addressing with the bucket name in the URL path.', - ) - - -class DestinationParameters(RootModel[GoogleCloudStorageParams - | AmazonS3Params - | AzureCloudStorageParams - | OracleCloudStorageParams - | S3CompatibleParams]): - root: (GoogleCloudStorageParams - | AmazonS3Params - | AzureCloudStorageParams - | OracleCloudStorageParams - | S3CompatibleParams) = Field( - ..., description='Parameters for the given Destination type.') - - -class DestinationPatchParameters(RootModel[GoogleCloudStoragePatchParams - | AmazonS3PatchParams - | AzureCloudStoragePatchParams - | OracleCloudStoragePatchParams - | S3CompatiblePatchParams]): - root: (GoogleCloudStoragePatchParams - | AmazonS3PatchParams - | AzureCloudStoragePatchParams - | OracleCloudStoragePatchParams - | S3CompatiblePatchParams) = Field( - ..., - description='Patch parameters for the given Destination type.') + access_key_id: Annotated[ + str, + Field( + description='Access key ID for authentication with the S3-compatible service.' + ), + ] + secret_access_key: Annotated[ + str, + Field( + description='Secret access key for authentication with the S3-compatible service.' + ), + ] + use_path_style: Annotated[ + bool, + Field( + description='Use path-style URL addressing with the bucket name in the URL path.' + ), + ] = False + + +class DestinationParameters( + RootModel[ + GoogleCloudStorageParams + | AmazonS3Params + | AzureCloudStorageParams + | OracleCloudStorageParams + | S3CompatibleParams + ] +): + root: Annotated[ + GoogleCloudStorageParams | AmazonS3Params | AzureCloudStorageParams | OracleCloudStorageParams | S3CompatibleParams, + Field(description='Parameters for the given Destination type.'), + ] + + +class DestinationPatchParameters( + RootModel[ + GoogleCloudStoragePatchParams + | AmazonS3PatchParams + | AzureCloudStoragePatchParams + | OracleCloudStoragePatchParams + | S3CompatiblePatchParams + ] +): + root: Annotated[ + GoogleCloudStoragePatchParams | AmazonS3PatchParams | AzureCloudStoragePatchParams | OracleCloudStoragePatchParams | S3CompatiblePatchParams, + Field(description='Patch parameters for the given Destination type.'), + ] class DestinationPatchRequest1(BaseModel): - model_config = ConfigDict(extra='forbid', ) - archive: bool | None = Field( - None, - description='True to archive the destination, false to unarchive.') + model_config = ConfigDict( + extra='allow', + ) + archive: Annotated[ + bool | None, + Field(description='True to archive the destination, false to unarchive.'), + ] = None name: Annotated[ - str, StringConstraints(min_length=3, max_length=63)] | None = Field( - None, description='A string to uniquely identify a Destination.') + str | None, + Field( + description='A string to uniquely identify a Destination.', + max_length=63, + min_length=3, + ), + ] = None parameters: DestinationPatchParameters class DestinationPatchRequest2(BaseModel): - model_config = ConfigDict(extra='forbid', ) - archive: bool = Field( - ..., - description='True to archive the destination, false to unarchive.') + model_config = ConfigDict( + extra='allow', + ) + archive: Annotated[ + bool, Field(description='True to archive the destination, false to unarchive.') + ] name: Annotated[ - str, StringConstraints(min_length=3, max_length=63)] | None = Field( - None, description='A string to uniquely identify a Destination.') + str | None, + Field( + description='A string to uniquely identify a Destination.', + max_length=63, + min_length=3, + ), + ] = None parameters: DestinationPatchParameters | None = None class DestinationPatchRequest3(BaseModel): - model_config = ConfigDict(extra='forbid', ) - archive: bool | None = Field( - None, - description='True to archive the destination, false to unarchive.') + model_config = ConfigDict( + extra='allow', + ) + archive: Annotated[ + bool | None, + Field(description='True to archive the destination, false to unarchive.'), + ] = None name: Annotated[ - str, StringConstraints(min_length=3, max_length=63)] = Field( - ..., description='A string to uniquely identify a Destination.') + str, + Field( + description='A string to uniquely identify a Destination.', + max_length=63, + min_length=3, + ), + ] parameters: DestinationPatchParameters | None = None -class DestinationPatchRequest(RootModel[DestinationPatchRequest1 - | DestinationPatchRequest2 - | DestinationPatchRequest3]): - root: ( - DestinationPatchRequest1 | DestinationPatchRequest2 +class DestinationPatchRequest( + RootModel[ + DestinationPatchRequest1 + | DestinationPatchRequest2 | DestinationPatchRequest3 - ) = Field( - ..., - description= - 'A DestinationPatchRequest is an object describing how to update a Destination.', - title='Destination patch request') + ] +): + root: Annotated[ + DestinationPatchRequest1 | DestinationPatchRequest2 | DestinationPatchRequest3, + Field( + description='A DestinationPatchRequest is an object describing how to update a Destination.', + title='Destination patch request', + ), + ] class DestinationRequest(BaseModel): - model_config = ConfigDict(extra='forbid', ) + model_config = ConfigDict( + extra='allow', + ) name: Annotated[ - str, StringConstraints(min_length=3, max_length=63)] | None = Field( - None, description='A name given to this Destination.') + str | None, + Field( + description='A name given to this Destination.', max_length=63, min_length=3 + ), + ] = None parameters: DestinationParameters type: DestinationType class Destination(BaseModel): - field_links: Links = Field(..., alias='_links') - archived: AwareDatetime | None = Field( - None, description='Timestamp when the Destination was archived.') - created: AwareDatetime = Field( - ..., description='Timestamp when the Destination was created.') - default: bool | None = Field( - None, - description= - 'True if this is the default destination for the organization.') - id: str = Field(..., - description='A string to uniquely identify a Destination.') - name: str = Field(..., description='A name given to this Destination.') + model_config = ConfigDict( + extra='allow', + ) + field_links: Annotated[Links, Field(alias='_links')] + archived: Annotated[ + AwareDatetime | None, + Field(description='Timestamp when the Destination was archived.'), + ] + created: Annotated[ + AwareDatetime, Field(description='Timestamp when the Destination was created.') + ] + default: Annotated[ + bool | None, + Field( + description='True if this is the default destination for the organization.' + ), + ] = False + id: Annotated[ + str, Field(description='A string to uniquely identify a Destination.') + ] + name: Annotated[str, Field(description='A name given to this Destination.')] ownership: Ownership parameters: DestinationParameters permissions: Permissions - pl_ref: str = Field(..., - alias='pl:ref', - description='A reference for the destination.') + pl_ref: Annotated[ + str, Field(alias='pl:ref', description='A reference for the destination.') + ] type: DestinationType - updated: AwareDatetime = Field( - ..., description='Timestamp when the Destination was last updated.') + updated: Annotated[ + AwareDatetime, + Field(description='Timestamp when the Destination was last updated.'), + ] class DestinationsResponse(BaseModel): - field_links: Links = Field(..., alias='_links') - destinations: list[Destination] = Field( - ..., description='Array of Destinations.') + model_config = ConfigDict( + extra='allow', + ) + field_links: Annotated[Links, Field(alias='_links')] + destinations: Annotated[ + list[Destination], Field(description='Array of Destinations.') + ] diff --git a/planet/cli/destinations.py b/planet/cli/destinations.py index 5c37e706f..a38667cc4 100644 --- a/planet/cli/destinations.py +++ b/planet/cli/destinations.py @@ -31,7 +31,11 @@ async def _patch_destination(ctx, destination_id, data, pretty): async with destinations_client(ctx) as cl: try: response = await cl.patch_destination(destination_id, data) - echo_json(response.model_dump(mode='json', by_alias=True), pretty) + echo_json( + response.model_dump(mode='json', + by_alias=True, + exclude_unset=True), + pretty) except Exception as e: raise ClickException(f"Failed to patch destination: {e}") @@ -48,7 +52,11 @@ async def _list_destinations(ctx, is_owner, can_write, is_default) - echo_json(response.model_dump(mode='json', by_alias=True), pretty) + echo_json( + response.model_dump(mode='json', + by_alias=True, + exclude_unset=True), + pretty) except Exception as e: raise ClickException(f"Failed to list destinations: {e}") @@ -57,7 +65,11 @@ async def _get_destination(ctx, destination_id, pretty): async with destinations_client(ctx) as cl: try: response = await cl.get_destination(destination_id) - echo_json(response.model_dump(mode='json', by_alias=True), pretty) + echo_json( + response.model_dump(mode='json', + by_alias=True, + exclude_unset=True), + pretty) except Exception as e: raise ClickException(f"Failed to get destination: {e}") @@ -66,7 +78,11 @@ async def _create_destination(ctx, data, pretty): async with destinations_client(ctx) as cl: try: response = await cl.create_destination(data) - echo_json(response.model_dump(mode='json', by_alias=True), pretty) + echo_json( + response.model_dump(mode='json', + by_alias=True, + exclude_unset=True), + pretty) except Exception as e: raise ClickException(f"Failed to create destination: {e}") @@ -75,7 +91,11 @@ async def _set_default_destination(ctx, destination_id, pretty): async with destinations_client(ctx) as cl: try: response = await cl.set_default_destination(destination_id) - echo_json(response.model_dump(mode='json', by_alias=True), pretty) + echo_json( + response.model_dump(mode='json', + by_alias=True, + exclude_unset=True), + pretty) except Exception as e: raise ClickException(f"Failed to set default destination: {e}") @@ -83,10 +103,8 @@ async def _set_default_destination(ctx, destination_id, pretty): async def _unset_default_destination(ctx, pretty): async with destinations_client(ctx) as cl: try: - response = await cl.unset_default_destination() - if response is not None: - echo_json(response.model_dump(mode='json', by_alias=True), - pretty) + await cl.unset_default_destination() + echo_json(None, pretty) except Exception as e: raise ClickException(f"Failed to unset default destination: {e}") @@ -95,7 +113,11 @@ async def _get_default_destination(ctx, pretty): async with destinations_client(ctx) as cl: try: response = await cl.get_default_destination() - echo_json(response.model_dump(mode='json', by_alias=True), pretty) + echo_json( + response.model_dump(mode='json', + by_alias=True, + exclude_unset=True), + pretty) except Exception as e: raise ClickException(f"Failed to get default destination: {e}") diff --git a/planet/sync/destinations.py b/planet/sync/destinations.py index b688c792c..444421365 100644 --- a/planet/sync/destinations.py +++ b/planet/sync/destinations.py @@ -48,7 +48,7 @@ def list_destinations( is_default (bool): If True, include only the default destination. Returns: - dict: A dictionary containing the list of destinations inside the 'destinations' key. + DestinationsResponse: The matching destinations, in its `destinations` attribute. Raises: APIError: If the API returns an error response. @@ -68,7 +68,7 @@ def get_destination(self, destination_id: str) -> Destination: destination_id (str): The ID of the destination to retrieve. Returns: - dict: A dictionary containing the destination details. + Destination: The destination details. Raises: APIError: If the API returns an error response. @@ -87,7 +87,7 @@ def patch_destination(self, destination_ref: str, request (dict): Destination content to update, only attributes to update are required. Returns: - dict: A dictionary containing the updated destination details. + Destination: The updated destination details. Raises: APIError: If the API returns an error response. @@ -104,7 +104,7 @@ def create_destination(self, request: Dict[str, Any]) -> Destination: request (dict): Destination content to create, all attributes are required. Returns: - dict: A dictionary containing the created destination details. + Destination: The created destination details. Raises: APIError: If the API returns an error response. @@ -123,7 +123,7 @@ def set_default_destination(self, destination_id: str) -> Destination: destination_id (str): The ID of the destination to set as default. Returns: - dict: A dictionary containing the default destination details. + Destination: The default destination details. Raises: APIError: If the API returns an error response. @@ -153,7 +153,7 @@ def get_default_destination(self) -> Destination: organization. Returns: - dict: A dictionary containing the default destination details. + Destination: The default destination details. Raises: APIError: If the API returns an error response. diff --git a/pyproject.toml b/pyproject.toml index c1f6ae592..2e8d7fee5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -44,7 +44,9 @@ test = [ ] validate_models = [ "pytest==8.3.3", - "datamodel-code-generator[http]>=0.25", + # Pinned exactly: the drift check byte-compares regenerated output, so a + # codegen release that changes formatting would fail it and block a release. + "datamodel-code-generator[http]==0.79.0", ] lint = [ "flake8", diff --git a/tests/drift/codegen_config.py b/tests/drift/codegen_config.py new file mode 100644 index 000000000..fce8b0c54 --- /dev/null +++ b/tests/drift/codegen_config.py @@ -0,0 +1,78 @@ +# Copyright 2024 Planet Labs PBC. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may not +# use this file except in compliance with the License. You may obtain a copy of +# the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations under +# the License. +"""Single source of truth for the Pydantic model codegen invocation. + +Both `nox -s generate_models` and the drift test import this module. The drift +test byte-compares regenerated output against the committed models, so the two +must build an identical command line from an identical version of +datamodel-code-generator (pinned in the `validate_models` extra). +""" +import pathlib + +REPO_ROOT = pathlib.Path(__file__).parent.parent.parent +MODELS_DIR = REPO_ROOT / "planet" / "api_models" + +# TODO: extend to other APIs as Pydantic models are adopted: +# "subscriptions": "https://api.planet.com/subscriptions/v1/spec", +# "orders": "https://api.planet.com/compute/ops/spec", +# "data": "https://api.planet.com/data/v1/spec", +SPECS = { + "destinations": "https://api.planet.com/destinations/v1/spec", +} + +HEADER = ("# flake8: noqa\n" + "# fmt: off\n" + "# Generated code — do not edit manually.\n" + "# Reformatting this file will break `nox -s validate_models`.\n" + "# To regenerate, run:\n" + "# nox -s generate_models") + + +def codegen_argv(url: str, output: pathlib.Path) -> list: + """Build the datamodel-codegen command line for one spec.""" + return [ + "datamodel-codegen", + "--url", + url, + "--input-file-type", + "openapi", + "--output", + str(output), + "--output-model-type", + "pydantic_v2.BaseModel", + # Responses must tolerate fields Planet adds to the API. Without this, + # an additive server change raises ValidationError in a shipped SDK. + "--extra-fields", + "allow", + # Express constraints as Annotated[str, Field(max_length=...)] rather + # than constr(...), which mypy rejects as an annotation in the modules + # that import these models. + "--use-annotated", + # Pinned, not inferred from the interpreter running codegen: output + # differs between Python versions, which would fail the drift check. + # 3.10 is the project's requires-python floor. + "--target-python-version", + "3.10", + # The spec is OpenAPI 3.0.3 and marks fields such as Destination.archived + # as both required and `nullable: true`. Without this, codegen drops the + # nullability and the model rejects the null the API actually returns. + "--strict-nullable", + # Honour schema-level defaults on required fields (e.g. Destination.default + # defaults to false), which the API omits rather than sending explicitly. + "--use-default", + "--custom-file-header", + HEADER, + "--formatters", + "builtin", + ] diff --git a/tests/drift/validate_models.py b/tests/drift/validate_models.py index 232625e03..ab371dbba 100644 --- a/tests/drift/validate_models.py +++ b/tests/drift/validate_models.py @@ -18,6 +18,9 @@ - The output is compared against the committed file in planet/api_models/. - The test fails if they differ, indicating the spec has changed. +The committed models are raw codegen output. They are excluded from yapf and +flake8 (see noxfile.py) because reformatting them would break this comparison. + When a test fails: 1. Review what changed in the spec. 2. Regenerate the committed models: @@ -25,44 +28,19 @@ 3. Update the client code if the API change requires it. 4. Commit the updated models. """ +import difflib import pathlib import subprocess import tempfile import pytest -REPO_ROOT = pathlib.Path(__file__).parent.parent.parent -MODELS_DIR = REPO_ROOT / "planet" / "api_models" - -HEADER = ("# flake8: noqa\n" - "# fmt: off\n" - "# Generated code — do not edit manually.\n" - "# To regenerate, run:\n" - "# nox -s generate_models\n" - "# Requires: uv tool install 'datamodel-code-generator[http]'") - -SPECS = { - "destinations": "https://api.planet.com/destinations/v1/spec", -} +from codegen_config import MODELS_DIR, SPECS, codegen_argv def _regenerate(url: str, output: pathlib.Path) -> None: result = subprocess.run( - [ - "datamodel-codegen", - "--url", - url, - "--input-file-type", - "openapi", - "--output", - str(output), - "--output-model-type", - "pydantic_v2.BaseModel", - "--custom-file-header", - HEADER, - "--formatters", - "builtin", - ], + codegen_argv(url, output), capture_output=True, text=True, ) @@ -84,9 +62,16 @@ def test_models_match_spec(name, url): current = committed.read_text() if generated != current: + diff = "".join( + difflib.unified_diff( + current.splitlines(keepends=True), + generated.splitlines(keepends=True), + fromfile=f"committed/{name}.py", + tofile=f"regenerated/{name}.py", + )) pytest.fail( f"planet/api_models/{name}.py is out of date with the live spec.\n" - f"Run `nox -s generate_models` to regenerate, then commit the result." - ) + f"Run `nox -s generate_models` to regenerate, then commit the result.\n\n" + f"{diff}") finally: tmp_path.unlink(missing_ok=True) From d474667f6adc65ca8e20cc8a699ddeb23cb1b4a2 Mon Sep 17 00:00:00 2001 From: Regan-Koopmans Date: Fri, 11 Sep 2026 10:38:44 +0200 Subject: [PATCH 03/10] Update year --- tests/drift/validate_models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/drift/validate_models.py b/tests/drift/validate_models.py index ab371dbba..c577b47db 100644 --- a/tests/drift/validate_models.py +++ b/tests/drift/validate_models.py @@ -1,4 +1,4 @@ -# Copyright 2024 Planet Labs PBC. +# Copyright 2026 Planet Labs PBC. # # Licensed under the Apache License, Version 2.0 (the "License"); you may not # use this file except in compliance with the License. You may obtain a copy of From ed019a3680f5935711482f8119943070dec1bee3 Mon Sep 17 00:00:00 2001 From: Regan-Koopmans Date: Wed, 16 Sep 2026 10:43:48 +0200 Subject: [PATCH 04/10] Address PR review: dict responses by default, opt-in Pydantic models - 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. --- noxfile.py | 13 ++++- planet/api_models/destinations.py | 55 +------------------- planet/cli/destinations.py | 36 +++---------- planet/clients/destinations.py | 37 +++++++------ planet/sync/destinations.py | 25 +++++---- setup.cfg | 2 +- tests/drift/codegen_config.py | 60 ++++++++++++++++++++-- tests/drift/validate_models.py | 26 +++++++--- tests/integration/test_destinations_api.py | 27 +++------- 9 files changed, 132 insertions(+), 149 deletions(-) diff --git a/noxfile.py b/noxfile.py index 40b8414b5..aa5cd221c 100644 --- a/noxfile.py +++ b/noxfile.py @@ -139,12 +139,23 @@ def generate_models(session): """ session.install("-e", ".[validate_models]") + import json + import tempfile + sys.path.insert(0, str(Path(__file__).parent / "tests" / "drift")) import codegen_config for name, url in codegen_config.SPECS.items(): output = Path("planet/api_models") / f"{name}.py" - session.run(*codegen_config.codegen_argv(url, output)) + spec = codegen_config.fetch_and_patch_spec(url) + with tempfile.NamedTemporaryFile( + suffix=".json", delete=False, mode="w") as spec_tmp: + json.dump(spec, spec_tmp) + spec_path = Path(spec_tmp.name) + try: + session.run(*codegen_config.codegen_argv(spec_path, output)) + finally: + spec_path.unlink(missing_ok=True) @nox.session diff --git a/planet/api_models/destinations.py b/planet/api_models/destinations.py index b2685db22..cb58de017 100644 --- a/planet/api_models/destinations.py +++ b/planet/api_models/destinations.py @@ -333,7 +333,7 @@ class DestinationPatchParameters( ] -class DestinationPatchRequest1(BaseModel): +class DestinationPatchRequest(BaseModel): model_config = ConfigDict( extra='allow', ) @@ -349,62 +349,9 @@ class DestinationPatchRequest1(BaseModel): min_length=3, ), ] = None - parameters: DestinationPatchParameters - - -class DestinationPatchRequest2(BaseModel): - model_config = ConfigDict( - extra='allow', - ) - archive: Annotated[ - bool, Field(description='True to archive the destination, false to unarchive.') - ] - name: Annotated[ - str | None, - Field( - description='A string to uniquely identify a Destination.', - max_length=63, - min_length=3, - ), - ] = None - parameters: DestinationPatchParameters | None = None - - -class DestinationPatchRequest3(BaseModel): - model_config = ConfigDict( - extra='allow', - ) - archive: Annotated[ - bool | None, - Field(description='True to archive the destination, false to unarchive.'), - ] = None - name: Annotated[ - str, - Field( - description='A string to uniquely identify a Destination.', - max_length=63, - min_length=3, - ), - ] parameters: DestinationPatchParameters | None = None -class DestinationPatchRequest( - RootModel[ - DestinationPatchRequest1 - | DestinationPatchRequest2 - | DestinationPatchRequest3 - ] -): - root: Annotated[ - DestinationPatchRequest1 | DestinationPatchRequest2 | DestinationPatchRequest3, - Field( - description='A DestinationPatchRequest is an object describing how to update a Destination.', - title='Destination patch request', - ), - ] - - class DestinationRequest(BaseModel): model_config = ConfigDict( extra='allow', diff --git a/planet/cli/destinations.py b/planet/cli/destinations.py index a38667cc4..6589a2b9b 100644 --- a/planet/cli/destinations.py +++ b/planet/cli/destinations.py @@ -31,11 +31,7 @@ async def _patch_destination(ctx, destination_id, data, pretty): async with destinations_client(ctx) as cl: try: response = await cl.patch_destination(destination_id, data) - echo_json( - response.model_dump(mode='json', - by_alias=True, - exclude_unset=True), - pretty) + echo_json(response, pretty) except Exception as e: raise ClickException(f"Failed to patch destination: {e}") @@ -52,11 +48,7 @@ async def _list_destinations(ctx, is_owner, can_write, is_default) - echo_json( - response.model_dump(mode='json', - by_alias=True, - exclude_unset=True), - pretty) + echo_json(response, pretty) except Exception as e: raise ClickException(f"Failed to list destinations: {e}") @@ -65,11 +57,7 @@ async def _get_destination(ctx, destination_id, pretty): async with destinations_client(ctx) as cl: try: response = await cl.get_destination(destination_id) - echo_json( - response.model_dump(mode='json', - by_alias=True, - exclude_unset=True), - pretty) + echo_json(response, pretty) except Exception as e: raise ClickException(f"Failed to get destination: {e}") @@ -78,11 +66,7 @@ async def _create_destination(ctx, data, pretty): async with destinations_client(ctx) as cl: try: response = await cl.create_destination(data) - echo_json( - response.model_dump(mode='json', - by_alias=True, - exclude_unset=True), - pretty) + echo_json(response, pretty) except Exception as e: raise ClickException(f"Failed to create destination: {e}") @@ -91,11 +75,7 @@ async def _set_default_destination(ctx, destination_id, pretty): async with destinations_client(ctx) as cl: try: response = await cl.set_default_destination(destination_id) - echo_json( - response.model_dump(mode='json', - by_alias=True, - exclude_unset=True), - pretty) + echo_json(response, pretty) except Exception as e: raise ClickException(f"Failed to set default destination: {e}") @@ -113,11 +93,7 @@ async def _get_default_destination(ctx, pretty): async with destinations_client(ctx) as cl: try: response = await cl.get_default_destination() - echo_json( - response.model_dump(mode='json', - by_alias=True, - exclude_unset=True), - pretty) + echo_json(response, pretty) except Exception as e: raise ClickException(f"Failed to get default destination: {e}") diff --git a/planet/clients/destinations.py b/planet/clients/destinations.py index 7fdd046d5..3da739d9d 100644 --- a/planet/clients/destinations.py +++ b/planet/clients/destinations.py @@ -18,7 +18,6 @@ from planet.clients.base import _BaseClient from planet.exceptions import APIError, ClientError from planet.http import Session -from ..api_models.destinations import Destination, DestinationsResponse from ..constants import PLANET_BASE_URL BASE_URL = f'{PLANET_BASE_URL}/destinations/v1/' @@ -61,7 +60,7 @@ async def list_destinations( archived: Optional[bool] = None, is_owner: Optional[bool] = None, can_write: Optional[bool] = None, - is_default: Optional[bool] = None) -> DestinationsResponse: + is_default: Optional[bool] = None) -> Dict[str, Any]: """ List all destinations. By default, all non-archived destinations in the requesting user's org are returned. @@ -72,7 +71,7 @@ async def list_destinations( is_default (bool): If True, include only the default destination. Returns: - DestinationsResponse: The list of destinations. + dict: The list of destinations. Raises: APIError: If the API returns an error response. @@ -97,9 +96,9 @@ async def list_destinations( except ClientError: # pragma: no cover raise else: - return DestinationsResponse.model_validate(response.json()) + return response.json() - async def get_destination(self, destination_id: str) -> Destination: + async def get_destination(self, destination_id: str) -> Dict[str, Any]: """ Get a specific destination by its ID. @@ -107,7 +106,7 @@ async def get_destination(self, destination_id: str) -> Destination: destination_id (str): The ID of the destination to retrieve. Returns: - Destination: The destination details. + dict: The destination details. Raises: APIError: If the API returns an error response. @@ -121,11 +120,11 @@ async def get_destination(self, destination_id: str) -> Destination: except ClientError: # pragma: no cover raise else: - return Destination.model_validate(response.json()) + return response.json() async def patch_destination(self, destination_id: str, - request: Dict[str, Any]) -> Destination: + request: Dict[str, Any]) -> Dict[str, Any]: """ Update a specific destination by its ID. @@ -134,7 +133,7 @@ async def patch_destination(self, request (dict): Destination content to update, only attributes to update are required. Returns: - Destination: The updated destination details. + dict: The updated destination details. Raises: APIError: If the API returns an error response. @@ -150,9 +149,9 @@ async def patch_destination(self, except ClientError: # pragma: no cover raise else: - return Destination.model_validate(response.json()) + return response.json() - async def create_destination(self, request: Dict[str, Any]) -> Destination: + async def create_destination(self, request: Dict[str, Any]) -> Dict[str, Any]: """ Create a new destination. @@ -160,7 +159,7 @@ async def create_destination(self, request: Dict[str, Any]) -> Destination: request (dict): Destination content to create, all attributes are required. Returns: - Destination: The created destination details. + dict: The created destination details. Raises: APIError: If the API returns an error response. @@ -175,10 +174,10 @@ async def create_destination(self, request: Dict[str, Any]) -> Destination: except ClientError: # pragma: no cover raise else: - return Destination.model_validate(response.json()) + return response.json() async def set_default_destination(self, - destination_id: str) -> Destination: + destination_id: str) -> Dict[str, Any]: """ Set an existing destination as the default destination. Default destinations are globally available to all members of an organization. An organization can have zero or one default destination at any time. @@ -188,7 +187,7 @@ async def set_default_destination(self, destination_id (str): The ID of the destination to set as default. Returns: - Destination: The default destination details. + dict: The default destination details. Raises: APIError: If the API returns an error response. @@ -205,7 +204,7 @@ async def set_default_destination(self, except ClientError: # pragma: no cover raise else: - return Destination.model_validate(response.json()) + return response.json() async def unset_default_destination(self) -> None: """ @@ -227,13 +226,13 @@ async def unset_default_destination(self) -> None: except ClientError: # pragma: no cover raise - async def get_default_destination(self) -> Destination: + async def get_default_destination(self) -> Dict[str, Any]: """ Get the current default destination. The default destination is globally available to all members of an organization. Returns: - Destination: The default destination details. + dict: The default destination details. Raises: APIError: If the API returns an error response. @@ -247,4 +246,4 @@ async def get_default_destination(self) -> Destination: except ClientError: # pragma: no cover raise else: - return Destination.model_validate(response.json()) + return response.json() diff --git a/planet/sync/destinations.py b/planet/sync/destinations.py index 444421365..ff18088a1 100644 --- a/planet/sync/destinations.py +++ b/planet/sync/destinations.py @@ -14,7 +14,6 @@ from typing import Any, Dict, Optional from planet.clients.destinations import DestinationsClient -from planet.api_models.destinations import Destination, DestinationsResponse from planet.http import Session @@ -37,7 +36,7 @@ def list_destinations( archived: Optional[bool] = None, is_owner: Optional[bool] = None, can_write: Optional[bool] = None, - is_default: Optional[bool] = None) -> DestinationsResponse: + is_default: Optional[bool] = None) -> Dict[str, Any]: """ List all destinations. By default, all non-archived destinations in the requesting user's org are returned. @@ -48,7 +47,7 @@ def list_destinations( is_default (bool): If True, include only the default destination. Returns: - DestinationsResponse: The matching destinations, in its `destinations` attribute. + dict: The matching destinations. Raises: APIError: If the API returns an error response. @@ -60,7 +59,7 @@ def list_destinations( can_write, is_default)) - def get_destination(self, destination_id: str) -> Destination: + def get_destination(self, destination_id: str) -> Dict[str, Any]: """ Get a specific destination by its ID. @@ -68,7 +67,7 @@ def get_destination(self, destination_id: str) -> Destination: destination_id (str): The ID of the destination to retrieve. Returns: - Destination: The destination details. + dict: The destination details. Raises: APIError: If the API returns an error response. @@ -78,7 +77,7 @@ def get_destination(self, destination_id: str) -> Destination: self._client.get_destination(destination_id)) def patch_destination(self, destination_ref: str, - request: Dict[str, Any]) -> Destination: + request: Dict[str, Any]) -> Dict[str, Any]: """ Update a specific destination by its ref. @@ -87,7 +86,7 @@ def patch_destination(self, destination_ref: str, request (dict): Destination content to update, only attributes to update are required. Returns: - Destination: The updated destination details. + dict: The updated destination details. Raises: APIError: If the API returns an error response. @@ -96,7 +95,7 @@ def patch_destination(self, destination_ref: str, return self._client._call_sync( self._client.patch_destination(destination_ref, request)) - def create_destination(self, request: Dict[str, Any]) -> Destination: + def create_destination(self, request: Dict[str, Any]) -> Dict[str, Any]: """ Create a new destination. @@ -104,7 +103,7 @@ def create_destination(self, request: Dict[str, Any]) -> Destination: request (dict): Destination content to create, all attributes are required. Returns: - Destination: The created destination details. + dict: The created destination details. Raises: APIError: If the API returns an error response. @@ -113,7 +112,7 @@ def create_destination(self, request: Dict[str, Any]) -> Destination: return self._client._call_sync( self._client.create_destination(request)) - def set_default_destination(self, destination_id: str) -> Destination: + def set_default_destination(self, destination_id: str) -> Dict[str, Any]: """ Set an existing destination as the default destination. Default destinations are globally available to all members of an organization. An organization can have zero or one default destination at any time. @@ -123,7 +122,7 @@ def set_default_destination(self, destination_id: str) -> Destination: destination_id (str): The ID of the destination to set as default. Returns: - Destination: The default destination details. + dict: The default destination details. Raises: APIError: If the API returns an error response. @@ -147,13 +146,13 @@ def unset_default_destination(self) -> None: return self._client._call_sync( self._client.unset_default_destination()) - def get_default_destination(self) -> Destination: + def get_default_destination(self) -> Dict[str, Any]: """ Get the current default destination. The default destination is globally available to all members of an organization. Returns: - Destination: The default destination details. + dict: The default destination details. Raises: APIError: If the API returns an error response. diff --git a/setup.cfg b/setup.cfg index dfa8ff892..0e8fd61d0 100644 --- a/setup.cfg +++ b/setup.cfg @@ -1,5 +1,5 @@ [options] -packages = planet, planet.cli, planet.clients, planet.data, planet.sync +packages = planet, planet.api_models, planet.cli, planet.clients, planet.data, planet.sync [options.packages.find] exclude = examples, tests diff --git a/tests/drift/codegen_config.py b/tests/drift/codegen_config.py index fce8b0c54..88de4a54c 100644 --- a/tests/drift/codegen_config.py +++ b/tests/drift/codegen_config.py @@ -18,7 +18,9 @@ must build an identical command line from an identical version of datamodel-code-generator (pinned in the `validate_models` extra). """ +import json import pathlib +import urllib.request REPO_ROOT = pathlib.Path(__file__).parent.parent.parent MODELS_DIR = REPO_ROOT / "planet" / "api_models" @@ -39,12 +41,64 @@ "# nox -s generate_models") -def codegen_argv(url: str, output: pathlib.Path) -> list: +# Schema names whose anyOf blocks are pure required-field constraints +# (each entry has only a `required` key, no properties of its own). +# These exist solely to express "at least one of these fields must be set", +# which is a server-side validation rule. datamodel-codegen cannot represent +# that constraint cleanly: it generates N numbered classes (e.g. +# DestinationPatchRequest1/2/3) that are otherwise identical except for which +# field is marked required. +# +# We drop the anyOf during codegen so the generator emits a single, flat model +# with all fields optional. The constraint is still enforced server-side; the +# client SDK's job is to build and send the request, not to duplicate server +# validation in a way that produces unreadable generated names. +_DROP_CONSTRAINT_ANY_OF: set[str] = { + "DestinationPatchRequest", +} + + +def fetch_and_patch_spec(url: str) -> dict: + """Fetch an OpenAPI spec and strip pure-constraint anyOf blocks. + + Some schemas use ``anyOf`` exclusively to express "at least one of these + fields must be present", using inline objects that each carry only a + ``required`` key. datamodel-codegen cannot name these inline schemas and + falls back to numbered suffixes (``DestinationPatchRequest1``, etc.). + + This function removes those anyOf blocks before codegen so the generator + produces a single, flat model. The constraint is server-enforced; the + client SDK does not need to replicate it. + """ + with urllib.request.urlopen(url) as resp: + spec = json.loads(resp.read()) + + schemas = spec.get("components", {}).get("schemas", {}) + for schema_name in _DROP_CONSTRAINT_ANY_OF: + schema = schemas.get(schema_name) + if schema is None: + continue + # Only drop anyOf entries that are pure required-field constraints + # (no properties of their own). If an entry has properties it is a + # real subtype and must be kept. + cleaned = [ + entry for entry in schema.get("anyOf", []) + if "properties" in entry or "$ref" in entry + ] + if cleaned: + schema["anyOf"] = cleaned + else: + schema.pop("anyOf", None) + + return spec + + +def codegen_argv(input_file: pathlib.Path, output: pathlib.Path) -> list: """Build the datamodel-codegen command line for one spec.""" return [ "datamodel-codegen", - "--url", - url, + "--input", + str(input_file), "--input-file-type", "openapi", "--output", diff --git a/tests/drift/validate_models.py b/tests/drift/validate_models.py index c577b47db..50c56d95f 100644 --- a/tests/drift/validate_models.py +++ b/tests/drift/validate_models.py @@ -29,23 +29,33 @@ 4. Commit the updated models. """ import difflib +import json import pathlib import subprocess import tempfile import pytest -from codegen_config import MODELS_DIR, SPECS, codegen_argv +from codegen_config import MODELS_DIR, SPECS, codegen_argv, fetch_and_patch_spec def _regenerate(url: str, output: pathlib.Path) -> None: - result = subprocess.run( - codegen_argv(url, output), - capture_output=True, - text=True, - ) - if result.returncode != 0: - pytest.fail(f"datamodel-codegen failed:\n{result.stderr}") + spec = fetch_and_patch_spec(url) + with tempfile.NamedTemporaryFile( + suffix=".json", delete=False, mode="w") as spec_tmp: + json.dump(spec, spec_tmp) + spec_path = pathlib.Path(spec_tmp.name) + + try: + result = subprocess.run( + codegen_argv(spec_path, output), + capture_output=True, + text=True, + ) + if result.returncode != 0: + pytest.fail(f"datamodel-codegen failed:\n{result.stderr}") + finally: + spec_path.unlink(missing_ok=True) @pytest.mark.parametrize("name,url", SPECS.items()) diff --git a/tests/integration/test_destinations_api.py b/tests/integration/test_destinations_api.py index d395edf46..8b738deff 100644 --- a/tests/integration/test_destinations_api.py +++ b/tests/integration/test_destinations_api.py @@ -20,7 +20,6 @@ from planet import DestinationsClient, Session from planet.auth import Auth from planet.sync.destinations import DestinationsAPI -from planet.api_models.destinations import Destination, DestinationsResponse pytestmark = pytest.mark.anyio @@ -108,8 +107,7 @@ def construct_list_response(destinations): async def test_list_destinations(): mock_response(TEST_URL, construct_list_response(DEST_LIST)) - expected = DestinationsResponse.model_validate( - construct_list_response(DEST_LIST)) + expected = construct_list_response(DEST_LIST) def assertf(resp): assert resp == expected @@ -123,8 +121,7 @@ async def test_list_destinations_filtering(): mock_response(f"{TEST_URL}?archived=false&is_owner=true", construct_list_response([DEST_1])) - expected = DestinationsResponse.model_validate( - construct_list_response([DEST_1])) + expected = construct_list_response([DEST_1]) def assertf(resp): assert resp == expected @@ -139,10 +136,8 @@ async def test_get_destination(): url = f"{TEST_URL}/{id}" mock_response(url, DEST_1) - expected = Destination.model_validate(DEST_1) - def assertf(resp): - assert resp == expected + assert resp == DEST_1 assertf(await cl_async.get_destination(id)) assertf(cl_sync.get_destination(id)) @@ -155,10 +150,8 @@ async def test_create_destination(): method="post", status_code=HTTPStatus.CREATED) - expected = Destination.model_validate(DEST_1) - def assertf(resp): - assert resp == expected + assert resp == DEST_1 assertf(await cl_async.create_destination(DEST_1_REQ_PAYLOAD)) assertf(cl_sync.create_destination(DEST_1_REQ_PAYLOAD)) @@ -170,10 +163,8 @@ async def test_patch_destination(): url = f"{TEST_URL}/{id}" mock_response(url, DEST_2, method="patch") - expected = Destination.model_validate(DEST_2) - def assertf(resp): - assert resp == expected + assert resp == DEST_2 assertf(await cl_async.patch_destination(id, DEST_2_PATCH_PAYLOAD)) assertf(cl_sync.patch_destination(id, DEST_2_PATCH_PAYLOAD)) @@ -200,10 +191,8 @@ async def test_set_default_destination(): url = f"{TEST_URL}/default" mock_response(url, DEST_1, method="put") - expected = Destination.model_validate(DEST_1) - def assertf(resp): - assert resp == expected + assert resp == DEST_1 assertf(await cl_async.set_default_destination(id)) assertf(cl_sync.set_default_destination(id)) @@ -231,10 +220,8 @@ async def test_get_default_destination(): url = f"{TEST_URL}/default" mock_response(url, DEST_1) - expected = Destination.model_validate(DEST_1) - def assertf(resp): - assert resp == expected + assert resp == DEST_1 assertf(await cl_async.get_default_destination()) assertf(cl_sync.get_default_destination()) From a15016bdef66e9b8ec68e8135e45d3a7de80a569 Mon Sep 17 00:00:00 2001 From: Regan-Koopmans Date: Wed, 16 Sep 2026 10:45:39 +0200 Subject: [PATCH 05/10] Fix yapf formatting --- noxfile.py | 5 +++-- planet/clients/destinations.py | 3 ++- planet/sync/destinations.py | 11 +++++------ tests/drift/codegen_config.py | 1 - tests/drift/validate_models.py | 4 ++-- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/noxfile.py b/noxfile.py index aa5cd221c..7fef675b6 100644 --- a/noxfile.py +++ b/noxfile.py @@ -148,8 +148,9 @@ def generate_models(session): for name, url in codegen_config.SPECS.items(): output = Path("planet/api_models") / f"{name}.py" spec = codegen_config.fetch_and_patch_spec(url) - with tempfile.NamedTemporaryFile( - suffix=".json", delete=False, mode="w") as spec_tmp: + with tempfile.NamedTemporaryFile(suffix=".json", + delete=False, + mode="w") as spec_tmp: json.dump(spec, spec_tmp) spec_path = Path(spec_tmp.name) try: diff --git a/planet/clients/destinations.py b/planet/clients/destinations.py index 3da739d9d..f061fb5ae 100644 --- a/planet/clients/destinations.py +++ b/planet/clients/destinations.py @@ -151,7 +151,8 @@ async def patch_destination(self, else: return response.json() - async def create_destination(self, request: Dict[str, Any]) -> Dict[str, Any]: + async def create_destination(self, request: Dict[str, + Any]) -> Dict[str, Any]: """ Create a new destination. diff --git a/planet/sync/destinations.py b/planet/sync/destinations.py index ff18088a1..68f82c5c0 100644 --- a/planet/sync/destinations.py +++ b/planet/sync/destinations.py @@ -31,12 +31,11 @@ def __init__(self, session: Session, base_url: Optional[str] = None): self._client = DestinationsClient(session, base_url) - def list_destinations( - self, - archived: Optional[bool] = None, - is_owner: Optional[bool] = None, - can_write: Optional[bool] = None, - is_default: Optional[bool] = None) -> Dict[str, Any]: + def list_destinations(self, + archived: Optional[bool] = None, + is_owner: Optional[bool] = None, + can_write: Optional[bool] = None, + is_default: Optional[bool] = None) -> Dict[str, Any]: """ List all destinations. By default, all non-archived destinations in the requesting user's org are returned. diff --git a/tests/drift/codegen_config.py b/tests/drift/codegen_config.py index 88de4a54c..eea1d2cc4 100644 --- a/tests/drift/codegen_config.py +++ b/tests/drift/codegen_config.py @@ -40,7 +40,6 @@ "# To regenerate, run:\n" "# nox -s generate_models") - # Schema names whose anyOf blocks are pure required-field constraints # (each entry has only a `required` key, no properties of its own). # These exist solely to express "at least one of these fields must be set", diff --git a/tests/drift/validate_models.py b/tests/drift/validate_models.py index 50c56d95f..81c647c01 100644 --- a/tests/drift/validate_models.py +++ b/tests/drift/validate_models.py @@ -41,8 +41,8 @@ def _regenerate(url: str, output: pathlib.Path) -> None: spec = fetch_and_patch_spec(url) - with tempfile.NamedTemporaryFile( - suffix=".json", delete=False, mode="w") as spec_tmp: + with tempfile.NamedTemporaryFile(suffix=".json", delete=False, + mode="w") as spec_tmp: json.dump(spec, spec_tmp) spec_path = pathlib.Path(spec_tmp.name) From 3a01abbd5141f4d7e6e3038beaf990e473c2231e Mon Sep 17 00:00:00 2001 From: Regan-Koopmans Date: Wed, 16 Sep 2026 10:49:39 +0200 Subject: [PATCH 06/10] Restore original return types and docstrings in destinations clients 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. --- planet/clients/destinations.py | 54 ++++++++++++++++++---------------- planet/sync/destinations.py | 24 +++++++-------- 2 files changed, 41 insertions(+), 37 deletions(-) diff --git a/planet/clients/destinations.py b/planet/clients/destinations.py index f061fb5ae..1d1f0dad0 100644 --- a/planet/clients/destinations.py +++ b/planet/clients/destinations.py @@ -13,7 +13,7 @@ # the License. import logging -from typing import Any, Dict, Optional +from typing import Any, Dict, Optional, TypeVar from planet.clients.base import _BaseClient from planet.exceptions import APIError, ClientError @@ -24,6 +24,8 @@ LOGGER = logging.getLogger() +T = TypeVar("T") + DEFAULT_DESTINATION_REF = "pl:destinations/default" @@ -55,12 +57,11 @@ def __init__(self, """ super().__init__(session, base_url or BASE_URL) - async def list_destinations( - self, - archived: Optional[bool] = None, - is_owner: Optional[bool] = None, - can_write: Optional[bool] = None, - is_default: Optional[bool] = None) -> Dict[str, Any]: + async def list_destinations(self, + archived: Optional[bool] = None, + is_owner: Optional[bool] = None, + can_write: Optional[bool] = None, + is_default: Optional[bool] = None) -> Dict: """ List all destinations. By default, all non-archived destinations in the requesting user's org are returned. @@ -71,7 +72,7 @@ async def list_destinations( is_default (bool): If True, include only the default destination. Returns: - dict: The list of destinations. + dict: A dictionary containing the list of destinations inside the 'destinations' key. Raises: APIError: If the API returns an error response. @@ -96,9 +97,10 @@ async def list_destinations( except ClientError: # pragma: no cover raise else: - return response.json() + dest_response = response.json() + return dest_response - async def get_destination(self, destination_id: str) -> Dict[str, Any]: + async def get_destination(self, destination_id: str) -> Dict: """ Get a specific destination by its ID. @@ -106,7 +108,7 @@ async def get_destination(self, destination_id: str) -> Dict[str, Any]: destination_id (str): The ID of the destination to retrieve. Returns: - dict: The destination details. + dict: A dictionary containing the destination details. Raises: APIError: If the API returns an error response. @@ -120,11 +122,12 @@ async def get_destination(self, destination_id: str) -> Dict[str, Any]: except ClientError: # pragma: no cover raise else: - return response.json() + dest = response.json() + return dest async def patch_destination(self, destination_id: str, - request: Dict[str, Any]) -> Dict[str, Any]: + request: Dict[str, Any]) -> Dict: """ Update a specific destination by its ID. @@ -133,7 +136,7 @@ async def patch_destination(self, request (dict): Destination content to update, only attributes to update are required. Returns: - dict: The updated destination details. + dict: A dictionary containing the updated destination details. Raises: APIError: If the API returns an error response. @@ -149,10 +152,10 @@ async def patch_destination(self, except ClientError: # pragma: no cover raise else: - return response.json() + dest = response.json() + return dest - async def create_destination(self, request: Dict[str, - Any]) -> Dict[str, Any]: + async def create_destination(self, request: Dict[str, Any]) -> Dict: """ Create a new destination. @@ -160,7 +163,7 @@ async def create_destination(self, request: Dict[str, request (dict): Destination content to create, all attributes are required. Returns: - dict: The created destination details. + dict: A dictionary containing the created destination details. Raises: APIError: If the API returns an error response. @@ -175,10 +178,10 @@ async def create_destination(self, request: Dict[str, except ClientError: # pragma: no cover raise else: - return response.json() + dest = response.json() + return dest - async def set_default_destination(self, - destination_id: str) -> Dict[str, Any]: + async def set_default_destination(self, destination_id: str) -> Dict: """ Set an existing destination as the default destination. Default destinations are globally available to all members of an organization. An organization can have zero or one default destination at any time. @@ -188,7 +191,7 @@ async def set_default_destination(self, destination_id (str): The ID of the destination to set as default. Returns: - dict: The default destination details. + dict: A dictionary containing the default destination details. Raises: APIError: If the API returns an error response. @@ -227,13 +230,13 @@ async def unset_default_destination(self) -> None: except ClientError: # pragma: no cover raise - async def get_default_destination(self) -> Dict[str, Any]: + async def get_default_destination(self) -> Dict: """ Get the current default destination. The default destination is globally available to all members of an organization. Returns: - dict: The default destination details. + dict: A dictionary containing the default destination details. Raises: APIError: If the API returns an error response. @@ -247,4 +250,5 @@ async def get_default_destination(self) -> Dict[str, Any]: except ClientError: # pragma: no cover raise else: - return response.json() + dest = response.json() + return dest diff --git a/planet/sync/destinations.py b/planet/sync/destinations.py index 68f82c5c0..a95b2f977 100644 --- a/planet/sync/destinations.py +++ b/planet/sync/destinations.py @@ -35,7 +35,7 @@ def list_destinations(self, archived: Optional[bool] = None, is_owner: Optional[bool] = None, can_write: Optional[bool] = None, - is_default: Optional[bool] = None) -> Dict[str, Any]: + is_default: Optional[bool] = None) -> Dict: """ List all destinations. By default, all non-archived destinations in the requesting user's org are returned. @@ -46,7 +46,7 @@ def list_destinations(self, is_default (bool): If True, include only the default destination. Returns: - dict: The matching destinations. + dict: A dictionary containing the list of destinations inside the 'destinations' key. Raises: APIError: If the API returns an error response. @@ -58,7 +58,7 @@ def list_destinations(self, can_write, is_default)) - def get_destination(self, destination_id: str) -> Dict[str, Any]: + def get_destination(self, destination_id: str) -> Dict: """ Get a specific destination by its ID. @@ -66,7 +66,7 @@ def get_destination(self, destination_id: str) -> Dict[str, Any]: destination_id (str): The ID of the destination to retrieve. Returns: - dict: The destination details. + dict: A dictionary containing the destination details. Raises: APIError: If the API returns an error response. @@ -76,7 +76,7 @@ def get_destination(self, destination_id: str) -> Dict[str, Any]: self._client.get_destination(destination_id)) def patch_destination(self, destination_ref: str, - request: Dict[str, Any]) -> Dict[str, Any]: + request: Dict[str, Any]) -> Dict: """ Update a specific destination by its ref. @@ -85,7 +85,7 @@ def patch_destination(self, destination_ref: str, request (dict): Destination content to update, only attributes to update are required. Returns: - dict: The updated destination details. + dict: A dictionary containing the updated destination details. Raises: APIError: If the API returns an error response. @@ -94,7 +94,7 @@ def patch_destination(self, destination_ref: str, return self._client._call_sync( self._client.patch_destination(destination_ref, request)) - def create_destination(self, request: Dict[str, Any]) -> Dict[str, Any]: + def create_destination(self, request: Dict[str, Any]) -> Dict: """ Create a new destination. @@ -102,7 +102,7 @@ def create_destination(self, request: Dict[str, Any]) -> Dict[str, Any]: request (dict): Destination content to create, all attributes are required. Returns: - dict: The created destination details. + dict: A dictionary containing the created destination details. Raises: APIError: If the API returns an error response. @@ -111,7 +111,7 @@ def create_destination(self, request: Dict[str, Any]) -> Dict[str, Any]: return self._client._call_sync( self._client.create_destination(request)) - def set_default_destination(self, destination_id: str) -> Dict[str, Any]: + def set_default_destination(self, destination_id: str) -> Dict: """ Set an existing destination as the default destination. Default destinations are globally available to all members of an organization. An organization can have zero or one default destination at any time. @@ -121,7 +121,7 @@ def set_default_destination(self, destination_id: str) -> Dict[str, Any]: destination_id (str): The ID of the destination to set as default. Returns: - dict: The default destination details. + dict: A dictionary containing the default destination details. Raises: APIError: If the API returns an error response. @@ -145,13 +145,13 @@ def unset_default_destination(self) -> None: return self._client._call_sync( self._client.unset_default_destination()) - def get_default_destination(self) -> Dict[str, Any]: + def get_default_destination(self) -> Dict: """ Get the current default destination. The default destination is globally available to all members of an organization. Returns: - dict: The default destination details. + dict: A dictionary containing the default destination details. Raises: APIError: If the API returns an error response. From 545f419098a158a9603d1af2fff5f635d8958a1c Mon Sep 17 00:00:00 2001 From: Regan-Koopmans Date: Wed, 16 Sep 2026 10:57:19 +0200 Subject: [PATCH 07/10] Remove stale docs describing Pydantic model return types Clients now return plain dicts; the upgrading guide section describing attribute access and model_dump() no longer applies. --- docs/get-started/upgrading-v3.md | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/docs/get-started/upgrading-v3.md b/docs/get-started/upgrading-v3.md index 93fce1207..a8fc6c760 100644 --- a/docs/get-started/upgrading-v3.md +++ b/docs/get-started/upgrading-v3.md @@ -97,22 +97,4 @@ should be preferred to the use of Planet API keys. * Deprecated `planet.subscription_request.clip_tool()` method for defining custom clip AOIs with requests to create subscriptions. Subscriptions API no longer supports custom clip AOIs; instead users can opt-in to clip to their subscription source geometry by including kwarg `clip_to_source=True` when constructing requests via `planet.subscription_request.build_request()`. See [PR #1169](https://github.com/planetlabs/planet-client-python/pull/1169) for implementation details. * Renamed `planet.cli.subscriptions.request_pv()` to `planet.cli.subscriptions.request_source()`, and removed `var_type` positional argument from the signature. This change, in effect renames the CLI argument `planet subscriptions request-pv` to `planet subscriptions request-source`. Also renamed `planet.subscription_request.planetary_variable_source()` to `planet.subscription_request.subscription_source()`. Source type positional arguments are removed from these methods in favor of `source_id`. See [PR #1170](https://github.com/planetlabs/planet-client-python/pull/1170) for implementation details. -* The Destinations API methods on `DestinationsClient` and `DestinationsAPI` now return Pydantic models generated from Planet's OpenAPI spec instead of plain dictionaries. `list_destinations()` returns a `DestinationsResponse`; `get_destination()`, `create_destination()`, `patch_destination()`, `set_default_destination()` and `get_default_destination()` return a `Destination`. Attribute access replaces subscript access: - - ```python - # Version 2 - resp = pl.destinations.list_destinations() - for d in resp["destinations"]: - print(d["id"]) - - # Version 3 - resp = pl.destinations.list_destinations() - for d in resp.destinations: - print(d.id) - ``` - - Call `.model_dump(mode="json", by_alias=True)` on any of these to recover a - JSON-compatible dictionary. The models allow unknown fields, so destinations - returned by a newer version of the API still parse. - ---- From 7618f74322505b79e0744d9ca10c2e7f60e22382 Mon Sep 17 00:00:00 2001 From: Regan-Koopmans Date: Fri, 18 Sep 2026 11:14:39 +0200 Subject: [PATCH 08/10] Honour spec additionalProperties; move drift check to PR CI 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. --- .github/workflows/publish-pypi.yml | 6 +- .github/workflows/test.yml | 23 ++++++ planet/api_models/destinations.py | 16 ++-- tests/drift/codegen_config.py | 81 ++++++++++++++++--- tests/unit/test_api_models.py | 121 +++++++++++++++++++++++++++++ 5 files changed, 222 insertions(+), 25 deletions(-) create mode 100644 tests/unit/test_api_models.py diff --git a/.github/workflows/publish-pypi.yml b/.github/workflows/publish-pypi.yml index e45e53a08..3aa701868 100644 --- a/.github/workflows/publish-pypi.yml +++ b/.github/workflows/publish-pypi.yml @@ -24,13 +24,9 @@ jobs: restore-keys: | ${{ runner.os }}-pip - - name: Validate models against live API specs - run: | - pip install --upgrade nox - nox -s validate_models - - name: Build, verify, and upload to PyPI run: | + pip install --upgrade nox nox -s build publish_pypi env: TWINE_PASSWORD: ${{ secrets.PYPI_API_TOKEN }} diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 41858b9ff..0c7515c44 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -56,6 +56,29 @@ jobs: pip install --upgrade nox nox -s analyze + validate-models: + name: Validate Models Against Live Specs + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v3 + - name: Set up Python + uses: actions/setup-python@v4 + with: + python-version: 3.12 + - name: Pip cache + uses: actions/cache@v4 + with: + path: ~/.cache/pip + key: ${{ runner.os }}-pip + restore-keys: | + ${{ runner.os }}-pip + # Fetches live OpenAPI specs from api.planet.com. No API key required. + - name: Validate models + run: | + pip install --upgrade nox + nox -s validate_models + test: name: Test Python ${{ matrix.python-version }} runs-on: ubuntu-latest diff --git a/planet/api_models/destinations.py b/planet/api_models/destinations.py index cb58de017..deb0b6b3c 100644 --- a/planet/api_models/destinations.py +++ b/planet/api_models/destinations.py @@ -41,7 +41,7 @@ class AmazonS3Params(BaseModel): class AmazonS3PatchParams(BaseModel): model_config = ConfigDict( - extra='allow', + extra='forbid', ) aws_access_key_id: Annotated[ str, Field(description='AWS access key ID for authentication with Amazon S3.') @@ -88,7 +88,7 @@ class AzureCloudStorageParams(BaseModel): class AzureCloudStoragePatchParams(BaseModel): model_config = ConfigDict( - extra='allow', + extra='forbid', ) sas_token: Annotated[ str, @@ -100,7 +100,7 @@ class AzureCloudStoragePatchParams(BaseModel): class DefaultDestinationRequest(BaseModel): model_config = ConfigDict( - extra='allow', + extra='forbid', ) destination_id: Annotated[ str, Field(description='The ID of the default destination.') @@ -143,7 +143,7 @@ class GoogleCloudStorageParams(BaseModel): class GoogleCloudStoragePatchParams(BaseModel): model_config = ConfigDict( - extra='allow', + extra='forbid', ) credentials: Annotated[ str, @@ -201,7 +201,7 @@ class OracleCloudStorageParams(BaseModel): class OracleCloudStoragePatchParams(BaseModel): model_config = ConfigDict( - extra='allow', + extra='forbid', ) customer_access_key_id: Annotated[ str, @@ -281,7 +281,7 @@ class S3CompatibleParams(BaseModel): class S3CompatiblePatchParams(BaseModel): model_config = ConfigDict( - extra='allow', + extra='forbid', ) access_key_id: Annotated[ str, @@ -335,7 +335,7 @@ class DestinationPatchParameters( class DestinationPatchRequest(BaseModel): model_config = ConfigDict( - extra='allow', + extra='forbid', ) archive: Annotated[ bool | None, @@ -354,7 +354,7 @@ class DestinationPatchRequest(BaseModel): class DestinationRequest(BaseModel): model_config = ConfigDict( - extra='allow', + extra='forbid', ) name: Annotated[ str | None, diff --git a/tests/drift/codegen_config.py b/tests/drift/codegen_config.py index eea1d2cc4..079608cf3 100644 --- a/tests/drift/codegen_config.py +++ b/tests/drift/codegen_config.py @@ -57,17 +57,71 @@ } +def _schema_refs(node) -> list: + """Collect every ``components/schemas`` name referenced under a node.""" + found = [] + if isinstance(node, dict): + for key, value in node.items(): + if key == "$ref" and isinstance(value, str): + found.append(value.rsplit("/", 1)[-1]) + else: + found.extend(_schema_refs(value)) + elif isinstance(node, list): + for value in node: + found.extend(_schema_refs(value)) + return found + + +def response_reachable_schemas(spec: dict) -> set: + """Names of schemas the API can return, followed transitively from responses. + + Everything else is request-only. The two halves want opposite handling of + unknown fields, so codegen needs to tell them apart. + """ + schemas = spec.get("components", {}).get("schemas", {}) + + pending = [] + for path_item in spec.get("paths", {}).values(): + for operation in path_item.values(): + if isinstance(operation, dict): + pending.extend(_schema_refs(operation.get("responses", {}))) + + reachable: set = set() + while pending: + name = pending.pop() + if name in reachable or name not in schemas: + continue + reachable.add(name) + pending.extend(_schema_refs(schemas[name])) + return reachable + + def fetch_and_patch_spec(url: str) -> dict: - """Fetch an OpenAPI spec and strip pure-constraint anyOf blocks. + """Fetch an OpenAPI spec and patch it for codegen. - Some schemas use ``anyOf`` exclusively to express "at least one of these - fields must be present", using inline objects that each carry only a - ``required`` key. datamodel-codegen cannot name these inline schemas and - falls back to numbered suffixes (``DestinationPatchRequest1``, etc.). + Two patches, both applied before datamodel-codegen sees the spec. - This function removes those anyOf blocks before codegen so the generator - produces a single, flat model. The constraint is server-enforced; the - client SDK does not need to replicate it. + 1. Strip pure-constraint ``anyOf`` blocks. Some schemas use ``anyOf`` + exclusively to express "at least one of these fields must be present", + using inline objects that each carry only a ``required`` key. + datamodel-codegen cannot name these inline schemas and falls back to + numbered suffixes (``DestinationPatchRequest1``, etc.). Removing the + block yields a single, flat model. The constraint is server-enforced; + the client SDK does not need to replicate it. + + 2. Relax ``additionalProperties`` on response schemas. Codegen is run + without a global ``--extra-fields`` override, so it honours the spec: + ``additionalProperties: false`` becomes ``extra='forbid'``. That is + what we want for request models -- a typo'd key fails client side, + before the round trip. It is wrong for responses: a shipped SDK must + not raise when Planet adds a field. So every schema reachable from a + response is forced to ``additionalProperties: true``, giving + ``extra='allow'``. + + Note the overlap. ``AmazonS3Params`` and its siblings appear in both + requests and responses, so tolerance wins and they are generated as + ``allow``. Only ``*PatchParams`` and the top-level request bodies are + request-only, and those get ``forbid``. """ with urllib.request.urlopen(url) as resp: spec = json.loads(resp.read()) @@ -89,6 +143,13 @@ def fetch_and_patch_spec(url: str) -> dict: else: schema.pop("anyOf", None) + for name in response_reachable_schemas(spec): + schema = schemas[name] + # Enums and unions (oneOf/anyOf roots) carry no properties of their + # own; additionalProperties is meaningless there and confuses codegen. + if "properties" in schema: + schema["additionalProperties"] = True + return spec @@ -104,10 +165,6 @@ def codegen_argv(input_file: pathlib.Path, output: pathlib.Path) -> list: str(output), "--output-model-type", "pydantic_v2.BaseModel", - # Responses must tolerate fields Planet adds to the API. Without this, - # an additive server change raises ValidationError in a shipped SDK. - "--extra-fields", - "allow", # Express constraints as Annotated[str, Field(max_length=...)] rather # than constr(...), which mypy rejects as an annotation in the modules # that import these models. diff --git a/tests/unit/test_api_models.py b/tests/unit/test_api_models.py new file mode 100644 index 000000000..12a050fa6 --- /dev/null +++ b/tests/unit/test_api_models.py @@ -0,0 +1,121 @@ +# Copyright 2026 Planet Labs PBC. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may not +# use this file except in compliance with the License. You may obtain a copy of +# the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations under +# the License. +"""Contract tests for the generated Destinations models. + +These pin the two halves of the unknown-field policy set in +tests/drift/codegen_config.py. Request models reject unknown fields so a typo +fails client side. Response models accept them so an additive server change +does not break a shipped SDK. +""" +import pydantic +import pytest + +from planet.api_models.destinations import ( + AmazonS3PatchParams, + DefaultDestinationRequest, + Destination, + DestinationPatchRequest, + DestinationRequest, + DestinationsResponse, +) + +DEST = { + "id": "dest1", + "name": "Destination 1", + "type": "amazon_s3", + "parameters": { + "bucket": "bucket1", + "aws_region": "us-west-2", + "aws_access_key_id": "key1", + "aws_secret_access_key": "secret1", + }, + "created": "2024-01-01T00:00:00Z", + "updated": "2024-01-01T00:00:00Z", + "pl:ref": "ref", + "_links": { + "_self": "url" + }, + "archived": None, + "permissions": { + "can_write": True + }, + "ownership": { + "is_owner": True, "owner_id": 1 + }, +} + +REQUEST_MODELS = [ + (DestinationRequest, + { + "type": "amazon_s3", + "parameters": { + "bucket": "bucket1", + "aws_region": "us-west-2", + "aws_access_key_id": "key1", + "aws_secret_access_key": "secret1", + }, + }), + (DestinationPatchRequest, { + "archive": True + }), + (DefaultDestinationRequest, { + "destination_id": "dest1" + }), + (AmazonS3PatchParams, { + "aws_access_key_id": "key1", "aws_secret_access_key": "secret1" + }), +] + + +@pytest.mark.parametrize("model,payload", REQUEST_MODELS) +def test_request_model_accepts_valid_payload(model, payload): + assert model.model_validate(payload) + + +@pytest.mark.parametrize("model,payload", REQUEST_MODELS) +def test_request_model_rejects_unknown_field(model, payload): + """The spec marks these additionalProperties: false. Catch typos locally.""" + with pytest.raises(pydantic.ValidationError, match="extra_forbidden"): + model.model_validate({**payload, "buckett": "typo"}) + + +def test_response_model_tolerates_unknown_field(): + """An additive server change must not break a released SDK.""" + dest = Destination.model_validate({**DEST, "future_field": "value"}) + assert dest.id == "dest1" + assert dest.future_field == "value" + + +def test_response_model_tolerates_unknown_nested_param(): + """Params are echoed in responses, so they tolerate extras too.""" + params = {**DEST["parameters"], "future_param": "value"} + dest = Destination.model_validate({**DEST, "parameters": params}) + assert dest.parameters.root.future_param == "value" + + +def test_destinations_response_round_trips_aliases(): + """Serialization emits wire names, not Python field names.""" + response = DestinationsResponse.model_validate({ + "destinations": [DEST], "_links": { + "_self": "url" + } + }) + dumped = response.model_dump(mode="json", + by_alias=True, + exclude_unset=True) + + assert "_links" in dumped + assert dumped["destinations"][0]["pl:ref"] == "ref" + assert dumped["destinations"][0]["created"].startswith("2024-01-01") + assert "field_links" not in dumped["destinations"][0] From 02fcd0f2ece5a8285ef6835bb37836d2586b854d Mon Sep 17 00:00:00 2001 From: Regan-Koopmans Date: Mon, 21 Sep 2026 13:01:52 +0200 Subject: [PATCH 09/10] Make pydantic an optional extra; rename to planet.types; split codegen 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. --- README.md | 7 +++ noxfile.py | 8 +-- planet/api_models/__init__.py | 0 planet/types/__init__.py | 34 +++++++++++ planet/{api_models => types}/destinations.py | 0 pyproject.toml | 12 +++- {tests/drift => scripts}/codegen_config.py | 59 +++++++----------- scripts/codegen_constants.py | 63 ++++++++++++++++++++ setup.cfg | 2 +- tests/drift/conftest.py | 7 +++ tests/drift/validate_models.py | 4 +- tests/unit/test_api_models.py | 37 +++++++++++- 12 files changed, 184 insertions(+), 49 deletions(-) delete mode 100644 planet/api_models/__init__.py create mode 100644 planet/types/__init__.py rename planet/{api_models => types}/destinations.py (100%) rename {tests/drift => scripts}/codegen_config.py (76%) create mode 100644 scripts/codegen_constants.py create mode 100644 tests/drift/conftest.py diff --git a/README.md b/README.md index acad36f51..d0b5665a1 100644 --- a/README.md +++ b/README.md @@ -97,6 +97,13 @@ The Planet SDK for Python is [hosted on PyPI](https://pypi.org/project/planet/) pip install planet ``` +For optional typed request and response models, generated from Planet's OpenAPI +specs, install the `models` extra. It adds a `pydantic` dependency: + +```console +pip install planet[models] +``` + To install from source, first clone this repository, then navigate to the root directory (where `setup.py` lives) and run: ```console diff --git a/noxfile.py b/noxfile.py index 7fef675b6..d83544306 100644 --- a/noxfile.py +++ b/noxfile.py @@ -11,7 +11,7 @@ source_files = ("planet", "examples", "tests", "setup.py", "noxfile.py") # Generated code — excluded from linting and formatting checks -generated_dirs = ("planet/api_models", ) +generated_dirs = ("planet/types", ) BUILD_DIRS = ['build', 'dist'] @@ -128,7 +128,7 @@ def examples(session): @nox.session def generate_models(session): - """Re-generate Pydantic models for the Destinations API in planet/api_models/. + """Re-generate Pydantic models for the Destinations API in planet/types/. Uses the same pinned datamodel-code-generator as `nox -s validate_models`, so the committed output is byte-identical to what the drift check @@ -142,11 +142,11 @@ def generate_models(session): import json import tempfile - sys.path.insert(0, str(Path(__file__).parent / "tests" / "drift")) + sys.path.insert(0, str(Path(__file__).parent / "scripts")) import codegen_config for name, url in codegen_config.SPECS.items(): - output = Path("planet/api_models") / f"{name}.py" + output = Path("planet/types") / f"{name}.py" spec = codegen_config.fetch_and_patch_spec(url) with tempfile.NamedTemporaryFile(suffix=".json", delete=False, diff --git a/planet/api_models/__init__.py b/planet/api_models/__init__.py deleted file mode 100644 index e69de29bb..000000000 diff --git a/planet/types/__init__.py b/planet/types/__init__.py new file mode 100644 index 000000000..44439d145 --- /dev/null +++ b/planet/types/__init__.py @@ -0,0 +1,34 @@ +# Copyright 2026 Planet Labs PBC. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may not +# use this file except in compliance with the License. You may obtain a copy of +# the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations under +# the License. +"""Typed request and response models, generated from Planet's OpenAPI specs. + +Requires pydantic, which is an optional dependency: + + pip install planet[models] + +The rest of the SDK does not import this package. Clients return plain dicts; +these models are opt-in validation on top of them: + + from planet.types.destinations import Destination + + dest = Destination.model_validate(client.get_destination(dest_id)) + +To regenerate after a spec change, run `nox -s generate_models`. +""" +try: + import pydantic as _pydantic # noqa: F401 +except ImportError as exc: # pragma: no cover + raise ImportError( + "planet.types requires pydantic, which is not installed. " + "Install it with: pip install planet[models]") from exc diff --git a/planet/api_models/destinations.py b/planet/types/destinations.py similarity index 100% rename from planet/api_models/destinations.py rename to planet/types/destinations.py diff --git a/pyproject.toml b/pyproject.toml index 2e8d7fee5..6d0619677 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -11,7 +11,6 @@ dependencies = [ "geojson", "httpx>=0.28.0", "jsonschema", - "pydantic>=2.0", "pyjwt>=2.1", "tqdm>=4.56", "typing-extensions", @@ -35,7 +34,14 @@ license = { file = "LICENSE" } dynamic = ["version"] [project.optional-dependencies] +# Typed request/response models for planet.types.*. Optional because pydantic +# pulls in pydantic-core, a compiled Rust extension. The SDK itself does not +# import it: clients return plain dicts either way. +models = [ + "pydantic>=2.0", +] test = [ + "planet[models]", "pytest==8.3.3", "anyio", "pytest-cov", @@ -43,7 +49,7 @@ test = [ "coverage[toml]" ] validate_models = [ - "pytest==8.3.3", + "planet[test]", # Pinned exactly: the drift check byte-compares regenerated output, so a # codegen release that changes formatting would fail it and block a release. "datamodel-code-generator[http]==0.79.0", @@ -62,7 +68,7 @@ docs = [ "mkdocs-macros-plugin==1.3.7" ] dev = [ - "planet[test, docs, lint]", + "planet[test, docs, lint, validate_models]", ] [project.scripts] diff --git a/tests/drift/codegen_config.py b/scripts/codegen_config.py similarity index 76% rename from tests/drift/codegen_config.py rename to scripts/codegen_config.py index 079608cf3..9d486fc00 100644 --- a/tests/drift/codegen_config.py +++ b/scripts/codegen_config.py @@ -17,44 +17,30 @@ test byte-compares regenerated output against the committed models, so the two must build an identical command line from an identical version of datamodel-code-generator (pinned in the `validate_models` extra). + +Spec URLs, output paths and other settings live in codegen_constants. """ import json import pathlib import urllib.request -REPO_ROOT = pathlib.Path(__file__).parent.parent.parent -MODELS_DIR = REPO_ROOT / "planet" / "api_models" - -# TODO: extend to other APIs as Pydantic models are adopted: -# "subscriptions": "https://api.planet.com/subscriptions/v1/spec", -# "orders": "https://api.planet.com/compute/ops/spec", -# "data": "https://api.planet.com/data/v1/spec", -SPECS = { - "destinations": "https://api.planet.com/destinations/v1/spec", -} - -HEADER = ("# flake8: noqa\n" - "# fmt: off\n" - "# Generated code — do not edit manually.\n" - "# Reformatting this file will break `nox -s validate_models`.\n" - "# To regenerate, run:\n" - "# nox -s generate_models") - -# Schema names whose anyOf blocks are pure required-field constraints -# (each entry has only a `required` key, no properties of its own). -# These exist solely to express "at least one of these fields must be set", -# which is a server-side validation rule. datamodel-codegen cannot represent -# that constraint cleanly: it generates N numbered classes (e.g. -# DestinationPatchRequest1/2/3) that are otherwise identical except for which -# field is marked required. -# -# We drop the anyOf during codegen so the generator emits a single, flat model -# with all fields optional. The constraint is still enforced server-side; the -# client SDK's job is to build and send the request, not to duplicate server -# validation in a way that produces unreadable generated names. -_DROP_CONSTRAINT_ANY_OF: set[str] = { - "DestinationPatchRequest", -} +from codegen_constants import ( + DROP_CONSTRAINT_ANY_OF, + HEADER, + MODELS_DIR, + REPO_ROOT, + SPECS, + TARGET_PYTHON_VERSION, +) + +__all__ = [ + "MODELS_DIR", + "REPO_ROOT", + "SPECS", + "codegen_argv", + "fetch_and_patch_spec", + "response_reachable_schemas", +] def _schema_refs(node) -> list: @@ -127,7 +113,7 @@ def fetch_and_patch_spec(url: str) -> dict: spec = json.loads(resp.read()) schemas = spec.get("components", {}).get("schemas", {}) - for schema_name in _DROP_CONSTRAINT_ANY_OF: + for schema_name in DROP_CONSTRAINT_ANY_OF: schema = schemas.get(schema_name) if schema is None: continue @@ -169,11 +155,8 @@ def codegen_argv(input_file: pathlib.Path, output: pathlib.Path) -> list: # than constr(...), which mypy rejects as an annotation in the modules # that import these models. "--use-annotated", - # Pinned, not inferred from the interpreter running codegen: output - # differs between Python versions, which would fail the drift check. - # 3.10 is the project's requires-python floor. "--target-python-version", - "3.10", + TARGET_PYTHON_VERSION, # The spec is OpenAPI 3.0.3 and marks fields such as Destination.archived # as both required and `nullable: true`. Without this, codegen drops the # nullability and the model rejects the null the API actually returns. diff --git a/scripts/codegen_constants.py b/scripts/codegen_constants.py new file mode 100644 index 000000000..bf0f437d5 --- /dev/null +++ b/scripts/codegen_constants.py @@ -0,0 +1,63 @@ +# Copyright 2026 Planet Labs PBC. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may not +# use this file except in compliance with the License. You may obtain a copy of +# the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations under +# the License. +"""Constants for Pydantic model code generation. + +Separated from the codegen logic so the spec URLs and output paths can be +read and changed without reading the generator. Adding an API means adding +one line to SPECS. +""" +import pathlib + +REPO_ROOT = pathlib.Path(__file__).parent.parent + +# Where generated models are written. One module per entry in SPECS. +MODELS_DIR = REPO_ROOT / "planet" / "types" + +# Live OpenAPI specs, keyed by the module name they generate. +# TODO: extend to other APIs as Pydantic models are adopted: +# "subscriptions": "https://api.planet.com/subscriptions/v1/spec", +# "orders": "https://api.planet.com/compute/ops/spec", +# "data": "https://api.planet.com/data/v1/spec", +SPECS = { + "destinations": "https://api.planet.com/destinations/v1/spec", +} + +# Prepended to every generated module. +HEADER = ("# flake8: noqa\n" + "# fmt: off\n" + "# Generated code — do not edit manually.\n" + "# Reformatting this file will break `nox -s validate_models`.\n" + "# To regenerate, run:\n" + "# nox -s generate_models") + +# Schema names whose anyOf blocks are pure required-field constraints +# (each entry has only a `required` key, no properties of its own). +# These exist solely to express "at least one of these fields must be set", +# which is a server-side validation rule. datamodel-codegen cannot represent +# that constraint cleanly: it generates N numbered classes (e.g. +# DestinationPatchRequest1/2/3) that are otherwise identical except for which +# field is marked required. +# +# We drop the anyOf during codegen so the generator emits a single, flat model +# with all fields optional. The constraint is still enforced server-side; the +# client SDK's job is to build and send the request, not to duplicate server +# validation in a way that produces unreadable generated names. +DROP_CONSTRAINT_ANY_OF: set[str] = { + "DestinationPatchRequest", +} + +# datamodel-codegen target. Pinned, not inferred from the interpreter running +# codegen: output differs between Python versions, which would fail the drift +# check. 3.10 is the project's requires-python floor. +TARGET_PYTHON_VERSION = "3.10" diff --git a/setup.cfg b/setup.cfg index 0e8fd61d0..18c4e1ce5 100644 --- a/setup.cfg +++ b/setup.cfg @@ -1,5 +1,5 @@ [options] -packages = planet, planet.api_models, planet.cli, planet.clients, planet.data, planet.sync +packages = planet, planet.types, planet.cli, planet.clients, planet.data, planet.sync [options.packages.find] exclude = examples, tests diff --git a/tests/drift/conftest.py b/tests/drift/conftest.py new file mode 100644 index 000000000..6b89d2cf0 --- /dev/null +++ b/tests/drift/conftest.py @@ -0,0 +1,7 @@ +import pathlib +import sys + +# codegen_config lives in scripts/ (not tests/) because it generates +# production code, not test fixtures. +sys.path.insert(0, + str(pathlib.Path(__file__).parent.parent.parent / "scripts")) diff --git a/tests/drift/validate_models.py b/tests/drift/validate_models.py index 81c647c01..0c73f6d5c 100644 --- a/tests/drift/validate_models.py +++ b/tests/drift/validate_models.py @@ -15,7 +15,7 @@ How it works: - datamodel-codegen fetches the live OpenAPI spec and generates models into a temp file. - - The output is compared against the committed file in planet/api_models/. + - The output is compared against the committed file in planet/types/. - The test fails if they differ, indicating the spec has changed. The committed models are raw codegen output. They are excluded from yapf and @@ -80,7 +80,7 @@ def test_models_match_spec(name, url): tofile=f"regenerated/{name}.py", )) pytest.fail( - f"planet/api_models/{name}.py is out of date with the live spec.\n" + f"planet/types/{name}.py is out of date with the live spec.\n" f"Run `nox -s generate_models` to regenerate, then commit the result.\n\n" f"{diff}") finally: diff --git a/tests/unit/test_api_models.py b/tests/unit/test_api_models.py index 12a050fa6..3026dcbc4 100644 --- a/tests/unit/test_api_models.py +++ b/tests/unit/test_api_models.py @@ -21,7 +21,8 @@ import pydantic import pytest -from planet.api_models.destinations import ( +import planet +from planet.types.destinations import ( AmazonS3PatchParams, DefaultDestinationRequest, Destination, @@ -119,3 +120,37 @@ def test_destinations_response_round_trips_aliases(): assert dumped["destinations"][0]["pl:ref"] == "ref" assert dumped["destinations"][0]["created"].startswith("2024-01-01") assert "field_links" not in dumped["destinations"][0] + + +def test_sdk_does_not_import_pydantic_outside_planet_types(): + """pydantic is an optional extra: `pip install planet[models]`. + + Nothing outside planet/types may import it, or a plain `pip install planet` + breaks at import time. Checked by AST rather than by installing the package + two ways, so it runs in the normal suite. + """ + import ast + import pathlib + + package = pathlib.Path(planet.__file__).parent + types_dir = package / "types" + + offenders = [] + for path in package.rglob("*.py"): + if types_dir in path.parents or path.parent == types_dir: + continue + tree = ast.parse(path.read_text(), filename=str(path)) + for node in ast.walk(tree): + if isinstance(node, ast.Import): + names = [alias.name for alias in node.names] + elif isinstance(node, ast.ImportFrom): + names = [node.module or ""] + else: + continue + if any(n == "pydantic" or n.startswith("pydantic.") + for n in names): + offenders.append(f"{path.relative_to(package)}:{node.lineno}") + + assert not offenders, ( + "pydantic imported outside planet/types, which breaks " + f"`pip install planet` without the models extra: {offenders}") From e76d6f84e4cccd8a3eaa5118b9670690427a2de2 Mon Sep 17 00:00:00 2001 From: Regan-Koopmans Date: Thu, 24 Sep 2026 09:18:57 +0200 Subject: [PATCH 10/10] Address review: rename codegen scripts, tidy noxfile and docstrings 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. --- .github/workflows/test.yml | 1 - noxfile.py | 37 ++++++++----------- .../{codegen_constants.py => constants.py} | 6 --- scripts/{codegen_config.py => type_gen.py} | 17 ++------- tests/drift/conftest.py | 2 +- tests/drift/validate_models.py | 5 ++- .../{test_api_models.py => test_types.py} | 2 +- 7 files changed, 25 insertions(+), 45 deletions(-) rename scripts/{codegen_constants.py => constants.py} (91%) rename scripts/{codegen_config.py => type_gen.py} (90%) rename tests/unit/{test_api_models.py => test_types.py} (98%) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 0c7515c44..29cc9915a 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -73,7 +73,6 @@ jobs: key: ${{ runner.os }}-pip restore-keys: | ${{ runner.os }}-pip - # Fetches live OpenAPI specs from api.planet.com. No API key required. - name: Validate models run: | pip install --upgrade nox diff --git a/noxfile.py b/noxfile.py index d83544306..9ad94f6ff 100644 --- a/noxfile.py +++ b/noxfile.py @@ -1,9 +1,14 @@ +import json from pathlib import Path import shutil import sys +import tempfile import nox +sys.path.insert(0, str(Path(__file__).parent / "scripts")) +import type_gen # noqa: E402 + nox.options.stop_on_first_error = True nox.options.reuse_existing_virtualenvs = False @@ -71,7 +76,7 @@ def lint(session): session.install("-e", ".[lint]") session.run("flake8", - f"--exclude={','.join(generated_dirs)}", + f"--extend-exclude={','.join(generated_dirs)}", *source_files) # yapf --exclude is a repeatable flag taking one fnmatch pattern; a bare # directory name matches nothing, so the trailing /* is required. @@ -126,40 +131,30 @@ def examples(session): session.run('pytest', '--no-cov', 'examples/', '-s', *options) -@nox.session +@nox.session(python="3.12") def generate_models(session): - """Re-generate Pydantic models for the Destinations API in planet/types/. - - Uses the same pinned datamodel-code-generator as `nox -s validate_models`, - so the committed output is byte-identical to what the drift check - regenerates. Do not reformat the result. + """Re-generate the Pydantic models in planet/types/ from the live specs. - Run after a known API spec change to refresh the models, then re-run - validate_models to confirm compatibility. + Output must stay byte-identical to what `nox -s validate_models` + regenerates. Run after a spec change, then re-run validate_models. """ session.install("-e", ".[validate_models]") - import json - import tempfile - - sys.path.insert(0, str(Path(__file__).parent / "scripts")) - import codegen_config - - for name, url in codegen_config.SPECS.items(): - output = Path("planet/types") / f"{name}.py" - spec = codegen_config.fetch_and_patch_spec(url) + for name, url in type_gen.SPECS.items(): + output = type_gen.MODELS_DIR / f"{name}.py" + spec = type_gen.fetch_and_patch_spec(url) with tempfile.NamedTemporaryFile(suffix=".json", delete=False, mode="w") as spec_tmp: json.dump(spec, spec_tmp) spec_path = Path(spec_tmp.name) try: - session.run(*codegen_config.codegen_argv(spec_path, output)) + session.run(*type_gen.codegen_argv(spec_path, output)) finally: spec_path.unlink(missing_ok=True) -@nox.session +@nox.session(python="3.12") def validate_models(session): """Validate committed Pydantic models match the live API specs. @@ -168,7 +163,7 @@ def validate_models(session): To refresh snapshots after a deliberate API change, run: nox -s generate_models - Intended as a pre-release gate; not included in the default nox session list. + Runs in PR CI; not included in the default nox session list. """ session.install("-e", ".[validate_models]") session.run( diff --git a/scripts/codegen_constants.py b/scripts/constants.py similarity index 91% rename from scripts/codegen_constants.py rename to scripts/constants.py index bf0f437d5..28e347ed0 100644 --- a/scripts/codegen_constants.py +++ b/scripts/constants.py @@ -11,12 +11,6 @@ # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the # License for the specific language governing permissions and limitations under # the License. -"""Constants for Pydantic model code generation. - -Separated from the codegen logic so the spec URLs and output paths can be -read and changed without reading the generator. Adding an API means adding -one line to SPECS. -""" import pathlib REPO_ROOT = pathlib.Path(__file__).parent.parent diff --git a/scripts/codegen_config.py b/scripts/type_gen.py similarity index 90% rename from scripts/codegen_config.py rename to scripts/type_gen.py index 9d486fc00..5a8ea6c40 100644 --- a/scripts/codegen_config.py +++ b/scripts/type_gen.py @@ -1,4 +1,4 @@ -# Copyright 2024 Planet Labs PBC. +# Copyright 2026 Planet Labs PBC. # # Licensed under the Apache License, Version 2.0 (the "License"); you may not # use this file except in compliance with the License. You may obtain a copy of @@ -11,20 +11,11 @@ # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the # License for the specific language governing permissions and limitations under # the License. -"""Single source of truth for the Pydantic model codegen invocation. - -Both `nox -s generate_models` and the drift test import this module. The drift -test byte-compares regenerated output against the committed models, so the two -must build an identical command line from an identical version of -datamodel-code-generator (pinned in the `validate_models` extra). - -Spec URLs, output paths and other settings live in codegen_constants. -""" import json import pathlib import urllib.request -from codegen_constants import ( +from constants import ( DROP_CONSTRAINT_ANY_OF, HEADER, MODELS_DIR, @@ -83,9 +74,9 @@ def response_reachable_schemas(spec: dict) -> set: def fetch_and_patch_spec(url: str) -> dict: - """Fetch an OpenAPI spec and patch it for codegen. + """Fetch an OpenAPI spec and patch it for generating typed models. - Two patches, both applied before datamodel-codegen sees the spec. + Two patches are applied before datamodel-codegen sees the spec. 1. Strip pure-constraint ``anyOf`` blocks. Some schemas use ``anyOf`` exclusively to express "at least one of these fields must be present", diff --git a/tests/drift/conftest.py b/tests/drift/conftest.py index 6b89d2cf0..b12f46c2c 100644 --- a/tests/drift/conftest.py +++ b/tests/drift/conftest.py @@ -1,7 +1,7 @@ import pathlib import sys -# codegen_config lives in scripts/ (not tests/) because it generates +# type_gen lives in scripts/ (not tests/) because it generates # production code, not test fixtures. sys.path.insert(0, str(pathlib.Path(__file__).parent.parent.parent / "scripts")) diff --git a/tests/drift/validate_models.py b/tests/drift/validate_models.py index 0c73f6d5c..5389f2ddb 100644 --- a/tests/drift/validate_models.py +++ b/tests/drift/validate_models.py @@ -14,7 +14,8 @@ """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. + - The live OpenAPI spec is fetched, patched, and written to a temp file. + - datamodel-codegen reads that file and generates models into another temp file. - The output is compared against the committed file in planet/types/. - The test fails if they differ, indicating the spec has changed. @@ -36,7 +37,7 @@ import pytest -from codegen_config import MODELS_DIR, SPECS, codegen_argv, fetch_and_patch_spec +from type_gen import MODELS_DIR, SPECS, codegen_argv, fetch_and_patch_spec def _regenerate(url: str, output: pathlib.Path) -> None: diff --git a/tests/unit/test_api_models.py b/tests/unit/test_types.py similarity index 98% rename from tests/unit/test_api_models.py rename to tests/unit/test_types.py index 3026dcbc4..1d87f739f 100644 --- a/tests/unit/test_api_models.py +++ b/tests/unit/test_types.py @@ -14,7 +14,7 @@ """Contract tests for the generated Destinations models. These pin the two halves of the unknown-field policy set in -tests/drift/codegen_config.py. Request models reject unknown fields so a typo +scripts/type_gen.py. Request models reject unknown fields so a typo fails client side. Response models accept them so an additive server change does not break a shipped SDK. """