core: fix MultipleObjectsReturned in views.serve.serve.docs #4356

xrmx · 2018-07-11T09:49:04Z

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 a project admin

Fix #4350

xrmx · 2018-07-24T08:36:42Z

Ping

RichardLitt · 2018-07-25T19:20:24Z

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.

xrmx · 2018-07-25T20:27:44Z

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.

stsewd · 2018-07-26T05:47:39Z

@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 :(

humitos · 2018-08-15T22:56:13Z

readthedocs/core/views/serve.py

    try:
-        version = project.versions.public(request.user).get(slug=version_slug)
+        version = project.versions.get(slug=version_slug)


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 · 2018-11-01T11:52:34Z

Codecov Report

Merging #4356 into master will decrease coverage by 2.31%.
The diff coverage is 50%.

@@            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

Impacted Files	Coverage Δ
readthedocs/core/views/serve.py	`86.4% <50%> (-2.26%)`	⬇️
readthedocs/search/utils.py	`0% <0%> (-53.66%)`	⬇️
readthedocs/core/models.py	`74.19% <0%> (-20.26%)`	⬇️
readthedocs/search/parse_json.py	`57.14% <0%> (-11.52%)`	⬇️
readthedocs/vcs_support/backends/svn.py	`27.69% <0%> (-9.81%)`	⬇️
readthedocs/restapi/urls.py	`80% <0%> (-9.66%)`	⬇️
readthedocs/core/utils/tasks/permission_checks.py	`28.57% <0%> (-8.93%)`	⬇️
readthedocs/restapi/utils.py	`91.3% <0%> (-8.7%)`	⬇️
readthedocs/builds/models.py	`75.6% <0%> (-8.22%)`	⬇️
readthedocs/doc_builder/config.py	`91.89% <0%> (-8.11%)`	⬇️
... and 159 more

xrmx · 2018-11-01T11:53:49Z

Rebased the PR on latest master

stale · 2019-01-10T15:29:52Z

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.

xrmx · 2019-01-10T15:32:28Z

Dear stale bot i plan to refresh this when i have time.

stale · 2019-02-24T15:44:16Z

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.

xrmx · 2019-02-24T19:03:55Z

stale bot please keep it open

stsewd · 2019-03-05T23:13:23Z

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.

xrmx · 2019-03-06T08:58:57Z

@stsewd yeah, still there's something fishy with tests

stale · 2019-04-20T09:38:30Z

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

stale · 2019-06-04T09:58:37Z

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.

xrmx · 2019-06-04T15:39:50Z

This is green, missing review :)

stsewd · 2019-06-11T01:26:01Z

Today I had this problem, digging a little more, the real issue comes from

https://github.com/rtfd/readthedocs.org/blob/45df7fd0da44be9eab3c0cb2888f6a9a15421fc5/readthedocs/builds/querysets.py#L22-L24

get_objects_for_user returns all versions of all its projects, but in the serve_docs function

https://github.com/rtfd/readthedocs.org/blob/45df7fd0da44be9eab3c0cb2888f6a9a15421fc5/readthedocs/core/views/serve.py#L205

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

stsewd · 2019-06-11T05:24:10Z

I have a fix with another approach in #5783

xrmx changed the title ~~core: fix MultipleObjectsReturned in core.views.serve.serve.docs~~ core: fix MultipleObjectsReturned in views.serve.serve.docs Jul 11, 2018

xrmx force-pushed the servedocs4350 branch from 5205ba5 to 4f142c5 Compare July 11, 2018 12:35

humitos reviewed Aug 15, 2018

View reviewed changes

xrmx force-pushed the servedocs4350 branch from 4f142c5 to 6c1402e Compare November 1, 2018 11:43

xrmx force-pushed the servedocs4350 branch 2 times, most recently from 523bfb8 to ff9ad5d Compare November 3, 2018 15:20

stsewd added the Needed: tests Tests are required label Nov 21, 2018

stale bot added the Status: stale Issue will be considered inactive soon label Jan 10, 2019

stale bot removed the Status: stale Issue will be considered inactive soon label Jan 10, 2019

stale bot added the Status: stale Issue will be considered inactive soon label Feb 24, 2019

stale bot removed the Status: stale Issue will be considered inactive soon label Feb 24, 2019

stale bot added the Status: stale Issue will be considered inactive soon label Apr 20, 2019

xrmx force-pushed the servedocs4350 branch from ff9ad5d to 33ef9fe Compare April 20, 2019 09:48

stale bot removed the Status: stale Issue will be considered inactive soon label Apr 20, 2019

stale bot added the Status: stale Issue will be considered inactive soon label Jun 4, 2019

dojutsu-user removed the Status: stale Issue will be considered inactive soon label Jun 4, 2019

ericholscher added the Accepted Accepted issue on our roadmap label Jun 10, 2019

stsewd mentioned this pull request Jun 11, 2019

Use querysets from the class not from an instance #5783

Merged

stsewd closed this in #5783 Jun 13, 2019

xrmx mentioned this pull request Aug 27, 2019

Aggiornamento alla versione più recente di ReadTheDocs italia/docs.italia.it#251

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: fix MultipleObjectsReturned in views.serve.serve.docs #4356

core: fix MultipleObjectsReturned in views.serve.serve.docs #4356

xrmx commented Jul 11, 2018 •

edited

Loading

xrmx commented Jul 24, 2018

RichardLitt commented Jul 25, 2018

xrmx commented Jul 25, 2018

stsewd commented Jul 26, 2018

humitos Aug 15, 2018

codecov bot commented Nov 1, 2018 •

edited

Loading

xrmx commented Nov 1, 2018

stale bot commented Jan 10, 2019

xrmx commented Jan 10, 2019

stale bot commented Feb 24, 2019

xrmx commented Feb 24, 2019

stsewd commented Mar 5, 2019

xrmx commented Mar 6, 2019

stale bot commented Apr 20, 2019

stale bot commented Jun 4, 2019

xrmx commented Jun 4, 2019 •

edited

Loading

stsewd commented Jun 11, 2019

stsewd commented Jun 11, 2019

core: fix MultipleObjectsReturned in views.serve.serve.docs #4356

core: fix MultipleObjectsReturned in views.serve.serve.docs #4356

Conversation

xrmx commented Jul 11, 2018 • edited Loading

xrmx commented Jul 24, 2018

RichardLitt commented Jul 25, 2018

xrmx commented Jul 25, 2018

stsewd commented Jul 26, 2018

humitos Aug 15, 2018

Choose a reason for hiding this comment

codecov bot commented Nov 1, 2018 • edited Loading

Codecov Report

xrmx commented Nov 1, 2018

stale bot commented Jan 10, 2019

xrmx commented Jan 10, 2019

stale bot commented Feb 24, 2019

xrmx commented Feb 24, 2019

stsewd commented Mar 5, 2019

xrmx commented Mar 6, 2019

stale bot commented Apr 20, 2019

stale bot commented Jun 4, 2019

xrmx commented Jun 4, 2019 • edited Loading

stsewd commented Jun 11, 2019

stsewd commented Jun 11, 2019

xrmx commented Jul 11, 2018 •

edited

Loading

codecov bot commented Nov 1, 2018 •

edited

Loading

xrmx commented Jun 4, 2019 •

edited

Loading