☝🏼 Upgrade all connectors (0.2.0) so protocol allows future / unknown properties #2238

ChristopheDuong · 2021-03-01T16:12:04Z

What

This PR unblocks #2228 which needs to add new properties to the protocol.

However, the current connectors are throwing JSON validation errors if we try to evolve the protocol with new properties as they are currently unknown to previously released connectors.

How

Describe the solution

Connectors should accept new properties (in case we add new features in the future) and continue working while ignoring those new properties.

This PR isn't introducing new actual features, it is just making sure that after this being merged, every source connectors will be able to properly run even if the protocol is changed in the future

Pre-merge Checklist

Run integration tests for all source connectors
Publish Docker images for all source connectors
Provide migration script to force upgrade all source connectors

Recommended reading order

airbyte-commons/src/main/java/io/airbyte/commons/json/Jsons.java
airbyte-migration/src/main/java/io/airbyte/migrate/migrations/MigrationV0_17_0.java
the rest

ChristopheDuong · 2021-03-01T17:03:30Z

I would need some initial approval and then I will start publishing new connectors along with migration script if this is the proper path we want to take.

cgardens · 2021-03-01T21:17:51Z

airbyte-commons/src/main/java/io/airbyte/commons/json/Jsons.java

  private static final ObjectWriter OBJECT_WRITER = OBJECT_MAPPER.writer(new JsonPrettyPrinter());

+  private static ObjectMapper initMapper() {
+    final ObjectMapper result = new ObjectMapper();
+    result.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);


if in an object has additionalProperties=false, does this override it? or it his just overriding default behavior?

shouldn't be be changing the additionalProperties field in objects as well (or instead, depending on the answer to the first question)?

To @cgardens point. There are 2 parts to allowing unknown properties:

Ensure our Json SerDe accepts it

Ensure our Json Schema validation accepts it

The PR only address the first point.

@cgardens I don't think your first point matter since additionalProperties only makes sense once the model goes through validation.

The additionalProperties fields seem to influence only the JSON Schema validation as explained by Michel (which results in modifications to generated files for pydantic only?)

The Jackson SerDe operations are only configured by the ObjectMapper feature configure call.

Anyway, I ran a manual test where I've tried building a docker image / artifact of a connector from this branch.

