Skip to content

Commit

Permalink
🪟🐛 [Connector Builder] Fix manifest conversion bugs (#21828)
Browse files Browse the repository at this point in the history
* add check for undefined auth type

* fix authenticator equality check and add test

* don't allow nested objects/arrays in refresh_request_body and add test

* explicitly do not use refs in json -> yaml dump

* ensure the CheckStream configuration always has valid stream names

* throw on any non-string value
  • Loading branch information
lmossman authored Jan 26, 2023
1 parent 3981c57 commit ad8a632
Show file tree
Hide file tree
Showing 4 changed files with 110 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,39 @@ describe("Conversion throws error when", () => {
};
expect(convert).toThrow("api_token value must be of the form {{ config[");
});

it("manifest has an OAuthAuthenticator with a refresh_request_body containing non-string values", () => {
const convert = () => {
const manifest: ConnectorManifest = {
...baseManifest,
streams: [
merge({}, stream1, {
retriever: {
requester: {
authenticator: {
type: "OAuthAuthenticator",
client_id: "{{ config['client_id'] }}",
client_secret: "{{ config['client_secret'] }}",
refresh_token: "{{ config['client_refresh_token'] }}",
refresh_request_body: {
key1: "val1",
key2: {
a: 1,
b: 2,
},
},
token_refresh_endpoint: "https://api.com/refresh_token",
grant_type: "client_credentials",
},
},
},
}),
],
};
convertToBuilderFormValues(manifest, DEFAULT_BUILDER_FORM_VALUES);
};
expect(convert).toThrow("OAuthAuthenticator contains a refresh_request_body with non-string values");
});
});

describe("Conversion successfully results in", () => {
Expand Down Expand Up @@ -454,4 +487,43 @@ describe("Conversion successfully results in", () => {
},
});
});

