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

APIv3 CRUD for Redirect objects #5879

Merged
merged 10 commits into from Jul 9, 2019
Merged

APIv3 CRUD for Redirect objects #5879

merged 10 commits into from Jul 9, 2019

Conversation

@humitos
Copy link
Member

@humitos humitos commented Jul 2, 2019

  • CRUD for Redirect objects at /api/v3/projects/<project_slug>/redirects/
  • These objects are accessed only by admins of the projects (listing and detail)
    • I'm not sure if it does make sense to use the same pattern of "Public Detail - Private Listing" on these
    • We are not exposing these objects to any other user that are not the maintainers via the WebUI
  • Adapt RedirectQuerySet to follow the .api method pattern
    • .public, .private, etc are not needed because they do not make sense (considering the previous item)
  • Move redirects/managers.py to redirects/querysets.py to be consistent with the other apps
  • Use combined permission_classes, like after the refactor at #5858
  • Minimal DB query optimization to use .only in one of the methods that just checks for in
  • Add some tests for Redirect endpoints
@humitos humitos requested a review from Jul 2, 2019
Copy link
Contributor

@davidfischer davidfischer left a comment

I know this is a work in progress but it is looking pretty good to me.

One thing that we should probably handle is that the queryset is not using select_related. Perhaps we could have a get_queryset method like this:

def get_queryset(self):
    qs = super().get_queryset()
    qs = qs.select_related('project')
    return qs

@@ -29,3 +29,13 @@ def has_permission(self, request, view):
return True

return False


class IsProjectAdmin(BasePermission):
Copy link
Contributor

@davidfischer davidfischer Jul 3, 2019

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

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

@humitos humitos Jul 3, 2019

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

@davidfischer davidfischer Jul 3, 2019

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

@humitos humitos Jul 8, 2019

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

@davidfischer davidfischer Jul 8, 2019

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

@humitos
Copy link
Member Author

@humitos humitos commented Jul 3, 2019

One thing that we should probably handle is that the queryset is not using select_related. Perhaps we could have a get_queryset method like this:

This reduces the numbers of queries in N, where N is the number of Redirect objects returned: 💯 --at least in a simple test, but makes sense.

@humitos
Copy link
Member Author

@humitos humitos commented Jul 3, 2019

This PR is ready for review.

The same pattern followed for this Redirect resource/objects will be followed for other ones as well, like EnvironmentVariable and WebHook/EmailHook in other PRs.

@ericholscher ericholscher requested a review from davidfischer Jul 8, 2019
@humitos humitos merged commit 801f63b into master Jul 9, 2019
2 checks passed
@delete-merged-branch delete-merged-branch bot deleted the humitos/api-v3-redirect-crud branch Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants