restapi: decouple ProjectViewSet from the pk #4192
Conversation
|
As a side note let me add that v1 api has the undocumented property that it supports out of the box using the slug as primary key that the v2 api does not have. And our users like the slug a lot more than the pk. |
|
Updated the PR after reivew |
There was a problem hiding this comment.
This is altering our queryset in an insecure way, you should look into the Project querysets to see the use case there. We extend this further with our commercial hosting, so this is a breaking change at the moment.
In general, it's best to start off a pull request as a design discussion first. This sort of issue doesn't benefit our prod environment, so doesn't have a good chance of being merged without discussion first.
Also, because this is off our roadmap, it will be a while before merge. We give precedence to issues and PRs on our roadmap.
| @decorators.detail_route() | ||
| def valid_versions(self, request, **kwargs): | ||
| """Maintain state of versions that are wanted.""" | ||
| project = get_object_or_404( | ||
| Project.objects.api(request.user), pk=kwargs['pk']) |
There was a problem hiding this comment.
The Project.objects.api queryset is required. This shouldn't be altered.
There was a problem hiding this comment.
UserSelectViewSet.get_queryset looks very much the same code
There was a problem hiding this comment.
Project.objects.api is overriden in our commercial site to handle different kind of permissions. That's the problem here.
This is the code used to extend this class from outside:
Even if get_queryset is too similar here, it will break our commercial site and we will need to refactor that code also.
I hope that helps you to understand why we can't accept this change for now.
There was a problem hiding this comment.
What am trying to tell you is that UserSelectViewSet.get_queryset is a fancy way to write return Project.objects.api(self.request.user).
https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/restapi/views/model_views.py#L79
|
Can we have the request changes removed reconsidered after my answer please? |
|
Ping :) |
|
@agjohnson care to reconsider your review after #4192 (comment) please? This one would make #4286 trivial for projects endpoints at least |
|
Ping |
|
@xrmx its in our radar. please wait some days! |
|
I'm not convinced yet that this is something that we need to change in our code base.
Because of this problem, among others, we are creating APIv3. So, if this PR doesn't fix a bug I think we shouldn't make this changes to be able to extend the API code for something that will be covered in APIv3 and put this energy in that PR instead. |
|
@humitos This PR cleans the code up to make it reusable instead of requiring people to copy & paste it. It's not extending the API so it's not adding more maintenance burden for you. |
xrmx
force-pushed
the
decoupleprojectsapi
branch
from
5a904bf
to
0f0a7fa
Compare
|
Rebased to fix conflict errors |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
We may want to hook the very same viewset under a different router in order to lookup by slug and not pk. If we use lookup_field instead of hardcoding it in the views, it becomes easier. While at it reuse get_queryset() method for filtering the queryset.
xrmx
force-pushed
the
decoupleprojectsapi
branch
from
0f0a7fa
to
c36d4af
Compare
|
Rebased, any chance this can be merged please? |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
We may want to hook the very same viewset under a different
router in order to lookup by slug and not pk. If we use
lookup_field instead of hardcoding it in the views, it becomes
easier.