-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
tighten validation to disallow expressions as stop values #7396
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works as I would expect and the resulting code paths make sense to me, however, I would say that the error message isn't very helpful. One alternative could be the following instead of mutating the value spec:
if (isExpression(deepUnbundle(value[1]))) {
return errors.concat([new ValidationError(`${key}[1]`, value[1], 'expressions are not allowed in function stops.' ]);
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the style-spec documentation, your change looks well targeted to me.
stop output values must be literal values (i.e. not functions or expressions), and appropriate for the property.
I did find that, similar to the example in #7387 , the following stop values will pass validation. When the expression array matches the stop value spec type entirely, it leaves a hole in the validation logic. Do you think it is necessary to check if the value is an expression and reject it where arrays are expected and expressions are not supported? It is an extreme corner-case.
"text-font": {
"base": 1,
"stops": [
[
6,
[
"upcase",
"Open Sans Bold"
]
],
[
7,
[
"downcase",
"Open Sans Bold"
]
]
]
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirming that this change produces the expected result in the Studio UI when I attempt to configure an expression within a stop value:
I can also confirm that the expression @asheemmamoowala articulated passes validation, but still triggers an invalid GL JSON error modal in the Studio UI:
Ideally, our validation would catch this before publishing to the stylesheet. If this level of validation is too much of an edge case to incorporate into the function validator, we can also add custom validation on top of the style spec's in Studio.
@emilymdubois thanks for checking! @mollymerp that fix looks cleaner and clearer. Switching to it. It should also handle the edge case that @asheemmamoowala is thinking about. Thanks! |
This fixes #7387 by tightening validation to not allow expressions within the old-style functions. It does this by passing a more limited valueSpec which disallows expressions while validation stop values.
@emilymdubois @asheemmamoowala @mollymerp could I get a pretty careful review of this? This is my first time working on style validation. I'm not sure if this could have unintended consequences. I know we have had issues with tightened validation in the past.
Launch Checklist