[Connector Builder]Add ability to convert from YAML manifest to UI #21142

lmossman · 2023-01-07T01:39:59Z

What

Resolves #21285

Adds logic to handle converting from a YAML manifest to the connector builder UI form values.

Also updates the confirmation modal service to allow sending FormattedMessage values along with the i18n id

flash1293

A few things I noticed that are not checked (probably missing some):

Nested cartesian slicers
Whether a requestor is a custom requester
Whether an extractor is custom extractor
checkpoint interval and transformations which can be defined on a stream and would get lost
error handler can be defined on the retriever and would get lost

Not sure whether it makes sense, but would it make sense to do the following:

Define a scoped down json schema which defines only the parts we want to support - then as a first step in the conversion logic we check against this scoped down schema. This will get rid of a lot of custom checks, probably easier to maintain
For the additional props we don't support in the UI - make sure they are kept around even if they are not editable (would work e.g. for the transformations or for the requester error handler). This should work already for the spec but it should probably become a common thing.
For the inferred inputs: Your suggestion sounds good to me
For the substream slicers: Yeah, this is unfortunately a little more complex. This is what I had in mind:
- Recursively go through all streams with its substream slicers (not actually converting them yet) and store all streams in serialized form in a set (JSON.stringify(streamDeclaration)
- For the actual conversion, instead of iterating over manifest.streams, iterate over the set of collected streams (id can be the index as it's unique)
- If a substream slicer is encountered, stringify the parent stream definition again and look it up from the set - this will give you the parent stream id.

lmossman · 2023-01-11T00:36:46Z

@flash1293 I've pushed up some more commits here to handle converting the spec and auth components from YAML to UI. Could you take a look and let me know what you think?

I haven't addressed the other parts of your feedback yet for a couple reasons:

This POC is starting to drag on due to the complexity of converting from YAML to UI, and I don't want to spend too much time on implementing these changes when it feels like there is other higher-priority work
This is already starting to feel pretty complex and prone to weird behavior, and I haven't even got to the other parts where I expect even more complexity (keeping around additional props not supported in the UI, and the substream slicer issue)
It feels like maintaining all of this complex conversion logic as more changes are made to the low-code YAML will be burdensome.

What do you think? Am I overemphasizing the complexity of this?
I just feel that the value gained from this may not outweigh the complexity in making all of this work and maintaining it going forward.
But if you feel differently and we do decide to move forward with this, then we can create separate tickets for the other parts you mentioned

EDIT: After thinking about this a bit more, I think I may have been overemphasizing the complexity. Since this will allow users to go back and forth, and we'll only need to store a single source of truth in the db, it may be worth the bit of extra complexity here.

flash1293 · 2023-01-11T10:24:39Z

airbyte-webapp/src/components/connectorBuilder/types.ts

+  result.inputs = Object.entries(
+    manifestSpec.connection_specification.properties as Record<string, AirbyteJSONSchema>
+  ).flatMap(([key, definition]) => {
+    if (Object.keys(authTypeToKeyToInferredInput[result.auth.type]).includes(key)) {


looks like the keys of that object are the properties of the authenticator object, not the name of the input (in this case it's causing a mismatch between api_key and api_token). Should probably be

if (Object.values(authTypeToKeyToInferredInput[result.auth.type]) .map((input) => input.key).includes(key)) {

Otherwise you will end up with two api_keys when switching between UI and YAML

flash1293 · 2023-01-11T10:27:46Z

airbyte-webapp/src/components/connectorBuilder/types.ts

+  const inlineSchemaLoader = manifestSchemaLoader as InlineSchemaLoader;
+
+  if (inlineSchemaLoader.schema) {
+    return JSON.stringify(inlineSchemaLoader.schema);


should use nicely formatted schema, otherwise this happens:

flash1293 · 2023-01-11T10:38:04Z

What do you think? Am I overemphasizing the complexity of this?
I just feel that the value gained from this may not outweigh the complexity in making all of this work and maintaining it going forward.

I think it's worth it for two reasons:

It brings UI and YAML pretty close together, allowing user to use it a little bit if it makes sense, but not being limited by it. Having it strictly one-way means a lot of advanced users will drop it completely because it limits them too much.
We wouldn't strictly need to have this logic right now, but it's a nice canary to make it less likely for the manifest and the UI to diverge over time as it forces us to think about the conversion every time we add new functionality on either side.

But if you feel differently and we do decide to move forward with this, then we can create separate tickets for the other parts you mentioned

Definitely, we can just fail on these conditions for now and get better in conversion later - it's lossy anyway, so it's not a fundamental change.

… handle inferred input overrides properly

lmossman · 2023-01-14T02:17:50Z

@flash1293 I think I've addressed all of the feedback here except for storing unsupported fields (I'll look into that next week).

This should be working with both inputs and substream slicers now.

Also, I didn't end up needing to check for nested Cartesian slicers, because the manifest schema already doesn't allow that. Notice that Cartsian isn't listed here:

airbyte/airbyte-cdk/python/airbyte_cdk/sources/declarative/declarative_component_schema.yaml

Lines 134 to 138 in 189891c

    
           - "$ref": "#/definitions/CustomStreamSlicer" 
        
           - "$ref": "#/definitions/DatetimeStreamSlicer" 
        
           - "$ref": "#/definitions/ListStreamSlicer" 
        
           - "$ref": "#/definitions/SingleSlice" 
        
           - "$ref": "#/definitions/SubstreamSlicer"

Keeping the PR in draft because I still want to add unit tests for the conversion logic, but I think it would be useful to get another review on the implementation.

This should be mergeable even before #21211 is complete, as I've added checks for refs and $options which will catch it from trying to convert if those exist in the YAML

lmossman · 2023-01-14T02:19:34Z

airbyte-webapp/src/components/connectorBuilder/manifestToBuilderForm.ts

+} from "./types";
+import { formatJson } from "./utils";
+
+export const convertToBuilderFormValues = (


Decided to move this and its related methods into its own file, because types.ts was starting to get pretty large

Thanks for that, definitely overdue!

lmossman · 2023-01-14T08:37:37Z

airbyte-webapp/src/components/connectorBuilder/manifestToBuilderForm.ts

+    const tracebackIndex = errorMessage.indexOf(" - Traceback");
+    if (tracebackIndex >= 0) {
+      errorMessage = errorMessage.substring(0, tracebackIndex);
+    }


@lmossman TODO: remove once #21443 is merged

Looks like this is merged in

flash1293 · 2023-01-17T15:23:45Z

airbyte-webapp/src/components/connectorBuilder/manifestToBuilderForm.ts

+    id: index.toString(),
+    name: stream.name ?? "",
+    urlPath: requester.path,
+    httpMethod: (requester.http_method as "GET" | "POST" | undefined) ?? "GET",


Not broken by you, but I just realized we do not set the http_method on the requester when doing form state -> manifest conversion. This is a bug right?

Yep, good catch. I'll include the fix in this PR since it's just one line

flash1293 · 2023-01-17T16:18:36Z

airbyte-webapp/src/components/connectorBuilder/YamlEditor/YamlEditor.tsx

 import { editor } from "monaco-editor/esm/vs/editor/editor.api";
 import { useEffect, useMemo, useRef, useState } from "react";

 import { CodeEditor } from "components/ui/CodeEditor";

 import { ConnectorManifest } from "core/request/ConnectorManifest";
+// import { useConfirmationModalService } from "hooks/services/ConfirmationModal";


flash1293 · 2023-01-17T17:28:08Z

Overall this looks great! Due to the high redundancy in the configuration I don't really see users switching between UI and YAML at the moment in a super effective way, but that's something we can gradually improve over time in multiple ways. It should work just fine for the initial use case we had in mind about only storing the manifest and not the builder form values along with it.

lmossman · 2023-01-19T04:07:53Z

airbyte-webapp/src/components/connectorBuilder/manifestToBuilderForm.test.ts

+  },
+});
+
+describe("Conversion throws error when", () => {


I originally started to add tests for every case that throws a ManifestCompatibilityError, but that felt overly excessive and just means that we would be repeating all of those cases in yet another place, so I decided against it.

I think its more important that we test the more complex conversion cases anyway.

Agreed, the complex ones are the relevant bit here.

flash1293

Tested and works really well! LGTM

flash1293 · 2023-01-19T15:32:53Z

airbyte-webapp/src/components/connectorBuilder/manifestToBuilderForm.test.ts

+
+  it("manifest contains refs", () => {
+    const convert = () => {
+      // have to use a string manifest here, because refs aren't in the schema


nit: You can also use a regular object, then convince Typescript with as unknown as ConnectorManifest

flash1293 · 2023-01-19T15:34:02Z

airbyte-webapp/src/components/connectorBuilder/manifestToBuilderForm.test.ts

+  },
+});
+
+describe("Conversion throws error when", () => {


Agreed, the complex ones are the relevant bit here.

…21142) * save * save more progress * try setting values directly * toggle editor * fix primary key * enforce consistency in name and primary key * refactor conversion method to be more readable * save progress * allow custom input keys to be used for inferred auth values * fix isMatch bug and remove console logs * fix type issues with reflect * properly handle undefined * format schema and gracefully handle non-inline schemas * verify no custom components * refactor and fix request options type * rest of refactor * move manifest to builder form conversion logic into its own file, and handle inferred input overrides properly * convert substream slicers * restore warning modal for switching back to UI * remove console logs * remove unneeded traceback filtering * set http method when converting to manifest * remove commented import * add unsupported fields to builder form values * save check stream values from manifest * save progress * add more tests * save record filter in unsupported fields * use type coersion instead of yaml strings

…irbytehq#21142) * save * save more progress * try setting values directly * toggle editor * fix primary key * enforce consistency in name and primary key * refactor conversion method to be more readable * save progress * allow custom input keys to be used for inferred auth values * fix isMatch bug and remove console logs * fix type issues with reflect * properly handle undefined * format schema and gracefully handle non-inline schemas * verify no custom components * refactor and fix request options type * rest of refactor * move manifest to builder form conversion logic into its own file, and handle inferred input overrides properly * convert substream slicers * restore warning modal for switching back to UI * remove console logs * remove unneeded traceback filtering * set http method when converting to manifest * remove commented import * add unsupported fields to builder form values * save check stream values from manifest * save progress * add more tests * save record filter in unsupported fields * use type coersion instead of yaml strings

lmossman added 7 commits January 4, 2023 14:17

save

3ab9607

save more progress

6834479

try setting values directly

e2c0a44

toggle editor

546e308

merge and expand conversion logic

b4ec687

fix primary key

c208d95

enforce consistency in name and primary key

5bc6ac0

octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Jan 7, 2023

flash1293 reviewed Jan 9, 2023

View reviewed changes

lmossman added 2 commits January 9, 2023 16:54

refactor conversion method to be more readable

3578222

save progress

12bf3cb

octavia-squidington-iv removed the area/platform issues related to the platform label Jan 10, 2023

allow custom input keys to be used for inferred auth values

75e4c5b

flash1293 reviewed Jan 11, 2023

View reviewed changes

This was referenced Jan 12, 2023

[Connector Builder] POC of YAML -> UI conversion #20611

Closed

[Connector Builder] Productionize YAML -> UI conversion #21285

Closed

lmossman added 10 commits January 12, 2023 16:59

fix isMatch bug and remove console logs

27b9a57

merge with master

9707a0a

fix type issues with reflect

8111c90

properly handle undefined

2550ade

format schema and gracefully handle non-inline schemas

0e58d98

verify no custom components

a83331b

refactor and fix request options type

113751c

rest of refactor

b224f8b

move manifest to builder form conversion logic into its own file, and…

17f4112

… handle inferred input overrides properly

convert substream slicers

0f8ee5a

restore warning modal for switching back to UI

8e6ad4d

lmossman changed the title ~~Yaml to UI POC~~ [Connector Builder]Add ability to convert from YAML manifest to UI Jan 14, 2023

lmossman commented Jan 14, 2023

View reviewed changes

lmossman added 2 commits January 13, 2023 18:20

remove console logs

7a7b03b

Merge branch 'master' into lmossman/yaml-to-ui-poc

5d2d065

lmossman commented Jan 14, 2023

View reviewed changes

flash1293 reviewed Jan 17, 2023

View reviewed changes

lmossman added 8 commits January 17, 2023 17:41

Merge branch 'master' into lmossman/yaml-to-ui-poc

fe403ad

remove unneeded traceback filtering

e7f3705

set http method when converting to manifest

f3c8e1a

remove commented import

2fdc20a

add unsupported fields to builder form values

1e02232

save check stream values from manifest

3338722

save progress

cccc35b

add more tests

059d8df

lmossman marked this pull request as ready for review January 19, 2023 04:06

lmossman commented Jan 19, 2023

View reviewed changes

flash1293 approved these changes Jan 19, 2023

View reviewed changes

lmossman added 2 commits January 19, 2023 08:58

save record filter in unsupported fields

6e428ad

use type coersion instead of yaml strings

c0abbf0

lmossman mentioned this pull request Jan 19, 2023

[Connector Builder] Use ref resolving endpoint before converting YAML to UI #21610

Closed

lmossman enabled auto-merge (squash) January 19, 2023 17:56

lmossman merged commit 492a422 into master Jan 19, 2023

lmossman deleted the lmossman/yaml-to-ui-poc branch January 19, 2023 17:57

octavia-squidington-iii mentioned this pull request Jan 24, 2023

Bump Airbyte version from 0.40.28 to 0.40.29 #21767

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Connector Builder]Add ability to convert from YAML manifest to UI #21142

[Connector Builder]Add ability to convert from YAML manifest to UI #21142

lmossman commented Jan 7, 2023 •

edited

Loading

flash1293 left a comment

lmossman commented Jan 11, 2023 •

edited

Loading

flash1293 Jan 11, 2023 •

edited

Loading

flash1293 Jan 11, 2023 •

edited

Loading

flash1293 commented Jan 11, 2023

lmossman commented Jan 14, 2023 •

edited

Loading

lmossman Jan 14, 2023

flash1293 Jan 17, 2023

lmossman Jan 14, 2023 •

edited

Loading

flash1293 Jan 17, 2023

flash1293 Jan 17, 2023

lmossman Jan 18, 2023

flash1293 Jan 17, 2023

flash1293 commented Jan 17, 2023

lmossman Jan 19, 2023

flash1293 Jan 19, 2023

flash1293 left a comment

flash1293 Jan 19, 2023

flash1293 Jan 19, 2023

[Connector Builder]Add ability to convert from YAML manifest to UI #21142

[Connector Builder]Add ability to convert from YAML manifest to UI #21142

Conversation

lmossman commented Jan 7, 2023 • edited Loading

What

flash1293 left a comment

Choose a reason for hiding this comment

lmossman commented Jan 11, 2023 • edited Loading

flash1293 Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

flash1293 Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

flash1293 commented Jan 11, 2023

lmossman commented Jan 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmossman Jan 14, 2023 • edited Loading

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

flash1293 commented Jan 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmossman commented Jan 7, 2023 •

edited

Loading

lmossman commented Jan 11, 2023 •

edited

Loading

flash1293 Jan 11, 2023 •

edited

Loading

flash1293 Jan 11, 2023 •

edited

Loading

lmossman commented Jan 14, 2023 •

edited

Loading

lmossman Jan 14, 2023 •

edited

Loading