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 schema definition validation to prevent collisions #8655
Improve schema definition validation to prevent collisions #8655
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8655 +/- ##
==========================================
- Coverage 87.15% 87.15% -0.01%
==========================================
Files 252 252
Lines 9509 9544 +35
==========================================
+ Hits 8288 8318 +30
- Misses 1221 1226 +5
Continue to review full report at Codecov.
|
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.
Thanks @fredericbarthelet 🙇 Looks good from my perspective, but as I'm not super familiar with schema engine yet, I'll ask for another review before merging this in
@@ -290,6 +309,21 @@ class ConfigSchemaHandler { | |||
} | |||
|
|||
function addPropertiesToSchema(subSchema, extension = { properties: {}, required: [] }) { | |||
let collidingExtensionPropertyKey; | |||
const existingSubSchemaPropertiesKeys = Object.keys(subSchema.properties); | |||
Object.keys(extension.properties).some(extensionPropertiesKey => { |
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.
At first I was thinking why not to throw an error inside the callback, but I've noticed you used some
to stop iteration on first colliding property, nice 👍
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.
@fredericbarthelet looks great! I have just one DX suggestion towards error reporting, as as proposed may result with vague error reports.
2dabcdb
to
a88767b
Compare
@medikoo I implemented your suggestions and added test cases for all different location where an error could be thrown :) |
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.
Thank you @fredericbarthelet ! Looks great
Closes: #8481