Closed jamesfisher-gis closed 6 days ago
I think we should change it to a pydantic class. @vincentsarago what do you think?
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.
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)
Clearly there might be another reason ...
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.
Clearly we can't use Field
with the the filter extension GET like you said before.
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 ?
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.
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
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
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 https://github.com/opengeospatial/ogcapi-common/issues/224 to see what's the OGC pov
well it seems that -
hyphen is a requirement, so we HAVE to comply to it
Ah, that's a bummer. Thank you for the detailed answer!
How should we proceed here?
@jamesfisher-gis I think moving to dataclass as proposed in #714 is the only solution
https://github.com/stac-utils/stac-fastapi/blob/main/stac_fastapi/extensions/stac_fastapi/extensions/core/filter/request.py#L13-L20
The
filter_crs
andfilter_lang
fields are invalid becauseFilterExtensionGetRequest
is anattrs
class, but it usesField
from pydantic.If I try to init the class and define
filter_lang
, for example.the class also does not impose the default values for
filter_crs
andfilter_lang
I think there are two options:
filter_crs
andfilter_lang
intoattrs
fields. AFAIKattrs
does not support applying an alias to fields.FilterExtensionGetRequest
into a pydantic class.Why do we have GET request models as
attrs
classes and POST request models aspydantic
classes?