Proxito: use unresolver in 404 handler #10074

stsewd · 2023-02-27T22:40:52Z

Moved the logic around checking if an index redirect can be done and finding a custom 404 file into functions, so they can be re-used in both implementations.
Logic is the same
One small change is that instead of checking if the version slug is defined now we are checking if we were able to find a version object, otherwise looking for an index file or custom 404 page in a version that doesn't exist will be useless (this doesn't change the logic, just an optimization).

This is on top of #10037 and requires #10044. I'll update this PR once those are merged.

readthedocs/proxito/views/serve.py

benjaoming · 2023-03-01T15:07:04Z

@stsewd heads up, I've added some commits here to make the refactor work, I'll take it for a test run and push changes directly unless you're working on it right now?

…e-proxito-v2-in-404-handler

benjaoming · 2023-03-01T15:18:44Z

The PR currently looks a bit crazy until refactor-unresolver also has merged in the latest main 👍

…ocs.org into use-proxito-v2-in-404-handler

stsewd · 2023-03-02T23:15:38Z

readthedocs/proxito/tests/test_old_redirects.py

+
+    def test_redirect_prefix_crossdomain_with_newline_chars(self):
+        """
+        Overridden to make it compatible with the new implementation.


We could also make the old implementation compatible, isn't a lot of code, but I'm not sure if we want to spend more time there... (but kind of already spent time debugging it :_)

"The old implementation", is the one we are going to delete, right?

stsewd · 2023-03-03T00:16:49Z

readthedocs/proxito/views/serve.py

+        project = None
+        version = None
+        filename = None
+        lang_slug = None
+        version_slug = None


This is ready for review... but I don't like that we are defining lots of variables that are kind of related, this is kind of a copy/paste of the old implementation, but maybe there is a better way...

They are mainly needed for custom redirects, maybe the answer is there... if we can simplify the redirects code.

And, yeah, we should be able to remove version_slug and lang_slug with #10112 :)

I think we can do x = y = z = None, at least? But not a huge deal, I'm fine with the more explicit version, also :)

yeah, I don't like too much the one-line assignment too much because it's harder to read the name of the variables. But that's a personal preference 😄

ericholscher · 2023-03-06T22:45:28Z

Is this ready to review? How does it relate to #9657?

ericholscher

This is a useful refactor, but want to make sure we aren't losing important context/comments in the refactor. Probably just needs to be compared again current main, and adapted from all the previous reviews.

Really great work knocking down these refactors @stsewd 🎉

readthedocs/core/unresolver.py

readthedocs/proxito/views/serve.py

ericholscher · 2023-03-06T22:50:25Z

readthedocs/proxito/views/serve.py

+                return HttpResponseRedirect(redirect_url)
+
+        return None
+


I didn't review the functions above too closely, since they are just copy/pasted from inside the views, right?

yeah logic is the same, copy/paste, just the variable names changed final_project -> project, version_slug -> version.slug and so.

ericholscher · 2023-03-06T22:51:37Z

readthedocs/proxito/views/serve.py

+        # We force all storage calls to use the external versions storage,
+        # since we are serving an external version.
+        if unresolved_domain.is_from_external_domain:
+            self.version_type = EXTERNAL


I wonder if there is a cleaner way to do this? Could we explicitly set a queryset manager or something here, instead of depending on the downstream code getting this passed in and interpretting it properly? Not 100% sure that makes sense, but would be good to be extra explicit to avoid issues around this.

The version that results from the unresolve_path() call already is validated to use the correct manager, this is more like an extra protection (and to reuse the same methods with the older implementation)

Cool 👍 . Probably worth noting that in the comment as well (I might like comments too much, but mission-critical code is important note this stuff), since I thought this was SECURITY CRITICAL, instead of Be extra defensive...

Suggested change

# We force all storage calls to use the external versions storage,

# since we are serving an external version.

if unresolved_domain.is_from_external_domain:

self.version_type = EXTERNAL

# We force all storage calls to use the external versions storage,

# since we are serving an external version.

# The version that results from the unresolve_path() call already is validated to use the correct manager,

# this is here to add defense in depth against serving the wrong version.

if unresolved_domain.is_from_external_domain:

self.version_type = EXTERNAL

ericholscher · 2023-03-06T22:52:12Z

readthedocs/proxito/views/serve.py

+        project = None
+        version = None
+        filename = None
+        lang_slug = None
+        version_slug = None


I think we can do x = y = z = None, at least? But not a huge deal, I'm fine with the more explicit version, also :)

ericholscher · 2023-03-06T22:52:57Z

readthedocs/proxito/views/serve.py

+        except VersionNotFoundError as exc:
+            project = exc.project
+            lang_slug = project.language
+            version_slug = exc.version_slug
+            filename = exc.filename
+        except TranslationNotFoundError as exc:
+            project = exc.project
+            lang_slug = exc.language
+            version_slug = exc.version_slug
+            filename = exc.filename
+        except InvalidExternalVersionError as exc:
+            project = exc.project
+        except InvalidPathForVersionedProjectError as exc:


These could use comments explaining what the actual case here is. I can kind of tell from the explicit naming, but I think a bit more context is useful (eg. when might these things happen? Why do we only care about a subset of the variables in those cases?)

Put a comment at the top of the try, since an exception happened, we fill the other variables with the information that we have available.

ericholscher · 2023-03-06T22:53:33Z

readthedocs/proxito/views/serve.py

+        request.path_project_slug = project.slug
+        request.path_version_slug = version_slug


These variable names seem not super correct? Are we sure these were parsed from a path?

yeah, the names aren't correct, we sometimes pass the slug from the external version domain or the default version on single version projects. But this is what we expect in the middleware

Probably an area for further refactor, but not important in this PR. 👍

readthedocs/proxito/views/serve.py

stsewd · 2023-03-07T00:05:42Z

Is this ready to review? How does it relate to #9657?

This is ready, #9657 uses this branch as base, and it can be reviewed/merged separately.

ericholscher

This looks good to merge after my comment suggestions are added in :)

humitos

Looks good to me. I left some small comments, tho.

readthedocs/core/unresolver.py

humitos · 2023-03-07T14:41:35Z

readthedocs/proxito/tests/test_old_redirects.py

+
+    def test_redirect_prefix_crossdomain_with_newline_chars(self):
+        """
+        Overridden to make it compatible with the new implementation.


