🪟🚨 Refactor connector form code

[flash1293]

Closes #14250
Closes #15108

What

This PR takes care of the rest of the connector form tech debt parts:

• Remove uiWidgetsInfo state by finding the enum/const field defining the selected oneOf and adding it to the condition form block to check the selected value by looking at the formik state
• Only do default value calculation once before rendering the formik component the first time (remove PatchInitialValuesWithWidgetConfig hack)
• Build yup schema once by flattening the oneOf conditions into a single object and adding when conditions on the found condition key (remove RevalidateOnValidationSchemaChange hack)

How

Selected conditions of a oneOf are not tracked via uiWidgetsInfo state anymore. Instead, the FormBlock data structure contains a new property "selectionPath" which is pointing to the shared const prop of all conditions. As this is part of the formik values, it can be looked up from there directly. A second new property "selectionConstValues" contains an array of possible values for the const field in the same order as the condition form blocks itself - checking which one is selected becomes a selectionConstValues.indexOf(formikState.get(selectionPath)).

The yup schema used for actual validation is not rebuilt based on the uiWidgetsInfo state anymore. Instead, the keys out of all conditions are flattened into a single yup object shape, with when conditions applying the correct sub-schema based on the value of the "selectionPath". If the selected condition does not specify a certain key at all, its schema is set to "strip" which removes it on casting.

Setting default values doesn't happen within a hack-component anymore but is part of the useBuildForm hook now which is also generating the form blocks data structure.

In a few places from within the new logic, errors are thrown if the rules of airbyte schema are violated. For these, there is a special error type which is caught by the error boundary and rendered with a nice message and a link to the documentation. This should not happen for any of the Airbyte-maintained connectors but might be a case for custom ones.

The uiWidgets terminology was used in inconsistent ways - this PR removes it all together - now this is the flow that's happening when rendering the form for the first time:

• json schema -> form blocks and initial default values
• json schema + form blocks -> yup schema

On update, only the formik values are updated.

As the initial form values on empty forms passed down from the connector card were re-built on every render, the init logic outlined above would sometimes happen multiple times (depending on how often react-query is re-rendering the component). By memoizing these initial values on the connector card level, it's only happening a single time.

🚨 User Impact 🚨

The snowflake destination in version <=0.4.40 does not work together with the changes on this PR - existing connections will continue to work fine, but it's not possible to change the configuration. Please update the snowflake destination connector to 0.4.41.

oneOf properties not following the rules described in the documentation will stop working in the UI - the form will crash and a meaningful error is shown which also links to the documentation:

[flash1293]

Thanks for the review, addressed all your points.

[lmossman]

Had a few more comments, main ones being about how the tests look like they may be incorrect

I also tested this locally with a few sources (postgres, linkedin ads, etc) and the behavior looked correct to me!

[lmossman]

One more small note - we should probably put these error messages behind i18n right? (which also serves the dual purpose of not repeating this same string on line 45 and line 49)

[lmossman]

This doesn't make sense to me. Wouldn't we expect the api_key schema to be something like

api_key: yup
  .mixed()
  .when("type", { is: "api", then: (x) => x.string().matches(\\w{5}), otherwise: (x) => x })
  .when("type", { is: (val) => !["api"].includes(val), then: (x) => x.strip() })

same comment goes for the redirect_uri schema below, and the schemas at the end of this file.

And what is maybe more concerning is that if these tests are actually incorrectly implemented, why didn't they fail?

[flash1293]

