Sync version fails when two version end up with the same slug

[humitos]

When sync_versions is called if there are two verbose_name that endup with the same slug this code breaks:

https://github.com/rtfd/readthedocs.org/blob/ba7623d2ff83b2c88cecbbe55800317f46116d0d/readthedocs/restapi/utils.py#L76-L82

As example,

In [11]: VersionSlugField(populate_from='').slugify('example/')                                                                                                                               
Out[11]: 'example-'

In [12]: VersionSlugField(populate_from='').slugify('/example/')                                                                                                                              
Out[12]: 'example-'

In [13]: VersionSlugField(populate_from='').slugify('example-')                                                                                                                               
Out[13]: 'example-'

Sentry: https://sentry.io/read-the-docs/readthedocscom/issues/746103998/

[dojutsu-user]

@humitos
I would like to work on this issue.
My suggestion:
We can modify the slugify method in VersionSlugField in a way so that it generates a unique hash for each slug and append it in last of the generated slug.

These lines

https://github.com/rtfd/readthedocs.org/blob/104c3a625978f31971a11766023516fc2de2aa8f/readthedocs/builds/version_slug.py#L84-L94

can be modified in this way

def slugify(self, content):
        if not content:
            return ''

        hashed = abs(hash(content)) % (10 ** 8)
        slugified = content.lower()
        slugified = self.invalid_chars_re.sub(self.placeholder, slugified)
        slugified = self.leading_punctuation_re.sub('', slugified)
        slugified += f'-{hashed}'

        if not slugified:
            return self.fallback_slug
        return slugified

Result:

>>> VersionSlugField(populate_from='').slugify('example/')
'example--81377426'
>>> VersionSlugField(populate_from='').slugify('/example/')
'example--24251688'
>>> VersionSlugField(populate_from='').slugify('example-')
'example--14556457'
>>> VersionSlugField(populate_from='').slugify('abcd')
'abcd-19060746'

[stsewd]

Interesting, our tests didn't catch that

https://github.com/rtfd/readthedocs.org/blob/104c3a625978f31971a11766023516fc2de2aa8f/readthedocs/rtd_tests/tests/test_version_slug.py#L76-L90

[dojutsu-user]

@stsewd When i test the test cases manually, they give the wrong result (as described in the issue)

>>> VersionSlugField(populate_from='').slugify('1!0')
'1-0'
>>> VersionSlugField(populate_from='').slugify('1%0')
'1-0'
>>> VersionSlugField(populate_from='').slugify('1?0')
'1-0'

[stsewd]

We actually call to create_slug which does the uniquifying

https://github.com/rtfd/readthedocs.org/blob/104c3a625978f31971a11766023516fc2de2aa8f/readthedocs/builds/version_slug.py#L174-L181

[humitos]

VersionSlugField(populate_from='').slugify('abcd')
'abcd-19060746'

We can't generate slugs like this ones because all the URLs will become very ugly. Example, https://example.readthedocs.io/en/abcd-19060746/ (version) or https://abcd-19060746.readthedocs.io/en/latest/ (project).

[stsewd]

I can't replicate even with the same verbose name from sentry

        version = Version.objects.create(
            verbose_name='raven/fix-start',
            project=self.pip
        )
        self.assertEqual(version.slug, 'raven-fix-start')

        version = Version.objects.create(
            verbose_name='raven/fix-start',
            project=self.pip
        )
        self.assertEqual(version.slug, 'raven-fix-start_a')

[stsewd]

duplicate key value violates unique constraint "builds_version_project_id_xxx_uniq"
DETAIL:  Key (project_id, slug)=(xxx, raven-fix-start) already exists.

I don't think the problem is in the sluggify step

[stsewd]

Here is the real problem

https://github.com/rtfd/readthedocs.org/blob/c37b00995c1bbc5ee51d3552ef176546373bb912/readthedocs/builds/models.py#L118-L118

Investigating

But it doesn't make sense anyway, slug should be unique in each project :/

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stsewd]

We still need to investigate more here, bot. Replication is needed.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stsewd]

Still valid, maybe a race condition in some part of the code

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[dojutsu-user]

Still valid bot.

[humitos]

We actually call to create_slug which does the uniquifying

@stsewd If I understand correctly, we only call create_slug when bool(slug) == False. That's probably the issue: the uniquification does not happen when creating Version object and passing a slug.