Http api schema validation#8068
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8068 +/- ##
==========================================
+ Coverage 88.37% 88.42% +0.05%
==========================================
Files 248 248
Lines 9446 9445 -1
==========================================
+ Hits 8348 8352 +4
+ Misses 1098 1093 -5
Continue to review full report at Codecov.
|
medikoo
left a comment
There was a problem hiding this comment.
@fredericbarthelet that's outstanding, thank you! It's great to see missing schemas being filled in.
Please see my remarks, I've proposed some additional fine tuning to what you've proposed, so it's as bulletproof as it can be (I hope)
| this.serverless.configSchemaHandler.defineFunctionEvent('aws', 'httpApi', { | ||
| anyOf: [{ type: 'string' }, { type: 'object' }], | ||
| anyOf: [ | ||
| { type: 'string' }, |
There was a problem hiding this comment.
It'll be nice to make it more restrictive with regexp pattern.
I guess we can improve regexp at methodPathPattern so it's:
const methodPathPattern = new RegExp(
`^(\\*|(?:${Array.from(allowedMethods).join('|')}))( .+)?$`,
'i'
);And define property with regexp setting
Also having that, it'll be nice to simplify validation logic in resolveConfiguration, as over there we may blindly assume that format is respected. So we should cover only cases which are not handled by schema
There was a problem hiding this comment.
@medikoo, noted, will update accordingly :)
One last thing on that matter : some error messages lose in clarity
Suppose you define for a function hello the httpApi attribute equals to the string myPath, the following error messages are displayed :
- before schema validation :
Invalid "<method> <path>" route in function hello for httpApi event in serverless.yml - after schema validation :
Configuration error at 'functions.hello.events[0].httpApi': should pass "regexp" keyword validation
One way to compensate would be to define custom JSON Schema error messages based on validation keywords. WDYT ?
There was a problem hiding this comment.
@fredericbarthelet that's a very good point.
In light of that I think we should improve messaging of regex keyword errors. It easily can be done here:
serverless/lib/classes/ConfigSchemaHandler/normalizeAjvErrors.js
Lines 210 to 233 in 20d9c64
Let's replace the message, with e.g. doesn't match expected format (if you feel you can come up with something better, please propose :)
There was a problem hiding this comment.
@medikoo, I added a custom message for regexp keyword : value '${error.data}' does not satisfy pattern ${error.schema}. The resulting error message in the scenario described above, now much clearer, is :
Configuration error at 'functions.hello.events[0].httpApi': value "myPath" does not satisfy pattern /^(?:*|(GET|POST|PUT|PATCH|OPTIONS|HEAD|DELETE) (/\S*))$/i
WDYT ?
There was a problem hiding this comment.
Looks good. I wonder whether listing pattern, won't be too noisy, but it's definitely very informative, so I'd say it's great!
dfc8794 to
ead97dc
Compare
|
@medikoo, thanks for your review. I updated my PR using your feedbacks. |
medikoo
left a comment
There was a problem hiding this comment.
@fredericbarthelet looks great! I've proposed just one last minor suggestion, and we should be good to go
|
@medikoo, PR updated with custom error message and regexp pattern for |
medikoo
left a comment
There was a problem hiding this comment.
Thank you @fredericbarthelet that looks great!
Closes: #8021