FilterExtensionGetRequest invalid fields about stac-fastapi HOT 14 CLOSED

I'm not sure how large of a change is required for that

good thing that we are still in alpha 😄

let me have a Quick Look

from stac-fastapi.

I think we should change it to a pydantic class. @vincentsarago what do you think?

from stac-fastapi.

I was wondering if attr is used for GET requests so we can use converter? ex.

@attr.s
class FieldsExtensionGetRequest(APIRequest):
    """Additional fields for the GET request."""

    fields: Optional[str] = attr.ib(default=None, converter=str2list)

from stac-fastapi.

I was wondering if attr is used for GET requests so we can use converter? ex.

@attr.s
class FieldsExtensionGetRequest(APIRequest):
    """Additional fields for the GET request."""

    fields: Optional[str] = attr.ib(default=None, converter=str2list)

This makes total sense to me.

I tried modifying FieldsExtensionGetRequest to be a pydantic class, but this causes mixed model type errors since the other GET models are attrs classes.

from stac-fastapi.

Clearly we can't use Field with the the filter extension GET like you said before.

from stac-fastapi.

ok I think I have a better understanding now.

For the GET request, we can't use pydantic model (which can be used only for POST request, as body parameter). In order to define GET query parameter we can either use simple class, dataclass or attr.

with Attr we can't use pydantic.Field or fastapi.Query, but we need to use attr.ib to define the parameters. Sadly attr doesn't allow - in alias name (maybe this should be raised in attr repo, but looking at how attr works, I'm not sure this will be resolved).

in #714 I've replaced attr with python's dataclass. This allow the use of fastapi.Query and resolve the original issue.

BUT this adds some edge case issue where we have to order the class we merge (base class + extension) to make sure the required parameter are not placed after optional parameters. This is working for now but if one introduce an extension with a required parameter, then the code will not work!

Maybe we should just relaxe the SPEC and allow both filter-crs and filter_crs

asked in opengeospatial/ogcapi-common#224 to see what's the OGC pov

from stac-fastapi.

well it seems that - hyphen is a requirement, so we HAVE to comply to it

from stac-fastapi.

@jamesfisher-gis I think moving to dataclass as proposed in #714 is the only solution

from stac-fastapi.

well I think attrs is used specifically for GET request, I don't really know why we use attr for GET and pydantic for POST but there might be a good reason.

from stac-fastapi.

Clearly there might be another reason ...

from stac-fastapi.

I tried modifying FieldsExtensionGetRequest to be a pydantic class, but this causes mixed model type errors since the other GET models are attrs classes.

what if we move all the class to pydantic ?

from stac-fastapi.

Clearly we can't use Field with the the filter extension GET like you said before.

We could fix the attrs class like this

@attr.s
class FilterExtensionGetRequest(APIRequest):
    """Filter extension GET request model."""

    filter: Optional[str] = attr.ib(default=None)
    filter_crs: Optional[str] = attr.ib(default=None)
    filter_lang: Optional[FilterLang] = attr.ib(default="cql2-text")

FilterExtensionGetRequest()
#FilterExtensionGetRequest(filter=None, filter_crs=None, filter_lang='cql2-text')

This would require the implementations to look for the filter-crs and filter-lang params in the request and update if they don't match the default. Looks like that is what they are doing anyway.

https://github.com/stac-utils/stac-fastapi-elasticsearch-opensearch/blob/main/stac_fastapi/core/stac_fastapi/core/core.py#L475-L481

from stac-fastapi.

I tried modifying FieldsExtensionGetRequest to be a pydantic class, but this causes mixed model type errors since the other GET models are attrs classes.

what if we move all the class to pydantic ?

Yeah that's the other option I guess. I'm not sure how large of a change is required for that

from stac-fastapi.

Ah, that's a bummer. Thank you for the detailed answer!

How should we proceed here?

from stac-fastapi.