Remove additionalProperties: false from all connector specs

[edgao]

As discovered in https://github.com/airbytehq/oncall/issues/289, a spec declaring "additionalProperties": false introduces the risk of accidental breaking changes. Specifically, when removing a property from the spec, existing connector configs will no longer be valid.

We should remove that declaration from all connector specs.

Until this issue is resolved, we should be careful when removing/renaming properties.

[edgao]

@grishick fyi, this is followup to https://docs.google.com/document/d/1ru9ApRI7fGtv5gNECchsQe6EB20LlY4UZhJOtEHvOAw/edit#

[andriikorotkov]

@grishick a similar issue - #14345

[grishick]

@edgao would it make sense to address this connector by connector starting with GA and Beta connectors?

[edgao]

probably, yes. I think the ideal ordering is:

1. Submit a gigantic PR to remove the additionalProperties declaration from ALL connectors spec.json
2. Submit a PR per connector to publish the image, starting with GA/beta, and maybe not even bothering with alpha at all.

Publishing the image is less urgent/important because it only matters if users downgrade the connector. The "remove property + upgrade" case (i.e. what's described in this issue's description) doesn't require a publish.

[grishick]

@sherifnada are you OK with the approach @edgao suggested?

[sherifnada]

cc @girarda @pedroslopez -- thoughts?

[girarda]

yes - it's safe to set additionalProperties to true.

i think this is a great occasion to try publishing multiple connectors at once

[pedroslopez]

Looks good to me, and good call out from @girarda - we can publish multiple connectors at the same time now so step 2 shouldn't require separate PRs per connector.

I will note that @Phlair recently tried to publish all the java connectors and ran into some issues #13947 - starting with the GA/betas seems fine.

[grishick]

yes - it's safe to set additionalProperties to true.

i think this is a great occasion to try publishing multiple connectors at once

[grishick]

A search for additionalProperties": false in spec.json files in connectors/ folder returns 207 results over 100+ connectors. So, I certainly wouldn't want to make 100+ PRs for this change, but I also would like to avoid blocking progress on prioritized connectors (we have 3 PRs waiting for this change). To unblock those other PRs, I will turn this into an Epic and create separate issues for unblocking current progress.

[bazarnov]

We need to add to the assertion the exact place in schema where the additionalProperties should be True but set to false, since now when you debug this, you're blind to see where it should be corrected. Also, we should specify what JSON schema is used (stream schema, spec.json, etc), there is no mention of this. Additionaly, if we have advancedAuth block in spec.json, should we cover this with additionalProperties as well? Please add more explanation from the technical perspective of how this should be properly set up and add more use-cases, not just simple JSON schema with 1 level of nesting. Thanks.

[edgao]

@bazarnov could you clarify what you're looking for? This issue is specifically for removing the additionalProperties: false from all connectors' spec.json, not about adding additionalProperties: true anywhere (since by default a schema allows additional properties)

[bazarnov]

Thanks for the question, at present, the value of this setting is obvious, no need to dive into a subject.

For all other future and current validation implementations based on checks of specific properties in schemas or input specifications, it would be nice to have the logging message where the issue occurred and what was expected (if applicable to the case).

Giving the general messages like in this test, not always playing good role for debugging purposes.

[evantahler]

I think this epic is able to be closed