Skip to content

Conversation

@orf
Copy link

@orf orf commented Nov 27, 2015

This is a first attempt at adding support for alternative primary keys. This is done by removing any hard-coded 'instance.pk' calls, instead first checking if the PK attributes name is in any serialized output. If it is then we return that instead of the pk attribute. This works for the following serializers/viewsets:

class AuthorSerializer(serializers.ModelSerializer):
    id = serializers.CharField(source='email')

    class Meta:
        model = Author  # has (id, email) fields
        fields = ('id',)

class AuthorViewSet(viewsets.ModelViewSet):
    queryset = Author.objects.all()
    serializer_class = AuthorSerializer

    lookup_field = 'email'

This produces the following output:

 {
      "type": "Author",
      "id": "someemail@gmail.com",
      "attributes": {},
}

Works for foreign keys and m2m relationships as well as creating new instances (as far as I can tell). I started to write some tests but quickly got a bit lost, so I thought I would submit this for comments before the weekend and maybe tidy it up on Monday if you think it looks promising.

@grjones
Copy link

grjones commented Oct 28, 2016

+1 For this. There are edge cases where this is needed and would have parity with DRF serializer support for custom primary keys.

@orf
Copy link
Author

orf commented Oct 28, 2016

I tried to explain the issue and premise of this PR in #155 but the maintainers didn't seem to get it. I don't think this will ever be added which is a great shame as it seems pretty arbitrary. DRF supports it, but this library doesn't, because reasons.

@auvipy
Copy link
Contributor

auvipy commented Jan 19, 2017

@orf how about adding to https://github.com/chibisov/drf-extensions

@orf
Copy link
Author

orf commented Jan 19, 2017

It's not an extension to DRF, DRF already supports this. It's a bugfix for this library that seems to always assume you want to expose your pk

@auvipy
Copy link
Contributor

auvipy commented Jan 19, 2017

then try https://github.com/AltSchool/dynamic-rest this

@mblayman
Copy link
Collaborator

mblayman commented Mar 1, 2017

Hi, @orf. Thanks for the contribution! This branch is now in conflict in the main branch and rather old. Are you interested in fixing this up or should we close out this PR? I'm stepping up to maintain the PR queue and get DRF JA changes flowing again, but I can't merge conflicted stuff. Also, this branch is marked as a work in progress so I'm not sure if it was ever finished.

If this is still valuable, resolving the merge conflicts will give me a chance to review it well. If this is no longer important to you, closing the PR is right next step.

Please let me know what you'd like to do. Thanks!

@orf
Copy link
Author

orf commented Mar 1, 2017

Hey @mblayman, I believe a more up-to-date patch is here: rpatterson@6334ceb

Feel free to close this MR, if @rpatterson is willing to make a MR with that change it would be great. I could contribute there rather than resurrect this branch.

@orf orf closed this Mar 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants