Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement dont require pk strictly related #2745 #2754

Merged

Conversation

bleib1dj
Copy link
Contributor

@bleib1dj bleib1dj commented Mar 23, 2015

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="this@email.com", 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
Copy link
Member

tomchristie commented Mar 23, 2015

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
Copy link
Contributor Author

bleib1dj commented Mar 23, 2015

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

@tomchristie
Copy link
Member

tomchristie commented Mar 23, 2015

Perfect!

@tomchristie
Copy link
Member

tomchristie commented Mar 23, 2015

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
Copy link
Contributor Author

bleib1dj commented Mar 23, 2015

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

@jpadilla
Copy link
Member

jpadilla commented Apr 4, 2015

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

@kevin-brown
Copy link
Member

kevin-brown commented Apr 4, 2015

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

@bleib1dj
Copy link
Contributor Author

bleib1dj commented Apr 5, 2015

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
Copy link
Member

jpadilla commented Apr 5, 2015

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!

jpadilla added a commit that referenced this pull request Apr 5, 2015
…rictly_related

Enhancement dont require pk strictly related #2745
@jpadilla jpadilla merged commit 2e6d39d into encode:master Apr 5, 2015
1 check failed
@bleib1dj
Copy link
Contributor Author

bleib1dj commented Apr 5, 2015

Sweet np! :)

tinchodipalma added a commit to tinchodipalma/django-rest-framework that referenced this pull request May 25, 2015
The following items had a wrong href value:

- Dont require pk strictly for related fields. (encode#2745, encode#2754)
- Restrict integer field to integers and strings. (encode#2835, encode#2836)
@tinchodipalma tinchodipalma mentioned this pull request May 25, 2015
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.

None yet

4 participants