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

Add ability for users to set their own URLConf #6963

Merged
merged 35 commits into from May 26, 2020
Merged

Conversation

ericholscher
Copy link
Member

@ericholscher ericholscher commented Apr 25, 2020

This lets users configure how they want their URLs to look.
This could include not having a version or language,
or having a different subpath for subprojects.

Refences #6868

Example

This serves docs from http://kong.community.dev.readthedocs.io/latest/index.html by settings project.urlconf = '$version/$filename.

Screenshot 2020-04-25 14 31 36

From the logs:

[25/Apr/2020 21:31:19] readthedocs.proxito.middleware:134[586]: INFO Setting URLConf: project=Kong, urlconf=$version/$filename, real_urlconf=(?P<version_slug>(?:[a-z0-9A-Z][-._a-z0-9A-Z]*?))/(?P<filename>(?:.*))

TODO

There's still a bunch of work we need to do to make this work:

  • Make the resolver respect the users URLs
  • Change a lot of our other logic in places that depends on existing URL structure
  • Lots and lots of QA testing

This lets users configure how they want their URLs to look.
This could include not having a version or language,
or having a different subpath for subprojects.

Refences #6868
@ericholscher ericholscher requested a review from Apr 25, 2020
@ericholscher ericholscher marked this pull request as draft Apr 25, 2020
urlpatterns = [
re_path(project.real_urlconf, ServeDocs.as_view()),
re_path('^' + project.real_urlconf, ServeDocs.as_view()),
re_path('^/' + project.real_urlconf, ServeDocs.as_view()),
Copy link
Member Author

@ericholscher ericholscher Apr 25, 2020

Choose a reason for hiding this comment

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

Just for debugging

Copy link
Contributor

@agjohnson agjohnson May 1, 2020

Choose a reason for hiding this comment

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

I had to bring in the 404, 500, and all of the proxito urls here as well.

Copy link
Member Author

@ericholscher ericholscher May 21, 2020

Choose a reason for hiding this comment

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

@agjohnson I didn't need the 404/500 yet, but I might be missing something.

Copy link
Member Author

@ericholscher ericholscher May 21, 2020

Choose a reason for hiding this comment

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

I've gone ahead and added them.

)

# Stop Django from caching URLs
ns = time.mktime(time.gmtime())
Copy link
Member Author

@ericholscher ericholscher Apr 25, 2020

Choose a reason for hiding this comment

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

This would be the model modified date probably

Copy link
Contributor

@agjohnson agjohnson left a comment

This seems like a reasonable approach. Manually manipulating sys.modules could be fragile, though I don't have a good idea of what exactly our concerns would be here. If we have concerns, we could dynamically instantiate objects once on startup, giving real dotted paths to these objects for request.urlconf. I'm not too worried unless someone has a good idea of the downfalls of patching sys.modules on each request.

@ericholscher ericholscher marked this pull request as ready for review May 20, 2020
@ericholscher ericholscher requested a review from May 20, 2020
@ericholscher ericholscher added the Needed: tests label May 20, 2020
@ericholscher ericholscher removed the Needed: tests label May 21, 2020
Copy link
Member

@humitos humitos left a comment

I haven't tested yet locally, but it seems it works.

I'd like to understand more the approach we are following and why. I left some questions for this.

Also, I made some suggestions to improve code readability since it took me some time to realize what was each thing --they have names that are confusing to me. Would be good if we can improve the naming before merging.

null=True,
help_text=_(
'Supports the following keys: $language, $version, $subproject, $filename. '
'An example `$language/$version/$filename`.'
Copy link
Member

@humitos humitos May 25, 2020

Choose a reason for hiding this comment

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

Suggested change
'An example `$language/$version/$filename`.'
'Default: `$language/$version/$filename`. '

Copy link
Member Author

@ericholscher ericholscher May 26, 2020

Choose a reason for hiding this comment

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

Not sure why Default: is better here? There isn't a default for this field.

Copy link
Member

@humitos humitos May 26, 2020

Choose a reason for hiding this comment

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

Because that URL is what Read the Docs uses by default: /en/latest/filename.html. It's not the default of the field, but that's the help text presented to the user.

url_prefix = self.urlconf.split('$', 1)[0]
return '/' + url_prefix.strip('/') + '/_'
Copy link
Member

@humitos humitos May 25, 2020

Choose a reason for hiding this comment

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

I'm not able to understand what this method does. Can you expand the docstring and add an example?

It seems like /hello/$language/ will be /hello/_, is that correct? In that case, why do we need this?

Also, that seems more like a path than a host. We should change the name in that case, maybe url_prefix or similar.

Copy link
Member Author

@ericholscher ericholscher May 26, 2020

Choose a reason for hiding this comment

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

What part don't you understand? It's passed as proxied_api_host in JS, which is where we hit the backend proxied API's.

It's a path in this case, but it can also be a fully formed URL, so we're using the existing name.

readthedocs/projects/models.py Outdated Show resolved Hide resolved
readthedocs/projects/models.py Outdated Show resolved Hide resolved
readthedocs/projects/models.py Outdated Show resolved Hide resolved
r'{proxied_api_url}api/v2/'.format(proxied_api_url=self.proxied_api_url),
include('readthedocs.api.v2.proxied_urls'),
name='fake_proxied_api'
Copy link
Member

@humitos humitos May 25, 2020

Choose a reason for hiding this comment

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

Why do we need to proxy these URLs with /hello/_/ prefix (following my previous comment)? Why not just use _/ as we are currently doing?

Copy link
Member Author

@ericholscher ericholscher May 26, 2020

Choose a reason for hiding this comment

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

Because we need to be able to support proxied API's at non-root URLs for a users who want to use a proxied subpath docs.company.com/rtd-hosted-project/_/

readthedocs/proxito/middleware.py Outdated Show resolved Hide resolved
readthedocs/proxito/middleware.py Outdated Show resolved Hide resolved
readthedocs/projects/models.py Outdated Show resolved Hide resolved
@ericholscher ericholscher merged commit 763894f into master May 26, 2020
1 check passed
@ericholscher ericholscher deleted the project-urlconf branch May 26, 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.

None yet

3 participants