Better handle control messages while editing the connector through UI (Part 1: API update)

[pedroslopez]

Tell us about the problem you're trying to solve

#20894 introduced updating connector configuration in check commands when the connector emits a config control message. This is mainly used for supporting single-use refresh tokens in an OAuth implementation, but can also be used for config migrations.

If the check operation changed your configuration (whether due to token refresh or config migration), the values in the form would become outdated. Because the UI calls an update after a successful check, the (non-secret) values updated by the control message would be reverted to those set on the form.

This is mostly a problem in the case of a reauthenticating a connector that always refreshes tokens ("eager token refresh"), since the check will invalidate the credentials and the UI will save the invalid credentials again. But it also means we're reverting any other changes that may be happening.

Describe the solution you’d like

We should:

1. Update the check_connection_for_update endpoint to return whether the configuration has changed as a result of the check. We can do this by comparing the provided config with the currently stored config after the check has completed.
2. On the frontend, if the above returns true, don't issue the update call (since something else updated the config) and show a message indicating to the user that the configuration has changed and they should reload the page.

Describe the alternative you’ve considered or used

Instead of showing a message telling users to reload, we could return the config and update the frontend to reflect the new configuration, but this is a bit more complex and doesn't seem high priority right now.

[evantahler]

@sherifnada / @flash1293 is this front-end change something you can help us with?

[flash1293]

@evantahler Absolutely, this seems like an important change.

Two questions about this:

• Just to be sure: With the proposed solution the connector might emit config control messages during a check on an unsaved configuration (e.g. the user setting up a new source) - these control messages would be lost but I guess that's expected, correct?
• Instead of returning the updated config in the response of the check_for_update call, what about sending the flag whether it updated automatically and have the frontend re-fetch the values of the form and repopulate it by calling (sources|destinations)/get (instead of showing a message to the user to reload manually)? This way no further backend changes would be needed and this kind of change is relatively simple in the frontend with our setup. This way the additional complexity of the connector updating its config during check is completely transparent to the user.

[pedroslopez]

Thanks @flash1293 !

1. Yeah, that's a known limitation of the current approach. This is tracked in airbytehq/airbyte-internal-issues#1260.
2. Refetching and updating the form sounds like a better UX! I had suggested displaying a message just as an easier approach, but if this is relatively simple as you described then we can definitely do that.

If you'd like, we can split this up and Connector Ops could implement the backend change of returning whether the config was updated as part of check_connection_for_update and your team would work on the frontend piece using this information.

[evantahler]

Grooming Notes:

• Connector Ops will make the API update first, indicating that that the config was updated during a CHECK/DISCOVER (boolean).
• Then, we will make a new ticket for @flash1293 and Connector Extensibility to consume the new API info and update the page with the new information.

So for now, this ticket remains with Connector Ops - new story for @flash1293 coming soon!

---

The technical path forward in the API is:

• The controller/endpoint-handler should do the work
• Store the config at the beginning of the method
• do the check...
• re-load the config after the check has completed and compare the new and old values

This comparison method handles cases where the config didn't save, we re-saved the same values, and other shenanigans

[evantahler]

Part 2 of this story is now here: #21041 and added to the Connector Extensibility backlog. @flash1293 can you please get this into your backlog for next sprint?

[flash1293]

@sherifnada I think the implementation part of this issue is on the connector ops team, once it's done we will take care of #21041. Could you confirm @evantahler ?

[evantahler]

@sherifnada I think the implementation part of this issue is on the connector ops team, once it's done we will take care of #21041. Could you confirm @evantahler ?

Correct!

[evantahler]

@flash1293 The API changes have been merged in, and #21041 is unblocked