Skip to content

Conversation

spockNinja
Copy link
Contributor

@spockNinja spockNinja commented Jul 12, 2017

Problem

By default, relations are only exposed as complex nested structures. So in order to get a related ID, you have to perform the join to the related field. Sometimes, only the id is needed at first, and the related object might be fetched by id later on an as-needed basis.

Solution

Add a new setting, INCLUDE_FOREIGNKEY_IDS that tells graphene-django to add graphene.ID fields for foreign keys.

Utilize this new option in construct_fields to look for ForeignKey fields and add them along with their existing relational field.

Current Behavior

{
  myObjects {
    relatedObject {
      id
    }
  }
}

Current Workaround

You can currently achieve the same behavior by simply adding the id attribute to your DjangoObjectType class explicitly.

class MyObject(DjangoObjectType):
    foreign_id_one = graphene.ID()
    foreign_id_two = graphene.ID()

    class Meta:
        model = MyObjectModel

Usage after this PR

Turn on the setting

GRAPHENE = {
    ...
    'INCLUDE_FOREIGNKEY_IDS': True
}

Then DjangoObjectTypes are simple

class MyObject(DjangoObjectType):

    class Meta:
        model = MyObjectModel

And then you can simply do:

{
  myObjects {
    relatedObjectId
  }
}

And avoid making a join when you don't have to.

While the workaround isn't difficult, in a large project with lots of models and relations, it can be a lot to add. In our case, we want these IDs present on all models, so it makes sense to provide a more convenient approach.

@spockNinja
Copy link
Contributor Author

@syrusakbary

Does this seem like a feature you'd like to include in this repo? If so, I'll add some tests and documentation. If not, I'll just implement it for our project's graphql wrappers and close this PR.

@coveralls
Copy link

coveralls commented Jul 12, 2017

Coverage Status

Coverage decreased (-0.08%) to 92.645% when pulling 6d2e60e on spockNinja:feature/include-id-fields into 0588f89 on graphql-python:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 92.645% when pulling 6d2e60e on spockNinja:feature/include-id-fields into 0588f89 on graphql-python:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 92.645% when pulling 6d2e60e on spockNinja:feature/include-id-fields into 0588f89 on graphql-python:master.

@spockNinja
Copy link
Contributor Author

I'm actually wondering if this would be nicer as a global setting? It's still a bit tedious to add that line to every DjangoObjectType Meta.

@syrusakbary syrusakbary force-pushed the master branch 2 times, most recently from bdc7189 to f93251b Compare September 1, 2017 08:12
@spockNinja spockNinja force-pushed the feature/include-id-fields branch from 6d2e60e to 377399d Compare November 24, 2017 22:33
@spockNinja spockNinja changed the title WIP Add option for including foreignkey IDs Add setting for including foreignkey IDs Nov 24, 2017
@coveralls
Copy link

coveralls commented Nov 24, 2017

Coverage Status

Coverage increased (+0.04%) to 93.184% when pulling fc5655e on spockNinja:feature/include-id-fields into d7c160c on graphql-python:master.

@firaskafri
Copy link
Collaborator

@jkimbo @dopeboy @danpalmer
What do you think?

@phalt
Copy link
Contributor

phalt commented May 3, 2019

This is a cool feature but it feels like a use case that isn't general enough to add a whole setting for. Could this not just be done with resolver methods?

I'd like to tidy up some of the PRs, so if we still want to work on this let me know or I will close this in 1 week.

@phalt phalt closed this May 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants