strawberry-graphql / strawberry-django

Strawberry GraphQL Django extension
https://strawberry.rocks/docs/django
MIT License
406 stars 118 forks source link

feat: filter_field optional value resolution #510

Closed Kitefiko closed 6 months ago

Kitefiko commented 6 months ago

Description

Adds option not to resolve value of custom filter method via @strawberry_django.filter_field(resolve_value=False).

Types of Changes

Issues Fixed or Closed by This PR

Checklist

bellini666 commented 6 months ago

@Kitefiko even looking at the example on the issue you created:

@strawberry_django.filter_field
def multi_filter(self, value: relay.GlobalID):
    # value here is not GlobalID instance, but already resolved node_id (str)
    # so following cannot be currently done

    if value.type_name == "FruitNode":
        ...
    else value.type_name == "VegetableNode":
        ... 
    else:
        raise ValidationError("Incorrect GlobalID type")

It seems strange if value in this case is not actually GlobalID. i.e. the typing should be respected.

Do you think we can refactor this to basically only convert the value for automatic filters and pass it as is to custom resolvers, to make sure the type is respected?

Kitefiko commented 6 months ago

Hi, yeah, that was precisely my question. So just to clear this up for me. You suggest to do breaking change and stop resolving value in custom filters. I assume still leaving the ability to use param to do so, but make it not resolve by default? Another question I found during adjustments, should RangeLookup resolve its start and end fields?

Edit: Adjusted. Made RangeLookup resolve it's field.

codecov-commenter commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.59%. Comparing base (e163d39) to head (a726be7).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #510 +/- ## ========================================== + Coverage 88.50% 88.59% +0.09% ========================================== Files 40 40 Lines 3400 3402 +2 ========================================== + Hits 3009 3014 +5 + Misses 391 388 -3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bellini666 commented 6 months ago

Hi, yeah, that was precisely my question. So just to clear this up for me. You suggest to do breaking change and stop resolving value in custom filters. I assume still leaving the ability to use param to do so, but make it not resolve by default? Another question I found during adjustments, should RangeLookup resolve its start and end fields?

Edit: Adjusted. Made RangeLookup resolve it's field.

Yes.

This is still a new change so I think it is safe

What I mean is that when I do something like this:

@strawberry_django.filter_field()
def some_filter(self, value: relay.GlobalID, prefix: str) -> Q:
    ...

I expect that the value will be GloabalID as it is typed, and not its resolved value. Not value not being GlobalID just like it is typed there is a bug to IMO

Maybe we can have an option to opt-in to "automatic resolution" of the values, but TBH I think we don't even need that as it seems more confusing. What are your thoughts here?

Kitefiko commented 6 months ago

Ok, so currently it is done as you describe BUT with opt-in resolve_value (more info in docs - I think it's pretty clear there). And also auto resolving for RangeLookup - still don't know, whether this is good choice.

I would probably leave resolve_value in? Don't really see reason to rip it out, there might be use-case (custom GlobalID?, different enum value resolution?) for custom Lookups where value resolution might be unwanted and this covers that.