Feature: Ability to set single object lookup key

[benhowes]

Description

This PR adds the ability to alter the behaviour of a single object lookup. Currently the behaviour is that a single object fetch can only operate on the pk of that object (with pk as the variable name), but in many cases developers may want to use other names/types.

For maximum flexibility, this PR adds 3 options to the strawberry.django.field definition which can be used in many combinations, such as:

@strawberry.type
class Query:
   fruit: Fruit = strawberry.django.field(
      lookup_key="id",
      lookup_key_django_name="uuid",
      lookup_key_type=UUID
   )

or

@strawberry.type
class Query:
   fruit: Fruit = strawberry.django.field(
      lookup_key="public_id",
      lookup_key_type=str
   )

or

@strawberry.type
class Query:
   fruit: Fruit = strawberry.django.field(
      lookup_key="id",
   )

It defaults to the existing behaviour of the lookup field being pk and the type being strawberry.ID.

The only "breaking" change is that the lookup key is now a required field, so this will slightly alter the generated schema. This can be considered more of a bugfix since the lookups would not work without an argument, other than in the special case where there was only a single record of that type (in which case .get() on the queryset would work). I would hope that's not behaviour that anyone is relying on though...

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Documentation

Issues Fixed or Closed by This PR

• Closes Resolver created for single object has optional pk argument #107
• Closes Possibility to rename pk to id or something else #121
• Closes Allow alteration of the single object lookup field and type #130

Checklist

• My code follows the code style of this project.
• My change requires a change to the documentation. - This PR should build on Started docs for query #147 when that's merged
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[patrick91]

This looks pretty solid to me! I've added a couple of minor comments 😊

[patrick91]

I think we can use None here as default :)

[patrick91]

can we add type hints for the new params?

[patrick91]

Is this to allow using id as the GraphQL argument, but map it to pk in the Django filters?

[patrick91]

Should we change this to be value?

[patrick91]

do you think we could infer lookup_key_type from the django model?

[benhowes]

In some cases it's certainly possible, but I found that if you're using a custom field type, there's not currently a means to add it to the mapping. Perhaps for the time being, the lookup_key_type could be optional.

[capital-G]

Would be interested in this - is there something one can do to get this merged?

[bellini666]

Would be interested in this - is there something one can do to get this merged?

We are basically waiting for some updates from @benhowes that @patrick91 made.

If for some reason @benhowes is not going to work on this anymore, you can probably take on this changes yourself (if you want to do that, of course 😊)

[benhowes]

I no longer work on the codebase which used this change, and have not touched it for a long time. I aspire to get the code review changes implemented, but realistically I doubt I will any time soon.

[bellini666]

I no longer work on the codebase which used this change, and have not touched it for a long time. I aspire to get the code review changes implemented, but realistically I doubt I will any time soon.

Thanks for updating us on that :)

@capital-G would you like to work on this to get it finished?

[capital-G]

@bellini666 yes :) Sounds like a good first thing to dive deeper into strawberry :)
Would you recommend continue working on this branch or rewrite it from scratch and use this as inspiration?

[bellini666]

@bellini666 yes :) Sounds like a good first thing to dive deeper into strawberry :) Would you recommend continue working on this branch or rewrite it from scratch and use this as inspiration?

Personally I would suggest creating a new PR starting from the commits on this issue, just to avoid losing the changes done by @benhowes and removing its credits for it. Then feel free to adapt it in any way you want.

When the new PR gets opened I'll close this one as superseded

[capital-G]

@bellini666 I started looking into this but I think I would prefer to do this from a new branch as there are merge conflicts when rebasing this to main, mainly in filters.py which can not be resolved in a nice way because some interfaces have changed and I would rather prefer to have a clean git history on those changes then incorporating the 2 commits from this PR.
Would this be OK?

[bellini666]

@bellini666 I started looking into this but I think I would prefer to do this from a new branch as there are merge conflicts when rebasing this to main, mainly in filters.py which can not be resolved in a nice way because some interfaces have changed and I would rather prefer to have a clean git history on those changes then incorporating the 2 commits from this PR. Would this be OK?

You can merge main instead of rebasing, so it is easier to resolve conflicts and still keep the original credits.

But it is up mostly to @benhowes to say what he prefers

[benhowes]

Anything is fine with me, no attribution required!

[bellini666]

This has not have any activity for a while now so I'm closing this to clean up the backlog a little.

In case op or anyone wants to get back to this, feel free to reopen, or ask me to reopen in case you don't have permission to :)