Skip to content

Http api schema validation#8068

Merged
medikoo merged 7 commits into
serverless:masterfrom
fredericbarthelet:httpApi-schema-validation
Aug 19, 2020
Merged

Http api schema validation#8068
medikoo merged 7 commits into
serverless:masterfrom
fredericbarthelet:httpApi-schema-validation

Conversation

@fredericbarthelet

Copy link
Copy Markdown
Contributor

Closes: #8021

@codecov-commenter

codecov-commenter commented Aug 9, 2020

Copy link
Copy Markdown

Codecov Report

Merging #8068 into master will increase coverage by 0.05%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
.../classes/ConfigSchemaHandler/normalizeAjvErrors.js 95.79% <0.00%> (-1.64%) ⬇️
lib/plugins/aws/provider/awsProvider.js 92.77% <ø> (ø)
...lugins/aws/package/compile/events/httpApi/index.js 92.57% <66.66%> (+2.06%) ⬆️
lib/classes/ConfigSchemaHandler/index.js 96.55% <100.00%> (+3.52%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20d9c64...8f6cc2c. Read the comment docs.

@medikoo medikoo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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' },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 ?

@medikoo medikoo Aug 18, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

switch (error.keyword) {
case 'additionalProperties':
if (error.dataPath === '.functions') {
error.message = `name '${error.params.additionalProperty}' must be alphanumeric`;
} else {
error.message = `unrecognized property '${error.params.additionalProperty}'`;
}
break;
case 'anyOf':
if (isEventTypeDataPath(error.dataPath)) {
error.message = 'unsupported function event';
break;
}
// fallthrough
case 'oneOf':
error.message = 'unsupported configuration format';
break;
case 'enum':
if (error.params.allowedValues.every(value => typeof value === 'string')) {
error.message += ` [${error.params.allowedValues.join(', ')}]`;
}
break;
default:
}

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 :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Comment thread lib/plugins/aws/package/compile/events/httpApi/index.js Outdated
Comment thread lib/plugins/aws/package/compile/events/httpApi/index.js Outdated
Comment thread lib/plugins/aws/package/compile/events/httpApi/index.js
Comment thread lib/plugins/aws/package/compile/events/httpApi/index.js
Comment thread lib/plugins/aws/package/compile/events/httpApi/index.js Outdated
Comment thread lib/plugins/aws/package/compile/events/httpApi/index.js Outdated
Comment thread lib/plugins/aws/provider/awsProvider.js
Comment thread lib/plugins/aws/provider/awsProvider.js Outdated
@fredericbarthelet
fredericbarthelet force-pushed the httpApi-schema-validation branch from dfc8794 to ead97dc Compare August 17, 2020 17:15
@fredericbarthelet

Copy link
Copy Markdown
Contributor Author

@medikoo, thanks for your review. I updated my PR using your feedbacks.

@medikoo medikoo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fredericbarthelet looks great! I've proposed just one last minor suggestion, and we should be good to go

Comment thread lib/plugins/aws/package/compile/events/httpApi/index.js Outdated
@fredericbarthelet

Copy link
Copy Markdown
Contributor Author

@medikoo, PR updated with custom error message and regexp pattern for path property :)

@medikoo medikoo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @fredericbarthelet that looks great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Config schema: Define AWS "httpApi" function event properties

3 participants