Skip to content

APIv3 CRUD for Redirect objects #5879

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.

Sign up for GitHub

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
merged 10 commits into from
Jul 9, 2019
Merged

APIv3 CRUD for Redirect objects #5879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6f9302c
Follow same pattern of .api method for RedirectQuerySet as well
humitos Jul 2, 2019
04e675f
APIv3 CRUD for Redirect objects
humitos Jul 2, 2019
89d2cb0
Split Redirect serializer to allow sanitize fields
humitos Jul 2, 2019
37663c5
Small queryset optimization (select_related)
humitos Jul 3, 2019
e1e92d3
Small optimization on base has_admin_permission check
humitos Jul 3, 2019
f9346d3
Include 'redirects' in '_links' responses
humitos Jul 3, 2019
1dacb63
Add some tests for Redirect endpoints
humitos Jul 3, 2019
ba2dfe2
Test for PUTi and DELETE methods
humitos Jul 3, 2019
213db7f
Rename `redirect_type` to `type` in the APIv3 response
humitos Jul 3, 2019
ba61d0c
Small lint fix
humitos Jul 3, 2019
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
6 changes: 4 additions & 2 deletions readthedocs/api/v3/mixins.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
from django.shortcuts import get_object_or_404
from rest_framework.exceptions import NotFound

from readthedocs.builds.models import Version
from readthedocs.projects.models import Project
Expand Down Expand Up @@ -62,7 +61,10 @@ def listing_objects(self, queryset, user):
return queryset.none()

def has_admin_permission(self, user, project):
if project in self.admin_projects(user):
# Use .only for small optimization
admin_projects = self.admin_projects(user).only('id')

if project in admin_projects:
return True

return False
Expand Down
12 changes: 11 additions & 1 deletion readthedocs/api/v3/permissions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from rest_framework.permissions import IsAuthenticated
from rest_framework.permissions import IsAuthenticated, BasePermission


class PublicDetailPrivateListing(IsAuthenticated):
Expand Down Expand Up @@ -29,3 +29,13 @@ def has_permission(self, request, view):
return True

return False


class IsProjectAdmin(BasePermission):
Copy link
Contributor

Choose a reason for hiding this comment

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

This permission seems to be generating something like 6 extra queries which was surprising. However, it is not generating N queries where N is the number of redirects.

Copy link
Member Author

@humitos humitos Jul 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Well, this is the basic permission check that we need in all the endpoints for user admin of a project. I split it here to make it simpler and have it isolated.

So, if this check is slow, we should take care of it since it's used a on most of the endpoints.

Basically, it does:

  • get a Project (from slug in the URL)
  • performs a Project.objects.filter(users__in=[user])

I think we can use .only('id') in ProjectQuerySetMixin.has_admin_permission in the if, to avoid bringing the full objects since we only want to make a simple check.

In my very simple local tests, there is a timing difference. I'll add a commit for 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.

Also, some of these method are called more than once (at least for the BrowsableAPIRenderer) and returned value won't change in the same request, so we could do something there if we care enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

Overall it probably isn't a big deal either way. Some number of queries is necessary for the permission check but 6 just seemed high. I do wonder if we shouldn't just cache the project object onto the view object since most of our views relate to projects.

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 think we can optimize it later if needed --I think it's a good option. Although, I don't want to introduce a "buggy cached object" at this point before knowing that everything is working as we expect :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with that for now. Let's keep it in mind though.


"""Grant permission if user has admin rights on the Project."""

def has_permission(self, request, view):
project = view._get_parent_project()
if view.has_admin_permission(request.user, project):
return True
78 changes: 78 additions & 0 deletions readthedocs/api/v3/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from readthedocs.builds.models import Build, Version
from readthedocs.projects.constants import LANGUAGES, PROGRAMMING_LANGUAGES
from readthedocs.projects.models import Project
from readthedocs.redirects.models import Redirect, TYPE_CHOICES as REDIRECT_TYPE_CHOICES


class UserSerializer(FlexFieldsModelSerializer):
Expand Down Expand Up @@ -321,6 +322,7 @@ class ProjectLinksSerializer(BaseLinksSerializer):

