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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 Fixed slugs from exceeding db limit #9251

Merged

Conversation

aileen
Copy link
Member

@aileen aileen commented Nov 16, 2017

closes #8143

Fixed a potential issue (edge-case), where our generated and validated (in terms of check for existance and add a counter) would return a slug, that will exceed the maximum length of the slug fields (191 chars).

This is mostly possible for the post title, which can be 255 chars long and would generate a slug with the same length. This would prevent the user from actually saving a post.

I tried first to determine the expected length for a slug that already exists, but decided that the easier and simplyfied implementation is to always cut a slug to 185 chars (+ counter). This makes it easier to find duplicates and includes a possible high number of counts (edge-edge-case).

@aileen
Copy link
Member Author

aileen commented Nov 16, 2017

@kirrg001 I would like to hear your thought about that. I am not entirely sure, if I might've forgotten a case...

I tried for quite a while to implement it in a different way, but take this situation:

  • slug gets generates with 191 chars
  • another slug is about to get generated, but is actually a duplicate, so I remove the - and the counter that is about to get added and trim the slug by the same amount. Let's say, it's now 189 chars long.
  • our fn checks again, if this slug exists and comes to the conclusion that it doesn't so the returned slug would then the the 189 chars long one (without - and count).
  • only for the third time then, we would add a counter to it.

We could definitely implement this logic, but IMO this is a but too much for such edge cases and I therefore vote for my solution.

@aileen aileen mentioned this pull request Nov 17, 2017
@aileen aileen force-pushed the no-slug-should-be-longer-than-soft-limit branch from bbac11a to 22b3b50 Compare November 17, 2017 08:14
// the slug may never be longer than the allowed limit of 191 chars, but should also
// take the counter into count. We reduce a too long slug to 185 so we're always on the
// safe side, also in terms of checking for existing slugs already.
slug = utils.safeString(base, options).slice(0, 185);

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@aileen aileen force-pushed the no-slug-should-be-longer-than-soft-limit branch from 22b3b50 to f38d68e Compare November 21, 2017 12:37
closes TryGhost#8143

Fixed a potential issue (edge-case), where our generated and validated (in terms of check for existance and add a counter) would return a slug, that will exceed the maximum length of the slug fields (191 chars).

This is mostly possible for the post title, which can be 255 chars long and would generate a slug with the same length. This would prevent the user from actually saving a post.

I tried first to determine the expected length for a slug that already exists, but decided that the **easier** and simplyfied implementation is to always cut a slug to **185 chars** (+ counter). This makes it easier to find duplicates and includes a possible high number of counts (edge-edge-case).

The slug will not be cut down to 185 chars if it's an import.
@aileen aileen force-pushed the no-slug-should-be-longer-than-soft-limit branch from f38d68e to 8a07be3 Compare November 21, 2017 12:38
Copy link
Contributor

@kirrg001 kirrg001 left a comment

Choose a reason for hiding this comment

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

This is a good fix. I can't of a simpler/better solution at the moment. It protects a validation error when the input source (post title, user name) is greater than 191. It does not auto-fix slugs to import. The user has to fix this alone, because a slug can represent an existing url. This is an edge case anyway, because e.g. post titles are usually not so long, same for user names.

@kirrg001 kirrg001 merged commit 982a75d into TryGhost:master Nov 21, 2017
@aileen aileen deleted the no-slug-should-be-longer-than-soft-limit branch November 21, 2017 13:22
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.

Database soft limits
2 participants