Skip to content
This repository has been archived by the owner on Jul 8, 2023. It is now read-only.

strawberry_django_plus.filters.filter makes fields required by default?! #55

Open
blueyed opened this issue Jun 2, 2022 · 3 comments
Open
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@blueyed
Copy link
Contributor

blueyed commented Jun 2, 2022

When using strawberry_django_plus.filters.filter instead of strawberry_django.filter, the annotated fields appear to be required, i.e. activated by default in GraphiQL, and using deprecation_reason causes the schema to validate:

❌ Required input field FooFilter.bar_status cannot be deprecated.

@filter(models.Foo)
class FooFilter:
    bar_status: str = gql.django.field(deprecation_reason="use … instead")

I've seen that | None can be used with the annotation to make it optional, but I think it should be optional by default.

code ref:

@__dataclass_transform__(
order_default=True,
field_descriptors=(
StrawberryField,
_field,
node,
connection,
field.field,
field.node,
field.connection,
),
)
def filter( # noqa:A001
model: Type[Model],
*,
name: Optional[str] = None,
description: Optional[str] = None,
directives: Optional[Sequence[object]] = (),
lookups: bool = False,
) -> Callable[[_T], _T]:
return input(
model,
name=name,
description=description,
directives=directives,
is_filter="lookups" if lookups else True,
partial=True,
)

@bellini666
Copy link
Member

Hey @blueyed , are you sure that strawberry_django forthis should work just like strawberry_django forces the type annotation to optional?

I'll take a look over the weekend to check what is strawberry_django_plus doing differently

@blueyed
Copy link
Contributor Author

blueyed commented Jun 9, 2022

are you sure that strawberry_django forthis should work just like strawberry_django forces the type annotation to optional?

Can you please rephrase / fix your question? :)

I cannot really tell though: from my experience filters are optional/gql.UNSET by default, and while it appears to be a nice feature to require them, I do not think this should be the default behavior.

@bellini666
Copy link
Member

are you sure that strawberry_django forthis should work just like strawberry_django forces the type annotation to optional?

Can you please rephrase / fix your question? :)

OMG, I started typing something, than changed my mind in the middle and didn't reread what I wrote.

What I meant was to ask is: Are you sure strawberry_django works like that? Because the behaviour here should be the same AFAIK.

I cannot really tell though: from my experience filters are optional/gql.UNSET by default, and while it appears to be a nice feature to require them, I do not think this should be the default behavior.

I agree with you! It is just strange from the typing point of view. But well, if strawberry_django is already doing that (based on the question above), strawberry_django_plus shouldn't be working differently.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants