From 6a940d6ed7dc0db685492b8d6c6e25e4cb0f629a Mon Sep 17 00:00:00 2001 From: lmossman Date: Wed, 25 Jan 2023 18:23:15 -0800 Subject: [PATCH 1/2] call /manifest/resolve before converting to builder form values --- .../YamlEditor/YamlEditor.tsx | 14 +- ...st.ts => useManifestToBuilderForm.test.ts} | 156 +++++------------- ...derForm.ts => useManifestToBuilderForm.ts} | 34 ++-- .../ConnectorBuilderRequestService.ts | 6 + .../ConnectorBuilderApiService.ts | 10 ++ 5 files changed, 85 insertions(+), 135 deletions(-) rename airbyte-webapp/src/components/connectorBuilder/{manifestToBuilderForm.test.ts => useManifestToBuilderForm.test.ts} (66%) rename airbyte-webapp/src/components/connectorBuilder/{manifestToBuilderForm.ts => useManifestToBuilderForm.ts} (93%) diff --git a/airbyte-webapp/src/components/connectorBuilder/YamlEditor/YamlEditor.tsx b/airbyte-webapp/src/components/connectorBuilder/YamlEditor/YamlEditor.tsx index 92e2902af97f..85abd79db6b0 100644 --- a/airbyte-webapp/src/components/connectorBuilder/YamlEditor/YamlEditor.tsx +++ b/airbyte-webapp/src/components/connectorBuilder/YamlEditor/YamlEditor.tsx @@ -12,16 +12,13 @@ import { Action, Namespace } from "core/analytics"; import { ConnectorManifest } from "core/request/ConnectorManifest"; import { useAnalyticsService } from "hooks/services/Analytics"; import { useConfirmationModalService } from "hooks/services/ConfirmationModal"; -import { - useConnectorBuilderFormState, - useConnectorBuilderTestState, -} from "services/connectorBuilder/ConnectorBuilderStateService"; +import { useConnectorBuilderFormState } from "services/connectorBuilder/ConnectorBuilderStateService"; import styles from "./YamlEditor.module.scss"; import { UiYamlToggleButton } from "../Builder/UiYamlToggleButton"; import { DownloadYamlButton } from "../DownloadYamlButton"; -import { convertToBuilderFormValues } from "../manifestToBuilderForm"; import { convertToManifest } from "../types"; +import { useManifestToBuilderForm } from "../useManifestToBuilderForm"; interface YamlEditorProps { toggleYamlEditor: () => void; @@ -41,8 +38,8 @@ export const YamlEditor: React.FC = ({ toggleYamlEditor }) => { setYamlIsValid, setJsonManifest, } = useConnectorBuilderFormState(); - const { streamListErrorMessage } = useConnectorBuilderTestState(); const [yamlValue, setYamlValue] = useState(yamlManifest); + const { convertToBuilderFormValues } = useManifestToBuilderForm(); // debounce the setJsonManifest calls so that it doesnt result in a network call for every keystroke const debouncedSetJsonManifest = useMemo(() => debounce(setJsonManifest, 200), [setJsonManifest]); @@ -90,10 +87,11 @@ export const YamlEditor: React.FC = ({ toggleYamlEditor }) => { return !isEqual(convertToManifest(builderFormValues), jsonManifest); }, [jsonManifest, builderFormValues]); - const handleToggleYamlEditor = () => { + const handleToggleYamlEditor = async () => { if (yamlIsDirty) { try { - setValues(convertToBuilderFormValues(jsonManifest, builderFormValues, streamListErrorMessage)); + const convertedFormValues = await convertToBuilderFormValues(jsonManifest, builderFormValues); + setValues(convertedFormValues); toggleYamlEditor(); } catch (e) { openConfirmationModal({ diff --git a/airbyte-webapp/src/components/connectorBuilder/manifestToBuilderForm.test.ts b/airbyte-webapp/src/components/connectorBuilder/useManifestToBuilderForm.test.ts similarity index 66% rename from airbyte-webapp/src/components/connectorBuilder/manifestToBuilderForm.test.ts rename to airbyte-webapp/src/components/connectorBuilder/useManifestToBuilderForm.test.ts index a7c5c854238f..8f63ec28efe3 100644 --- a/airbyte-webapp/src/components/connectorBuilder/manifestToBuilderForm.test.ts +++ b/airbyte-webapp/src/components/connectorBuilder/useManifestToBuilderForm.test.ts @@ -1,14 +1,9 @@ import merge from "lodash/merge"; -import { - ConnectorManifest, - DeclarativeStream, - DeclarativeStreamRetriever, - HttpRequester, -} from "core/request/ConnectorManifest"; +import { ConnectorManifest, DeclarativeStream } from "core/request/ConnectorManifest"; -import { convertToBuilderFormValues } from "./manifestToBuilderForm"; import { DEFAULT_BUILDER_FORM_VALUES } from "./types"; +import { convertToBuilderFormValues } from "./useManifestToBuilderForm"; const baseManifest: ConnectorManifest = { type: "DeclarativeSource", @@ -53,89 +48,24 @@ const stream2: DeclarativeStream = merge({}, stream1, { }, }); -describe("Conversion throws error when", () => { - it("streamListErrorMessage is present", () => { - const streamListErrorMessage = "Some error message"; - const convert = () => { - convertToBuilderFormValues({} as ConnectorManifest, DEFAULT_BUILDER_FORM_VALUES, streamListErrorMessage); - }; - expect(convert).toThrow(streamListErrorMessage); - }); +const noOpResolve = (manifest: ConnectorManifest) => { + return Promise.resolve({ manifest }); +}; - it("manifest contains refs", () => { - const convert = () => { - const manifest: ConnectorManifest = { - ...baseManifest, - definitions: { - retriever: { - name: "pokemon", - primary_key: "id", - requester: { - name: "pokemon", - path: "/pokemon/{{config['pokemon_name']}}", - url_base: "https://pokeapi.co/api/v2", - http_method: "GET", - }, - record_selector: { - extractor: { - type: "DpathExtractor", - field_pointer: [], - }, - }, - }, - }, - streams: [ - { - type: "DeclarativeStream", - name: "pokemon", - retriever: { - $ref: "*ref(definitions.retriever)", - } as unknown as DeclarativeStreamRetriever, - }, - ], - }; - convertToBuilderFormValues(manifest, DEFAULT_BUILDER_FORM_VALUES); +describe("Conversion throws error when", () => { + it("resolve throws error", async () => { + const errorMessage = "Some resolve error message"; + const resolve = (_manifest: ConnectorManifest) => { + throw new Error(errorMessage); }; - expect(convert).toThrow("contains refs"); - }); - - it("manifest contains options", () => { - const convert = () => { - const manifest: ConnectorManifest = { - ...baseManifest, - streams: [ - { - type: "DeclarativeStream", - $options: { - name: "pokemon", - primary_key: "id", - path: "/pokemon/{{config['pokemon_name']}}", - }, - retriever: { - type: "SimpleRetriever", - requester: { - type: "HttpRequester", - url_base: "https://pokeapi.co/api/v2", - http_method: "GET", - } as unknown as HttpRequester, - record_selector: { - type: "RecordSelector", - extractor: { - type: "DpathExtractor", - field_pointer: [], - }, - }, - }, - }, - ], - }; - convertToBuilderFormValues(manifest, DEFAULT_BUILDER_FORM_VALUES); + const convert = async () => { + return convertToBuilderFormValues(resolve, {} as ConnectorManifest, DEFAULT_BUILDER_FORM_VALUES); }; - expect(convert).toThrow("contains refs"); + await expect(convert).rejects.toThrow(errorMessage); }); - it("manifests has incorrect retriever type", () => { - const convert = () => { + it("manifests has incorrect retriever type", async () => { + const convert = async () => { const manifest: ConnectorManifest = { ...baseManifest, streams: [ @@ -149,13 +79,13 @@ describe("Conversion throws error when", () => { }, ], }; - convertToBuilderFormValues(manifest, DEFAULT_BUILDER_FORM_VALUES); + return convertToBuilderFormValues(noOpResolve, manifest, DEFAULT_BUILDER_FORM_VALUES); }; - expect(convert).toThrow("doesn't use a SimpleRetriever"); + await expect(convert).rejects.toThrow("doesn't use a SimpleRetriever"); }); - it("manifest has incorrect requester type", () => { - const convert = () => { + it("manifest has incorrect requester type", async () => { + const convert = async () => { const manifest: ConnectorManifest = { ...baseManifest, streams: [ @@ -169,13 +99,13 @@ describe("Conversion throws error when", () => { }), ], }; - convertToBuilderFormValues(manifest, DEFAULT_BUILDER_FORM_VALUES); + return convertToBuilderFormValues(noOpResolve, manifest, DEFAULT_BUILDER_FORM_VALUES); }; - expect(convert).toThrow("doesn't use a HttpRequester"); + await expect(convert).rejects.toThrow("doesn't use a HttpRequester"); }); - it("manifest has an authenticator with a non-interpolated secret key", () => { - const convert = () => { + it("manifest has an authenticator with a non-interpolated secret key", async () => { + const convert = async () => { const manifest: ConnectorManifest = { ...baseManifest, streams: [ @@ -192,19 +122,19 @@ describe("Conversion throws error when", () => { }), ], }; - convertToBuilderFormValues(manifest, DEFAULT_BUILDER_FORM_VALUES); + return convertToBuilderFormValues(noOpResolve, manifest, DEFAULT_BUILDER_FORM_VALUES); }; - expect(convert).toThrow("api_token value must be of the form {{ config["); + await expect(convert).rejects.toThrow("api_token value must be of the form {{ config["); }); }); describe("Conversion successfully results in", () => { - it("default values if manifest is empty", () => { - const formValues = convertToBuilderFormValues(baseManifest, DEFAULT_BUILDER_FORM_VALUES); + it("default values if manifest is empty", async () => { + const formValues = await convertToBuilderFormValues(noOpResolve, baseManifest, DEFAULT_BUILDER_FORM_VALUES); expect(formValues).toEqual(DEFAULT_BUILDER_FORM_VALUES); }); - it("spec properties converted to inputs if no streams present", () => { + it("spec properties converted to inputs if no streams present", async () => { const manifest: ConnectorManifest = { ...baseManifest, spec: { @@ -223,7 +153,7 @@ describe("Conversion successfully results in", () => { }, }, }; - const formValues = convertToBuilderFormValues(manifest, DEFAULT_BUILDER_FORM_VALUES); + const formValues = await convertToBuilderFormValues(noOpResolve, manifest, DEFAULT_BUILDER_FORM_VALUES); expect(formValues.inferredInputOverrides).toEqual({}); expect(formValues.inputs).toEqual([ { @@ -234,7 +164,7 @@ describe("Conversion successfully results in", () => { ]); }); - it("spec properties converted to input overrides on matching auth keys", () => { + it("spec properties converted to input overrides on matching auth keys", async () => { const manifest: ConnectorManifest = { ...baseManifest, streams: [ @@ -270,7 +200,7 @@ describe("Conversion successfully results in", () => { }, }, }; - const formValues = convertToBuilderFormValues(manifest, DEFAULT_BUILDER_FORM_VALUES); + const formValues = await convertToBuilderFormValues(noOpResolve, manifest, DEFAULT_BUILDER_FORM_VALUES); expect(formValues.inputs).toEqual([ { key: "numeric_key", @@ -283,7 +213,7 @@ describe("Conversion successfully results in", () => { }); }); - it("request options converted to key-value list", () => { + it("request options converted to key-value list", async () => { const manifest: ConnectorManifest = { ...baseManifest, streams: [ @@ -302,14 +232,14 @@ describe("Conversion successfully results in", () => { }), ], }; - const formValues = convertToBuilderFormValues(manifest, DEFAULT_BUILDER_FORM_VALUES); + const formValues = await convertToBuilderFormValues(noOpResolve, manifest, DEFAULT_BUILDER_FORM_VALUES); expect(formValues.streams[0].requestOptions.requestParameters).toEqual([ ["k1", "v1"], ["k2", "v2"], ]); }); - it("primary key string converted to array", () => { + it("primary key string converted to array", async () => { const manifest: ConnectorManifest = { ...baseManifest, streams: [ @@ -321,11 +251,11 @@ describe("Conversion successfully results in", () => { }), ], }; - const formValues = convertToBuilderFormValues(manifest, DEFAULT_BUILDER_FORM_VALUES); + const formValues = await convertToBuilderFormValues(noOpResolve, manifest, DEFAULT_BUILDER_FORM_VALUES); expect(formValues.streams[0].primaryKey).toEqual(["id"]); }); - it("cartesian product stream slicer converted to builder cartesian product slicer", () => { + it("cartesian product stream slicer converted to builder cartesian product slicer", async () => { const manifest: ConnectorManifest = { ...baseManifest, streams: [ @@ -354,11 +284,11 @@ describe("Conversion successfully results in", () => { }), ], }; - const formValues = convertToBuilderFormValues(manifest, DEFAULT_BUILDER_FORM_VALUES); + const formValues = await convertToBuilderFormValues(noOpResolve, manifest, DEFAULT_BUILDER_FORM_VALUES); expect(formValues.streams[0].streamSlicer).toEqual(manifest.streams[0].retriever.stream_slicer); }); - it("substream stream slicer converted to builder substream slicer", () => { + it("substream stream slicer converted to builder substream slicer", async () => { const manifest: ConnectorManifest = { ...baseManifest, streams: [ @@ -380,7 +310,7 @@ describe("Conversion successfully results in", () => { }), ], }; - const formValues = convertToBuilderFormValues(manifest, DEFAULT_BUILDER_FORM_VALUES); + const formValues = await convertToBuilderFormValues(noOpResolve, manifest, DEFAULT_BUILDER_FORM_VALUES); expect(formValues.streams[1].streamSlicer).toEqual({ type: "SubstreamSlicer", parent_key: "key", @@ -389,7 +319,7 @@ describe("Conversion successfully results in", () => { }); }); - it("schema loader converted to schema", () => { + it("schema loader converted to schema", async () => { const manifest: ConnectorManifest = { ...baseManifest, streams: [ @@ -403,7 +333,7 @@ describe("Conversion successfully results in", () => { }), ], }; - const formValues = convertToBuilderFormValues(manifest, DEFAULT_BUILDER_FORM_VALUES); + const formValues = await convertToBuilderFormValues(noOpResolve, manifest, DEFAULT_BUILDER_FORM_VALUES); expect(formValues.streams[0].schema).toEqual( `{ "key": "value" @@ -411,7 +341,7 @@ describe("Conversion successfully results in", () => { ); }); - it("stores unsupported fields", () => { + it("stores unsupported fields", async () => { const manifest: ConnectorManifest = { ...baseManifest, streams: [ @@ -440,7 +370,7 @@ describe("Conversion successfully results in", () => { }), ], }; - const formValues = convertToBuilderFormValues(manifest, DEFAULT_BUILDER_FORM_VALUES); + const formValues = await convertToBuilderFormValues(noOpResolve, manifest, DEFAULT_BUILDER_FORM_VALUES); expect(formValues.streams[0].unsupportedFields).toEqual({ transformations: manifest.streams[0].transformations, checkpoint_interval: manifest.streams[0].checkpoint_interval, diff --git a/airbyte-webapp/src/components/connectorBuilder/manifestToBuilderForm.ts b/airbyte-webapp/src/components/connectorBuilder/useManifestToBuilderForm.ts similarity index 93% rename from airbyte-webapp/src/components/connectorBuilder/manifestToBuilderForm.ts rename to airbyte-webapp/src/components/connectorBuilder/useManifestToBuilderForm.ts index 3c8d5d8b487a..eeaf1d76c4b3 100644 --- a/airbyte-webapp/src/components/connectorBuilder/manifestToBuilderForm.ts +++ b/airbyte-webapp/src/components/connectorBuilder/useManifestToBuilderForm.ts @@ -1,6 +1,7 @@ import isEqual from "lodash/isEqual"; import { AirbyteJSONSchema } from "core/jsonSchema/types"; +import { ResolveManifest } from "core/request/ConnectorBuilderClient"; import { CartesianProductStreamSlicer, ConnectorManifest, @@ -19,6 +20,7 @@ import { Spec, SubstreamSlicer, } from "core/request/ConnectorManifest"; +import { useResolveManifest } from "services/connectorBuilder/ConnectorBuilderApiService"; import { authTypeToKeyToInferredInput, @@ -33,14 +35,21 @@ import { } from "./types"; import { formatJson } from "./utils"; -export const convertToBuilderFormValues = ( +export const useManifestToBuilderForm = () => { + const { resolve } = useResolveManifest(); + return { convertToBuilderFormValues: convertToBuilderFormValues.bind(this, resolve) }; +}; + +export const convertToBuilderFormValues = async ( + resolve: (manifest: ConnectorManifest) => Promise, manifest: ConnectorManifest, - currentBuilderFormValues: BuilderFormValues, - streamListErrorMessage?: string + currentBuilderFormValues: BuilderFormValues ) => { - // TODO: replace these checks with a call to the soon-to-be /manifest/resolve endpoint, to resolve refs, options, and validate the manifest against the schema - if (streamListErrorMessage) { - let errorMessage = streamListErrorMessage; + let resolveResult: ResolveManifest; + try { + resolveResult = await resolve(manifest); + } catch (e) { + let errorMessage = e.message; if (errorMessage[0] === '"') { errorMessage = errorMessage.substring(1, errorMessage.length); } @@ -49,18 +58,15 @@ export const convertToBuilderFormValues = ( } throw new ManifestCompatibilityError(undefined, errorMessage.trim()); } - const manifestString = JSON.stringify(manifest); - if (manifestString.includes("*ref") || manifestString.includes("$ref") || manifestString.includes("$options")) { - throw new ManifestCompatibilityError(undefined, "Manifest contains refs or $options, which are unsupported"); - } + const resolvedManifest = resolveResult.manifest as ConnectorManifest; const builderFormValues = DEFAULT_BUILDER_FORM_VALUES; builderFormValues.global.connectorName = currentBuilderFormValues.global.connectorName; - builderFormValues.checkStreams = manifest.check.stream_names; + builderFormValues.checkStreams = resolvedManifest.check.stream_names; - const streams = manifest.streams; + const streams = resolvedManifest.streams; if (streams === undefined || streams.length === 0) { - const { inputs, inferredInputOverrides } = manifestSpecAndAuthToBuilder(manifest.spec, undefined); + const { inputs, inferredInputOverrides } = manifestSpecAndAuthToBuilder(resolvedManifest.spec, undefined); builderFormValues.inputs = inputs; builderFormValues.inferredInputOverrides = inferredInputOverrides; @@ -72,7 +78,7 @@ export const convertToBuilderFormValues = ( builderFormValues.global.urlBase = streams[0].retriever.requester.url_base; const { inputs, inferredInputOverrides, auth } = manifestSpecAndAuthToBuilder( - manifest.spec, + resolvedManifest.spec, streams[0].retriever.requester.authenticator ); builderFormValues.inputs = inputs; diff --git a/airbyte-webapp/src/core/domain/connectorBuilder/ConnectorBuilderRequestService.ts b/airbyte-webapp/src/core/domain/connectorBuilder/ConnectorBuilderRequestService.ts index 9e72857f8bfc..3d8e7d0059fd 100644 --- a/airbyte-webapp/src/core/domain/connectorBuilder/ConnectorBuilderRequestService.ts +++ b/airbyte-webapp/src/core/domain/connectorBuilder/ConnectorBuilderRequestService.ts @@ -2,6 +2,8 @@ import { getManifestTemplate, listStreams, readStream, + resolveManifest, + ResolveManifestRequestBody, StreamRead, StreamReadRequestBody, StreamsListRead, @@ -21,4 +23,8 @@ export class ConnectorBuilderRequestService extends AirbyteRequestService { public getManifestTemplate(): Promise { return getManifestTemplate(this.requestOptions); } + + public resolveManifest(resolveParams: ResolveManifestRequestBody) { + return resolveManifest(resolveParams, this.requestOptions); + } } diff --git a/airbyte-webapp/src/services/connectorBuilder/ConnectorBuilderApiService.ts b/airbyte-webapp/src/services/connectorBuilder/ConnectorBuilderApiService.ts index 2f2674c1c3f4..8f90a4f7e9fd 100644 --- a/airbyte-webapp/src/services/connectorBuilder/ConnectorBuilderApiService.ts +++ b/airbyte-webapp/src/services/connectorBuilder/ConnectorBuilderApiService.ts @@ -3,11 +3,13 @@ import { useQuery } from "react-query"; import { useConfig } from "config"; import { ConnectorBuilderRequestService } from "core/domain/connectorBuilder/ConnectorBuilderRequestService"; import { + ResolveManifestRequestBodyManifest, StreamReadRequestBody, StreamsListRequestBody, StreamsListRequestBodyConfig, StreamsListRequestBodyManifest, } from "core/request/ConnectorBuilderClient"; +import { ConnectorManifest } from "core/request/ConnectorManifest"; import { useSuspenseQuery } from "services/connector/useSuspenseQuery"; import { useInitService } from "services/useInitService"; @@ -17,6 +19,8 @@ const connectorBuilderKeys = { list: (manifest: StreamsListRequestBodyManifest, config: StreamsListRequestBodyConfig) => [...connectorBuilderKeys.all, "list", { manifest, config }] as const, template: ["template"] as const, + resolve: (manifest: ResolveManifestRequestBodyManifest) => + [...connectorBuilderKeys.all, "resolve", { manifest }] as const, }; function useConnectorBuilderService() { @@ -51,3 +55,9 @@ export const useManifestTemplate = () => { return useSuspenseQuery(connectorBuilderKeys.template, () => service.getManifestTemplate()); }; + +export const useResolveManifest = () => { + const service = useConnectorBuilderService(); + + return { resolve: (manifest: ConnectorManifest) => service.resolveManifest({ manifest }) }; +}; From d579c3447d6cde7bc7ac3362387baaf7f56cb521 Mon Sep 17 00:00:00 2001 From: lmossman Date: Thu, 26 Jan 2023 16:59:05 -0800 Subject: [PATCH 2/2] fix test --- .../connectorBuilder/useManifestToBuilderForm.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/airbyte-webapp/src/components/connectorBuilder/useManifestToBuilderForm.test.ts b/airbyte-webapp/src/components/connectorBuilder/useManifestToBuilderForm.test.ts index 8ef9bf00642b..be685f15a295 100644 --- a/airbyte-webapp/src/components/connectorBuilder/useManifestToBuilderForm.test.ts +++ b/airbyte-webapp/src/components/connectorBuilder/useManifestToBuilderForm.test.ts @@ -127,7 +127,7 @@ describe("Conversion throws error when", () => { await expect(convert).rejects.toThrow("api_token value must be of the form {{ config["); }); - it("manifest has an OAuthAuthenticator with a refresh_request_body containing non-string values", () => { + it("manifest has an OAuthAuthenticator with a refresh_request_body containing non-string values", async () => { const convert = () => { const manifest: ConnectorManifest = { ...baseManifest, @@ -155,9 +155,9 @@ describe("Conversion throws error when", () => { }), ], }; - convertToBuilderFormValues(manifest, DEFAULT_BUILDER_FORM_VALUES); + return convertToBuilderFormValues(noOpResolve, manifest, DEFAULT_BUILDER_FORM_VALUES); }; - expect(convert).toThrow("OAuthAuthenticator contains a refresh_request_body with non-string values"); + await expect(convert).rejects.toThrow("OAuthAuthenticator contains a refresh_request_body with non-string values"); }); }); @@ -418,7 +418,7 @@ describe("Conversion successfully results in", () => { }); }); - it("OAuth authenticator refresh_request_body converted to array", () => { + it("OAuth authenticator refresh_request_body converted to array", async () => { const manifest: ConnectorManifest = { ...baseManifest, streams: [ @@ -442,7 +442,7 @@ describe("Conversion successfully results in", () => { }), ], }; - const formValues = convertToBuilderFormValues(manifest, DEFAULT_BUILDER_FORM_VALUES); + const formValues = await convertToBuilderFormValues(noOpResolve, manifest, DEFAULT_BUILDER_FORM_VALUES); expect(formValues.global.authenticator).toEqual({ type: "OAuthAuthenticator", client_id: "{{ config['client_id'] }}",