s3 event schema by thewizarodofoz · Pull Request #8330 · serverless/serverless

thewizarodofoz · 2020-10-02T08:41:22Z

Closes: #8031

codecov-commenter · 2020-10-02T08:43:31Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #8330      +/-   ##
==========================================
- Coverage   88.01%   88.01%   -0.01%     
==========================================
  Files         249      249              
  Lines        9307     9306       -1     
==========================================
- Hits         8192     8191       -1     
  Misses       1115     1115

Impacted Files	Coverage Δ
...ib/plugins/aws/package/compile/events/sns/index.js	`100.00% <ø> (ø)`
lib/plugins/aws/package/compile/functions/index.js	`96.64% <ø> (-0.02%)`	⬇️
lib/plugins/aws/provider/awsProvider.js	`93.04% <ø> (ø)`

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 a020a4a...fbed014. Read the comment docs.

medikoo

Thank you @thewizarodofoz ! It looks promising. Please see my comments.

Also (I forgot to initially note in dedicated issue, sorry for that). We need to also define s3 block at provider section. It supports name, and all this properties:

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

Lines 19 to 36 in fc34140

    
           'accelerateConfiguration', 
        
           'accessControl', 
        
           'analyticsConfigurations', 
        
           'bucketEncryption', 
        
           'bucketName', 
        
           'corsConfiguration', 
        
           'inventoryConfigurations', 
        
           'lifecycleConfiguration', 
        
           'loggingConfiguration', 
        
           'metricsConfigurations', 
        
           'notificationConfiguration', 
        
           'objectLockConfiguration', 
        
           'objectLockEnabled', 
        
           'publicAccessBlockConfiguration', 
        
           'replicationConfiguration', 
        
           'tags', 
        
           'versioningConfiguration', 
        
           'websiteConfiguration',

(having that defined in schema, we can remove that allowed properties validation)

thewizarodofoz · 2020-10-03T14:25:13Z

We need to also define s3 block at provider section

@medikoo do you mean you would like to see a full fledged schema of all the bucket properties? Or just a validation for the allowed properties?

medikoo · 2020-10-05T14:53:46Z

@medikoo do you mean you would like to see a full fledged schema of all the bucket properties?

I think it's similar case to CloudFront, where we take CF template properties one by one. So I would do same as there. So introduce proper schema for each property

codecov-io · 2020-10-11T13:30:09Z

Codecov Report

Merging #8330 into master will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #8330      +/-   ##
==========================================
- Coverage   87.39%   87.36%   -0.04%     
==========================================
  Files         254      255       +1     
  Lines        9476     9461      -15     
==========================================
- Hits         8282     8266      -16     
- Misses       1194     1195       +1

Impacted Files	Coverage Δ
lib/classes/ConfigSchemaHandler/index.js	`90.00% <ø> (+0.43%)`	⬆️
lib/plugins/aws/provider/awsProvider.js	`93.43% <ø> (ø)`
...gins/aws/package/compile/events/s3/configSchema.js	`100.00% <100.00%> (ø)`
lib/plugins/aws/package/compile/events/s3/index.js	`98.48% <100.00%> (-1.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 b6ae44e...673f776. Read the comment docs.

medikoo

@thewizarodofoz Thank you! That looks very promising. Still I believe we should maintain convention with which other schemas are configured. Please see my comment.

If convention can be improved, I would first discuss and eventually refactor with other PR already existing schemas into new convention before adapting to new one in this PR

medikoo · 2020-10-12T10:54:20Z

    serverless.setProvider('aws', new AwsProvider(serverless));
    awsCompileS3Events = new AwsCompileS3Events(serverless);
    awsCompileS3Events.serverless.service.service = 'new-service';
+    // TODO: remove awsS3BucketName.pattern stub when tests are refactored to use runServerless


We can remove this comment, as converting to runServerless means not writing this stub at all (a kind of test where AwsCompileS3Events instance is created individually)

medikoo · 2020-10-12T10:57:36Z

          },
          additionalProperties: false,
        },
+        s3: require('./lib/s3/Bucket'),


Moving this out, breaks the convention. All others properties are defined inline, and this one is not, why exactly?

If there's no specific reason, I would keep it inline with other schema.

It's also way easier for inspection, which can be common when users may want to investigate why they receive the schema errors

I will move the top bucket schema inline.
How do you feel about all other sub-schemas in seperate files and required by ./lib/s3/Bucket.js?

I think all of them should be inline (or are they reusable across different places?)

Eventually all could be in one external s3.js file, but with that I would also move few other big blocks into separate external files (eg. definitions, functions, layers, resources`) - and that will be better to first handle in separate PR

There is quite a lot of reuse of sub schemas. So having it all inline will require repetition.
I vote for moving all s3 related schemas into a single s3.js file which will be required by awsProvider.js and externalise other big blocks in a later PR.

There is quite a lot of reuse of sub schemas. So having it all inline will require repetition.

In such case we may reference them in local env vars (we already do that in some places)

