From 994df62f2a037ee5a48dc4a58e98d09cedba97ed Mon Sep 17 00:00:00 2001 From: Peter Hicks Date: Mon, 3 Feb 2025 10:31:40 -0800 Subject: [PATCH] feat(edit-sources): Editing existing DW sources (#27934) --- .../external/forms/SourceForm.tsx | 58 ++++++++++++++----- .../data-warehouse/new/NewSourceWizard.tsx | 25 ++++---- .../data-warehouse/new/sourceWizardLogic.tsx | 24 ++++---- .../settings/source/SourceConfiguration.tsx | 21 +++++-- posthog/warehouse/api/external_data_source.py | 40 ++++++++++--- .../api/test/test_external_data_source.py | 18 +++--- 6 files changed, 123 insertions(+), 63 deletions(-) diff --git a/frontend/src/scenes/data-warehouse/external/forms/SourceForm.tsx b/frontend/src/scenes/data-warehouse/external/forms/SourceForm.tsx index 1663e71868676..8d08a13110b0a 100644 --- a/frontend/src/scenes/data-warehouse/external/forms/SourceForm.tsx +++ b/frontend/src/scenes/data-warehouse/external/forms/SourceForm.tsx @@ -1,7 +1,7 @@ import { LemonDivider, LemonFileInput, LemonInput, LemonSelect, LemonSwitch, LemonTextArea } from '@posthog/lemon-ui' -import { Form, Group } from 'kea-forms' +import { FieldName, Form, Group } from 'kea-forms' import { LemonField } from 'lib/lemon-ui/LemonField' -import React from 'react' +import React, { useEffect } from 'react' import { SourceConfig, SourceFieldConfig } from '~/types' @@ -13,14 +13,25 @@ export interface SourceFormProps { sourceConfig: SourceConfig showPrefix?: boolean jobInputs?: Record + setSourceConfigValue?: (key: FieldName, value: any) => void } const CONNECTION_STRING_DEFAULT_PORT = { Postgres: 5432, } -const sourceFieldToElement = (field: SourceFieldConfig, sourceConfig: SourceConfig, lastValue?: any): JSX.Element => { +const sourceFieldToElement = ( + field: SourceFieldConfig, + sourceConfig: SourceConfig, + lastValue?: any, + isUpdateMode?: boolean +): JSX.Element => { + // It doesn't make sense for this to show on an update to an existing connection since we likely just want to change + // a field or two. There is also some divergence in creates vs. updates that make this a bit more complex to handle. if (field.type === 'text' && field.name === 'connection_string') { + if (isUpdateMode) { + return + } return ( @@ -38,7 +49,7 @@ const sourceFieldToElement = (field: SourceFieldConfig, sourceConfig: SourceConf if (isValid) { sourceWizardLogic.actions.setSourceConnectionDetailsValue( - ['payload', 'dbname'], + ['payload', 'database'], database || '' ) sourceWizardLogic.actions.setSourceConnectionDetailsValue( @@ -126,13 +137,16 @@ const sourceFieldToElement = (field: SourceFieldConfig, sourceConfig: SourceConf if (field.type === 'textarea') { return ( - + {({ value, onChange }) => ( + + )} ) } @@ -172,8 +186,7 @@ const sourceFieldToElement = (field: SourceFieldConfig, sourceConfig: SourceConf data-attr={field.name} placeholder={field.placeholder} type={field.type as 'text'} - defaultValue={lastValue} - value={value ?? ''} + value={value || ''} onChange={onChange} /> )} @@ -189,12 +202,27 @@ export default function SourceFormContainer(props: SourceFormProps): JSX.Element ) } -export function SourceFormComponent({ sourceConfig, showPrefix = true, jobInputs }: SourceFormProps): JSX.Element { +export function SourceFormComponent({ + sourceConfig, + showPrefix = true, + jobInputs, + setSourceConfigValue, +}: SourceFormProps): JSX.Element { + useEffect(() => { + if (jobInputs && setSourceConfigValue) { + for (const input of Object.keys(jobInputs || {})) { + setSourceConfigValue(['payload', input], jobInputs[input]) + } + } + }, [JSON.stringify(jobInputs), setSourceConfigValue]) + + const isUpdateMode = !!setSourceConfigValue + return (
{SOURCE_DETAILS[sourceConfig.name].fields.map((field) => - sourceFieldToElement(field, sourceConfig, jobInputs?.[field.name]) + sourceFieldToElement(field, sourceConfig, jobInputs?.[field.name], isUpdateMode) )} {showPrefix && ( diff --git a/frontend/src/scenes/data-warehouse/new/NewSourceWizard.tsx b/frontend/src/scenes/data-warehouse/new/NewSourceWizard.tsx index d9027e32296af..eb2aafa9d7e6c 100644 --- a/frontend/src/scenes/data-warehouse/new/NewSourceWizard.tsx +++ b/frontend/src/scenes/data-warehouse/new/NewSourceWizard.tsx @@ -3,7 +3,7 @@ import { useActions, useValues } from 'kea' import { PageHeader } from 'lib/components/PageHeader' import { FEATURE_FLAGS } from 'lib/constants' import { featureFlagLogic } from 'lib/logic/featureFlagLogic' -import { useCallback } from 'react' +import { useCallback, useEffect } from 'react' import { SceneExport } from 'scenes/sceneTypes' import { ManualLinkSourceType, SourceConfig } from '~/types' @@ -52,20 +52,17 @@ interface NewSourcesWizardProps { export function NewSourcesWizard({ onComplete }: NewSourcesWizardProps): JSX.Element { const wizardLogic = sourceWizardLogic({ onComplete }) - const { - modalTitle, - modalCaption, - isWrapped, - currentStep, - isLoading, - canGoBack, - canGoNext, - nextButtonText, - showSkipButton, - } = useValues(wizardLogic) - const { onBack, onSubmit } = useActions(wizardLogic) + const { modalTitle, modalCaption, isWrapped, currentStep, isLoading, canGoBack, canGoNext, nextButtonText } = + useValues(wizardLogic) + const { onBack, onSubmit, onClear } = useActions(wizardLogic) const { tableLoading: manualLinkIsLoading } = useValues(dataWarehouseTableLogic) + useEffect(() => { + return () => { + onClear() + } + }, [onClear]) + const footer = useCallback(() => { if (currentStep === 1) { return null @@ -96,7 +93,7 @@ export function NewSourcesWizard({ onComplete }: NewSourcesWizardProps): JSX.Ele
) - }, [currentStep, isLoading, manualLinkIsLoading, canGoNext, canGoBack, nextButtonText, showSkipButton]) + }, [currentStep, canGoBack, onBack, isLoading, manualLinkIsLoading, canGoNext, nextButtonText, onSubmit]) return ( <> diff --git a/frontend/src/scenes/data-warehouse/new/sourceWizardLogic.tsx b/frontend/src/scenes/data-warehouse/new/sourceWizardLogic.tsx index c4cb49c134f64..9ef54824cc165 100644 --- a/frontend/src/scenes/data-warehouse/new/sourceWizardLogic.tsx +++ b/frontend/src/scenes/data-warehouse/new/sourceWizardLogic.tsx @@ -51,14 +51,14 @@ export const SOURCE_DETAILS: Record = { caption: , fields: [ { - name: 'account_id', + name: 'stripe_account_id', label: 'Account id', type: 'text', required: false, - placeholder: 'acct_...', + placeholder: 'stripe_account_id', }, { - name: 'client_secret', + name: 'stripe_secret_key', label: 'Client secret', type: 'password', required: true, @@ -103,7 +103,7 @@ export const SOURCE_DETAILS: Record = { placeholder: '5432', }, { - name: 'dbname', + name: 'database', label: 'Database', type: 'text', required: true, @@ -234,7 +234,7 @@ export const SOURCE_DETAILS: Record = { placeholder: '3306', }, { - name: 'dbname', + name: 'database', label: 'Database', type: 'text', required: true, @@ -383,7 +383,7 @@ export const SOURCE_DETAILS: Record = { placeholder: '1433', }, { - name: 'dbname', + name: 'database', label: 'Database', type: 'text', required: true, @@ -872,21 +872,19 @@ export const sourceWizardLogic = kea([ { setDatabaseSchemas: (_, { schemas }) => schemas, toggleSchemaShouldSync: (state, { schema, shouldSync }) => { - const newSchema = state.map((s) => ({ + return state.map((s) => ({ ...s, should_sync: s.table === schema.table ? shouldSync : s.should_sync, })) - return newSchema }, updateSchemaSyncType: (state, { schema, syncType, incrementalField, incrementalFieldType }) => { - const newSchema = state.map((s) => ({ + return state.map((s) => ({ ...s, sync_type: s.table === schema.table ? syncType : s.sync_type, incremental_field: s.table === schema.table ? incrementalField : s.incremental_field, incremental_field_type: s.table === schema.table ? incrementalFieldType : s.incremental_field_type, })) - return newSchema }, }, ], @@ -1304,9 +1302,7 @@ export const sourceWizardLogic = kea([ fileReader.onerror = (e) => reject(e) fileReader.readAsText(payload['payload'][name][0]) }) - const jsonConfig = JSON.parse(loadedFile) - - fieldPayload[name] = jsonConfig + fieldPayload[name] = JSON.parse(loadedFile) } catch (e) { return lemonToast.error('File is not valid') } @@ -1375,7 +1371,7 @@ export const getErrorsForFields = ( } } else { errorsObj[field.name] = {} - const selection = valueObj[field.name]['selection'] + const selection = valueObj[field.name]?.['selection'] field.options .find((n) => n.value === selection) ?.fields?.forEach((f) => validateField(f, valueObj[field.name], errorsObj[field.name])) diff --git a/frontend/src/scenes/data-warehouse/settings/source/SourceConfiguration.tsx b/frontend/src/scenes/data-warehouse/settings/source/SourceConfiguration.tsx index 8df7e27c760d7..b1dfd74983b82 100644 --- a/frontend/src/scenes/data-warehouse/settings/source/SourceConfiguration.tsx +++ b/frontend/src/scenes/data-warehouse/settings/source/SourceConfiguration.tsx @@ -1,8 +1,10 @@ import { LemonBanner, LemonButton, LemonSkeleton } from '@posthog/lemon-ui' -import { BindLogic, useValues } from 'kea' +import { BindLogic, useActions, useValues } from 'kea' import { Form } from 'kea-forms' import { SourceFormComponent, SourceFormProps } from 'scenes/data-warehouse/external/forms/SourceForm' +import { ExternalDataSourceType } from '~/types' + import { dataWarehouseSourceSettingsLogic } from './dataWarehouseSourceSettingsLogic' interface SourceConfigurationProps { @@ -26,24 +28,33 @@ interface UpdateSourceConnectionFormContainerProps extends SourceFormProps { id: string } +const supportedEditingSourceTypes: ExternalDataSourceType[] = ['MSSQL', 'MySQL', 'Postgres', 'Stripe'] + function UpdateSourceConnectionFormContainer(props: UpdateSourceConnectionFormContainerProps): JSX.Element { const { source, sourceLoading } = useValues(dataWarehouseSourceSettingsLogic({ id: props.id })) + const { setSourceConfigValue } = useActions(dataWarehouseSourceSettingsLogic) - if (source?.source_type !== 'MSSQL' && source?.source_type !== 'MySQL' && source?.source_type !== 'Postgres') { + if (!source?.source_type || !supportedEditingSourceTypes.includes(source?.source_type)) { return (

- Only Postgres, MSSQL, and MySQL are configurable. Please delete and recreate your source if you need - to connect to a new source of the same type. + Only {supportedEditingSourceTypes.slice(0, -1).join(', ')}, and {supportedEditingSourceTypes.at(-1)}{' '} + are configurable. Please delete and recreate your source if you need to connect to a new source of + the same type.

) } + return ( <> Overwrite your existing configuration here
- +
str: latest_completed_run = instance.ordered_jobs[0] if instance.ordered_jobs else None # type: ignore @@ -429,8 +455,8 @@ def create(self, request: Request, *args: Any, **kwargs: Any) -> Response: def _handle_stripe_source(self, request: Request, *args: Any, **kwargs: Any) -> ExternalDataSource: payload = request.data["payload"] - client_secret = payload.get("client_secret") - account_id = payload.get("account_id", None) + client_secret = payload.get("stripe_secret_key") + account_id = payload.get("stripe_account_id", None) prefix = request.data.get("prefix", None) source_type = request.data["source_type"] # TODO: remove dummy vars @@ -576,7 +602,7 @@ def _handle_sql_source(self, request: Request, *args: Any, **kwargs: Any) -> tup host = payload.get("host") port = payload.get("port") - database = payload.get("dbname") + database = payload.get("database") user = payload.get("user") password = payload.get("password") @@ -847,7 +873,7 @@ def database_schema(self, request: Request, *arg: Any, **kwargs: Any): # Validate sourced credentials if source_type == ExternalDataSource.Type.STRIPE: - key = request.data.get("client_secret", "") + key = request.data.get("stripe_secret_key", "") if not validate_stripe_credentials(api_key=key): return Response( status=status.HTTP_400_BAD_REQUEST, @@ -960,7 +986,7 @@ def database_schema(self, request: Request, *arg: Any, **kwargs: Any): host = request.data.get("host", None) port = request.data.get("port", None) - database = request.data.get("dbname", None) + database = request.data.get("database", None) user = request.data.get("user", None) password = request.data.get("password", None) diff --git a/posthog/warehouse/api/test/test_external_data_source.py b/posthog/warehouse/api/test/test_external_data_source.py index 4d09ce1eb7fc8..f39dcd9f6706b 100644 --- a/posthog/warehouse/api/test/test_external_data_source.py +++ b/posthog/warehouse/api/test/test_external_data_source.py @@ -391,7 +391,8 @@ def test_dont_expose_job_inputs(self): assert len(results) == 1 result = results[0] - assert result.get("job_inputs") is None + # we should scrape out `stripe_secret_key` from job_inputs + assert result.get("job_inputs") == {} def test_get_external_data_source_with_schema(self): source = self._create_external_data_source() @@ -412,6 +413,7 @@ def test_get_external_data_source_with_schema(self): "prefix", "last_run_at", "schemas", + "job_inputs", ], ) self.assertEqual( @@ -485,7 +487,7 @@ def test_database_schema(self): "source_type": "Postgres", "host": settings.PG_HOST, "port": int(settings.PG_PORT), - "dbname": settings.PG_DATABASE, + "database": settings.PG_DATABASE, "user": settings.PG_USER, "password": settings.PG_PASSWORD, "schema": "public", @@ -619,7 +621,7 @@ def test_internal_postgres(self, patch_get_sql_schemas_for_source_type, patch_ge "source_type": "Postgres", "host": "172.16.0.0", "port": int(settings.PG_PORT), - "dbname": settings.PG_DATABASE, + "database": settings.PG_DATABASE, "user": settings.PG_USER, "password": settings.PG_PASSWORD, "schema": "public", @@ -646,7 +648,7 @@ def test_internal_postgres(self, patch_get_sql_schemas_for_source_type, patch_ge "source_type": "Postgres", "host": "172.16.0.0", "port": int(settings.PG_PORT), - "dbname": settings.PG_DATABASE, + "database": settings.PG_DATABASE, "user": settings.PG_USER, "password": settings.PG_PASSWORD, "schema": "public", @@ -664,7 +666,7 @@ def test_internal_postgres(self, patch_get_sql_schemas_for_source_type, patch_ge "host": "172.16.0.0", "rows": 42, "port": int(settings.PG_PORT), - "dbname": settings.PG_DATABASE, + "database": settings.PG_DATABASE, "user": settings.PG_USER, "password": settings.PG_PASSWORD, "schema": "public", @@ -692,7 +694,7 @@ def test_internal_postgres(self, patch_get_sql_schemas_for_source_type, patch_ge "source_type": "Postgres", "host": "172.16.0.0", "port": int(settings.PG_PORT), - "dbname": settings.PG_DATABASE, + "database": settings.PG_DATABASE, "user": settings.PG_USER, "password": settings.PG_PASSWORD, "schema": "public", @@ -825,8 +827,8 @@ def test_trimming_payload(self): data={ "source_type": "Stripe", "payload": { - "client_secret": " sk_test_123 ", - "account_id": " blah ", + "stripe_secret_key": " sk_test_123 ", + "stripe_account_id": " blah ", "schemas": [ {"name": "BalanceTransaction", "should_sync": True, "sync_type": "full_refresh"}, ],