I think we should also require 'type'

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.

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

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 ?

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.

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 ?

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!

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.

agreed

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

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)