Websocket event schema

[rzaldana]

Closes: #8019

Hi this is my proposal for the websocket event schema. This is my first PR so sorry if I missed something basic!

Also, I'm not sure how to do testing for the schema. I did my own testing but I'm not sure how to do it within SLS. Maybe someone could provide some guidance (e.g. which files I should be working with to do the testing)? Thank you!

[codecov-commenter]

Codecov Report

Merging #8218 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #8218      +/-   ##
==========================================
- Coverage   88.09%   88.08%   -0.02%     
==========================================
  Files         248      248              
  Lines        9377     9369       -8     
==========================================
- Hits         8261     8253       -8     
  Misses       1116     1116              

Impacted Files	Coverage Δ
lib/configSchema.js	100.00% <ø> (ø)
...ins/aws/package/compile/events/websockets/index.js	100.00% <ø> (ø)
.../package/compile/events/websockets/lib/validate.js	97.22% <ø> (-0.51%)	⬇️
lib/plugins/aws/provider/awsProvider.js	93.11% <ø> (ø)

---

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 29d5fe7...c149d29. Read the comment docs.

[medikoo]

Great thanks @zaldanaraul ! That looks very promising.

Please see my remarks, there's nothing significant, I think it's close to what we're after.

Also, I'm not sure how to do testing for the schema.

We've agreed to not write tests that confirm schema errors (simply to avoid maintanance cost, and validation schema engine is tested extensively on its own).

By tests we only confirm that valid configuration works, and all tests that are configured through runServerless (see https://github.com/serverless/serverless/tree/master/test#unit-tests) are configured to crash whenever configuration error is approached in tested case.

Ideally such tests should also cover all supported configuration cases.

Unfortunately websockets, aside of integration tests, are not tested well via service configurations

Ideally would be to have its unit tests refactored, so they're solely based on runServerless, but that's definitely not in scope of this PR (anyway PR that does that is defintely welcome)

Otherwise you may just test it manually, by configuring simple service, and testing configuration via sls print

[medikoo]

@zaldanaraul great thanks for update! It looks very promising. Please see my remarks.

Also it'll be good to remove inline validation we have in event implementation (see e.g. errors removed here: https://github.com/serverless/serverless/pull/8230/files )

[rzaldana]

@medikoo

Also it'll be good to remove inline validation we have in event implementation (see e.g. errors removed here: https://github.com/serverless/serverless/pull/8230/files )

For websockets, validation seems to be implemented through the validate function defined here:

https://github.com/serverless/serverless/blob/master/lib/plugins/aws/package/compile/events/websockets/lib/validate.js

Although it seems to check for things that the schema also checks for, the errors that it throws do seem clearer than the ones from the schema at this point. Also, because this function is simply included but not called in the AWSCompileWebsockets, I'm not really sure when/how it's called so I think simply removing it would break the code:

serverless/lib/plugins/aws/package/compile/events/websockets/index.js

Line 5 in 6f392b3

const validate = require('./lib/validate');

serverless/lib/plugins/aws/package/compile/events/websockets/index.js

Lines 15 to 23 in 6f392b3

	class AwsCompileWebsockets {
	constructor(serverless, options) {
	this.serverless = serverless;
	this.options = options;
	this.provider = this.serverless.getProvider('aws');
	Object.assign(
	this,
	validate,

Please advise on how to proceed here :(

[medikoo]

Although it seems to check for things that the schema also checks for, the errors that it throws do seem clearer than the ones from the schema at this point.

Which errors exactly? In general we follow the rule to not duplicate schema validation with inline validation that validates very same things. Also schema errors are configured to be well descriptive

I think simply removing it would break the code

I don't think that we should remove that functions. In most cases validate also normalizes input, and that should stay untouched. See e.g. in this PR, how inline validation was striped out: https://github.com/serverless/serverless/pull/8201/files

[medikoo]

Thank you @zaldanaraul that looks really good! All that we miss, is what we've discussed in other comments (so restricting routeResponseSelectionExpression to $default, and stripping inline validation which covers what would be covered now by config schema validation)

[rzaldana]

Which errors exactly? In general we follow the rule to not duplicate schema validation with inline validation that validates very same things. Also schema errors are configured to be well descriptive

I'll stick to this rule then. In that case I removed three inline validations from the validate.js file (all of which the schema already validates):

• error if event doesn't have a route property
• error if both http and websocket properties are present in event
• error if authorizer has neither name nor arn property

Also changed the routeResponseSelectionExpression validation to use const. Please take a look!

[medikoo]

That looks great @zaldanaraul ! Thank you