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

Improve settings validation and type handling #10582

Closed
5 tasks done
naz opened this issue Mar 7, 2019 · 4 comments
Closed
5 tasks done

Improve settings validation and type handling #10582

naz opened this issue Mar 7, 2019 · 4 comments
Assignees
Labels
affects:api Affects the Ghost API server / core Issues relating to the server or core of Ghost

Comments

@naz
Copy link
Contributor

naz commented Mar 7, 2019

The Problem

It is unclear what values can be set for each key in the settings object. This caused issues like #10576 and #10580

Current state

At the moment we don't have clear schema definition for all the values that can be stored in settings. There is a validation present on model layer, but it doesn't contain validations for all fields. We are not consistent with how boolean values are stored in the database.

Future improvements

We need to review uses of following data types/values that might cause the most of problems:

  • booleans - we currently store "true", "false", "0" and "1" in the DB, but should be storing only "true"/"false"
  • null/empty/undefined,
  • dates/timestamps

List of issues should be tackled for booleas:

  • migration which corrects "0" to "false" and "1" to "true"
  • a proper db input normalizer on model layer. this would handle e.g. stringification of booleans true/false into "true"/"false" to be stored in the db
  • API validator which ensures sending "false" or "0" to settings is converted into boolean bot be used in further layers
  • adapt importer to not forward "0" to model layer
  • remove force_i18n in api input serializer
@naz naz added data server / core Issues relating to the server or core of Ghost affects:api Affects the Ghost API labels Mar 7, 2019
@kirrg001
Copy link
Contributor

kirrg001 commented Mar 7, 2019

One addition: Exports might have "0" or "1". We have to ensure that these imports won't fail again 👍

@kirrg001
Copy link
Contributor

kirrg001 commented Mar 7, 2019

Another addition: force_i18n is deprecated. We won't serve it. We should not skip this value on API layer.

naz added a commit to naz/Ghost that referenced this issue Mar 7, 2019
refs TryGhost#10582

- Importer should do similar conversion introduced in 04c60b4
naz added a commit that referenced this issue Mar 7, 2019
refs #10582

- Importer should do similar conversion introduced in 04c60b4
@naz
Copy link
Contributor Author

naz commented Mar 8, 2019

We don't have any specific handling for null/undefined values in keys atm. When the value is not present for a key, this check in Settings model prevents it from being set to undefined:

if (item.hasOwnProperty('value')) {
    setting.set('value', item.value);
}

Ideally objects without value provided woulld be filtered out in API validation layer.

As for null value, it is processed as expecte and stored as NULL in the db. There is a minor inconsistency where we sometimest store empty string "" to identify emtpy value but also allow it to be set to null (for example value for icon key).

Regarding date/timestamp fields, the only values we store are for next_update_check which comes from the request made to https://updates.ghost.org. We don't have any validation around this value but seems improbable that anything will break. Unless we would want to be extra cautious, don't think we need to do anything about it.

@kirrg001
Copy link
Contributor

As for null value, it is processed as expecte and stored as NULL in the db. There is a minor inconsistency where we sometimest store empty string "" to identify emtpy value but also allow it to be set to null (for example value for icon key).

This needs some more context. Worth raising a separate issue with your findings.

kirrg001 added a commit to kirrg001/Ghost that referenced this issue Mar 11, 2019
kirrg001 added a commit to kirrg001/Ghost that referenced this issue Mar 11, 2019
…oleans

refs TryGhost#10582

- otherwise we will forward string booleans to model layer
- causes trouble if we trigger events
- causes trouble if we want to add conditions to the model e.g. setting.get('value') ?
kirrg001 added a commit to kirrg001/Ghost that referenced this issue Mar 11, 2019
refs TryGhost#10582

- ensure we won't forward booleans to database
- type TEXT will transform booleans to "0"/"1!
kirrg001 added a commit to kirrg001/Ghost that referenced this issue Mar 11, 2019
refs TryGhost#10582

- deprecated
- won't serve
- won't save
kirrg001 added a commit to kirrg001/Ghost that referenced this issue Mar 11, 2019
refs TryGhost#10582

- the db input formatter ensures we always forward "true" or "false" for boolean fields
kirrg001 added a commit to kirrg001/Ghost that referenced this issue Mar 11, 2019
kirrg001 added a commit to kirrg001/Ghost that referenced this issue Mar 11, 2019
refs TryGhost#10582

- I don't think this is a good idea
- If a user passses "null", we should treat it as a string
- I am not aware of a use case why people have "null" in their database
- If people send "null" via the API, we should respect this and accept a string
kirrg001 added a commit that referenced this issue Mar 11, 2019
kirrg001 added a commit that referenced this issue Mar 11, 2019
…oleans

refs #10582

- otherwise we will forward string booleans to model layer
- causes trouble if we trigger events
- causes trouble if we want to add conditions to the model e.g. setting.get('value') ?
kirrg001 added a commit that referenced this issue Mar 11, 2019
refs #10582

- ensure we won't forward booleans to database
- type TEXT will transform booleans to "0"/"1!
kirrg001 added a commit that referenced this issue Mar 11, 2019
refs #10582

- deprecated
- won't serve
- won't save
kirrg001 added a commit that referenced this issue Mar 11, 2019
refs #10582

- the db input formatter ensures we always forward "true" or "false" for boolean fields
kirrg001 added a commit that referenced this issue Mar 11, 2019
kirrg001 added a commit that referenced this issue Mar 11, 2019
refs #10582

- I don't think this is a good idea
- If a user passses "null", we should treat it as a string
- I am not aware of a use case why people have "null" in their database
- If people send "null" via the API, we should respect this and accept a string
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects:api Affects the Ghost API server / core Issues relating to the server or core of Ghost
Projects
None yet
Development

No branches or pull requests

2 participants