core: fix MultipleObjectsReturned in views.serve.serve.docs #4356
Conversation
xrmx
changed the title
|
Ping |
|
Thanks @xrmx. I'm speaking for myself here, but I'm not sure there's a need to ping for cases like these. The maintainers know the PR is here, and they'll get to it when they can. |
|
Sorry, I don't want to nag but the time i can spend on RTD is limited and will end next month so I'd like to push the more fixes i can upstream. |
|
@xrmx I really appreciate that. And I also very sorry, the current core team is in a very large migration, so probably this (and others) PR will not merge very soon :( |
| try: | ||
| version = project.versions.public(request.user).get(slug=version_slug) | ||
| version = project.versions.get(slug=version_slug) |
There was a problem hiding this comment.
This change needs to be looked at carefully since .public has a different meaning than .get under our corporate site.
I'm not saying that it's wrong, but just making notes about it when proper review.
Codecov Report
@@ Coverage Diff @@
## master #4356 +/- ##
=========================================
- Coverage 78.72% 76.4% -2.32%
=========================================
Files 164 158 -6
Lines 10107 9991 -116
Branches 1282 1263 -19
=========================================
- Hits 7957 7634 -323
- Misses 1820 2017 +197
- Partials 330 340 +10
|
|
Rebased the PR on latest master |
xrmx
force-pushed
the
servedocs4350
branch
2 times, most recently
from
523bfb8 to
ff9ad5d
Compare
|
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. |
|
Dear stale bot i plan to refresh this when i have time. |
|
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. |
|
stale bot please keep it open |
|
I faced this problem again locally. The solution looks correct, but I still not sure why this is happening randomly, makes me think the problem is in some other place, maybe we are mutating something. |
|
@stsewd yeah, still there's something fishy with tests |
|
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 get this exception while trying to serve some docs:
MultipleObjectsReturned: get() returned more than one Version -- it returned 2!
File "django/core/handlers/base.py", line 149, in get_response
response = self.process_exception_by_middleware(e, request)
File "django/core/handlers/base.py", line 147, in get_response
response = wrapped_callback(request, *callback_args, **callback_kwargs)
File "readthedocs/core/views/serve.py", line 96, in inner_view
return view_func(request, project=project, *args, **kwargs)
File "readthedocs/core/views/serve.py", line 74, in inner_view
return view_func(request, subproject=subproject, *args, **kwargs)
File "readthedocs/core/views/serve.py", line 156, in serve_docs
version = project.versions.public(request.user).get(slug=version_slug)
File "django/db/models/query.py", line 391, in get
(self.model._meta.object_name, num)
The view code looks legit at a first look but there's a huge side
effect of the user filtering in the public() method. It does not
filter the projects by user but it adds to the queryset all the
other user projects which is not what we need here.
Instead simplify the code to:
- return 404 if the requested version does not exist
- return 401 if the version is private and the user is not admin
- serve the file if the version is private and the user is the admin
Fix readthedocs#4350
|
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. |
|
This is green, missing review :) |
|
Today I had this problem, digging a little more, the real issue comes from
we expect to only have versions of one project. |
When filtering using `public` and using a user, the queryset hit this https://github.com/rtfd/readthedocs.org/blob/45df7fd0da44be9eab3c0cb2888f6a9a15421fc5/readthedocs/builds/querysets.py#L22-L24 When the user is authenticated, we call to `get_objects_for_user` which gets all the versions from all the user's projects. Overriding any previous filter (`project.versions` in this case) We don't see this in production because we serve from another domain. And we don't see this in the corporate site because we override the serve_docs view. Fix readthedocs#4350 Closes readthedocs#4356
|
I have a fix with another approach in #5783 |
We get this exception while trying to serve some docs:
The view code looks legit at a first look but there's a huge side
effect of the user filtering in the public() method. It does not
filter the projects by user but it adds to the queryset all the
other user projects which is not what we need here.
Instead simplify the code to:
Fix #4350