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

Issue #337 #366

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

Merged
jsenecal merged 20 commits into develop from issue-337
Jul 22, 2017
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
aa962c1
Missing dependancy
jsenecal Jul 19, 2017
da0c94c
Add Django Debug Toolbar, and add a failing test setting out our perf...
aidanlister Jul 20, 2017
1d84c80
Add a helper for specifying a prefetch_for_related attribute on your ...
aidanlister Jul 20, 2017
9e284f4
Merge branch 'issue-337' into feature/add-ddt-pylint-and-test
jsenecal Jul 21, 2017
b4999e4
Merge pull request #364 from ABASystems/feature/add-ddt-pylint-and-test
jsenecal Jul 21, 2017
cacca2e
Trying to make isort happy
jsenecal Jul 21, 2017
5a7d73a
Merge pull request #365 from ABASystems/feature/auto-prefetch-helpers
jsenecal Jul 21, 2017
e96fe6c
Adding `select_related` and `prefetch_for_includes` to `CommentViewSet`
jsenecal Jul 21, 2017
8fdbc0a
Fixes ImportError on TravisCI
jsenecal Jul 21, 2017
46c3809
E265 block comment should start with '# '
jsenecal Jul 21, 2017
3c24093
E501 line too long (121 > 100 characters)
jsenecal Jul 21, 2017
151bdb9
F401 '..factories' imported but unused
jsenecal Jul 21, 2017
844e88e
Imports are (now) correctly sorted as per isort.
jsenecal Jul 21, 2017
07f67a3
Package name, not module name... woops!
jsenecal Jul 21, 2017
f924ac8
More of this isort madness
jsenecal Jul 21, 2017
97b6dd3
Use forms in the browsable API
jsenecal Jul 22, 2017
f0bda69
Updating docs to reflect the added ModelViewSet helper
jsenecal Jul 22, 2017
63020b4
Including packaging to the isort command that checks example.
jsenecal Jul 22, 2017
a7130b9
Merge branch 'issue-337' of github.com:django-json-api/django-rest-fr...
jsenecal Jul 22, 2017
7343def
Updating docs to give some context.
jsenecal Jul 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,7 @@ pip-delete-this-directory.txt
# VirtualEnv
.venv/

# Developers
*.sw*
manage.py
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this ignoring manage.py?

Copy link
Member Author

@jsenecal jsenecal Jul 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@aidanlister must have a custom one (introduced by da0c94c).

We're not using it to start the app anyways ($ django-admin.py runserver --settings=example.settings)

.DS_Store
12 changes: 10 additions & 2 deletions example/factories/__init__.py → example/factories.py
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,17 @@

import factory
from faker import Factory as FakerFactory

from example.models import (
Blog, Author, AuthorBio, Entry, Comment, TaggedItem, ArtProject, ResearchProject, Company
ArtProject,
Author,
AuthorBio,
Blog,
Comment,
Company,
Entry,
ResearchProject,
TaggedItem
)

faker = FakerFactory.create()
Expand Down Expand Up @@ -64,7 +73,6 @@ class Meta:


class TaggedItemFactory(factory.django.DjangoModelFactory):

class Meta:
model = TaggedItem

Expand Down
18 changes: 18 additions & 0 deletions example/models.py
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ class TaggedItem(BaseModel):
def __str__(self):
return self.tag

class Meta:
ordering = ('id',)


@python_2_unicode_compatible
class Blog(BaseModel):
Expand All @@ -38,6 +41,9 @@ class Blog(BaseModel):
def __str__(self):
return self.name

class Meta:
ordering = ('id',)


@python_2_unicode_compatible
class Author(BaseModel):
Expand All @@ -47,6 +53,9 @@ class Author(BaseModel):
def __str__(self):
return self.name

class Meta:
ordering = ('id',)


@python_2_unicode_compatible
class AuthorBio(BaseModel):
Expand All @@ -56,6 +65,9 @@ class AuthorBio(BaseModel):
def __str__(self):
return self.author.name

class Meta:
ordering = ('id',)


@python_2_unicode_compatible
class Entry(BaseModel):
Expand All @@ -73,6 +85,9 @@ class Entry(BaseModel):
def __str__(self):
return self.headline

class Meta:
ordering = ('id',)


@python_2_unicode_compatible
class Comment(BaseModel):
Expand All @@ -87,6 +102,9 @@ class Comment(BaseModel):
def __str__(self):
return self.body

class Meta:
ordering = ('id',)


class Project(PolymorphicModel):
topic = models.CharField(max_length=30)
Expand Down
15 changes: 13 additions & 2 deletions example/settings/dev.py
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
'rest_framework',
'polymorphic',
'example',
'debug_toolbar',
]

