Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

"Data" key optional in relationships #443

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

Closed
Anton-Shutik wants to merge 1 commit into django-json-api:master from Anton-Shutik:master

Conversation

Copy link
Contributor

@Anton-Shutik Anton-Shutik commented Jul 11, 2018
edited
Loading

Fixes #

Description of the Change

This PR adds a flag render_data on ResourceRelatedField. Passing render_data=False will skip rendering "data" in responce, thus saving sql queries and improveing performance.

Json api docs says:

A "relationship object" MUST contain at least one of the following:
links, data, meta

Before:

"relationships": {
 "comments": {
 "data": [
 {
 "type": "comments",
 "id": "874"
 },
 {
 "type": "comments",
 "id": "877"
 },
 {
 "type": "comments",
 "id": "2273"
 }
 ],
 "links": {
 "self": "http://127.0.0.1:9000/drf/users/1/relationships/comments/",
 "related": "http://127.0.0.1:9000/drf/users/1/comments/",
 "first": "http://127.0.0.1:9000/drf/users/1/comments/?page=1",
 "last": "http://127.0.0.1:9000/drf/users/1/comments/?page=1",
 "next": null,
 "prev": null
 }
},

Using `comments=ResourceRelatedField(render_data=False, ...):

"relationships": {
 "comments": {
 "links": {
 "self": "http://127.0.0.1:9000/drf/users/1/relationships/comments/",
 "related": "http://127.0.0.1:9000/drf/users/1/comments/",
 "first": "http://127.0.0.1:9000/drf/users/1/comments/?page=1",
 "last": "http://127.0.0.1:9000/drf/users/1/comments/?page=1",
 "next": null,
 "prev": null
 }
},

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • changelog entry added to CHANGELOG.md
  • author name in AUTHORS

Copy link
Member

sliverc commented Jul 12, 2018

Thanks for proposing this feature. I think this is mostly related to issue #296 (I guess #178 is a slightly different approach)

I have a feeling such a change should not be added to ResourceRelatedField but to a different field resp. serializer. There is already serializer serializer.HyperlinkedModelSerializer and rest_framework.relations.HyperlinkedRelatedField which according to the documentation on DRF should be used for such a use case. What do you think?

In terms of performance resp. too many sql queries: What I usually do is to use prefetch related data as documented here. Maybe this helps you as well.

Copy link
Member

sliverc commented Jul 16, 2018

Continuing discussion from #296

We cant have 3 Fields like LinksField, DataField, MetaField, and use them separately.

Maybe this would be the best but I am not sure whether there is really a use case for all three fields even though the specification allows it.

I think for now we should stick to the two use cases:

  • resource representing all parts (meta, links, data)
  • resource only represented as links and optional meta (for better performance)

I've tried to create HyperlinkedRelatedField based on DRF's but came up rewriting same code from ResourceRelatedField about link handling.

What about moving the get_links logic into a mixin? This could then be used from ResourceRelatedField and HyperlinkedRelatedField. If this solves the issue that we do not need to use SkipField and overwriting ManyReleatedField I would certainly prefer it.
As you have tried this in case of deriving from HyperlinkedRelatedField did you still need to overwrite ManyReleatedField field or not?

Copy link
Contributor Author

@sliverc I've tried to implement it in this PR
Current PR looks easier and safer to me.

Copy link
Member

sliverc commented Jul 18, 2018

I am closing this in favor of #445

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sliverc sliverc Awaiting requested review from sliverc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /