Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stream schema #8201

Merged
merged 5 commits into from Sep 14, 2020
Merged

Stream schema #8201

merged 5 commits into from Sep 14, 2020

Conversation

fredericbarthelet
Copy link
Contributor

Closes: #8034

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

Thanks you @fredericbarthelet, that's a great addition!

Unfortunately CI fails suggesting that proposed schema has some issues (?)

@fredericbarthelet
Copy link
Contributor Author

fredericbarthelet commented Sep 8, 2020

@medikoo , sorry, made a typo, interger instead of integer. Should be fixed now :)

What kind of tests could I put in place to replace removed test regarding configuration format ? Is there any integration test I could add to cover JSON schema validation ?

@codecov-commenter
Copy link

codecov-commenter commented Sep 8, 2020

Codecov Report

Merging #8201 into master will decrease coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8201      +/-   ##
==========================================
- Coverage   88.16%   88.11%   -0.06%     
==========================================
  Files         248      248              
  Lines        9430     9388      -42     
==========================================
- Hits         8314     8272      -42     
  Misses       1116     1116              
Impacted Files Coverage Δ
...plugins/aws/package/compile/events/stream/index.js 98.00% <100.00%> (-0.60%) ⬇️
lib/utils/getServerlessConfigFile.js 94.11% <0.00%> (ø)
lib/plugins/aws/provider/awsProvider.js 93.11% <0.00%> (ø)

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 4af57a5...3b17faf. Read the comment docs.

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

@fredericbarthelet looks very good, I have just few minor remarks. Let me know if my suggestions are correct

type: { enum: ['sns', 'sqs'] },
},
additionalProperties: false,
required: ['arn'],
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should also require 'type'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

type is only required when arn is not provided as a string, otherwise it can be computed from the string, thus making it conditionally required. However, I did not managed to achieve such conditional dependency in JSON schema. The properties dependency feature does not allow the same property to have multiple definition.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, I got confused as we support ARN string directly at onFailure and at onFailure.arn (and in that case both are equivalent).

In that case maybe it'll be good to configure three variants:

  • onFailure as arn string
  • onFailure as object with arn string (no type)
  • onFailure as object with arn and type required

oneOf will make case where arn string and type is used, invalid, but I think it's ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used anyOf to prevent such issue.

I updated with 3 possible cases. The same schema was also implemented for stream.arn and stream.type property at root. In order to avoid too much duplication, I added a addDefinitionsToSchema function on configSchemaHandler and defined mutual schema specific to stream event in this function. @medikoo , WDYT ?

lib/plugins/aws/package/compile/events/stream/index.js Outdated Show resolved Hide resolved
additionalProperties: false,
required: ['arn', 'type'],
},
],
Copy link
Contributor

Choose a reason for hiding this comment

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

Going that route is problematic for error reporting.

As e.g. if one makes error in batchSize, we will get error for both alternatives, and error normalizer, not being able to decide which alternative was chosen to be followed by user, will expose the error as generic functions[].events[].sns: unsupported configuration format which doesn't give a hint of what the real error is.

I think this can be cleanly solved solved by using additionalProperties as a container to define two options for arn an type, and keep others defined as it was in previous version

It also doesn't feel good, that we repeat definitions for most of properties, seems error-prone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @medikoo , not sur what you mean by using additionalProperties. Could you provide an exemple ?

In the meantime, I found a solution detailed in json-schema/json-schema#158 showing how to implement validation based on property value (the correct way of JSON schema is schema dependency based on property existence with dependencies keyword).

The resulting error message when specifying a CF intrinsic function without type is Configuration error at 'functions.toto.events[0].stream.destinations.onFailure.arn': should be string which seems correct. WDYT ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @medikoo , not sur what you mean by using additionalProperties. Could you provide an exemple ?

Sorry, that was an invalid suggestion, via additionalProperties you can enforce schema for other properties, but obviously not one by one, but for all generally.

which seems correct. WDYT ?

Looks great to me!

Comment on lines 50 to 64
oneOf: [
{
properties: {
arn: { $ref: '#/definitions/awsCfFunction' },
},
required: ['type'],
},
{
properties: {
arn: { $ref: '#/definitions/awsArnString' },
},
},
],
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder wouldn't it be more clear (less confusing), to totally discard properties above, and simply configure both variants fully in scope oneOf.

In current form it's a bit difficult to read. We have three properties references that outline two acceptable forms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@medikoo one small problem to that :

  • if I remove completly arn definition from properties base field, I get a validation error 'functions.hello.events[0].stream': unrecognized property 'arn' since additionalProperties: false is set.
  • if I want this error to not be thrown, arn needs to be defined outside of the oneOf clause. All statements in oneOf are complementary validation logic : both base object JSON schema and one statement of oneOf keyword must validate.

I have then 3 solutions :

  • keeping the PR as it is now where oneOf clauses about arn definitions are subparts of base properties.arn definition.
  • specifying arn: {} definition in the base properties keyword in order to tell that there is an arn property that will be defined later. The risk is someone opening up to widely the oneOf clauses about arn and have invalid arn specified. (what I updated the PR to with the last commit, for me this would be the preferred way since there are very small chances above scenario actually happen)
  • removing additionalProperties: false clause

WDYT ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I also think arn: {} would be the way to go. So in properties we indicate purely that it's a recognized property, but full schema is outlined in oneOf.

It gives clear separation, and minimal risk for mismatch error (e.g. there could be configuration issue, where in properties we configure schema that doesn't align with one configured in oneOf options)

lib/plugins/aws/package/compile/events/stream/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

Thank you @fredericbarthelet Looks great!

@medikoo medikoo merged commit 1fb338b into serverless:master Sep 14, 2020
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 "stream" function event properties
3 participants