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

Database soft limits #8143

Closed
kirrg001 opened this issue Mar 13, 2017 · 16 comments · Fixed by TryGhost/Admin#683 or #9251
Closed

Database soft limits #8143

kirrg001 opened this issue Mar 13, 2017 · 16 comments · Fixed by TryGhost/Admin#683 or #9251
Assignees
Labels
affects:admin Anything relating to Ghost Admin server / core Issues relating to the server or core of Ghost

Comments

@kirrg001
Copy link
Contributor

We have recently optimised our hard limits, see #7932.
One optimisation which is missing are the soft limits. A soft limit is a limit which is defined within the application as validation condition - client and server side.

See original issue and discussion #4134.
e.g. it was suggested to increase the post title validation from 150 to 255.

We have to go over all soft limits and decide on a reasonable limit.

@kirrg001 kirrg001 added affects:admin Anything relating to Ghost Admin data server / core Issues relating to the server or core of Ghost labels Mar 13, 2017
@kirrg001 kirrg001 added this to the 1.0.0 Beta Ready milestone Apr 3, 2017
@kirrg001
Copy link
Contributor Author

kirrg001 commented Apr 3, 2017

Assigned to 1.0.0 Beta Ready. The plan here is to get an overview of which soft limits we would like to define. And then implement them before launching 1.0.

@kirrg001 kirrg001 modified the milestones: 1.0.0 Launch, 1.0.0 Beta Ready Apr 4, 2017
@kirrg001
Copy link
Contributor Author

kirrg001 commented Apr 4, 2017

We would like to change the following database soft limits (== input validation).

  • increase post title to 255 (current is 150)
  • increase meta title to 300 (current is 150)
  • increase meta description to 500 (current is 200)

I will move this issue into the 1.0 Launch milestone.

aileen added a commit to aileen/Admin that referenced this issue May 2, 2017
closes TryGhost/Ghost#8143

- uses new soft limits in validation:
	- post title to 255
	- meta title to 300
	- meta description to 500
- updates test
@kirrg001 kirrg001 removed this from the 1.0.0 Backlog milestone May 12, 2017
@kirrg001 kirrg001 assigned aileen and unassigned kirrg001 May 12, 2017
aileen added a commit to aileen/Admin that referenced this issue May 15, 2017
closes TryGhost/Ghost#8143

- uses new soft limits in validation:
	- post title to 255
	- meta title to 300
	- meta description to 500
- updates test
aileen added a commit to aileen/Admin that referenced this issue May 15, 2017
closes TryGhost/Ghost#8143

- uses new soft limits in validation:
	- post title to 255
	- meta title (post and tag) to 300
	- meta description (post and tag) to 500
- updates test
kirrg001 pushed a commit to TryGhost/Admin that referenced this issue May 15, 2017
closes TryGhost/Ghost#8143

- uses new soft limits in validation:
	- post title to 255
	- meta title (post and tag) to 300
	- meta description (post and tag) to 500
@kirrg001
Copy link
Contributor Author

kirrg001 commented Nov 6, 2017

This was not added on the server side. We should figure out first how complex it is to add the soft limits on the server side now. e.g. other external clients might not have the same soft limits we have added on the Ghost admin (mobile apps) or imports from LTS to 1.0.

@kirrg001 kirrg001 reopened this Nov 6, 2017
@kirrg001
Copy link
Contributor Author

kirrg001 commented Nov 7, 2017

We had a call today, @AileenCGN will post the result from it tomorrow morning.

@aileen
Copy link
Member

aileen commented Nov 8, 2017

In the call yesterday, @kirrg001 and I came to following conclusion:

We have two options:

  1. We set the soft limits anyway, but with common sense. Means, we will try to decrease the soft limits for as little fields as possible to minimize any negative effects for the users 1.
  2. We run over every db and check if entries exceed the - planned to set - soft limits and do something with is (e. g. cut them off manually)

