Enhancement dont require pk strictly related #2745

[bleib1dj]

Rationale

I implemented the enhancement with the id_field to provide users using non-django ORM backends the ability to still utilize the check originally provided by if obj.pk is None, prior to providing a related reference. I believe this gives more flexibility but also potentially provides unnecessary functionality as alternative backends may not generate an identification value solely upon storage and could potentially have their identifier provided prior to save. If the backend does that this check would only serve to verify that the user properly assigned an identifier but would not provide the intended affect of verifying the object has been created. That being said, there is the potential for this to also be done with the django ORM as the following example illustrates:

from django.contrib.auth.models import User
from uuid import uuid1

new_uuid = str(uuid1())
my_user = User(pk=new_uuid, email="[email protected]", username="test_user")
print my_user.pk
query_user = User.objects.get(pk=new_uuid)

Out: '5412f4cc-d166-11e4-b274-080027a2dd8c'
Out: DoesNotExist: User matching query does not exist

This goes against using User.objects.create(...) but could be done. In either case providing the id_field gives the user the ability to choose, where hasattr() limits the check to only working when the pk attribute is present.

If this way of handling the enhancement is agreed upon I can add additional docs detailing the optional field and when to utilize it. Otherwise we can close out this PR and I can open one with my hassattr() modifications instead.

Additional references

Per our discussion in #2745 I found a couple additional locations of pk attribute references.

PrimaryKeyRelatedField

There's a reference in the to_representation method but I opted not to change it as I believe the SlugField provides a way of achieving this functionality for a non-django ORM based backend.

ManyRelatedField

There was a reference in the get_attribute method that I swapped out with self.id_field which also required the definition of self.id_field within it's own __init__ method.

I chose to define it there rather than up in the Field class as the ManyRelatedField is "Automagically" created in the RelatedField and passed its kwargs. So instead of giving users the ability to set an id_field on every field, which would be unnecessary, this should reduce it to them only needing to set it on the HyperlinkedRelatedField and HyperlinkedIdentityField if they choose to, while still supporting many=True.

[tomchristie]

Great stuff.

I'd rather we drop the id_field parameter. Hardcoding that as 'pk'mimimizes the interface, while still ensuring we're providing nice behavior to uses of non-ORM backends. Other backends may have other 'saved'/'unsaved' considerations, and I'm fine with the pk==None special casing only being provided for the Django ORM specifically.

We could reconsider this if we end up with a large amount of support for any other backends in particular that make a clear case for needing an id_field. Until then simplicity of the interface trumps any other considerations.

[bleib1dj]

Cool sounds good, I'll update it :).

[tomchristie]

Perfect!

[tomchristie]

Something odd going on at the moment with the travis results never returning.
Looks to be related to this awkward pep8 versioning clash.
Unrelated to this pull request, but I'll look into it and get this merged after it's resolved.
Thanks for the input! 😄 👍

[bleib1dj]

Gotcha, let me know if anything ends up needing to be modified :) and not a problem!

[jpadilla]

Restarted build. It's currently failing to something unrelated to this PR, because of outdated .travis.yml

[kevin-brown]

Still failing, this branch needs to be rebased in order to fix the .travis.yml issue.

[bleib1dj]

Rebased and pushed up the rebase which fixed the build on Travis in my fork but for some reason the rebase didn't propagate to the PR. Any ideas?

[jpadilla]

Restarted the build but it didn't reflect changes in the actual matrix, although the branch is correctly updated. I'm gonna go ahead and merge this anyways, since opening another PR just to workaround whatever is going on in Travis seems like a bit too much for this change.

@bleib1dj thanks again!

[bleib1dj]

Sweet np! :)