"The old implementation", is the one we are going to delete, right?

humitos · 2023-03-07T14:44:57Z

readthedocs/proxito/views/serve.py

+        # If we were able to resolve to a valid version, it means that the
+        # current file doesn't exist. So we check if we can redirect to its
+        # index file if it exists before doing anything else.


What do you mean by "redirect ot its index file"? /en/latest/file/ -> /en/latest/file/index.html?

If that's correct, can you expand the comment with an example similar to the one I wrote?

humitos · 2023-03-07T14:50:28Z

readthedocs/proxito/views/serve.py

+        project = None
+        version = None
+        filename = None
+        lang_slug = None
+        version_slug = None


yeah, I don't like too much the one-line assignment too much because it's harder to read the name of the variables. But that's a personal preference 😄

stsewd added 8 commits February 16, 2023 17:25

Refactor unresolver

c505544

Merge branch 'main' into refactor-unresolver

d2eadec

Move external version checks inside unresolver

14bdbd2

Merge branch 'main' into refactor-unresolver

b807b2d

Merge branch 'main' into refactor-unresolver

b5006dc

Updates from review

36b9f02

Merge branch 'main' into refactor-unresolver

47338e9

Proxito: use unresolved in 404 handler

38496bc

benjaoming reviewed Feb 28, 2023

View reviewed changes

readthedocs/proxito/views/serve.py Show resolved Hide resolved

ericholscher linked an issue Feb 28, 2023 that may be closed by this pull request

Proxito: Next steps on Improve URL Handling #9911

Closed

stsewd and others added 4 commits February 28, 2023 17:12

Merge branch 'main' into refactor-unresolver

46496df

Updates from review

69f4886

Missing imports

2184112

Linting: Too many blank lines

ce28e12

Merge branch 'main' of github.com:readthedocs/readthedocs.org into us…

3c8b76b

…e-proxito-v2-in-404-handler

Merge branch 'refactor-unresolver' of github.com:readthedocs/readthed…