We decided to go with option 1, because there only very few fields that are affected, and the chances, that someone would have an entry in the db, which exceeds these soft limits, are pretty low, especially for LTS imports, some limits were lower and Ghost only worked with Ghost Admin client.

Soft limits for server side

These are the affected db tables and fields, where we're going to set a soft limit, using the isLength() method from validator.js (this is done already for post.custom_excerpt. See here):

  • posts:
    • title: 255 chars (current hard limit: 2,000 chars)
    • meta_title: 300 chars (current hard limit: 2,000 chars)
    • meta_description: 500 chars (current hard limit: 2,000 chars)
  • users:
    • bio: 200 chars (current hard limit: 65,535 chars)
    • location: 150 chars (current hard limit: 65,535 chars)
    • meta_description: 500 chars (current hard limit: 2,000 chars) -> special case, as it's not currently used in our client
    • meta_title: 300 chars (current hard limit: 2,000 chars) -> special case, as it's not currently used in our client
  • tags:
    • description: 500 chars (current hard limit: 65,535 chars)
    • meta_title: 300 chars (current hard limit: 2,000 chars)
    • meta_description: 500 chars (current hard limit: 2,000 chars)

It's going to be more tricky to set a soft limit for the value field in the settings table for a specific key, so we will have to do it manually (see additional tasks):
- title: 150 chars
- description: 200 chars

Add/increase soft limits on client side

  • soft limits we're going to increase on the client side (reason: increasing on the client side rather than setting a soft limit on the server side, so we have less fields that could possible have a negative impact for the user):

    • tags.name: 191 chars
    • tags.slug: 191 chars
    • users.name: 191 chars
  • soft limits missing on client side (see additional tasks):

    • posts.slug: 191 chars
    • users.slug: 191 chars
    • users.email: 191 chars
    • subscribers.name: 191 chars -> not used in client
    • subscribers.email: 191 chars
    • settings.blog.title in setup flow: 150 chars

Tasks

  • Add soft limits to server side as listed above
  • Increase soft limits client side as listed above
  • Verify if db inputs, that exceed the hard limit get cut of by the db. If they get cut off, we will add validations for every input field, which then validates for the same max length as the set hard limit.
  • Inform @cobbspur that new soft limits exist, which could fail a migration

Additional tasks

  • Find a way to validate settings (blog title and description)
  • Improve validation error message
  • Add missing soft limits in admin client
  • Prevent generateSlug from generating slugs > 191 chars (see comment here)

1 I tested the following scenario with users.name:

  1. Deactivate all client side validations, so it's possible to reach the db hard limits.
  2. Enter a user name, that is 191 characters long and save.
  3. Set soft limit in schema.js for user name to 150 chars.
  4. See, that everything still loads fine, only when changes to the name are made, the saving fails because of the new validation rule.

@aileen
Copy link
Member

aileen commented Nov 8, 2017

@kirrg001 Can you please let me know, if I got everything right? If not, just change it in my comment

@aileen
Copy link
Member

aileen commented Nov 8, 2017

I tested with MySQL and sqlite, if values get cut of by the db if they exceed the hard limit and the result is that they don't. This means, there's no need to use set soft limits for the remaining db fields.

@aileen
Copy link
Member

aileen commented Nov 8, 2017

@kirrg001 I also stumbled over another problem:

The generateSlug method in our model layer here doesn't have a validation. In our case, for posts.title, users.name, and tags.name, it is then possible to generate a slug, which is longer then the allowed 191 characters, because the allowed title or name can either have the same length or (in the case of posts.title) be even longer.

I suggest, that we cut the rendered slug to a maximum of 191 chars, so we never return a slug that would fail the validation and therefore prevent the saving process. I added this to the task-list in my comment above.

aileen added a commit to aileen/Admin that referenced this issue Nov 8, 2017
refs TryGhost/Ghost#8143

Increases existing input validation lenght (soft limits) of the following fields:
   - `tags.name`: 191 chars
   - `tags.slug`: 191 chars
   - `users.name`: 191 chars
@aileen
Copy link
Member

aileen commented Nov 8, 2017

