diff --git a/web-admin/src/features/projects/environment-variables/ActionsCell.svelte b/web-admin/src/features/projects/environment-variables/ActionsCell.svelte index ad31bc22a27..2a2cf6b5ddb 100644 --- a/web-admin/src/features/projects/environment-variables/ActionsCell.svelte +++ b/web-admin/src/features/projects/environment-variables/ActionsCell.svelte @@ -5,12 +5,13 @@ import { Trash2Icon, Pencil } from "lucide-svelte"; import EditDialog from "./EditDialog.svelte"; import DeleteDialog from "./DeleteDialog.svelte"; + import type { VariableNames } from "./types"; export let id: string; export let environment: string; export let name: string; export let value: string; - export let variableNames: string[] = []; + export let variableNames: VariableNames = []; let isDropdownOpen = false; let isEditDialogOpen = false; diff --git a/web-admin/src/features/projects/environment-variables/ActivityCell.svelte b/web-admin/src/features/projects/environment-variables/ActivityCell.svelte index 94a3104dd96..201f9095c83 100644 --- a/web-admin/src/features/projects/environment-variables/ActivityCell.svelte +++ b/web-admin/src/features/projects/environment-variables/ActivityCell.svelte @@ -20,7 +20,7 @@ -
+
{formattedText}
diff --git a/web-admin/src/features/projects/environment-variables/AddDialog.svelte b/web-admin/src/features/projects/environment-variables/AddDialog.svelte index 2bb17716d62..ce5692c7466 100644 --- a/web-admin/src/features/projects/environment-variables/AddDialog.svelte +++ b/web-admin/src/features/projects/environment-variables/AddDialog.svelte @@ -22,13 +22,14 @@ import { defaults, superForm } from "sveltekit-superforms"; import { yup } from "sveltekit-superforms/adapters"; import { object, string, array } from "yup"; - import { EnvironmentType } from "./types"; + import { type VariableNames } from "./types"; import Input from "@rilldata/web-common/components/forms/Input.svelte"; import IconButton from "@rilldata/web-common/components/button/IconButton.svelte"; import { Trash2Icon, UploadIcon } from "lucide-svelte"; + import { getCurrentEnvironment, isDuplicateKey } from "./utils"; export let open = false; - export let variableNames: string[] = []; + export let variableNames: VariableNames = []; let inputErrors: { [key: number]: boolean } = {}; let isKeyAlreadyExists = false; @@ -61,8 +62,9 @@ key: string() .optional() .matches( - /^[a-zA-Z0-9_]+$/, - "Key must only contain letters, numbers, and underscores.", + /^[a-zA-Z_][a-zA-Z0-9_.]*$/, + // See: https://github.com/rilldata/rill/pull/6121/files#diff-04140a6ac071a4bac716371f8b66a56c89c9d52cfbf2b05ea1e14ee8d4e301e7R12 + "Key must start with a letter or underscore and can only contain letters, digits, underscores, and dots", ), value: string().optional(), }), @@ -80,12 +82,18 @@ if (!form.valid) return; const values = form.data; - // Omit draft variables that do not have a key + // Check for duplicates before proceeding + const duplicates = checkForExistingKeys(); + if (duplicates > 0) { + // Early return without resetting the form + return; + } + + // Only filter and process if there are no duplicates const filteredVariables = values.variables.filter( ({ key }) => key !== "", ); - // Flatten the variables to match the schema const flatVariables = Object.fromEntries( filteredVariables.map(({ key, value }) => [key, value]), ); @@ -93,28 +101,13 @@ try { await handleUpdateProjectVariables(flatVariables); open = false; + handleReset(); } catch (error) { console.error(error); } }, }); - function getCurrentEnvironment() { - if (isDevelopment && isProduction) { - return undefined; - } - - if (isDevelopment) { - return EnvironmentType.DEVELOPMENT; - } - - if (isProduction) { - return EnvironmentType.PRODUCTION; - } - - return undefined; - } - async function handleUpdateProjectVariables( flatVariables: AdminServiceUpdateProjectVariablesBodyVariables, ) { @@ -123,7 +116,7 @@ organization, project, data: { - environment: getCurrentEnvironment(), + environment: getCurrentEnvironment(isDevelopment, isProduction), variables: flatVariables, }, }); @@ -172,17 +165,29 @@ } function checkForExistingKeys() { - const existingKeys = $form.variables.map((variable) => variable.key); inputErrors = {}; isKeyAlreadyExists = false; - existingKeys.forEach((key, index) => { - // Case sensitive - if (variableNames.some((existingKey) => existingKey === key)) { - inputErrors[index] = true; + const existingKeys = $form.variables.map((variable) => { + return { + environment: getCurrentEnvironment(isDevelopment, isProduction), + name: variable.key, + }; + }); + + let duplicateCount = 0; + existingKeys.forEach((key, idx) => { + const variableEnvironment = key.environment; + const variableKey = key.name; + + if (isDuplicateKey(variableEnvironment, variableKey, variableNames)) { + inputErrors[idx] = true; isKeyAlreadyExists = true; + duplicateCount++; } }); + + return duplicateCount; } function handleFileUpload(event) { @@ -356,7 +361,9 @@ {#if isKeyAlreadyExists}

- These keys already exist for this project. + {Object.keys(inputErrors).length > 1 + ? "These keys already exist for your target environment(s)" + : "This key already exists for your target environment(s)"}

{/if} diff --git a/web-admin/src/features/projects/environment-variables/EditDialog.svelte b/web-admin/src/features/projects/environment-variables/EditDialog.svelte index 0774be048cd..f6c1f662021 100644 --- a/web-admin/src/features/projects/environment-variables/EditDialog.svelte +++ b/web-admin/src/features/projects/environment-variables/EditDialog.svelte @@ -21,16 +21,20 @@ import { defaults, superForm } from "sveltekit-superforms"; import { yup } from "sveltekit-superforms/adapters"; import { object, string } from "yup"; - import { EnvironmentType } from "./types"; + import { EnvironmentType, type VariableNames } from "./types"; import Input from "@rilldata/web-common/components/forms/Input.svelte"; - import { onMount } from "svelte"; + import { + getCurrentEnvironment, + getEnvironmentType, + isDuplicateKey, + } from "./utils"; export let open = false; export let id: string; export let environment: string; export let name: string; export let value: string; - export let variableNames: string[] = []; + export let variableNames: VariableNames = []; let initialEnvironment: { isDevelopment: boolean; @@ -45,7 +49,7 @@ $: project = $page.params.project; $: isEnvironmentSelected = isDevelopment || isProduction; - $: hasChanges = + $: hasNewChanges = $form.key !== initialValues.key || $form.value !== initialValues.value || initialEnvironment?.isDevelopment !== isDevelopment || @@ -67,8 +71,9 @@ key: string() .optional() .matches( - /^[a-zA-Z0-9_]+$/, - "Key must only contain letters, numbers, and underscores", + /^[a-zA-Z_][a-zA-Z0-9_.]*$/, + // See: https://github.com/rilldata/rill/pull/6121/files#diff-04140a6ac071a4bac716371f8b66a56c89c9d52cfbf2b05ea1e14ee8d4e301e7R12 + "Key must start with a letter or underscore and can only contain letters, digits, underscores, and dots", ), value: string().optional(), }), @@ -92,6 +97,9 @@ if (!form.valid) return; const values = form.data; + checkForExistingKeys(); + if (isKeyAlreadyExists) return; + const flatVariable = { [values.key]: values.value, }; @@ -99,31 +107,17 @@ try { await handleUpdateProjectVariables(flatVariable); open = false; + handleReset(); } catch (error) { console.error(error); } }, }); - function processEnvironment() { - if (isDevelopment && isProduction) { - return undefined; - } - - if (isDevelopment) { - return EnvironmentType.DEVELOPMENT; - } - - if (isProduction) { - return EnvironmentType.PRODUCTION; - } - - return undefined; - } - async function handleUpdateProjectVariables( flatVariable: AdminServiceUpdateProjectVariablesBodyVariables, ) { + // Check if the key has changed, if so, check for existing keys if ($form.key !== initialValues.key) { checkForExistingKeys(); } @@ -146,7 +140,7 @@ organization, project, data: { - environment: processEnvironment(), + environment: getCurrentEnvironment(isDevelopment, isProduction), variables: flatVariable, }, }); @@ -155,7 +149,10 @@ // If the key remains the same, update the environment or value if (initialValues.key === $form.key) { // If environment has changed, remove the old key and add the new key - if (initialValues.environment !== processEnvironment()) { + if ( + initialValues.environment !== + getCurrentEnvironment(isDevelopment, isProduction) + ) { await $updateProjectVariables.mutateAsync({ organization, project, @@ -170,7 +167,7 @@ organization, project, data: { - environment: processEnvironment(), + environment: getCurrentEnvironment(isDevelopment, isProduction), variables: flatVariable, }, }); @@ -204,21 +201,30 @@ function handleReset() { reset(); + isDevelopment = false; + isProduction = false; + inputErrors = {}; isKeyAlreadyExists = false; } function checkForExistingKeys() { - const existingKeys = [$form.key]; inputErrors = {}; isKeyAlreadyExists = false; - existingKeys.forEach((key, index) => { - // Case sensitive - if (variableNames.some((existingKey) => existingKey === key)) { - inputErrors[index] = true; - isKeyAlreadyExists = true; - } - }); + const existingKey = { + environment: getEnvironmentType( + getCurrentEnvironment(isDevelopment, isProduction), + ), + name: $form.key, + }; + + const variableEnvironment = existingKey.environment; + const variableKey = existingKey.name; + + if (isDuplicateKey(variableEnvironment, variableKey, variableNames)) { + inputErrors[0] = true; + isKeyAlreadyExists = true; + } } function setInitialCheckboxState() { @@ -236,13 +242,17 @@ } } - onMount(() => { + function handleDialogOpen() { setInitialCheckboxState(); initialEnvironment = { isDevelopment, isProduction, }; - }); + } + + $: if (open) { + handleDialogOpen(); + } handleKeyChange(e)} - onBlur={() => { - if ($form.key !== initialValues.key) { - checkForExistingKeys(); - } - }} />

- This key already exists for this project. + This key already exists for your target environment(s)

{/if} @@ -343,7 +348,7 @@ type="primary" form={$formId} disabled={$submitting || - !hasChanges || + !hasNewChanges || !isEnvironmentSelected || hasExistingKeys || $allErrors.length > 0} diff --git a/web-admin/src/features/projects/environment-variables/EnvironmentVariablesTable.svelte b/web-admin/src/features/projects/environment-variables/EnvironmentVariablesTable.svelte index 2d6300dc235..e896e6d9222 100644 --- a/web-admin/src/features/projects/environment-variables/EnvironmentVariablesTable.svelte +++ b/web-admin/src/features/projects/environment-variables/EnvironmentVariablesTable.svelte @@ -8,10 +8,11 @@ import KeyCell from "./KeyCell.svelte"; import ValueCell from "./ValueCell.svelte"; import ActionsCell from "./ActionsCell.svelte"; + import type { VariableNames } from "./types"; export let data: V1ProjectVariable[]; export let emptyText: string = "No environment variables"; - export let variableNames: string[] = []; + export let variableNames: VariableNames = []; const columns: ColumnDef[] = [ { diff --git a/web-admin/src/features/projects/environment-variables/KeyCell.svelte b/web-admin/src/features/projects/environment-variables/KeyCell.svelte index 1a77da44de9..35280afbf29 100644 --- a/web-admin/src/features/projects/environment-variables/KeyCell.svelte +++ b/web-admin/src/features/projects/environment-variables/KeyCell.svelte @@ -5,8 +5,8 @@ export let name: string; // NOTE: if environment is empty, the variable is shared for all environments - function getEnvironmentLabel(environment: string) { - if (environment === "") { + function getEnvironmentType(environment: string) { + if (environment === EnvironmentType.UNDEFINED) { return "Development, Production"; } if (environment === EnvironmentType.DEVELOPMENT) { @@ -18,7 +18,7 @@ return ""; } - $: environmentLabel = getEnvironmentLabel(environment); + $: environmentLabel = getEnvironmentType(environment);
diff --git a/web-admin/src/features/projects/environment-variables/types.ts b/web-admin/src/features/projects/environment-variables/types.ts index b0f40cebe28..2fdaba1b836 100644 --- a/web-admin/src/features/projects/environment-variables/types.ts +++ b/web-admin/src/features/projects/environment-variables/types.ts @@ -7,7 +7,7 @@ export enum EnvironmentType { export type EnvironmentTypes = "dev" | "prod" | ""; -export type EnvironmentVariable = { - key: string; - value: string; -}; +export type VariableNames = { + environment: string; + name: string; +}[]; diff --git a/web-admin/src/features/projects/environment-variables/utils.ts b/web-admin/src/features/projects/environment-variables/utils.ts new file mode 100644 index 00000000000..026a867c469 --- /dev/null +++ b/web-admin/src/features/projects/environment-variables/utils.ts @@ -0,0 +1,77 @@ +import { EnvironmentType, type VariableNames } from "./types"; + +/** + * + * - Environment it belongs to ("prod" or "dev"). + * - If empty, then the value is used as the fallback for all environments. + */ +export function getEnvironmentType(environment: string) { + return environment === EnvironmentType.UNDEFINED ? "" : environment; +} + +/** + * Checks if a key would create a duplicate environment variable. + * + * Rules for environment variables: + * 1. A key can be tied to multiple environment configurations, e.g. my_key[dev] & my_key[prod] + * 2. If a key exists in all environments (UNDEFINED), it cannot be created in dev or prod + * 3. If a key exists in dev or prod, it cannot be created in all environments + * 4. If a key exists in dev, it cannot be created in dev but CAN be created in prod + * + * Examples: + * - If test[dev] exists: + * - Cannot create test[dev] (same environment) + * - Cannot create test[all] (key already used) + * - CAN create test[prod] (different environment) + * - If test[all] exists: + * - Cannot create test[dev] (key used in all environments) + * - Cannot create test[prod] (key used in all environments) + * + * @param environment - The target environment ("development", "production", or "undefined" for all) + * @param key - The environment variable key to check + * @param variableNames - Existing environment variables + * @returns true if the key would create a duplicate, false otherwise + */ +export function isDuplicateKey( + environment: string, + key: string, + variableNames: VariableNames, +): boolean { + return variableNames.some((variable) => { + // Only consider it a duplicate if the same key exists + if (variable.name === key) { + // If either the existing or new variable is for all environments, it's a duplicate + if ( + variable.environment === EnvironmentType.UNDEFINED || + environment === EnvironmentType.UNDEFINED + ) { + return true; + } + + // If trying to create in the same environment as existing variable + if (variable.environment === environment) { + return true; + } + } + return false; + }); +} + +export function getCurrentEnvironment( + isDevelopment: boolean, + isProduction: boolean, +) { + if (isDevelopment && isProduction) { + return EnvironmentType.UNDEFINED; + } + + if (isDevelopment) { + return EnvironmentType.DEVELOPMENT; + } + + if (isProduction) { + return EnvironmentType.PRODUCTION; + } + + return EnvironmentType.UNDEFINED; +} diff --git a/web-admin/src/routes/[organization]/[project]/-/settings/environment-variables/+page.svelte b/web-admin/src/routes/[organization]/[project]/-/settings/environment-variables/+page.svelte index 715a4816f93..f209f81687b 100644 --- a/web-admin/src/routes/[organization]/[project]/-/settings/environment-variables/+page.svelte +++ b/web-admin/src/routes/[organization]/[project]/-/settings/environment-variables/+page.svelte @@ -15,6 +15,7 @@ EnvironmentType, type EnvironmentTypes, } from "@rilldata/web-admin/features/projects/environment-variables/types"; + import { getEnvironmentType } from "@rilldata/web-admin/features/projects/environment-variables/utils"; let open = false; let searchText = ""; @@ -34,7 +35,12 @@ $: projectVariables = $getProjectVariables.data?.variables || []; - $: variableNames = projectVariables.map((variable) => variable.name); + $: variableNames = projectVariables.map((variable) => { + return { + environment: getEnvironmentType(variable.environment), + name: variable.name, + }; + }); $: searchedVariables = projectVariables.filter((variable) => variable.name.toLowerCase().includes(searchText.toLowerCase()),