From 2c9d099b9e20f1b376a4c655a8f31cc352369c20 Mon Sep 17 00:00:00 2001 From: Edmundo Ruiz Ghanem <168664+edmundito@users.noreply.github.com> Date: Tue, 29 Nov 2022 11:30:38 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=AA=9F=20=F0=9F=90=9B=20Update=20refresh?= =?UTF-8?q?=20schema=20logic=20to=20handle=20confirmation=20modal=20and=20?= =?UTF-8?q?other=20edge=20cases=20(#19131)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Update refresh schema logic to handle confirmation modal * Update the refresh schema separately * Only mark as refreshed if the syncCatalog has changed * Add option to clear refreshed schema * Update tests * Update connection edit service to track the schema changes separately * ConnectionEditService now marks schema as refreshed only if the catalog has changed, clears refresh after schema update * Add mockCatalog diff and add refresh schema tests to connection edit service * Add interfaces to useConnectionEdit and useConnectionForm to better track references * Remove clearRefreshedSchema from ConnectionReplicationTab form submit * Update useConfirmCatalogDiff to only show if the diff has changed, instead of the whole connection * Cleanup discardRefreshedSchema name in ConnectionReplicationTab --- .../ConnectionEditService.test.tsx | 92 ++++++++++++++++++- .../ConnectionEdit/ConnectionEditService.tsx | 74 ++++++++++++--- .../ConnectionForm/ConnectionFormService.tsx | 28 +++++- .../ConnectionReplicationTab.tsx | 17 +++- .../test-utils/mock-data/mockCatalogDiff.ts | 46 ++++++++++ .../useConfirmCatalogDiff.tsx | 4 +- 6 files changed, 234 insertions(+), 27 deletions(-) create mode 100644 airbyte-webapp/src/test-utils/mock-data/mockCatalogDiff.ts diff --git a/airbyte-webapp/src/hooks/services/ConnectionEdit/ConnectionEditService.test.tsx b/airbyte-webapp/src/hooks/services/ConnectionEdit/ConnectionEditService.test.tsx index b6ef1f314513..4e829cb10b1c 100644 --- a/airbyte-webapp/src/hooks/services/ConnectionEdit/ConnectionEditService.test.tsx +++ b/airbyte-webapp/src/hooks/services/ConnectionEdit/ConnectionEditService.test.tsx @@ -1,11 +1,12 @@ import { act, renderHook } from "@testing-library/react-hooks"; import React from "react"; +import { mockCatalogDiff } from "test-utils/mock-data/mockCatalogDiff"; import { mockConnection } from "test-utils/mock-data/mockConnection"; import { mockDestination } from "test-utils/mock-data/mockDestination"; import { mockWorkspace } from "test-utils/mock-data/mockWorkspace"; import { TestWrapper } from "test-utils/testutils"; -import { WebBackendConnectionUpdate } from "core/request/AirbyteClient"; +import { WebBackendConnectionRead, WebBackendConnectionUpdate } from "core/request/AirbyteClient"; import { useConnectionFormService } from "../ConnectionForm/ConnectionFormService"; import { ConnectionEditServiceProvider, useConnectionEditService } from "./ConnectionEditService"; @@ -18,14 +19,24 @@ jest.mock("services/workspaces/WorkspacesService", () => ({ useCurrentWorkspace: () => mockWorkspace, })); +const utils = { + getMockConnectionWithRefreshedCatalog: (): WebBackendConnectionRead => ({ + ...mockConnection, + catalogDiff: mockCatalogDiff, + catalogId: `${mockConnection.catalogId}1`, + }), +}; + jest.mock("../useConnectionHook", () => ({ useGetConnection: () => mockConnection, useWebConnectionService: () => ({ - getConnection: () => mockConnection, + getConnection: (_connectionId: string, withRefreshedCatalog?: boolean) => + withRefreshedCatalog ? utils.getMockConnectionWithRefreshedCatalog() : mockConnection, }), useUpdateConnection: () => ({ mutateAsync: jest.fn(async (connection: WebBackendConnectionUpdate) => { - return { ...mockConnection, ...connection }; + const { sourceCatalogId, ...connectionUpdate } = connection; + return { ...mockConnection, ...connectionUpdate, catalogId: sourceCatalogId ?? mockConnection.catalogId }; }), isLoading: false, }), @@ -77,7 +88,7 @@ describe("ConnectionEditService", () => { expect(result.current.connection).toEqual({ ...mockConnection, ...mockUpdateConnection }); }); - it("should refresh connection", async () => { + it("should refresh schema", async () => { // Need to combine the hooks so both can be used. const useMyTestHook = () => { return [useConnectionEditService(), useConnectionFormService()] as const; @@ -108,6 +119,77 @@ describe("ConnectionEditService", () => { }); expect(result.current[0].schemaHasBeenRefreshed).toBe(true); - expect(result.current[0].connection).toEqual(mockConnection); + expect(result.current[0].schemaRefreshing).toBe(false); + expect(result.current[0].connection).toEqual(utils.getMockConnectionWithRefreshedCatalog()); + }); + + it("should refresh schema only if the sync catalog has diffs", async () => { + // Need to combine the hooks so both can be used. + const useMyTestHook = () => + ({ editService: useConnectionEditService(), formService: useConnectionFormService() } as const); + + const { result } = renderHook(useMyTestHook, { + wrapper: Wrapper, + initialProps: { + connectionId: mockConnection.connectionId, + }, + }); + + const connectionUpdate = { + connectionId: mockConnection.connectionId, + name: "new connection name", + prefix: "new connection prefix", + }; + + const updatedConnection: WebBackendConnectionRead = { + ...mockConnection, + ...connectionUpdate, + }; + + jest.spyOn(utils, "getMockConnectionWithRefreshedCatalog").mockImplementationOnce( + (): WebBackendConnectionRead => ({ + ...updatedConnection, + catalogDiff: { transforms: [] }, + }) + ); + + await act(async () => { + await result.current.editService.updateConnection(connectionUpdate); + await result.current.formService.refreshSchema(); + }); + + expect(result.current.editService.schemaHasBeenRefreshed).toBe(false); + expect(result.current.editService.schemaRefreshing).toBe(false); + expect(result.current.editService.connection).toEqual(updatedConnection); + }); + + it("should discard the refreshed schema", async () => { + const useMyTestHook = () => + ({ editService: useConnectionEditService(), formService: useConnectionFormService() } as const); + + const { result } = renderHook(useMyTestHook, { + wrapper: Wrapper, + initialProps: { + connectionId: mockConnection.connectionId, + }, + }); + + const connectionUpdate: WebBackendConnectionUpdate = { + connectionId: mockConnection.connectionId, + name: "new connection name", + prefix: "new connection prefix", + }; + + const updatedConnection = { ...mockConnection, ...connectionUpdate }; + + await act(async () => { + await result.current.formService.refreshSchema(); + await result.current.editService.updateConnection(connectionUpdate); + result.current.editService.discardRefreshedSchema(); + }); + + expect(result.current.editService.schemaHasBeenRefreshed).toBe(false); + expect(result.current.editService.schemaRefreshing).toBe(false); + expect(result.current.editService.connection).toEqual(updatedConnection); }); }); diff --git a/airbyte-webapp/src/hooks/services/ConnectionEdit/ConnectionEditService.tsx b/airbyte-webapp/src/hooks/services/ConnectionEdit/ConnectionEditService.tsx index 7ba926e10725..78b554415df5 100644 --- a/airbyte-webapp/src/hooks/services/ConnectionEdit/ConnectionEditService.tsx +++ b/airbyte-webapp/src/hooks/services/ConnectionEdit/ConnectionEditService.tsx @@ -1,7 +1,13 @@ +import { pick } from "lodash"; import { useContext, useState, createContext, useCallback } from "react"; import { useAsyncFn } from "react-use"; -import { ConnectionStatus, WebBackendConnectionUpdate } from "core/request/AirbyteClient"; +import { + AirbyteCatalog, + ConnectionStatus, + WebBackendConnectionRead, + WebBackendConnectionUpdate, +} from "core/request/AirbyteClient"; import { ConnectionFormServiceProvider } from "../ConnectionForm/ConnectionFormService"; import { useGetConnection, useUpdateConnection, useWebConnectionService } from "../useConnectionHook"; @@ -11,22 +17,69 @@ interface ConnectionEditProps { connectionId: string; } -const useConnectionEdit = ({ connectionId }: ConnectionEditProps) => { - const [connection, setConnection] = useState(useGetConnection(connectionId)); +export interface ConnectionCatalog { + syncCatalog: AirbyteCatalog; + catalogId?: string; +} + +interface ConnectionEditHook { + connection: WebBackendConnectionRead; + connectionUpdating: boolean; + schemaError?: Error; + schemaRefreshing: boolean; + schemaHasBeenRefreshed: boolean; + updateConnection: (connectionUpdates: WebBackendConnectionUpdate) => Promise; + refreshSchema: () => Promise; + discardRefreshedSchema: () => void; +} + +const getConnectionCatalog = (connection: WebBackendConnectionRead): ConnectionCatalog => + pick(connection, ["syncCatalog", "catalogId"]); + +const useConnectionEdit = ({ connectionId }: ConnectionEditProps): ConnectionEditHook => { const connectionService = useWebConnectionService(); + const [connection, setConnection] = useState(useGetConnection(connectionId)); + const [catalog, setCatalog] = useState(() => getConnectionCatalog(connection)); const [schemaHasBeenRefreshed, setSchemaHasBeenRefreshed] = useState(false); const [{ loading: schemaRefreshing, error: schemaError }, refreshSchema] = useAsyncFn(async () => { const refreshedConnection = await connectionService.getConnection(connectionId, true); - setConnection(refreshedConnection); - setSchemaHasBeenRefreshed(true); + if (refreshedConnection.catalogDiff && refreshedConnection.catalogDiff.transforms?.length > 0) { + setConnection(refreshedConnection); + setSchemaHasBeenRefreshed(true); + } }, [connectionId]); + const discardRefreshedSchema = useCallback(() => { + setConnection((connection) => ({ + ...connection, + ...catalog, + catalogDiff: undefined, + })); + setSchemaHasBeenRefreshed(false); + }, [catalog]); + const { mutateAsync: updateConnectionAction, isLoading: connectionUpdating } = useUpdateConnection(); const updateConnection = useCallback( - async (connection: WebBackendConnectionUpdate) => { - setConnection(await updateConnectionAction(connection)); + async (connectionUpdates: WebBackendConnectionUpdate) => { + const updatedConnection = await updateConnectionAction(connectionUpdates); + const updatedKeys = Object.keys(connectionUpdates).map((key) => (key === "sourceCatalogId" ? "catalogId" : key)); + const connectionPatch = pick(updatedConnection, updatedKeys); + const wasSyncCatalogUpdated = !!connectionPatch.syncCatalog; + + // Mutate the current connection state only with the values that were updated + setConnection((connection) => ({ + ...connection, + ...connectionPatch, + catalogDiff: wasSyncCatalogUpdated ? undefined : connection.catalogDiff, + })); + + if (wasSyncCatalogUpdated) { + // The catalog ws also saved, so update the current catalog + setCatalog(getConnectionCatalog(updatedConnection)); + setSchemaHasBeenRefreshed(false); + } }, [updateConnectionAction] ); @@ -38,15 +91,12 @@ const useConnectionEdit = ({ connectionId }: ConnectionEditProps) => { schemaRefreshing, schemaHasBeenRefreshed, updateConnection, - setSchemaHasBeenRefreshed, refreshSchema, + discardRefreshedSchema, }; }; -const ConnectionEditContext = createContext, - "refreshSchema" | "schemaError" -> | null>(null); +const ConnectionEditContext = createContext | null>(null); export const ConnectionEditServiceProvider: React.FC> = ({ children, diff --git a/airbyte-webapp/src/hooks/services/ConnectionForm/ConnectionFormService.tsx b/airbyte-webapp/src/hooks/services/ConnectionForm/ConnectionFormService.tsx index 19f7d273cc32..d42b21461dcf 100644 --- a/airbyte-webapp/src/hooks/services/ConnectionForm/ConnectionFormService.tsx +++ b/airbyte-webapp/src/hooks/services/ConnectionForm/ConnectionFormService.tsx @@ -1,7 +1,12 @@ import React, { createContext, useCallback, useContext, useState } from "react"; import { useIntl } from "react-intl"; -import { ConnectionScheduleType, OperationRead, WebBackendConnectionRead } from "core/request/AirbyteClient"; +import { + ConnectionScheduleType, + DestinationDefinitionSpecificationRead, + OperationRead, + WebBackendConnectionRead, +} from "core/request/AirbyteClient"; import { useGetDestinationDefinitionSpecification } from "services/connector/DestinationDefinitionSpecificationService"; import { FormError, generateMessageFromError } from "utils/errorStatusMessage"; import { @@ -54,7 +59,24 @@ export const tidyConnectionFormValues = ( return formValues; }; -const useConnectionForm = ({ connection, mode, schemaError, refreshSchema }: ConnectionServiceProps) => { +interface ConnectionFormHook { + connection: ConnectionOrPartialConnection; + mode: ConnectionFormMode; + destDefinition: DestinationDefinitionSpecificationRead; + initialValues: FormikConnectionFormValues; + schemaError?: SchemaError; + formId: string; + setSubmitError: (submitError: FormError | null) => void; + getErrorMessage: (formValid: boolean, connectionDirty: boolean) => string | JSX.Element | null; + refreshSchema: () => Promise; +} + +const useConnectionForm = ({ + connection, + mode, + schemaError, + refreshSchema, +}: ConnectionServiceProps): ConnectionFormHook => { const destDefinition = useGetDestinationDefinitionSpecification(connection.destination.destinationDefinitionId); const initialValues = useInitialValues(connection, destDefinition, mode !== "create"); const { formatMessage } = useIntl(); @@ -84,7 +106,7 @@ const useConnectionForm = ({ connection, mode, schemaError, refreshSchema }: Con }; }; -const ConnectionFormContext = createContext | null>(null); +const ConnectionFormContext = createContext(null); export const ConnectionFormServiceProvider: React.FC> = ({ children, diff --git a/airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/ConnectionReplicationTab.tsx b/airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/ConnectionReplicationTab.tsx index af20836fe425..0ba62b8e11e0 100644 --- a/airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/ConnectionReplicationTab.tsx +++ b/airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/ConnectionReplicationTab.tsx @@ -1,6 +1,7 @@ import { Form, Formik, FormikHelpers } from "formik"; import React, { useCallback, useState } from "react"; import { FormattedMessage, useIntl } from "react-intl"; +import { useUnmount } from "react-use"; import { SchemaError } from "components/CreateConnection/SchemaError"; import LoadingSchema from "components/LoadingSchema"; @@ -40,7 +41,7 @@ export const ConnectionReplicationTab: React.FC = () => { const [saved, setSaved] = useState(false); - const { connection, schemaRefreshing, schemaHasBeenRefreshed, updateConnection, setSchemaHasBeenRefreshed } = + const { connection, schemaRefreshing, schemaHasBeenRefreshed, updateConnection, discardRefreshedSchema } = useConnectionEditService(); const { initialValues, mode, schemaError, getErrorMessage, setSubmitError } = useConnectionFormService(); const allowSubOneHourCronExpressions = useFeature(FeatureItem.AllowSyncSubOneHourCronExpressions); @@ -134,7 +135,6 @@ export const ConnectionReplicationTab: React.FC = () => { } setSaved(true); - setSchemaHasBeenRefreshed(false); } catch (e) { setSubmitError(e); } @@ -150,7 +150,6 @@ export const ConnectionReplicationTab: React.FC = () => { mode, openModal, saveConnection, - setSchemaHasBeenRefreshed, setSubmitError, workspaceId, allowSubOneHourCronExpressions, @@ -159,6 +158,10 @@ export const ConnectionReplicationTab: React.FC = () => { useConfirmCatalogDiff(); + useUnmount(() => { + discardRefreshedSchema(); + }); + return (
{schemaError && !schemaRefreshing ? ( @@ -172,14 +175,18 @@ export const ConnectionReplicationTab: React.FC = () => { > {({ values, isSubmitting, isValid, dirty, resetForm }) => (
- + { resetForm(); - setSchemaHasBeenRefreshed(false); + discardRefreshedSchema(); }} successMessage={saved && !dirty && } errorMessage={getErrorMessage(isValid, dirty)} diff --git a/airbyte-webapp/src/test-utils/mock-data/mockCatalogDiff.ts b/airbyte-webapp/src/test-utils/mock-data/mockCatalogDiff.ts new file mode 100644 index 000000000000..a95242228ce2 --- /dev/null +++ b/airbyte-webapp/src/test-utils/mock-data/mockCatalogDiff.ts @@ -0,0 +1,46 @@ +import { CatalogDiff } from "core/request/AirbyteClient"; + +export const mockCatalogDiff: CatalogDiff = { + transforms: [ + { + transformType: "update_stream", + streamDescriptor: { namespace: "apple", name: "harissa_paste" }, + updateStream: [ + { transformType: "add_field", fieldName: ["users", "phone"], breaking: false }, + { transformType: "add_field", fieldName: ["users", "email"], breaking: false }, + { transformType: "remove_field", fieldName: ["users", "lastName"], breaking: false }, + + { + transformType: "update_field_schema", + breaking: false, + fieldName: ["users", "address"], + updateFieldSchema: { oldSchema: { type: "number" }, newSchema: { type: "string" } }, + }, + ], + }, + { + transformType: "add_stream", + streamDescriptor: { namespace: "apple", name: "banana" }, + }, + { + transformType: "add_stream", + streamDescriptor: { namespace: "apple", name: "carrot" }, + }, + { + transformType: "remove_stream", + streamDescriptor: { namespace: "apple", name: "dragonfruit" }, + }, + { + transformType: "remove_stream", + streamDescriptor: { namespace: "apple", name: "eclair" }, + }, + { + transformType: "remove_stream", + streamDescriptor: { namespace: "apple", name: "fishcake" }, + }, + { + transformType: "remove_stream", + streamDescriptor: { namespace: "apple", name: "gelatin_mold" }, + }, + ], +}; diff --git a/airbyte-webapp/src/views/Connection/CatalogDiffModal/useConfirmCatalogDiff.tsx b/airbyte-webapp/src/views/Connection/CatalogDiffModal/useConfirmCatalogDiff.tsx index 5e9e7d94298d..9261d31175d6 100644 --- a/airbyte-webapp/src/views/Connection/CatalogDiffModal/useConfirmCatalogDiff.tsx +++ b/airbyte-webapp/src/views/Connection/CatalogDiffModal/useConfirmCatalogDiff.tsx @@ -10,10 +10,10 @@ export const useConfirmCatalogDiff = () => { const { formatMessage } = useIntl(); const { openModal } = useModalService(); const { connection } = useConnectionEditService(); + const { catalogDiff, syncCatalog } = connection; useEffect(() => { // If we have a catalogDiff we always want to show the modal - const { catalogDiff, syncCatalog } = connection; if (catalogDiff?.transforms && catalogDiff.transforms?.length > 0) { openModal({ title: formatMessage({ id: "connection.updateSchema.completed" }), @@ -25,5 +25,5 @@ export const useConfirmCatalogDiff = () => { ), }); } - }, [connection, formatMessage, openModal]); + }, [catalogDiff, syncCatalog, formatMessage, openModal]); };