Enforce custom type keys are lowercase #47
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: Lizzy Ballantine <lizzy.kayballantine@gmail.com>
We are adding another one soon Co-authored-by: Lizzy Ballantine <lizzy.kayballantine@gmail.com>
sldblog
requested review from
a team,
ianmooney and
eballantine
and removed request for
a team
sldblog
force-pushed
the
add-validation-against-key-casing
branch
from
dbd055a
to
100457d
Compare
geoffreymm
approved these changes
Mar 22, 2023
There was a problem hiding this comment.
Nice one! My only feedback is around ensure we highlight (on the commit message and not only on the PR description) that this behaviour is just enforced for custom type definition and not across all blocks, menus and so on that do have a schema definition too. Hope this makes sense.
| @@ -38,6 +38,7 @@ def validate | |||
| ensure_has_required_keys && | |||
There was a problem hiding this comment.
Could we add to the commit message that this behaviour is for the schema definition of the custom types and not "all possible schemas"?
We store custom types in the database with a requirement to have keys unique within a theme. This validation is case sensitive; currently, it's possible to create multiple version of the same type with names like `type` and `Type`. Changing this validation to case insensitive in the persistence layer could lead into themes failing to load custom types in the future. (Now it's guaranteed to be unique as the first time load from the theme creates the first custom types.) To ensure integrity across all layers, we want to enforce lowercase keys at the point of custom type validation. Co-authored-by: Lizzy Ballantine <lizzy.kayballantine@gmail.com>
To guarantee interoperatibility with current attribute and block schemas, ensure that custom type lookup for validation is case insensitive.
Due to incompatible key validation changes Co-authored-by: Lizzy Ballantine <lizzy.kayballantine@gmail.com>
sldblog
force-pushed
the
add-validation-against-key-casing
branch
from
100457d
to
54982c4
Compare
sldblog
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A creator was unable to delete an image which was part of a custom type with a name like
meetTheCard. The cause was a type mismatch caused by downcasing. We opted to enforce consistent casing.To ensure integrity across all layers, we want to enforce lowercase keys at the point of custom type validation.
Decisions
Should we changeblock_schema_spec?Should we changemenu_schema_spec?Should we changeschema_attribute_spec?Answer to the above: No: we already treat built-in types as case insensitive, which would be too impactful to change. Keeping the type name validation scoped to custom types looks safe.
Consequences
We will manually fix custom types with uppercase letters (there are 4 of them).
All three current themes pass this new rule ✅
This will hook into the validation of custom types on the UI:
Also when linting a theme:
Co-authored-by: @eballantine