Websocket event schema by rzaldana · Pull Request #8218 · serverless/serverless

rzaldana · 2020-09-10T14:42:15Z

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!

Updating My Fork

pulling changes from master

merging changes from master branch into my forked repo

merging recent commits from master into my fork

codecov-commenter · 2020-09-10T14:44:13Z

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 · 2020-09-16T18:17:14Z

@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 · 2020-09-18T07:34:53Z

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)

personal fork update

rzaldana · 2020-09-18T14:58:33Z

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

rzaldana and others added 8 commits September 3, 2020 13:26

Merge pull request #1 from serverless/master

d0864d9

Updating My Fork

Merge pull request #2 from serverless/master

ce8fc14

pulling changes from master

Started working on websockets schema

6001d4a

Merge branch 'master' of https://github.com/serverless/serverless

6eb858d

merging changes from master branch into my forked repo

finished out schemas

7b7517a

fixed some errors with Websocket schema

6f433be

finished up provider attributes

4b82ab8

Merge branch 'master' of https://github.com/serverless/serverless

58870fd

merging recent commits from master into my fork

medikoo suggested changes Sep 10, 2020

View reviewed changes

Comment thread lib/plugins/aws/package/compile/events/websockets/index.js Outdated

Comment thread lib/plugins/aws/package/compile/events/websockets/index.js Outdated

Comment thread lib/plugins/aws/provider/awsProvider.js Outdated

Comment thread lib/plugins/aws/provider/awsProvider.js

medikoo added cat/config-schema enhancement cat/aws-event-websocket labels Sep 10, 2020

medikoo assigned rzaldana Sep 10, 2020

rzaldana added 2 commits September 14, 2020 11:02

made requested changes to websockets schema

99a67fb

removing routePattern regexp

b4be0bb

rzaldana requested a review from medikoo September 15, 2020 14:36

medikoo suggested changes Sep 16, 2020

View reviewed changes

requested changes

9f5757e

rzaldana requested a review from medikoo September 16, 2020 18:21

medikoo suggested changes Sep 18, 2020

View reviewed changes

rzaldana and others added 3 commits September 18, 2020 09:44

removed inline validation for websockets

584e96e

removed additional inline validation from websocket impl

4fdb552

Merge pull request #3 from serverless/master

c149d29

personal fork update

rzaldana requested a review from medikoo September 18, 2020 14:59

medikoo approved these changes Sep 18, 2020

View reviewed changes

medikoo merged commit e1ca63c into serverless:master Sep 18, 2020

Uh oh!

Conversation

rzaldana commented Sep 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Sep 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rzaldana commented Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

medikoo commented Sep 18, 2020

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

rzaldana commented Sep 18, 2020

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rzaldana commented Sep 10, 2020 •

edited

Loading

codecov-commenter commented Sep 10, 2020 •

edited

Loading

rzaldana commented Sep 16, 2020 •

edited

Loading