Add breakingChange to catalogDiff #17588

alovew · 2022-10-04T23:02:39Z

isBreaking is a new field on the FieldTransform object within the CatalogDiff object. When we calculate catalog diffs, we will now pass in the connection's configuredCatalog since that has data about the sync modes & primary and cursor fields for a connection.

A FieldTransform is considered 'breaking' if:

the connection is INCREMENTAL and the cursor field was removed OR
the connection is DEDUP and the primary key was removed

This PR does the following:

Update the FieldTransform object to add this new field
Populate the isBreaking field correctly within the getDiff calculation
Update the API to require the isBreaking field with the CatalogDiff response
Return the isBreaking field with API responses that include CatalogDiffs

alovew · 2022-10-05T21:23:04Z

docs/reference/api/generated-api-html/index.html

@@ -8285,13 +8285,15 @@ <h3 class="field-label">Example data</h3>
        "fieldName" : [ "fieldName", "fieldName" ],
        "addField" : { },
        "transformType" : "add_field",
-        "removeField" : { }
+        "removeField" : { },
+        "isBreaking" : true


this was automatically generated and I'm not sure what it is or whether I should change it

I think this is fine.

alovew · 2022-10-05T21:23:20Z

docs/reference/api/generated-api-html/index.html

@@ -10475,6 +10487,7 @@ <h3><a name="FieldTransform"><code>FieldTransform</code> - </a> <a class="up" hr
        <div class="param-enum-header">Enum:</div>
        <div class="param-enum">add_field</div><div class="param-enum">remove_field</div><div class="param-enum">update_field_schema</div>
 <div class="param">fieldName </div><div class="param-desc"><span class="param-type"><a href="#string">array[String]</a></span> A field name is a list of strings that form the path to the field. </div>
+<div class="param">isBreaking </div><div class="param-desc"><span class="param-type"><a href="#boolean">Boolean</a></span>  </div>


this was also automatically generated

edmundito

Frontend changes look good. ✅

jdpgrailsdev · 2022-10-06T14:22:09Z

airbyte-api/src/main/openapi/config.yaml

@@ -4163,6 +4164,8 @@ components:
            - update_field_schema
        fieldName:
          $ref: "#/components/schemas/FieldName"
+        isBreaking:


Nit: It doesn't look like we are using the isX convention for other boolean fields in the OpenAPI config. Most appear to be just X (e.g. succeeded, available, retryable, etc). I do see one example of hasX, but I'm not sure if we have a convention and/or want to follow it with this field by using breakingChange instead of isBreaking. @cgardens Thoughts on whether or not we have a naming convention for OpenAPI boolean properties?

I saw isSyncing on webBackendConnectionRead, but happy to change this if it's more in line with our convention - isSyncing might be the only one

@alovew I don't really have an opinion one way or another -- I just noticed that we are not using the isX convention. Just wanted to make sure that there wasn't a decision that predates both of us to not use that convention before we introduce inconsistency in the API.

I vote we drop the is prefix. Looking at all the other type: boolean fields we have, one of them has the is prefix; isSyncing, three have a has prefix; hasConnections, hasSources, hasDestination and every other one has no prefix.

Additionally can we add a description to this field indicating what it is used for?

krishnaglick

I was trying to test this but I was getting this error when trying to refresh my schema.



"Internal  Server Error: Cannot invoke  \"io.airbyte.api.model.generated.AirbyteCatalog.getStreams()\" because  \"discovered\" is null"

Unsure if this is related to this PR.

alovew · 2022-10-06T18:39:08Z

@krishnaglick can you give me more details about how you saw this error? which source? was it a new connection & were there schema changes?

alovew · 2022-10-06T18:39:54Z

@krishnaglick also where in the codebase the error was invoked?

krishnaglick · 2022-10-06T18:40:34Z

@krishnaglick can you give me more details about how you saw this error? which source? was it a new connection & were there schema changes?

I have two postgres DB's syncing to eachother. I cleaned out one to a default, pristine state, and did a schema update. I'm going to try against master too.

alovew · 2022-10-06T19:13:05Z

spoke with @krishnaglick on slack, and apparently he is seeing this error on master as well

colesnodgrass

Found potential for a couple bugs.

Does this need to be a Boolean field or can we make it a boolean instead?

colesnodgrass · 2022-10-10T17:22:24Z

airbyte-protocol/protocol-models/src/main/java/io/airbyte/protocol/models/CatalogHelpers.java

+          final List<ConfiguredAirbyteStream> streamList = configuredCatalog.getStreams().stream()
+              .filter(s -> Objects.equals(s.getStream().getNamespace(), descriptor.getNamespace())
+                  && s.getStream().getName().equals(descriptor.getName()))
+              .toList();
+
+          final ConfiguredAirbyteStream stream = streamList.get(0);


These two statements could be combined into one statement which removes the need to have the streamList variable by replacing toList() with findFirst()

final var stream = configuredCatalog.getStreams().stream() .filter(s -> Objects.equals(s.getStream().getNamespace(), descriptor.getNamespace()) && s.getStream().getName().equals(descriptor.getName())) .findFirst();

Additionally, as the stream variable is only referenced if !streamOld.equals(streamNew) this work would be moved to inside that if statement. No reason to do this work if there is a chance it's going to be ignored.

The findFirst() call returns an Optional so you would need to check if you actually received a value.

Also there is an issue with the current code, what if the filter call returns no results? The streamList.get(0) line will fail with an IndexOutOfBoundsException.

colesnodgrass · 2022-10-10T17:40:55Z

airbyte-api/src/main/openapi/config.yaml

@@ -4163,6 +4164,8 @@ components:
            - update_field_schema
        fieldName:
          $ref: "#/components/schemas/FieldName"
+        isBreaking:


I vote we drop the is prefix. Looking at all the other type: boolean fields we have, one of them has the is prefix; isSyncing, three have a has prefix; hasConnections, hasSources, hasDestination and every other one has no prefix.

Additionally can we add a description to this field indicating what it is used for?

colesnodgrass · 2022-10-10T17:47:40Z

airbyte-protocol/protocol-models/src/main/java/io/airbyte/protocol/models/CatalogHelpers.java

@@ -384,4 +405,17 @@ static void combineAccumulator(final Map<List<String>, JsonNode> accumulatorLeft
    });
  }

+  static Boolean transformBreaksConnection(final ConfiguredAirbyteStream configuredStream, final List<String> fieldName) {


Can this return boolean instead of Boolean?

colesnodgrass · 2022-10-10T17:54:35Z

airbyte-protocol/protocol-models/src/main/java/io/airbyte/protocol/models/CatalogHelpers.java

+    if (SyncMode.INCREMENTAL == syncMode && configuredStream.getCursorField().equals(fieldName)) {
+      return true;
+    }
+
+    final DestinationSyncMode destinationSyncMode = configuredStream.getDestinationSyncMode();
+    if (DestinationSyncMode.APPEND_DEDUP == destinationSyncMode && configuredStream.getPrimaryKey().contains(fieldName)) {


Potential for a subtle bug here as the equals of a list is only true when

two lists are defined to be equal if they contain the same elements in the same order

Is there a potential that these lists contain the same fields in a different order? Do we need to order these lists before comparing them?

This affects both configuredStream.getCursorField().equals(fieldName) and configuredStream.getPrimaryKey().contains(fieldName).

I thought there could only ever be one cursorField, but I think changing it to contains would fix this. I don't think there's a problem with the second one since it's using contains instead of equals. Is there something I'm missing about contains?

Oh I see, fieldName is also a list. I didn't know why this is a list - there is also only ever one item in the fieldName list. I couldn't find examples where there were fieldNames that consisted of a list of multiple strings. I can check with the connectors team on this though.

@colesnodgrass it looks like the way we do existing diffs between field names, the order matters. I can't find an example of this, but if one field name is a list of ["field_prefix", "field_name"] and another is ["field_name", "field_prefix"], this would be considered a different field and it would be added to the CatalogDiff

Interesting, this may be something we want to explicitly state/enforce somewhere in the code. Or maybe abstract behind a new datatype as there is a potential here for breaking this accidentally. But this doesn't need to be done as part of this PR.

colesnodgrass · 2022-10-10T17:56:59Z

airbyte-protocol/protocol-models/src/main/java/io/airbyte/protocol/models/CatalogHelpers.java

+          if (transformBreaksConnection(configuredStream, fieldName)) {
+            fieldTransforms.add(FieldTransform.createRemoveFieldTransform(fieldName, fieldNameToTypeOld.get(fieldName), true));
+          } else {
+            fieldTransforms.add(FieldTransform.createRemoveFieldTransform(fieldName, fieldNameToTypeOld.get(fieldName), false));
+          }


the if/else isn't necessary, can be replaced with

fieldTransforms.add(FieldTransform.createRemoveFieldTransform( fieldName, fieldNameToTypeOld.get(fieldName), transformBreaksConnection(configuredStream, fieldName) ));

colesnodgrass · 2022-10-10T17:57:22Z

airbyte-protocol/protocol-models/src/main/java/io/airbyte/protocol/models/CatalogHelpers.java

    Sets.difference(fieldNameToTypeNew.keySet(), fieldNameToTypeOld.keySet())
-        .forEach(fieldName -> fieldTransforms.add(FieldTransform.createAddFieldTransform(fieldName, fieldNameToTypeNew.get(fieldName))));
+        .forEach(fieldName -> {


Are the curly-braces necessary here?

colesnodgrass · 2022-10-10T17:58:34Z

...rotocol-models/src/main/java/io/airbyte/protocol/models/transform_models/FieldTransform.java

@@ -64,4 +67,8 @@ public UpdateFieldSchemaTransform getUpdateFieldTransform() {
    return updateFieldTransform;
  }

+  public Boolean getIsBreaking() {


This should be isBreaking() instead of getIsBreaking().

Also could this be a boolean instead of a Boolean?

benmoriceau

LGTM, I agree with comments of @colesnodgrass . I'm going to leave him to approve the PR.

colesnodgrass

See my comment on the Optional.get() usage.

colesnodgrass · 2022-10-10T22:22:53Z

airbyte-protocol/protocol-models/src/main/java/io/airbyte/protocol/models/CatalogHelpers.java

+          final ConfiguredAirbyteStream stream = configuredCatalog.getStreams().stream()
+              .filter(s -> Objects.equals(s.getStream().getNamespace(), descriptor.getNamespace())
+                  && s.getStream().getName().equals(descriptor.getName()))
+              .findFirst().get();


.get() will throw a NoSuchElementException if called on an empty Optional.

What should happen here if we have nothing? Should streamTransoforms.add still be called? Is nothing an error case?

I don't think this is a case that should happen, but if it does, I think we should default to calling the transform non-breaking. I'll make that change.

colesnodgrass

👍🏻 I think it's also worth looking into using final var more than explicitly specifying the object type as this makes cleaning-up/refactoring this code easier.

implemented changes

* Add breaking field to FieldTransform on catalogDiff

github-actions bot added area/api Related to the api area/documentation Improvements or additions to documentation area/platform issues related to the platform area/protocol area/server area/frontend Related to the Airbyte webapp labels Oct 4, 2022

alovew temporarily deployed to more-secrets October 4, 2022 23:03 Inactive

alovew temporarily deployed to more-secrets October 5, 2022 17:56 Inactive

alovew temporarily deployed to more-secrets October 5, 2022 20:52 Inactive

alovew temporarily deployed to more-secrets October 5, 2022 21:19 Inactive

alovew marked this pull request as ready for review October 5, 2022 21:20

alovew requested review from a team as code owners October 5, 2022 21:20

alovew commented Oct 5, 2022

View reviewed changes

alovew requested review from gosusnp, benmoriceau, jdpgrailsdev, colesnodgrass and teallarson October 5, 2022 21:23

alovew force-pushed the anne/add-breaking-change-to-catalog-diff branch from 5b74a75 to c4897a6 Compare October 5, 2022 22:18

alovew temporarily deployed to more-secrets October 5, 2022 22:20 Inactive

alovew temporarily deployed to more-secrets October 6, 2022 00:14 Inactive

edmundito reviewed Oct 6, 2022

View reviewed changes

jdpgrailsdev reviewed Oct 6, 2022

View reviewed changes

krishnaglick reviewed Oct 6, 2022

View reviewed changes

colesnodgrass requested changes Oct 10, 2022

View reviewed changes

benmoriceau reviewed Oct 10, 2022

View reviewed changes

alovew requested a review from colesnodgrass October 10, 2022 21:49

alovew temporarily deployed to more-secrets October 10, 2022 22:18 Inactive

colesnodgrass previously requested changes Oct 10, 2022

View reviewed changes

alovew temporarily deployed to more-secrets October 10, 2022 22:46 Inactive

alovew temporarily deployed to more-secrets October 10, 2022 22:58 Inactive

alovew temporarily deployed to more-secrets October 10, 2022 23:16 Inactive

colesnodgrass approved these changes Oct 10, 2022

View reviewed changes

alovew added 10 commits October 10, 2022 16:52

Add breakingChange to catalogDiff

8743925

fix existing tests

58a0884

catalog diff test

9c0e8ed

formatting

f6d4be0

tests and pmd

f7d0067

code review comments

d95b22c

code review changes

4b6f878

change is non-breaking if stream config cannot be found

76936ad

test for not finding stream in configured catalog

87ca87a

pmd

cb3639b

alovew force-pushed the anne/add-breaking-change-to-catalog-diff branch from 74928be to cb3639b Compare October 10, 2022 23:52

alovew temporarily deployed to more-secrets October 10, 2022 23:55 Inactive

alovew merged commit ca2605d into master Oct 11, 2022

alovew deleted the anne/add-breaking-change-to-catalog-diff branch October 11, 2022 01:04

This was referenced Oct 13, 2022

Bump Airbyte version from 0.40.14 to 0.40.15 #17961

Closed

Bump Airbyte version from 0.40.14 to 0.40.15 #17970

Merged

jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022

Add breakingChange to catalogDiff (airbytehq#17588)

ae92b06

* Add breaking field to FieldTransform on catalogDiff

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add breakingChange to catalogDiff #17588

Add breakingChange to catalogDiff #17588

alovew commented Oct 4, 2022 •

edited

Loading

alovew Oct 5, 2022

krishnaglick Oct 6, 2022

alovew Oct 5, 2022

edmundito left a comment

jdpgrailsdev Oct 6, 2022

alovew Oct 6, 2022

jdpgrailsdev Oct 6, 2022

colesnodgrass Oct 10, 2022

krishnaglick left a comment

alovew commented Oct 6, 2022

alovew commented Oct 6, 2022

krishnaglick commented Oct 6, 2022

alovew commented Oct 6, 2022

colesnodgrass left a comment

colesnodgrass Oct 10, 2022

colesnodgrass Oct 10, 2022

colesnodgrass Oct 10, 2022

colesnodgrass Oct 10, 2022

colesnodgrass Oct 10, 2022

alovew Oct 10, 2022

alovew Oct 10, 2022

alovew Oct 10, 2022

colesnodgrass Oct 10, 2022

colesnodgrass Oct 10, 2022

colesnodgrass Oct 10, 2022

colesnodgrass Oct 10, 2022

benmoriceau left a comment

colesnodgrass left a comment

colesnodgrass Oct 10, 2022

alovew Oct 10, 2022

colesnodgrass left a comment

Add breakingChange to catalogDiff #17588

Add breakingChange to catalogDiff #17588

Conversation

alovew commented Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmundito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krishnaglick left a comment

Choose a reason for hiding this comment

alovew commented Oct 6, 2022

alovew commented Oct 6, 2022

krishnaglick commented Oct 6, 2022

alovew commented Oct 6, 2022

colesnodgrass left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmoriceau left a comment

Choose a reason for hiding this comment

colesnodgrass left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colesnodgrass left a comment

Choose a reason for hiding this comment

alovew commented Oct 4, 2022 •

edited

Loading