versions = serializers.SerializerMethodField()
builds = serializers.SerializerMethodField()
redirects = serializers.SerializerMethodField()
subprojects = serializers.SerializerMethodField()
superproject = serializers.SerializerMethodField()
translations = serializers.SerializerMethodField()
Expand All @@ -338,6 +340,15 @@ def get_versions(self, obj):
)
return self._absolute_url(path)

def get_redirects(self, obj):
path = reverse(
'projects-redirects-list',
kwargs={
'parent_lookup_project__slug': obj.slug,
},
)
return self._absolute_url(path)

def get_builds(self, obj):
path = reverse(
'projects-builds-list',
Expand Down Expand Up @@ -451,3 +462,70 @@ def get_subproject_of(self, obj):
return self.__class__(obj.superprojects.first().parent).data
except Exception:
return None


class RedirectLinksSerializer(BaseLinksSerializer):
_self = serializers.SerializerMethodField()
project = serializers.SerializerMethodField()

def get__self(self, obj):
path = reverse(
'projects-redirects-detail',
kwargs={
'parent_lookup_project__slug': obj.project.slug,
'redirect_pk': obj.pk,
},
)
return self._absolute_url(path)

def get_project(self, obj):
path = reverse(
'projects-detail',
kwargs={
'project_slug': obj.project.slug,
},
)
return self._absolute_url(path)


class RedirectSerializerBase(serializers.ModelSerializer):

project = serializers.SlugRelatedField(slug_field='slug', read_only=True)
created = serializers.DateTimeField(source='create_dt', read_only=True)
modified = serializers.DateTimeField(source='update_dt', read_only=True)
_links = RedirectLinksSerializer(source='*', read_only=True)

type = serializers.ChoiceField(source='redirect_type', choices=REDIRECT_TYPE_CHOICES)

class Meta:
model = Redirect
fields = [
'pk',
'created',
'modified',
'project',
'type',
'from_url',
'to_url',
'_links',
]


class RedirectCreateSerializer(RedirectSerializerBase):
pass


class RedirectDetailSerializer(RedirectSerializerBase):

"""Override RedirectSerializerBase to sanitize the empty fields."""

from_url = serializers.SerializerMethodField()
to_url = serializers.SerializerMethodField()

def get_from_url(self, obj):
# Overridden only to return ``None`` when the description is ``''``
return obj.from_url or None

def get_to_url(self, obj):
# Overridden only to return ``None`` when the description is ``''``
return obj.to_url or None
1 change: 1 addition & 0 deletions readthedocs/api/v3/tests/responses/projects-detail.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
"_links": {
"_self": "https://readthedocs.org/api/v3/projects/project/",
"builds": "https://readthedocs.org/api/v3/projects/project/builds/",
"redirects": "https://readthedocs.org/api/v3/projects/project/redirects/",
"subprojects": "https://readthedocs.org/api/v3/projects/project/subprojects/",
"superproject": "https://readthedocs.org/api/v3/projects/project/superproject/",
"translations": "https://readthedocs.org/api/v3/projects/project/translations/",
Expand Down
1 change: 1 addition & 0 deletions readthedocs/api/v3/tests/responses/projects-list.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
"_self": "https://readthedocs.org/api/v3/projects/project/",
"versions": "https://readthedocs.org/api/v3/projects/project/versions/",
"builds": "https://readthedocs.org/api/v3/projects/project/builds/",
"redirects": "https://readthedocs.org/api/v3/projects/project/redirects/",
"subprojects": "https://readthedocs.org/api/v3/projects/project/subprojects/",
"superproject": "https://readthedocs.org/api/v3/projects/project/superproject/",
"translations": "https://readthedocs.org/api/v3/projects/project/translations/"
Expand Down
13 changes: 13 additions & 0 deletions readthedocs/api/v3/tests/responses/projects-redirects-detail.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"_links": {
"_self": "https://readthedocs.org/api/v3/projects/project/redirects/1/",
"project": "https://readthedocs.org/api/v3/projects/project/"
},
"created": "2019-04-29T10:00:00Z",
"modified": "2019-04-29T12:00:00Z",
"from_url": "/docs/",
"pk": 1,
"project": "project",
"type": "page",
"to_url": "/documentation/"
}
13 changes: 13 additions & 0 deletions readthedocs/api/v3/tests/responses/projects-redirects-detail_PUT.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"_links": {
"_self": "https://readthedocs.org/api/v3/projects/project/redirects/1/",
"project": "https://readthedocs.org/api/v3/projects/project/"
},
"created": "2019-04-29T10:00:00Z",
"modified": "2019-04-29T12:00:00Z",
"from_url": "/changed/",
"pk": 1,
"project": "project",
"type": "page",
"to_url": "/toanother/"
}
20 changes: 20 additions & 0 deletions readthedocs/api/v3/tests/responses/projects-redirects-list.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
{
"count": 1,
"next": null,
"previous": null,
"results": [
{
"_links": {
"_self": "https://readthedocs.org/api/v3/projects/project/redirects/1/",
"project": "https://readthedocs.org/api/v3/projects/project/"
},
"created": "2019-04-29T10:00:00Z",
"modified": "2019-04-29T12:00:00Z",
"from_url": "/docs/",
"pk": 1,
"project": "project",
"type": "page",
"to_url": "/documentation/"
}
]
}
13 changes: 13 additions & 0 deletions readthedocs/api/v3/tests/responses/projects-redirects-list_POST.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"_links": {
"_self": "https://readthedocs.org/api/v3/projects/project/redirects/2/",
"project": "https://readthedocs.org/api/v3/projects/project/"
},
"created": "2019-04-29T10:00:00Z",
"modified": "2019-04-29T12:00:00Z",
"from_url": "/page/",
"pk": 2,
"project": "project",
"type": "page",
"to_url": "/another/"
}
2 changes: 2 additions & 0 deletions readthedocs/api/v3/tests/responses/projects-subprojects-list.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
"_self": "https://readthedocs.org/api/v3/projects/project/",
"versions": "https://readthedocs.org/api/v3/projects/project/versions/",
"builds": "https://readthedocs.org/api/v3/projects/project/builds/",
"redirects": "https://readthedocs.org/api/v3/projects/project/redirects/",
"subprojects": "https://readthedocs.org/api/v3/projects/project/subprojects/",
"superproject": "https://readthedocs.org/api/v3/projects/project/superproject/",
"translations": "https://readthedocs.org/api/v3/projects/project/translations/"
Expand All @@ -90,6 +91,7 @@
"_self": "https://readthedocs.org/api/v3/projects/subproject/",
"versions": "https://readthedocs.org/api/v3/projects/subproject/versions/",
"builds": "https://readthedocs.org/api/v3/projects/subproject/builds/",
"redirects": "https://readthedocs.org/api/v3/projects/subproject/redirects/",
"subprojects": "https://readthedocs.org/api/v3/projects/subproject/subprojects/",
"superproject": "https://readthedocs.org/api/v3/projects/subproject/superproject/",
"translations": "https://readthedocs.org/api/v3/projects/subproject/translations/"
Expand Down
1 change: 1 addition & 0 deletions readthedocs/api/v3/tests/responses/projects-superproject.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
"_links": {
"_self": "https://readthedocs.org/api/v3/projects/project/",
"builds": "https://readthedocs.org/api/v3/projects/project/builds/",
"redirects": "https://readthedocs.org/api/v3/projects/project/redirects/",
"subprojects": "https://readthedocs.org/api/v3/projects/project/subprojects/",
"superproject": "https://readthedocs.org/api/v3/projects/project/superproject/",
"translations": "https://readthedocs.org/api/v3/projects/project/translations/",
Expand Down
1 change: 1 addition & 0 deletions readthedocs/api/v3/tests/responses/projects-versions-builds-list_POST.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
"_links": {
"_self": "https://readthedocs.org/api/v3/projects/project/",
"builds": "https://readthedocs.org/api/v3/projects/project/builds/",
"redirects": "https://readthedocs.org/api/v3/projects/project/redirects/",
"subprojects": "https://readthedocs.org/api/v3/projects/project/subprojects/",
"superproject": "https://readthedocs.org/api/v3/projects/project/superproject/",
"translations": "https://readthedocs.org/api/v3/projects/project/translations/",
Expand Down
Loading