it("OAuth authenticator refresh_request_body converted to array", () => {
const manifest: ConnectorManifest = {
...baseManifest,
streams: [
merge({}, stream1, {
retriever: {
requester: {
authenticator: {
type: "OAuthAuthenticator",
client_id: "{{ config['client_id'] }}",
client_secret: "{{ config['client_secret'] }}",
refresh_token: "{{ config['client_refresh_token'] }}",
refresh_request_body: {
key1: "val1",
key2: "val2",
},
token_refresh_endpoint: "https://api.com/refresh_token",
grant_type: "client_credentials",
},
},
},
}),
],
};
const formValues = convertToBuilderFormValues(manifest, DEFAULT_BUILDER_FORM_VALUES);
expect(formValues.global.authenticator).toEqual({
type: "OAuthAuthenticator",
client_id: "{{ config['client_id'] }}",
client_secret: "{{ config['client_secret'] }}",
refresh_token: "{{ config['client_refresh_token'] }}",
refresh_request_body: [
["key1", "val1"],
["key2", "val2"],
],
token_refresh_endpoint: "https://api.com/refresh_token",
grant_type: "client_credentials",
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,13 @@ export const convertToBuilderFormValues = (

const serializedStreamToIndex = Object.fromEntries(streams.map((stream, index) => [JSON.stringify(stream), index]));
builderFormValues.streams = streams.map((stream, index) =>
manifestStreamToBuilder(stream, index, serializedStreamToIndex, builderFormValues.global)
manifestStreamToBuilder(
stream,
index,
serializedStreamToIndex,
streams[0].retriever.requester.url_base,
streams[0].retriever.requester.authenticator
)
);

return builderFormValues;
Expand All @@ -91,7 +97,8 @@ const manifestStreamToBuilder = (
stream: DeclarativeStream,
index: number,
serializedStreamToIndex: Record<string, number>,
builderFormGlobal: BuilderFormValues["global"]
firstStreamUrlBase: string,
firstStreamAuthenticator?: HttpRequesterAuthenticator
): BuilderStream => {
assertType<SimpleRetriever>(stream.retriever, "SimpleRetriever", stream.name);
const retriever = stream.retriever;
Expand All @@ -100,14 +107,14 @@ const manifestStreamToBuilder = (
const requester = retriever.requester;

if (
builderFormGlobal.authenticator.type === "NoAuth"
!firstStreamAuthenticator || firstStreamAuthenticator.type === "NoAuth"
? requester.authenticator && requester.authenticator.type !== "NoAuth"
: !isEqual(retriever.requester.authenticator, builderFormGlobal.authenticator)
: !isEqual(retriever.requester.authenticator, firstStreamAuthenticator)
) {
throw new ManifestCompatibilityError(stream.name, "authenticator does not match the first stream's");
}

if (retriever.requester.url_base !== builderFormGlobal.urlBase) {
if (retriever.requester.url_base !== firstStreamUrlBase) {
throw new ManifestCompatibilityError(stream.name, "url_base does not match the first stream's");
}

Expand Down Expand Up @@ -150,7 +157,7 @@ const manifestStreamToBuilder = (
),
},
primaryKey: manifestPrimaryKeyToBuilder(stream),
paginator: manifestPaginatorToBuilder(retriever.paginator, stream.name, builderFormGlobal.urlBase),
paginator: manifestPaginatorToBuilder(retriever.paginator, stream.name, firstStreamUrlBase),
streamSlicer: manifestStreamSlicerToBuilder(retriever.stream_slicer, serializedStreamToIndex, stream.name),
schema: manifestSchemaLoaderToBuilderSchema(stream.schema_loader),
unsupportedFields: {
Expand Down Expand Up @@ -316,9 +323,21 @@ function manifestAuthenticatorToBuilder(
builderAuthenticator = {
type: "NoAuth",
};
} else if (manifestAuthenticator.type === undefined) {
throw new ManifestCompatibilityError(streamName, "authenticator has no type");
} else if (manifestAuthenticator.type === "CustomAuthenticator") {
throw new ManifestCompatibilityError(streamName, "uses a CustomAuthenticator");
} else if (manifestAuthenticator.type === "OAuthAuthenticator") {
if (
Object.values(manifestAuthenticator.refresh_request_body ?? {}).filter((value) => typeof value !== "string")
.length > 0
) {
throw new ManifestCompatibilityError(
streamName,
"OAuthAuthenticator contains a refresh_request_body with non-string values"
);
}

builderAuthenticator = {
...manifestAuthenticator,
refresh_request_body: Object.entries(manifestAuthenticator.refresh_request_body ?? {}),
Expand Down
7 changes: 6 additions & 1 deletion airbyte-webapp/src/components/connectorBuilder/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -640,12 +640,17 @@ export const convertToManifest = (values: BuilderFormValues): ConnectorManifest
type: "Spec",
};

const streamNames = values.streams.map((s) => s.name);
const validCheckStreamNames = values.checkStreams.filter((checkStream) => streamNames.includes(checkStream));
const correctedCheckStreams =
validCheckStreamNames.length > 0 ? validCheckStreamNames : streamNames.length > 0 ? [streamNames[0]] : [];

return merge({
version: values.version,
type: "DeclarativeSource",
check: {
type: "CheckStream",
stream_names: values.checkStreams,
stream_names: correctedCheckStreams,
},
streams: manifestStreams,
spec,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,13 @@ export const ConnectorBuilderFormStateProvider: React.FC<React.PropsWithChildren
const [yamlIsValid, setYamlIsValid] = useState(true);
const [yamlEditorIsMounted, setYamlEditorIsMounted] = useState(true);

const yamlManifest = useMemo(() => dump(derivedJsonManifest), [derivedJsonManifest]);
const yamlManifest = useMemo(
() =>
dump(derivedJsonManifest, {
noRefs: true,
}),
[derivedJsonManifest]
);

const lastValidBuilderFormValues = lastValidBuilderFormValuesRef.current;
/**
Expand Down

0 comments on commit ad8a632

Please sign in to comment.