I vote for moving all s3 related schemas into a single s3.js file which will be required by awsProvider.js and externalise other big blocks in a later PR.

I'm fine with that, still let's put it in lib/plugins/aws/package/compile/events/s3/configSchema.js (the only file, with reusable parts organized via local vars)

medikoo

Thanks @thewizarodofoz for update ! Still, I've just noticed that as configured, schema intended for provider.s3 is not really attached.

It'll be nice to confirm that it works testing it against some invalid s3 event configuration

Also can you double check that s3 integration tests pass (see instructions: https://github.com/serverless/serverless/tree/master/test#aws-integration-tests ), e.g. when adding schema for provider.apiGateway we've introduced some bugs that got exposed by those tests (I've already fixed them with #8399 and #8400)

Thank you!

medikoo · 2020-10-16T09:18:58Z

          },
          additionalProperties: false,
        },
+        s3: require('../package/compile/events/s3/configSchema'),


Here it's in effective, it needs to be in provider.properties group.

medikoo · 2020-10-16T10:32:55Z

One more thing: In so far schemas I've converted most of oneOf to anyOf, as it doesn't play well in some cases with implied coercion, also it's more optimal for checking, as with oneOf AJV will always validate against each option, while with anyOf it'll stop at first matched.

Therefore I think it'll be nice to also adapt to it in context of this PR

thewizarodofoz · 2020-10-16T14:44:56Z

Integration tests pass (ran them all once, and then s3 alone)

medikoo

Thank you @thewizarodofoz that's impressive!

I've spotted just few last issues. Also if I see correctly, it's the last missing schema (yay!).

Therefore I believe we also should remove this:

serverless/lib/classes/ConfigSchemaHandler/index.js

Lines 70 to 73 in bf356fd

    
           if (this.serverless.service.provider.name === 'aws') { 
        
             // TODO: Remove after provider is fully covered, see https://github.com/serverless/serverless/issues/8022 
        
             this.serverless.configSchemaHandler.relaxAwsProviderSchema(); 
        
           }

(together with relaxAwsProviderSchema method implementation)

medikoo · 2020-10-19T08:30:57Z

+      enum: ['DEEP_ARCHIVE', 'GLACIER', 'INTELLIGENT_TIERING', 'ONEZONE_IA', 'STANDARD_IA'],
+    },
+    TransitionInDays: {
+      type: 'integer',


I think in such cases where natural number is expected, it'll be good to add minimum: 0

ok.
the only place I left without min 0 is ReplicationRule.Priority because maybe one can de-prioritise a rule by setting a negative priority.

medikoo · 2020-10-19T08:34:04Z

+                type: 'array',
+                items: tagFilter,
+              },
+              transition,


Let's remove that one, as it's deprecated (also it's casing is not right)

medikoo · 2020-10-19T08:46:12Z

+                AllowedHeaders: {
+                  type: 'array',
+                  items: { type: 'string' },
+                },
+                AllowedMethods: {
+                  type: 'array',
+                  items: { enum: ['GET', 'PUT', 'HEAD', 'POST', 'DELETE'] },
+                },
+                AllowedOrigins: {
+                  type: 'array',
+                  items: { type: 'string' },
+                },
+                ExposedHeaders: {
+                  type: 'array',
+                  items: { type: 'string' },
+                },
+                Id: {
+                  type: 'string',
+                  maxLength: 255,
+                },
+                MaxAge: {
+                  type: 'integer',
+                },
+              },
+              required: ['AllowedMethods', 'AllowedOrigins'],


This one seems not matching https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-s3-bucket-defaultretention.html

thewizarodofoz · 2020-10-20T20:52:07Z

@medikoo after removing relaxAwsProviderSchema two tests are failing (one, two). They both use a field deploymentBucketObject which is not part of the schema.
They used to pass because relaxAwsProviderSchema was setting additionalProperties = true.
This field is not documented so I'm not sure what to do about it. Please advise.

medikoo · 2020-10-27T21:10:36Z

@medikoo after removing relaxAwsProviderSchema two tests are failing (one, two). They both use a field deploymentBucketObject which is not part of the schema.

@thewizarodofoz sorry for late response. It's errors in tests. I've fixed them in #8447, so now issue should be gone,

thewizarodofoz · 2020-10-28T11:32:00Z

Thanks @medikoo, unfortunately I don't think it solved it.
It seems awsProvider is populating deploymentBucketObject prior to the validation, so when we validate the schema we still encounter the issue.

medikoo · 2020-10-29T08:40:30Z

It seems awsProvider is populating deploymentBucketObject prior to the validation, so when we validate the schema we still encounter the issue.

@thewizarodofoz Indeed there was an error as we've taken modified provider input for validation. I've fixed that here: #8450

I believe all should be fine now

medikoo

Thank you @thewizarodofoz !