This test is only testing the structure of the yup schema which does not capture the actual conditions of the whens:

 "api_key": {
              "type": "mixed",
              "_whitelist": {
                "list": {},
                "refs": {}
              },
              "_blacklist": {
                "list": {},
                "refs": {}
              },
              "exclusiveTests": {},
              "deps": [
                "type",
                "type"
              ],
              "conditions": [
                {
                  "refs": [
                    {
                      "key": "type",
                      "isContext": false,
                      "isValue": false,
                      "isSibling": true,
                      "path": "type"
                    }
                  ]
                },
                {
                  "refs": [
                    {
                      "key": "type",
                      "isContext": false,
                      "isValue": false,
                      "isSibling": true,
                      "path": "type"
                    }
                  ]
                }
              ],

The tests below this one (the stuff in should schema build conditional case that validates correctly based on selection key) test whether the whens actually kick in.

Added a comment to explain better.

[lmossman]

This test implementation doesn't seem to line up with this description to me. I'm not sure what topKey and topKey.subKey are doing on lines 250-251. It seems like this should be nesting credentials under a few levels or something?

[flash1293]

I copied this test over and didn't adjust much about it. It is testing whether conditionals also work in nested cases, but I don't see how we need that test as the base schema is also nesting the oneOf in an object...

Removed the test as it's more confusing than helpful.

[lmossman]

The examples are super helpful, thanks!

[lmossman]

Would it work to make the type of FormConditionItem.selectionConstValues be something like Array<JSONSchema7Type | undefined> then?

Just asking because it feels like we should try to avoid unknown where we can and this feels like a place where we may be able to eliminate it

[flash1293]

I clicked through all the sources and all the destinations and found one that doesn't work - the Snowflake destination.
It does not specify the "auth_type" property for the simple password case:

[
    {
        "type": "object",
        "order": 0,
        "title": "OAuth2.0",
        "required": [
            "access_token",
            "refresh_token"
        ],
        "properties": {
            "auth_type": {
                "enum": [
                    "OAuth2.0"
                ],
                "type": "string",
                "const": "OAuth2.0",
                "order": 0,
                "default": "OAuth2.0"
            },
            "client_id": {
                "type": "string",
                "title": "Client ID",
                "description": "Enter your application's Client ID",
                "airbyte_secret": true
            },
            "access_token": {
                "type": "string",
                "title": "Access Token",
                "description": "Enter you application's Access Token",
                "airbyte_secret": true
            },
            "client_secret": {
                "type": "string",
                "title": "Client Secret",
                "description": "Enter your application's Client secret",
                "airbyte_secret": true
            },
            "refresh_token": {
                "type": "string",
                "title": "Refresh Token",
                "description": "Enter your application's Refresh Token",
                "airbyte_secret": true
            }
        }
    },
    {
        "type": "object",
        "order": 1,
        "title": "Key Pair Authentication",
        "required": [
            "private_key"
        ],
        "properties": {
            "auth_type": {
                "enum": [
                    "Key Pair Authentication"
                ],
                "type": "string",
                "const": "Key Pair Authentication",
                "order": 0,
                "default": "Key Pair Authentication"
            },
            "private_key": {
                "type": "string",
                "title": "Private Key",
                "multiline": true,
                "description": "RSA Private key to use for Snowflake connection. See the <a href=\"https://docs.airbyte.com/integrations/destinations/snowflake\">docs</a> for more information on how to obtain this key.",
                "airbyte_secret": true
            },
            "private_key_password": {
                "type": "string",
                "title": "Passphrase",
                "description": "Passphrase for private key",
                "airbyte_secret": true
            }
        }
    },
    {
        "type": "object",
        "order": 2,
        "title": "Username and Password",
        "required": [
            "password"
        ],
        "properties": {
            "password": {
                "type": "string",
                "order": 1,
                "title": "Password",
                "description": "Enter the password associated with the username.",
                "airbyte_secret": true
            }
        }
    }
]

Here is my plan for how to fix this:

• Update the connector spec to include the auth_type property for the password case as well before merging this PR: Fix snowflake destination spec #20566
• Handle this situation better in the frontend - if the actual value of the selection property can't be found in the list of allowed values, render the form group in unselected state and make sure the object in this property is not changed if the user does not interact with it. So if there is an existing connection of snowflake destination and it uses the following credentials

{ "credentials": { "password": "abcd" } }

Then the form will render like this:

if the user changes something else and saves, it will retain the current password. If they choose "Username and password" from the list, it will also retain the existing password (and add the const key):

I think the changes I made to the webapp make sense in general because they make sure to not break on weird configurations which is something that could always happen in practice.

[lmossman]

Just had one comment about improving error tracking a bit, but I don't think it is blocking.

I think the approach you laid out for dealing with the Snowflake destination issue makes sense, though it does raise one concern:

If OSS users upgrade their airbyte version to pull in these webapp changes, but they don't update their snowflake destination connector version to the one with your fix, then they will see the Spec uses oneOf without a shared const property error message if they try to create or edit any snowflake connector.

This may not be a huge concern as they would just need to update that connector in order to fix it -- we just need to be sure to communicate that clearly in the release notes. But is it worth exploring a way for us to handle that case without completely failing the webapp?

[lmossman]

It would be nice if this could include a second argument to add some metadata about these errors, so they are easier to discover in datadog. I'm thinking of something like how errors are tracked in the useAuthenticator hook:

airbyte/airbyte-webapp/src/views/Connector/ConnectorForm/useAuthentication.tsx

Lines 156 to 159 in 2392acb

	trackError(e, {
	id: "useAuthentication.getValues",
	connector: connectorSpec ? ConnectorSpecification.id(connectorSpec) : null,
	});

If this could include an id like "formBuildError", and also attach the connector ID to it, then we could see which connector the error occurred for. This seems important because I don't know how we would address a generic form build error that we see in datadog unless we know what connector it occurred for.

[flash1293]

If OSS users upgrade their airbyte version to pull in these webapp changes, but they don't update their snowflake destination connector version to the one with your fix, then they will see the Spec uses oneOf without a shared const property error message if they try to create or edit any snowflake connector

agreed, this needs to be made obvious. We could add something like “Upgrade your connector to the latest version” or so to the error message

But is it worth exploring a way for us to handle that case without completely failing the webapp?

I honestly can’t think of a good way to do so without building a whole separate system of tracking the state which kind of goes counter the idea of the refactoring. Do you have an idea?

[lmossman]

@flash1293 no I think I agree that it would just add more complexity to track the state in a way that we don't even want to support, so I don't think it makes sense to try to do that. I think your idea of adding something like

Please ensure your connector is updated to the latest version

to the error message is a good approach for making that clear, so let's go with that

[flash1293]

to the error message is a good approach for making that clear, so let's go with that

done

It would be nice if this could include a second argument to add some metadata about these errors, so they are easier to discover in datadog

done, also refactored the hooks a bit as it made things easier - now the connector form only knows a single "useBuildForm" hook which does all the things.

[lmossman]

Just had one more small suggestion, otherwise this LGTM and I didnt see any unexpected behavior during testing.

Reminder: this should not be merged until #20566 is published and merged

[lmossman]

Suggested change

	connector: error.connectorId,
	connectorDefinitionId: error.connectorId,

nit suggestion to make it clear that this is a connector definition ID (since connector ID usually refers to the ID of an instantiated source or destination record).

Could you also update the field on the FormBuildError class to be connectorDefinitionId instead of connectorId?