29a560c

…ocs.org into use-proxito-v2-in-404-handler

Base automatically changed from refactor-unresolver to main March 2, 2023 00:04

stsewd added 6 commits March 1, 2023 20:20

Merge branch 'main' into use-proxito-v2-in-404-handler

3085d95

Remove duplicated imports

05384be

Black

7af491a

Fix exc

86f67b2

Update tests

2d92061

Merge branch 'main' into use-proxito-v2-in-404-handler

f5b2db0

stsewd commented Mar 2, 2023

View reviewed changes

Move check to new implementation only

c017503

stsewd commented Mar 3, 2023

View reviewed changes

benjaoming mentioned this pull request Mar 3, 2023

Contextualize 404 page #9657

Merged

8 tasks

stsewd marked this pull request as ready for review March 6, 2023 19:33

stsewd requested a review from a team as a code owner March 6, 2023 19:33

stsewd requested a review from humitos March 6, 2023 19:33

auto-assign bot assigned stsewd Mar 6, 2023

stsewd requested a review from ericholscher March 6, 2023 19:33

ericholscher reviewed Mar 6, 2023

View reviewed changes

Updates from review

be22848

ericholscher approved these changes Mar 7, 2023

View reviewed changes

humitos approved these changes Mar 7, 2023

View reviewed changes

Updates from review

03bbc6d

stsewd merged commit 8c462bd into main Mar 7, 2023

stsewd deleted the use-proxito-v2-in-404-handler branch March 7, 2023 18:54

benjaoming restored the use-proxito-v2-in-404-handler branch March 8, 2023 10:21

benjaoming deleted the use-proxito-v2-in-404-handler branch March 8, 2023 10:22

benjaoming restored the use-proxito-v2-in-404-handler branch March 8, 2023 10:31

benjaoming deleted the use-proxito-v2-in-404-handler branch March 8, 2023 10:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proxito: use unresolver in 404 handler #10074

Proxito: use unresolver in 404 handler #10074

stsewd commented Feb 27, 2023

benjaoming commented Mar 1, 2023

benjaoming commented Mar 1, 2023

stsewd Mar 2, 2023

humitos Mar 7, 2023

stsewd Mar 7, 2023

stsewd Mar 3, 2023

stsewd Mar 3, 2023 •

edited

Loading

stsewd Mar 6, 2023

ericholscher Mar 6, 2023 •

edited

Loading

humitos Mar 7, 2023

ericholscher commented Mar 6, 2023 •

edited

Loading

ericholscher left a comment •

edited

Loading

ericholscher Mar 6, 2023

stsewd Mar 6, 2023

ericholscher Mar 6, 2023

stsewd Mar 6, 2023

ericholscher Mar 7, 2023 •

edited

Loading

ericholscher Mar 6, 2023 •

edited

Loading

ericholscher Mar 6, 2023 •

edited

Loading

stsewd Mar 7, 2023

ericholscher Mar 6, 2023

stsewd Mar 6, 2023

ericholscher Mar 7, 2023

stsewd commented Mar 7, 2023

ericholscher left a comment

humitos left a comment

humitos Mar 7, 2023

humitos Mar 7, 2023

humitos Mar 7, 2023

		request.path_project_slug = project.slug
		request.path_version_slug = version_slug

Proxito: use unresolver in 404 handler #10074

Proxito: use unresolver in 404 handler #10074

Conversation

stsewd commented Feb 27, 2023

benjaoming commented Mar 1, 2023

benjaoming commented Mar 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericholscher Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericholscher commented Mar 6, 2023 • edited Loading

ericholscher left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericholscher Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

ericholscher Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

ericholscher Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd commented Mar 7, 2023

ericholscher left a comment

Choose a reason for hiding this comment

humitos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd Mar 3, 2023 •

edited

Loading

ericholscher Mar 6, 2023 •

edited

Loading

ericholscher commented Mar 6, 2023 •

edited

Loading

ericholscher left a comment •

edited

Loading

ericholscher Mar 7, 2023 •

edited

Loading

ericholscher Mar 6, 2023 •

edited

Loading

ericholscher Mar 6, 2023 •

edited

Loading