FilterExtensionGetRequest invalid fields

[jamesfisher-geo]

https://github.com/stac-utils/stac-fastapi/blob/main/stac_fastapi/extensions/stac_fastapi/extensions/core/filter/request.py#L13-L20

The filter_crs and filter_lang fields are invalid because FilterExtensionGetRequest is an attrs class, but it uses Field from pydantic.

If I try to init the class and define filter_lang, for example.

from typing import Any, Dict, Literal, Optional

import attr
from pydantic import BaseModel, Field

from stac_fastapi.types.search import APIRequest

FilterLang = Literal["cql-json", "cql2-json", "cql2-text"]


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

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


FilterExtensionGetRequest(
    filter="{}",
    filter_crs="",
    filter_lang="cql2-json"
)
# TypeError: init() got an unexpected keyword argument 'filter_crs'

the class also does not impose the default values for filter_crs and filter_lang

FilterExtensionGetRequest()
# FilterExtensionGetRequest(filter=None)

I think there are two options:

• Make filter_crs and filter_lang into attrs fields. AFAIK attrs does not support applying an alias to fields.
• Make FilterExtensionGetRequest into a pydantic class.

Why do we have GET request models as attrs classes and POST request models as pydantic classes?

[jonhealy1]

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

[vincentsarago]

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.

[jonhealy1]

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)

[jonhealy1]

Clearly there might be another reason ...

[jamesfisher-geo]

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.

[jonhealy1]

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

[vincentsarago]

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 ?

[jamesfisher-geo]

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

[jamesfisher-geo]

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

[vincentsarago]

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

[vincentsarago]

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

[vincentsarago]

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