Improve validation error message

This is actually a message, we produce here.

Is it necessary to change the message, @kirrg001? And if so, what is your suggestion?

@kirrg001
Copy link
Contributor Author

kirrg001 commented Nov 8, 2017

chances, that someone would have an entry in the db, which exceeds these soft limits, are pretty low, especially for LTS imports, some limits were lower...

To extend that a bit. If you did an import without having the server-side soft limit validator in place for e.g. the meta fields, you might already hit the client validation as soon as you start editing an affected post.

...and Ghost only worked with Ghost Admin client.

It's always possible to use the Ghost API directly without any client, but we should not care about this case too much. Only common external clients are important e.g. the native android Ghost app.

I tested with MySQL and sqlite, if values get cut of by the db if they exceed the hard limit and the result is that they don't. This means, there's no need to use set soft limits for the remaining db fields.

Yeah we have a maxlength validator for the hard limits for each field which has a maxlength, see.

Is it necessary to change the message, @kirrg001? And if so, what is your suggestion?

I think it's necessary, but this is how the soft limit validation message looks like: Validation (isLength) failed for slug. It does not sound very user friendly. Furthermore, because we would like to go with option 1, we should care about showing an affected user a very clean error message. We could use the a similar description like we use for the hard limits, see.

The generateSlug method in our model layer here doesn't have a validation... I suggest, that we cut the rendered slug to a maximum of 191 chars, so we never return a slug that would fail the validation and therefore prevent the saving process. I added this to the task-list in my comment above.

If a generated slug is too long, it would hit the hard limit validation by default. But e.g. a post title can be up to 255 characters, whereas the post slug can be up to 191 only, this is definitely an additional task, because this is out of the control of the user - you won't be able to create a post.


If you are doing an LTS migration to 1.x and some of your fields hit a server-side soft limit, should we auto-fix or let the user fix it manually?

@aileen
Copy link
Member

aileen commented Nov 8, 2017

If you are doing an LTS migration to 1.x and some of your fields hit a server-side soft limit, should we auto-fix or let the user fix it manually?

This is a very good point. I am not sure, which way to go here. If we auto-fix those cases, the user loses data. But it's also very unlikely (IMO) that those limits are hit, but it's still possible. On the other hand, if we don't auto-fix them, the migration would fail and the user has to find the affected part and fix it himself. 🤔

I think it's necessary, but this is how the soft limit validation message looks like: Validation (isLength) failed for slug. It does not sound very user friendly.

Can certainly optimize the error message to make it more user friendly.

Furthermore, because we would like to go with option 1, we should care about showing an affected user a very clean error message.

So, we go for option 1 now?

@kirrg001
Copy link
Contributor Author

kirrg001 commented Nov 8, 2017

Can certainly optimize the error message to make it more user friendly.

👍

So, we go for option 1 now?

Will confirm this approach with @ErisDS - we have a call in an hour anyway. As well as the import auto-fix concern. Will come back here soon.

@kirrg001
Copy link
Contributor Author

kirrg001 commented Nov 8, 2017

Option 1 confirmed. No auto-fix for migrations.
Could you please add a sub task to notify @cobbspur of this change?

@aileen
Copy link
Member

aileen commented Nov 9, 2017

Option 1 confirmed

@kirrg001 I updated the bis comment above reflecting those changes.

aileen added a commit to aileen/Ghost that referenced this issue Nov 9, 2017
refs TryGhost#8143

Sets soft limits for certain db fields:
- `posts`:
	- `title`: 255 chars (current hard limit: 2,000 chars)
	- `meta_title`: 300 chars (current hard limit: 2,000 chars)
	- `meta_description`: 500 chars (current hard limit: 2,000 chars)
- `users`:
	- `bio`: 200 chars (current hard limit: 65,535 chars)
	- `location`: 150 chars (current hard limit: 65,535 chars)
	- `meta_description`: 500 chars (current hard limit: 2,000 chars)
	- `meta_title`: 300 chars (current hard limit: 2,000 chars)
