Open Mark90 opened 6 months ago
Separating on -
has a number of other problems. Dates, UUIDs and negative numbers naturally have -
characters which makes it almost impossible to write generic filtering logic using this disjunction/OR.
I have already added splitting on |
, which is a better option so -
can already be deprecated. Using |
is also consistent with the logical OR meaning in the query
parameter.
If we change the type of filterBy
, the "value"
will become a union of str and list of str. I don't mind, but it makes handling the typechecker a bit of a pain. Maybe we should wait and see of the current splitting on |
yields any practical issues?
Yeah |
might be sufficient indeed.
But since the query
parameter is for the frontend to spew user search queries to, filterBy
is specifically for backends which can programmatically create the filtering params - so it's a bit weird that they have to create a 'query-like' filtering string, only for it to be split again.
My str | list[str]
union suggestion is indeed not appreciated by strawberry. :)
Another option: add a parameter values: list[str]
on the Filter type, which can co-exist with the current value: str
. It seems to work.. but do we like it?
# db/filters/filters.py
class Filter(BaseModel):
field: str
value: str
values: list[str] | None = None
...
def _filter_to_node(filter_item: Filter) -> Node:
...
values = filter_item.values or _re_split.split(filter_item.value) if should_split else filter_item.value.split("|")
...
# graphql/types.py
@strawberry.experimental.pydantic.input(model=Filter)
class GraphqlFilter:
field: str = strawberry.field(description="Field to filter on")
value: str = strawberry.field(description="Value to filter by")
values: list[str] | None = strawberry.field(description="Values to filter by", default=None)
The
filterBy
parameter is defined in graphql resolvers asfilter_by: list[GraphqlFilter] | None = None
with the following implementationFor example:
filterBy: [{field: "product", value: "FW"}, {field: "status": "active"}]
filtersFW
products that have statusactive
.If we want to filter multiple products, i.e. we want to have
FW
ORIP
products with the statusactive
, this is done throughfilterBy: [{field: "product", value: "FW-IP"}, {field: "status": "active"}]
where-
acts as a value separator. I believe this was inherited from the REST endpoint; it isn't great design as leaks implementation details and it prevents searching on values with-
in the name.It would be nice if we can do
filterBy: [{field: "product", value: ["FW", "IP"]}, {field: "status": "active"}]
.Tasks:
GraphqlFilter
accept both astr
andlist[str]
list[str]
should do a logical-OR search-
value separator! Deprecate it and raise a warning. Create follow-up ticket to remove in a future minor release.