TEMPLATES = [
Expand Down Expand Up @@ -58,7 +59,11 @@

PASSWORD_HASHERS = ('django.contrib.auth.hashers.UnsaltedMD5PasswordHasher', )

MIDDLEWARE_CLASSES = ()
MIDDLEWARE_CLASSES = (
'debug_toolbar.middleware.DebugToolbarMiddleware',
)

INTERNAL_IPS = ('127.0.0.1', )

JSON_API_FORMAT_KEYS = 'camelize'
JSON_API_FORMAT_TYPES = 'camelize'
Expand All @@ -74,7 +79,13 @@
),
'DEFAULT_RENDERER_CLASSES': (
'rest_framework_json_api.renderers.JSONRenderer',
'rest_framework.renderers.BrowsableAPIRenderer',

# If you're performance testing, you will want to use the browseable API
# without forms, as the forms can generate their own queries.
# If performance testing, enable:
'example.utils.BrowsableAPIRendererWithoutForms',
# Otherwise, to play around with the browseable API, enable:
# 'rest_framework.renderers.BrowsableAPIRenderer',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since example is what runs on the demo site, should we leave the form version on by default instead of the perf version so that people can add extra data to play with it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess it would make sense indeed - let me fix this

),
'DEFAULT_METADATA_CLASS': 'rest_framework_json_api.metadata.JSONAPIMetadata',
}
57 changes: 57 additions & 0 deletions example/tests/test_performance.py
View file Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
from django.utils import timezone
from rest_framework.test import APITestCase

from example.factories import CommentFactory
from example.models import Author, Blog, Comment, Entry


class PerformanceTestCase(APITestCase):
def setUp(self):
self.author = Author.objects.create(name='Super powerful superhero', email='i.am@lost.com')
self.blog = Blog.objects.create(name='Some Blog', tagline="It's a blog")
self.other_blog = Blog.objects.create(name='Other blog', tagline="It's another blog")
self.first_entry = Entry.objects.create(
blog=self.blog,
headline='headline one',
body_text='body_text two',
pub_date=timezone.now(),
mod_date=timezone.now(),
n_comments=0,
n_pingbacks=0,
rating=3
)
self.second_entry = Entry.objects.create(
blog=self.blog,
headline='headline two',
body_text='body_text one',
pub_date=timezone.now(),
mod_date=timezone.now(),
n_comments=0,
n_pingbacks=0,
rating=1
)
self.comment = Comment.objects.create(entry=self.first_entry)
CommentFactory.create_batch(50)

def test_query_count_no_includes(self):
""" We expect a simple list view to issue only two queries.

1. The number of results in the set (e.g. a COUNT query),
only necessary because we're using PageNumberPagination
2. The SELECT query for the set
"""
with self.assertNumQueries(2):
response = self.client.get('/comments?page_size=25')
self.assertEqual(len(response.data['results']), 25)

def test_query_count_include_author(self):
""" We expect a list view with an include have three queries:

1. Primary resource COUNT query
2. Primary resource SELECT
3. Authors prefetched
3. Entries prefetched
"""
with self.assertNumQueries(4):
response = self.client.get('/comments?include=author&page_size=25')
self.assertEqual(len(response.data['results']), 25)
8 changes: 8 additions & 0 deletions example/urls.py
View file Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from django.conf import settings
from django.conf.urls import include, url
from rest_framework import routers

Expand All @@ -22,3 +23,10 @@
urlpatterns = [
url(r'^', include(router.urls)),
]


if settings.DEBUG:
import debug_toolbar
urlpatterns = [
url(r'^__debug__/', include(debug_toolbar.urls)),
] + urlpatterns
20 changes: 20 additions & 0 deletions example/utils.py
View file Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
from rest_framework.renderers import BrowsableAPIRenderer


class BrowsableAPIRendererWithoutForms(BrowsableAPIRenderer):
"""Renders the browsable api, but excludes the forms."""

def get_context(self, *args, **kwargs):
ctx = super().get_context(*args, **kwargs)
ctx['display_edit_forms'] = False
return ctx

def show_form_for_method(self, view, method, request, obj):
"""We never want to do this! So just return False."""
return False

def get_rendered_html_form(self, data, view, method, request):
"""Why render _any_ forms at all. This method should return
rendered HTML, so let's simply return an empty string.
"""
return ""
7 changes: 6 additions & 1 deletion example/views.py
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,13 @@ class AuthorViewSet(ModelViewSet):


class CommentViewSet(ModelViewSet):
queryset = Comment.objects.all()
queryset = Comment.objects.select_related('author', 'entry')
Copy link
Member Author

