-
Notifications
You must be signed in to change notification settings - Fork 298
Allowed to overwrite resource id in serializer #1127
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
Allowed to overwrite resource id in serializer #1127
Conversation
Thanks for looking into our oldest open issue. It is great to tackle this.
I think it is a great idea to allow a DJA user to overwrite an id by configuring an id field on the serializer, as this is most intuitive. I think your code needs to be adjusted though, as in its current states it expects an id on the instance, and not on the serialized data.
A check would need to do the following:
- check if there is an
idin the serialized data (in the methodbuild_json_resource_objthe serialized data is calledresource). - if not fall back to the pk on the resource instance
That should be enough in my opinion and no more if/else are needed. It will also be important to document this behavior in our documentation, that setting an id field on the serializer is the way to have a custom id or when using generic serializers. To fully resolve #155 it also needs to be possible to configure a custom pk for a relation. When digging into the code, this is actually already possible by setting pk_field on the ResourceRelatedField, so we would only need to document and add a test.
You think you could work on those things?
@axieum Do you think you can still work on this PR (open issues see comment above)? I very much like the implementation and would love to get it integrated.
test: add tests for `get_resource_id_from_instance` util function
Hi, sorry about that @sliverc - got heavily distracted elsewhere.
I've gone ahead and quickly added some commits to do what you proposed, first it'll try the resource and then the resource_instance. In both scenarios, the resource (and resource_instance) may be a dict or object.
I couldn't think of how to write a test for the case where resource is preferred over resource_instance, could you assist here please?
Thanks,
Jonathan.
def get_resource_id(resource_instance, resource): if resource and 'id' in resource: return force_str(resource['id']) return resource_instance and force_str(resource_instance.pk) or None
As we need to add a utility method anyway to get the resource id, force_str, checking for None etc. should all be in there. Above is how the method could look like (not tested, but as an idea). As passing on resource_instance and resource separately, it is actually much simpler.
For writing a test, you need to write a test where the serializer has a field called id. Easiest would then be to add a method get_id(self) to the serializer which returns a different id to see whether that id is actually returned. Then test it as you already do in the test_views.py file.
Hope this makes things clearer.
I think we're going to need to explicitly add code to check for and execute a get_id method for that piece to work.
2b9483a to
0d3a0bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for continue working on this. See my inline comments + the tests do not seem to pass, therefore haven't reviewed those in detail yet.
...missing-pk # Conflicts: # CHANGELOG.md
This is a very important feature and as we are soon releasing a new version I would like to get this ready. So I am currently working on it to be able to merge it before our next release.
I now added some documentation as well. For this feature to be complete, it is also necessary to be able to overwrite resource id for resource related fields. Added this now as well. So this is ready to merge. In case there are any comments of anything I missed, let me know.
Uh oh!
There was an error while loading. Please reload this page.
Fixes #155
Fixes #722
Fixes #1126
Description of the Change
Currently, per default, the
idis always retrieved by callingpkproperty on the instance. This change allows the id to be overwritten in the serializer by defining aidfield.Checklist
CHANGELOG.mdupdated (only for user relevant changes)AUTHORS