🪟🎉 Connector form: Reload config if updated during connection check

[flash1293]

What

Fixes #21041

This PR will make sure updates to the configuration of an existing source or destination during the connection check won't be lost due to a subsequent update dispatched by the UI.

How

The flag indicating that the config got updated during the check is already returned to the webapp (jobInfo.connectorConfigurationUpdated). In airbyte-webapp/src/views/Connector/ConnectorCard/ConnectorCard.tsx this response can be checked and if it's set, the actual update call can be prevented as it has been done implicitly by the connection check.

At this point the form should be reloaded with the new config. This is done by passing down a reloadConfig callback which is calling queryClient.invalidateQueries for the source/destination which will cause react-query to reload the config and pass down the new state.

How to test

Unfortunately it's hard to cause a real update-on-check to happen - I tested by mocking the server like this (need to run ./gradlew airbyte-server:assemble and restart the docker containers):

--- a/airbyte-server/src/main/java/io/airbyte/server/apis/SourceApiController.java
+++ b/airbyte-server/src/main/java/io/airbyte/server/apis/SourceApiController.java
@@ -57,7 +57,11 @@ public class SourceApiController implements SourceApi {
   @Secured({EDITOR})
   @Override
   public CheckConnectionRead checkConnectionToSourceForUpdate(final SourceUpdate sourceUpdate) {
-    return ApiHelper.execute(() -> schedulerHandler.checkSourceConnectionFromSourceIdForUpdate(sourceUpdate));
+    return ApiHelper.execute(() -> {
+      final CheckConnectionRead r = schedulerHandler.checkSourceConnectionFromSourceIdForUpdate(sourceUpdate);
+      r.setJobInfo(r.getJobInfo().connectorConfigurationUpdated(true));
+      return r;
+    });
   }

   @Post("/clone")
@@ -95,7 +99,11 @@ public class SourceApiController implements SourceApi {
   @Secured({READER})
   @Override
   public SourceRead getSource(final SourceIdRequestBody sourceIdRequestBody) {
-    return ApiHelper.execute(() -> sourceHandler.getSource(sourceIdRequestBody));
+    return ApiHelper.execute(() -> {
+      final SourceRead r = sourceHandler.getSource(sourceIdRequestBody);
+      r.setName(r.getName() + System.currentTimeMillis());
+      return r;
+    });
   }

checkConnectionToSourceForUpdate automatically sets the flag to true for all connection checks. The getSource endpoint adds the current time in ms to the name of the connection to make it obvious the form actually updated.

https://www.loom.com/share/7d73b667dc4b40378f268ad8bf987f22

[lmossman]

I tested this out locally following your instructions, and I noticed some weird behavior which I think is only happening because the test isn't matching prod behavior, but I wanted to double check my understanding:

• I tried editing the Pokemon Name on one of my existing connectors, and when I clicked Test and save, the Source name was updated with the current timestamp as expected, but the Pokemon Name was set back to the name that had been saved to the source before I changed it.
• I believe this is expected from this test though, because we just modified the airbyte-server to return a response with the connectorConfigurationUpdated flag set to true, and only the connector name is changed in the response of getSource(), so the new pokemon name was never actually saved.
• In the production use case that this PR is handling, the checkConnectionToSourceForUpdate() call would actually update the source to have the new pokemon name, so the subsequent getSource() call would respond with that new name, and it therefore wouldn't go back to the old name.

If the above sounds correct to you, then I think the changes here look good.

I just had one question about a return that you added, but I think it can be fixed without me needing to give it another look

[lmossman]

If we return here on line 134, doesn't that mean we will never execute the next line to track the test success?

It feels like we should instead be saving the result of testConnector() to a variable, then calling track, then returning that variable

[flash1293]

Good catch, no idea how I missed this. @timroes is there a reason not to fail the build on unreachable code?

[flash1293]

If the above sounds correct to you, then I think the changes here look good.

Exactly, that's how I understood the flow. Just to make sure - @pedroslopez can you confirm that if jobInfo.connectorConfigurationUpdated is set, then the whole config object that was sent to the check_for_update endpoint is persisted already, right?

[pedroslopez]

If the above sounds correct to you, then I think the changes here look good.

Exactly, that's how I understood the flow. Just to make sure - @pedroslopez can you confirm that if jobInfo.connectorConfigurationUpdated is set, then the whole config object that was sent to the check_for_update endpoint is persisted already, right?

Yep! With the caveat that it’s likely not the exact config object sent as part of the request, since this happens because the connector itself wanted to change some config values.