diff --git a/frontend/src/lib/components/ScriptBuilder.svelte b/frontend/src/lib/components/ScriptBuilder.svelte index f209907537..da358154ab 100644 --- a/frontend/src/lib/components/ScriptBuilder.svelte +++ b/frontend/src/lib/components/ScriptBuilder.svelte @@ -43,6 +43,7 @@ import { invalidateWorkspacePaths } from './PathNameAutocomplete.svelte' import { notifyContractWarnings } from './assets/AssetGraph/schemaContracts' import ScriptEditor from './ScriptEditor.svelte' + import { findModulePathClash } from './scriptModulePath' import { Alert, Button, Drawer, SecondsInput, Tab, TabContent, Tabs } from './common' import LanguageIcon from './common/languageIcons/LanguageIcon.svelte' import type { SupportedLanguage, Schema } from '$lib/common' @@ -984,7 +985,10 @@ // draft that grew modules under another language carries none of what dbt // needs, and the worker refuses a bundle with no `dbt_project.yml` — so // that draft reached dbt in a state it could neither deploy nor run. - if (script.modules?.['dbt_project.yml']) return + // Matched on the canonical path: a bundle pushed with `./dbt_project.yml` + // already has the project file, and seeding a second spelling of it is the + // two-keys-one-file collision the editor's add-file checks refuse. + if (findModulePathClash(script.modules, 'dbt_project.yml')) return script.modules = { 'dbt_project.yml': { content: diff --git a/frontend/src/lib/components/ScriptEditor.svelte b/frontend/src/lib/components/ScriptEditor.svelte index e112f92b3b..174753850e 100644 --- a/frontend/src/lib/components/ScriptEditor.svelte +++ b/frontend/src/lib/components/ScriptEditor.svelte @@ -23,6 +23,7 @@ dbtFileLang, dbtModelSelector } from '$lib/components/dbt/DbtProjectPanel.svelte' + import { canonicalModulePath, findModulePathClash } from './scriptModulePath' import SchemaForm from './SchemaForm.svelte' import PowerShellCommonParams from './PowerShellCommonParams.svelte' import LogPanel from './scriptEditor/LogPanel.svelte' @@ -533,35 +534,43 @@ /// The descriptor is the script's CONTENT, not a module. A module at that same /// path would be a second, independent value for one file: the export writes /// the content there, and the bundle would emit over it. - function reservedDbtPath(path: string): string | undefined { - return lang === 'dbt' && path.trim() === 'wm_dbt.yaml' + function reservedDbtPath(canonicalPath: string): string | undefined { + return lang === 'dbt' && canonicalPath === 'wm_dbt.yaml' ? `wm_dbt.yaml is the descriptor, edited from the tree — it cannot also be a file` : undefined } function validateModulePath(path: string): string { if (!path.trim()) return '' - const reserved = reservedDbtPath(path) + const canonical = canonicalModulePath(path) + if ('error' in canonical) return canonical.error + const reserved = reservedDbtPath(canonical.path) if (reserved) return reserved - const moduleLang = inferModuleLang(path) + const moduleLang = inferModuleLang(canonical.path) if (!moduleLang) { const exts = allowedModuleExtensions.join(', ') return `File must end with a supported extension: ${exts}` } - const matchedExt = allowedModuleExtensions.find((ext) => path.endsWith(ext)) + const matchedExt = allowedModuleExtensions.find((ext) => canonical.path.endsWith(ext)) if (!matchedExt) { const exts = allowedModuleExtensions.join(', ') return `File must end with a supported extension for this language: ${exts}` } - if (modules?.[path.trim()]) { - return `Module ${path.trim()} already exists` + const clash = findModulePathClash(modules, canonical.path) + if (clash) { + return `Module ${clash} already exists` } return '' } function addModule() { - const modulePath = modulePathInput.trim() - if (!modulePath) return + if (!modulePathInput.trim()) return + const canonical = canonicalModulePath(modulePathInput) + if ('error' in canonical) { + modulePathError = canonical.error + return + } + const modulePath = canonical.path const error = validateModulePath(modulePath) if (error) { modulePathError = error @@ -592,29 +601,43 @@ function validateRenameModulePath(newPath: string, oldPath: string): string { if (!newPath.trim()) return '' - const reserved = reservedDbtPath(newPath) + const canonical = canonicalModulePath(newPath) + if ('error' in canonical) return canonical.error + const reserved = reservedDbtPath(canonical.path) if (reserved) return reserved - const moduleLang = inferModuleLang(newPath) + const moduleLang = inferModuleLang(canonical.path) if (!moduleLang) { const exts = allowedModuleExtensions.join(', ') return `File must end with a supported extension: ${exts}` } - const matchedExt = allowedModuleExtensions.find((ext) => newPath.endsWith(ext)) + const matchedExt = allowedModuleExtensions.find((ext) => canonical.path.endsWith(ext)) if (!matchedExt) { const exts = allowedModuleExtensions.join(', ') return `File must end with a supported extension for this language: ${exts}` } - if (newPath.trim() !== oldPath && modules?.[newPath.trim()]) { - return `Module ${newPath.trim()} already exists` + const clash = findModulePathClash(modules, canonical.path, oldPath) + if (clash) { + return `Module ${clash} already exists` } return '' } + /// A spelling of the name the module already has. Nothing to do, so the button + /// that would submit it stays disabled rather than being a dead click. + function renameIsNoop(input: string, oldPath: string): boolean { + const canonical = canonicalModulePath(input) + return 'path' in canonical && canonical.path === oldPath + } + function renameModule(oldPath: string) { - const newPath = renameModuleInput.trim() - if (!newPath || newPath === oldPath) { + if (!renameModuleInput.trim()) return + const canonical = canonicalModulePath(renameModuleInput) + if ('error' in canonical) { + renameModuleError = canonical.error return } + const newPath = canonical.path + if (newPath === oldPath) return const error = validateRenameModulePath(newPath, oldPath) if (error) { renameModuleError = error @@ -2590,7 +2613,7 @@ close() }} disabled={!renameModuleInput.trim() || - renameModuleInput.trim() === oldPath || + renameIsNoop(renameModuleInput, oldPath) || !!renameModuleError}>Rename diff --git a/frontend/src/lib/components/scriptModulePath.test.ts b/frontend/src/lib/components/scriptModulePath.test.ts new file mode 100644 index 0000000000..d90624d10d --- /dev/null +++ b/frontend/src/lib/components/scriptModulePath.test.ts @@ -0,0 +1,57 @@ +import { describe, it, expect } from 'vitest' +import { canonicalModulePath, findModulePathClash } from './scriptModulePath' + +describe('canonicalModulePath', () => { + // The pair the duplicate and reserved-name checks exist to catch: both + // spellings resolve to the same file in the job directory. + it('rewrites a redundant spelling to the file it names', () => { + expect(canonicalModulePath('./dbt_project.yml')).toEqual({ path: 'dbt_project.yml' }) + expect(canonicalModulePath('models//x.sql')).toEqual({ path: 'models/x.sql' }) + expect(canonicalModulePath(' ./models/./sub//x.sql ')).toEqual({ + path: 'models/sub/x.sql' + }) + expect(canonicalModulePath('models/x.sql')).toEqual({ path: 'models/x.sql' }) + }) + + it('refuses a path that leaves the bundle', () => { + expect(canonicalModulePath('../x.sql')).toHaveProperty('error') + expect(canonicalModulePath('models/../../x.sql')).toHaveProperty('error') + expect(canonicalModulePath('/etc/x.sql')).toHaveProperty('error') + expect(canonicalModulePath('./')).toHaveProperty('error') + }) + + // Matches the worker's own rule: `..` is traversal only as a whole segment. + it('takes dots inside a name as part of the name', () => { + expect(canonicalModulePath('models/weird..name.sql')).toEqual({ + path: 'models/weird..name.sql' + }) + }) +}) + +describe('findModulePathClash', () => { + // A bundle pushed by the CLI can hold a non-canonical key, so the clash has to + // be found from either side, and named the way the tree shows it. + it('finds a key that resolves to the same file, however either is spelled', () => { + const modules = { './dbt_project.yml': {}, 'models/x.sql': {} } + expect(findModulePathClash(modules, 'dbt_project.yml')).toBe('./dbt_project.yml') + expect(findModulePathClash(modules, 'models/x.sql')).toBe('models/x.sql') + expect(findModulePathClash(modules, 'models/y.sql')).toBeUndefined() + expect(findModulePathClash(undefined, 'models/x.sql')).toBeUndefined() + }) + + // The worker does not trim path components, so an imported `x.sql ` is its + // own file and must not stand in the way of adding `x.sql`. + it('does not fold a key whose name carries whitespace', () => { + expect(findModulePathClash({ 'models/x.sql ': {} }, 'models/x.sql')).toBeUndefined() + }) + + // A rename must not stop at the module being renamed: with both spellings in + // the bundle, that would hide the other one and overwrite its content. + it('keeps looking past the key being renamed', () => { + const modules = { './models/x.sql': {}, 'models/x.sql': {} } + expect(findModulePathClash(modules, 'models/x.sql', './models/x.sql')).toBe('models/x.sql') + expect( + findModulePathClash({ './models/x.sql': {} }, 'models/x.sql', './models/x.sql') + ).toBeUndefined() + }) +}) diff --git a/frontend/src/lib/components/scriptModulePath.ts b/frontend/src/lib/components/scriptModulePath.ts new file mode 100644 index 0000000000..63271135c6 --- /dev/null +++ b/frontend/src/lib/components/scriptModulePath.ts @@ -0,0 +1,64 @@ +function canonicalize(path: string): { path: string } | { error: string } { + if (path.startsWith('/')) { + return { error: `File path must be relative, without a leading /` } + } + const segments = path.split('/').filter((s) => s !== '' && s !== '.') + if (segments.includes('..')) { + return { error: `File path cannot contain ..` } + } + if (segments.length === 0) { + return { error: `File name cannot be empty` } + } + return { path: segments.join('/') } +} + +/** + * The canonical spelling of a script module's path (the key of the module + * bundle), or the reason it cannot be one. + * + * The worker resolves `.` and `//` away when it materialises the bundle into + * the job directory, so `./dbt_project.yml` and `dbt_project.yml` are two keys + * for one file on disk; the bundle is a Rust `HashMap`, so which content lands + * there is undefined. Canonicalising before the duplicate and reserved-name + * checks is what keeps them from being walked past. + * + * Redundant spellings are rewritten rather than refused: they name the file the + * user meant, and the tree shows the canonical form regardless. `..` and + * absolute paths name a file OUTSIDE the bundle, which has no canonical form + * inside it, so they are refused here — the worker drops them, which would + * otherwise show up as a file that was added and then silently never written. + * + * Surrounding whitespace is dropped because this is what someone typed into a + * text box. A key already in a bundle gets no such courtesy — see + * `findModulePathClash`. + */ +export function canonicalModulePath(path: string): { path: string } | { error: string } { + return canonicalize(path.trim()) +} + +/** + * The existing module key that would land on the same file as `canonicalPath`, + * spelled as the bundle holds it (which is what the file tree shows). + * + * Keys already in the bundle are not canonical either: nothing on the push path + * rewrites them, so a project imported with `./dbt_project.yml` in it must still + * refuse a second `dbt_project.yml`. They are matched WITHOUT trimming, because + * the worker does not trim path components: an imported `x.sql ` is its own file + * on disk and must not stand in the way of adding `x.sql`. + * + * `ignoreKey` is the module being renamed. It has to be skipped inside the + * search rather than compared against the result: a bundle can hold BOTH + * spellings, and stopping at the renamed one would hide the other and let the + * rename overwrite it. + */ +export function findModulePathClash( + modules: Record | null | undefined, + canonicalPath: string, + ignoreKey?: string +): string | undefined { + return Object.keys(modules ?? {}).find((key) => { + if (key === ignoreKey) return false + const canonical = canonicalize(key) + return 'path' in canonical && canonical.path === canonicalPath + }) +}