strawberry-graphql / strawberry-django

Strawberry GraphQL Django extension
MIT License
391 stars 115 forks source link

Auto Enums does not works with gettext_lazy choice values #489

Closed fabien-michel closed 3 months ago

fabien-michel commented 4 months ago

When a django choices is defined using gettext_lazy strings, it makes the "description is a string" check fail in graphql.type.definition.GraphQLEnumValue

The check is_description(description) return False if description is of type django.utils.functional.lazy.<locals>.__proxy__

Context

Strawberry django's settings

STRAWBERRY_DJANGO = {
    "GENERATE_ENUMS_FROM_CHOICES": True,
}

Failing test

in tests/test_enums.py

Add choice with gettext_lazy description

from django.utils.translation import gettext_lazy

class Choice(models.TextChoices):
    """Choice description."""

    A = "a", "A description"
    B = "b", "B description"
    C = "c", "C description"
    D = "d", gettext_lazy("D description")

Possible fix

As a fix I would suggest to change this line from:

c[0]: EnumValueDefinition(value=c[0], description=c[1])

to

c[0]: EnumValueDefinition(value=c[0], description=str(c[1]))

It would also be possible to change this line from

choices = cast(List[Tuple[Any, str]], model_field.choices)

to

choices = [c[0]: str(c[1]) for c in model_field.choices]

I can propose a PR if you agree with one of these changes.

Thanks

Upvote & Fund

Fund with Polar

sdobbelaere commented 4 months ago

@fabien-michel I like this discovery about this setting. - and indeed I have the same issue as yourself. I'm sure @bellini666 would appreciate a PR for it. I know I would.

fabien-michel commented 4 months ago

I have a concern about the fixes I proposed. Adding str() will trigger the translation mechanism, and because the enum loading is done at GraphQL schema init on python code load, it voids the laziness benefit of using gettext_lazy... I'm not sure if there is a better solution.

bellini666 commented 4 months ago

I agree that's possibly the only solution right now, and as @sdobbelaere said, I would indeed appreciate a PR for it :)

For a better solution I think we would need to introduce some kind of "laziness" support on strawberry itself for loading descriptions that doesn't rely on django. I'll try to think about something to better solve this in the future... Any ideas here @patrick91 ?