Then I tested with modifications to the protocol (on top of the Step 2: Add Namespace to Airbyte Protocol #2228 branch changes)

I verified that JSON validation in the connector works thanks to this PR. (Connectors with versions 0.1.X would fail)

So everything is able to run normally (even though the connector is not supposed to support namespace fields yet)

are you saying this change isn't necessary or that this change is what makes this possible? if you know what's going on here then i'm sure it's fine. i'm not fully understanding your last comment though.

Yes, these are the changes that make it possible

michel-tricot

Once you've also addressed the validation and updated all the connectors that need it, I think the PR is good to go.

michel-tricot · 2021-03-01T23:08:04Z

airbyte-commons/src/main/java/io/airbyte/commons/json/Jsons.java

  private static final ObjectWriter OBJECT_WRITER = OBJECT_MAPPER.writer(new JsonPrettyPrinter());

+  private static ObjectMapper initMapper() {
+    final ObjectMapper result = new ObjectMapper();
+    result.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);


To @cgardens point. There are 2 parts to allowing unknown properties:

Ensure our Json SerDe accepts it

Ensure our Json Schema validation accepts it

The PR only address the first point.

@cgardens I don't think your first point matter since additionalProperties only makes sense once the model goes through validation.

cgardens

i think we don't need to copy the full definitions file. other than that looks good.

airbyte-migration/src/main/java/io/airbyte/migrate/migrations/MigrationV0_17_0.java

...n/src/main/resources/migrations/migrationV0_17_0/airbyte_config/destination_definitions.yaml

cgardens · 2021-03-02T21:59:52Z

airbyte-commons/src/main/java/io/airbyte/commons/json/Jsons.java

  private static final ObjectWriter OBJECT_WRITER = OBJECT_MAPPER.writer(new JsonPrettyPrinter());

+  private static ObjectMapper initMapper() {
+    final ObjectMapper result = new ObjectMapper();
+    result.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);


are you saying this change isn't necessary or that this change is what makes this possible? if you know what's going on here then i'm sure it's fine. i'm not fully understanding your last comment though.

airbyte-migration/src/main/java/io/airbyte/migrate/migrations/MigrationV0_17_0.java

cgardens · 2021-03-02T22:11:31Z

I understand why you want to write a migration. I didn't know we were going to automatically force upgrade connectors. I like the idea though and it seems like a good choice that saves customers headache in this case.

michel-tricot

LGTM % charles comments

airbyte-integrations/connectors/source-jdbc/src/main/resources/jdbc_models/jdbc_models.yaml

ChristopheDuong · 2021-03-03T12:08:35Z

/publish connector=connectors/source-appstore-singer

🕑 connectors/source-appstore-singer https://github.com/airbytehq/airbyte/actions/runs/617464383
✅ connectors/source-appstore-singer https://github.com/airbytehq/airbyte/actions/runs/617464383

ChristopheDuong · 2021-03-03T12:19:36Z

/publish connector=connectors/source-braintree-singer

🕑 connectors/source-braintree-singer https://github.com/airbytehq/airbyte/actions/runs/617492893
✅ connectors/source-braintree-singer https://github.com/airbytehq/airbyte/actions/runs/617492893

ChristopheDuong · 2021-03-03T12:38:51Z

/publish connector=connectors/destination-postgres

🕑 connectors/destination-postgres https://github.com/airbytehq/airbyte/actions/runs/617543998
✅ connectors/destination-postgres https://github.com/airbytehq/airbyte/actions/runs/617543998

ChristopheDuong · 2021-03-03T12:47:08Z

/publish connector=connectors/source-drift

🕑 connectors/source-drift https://github.com/airbytehq/airbyte/actions/runs/617565339
❌ connectors/source-drift https://github.com/airbytehq/airbyte/actions/runs/617565339
🕑 connectors/source-drift https://github.com/airbytehq/airbyte/actions/runs/617565339
❌ connectors/source-drift https://github.com/airbytehq/airbyte/actions/runs/617565339

ChristopheDuong · 2021-03-03T12:47:09Z

/publish connector=connectors/source-exchangeratesapi-singer

🕑 connectors/source-exchangeratesapi-singer https://github.com/airbytehq/airbyte/actions/runs/617565291
✅ connectors/source-exchangeratesapi-singer https://github.com/airbytehq/airbyte/actions/runs/617565291

ChristopheDuong · 2021-03-03T12:47:11Z

/publish connector=connectors/source-facebook-marketing

🕑 connectors/source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/617565416
✅ connectors/source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/617565416

ChristopheDuong · 2021-03-03T12:47:13Z

/publish connector=connectors/source-file

🕑 connectors/source-file https://github.com/airbytehq/airbyte/actions/runs/617565426
✅ connectors/source-file https://github.com/airbytehq/airbyte/actions/runs/617565426

ChristopheDuong · 2021-03-03T12:47:15Z

/publish connector=connectors/source-freshdesk

🕑 connectors/source-freshdesk https://github.com/airbytehq/airbyte/actions/runs/617565568
✅ connectors/source-freshdesk https://github.com/airbytehq/airbyte/actions/runs/617565568

ChristopheDuong · 2021-03-03T12:47:16Z

/publish connector=connectors/source-github-singer

🕑 connectors/source-github-singer https://github.com/airbytehq/airbyte/actions/runs/617565610
✅ connectors/source-github-singer https://github.com/airbytehq/airbyte/actions/runs/617565610

ChristopheDuong · 2021-03-03T12:47:18Z

/publish connector=connectors/source-google-adwords-singer

🕑 connectors/source-google-adwords-singer https://github.com/airbytehq/airbyte/actions/runs/617565743
❌ connectors/source-google-adwords-singer https://github.com/airbytehq/airbyte/actions/runs/617565743

ChristopheDuong · 2021-03-03T12:55:18Z

/publish connector=connectors/destination-csv

🕑 connectors/destination-csv https://github.com/airbytehq/airbyte/actions/runs/617587254
✅ connectors/destination-csv https://github.com/airbytehq/airbyte/actions/runs/617587254

ChristopheDuong · 2021-03-03T12:55:19Z

/publish connector=connectors/destination-jdbc

🕑 connectors/destination-jdbc https://github.com/airbytehq/airbyte/actions/runs/617587285
✅ connectors/destination-jdbc https://github.com/airbytehq/airbyte/actions/runs/617587285

ChristopheDuong · 2021-03-03T12:55:21Z

/publish connector=connectors/destination-local-json

🕑 connectors/destination-local-json https://github.com/airbytehq/airbyte/actions/runs/617587305
✅ connectors/destination-local-json https://github.com/airbytehq/airbyte/actions/runs/617587305

ChristopheDuong · 2021-03-03T12:55:23Z

/publish connector=connectors/destination-meilisearch

🕑 connectors/destination-meilisearch https://github.com/airbytehq/airbyte/actions/runs/617587577
✅ connectors/destination-meilisearch https://github.com/airbytehq/airbyte/actions/runs/617587577

ChristopheDuong · 2021-03-03T12:55:25Z

/publish connector=connectors/destination-redshift

🕑 connectors/destination-redshift https://github.com/airbytehq/airbyte/actions/runs/617587466
✅ connectors/destination-redshift https://github.com/airbytehq/airbyte/actions/runs/617587466

ChristopheDuong · 2021-03-03T12:55:26Z

/publish connector=connectors/destination-snowflake

🕑 connectors/destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/617587472
✅ connectors/destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/617587472

ChristopheDuong · 2021-03-03T14:02:41Z

/test connector=source-drift

🕑 source-drift https://github.com/airbytehq/airbyte/actions/runs/617766492
❌ source-drift https://github.com/airbytehq/airbyte/actions/runs/617766492

…wn-properties

Allow new jsons properties

de9db95

ChristopheDuong added this to the 2021-03-05 milestone Mar 1, 2021

ChristopheDuong self-assigned this Mar 1, 2021

ChristopheDuong mentioned this pull request Mar 1, 2021

Namespace destination output #1921

Closed

cgardens reviewed Mar 1, 2021

View reviewed changes

cgardens requested review from michel-tricot and jrhizor March 1, 2021 21:20

michel-tricot approved these changes Mar 1, 2021

View reviewed changes

ChristopheDuong added 5 commits March 2, 2021 14:52

Allow extra properties in validation

1eee3bb

allow extra properties for source jdbc models

09b043e

Merge remote-tracking branch 'origin/master' into local

1d419b5

Create migration script to upgrade all connectors versions

9f8e53b

Bumpversion of all connectors

41167e2

cgardens self-requested a review March 2, 2021 19:57

cgardens approved these changes Mar 2, 2021

View reviewed changes

cgardens reviewed Mar 2, 2021

View reviewed changes

airbyte-migration/src/main/java/io/airbyte/migrate/migrations/MigrationV0_17_0.java Show resolved Hide resolved

michel-tricot reviewed Mar 3, 2021

View reviewed changes

airbyte-integrations/connectors/source-jdbc/src/main/resources/jdbc_models/jdbc_models.yaml Outdated Show resolved Hide resolved

Changes from code review

117ccd6

cgardens modified the milestones: Core - 2021-03-05, Core - 2021-03-12 Mar 8, 2021

cgardens assigned cgardens and unassigned cgardens Mar 8, 2021

Merge remote-tracking branch 'origin/master' into protocol-with-unkno…

bb6cfaa

…wn-properties

ChristopheDuong merged commit 070575f into master Mar 9, 2021

ChristopheDuong deleted the chris/protocol-with-unknown-properties branch March 9, 2021 12:36

ChristopheDuong changed the title ~~Protocol allows future / unknown properties~~ ☝🏼 Upgrade all connectors so protocol allows future / unknown properties Mar 9, 2021

ChristopheDuong changed the title ~~☝🏼 Upgrade all connectors so protocol allows future / unknown properties~~ ☝🏼 Upgrade all connectors (0.2.0) so protocol allows future / unknown properties Mar 9, 2021

ChristopheDuong mentioned this pull request Mar 30, 2021

Source & Destination sync modes are required #2500

Merged

2 tasks

igrankova added connectors/sources-api connectors/source/appstore-singer labels Jan 4, 2022

karinakuz added connectors/source/facebook-marketing connectors/destination/bigquery connectors/destinations-api labels Jan 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

☝🏼 Upgrade all connectors (0.2.0) so protocol allows future / unknown properties #2238

☝🏼 Upgrade all connectors (0.2.0) so protocol allows future / unknown properties #2238

ChristopheDuong commented Mar 1, 2021 •

edited

Loading

ChristopheDuong commented Mar 1, 2021

cgardens Mar 1, 2021

michel-tricot Mar 1, 2021

ChristopheDuong Mar 2, 2021 •

edited

Loading

cgardens Mar 2, 2021

ChristopheDuong Mar 3, 2021

michel-tricot left a comment

michel-tricot Mar 1, 2021

cgardens left a comment

cgardens Mar 2, 2021

cgardens commented Mar 2, 2021

michel-tricot left a comment

ChristopheDuong commented Mar 3, 2021 •

edited

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

☝🏼 Upgrade all connectors (0.2.0) so protocol allows future / unknown properties #2238

☝🏼 Upgrade all connectors (0.2.0) so protocol allows future / unknown properties #2238

Conversation

ChristopheDuong commented Mar 1, 2021 • edited Loading

What

How

Pre-merge Checklist

Recommended reading order

ChristopheDuong commented Mar 1, 2021

cgardens Mar 1, 2021

Choose a reason for hiding this comment

michel-tricot Mar 1, 2021

Choose a reason for hiding this comment

ChristopheDuong Mar 2, 2021 • edited Loading

Choose a reason for hiding this comment

cgardens Mar 2, 2021

Choose a reason for hiding this comment

ChristopheDuong Mar 3, 2021

Choose a reason for hiding this comment

michel-tricot left a comment

Choose a reason for hiding this comment

michel-tricot Mar 1, 2021

Choose a reason for hiding this comment

cgardens left a comment

Choose a reason for hiding this comment

cgardens Mar 2, 2021

Choose a reason for hiding this comment

cgardens commented Mar 2, 2021

michel-tricot left a comment

Choose a reason for hiding this comment

ChristopheDuong commented Mar 3, 2021 • edited Loading

ChristopheDuong commented Mar 3, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 3, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 3, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 3, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 3, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 3, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 3, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 3, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 3, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 3, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 3, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 3, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 3, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 3, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 3, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 3, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 1, 2021 •

edited

Loading

ChristopheDuong Mar 2, 2021 •

edited

Loading

ChristopheDuong commented Mar 3, 2021 •

edited

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 3, 2021 •

edited by github-actions bot

Loading