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

Make slug field a valid DNS label #3464

Merged
merged 3 commits into from
Nov 2, 2018

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Dec 29, 2017

This PR closes #2600

Currently rtd serve the docs with the slug of the project as a subdomain, the spec says that a DNS label can contain up to 63 characters.

I don't know if there is valid project with a slug of this length on rtd.org.

  • What action should be taken on this?
  • If someone exports a project with a very long name (or put that name on purpose), what slug name should be generated? The slugify function must truncated this names?

@RichardLitt RichardLitt added the PR: work in progress Pull request is not ready for full review label Dec 30, 2017
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

Good question on how to handle long slugs. Perhaps we do something similar to our version slugging, as we can't ensure uniqueness if we just do truncating. Our version slugging adds some letters and numbers to the slug to avoid duplication, I'd say we're looking to duplicate that pattern.

There does look to be a number of unrelated migrations here. I think we might be out of sync on these. Let's remove the steps that are not necessary for this migration and deal with the other separate. I'll test on a fresh db to see what is out of date -- we haven't done this in a long time.

There are a number of projects that will likely fail the 63 character limit of this field. I'm not sure what to do about these projects -- they aren't working already, links to their documentation wouldn't work as of right now. We'll need to do something with these projects in a separate data migration first. We might need to discuss this more though.

@agjohnson agjohnson added the Needed: design decision A core team decision is required label Jan 2, 2018
@stsewd
Copy link
Member Author

stsewd commented Jan 5, 2018

@agjohnson I think is better to let the user choose the slug (see related issue #1440). Maybe changing the slug is a little further, but choosing my own slug in the project creation seems reasonable to me. This will allow the users to put an abbreviation (like rtd instead of read-the-docs) and not get a surprise with a strange slug (like very-long-project-abc for Verry long project), unreal case anyway, I don't think that a project like that is a real project (+64 characters), probably spam.

@ericholscher
Copy link
Member

Looking into this and it seems most projects of this length are spam. I'm wondering what is required to move this forward though. Currently those projects docs aren't being served because of a DNS resolution error, so it seems like we could just make a data migrations the truncated the length to ship this.

curl: (6) Could not resolve host: torrent-satyamev-jayate-2018-full-movie-850mb-download-torrent-direct-link.readthedocs.io

@ericholscher
Copy link
Member

I've audited all the existing projects which have good builds, and they are all spam. So I think we're probably safe to just ship this after rebasing the migration.

@stsewd
Copy link
Member Author

stsewd commented Nov 2, 2018

I have updated the migration and added a data migration, it works locally. But it will explode if there is a collision between slugs. So, maybe we want to implement the slugify thing first?

@codecov
Copy link

codecov bot commented Nov 2, 2018

Codecov Report

Merging #3464 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #3464   +/-   ##
=======================================
  Coverage   76.41%   76.41%           
=======================================
  Files         158      158           
  Lines        9990     9990           
  Branches     1262     1262           
=======================================
  Hits         7634     7634           
  Misses       2016     2016           
  Partials      340      340
Impacted Files Coverage Δ
readthedocs/projects/models.py 85.5% <100%> (ø) ⬆️

@ericholscher
Copy link
Member

I think we're all good:

In [5]: Project.objects.annotate(text_len=Length('slug')).filter(text_len__gt=65).count()
Out[5]: 0

@ericholscher ericholscher merged commit ab78b3a into readthedocs:master Nov 2, 2018
@ericholscher
Copy link
Member

Ah yea, I just tested this locally, and it blows up with a DB 500, so we should probably handle this in validation.

@ericholscher
Copy link
Member

Actually, we should probably just shorten the name param to 65 also, which should solve this.

@ericholscher
Copy link
Member

#4856

@stsewd stsewd deleted the slug-valid-dns-label branch November 2, 2018 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needed: design decision A core team decision is required PR: work in progress Pull request is not ready for full review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Project slug names that are not valid DNS names are not caught on project creation
4 participants