limit value above the maximum advertised (or allowed) should return that max limit rather than 400

[philvarner]

This was recently resolved as an ambiguity in the OAFeat and STAC API specs for limit. If a limit parameter value above the max allowed limit is sent, the server should respond as if the max limit value were sent, rather than with 400 error.

[eseglem]

I was taking a look at this and noticed that the BaseSearchPostRequest has a comment that it exists because it was waiting on a PR in stac-pydantic that has been merged for a while now. While there are some slight differences between the two functions, would it make sense to address this in stac-pydantic instead of here?

I played with a fix for this on my fork here but wanted to ask about that before submitting a PR anywhere.

There are a few other minor differences between the models, but it would be easy enough to port them all over there if that would be the better place for them to live.

[gadomski]

While there are some slight differences between the two functions, would it make sense to address this in stac-pydantic instead of here?

I'm +0 on switching BaseSearchPostRequest to use stac-pydantic -- while it makes sense to put pydantic models in stac-pydantic, it does increase the time-to-fix for any changes, as we wait for bugfixes to propagate into a stac-pydantic release.

I think the best course of action would be to

1. Fix this issue in stac-fastapi using your branch
2. Reconcile any differences BaseSearchPostRequest and Search in stac-pydantic, get those changes released
3. In a new PR, update stac-fastapi to the new stac-pydantic version and remove BaseSearchPostRequest in favor of Search

[eseglem]

That course of action seems totally reasonable. The delay in fixes certainly can definitely be a pain point. Especially with as many moving parts as there are. I understand why it ends up happening. Hopefully coordination can be streamlined over time.

I have one question about the tests in other parts of the code: This broke a few of the tests which are expecting a 400 error when the limit is set to 10001. For example: sqlalchemy/tests/api/test_api.py#L202 Would the right thing to do be just swap the 400 to a 200? I don't think it actually tests if the limit itself is functional, but it would catch a regression if it starts throwing errors in the future. Is there additional testing which should be added for this somewhere else?

[philvarner]

@eseglem just checking a 200 sounds right to me. The behavior that should happen is:

• Always a 200 for any limit >= 1
• Response ItemCollections contains >=1 and <= limit Items (assuming there are >=0 items that match the rest of the query)

[geospatial-jeff]

I'm +0 on switching BaseSearchPostRequest to use stac-pydantic -- while it makes sense to put pydantic models in stac-pydantic, it does increase the time-to-fix for any changes, as we wait for bugfixes to propagate into a stac-pydantic release.

I agree with this. For historical context, the original goal was to write stac-pydantic (get the data models right first) then build stac-fastapi on top of this. But in reality the spec has changed quite a bit and the interpretation of the spec changes too which makes it difficult to always update stac-pydantic first because there is high coupling between the data models (stac-pydantic) and API validation (stac-fastapi). I think its better to keep changes in stac-fastapi then backport to stac-pydantic where appropriate.

I have one question about the tests in other parts of the code: This broke a few of the tests which are expecting a 400 error when the limit is set to 10001. For example: sqlalchemy/tests/api/test_api.py#L202

Update tests to match the semantics of the spec. So I'd expect a limit of 10,001 to return a 200 with limit being set to the server default maximum.

[gadomski]

I think its better to keep changes in stac-fastapi then backport to stac-pydantic where appropriate.

Understood, thanks for the context. After the v1.0 release of STAC-API and a prolonged period of stability in stac-fastapi, it may make sense to refactor the models back down to stac-pydantic, but it does not seem like now is the right time.