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

Support single version subprojects URLs to serve from Django #5690

Merged
merged 5 commits into from
Jun 18, 2019

Conversation

humitos
Copy link
Member

@humitos humitos commented May 13, 2019

Currently, Read the Docs Community does not have this problem because we serve directly from NGINX without passing through Django to get the X-Accel-Redirect header in its response.

Although, on Corporate we are returning an infinite redirect when accessing /projects/subproject/ URL when subproject is a Single Version project.

  • We have a redirect_project_slug view that is called when hitting /projects/subproject/ to redirect to its default lang/version --this does not apply on single version subproject
  • This redirect URL is only registered on urls/subdomain.py file (and not on urls/single_version.py), which should not be called when accessing a single version project is hit
  • Our SingleVersionMiddleware (which overrides the request.urlconf) is not detecting that we are accessing to a single version project, because it uses the request.slug (which is the superproject) to check it. Related to Proxito: look for custom 404 pages in the parent project of subprojects #5557 (comment)

The current changes on this PR makes it work locally when USE_SUBDOMAIN=True, although it's not the best fix for this. To define this URL where it belongs to, we need to touch our SubdomainMiddleware to properly set subproject_slug and use that in different places instead --which could be a bigger refactor/improvement.

@humitos humitos added the PR: work in progress Pull request is not ready for full review label May 13, 2019
@humitos
Copy link
Member Author

humitos commented May 13, 2019

To define this URL where it belongs to, we need to touch our SubdomainMiddleware to properly set subproject_slug and use that in different places instead --which could be a bigger refactor/improvement.

In fact, this problem is more related to "Given an URL, parse it to get all the RTD objects from it" as we have talked. Currently, we are doing this at map_project_slug and map_subproject_slug (via Django URL named regex) and SubdomainMiddleware (via manipulating the host/domain and checking X-RTD-SLUG header).

Maybe it's time to work on this if we are considering the "big refactor".

@humitos humitos added the Needed: design decision A core team decision is required label May 13, 2019
@humitos humitos requested a review from a team May 13, 2019 14:54
@agjohnson
Copy link
Contributor

It's not super clear to me where the design decision lays in this PR. Perhaps it would be more helpful to put this change into more context and summarize in more detail where you think the problems are with this implementation. It's also not clear why the middleware refactor is required, so I don't feel I have enough context to offer guidance.

@humitos
Copy link
Member Author

humitos commented May 15, 2019

It's not super clear to me where the design decision lays in this PR.

Basically, if

  1. we want to deploy and go with the current solution for now (which is a hack) or,
  2. we want to improve our middleware to detect that a subproject is being accessed and set the slug of it in the request object, so other places can trust on that. For example, the SingleVersionMiddleware that sets the proper urlconf to the request in that case.

Perhaps it would be more helpful to put this change into more context and summarize in more detail where you think the problems are with this implementation.

The problem is that it does not follow our pattern of using the specific urlconf depending on project specific values (like single_version), which is confusing in the end to have this exception here. Because of this, I need to add a shortcut on redirect_project_slug because this URL is defined only for non single-version urlconf.

It's also not clear why the middleware refactor is required, so I don't feel I have enough context to offer guidance.

When accessing a subproject (e.g. docs.cname.org/projects/subproject), our middleware sets request.slug to the project that has the Domain docs.cname.org and the rest of the code thinks we are accessing that project instead of subproject. The refactor is about adding more data like request.subproject_slug to give the ability to some part of the code that depends on the subproject, use that value instead.

Besides, improving the middleware by setting this extra value(s) is useful in another places like for custom 404 page in subprojects.

@ericholscher
Copy link
Member

If this approach works, I'm +1 on shipping it and fixing a user's approach.

@ericholscher
Copy link
Member

Hopefully this code will all disappear in the next couple months, so I'm OK with hacks :)

# the index file instead.
if subproject and subproject.single_version:
# TODO: find a way to import ``serve_docs`` from corporate
return serve_docs(request, project, project.slug, subproject, subproject.slug)
Copy link
Member Author

Choose a reason for hiding this comment

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

Since this is only needed for corporate, the only missing thing in this hacky PR is to import serve_docs from corporate before merging (readthedocsinc.core.views.serve_docs).

Use the serve_docs from Corporate as a HACK and do nothing extra if it
fails for any reason
@humitos humitos force-pushed the humitos/single-version-subproject branch from 3319558 to f43ece3 Compare June 13, 2019 11:41
@humitos humitos removed Needed: design decision A core team decision is required PR: work in progress Pull request is not ready for full review labels Jun 13, 2019
@humitos
Copy link
Member Author

humitos commented Jun 13, 2019

I finished the hack for this PR. It should not affect .org at all. We should check that this works when deploying .com.

This was moved to Corporate project, where it belongs.
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

Looks fine I guess -- I really dislike this pattern, but at least it's temporary.

from readthedocsinc.core.views import serve_docs as corporate_serve_docs # noqa
return corporate_serve_docs(request, project, project.slug, subproject, subproject.slug)
except Exception:
pass
Copy link
Member

Choose a reason for hiding this comment

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

We should log here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea! I added logs in both cases.

@humitos humitos merged commit 3c3a1c3 into master Jun 18, 2019
@delete-merged-branch delete-merged-branch bot deleted the humitos/single-version-subproject branch June 18, 2019 09:17
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.

3 participants