- `tags`:
	- `description`: 500 chars (current hard limit: 65,535 chars)
	- `meta_title`: 300 chars (current hard limit: 2,000 chars)
	- `meta_description`: 500 chars (current hard limit: 2,000 chars)
@aileen
Copy link
Member

aileen commented Nov 9, 2017

NAME: ValidationError
MESSAGE: Value in [posts.meta_description] exceeds maximum length of 500 characters.

level:normal

posts.meta_description
ValidationError: Value in [posts.meta_description] exceeds maximum length of 500 characters.

@cobbspur This is an example error message, that Ghost will return.

Imports and migrations can then fail when they exceeded the soft limits of the above mentioned fields.

kirrg001 pushed a commit that referenced this issue Nov 9, 2017
refs #8143

Sets soft limits for certain db fields:

- `posts`:
	- `title`: 255 chars (current hard limit: 2,000 chars)
	- `meta_title`: 300 chars (current hard limit: 2,000 chars)
	- `meta_description`: 500 chars (current hard limit: 2,000 chars)
- `users`:
	- `bio`: 200 chars (current hard limit: 65,535 chars)
	- `location`: 150 chars (current hard limit: 65,535 chars)
	- `meta_description`: 500 chars (current hard limit: 2,000 chars)
	- `meta_title`: 300 chars (current hard limit: 2,000 chars)
- `tags`:
	- `description`: 500 chars (current hard limit: 65,535 chars)
	- `meta_title`: 300 chars (current hard limit: 2,000 chars)
	- `meta_description`: 500 chars (current hard limit: 2,000 chars)

- same error message for isLength validator as for hard limits (more improvements are comming with #6050)
- added more tests for importer
- added dynamic translation key handling for validators errors (isLength is only supported atm)
kevinansfield pushed a commit to TryGhost/Admin that referenced this issue Nov 9, 2017
refs TryGhost/Ghost#8143

Increases existing input validation length (soft limits) of the following fields:
   - `tags.name`: 191 chars
   - `tags.slug`: 191 chars
   - `users.name`: 191 chars
aileen added a commit to aileen/Ghost that referenced this issue Nov 16, 2017
refs TryGhost#8143

Add max length validations to settings:
- `blog.title`: 150 chars
- `blog.description`: 200 chars

The `validateSettings` fn in our validations checks for existing `validations` properties in our `default-settings.json` file, similar to other tables in our `schema.js`.
aileen added a commit to aileen/Admin that referenced this issue Nov 16, 2017
refs TryGhost/Ghost#8143

Added more client side validations for input fields:

- Blog title in setup flow (150 chars)
- User email (191 chars)
- Subscribers email (191 chars)
kevinansfield pushed a commit to TryGhost/Admin that referenced this issue Nov 16, 2017
…mail fields (#909)

refs TryGhost/Ghost#8143

Added more client side validations for input fields:

- Blog title in setup flow (150 chars)
- User email (191 chars)
- Subscribers email (191 chars)
kirrg001 pushed a commit that referenced this issue Nov 16, 2017
refs #8143

Add max length validations to settings:
- `blog.title`: 150 chars
- `blog.description`: 200 chars

The `validateSettings` fn in our validations checks for existing `validations` properties in our `default-settings.json` file, similar to other tables in our `schema.js`.
@aileen
Copy link
Member

aileen commented Nov 17, 2017

If #9251 gets merged as is, we won't need to add the two last missing client side validations for posts.slug and users.slug, as it won't be possible to enter or save a slug which is longer than 191 characters.

The generated slug will always be max 185 chars plus possible counter, and updating a slug goes through the same bit of logic, where we first cut the slug and then check for duplicates.

aileen added a commit to aileen/Ghost that referenced this issue Nov 21, 2017
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.
kirrg001 pushed a commit that referenced this issue Nov 21, 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).

The slug will not be cut down to 185 chars if it's an import.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects:admin Anything relating to Ghost Admin server / core Issues relating to the server or core of Ghost
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants