strawberry-graphql / strawberry-django

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

ForeignKey `_id` field resolves to relation object, not field value #506

Closed kevinmsun closed 6 months ago

kevinmsun commented 6 months ago

Describe the Bug

When I attempt to surface the key used for a many-to-one relation as a resolvable field, it seems to resolve to the object itself (and implicitly, its string form) rather than the key value.

the schema is something like this

# Django User model
class User(models.Model):
  # standard primary key
  organization = models.ForeignKey(
        Organization,
    )

# Django Organization model
class Organization(models.Model):
  # standard primary key
  pass

# Strawberry schemas
@strawberry_django.type(User, name="User")
class UserType:
    id: strawberry.scalars.ID
    organization_id: strawberry.scalars.ID

querying for user { organizationId } yields the str form of the Organization object, rather than the contents of the User.organization_id column

From what I can tell, the deepest issue comes from this django behavior https://github.com/django/django/blob/921670c6943e9c532137b7d164885f2d3ab436b8/django/db/models/options.py#L638-L640 , where get_field returns the relation object rather than the relation ID. This leads to evaluating the wrong value via https://github.com/strawberry-graphql/strawberry-django/blob/425b75bcb33c3ea2abd4b7845ed1e4865e1c83d3/strawberry_django/fields/types.py#L529-L540 , with django_name being set to the wrong value (organization instead of organization_id) at https://github.com/strawberry-graphql/strawberry-django/blob/425b75bcb33c3ea2abd4b7845ed1e4865e1c83d3/strawberry_django/type.py#L321-L325

System Information

Additional Context

Upvote & Fund

Fund with Polar

kevinmsun commented 6 months ago

one workaround is organization_id: strawberry.scalars.ID = strawberry_django.field(field_name="organization_id"), which fixes the name and prevents it from being reassigned/detected

bellini666 commented 6 months ago

@kevinmsun this should be solved in the latest release.

Let me know if it works ok for you!

kevinmsun commented 6 months ago

@kevinmsun this should be solved in the latest release.

Let me know if it works ok for you!

thanks so much, I'll check it out!

moritz89 commented 5 months ago

This looks is really useful. All my types are annotated with

def to_gid(gql_type: Type, pk: uuid.UUID) -> relay.GlobalID | None:
    if pk:
        return relay.GlobalID(gql_type.__strawberry_definition__.name, str(pk))
    return None

@strawberry_django.type(Site)
class SiteGQLNode(relay.Node):
    @strawberry_django.field
    def organization_id(self, root: PermissionGroup) -> relay.GlobalID | None:
        return to_gid(OrganizationGQLNode, root.organization_id)
moritz89 commented 5 months ago

Just noticed that it returns the UUID (if the primary key is set as UUIDs) instead of the relay.GID. Is there an approach to write code similar to the following:

@strawberry_django.type(Site)
class SiteGQLNode(relay.Node):
    organization_id: relay.GlobalID | None

And the respective GQL query:

{
  allSites {
    edges { node {
      id
      organizationId
    }}
  }
}
bellini666 commented 5 months ago

@moritz89 you mean, using organization_id: auto but have it being exposed as organization_id: relay.GlobalID | None?

moritz89 commented 5 months ago

Not necessarily, just a method to be able to specify it as a single line, organization_id: auto or organization_id: relay.GlobalID | None instead of

@strawberry_django.field
def organization_id(self, root: PermissionGroup) -> relay.GlobalID | None:
    return to_gid(OrganizationGQLNode, root.organization_id)

When I use either of the single line options, it returns the UUID instead of GQL GID.

With v0.33.0 the result was:

{
  "data": {
    "allSites": {
      "edges": [
        {
          "node": {
            "id": "U2l0ZUdRTE5vZGU6N2JmN2ZhNjYtNTU1ZS00N2QzLThiNzktYjA0MGVhYTA1YWM1",
            "organizationId": "Admin's organization"
          }
        },
        {
          "node": {
            "id": "U2l0ZUdRTE5vZGU6ZmE2MzUxMWQtOWEzMC00MTBkLThhMmEtNWY3MmU1NmEyMDcy",
            "organizationId": "Admin's organization"
          }
        }
      ]
    }
  }
}

With v0.37.0 the output is:

{
  "data": {
    "allSites": {
      "edges": [
        {
          "node": {
            "id": "U2l0ZUdRTE5vZGU6N2JmN2ZhNjYtNTU1ZS00N2QzLThiNzktYjA0MGVhYTA1YWM1",
            "organizationId": "6ee306b6-cf33-4b7c-91c1-5371289257ac"
          }
        },
        {
          "node": {
            "id": "U2l0ZUdRTE5vZGU6ZmE2MzUxMWQtOWEzMC00MTBkLThhMmEtNWY3MmU1NmEyMDcy",
            "organizationId": "6ee306b6-cf33-4b7c-91c1-5371289257ac"
          }
        }
      ]
    }
  }
}
bellini666 commented 5 months ago

@moritz89 sorry for taking too long to reply here...

I'm still confused about what exactly regressed here. Can you give me a longer explanation, or even maybe a MRE?

Also, you might want to open a new issue to register the regression as it will make it easier to track