Choose a reason for hiding this comment

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

I still have an issue with the way relations are extracted first level... I had to prefetch things that I shouldn't have to only to make tests pass... Feels like the queryset might be over reaching to overcome an issue with drf_jsonapi.

Copy link
Member Author

@jsenecal jsenecal Jul 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

So, it really happens in b96d79d merged a year ago.

@jerel do you remember the conversation that was around this ?

Copy link
Member Author

Choose a reason for hiding this comment

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

After further investigation, the problem does not occur inside extract_relationships - Still looking

Copy link
Contributor

@aidanlister aidanlister Jul 22, 2017

Choose a reason for hiding this comment

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

@jsenecal the problem is in two places, setting use_pk_only_optimization to False, and extract_attributes. You have to whack them both, which made it so hard to find the source of the explosion.

I think you should probably remove the select_related in this because the tests should fail until we fix that other issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for this - I merged already but will update my branch to reflect this conversation

serializer_class = CommentSerializer
prefetch_for_includes = {
'__all__': [],
'author': ['author', 'author__bio', 'author__entries'],
'entry': ['author', 'author__bio', 'author__entries']
}


class CompanyViewset(ModelViewSet):
Expand Down
3 changes: 3 additions & 0 deletions requirements-development.txt
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,6 @@ recommonmark
Sphinx
sphinx_rtd_theme
tox
mock
django-debug-toolbar
packaging==16.8
7 changes: 6 additions & 1 deletion rest_framework_json_api/relations.py
View file Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
import collections
import json
from collections import OrderedDict

import inflection
import six
from django.core.exceptions import ImproperlyConfigured
from django.core.urlresolvers import NoReverseMatch
from django.utils.translation import ugettext_lazy as _
from rest_framework.fields import MISSING_ERROR_MESSAGE
from rest_framework.relations import * # noqa: F403
from rest_framework.relations import MANY_RELATION_KWARGS, PrimaryKeyRelatedField
Copy link
Collaborator

Choose a reason for hiding this comment

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

👏 I'm a fan of any time a * import can be removed.

from rest_framework.reverse import reverse
from rest_framework.serializers import Serializer

from rest_framework_json_api.exceptions import Conflict
Expand Down
1 change: 0 additions & 1 deletion rest_framework_json_api/serializers.py
View file Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import inflection
from django.db.models.query import QuerySet
from django.utils import six
from django.utils.translation import ugettext_lazy as _
from rest_framework.exceptions import ParseError
from rest_framework.serializers import * # noqa: F403
Expand Down
39 changes: 37 additions & 2 deletions rest_framework_json_api/views.py
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,40 @@
)


class ModelViewSet(viewsets.ModelViewSet):
class PrefetchForIncludesHelperMixin(object):
def get_queryset(self):
""" This viewset provides a helper attribute to prefetch related models
based on the include specified in the URL.

__all__ can be used to specify a prefetch which should be done regardless of the include

@example
# When MyViewSet is called with ?include=author it will prefetch author and authorbio
class MyViewSet(viewsets.ModelViewSet):
queryset = Book.objects.all()
prefetch_for_includes = {
'__all__': [],
'author': ['author', 'author__authorbio']
'category.section': ['category']
}
"""
qs = super(PrefetchForIncludesHelperMixin, self).get_queryset()
if not hasattr(self, 'prefetch_for_includes'):
return qs

includes = self.request.GET.get('include', '').split(',')
for inc in includes + ['__all__']:
prefetches = self.prefetch_for_includes.get(inc)
if prefetches:
qs = qs.prefetch_related(*prefetches)

return qs


class AutoPrefetchMixin(object):
def get_queryset(self, *args, **kwargs):
qs = super(ModelViewSet, self).get_queryset(*args, **kwargs)
""" This mixin adds automatic prefetching for OneToOne and ManyToMany fields. """
qs = super(AutoPrefetchMixin, self).get_queryset(*args, **kwargs)
included_resources = get_included_resources(self.request)

for included in included_resources:
Expand Down Expand Up @@ -84,6 +115,10 @@ def get_queryset(self, *args, **kwargs):
return qs


class ModelViewSet(AutoPrefetchMixin, PrefetchForIncludesHelperMixin, viewsets.ModelViewSet):
pass


class RelationshipView(generics.GenericAPIView):
serializer_class = ResourceIdentifierObjectSerializer
self_link_view_name = None
Expand Down
1 change: 1 addition & 0 deletions setup.py
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ def get_package_data(package):
'pytest>=2.8,<3',
'django-polymorphic',
'packaging',
'django-debug-toolbar'
] + mock,
zip_safe=False,
)

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