codecov-commenter · 2026-04-03T05:30:05Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.36%. Comparing base (b6ae44e) to head (673f776).
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8330      +/-   ##
==========================================
- Coverage   87.39%   87.36%   -0.04%     
==========================================
  Files         254      255       +1     
  Lines        9476     9461      -15     
==========================================
- Hits         8282     8266      -16     
- Misses       1194     1195       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

s3 event schema

fbed014

medikoo suggested changes Oct 2, 2020

View reviewed changes

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

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

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

revert bucket name validation

2cfbccf

ozwi added 11 commits October 5, 2020 22:26

working on schema

7cef1b5

more s3 schema

f873251

all s3 bucket schemas

c3b82c5

rename sub schemas

f617ece

move sub schemas

c80c46d

more nuances

4e1c093

fix bucket schema

2cc0d57

revert .only

63c41af

Merge remote-tracking branch 'upstream/master' into aws-s3-event-schema

d711ddb

lowercase bucket properties + name

1ef487e

remove bucket props manual validation

2f65be7

thewizarodofoz requested a review from medikoo October 11, 2020 08:44

thewizarodofoz marked this pull request as draft October 11, 2020 12:09

fix tests

54c5aac

thewizarodofoz marked this pull request as ready for review October 11, 2020 13:28

lint

53a6a36

medikoo suggested changes Oct 12, 2020

View reviewed changes

ozwi added 6 commits October 15, 2020 15:37

unify s3 schema

d523cce

minor fixes

ce019ce

remove redundant comment

39cacc1

Merge remote-tracking branch 'upstream/master' into aws-s3-event-schema

0c78aa6

fix test

148d2b2

prettify

3fb3632

ozwi added 3 commits October 15, 2020 17:10

camelcase

91bf879

s3 event value could be a simple string

fd0a282

inline everything

c485812

thewizarodofoz requested a review from medikoo October 15, 2020 14:59

medikoo suggested changes Oct 16, 2020

View reviewed changes

ozwi added 2 commits October 16, 2020 13:01

fix date-time and move schema to right place

7cf21d9

fix error message

9265f8c

ozwi added 4 commits October 16, 2020 17:56

prettify

6a76210

replace oneOf with anyOf

9c7d21a

Merge remote-tracking branch 'upstream/master' into aws-s3-event-schema

fccb323

prettify

726766b

thewizarodofoz requested a review from medikoo October 16, 2020 15:11

medikoo suggested changes Oct 19, 2020

View reviewed changes

aws provider schema is done, no need for relax

6dc762f

medikoo mentioned this pull request Oct 27, 2020

Tests: Fix fixture service config #8447

Merged

ozwi added 2 commits October 28, 2020 13:22

fix DefaultRetention

f263897

Merge remote-tracking branch 'upstream/master' into aws-s3-event-schema

58b3e1d

medikoo mentioned this pull request Oct 28, 2020

Config Schema: Fix provider validation #8450

Merged

Merge remote-tracking branch 'upstream/master' into aws-s3-event-schema

673f776

thewizarodofoz requested a review from medikoo October 30, 2020 13:47

medikoo approved these changes Oct 30, 2020

View reviewed changes

medikoo merged commit 61d8ee9 into serverless:master Oct 30, 2020

thewizarodofoz deleted the aws-s3-event-schema branch October 30, 2020 19:18

	'accelerateConfiguration',
	'accessControl',
	'analyticsConfigurations',
	'bucketEncryption',
	'bucketName',
	'corsConfiguration',
	'inventoryConfigurations',
	'lifecycleConfiguration',
	'loggingConfiguration',
	'metricsConfigurations',
	'notificationConfiguration',
	'objectLockConfiguration',
	'objectLockEnabled',
	'publicAccessBlockConfiguration',
	'replicationConfiguration',
	'tags',
	'versioningConfiguration',
	'websiteConfiguration',

	if (this.serverless.service.provider.name === 'aws') {
	// TODO: Remove after provider is fully covered, see https://github.com/serverless/serverless/issues/8022
	this.serverless.configSchemaHandler.relaxAwsProviderSchema();
	}

Uh oh!

Conversation

thewizarodofoz commented Oct 2, 2020

Uh oh!

codecov-commenter commented Oct 2, 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!

thewizarodofoz commented Oct 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

medikoo commented Oct 5, 2020

Uh oh!

codecov-io commented Oct 11, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

medikoo Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

medikoo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

medikoo commented Oct 16, 2020

Uh oh!

thewizarodofoz commented Oct 16, 2020

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thewizarodofoz commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

medikoo commented Oct 27, 2020

Uh oh!

thewizarodofoz commented Oct 28, 2020

Uh oh!

medikoo commented Oct 29, 2020

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Apr 3, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

codecov-commenter commented Oct 2, 2020 •

edited

Loading

thewizarodofoz commented Oct 3, 2020 •

edited

Loading

codecov-io commented Oct 11, 2020 •

edited

Loading

medikoo Oct 14, 2020 •

edited

Loading

medikoo left a comment •

edited

Loading

thewizarodofoz commented Oct 20, 2020 •

edited

Loading