Skip to content

Conversation

@rkm-ursa
Copy link

@rkm-ursa rkm-ursa commented Oct 2, 2025

What I'm changing

  • Making datetime and geometry optional in OrderPayload for cases where they are not required. This is aimed at ordering archival imagery where a filter or order parameter is sufficient to fulfill the order without requiring spatiotemporal constraints.

How I did it

  • Made None an option for the two fields.

Checklist

  • [x ] Tests pass: uv run pytest
  • [ x] Checks pass: uv run pre-commit run --all-files
  • [x ] CHANGELOG is updated (if necessary)

@rkm-ursa rkm-ursa requested a review from gadomski as a code owner October 2, 2025 15:32
Copy link
Member

@gadomski gadomski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would need to be changed in the spec first: https://github.com/stapi-spec/stapi-spec/tree/main/order#body-fields

@jkeifer
Copy link
Member

jkeifer commented Oct 3, 2025

It turns out this is a more complex issue than appears at first glance. We have a fairly lengthy issue about this problem and options to consider to solve it. The short version is that making these fields optional doesn't really address the larger ambiguities in this model, given that the CQL2 filter could easily duplicate these parameters and the spec does not address what to do in this case.

All this said, the idea of making these fields option when placing an order has come up previously (at least twice before), and is potentially the most pragmatic solution to the problem, at least until we can address linked issue more thoroughly in the spec. But it is a breaking change: implementations that do require these fields are currently expecting pydantic validation to ensure they are present. Making these fields optional will require such implementations to add validation to check that these fields are set and error if they are not present.

Given that, I don't love the idea of making a change like this in a way that is effectively "opt-out". That is, implementations will get a silent change in behavior that requires code changes on their end to restore a similar behavior to before, but would do so in a way that is not documented in the OpenAPI docs (as the model would show the fields as optional whether that is true in the implementation or not).

I'd rather see a solution to this problem that is "opt-in", whereby an implementation that specifically wants this could change the payload model to one with these fields as optional. But the default behavior would be to have these fields required.

An additional complication: the pending spec changes for v0.2.0 require a fairly significant change to this model. Those pending changes outline that the order request model should not have geometry, datetime, or filter as top-level parameters, but should have an embedded SearchParameters model with those fields. This new spec says that the SearchParameters object be the same object used for opportunity searches.

A naive implementation of the v0.2.0 spec draft would necessitate making this new SearchParameters model, making the current OpportunityPayload inherit from that model, and then making OrderPayload look something like this:

class OrderPayload(BaseModel, Generic[ORP]):
    search_parameters: SearchParameters
    order_parameters: ORP

    model_config = ConfigDict(strict=True)

Now we already make OrderPayload a generic so implementations can override the order_parameters model. To specify that model, we allow passing in a custom order parameters model in the Product constructor (see the model code here). We could allow one of two models to be passed in here for a new search_parameters model argument, either the normal SearchParameters with the geometry and datetime required, or a SearchParametersOptionalFields (or whatever a better name would be) that has those fields marked as optional.

I suppose this could also be a flag parameter on the Product class, taking True/False to indicate whether it should use a SearchParameters model with those fields required. That might be cleaner for users than making them have to provide just the right model type reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants