strawberry-graphql / strawberry

A GraphQL library for Python that leverages type annotations 🍓
https://strawberry.rocks
MIT License
4.01k stars 533 forks source link

Support serialization of fields using client directives that modify their return types #2556

Open bellini666 opened 1 year ago

bellini666 commented 1 year ago

Suppose we have a type like this:

@strawberry.type
class Foo:
    some_date: datetime.date

We also have a directive called format_date that formats a date into a human friendly date. We query that type like this:

foo {
  someDateFormatted: someDate @format_date
}

Currently this will raise an error because the Date scalar will call value.isoformat(), expecting a datetime.date as the return value, but the value now is what format_date returns (which in this example, is already a string)

Upvote & Fund

Fund with Polar

jkimbo commented 1 year ago

@bellini666 in your opinion is the problem here that directives are called before custom scaler serialisation happens or that the Date scalar cannot handle strings? I'm not sure which is the better option.

bellini666 commented 1 year ago

@jkimbo not really sure to be honest =P.

If the directive gets called before the scalar serialization, it could receive something that it cannot serialize (that's the current behaviour)

If the scalar serialization happens after it, that means that the directive needs to handle serialized values. In that case, it would need to reparse the date in isoformat to a date again to then format it again to a string. It is a better option than the current behaviour, but I don't know if it is the correct one.

Another approach would be to use the directive to serialize the value instead of the scalar serialization. Are there any downsides to this?

jkimbo commented 1 year ago

If the scalar serialization happens after it, that means that the directive needs to handle serialized values. In that case, it would need to reparse the date in isoformat to a date again to then format it again to a string. It is a better option than the current behaviour, but I don't know if it is the correct one.

It feels like this is the correct behaviour (even if it means parsing the date twice) because the client could theoretically apply the @format_date directive on any field not just datetime.date fields. I'm not sure how directives are called though so I don't now if it's reasonable to change this behaviour.

Another approach would be to use the directive to serialize the value instead of the scalar serialization. Are there any downsides to this?

I think this would lead to more confusion because it would duplicate any serialization logic into both the scalar and directives making it harder to maintain.

bellini666 commented 1 year ago

It feels like this is the correct behaviour (even if it means parsing the date twice) because the client could theoretically apply the @format_date directive on any field not just datetime.date fields. I'm not sure how directives are called though so I don't now if it's reasonable to change this behaviour.

I agree. Specially because directives can be chained