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

Improve Proxito 404 handler to render user-facing Maze when needed #6726

Merged
merged 4 commits into from Mar 3, 2020

Conversation

humitos
Copy link
Member

@humitos humitos commented Feb 28, 2020

Use a specific 404 handler in El Proxito to decide if it's called to
be executed as a NGINX internal redirect (fast_404 view) or if was
called to render the 404 user-facing page (ServeError404 view).

This way, we keep the fast 404 view for all our internal handling but
we use the standard Django flow to render 404 pages for user.

Closes #6721

Use a specific 404 handler in El Proxito to decide if it's called to
be executed as a NGINX internal redirect (`fast_404` view) or if was
called to render the 404 user-facing page (`ServeError404` view).

This way, we keep the fast 404 view for all our internal handling but
we use the standard Django flow to render 404 pages for user.
@humitos humitos requested a review from Feb 28, 2020
"""

if request.resolver_match.url_name != 'proxito_404_handler':
return fast_404(request, exception, template_name)
Copy link
Member

@ericholscher ericholscher Feb 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When would this ever be True? It's only configured on our proxito views, so it seems like we could just use the normal RTD 404 handler here, no?

Copy link
Member Author

@humitos humitos Feb 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you hit a URL of a non existing project (*) there are going to be two 404 happening there:

  1. internal NGINX redirect, produced at decorator project mapping:
    raise Http404('Project does not exist.')
  2. user-facing 404, in our ServeError404 view itself:
    raise Http404('No custom 404 page found.')

The 1) is going to execute fast_404 and 2) is going to execute the user-facing one.

We could use always the user-facing one, but we "loose the speed" of the fast_404 in those cases. I'm fine with that, since it's simpler, though.

(*) any 404 happening inside the ServeDocs should fastly respond 404 so it's caught by our own ServeError404 handler. In the end, the last 404 which will be shown to the user must be the one that calls render(template), all the others could be the fast one.

Copy link
Member

@ericholscher ericholscher left a comment

I'm fine with this change for now, and we can adjust later. We should at least fix tests tho.

@humitos
Copy link
Member Author

@humitos humitos commented Mar 3, 2020

I fixed two tests, but there is one that seems unrelated to my changes. It needs to be fixed, though since it's the one that checks for an infinite redirect --but looks that some change needs to be done in the code, not just in the test.

We can not call `proxito_404_handler` URL anymore with this setup
because we override the handler404 for these tests and we are raising
an Http404 exception with the new code. That means that the
`ServeError404` is executed twice and the second one includes an
invalid `proxito_path` argument.

We can just call the regular URL now that we raise an Http404 inside
the `ServeError404` view when there is no custom 404 page.
@humitos humitos merged commit 1c15da1 into master Mar 3, 2020
3 checks passed
@humitos humitos deleted the humitos/default-404-page branch Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants