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

Avoid query explosion for related fields where I can user pk_only_opt… #374

Merged
merged 15 commits into from
Jan 9, 2018

Conversation

santiavenda2
Copy link
Contributor

…imization.

Also, fix AutoPrefetchMixin for ReverseOneToOneDescriptors.

@sliverc
Copy link
Member

sliverc commented Oct 19, 2017

I tested this change in on one of my projects and it works smoothly. Thanks.

It seems that in drf 3.1 _readable_fields doesn't exist so CI is failing. Not sure whether there is an alternative but I would be in favor of dropping support for 3.1 which is already more than 2.5 years old.

@santiavenda2
Copy link
Contributor Author

santiavenda2 commented Nov 8, 2017

@sliverc I have fixed the problem we had in DRF<3.2 (_readable_field property didn't exist on Serializer class)

I think the change is ready to be merged

@codecov-io
Copy link

codecov-io commented Nov 8, 2017

Codecov Report

Merging #374 into master will decrease coverage by 0.4%.
The diff coverage is 57.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #374      +/-   ##
==========================================
- Coverage   92.37%   91.97%   -0.41%     
==========================================
  Files          56       56              
  Lines        2887     2917      +30     
==========================================
+ Hits         2667     2683      +16     
- Misses        220      234      +14
Impacted Files Coverage Δ
example/views.py 90.74% <100%> (ø) ⬆️
rest_framework_json_api/utils.py 91.13% <100%> (ø) ⬆️
rest_framework_json_api/relations.py 82.95% <100%> (+0.19%) ⬆️
rest_framework_json_api/renderers.py 85.81% <41.66%> (-1.12%) ⬇️
rest_framework_json_api/serializers.py 80.62% <54.54%> (-4.16%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b0257c0...08d652d. Read the comment docs.

@sliverc sliverc mentioned this pull request Nov 28, 2017
@mblayman
Copy link
Collaborator

I'm going to re-target this PR to master since the main development branch is now over there.

@mblayman mblayman changed the base branch from develop to master December 17, 2017 02:19
@mblayman
Copy link
Collaborator

@santiavenda2 because this is now targeting master, there is a conflict on the branch. master dropped a bunch of supported Django version so I think that might provided an opportunity to simplify the PR code.

@santiavenda2
Copy link
Contributor Author

@mblayman done

@@ -170,6 +170,47 @@ def get_field_names(self, declared_fields, info):
fields = super(ModelSerializer, self).get_field_names(declared, info)
return list(fields) + list(getattr(self.Meta, 'meta_fields', list()))

def to_representation(self, instance):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why this is overriding to_representation from the ModelSerializer? It seems like DJA is likely to diverge from the DRF upstream if this method is added. Is there some extra stuff added here that you could call out?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mblayman this is returning the JSON API identifier of the object without diving down the recursive rabbit whole. It looks like an elegant solution.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the second look, @aidanlister. I took a look at the DRF implementation and the method seems small enough that it would be hard for DJA to diverge too much from upstream.

@aidanlister
Copy link
Contributor

I've reviewed the PR and we're now testing this PR internally - everything seems to be working as expected - very impressive. Next step we'd do a production release in two weeks, but I would love to have this merged before then.

@mblayman
Copy link
Collaborator

mblayman commented Jan 9, 2018

With the added input from @aidanlister, I think we've had enough eyes looking at this to declare victory. Thanks for the contribution, @santiavenda2!

@mblayman mblayman merged commit b4dffb8 into django-json-api:master Jan 9, 2018
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.

5 participants