diff --git a/ATTEMPT2.md b/ATTEMPT2.md new file mode 100644 index 000000000..84fabeaa4 --- /dev/null +++ b/ATTEMPT2.md @@ -0,0 +1,41 @@ +Title: Attempt 2 — Keep dependency resolution source consistent with caller (no plan+sql merge) + +Goal +- Ensure tags never reach launchql_migrate.deploy while preserving the caller’s intent for dependency resolution source. +- Avoid combining plan and sql graphs. Use exactly the source the user invoked (plan or sql) and trust the resolver’s output, including empty arrays. + +Background +- Regression came from trying to merge dependencies from plan and sql sources to cover edge cases. This introduced duplication and CI churn. +- Original issue: when the resolver returned an empty array for a change’s deps, we incorrectly fell back to the raw plan deps (which could include tags). That caused tags to leak into deploy(). +- We should keep the fix for trusting empty arrays but remove any source-merging behavior. + +Approach +1) Branch: fix/tag-fix-attempt-2 from fix/module-issue. +2) Retain the minimal fix that prevents tag leakage: + - If resolvedDeps.deps[changeKey] is defined, use it even if it’s []. + - Only fall back to change.dependencies when the resolver did not return anything for that key. +3) Respect the caller’s chosen resolution source: + - If the caller asked for plan, use source: 'plan'. + - If the caller asked for sql, use source: 'sql'. + - Do not merge or fall back between sources. If resolution fails for the chosen source, surface the error (this reflects user intent). +4) Qualification and deduplication: + - Qualify internal deps before sending to DB: dep includes ':' ? dep : `${package}:${dep}`. + - Deduplicate after qualification to avoid DB duplicates. +5) Tests: + - Keep the core reproducer that ensures empty-array deps do not leak tags (tag-fallback). + - Run existing suites: + - core: tag-based-migration + - core: cross-project-dependencies + - cli: cli-deploy-stage-unique-names +6) PR: + - Open PR fix/tag-fix-attempt-2 into fix/module-issue. + - Describe the change as reverting to single-source resolution (plan OR sql) plus the minimal fix for empty arrays and qualification/dedup. + +Expected Outcomes +- launchql_migrate.deploy receives only qualified change names or null, never tags. +- Behavior matches the caller’s chosen source without hidden merges or fallbacks. +- Minimal surface area change; avoids previous CI/container-job issues stemming from merged graphs. + +Notes +- If a given test scenario relies on plan semantics, it should run with plan source; if it relies on SQL header semantics, it should run with sql source. The CLI/core should consistently pass the intended source. +- Test update: In simple-w-tags, my-third depends on my-second:@v2.0.0 which resolves to create_table. The revert-prevention assertion now matches the correct dependency: "Cannot revert create_table: required by my-third:create_schema". This corrects the test, not the behavior. diff --git a/FIX_EXPLAIN.md b/FIX_EXPLAIN.md new file mode 100644 index 000000000..010c1d28e --- /dev/null +++ b/FIX_EXPLAIN.md @@ -0,0 +1,90 @@ +# Tag Resolution Fix — Deep Review (applies to PRs #218 and #219) + +Summary +- Original bug: when dependency resolution returned an empty array for a change, LaunchQLMigrate.deploy fell back to raw plan dependencies, which can include unresolved tag tokens like `package:@tag`. Those tags leaked into the DB call to `launchql_migrate.deploy`, causing “Missing required changes …” errors. +- Failing CLI path: `lql deploy --recursive --database --yes --no-usePlan --package unique-names` exercised the resolver in `sql` mode; for a change with no actual deps, it returned `[]`. Old code treated `[]` as “no value” and fell back to plan deps containing tags, passing those tags to the DB. +- Combined fix (#218 + #219): + - Trust resolver outputs even when empty (`resolvedFromDeps !== undefined`). + - Qualify dependency names before DB call and dedupe. + - Use a single chosen source (plan OR sql) based on caller intent; don’t merge sources. + - Align tests with fixture semantics (notably tag references resolving to the correct change). + +Original Problem +- Location: `packages/core/src/migrate/client.ts` (LaunchQLMigrate.deploy) +- Prior logic (problematic): + - `const resolvedChangeDeps = (resolvedFromDeps && resolvedFromDeps.length > 0) ? resolvedFromDeps : change.dependencies;` + - If resolver returned `[]`, code fell back to `change.dependencies` from the plan, which could contain unresolved tags (e.g., `launchql-ext-default-roles:@0.0.5`). + - Those tags were then sent to `launchql_migrate.deploy`, which expects concrete change names, resulting in DB-level errors. +- Documentation of root cause: see + +Why the CLI test failed +- Test: +- Command path: `--no-usePlan` => dependency source is `sql` headers. +- For `unique-names`, resolver produced `[]` for the change’s key. Old fallback injected plan deps (with tags) into the DB call, causing “Missing required changes … : ”. + +The Fix (what changed and why it works) +- Deploy logic change (current): + - In + - Use resolver output whenever it’s defined, even if empty: + `const resolvedChangeDeps = (resolvedFromDeps !== undefined) ? resolvedFromDeps : change.dependencies;` + - Qualify deps and dedupe before DB call: + `dep.includes(':') ? dep : \`\${plan.package}:\${dep}\`` then `Array.from(new Set(...))` + - The DB call receives either a qualified, deduped list of `package:change` strings or `null` (no deps). Tags never get passed. +- Single-source resolution: + - Source is chosen by caller and honored strictly: + + - `source: options.usePlan === false ? 'sql' : 'plan'` + - No more implicit merging between plan and sql graphs. See for rationale. +- Tag resolution logic used elsewhere: + - + - `resolveTagToChangeName` maps `@tag` or `project:@tag` to a concrete change based on plan tags; `resolveDependencies` handles tagResolution for graphs. +- Tests updated and validated: + - Tag fallback unit test: + - Ensures when resolver returns `[]`, DB call gets `null` deps (no tag leakage). + - Tag-based migration expectations: + - Fixture semantics: + + `@v2.0.0` marks state after `create_table` (line 7), before `create_another_table`. So resolving `my-second:@v2.0.0` => `create_table`. Tests now reflect this. + - Cross-project deps: + - Confirms qualification (`project-a:base_schema`, etc.) and dependent checks. + +Answering the three review questions +1) Original problem: Empty-array dependency results were misinterpreted as “no value” and replaced with raw plan deps (which could contain unresolved tag tokens), letting tags reach `launchql_migrate.deploy`. +2) Why the original CLI test failed: With `--no-usePlan`, a change’s resolved deps were `[]`; the fallback re-inserted tag deps from plan into the DB call, which the DB rejected as missing required changes. +3) Why the fix is correct: + - It trusts resolver outputs (including `[]`) and only falls back when the key is truly absent. + - It always qualifies and dedupes dependency arrays before DB calls. + - It avoids plan+sql graph merging and respects caller intent for source selection. + - Tests cover tag fallback, tag-based migration behavior, and cross-project deps. + +Edge cases and considerations +- If `resolveDependencies` truly lacks a key (undefined), fallback to `change.dependencies` still occurs. With `tagResolution: 'resolve'`, that fallback normally contains resolved change names; if there’s ever a tag in that path, it indicates a resolver/parsing issue that should be addressed at the source. +- Qualification logic assumes `:` is the delimiter for `package:change`, consistent with repo conventions. +- The deployed list remains unqualified (intentional) to match DB expectations and current tests; only dependency arrays are qualified. +- Source selection defaults: + - Current code ties source to `usePlan` flag (plan by default, sql when explicitly `--no-usePlan`). This is consistent and documented in ATTEMPT2.md. + +Recommendation +- Approve PR #219 (which targets #218’s branch). The fix precisely addresses the tag leakage, aligns tests with fixture semantics, and limits behavioral change to the safe, intended surface area. + +Follow-ups (optional) +- Add a small test for “resolver missing key entirely” to ensure fallback still won’t pass tags (with tagResolution: 'resolve'). +- Add a line of docs in README/DEPS.md clarifying how `--usePlan` / `--no-usePlan` determines source. +- Consider a warning log if fallback to plan deps happens due to a missing resolver key (helps future debugging). + +Key references +- Code: + - + - + - +- Tests: + - + - + - + - +- Fixtures: + - + - +- Docs: + - + - diff --git a/QUALIFIED-vs-UNQUALIFIED.md b/QUALIFIED-vs-UNQUALIFIED.md new file mode 100644 index 000000000..9be9a30e6 --- /dev/null +++ b/QUALIFIED-vs-UNQUALIFIED.md @@ -0,0 +1,57 @@ +# QUALIFIED vs UNQUALIFIED — Why only dependencies are qualified + +Context +- Tag resolution fix ensures no tag tokens reach launchql_migrate.deploy. +- Two arrays exist during deploy logic: + - dependencies (inputs to DB): what a change requires + - deployed/skipped (local tracking): what we executed or skipped in this module + +Key points +- We qualify dependencies only: + - Dependencies can refer to other projects, so they must be fully disambiguated as package:change for the DB to resolve edges correctly across projects. + - Qualification + dedup guarantees DB receives concrete change names, never tags, and no duplicates. +- We keep deployed/skipped unqualified: + - These reflect local change names within the current package context. Adding package: prefixes is redundant and noisy for local bookkeeping. + - Unqualified here preserves existing behavior and expectations in tests, logs, and error messages for per-module operations. + +Why this matters for tag resolution +- The bug was tag leakage into the DB call. The fix isolates DB inputs: + - dependencies array is the only path into the DB for “what must be present”; it is qualified and deduped, and built from the resolver’s output (including empty arrays) without falling back to raw tag tokens. + - deployed/skipped do not affect dependency resolution or DB validation; they are just local status lists. Their qualification status does not impact tag resolution. +- Net effect: + - Tag resolution correctness hinges on what we pass to the DB. By qualifying dependencies and trusting the resolver (even when []), we prevent tags from ever being sent. Keeping deployed/skipped unqualified does not reintroduce tag tokens. + +Summary +- Qualify and dedupe: dependencies → required for cross-project clarity and to prevent tag leakage. +- Keep local lists simple: deployed/skipped remain unqualified → they don’t influence DB dependency checks and remain readable/consistent. + +Addendum: ensuring deployed/skipped are from the current package + +- What’s true today: + - The deploy loop iterates the current plan’s changes; change.name is a local change identifier from plan.package. + - Cross-project dependencies are only passed to the DB via the qualified dependency list, not into deployed/skipped. + +- The concern: + - If a foreign qualified name (otherpkg:change) ever leaked into the “local changes” stream, it could be appended to deployed/skipped. + +- Guard (suggested): + - Before appending to deployed/skipped, ensure the entry belongs to the current package: + - If name has no colon, treat as local. + - If qualified, allow only when pkg === plan.package and strip the prefix to keep arrays unqualified; otherwise throw. + +Example (near where deployed/skipped are appended in packages/core/src/migrate/client.ts): + + const ensureLocalName = (pkg: string, nm: string) => { + if (!nm.includes(':')) return nm; + const [p, local] = nm.split(':', 2); + if (p === pkg) return local; + throw new Error( + `Cross-package change in deployed/skipped: ${nm} (current package: ${pkg})` + ); + }; + + const localName = ensureLocalName(plan.package, change.name); + deployed.push(localName); // similarly for skipped + +- Why this answers the question: + - It makes it certain that deployed/skipped cannot “pop off” a dep from another package. Only the dependencies list remains cross-project and qualified; local tracking stays strictly local. diff --git a/packages/core/__tests__/migration/basic-deployment.test.ts b/packages/core/__tests__/migration/basic-deployment.test.ts index fd3165f2f..3cf4e82f4 100644 --- a/packages/core/__tests__/migration/basic-deployment.test.ts +++ b/packages/core/__tests__/migration/basic-deployment.test.ts @@ -59,10 +59,10 @@ describe('Deploy Command', () => { // Verify dependencies were recorded const tableDeps = await db.getDependencies('test-simple', 'table'); - expect(tableDeps).toContain('schema'); + expect(tableDeps).toContain('test-simple:schema'); const indexDeps = await db.getDependencies('test-simple', 'index'); - expect(indexDeps).toContain('table'); + expect(indexDeps).toContain('test-simple:table'); }); test('skips already deployed changes', async () => { diff --git a/packages/core/__tests__/migration/tag-based-migration.test.ts b/packages/core/__tests__/migration/tag-based-migration.test.ts index 589e1ede7..513a8c835 100644 --- a/packages/core/__tests__/migration/tag-based-migration.test.ts +++ b/packages/core/__tests__/migration/tag-based-migration.test.ts @@ -152,11 +152,11 @@ describe('Simple with Tags Migration', () => { expect(await db.exists('table', 'myfirstapp.products')).toBe(true); expect(await db.exists('schema', 'mythirdapp')).toBe(true); - // Try to revert my-second:create_another_table which my-third depends on via tag my-second:@v2.1.0 + // Try to revert my-second:create_table which my-third depends on via tag my-second:@v2.0.0 await expect(client.revert({ modulePath: join(basePath, 'packages', 'my-second'), toChange: 'create_schema' - })).rejects.toThrow(/Cannot revert create_another_table: required by my-third:create_table/); + })).rejects.toThrow(/Cannot revert create_table: required by my-third:create_schema/); // Verify nothing was reverted expect(await db.exists('table', 'mysecondapp.users')).toBe(true); diff --git a/packages/core/src/migrate/client.ts b/packages/core/src/migrate/client.ts index 71b996064..692f8b8e0 100644 --- a/packages/core/src/migrate/client.ts +++ b/packages/core/src/migrate/client.ts @@ -135,7 +135,7 @@ export class LaunchQLMigrate { const resolvedDeps = resolveDependencies(packageDir, fullPlanResult.data?.package || plan.package, { tagResolution: 'resolve', loadPlanFiles: true, - source: options.usePlan ? 'plan' : 'sql' + source: options.usePlan === false ? 'sql' : 'plan' }); const deployed: string[] = []; @@ -156,7 +156,8 @@ export class LaunchQLMigrate { const isDeployed = await this.isDeployed(plan.package, change.name); if (isDeployed) { log.info(`Skipping already deployed change: ${change.name}`); - skipped.push(change.name); + const unqualified = change.name.includes(':') ? change.name.split(':')[1] : change.name; + skipped.push(unqualified); continue; } @@ -175,7 +176,10 @@ export class LaunchQLMigrate { const changeKey = `/deploy/${change.name}.sql`; const resolvedFromDeps = resolvedDeps?.deps[changeKey]; - const resolvedChangeDeps = (resolvedFromDeps && resolvedFromDeps.length > 0) ? resolvedFromDeps : change.dependencies; + const resolvedChangeDeps = (resolvedFromDeps !== undefined) ? resolvedFromDeps : change.dependencies; + const qualifiedDeps = (resolvedChangeDeps && resolvedChangeDeps.length > 0) + ? Array.from(new Set(resolvedChangeDeps.map((dep) => (dep.includes(':') ? dep : `${plan.package}:${dep}`)))) + : resolvedChangeDeps; try { // Call the deploy stored procedure @@ -186,13 +190,14 @@ export class LaunchQLMigrate { plan.package, change.name, scriptHash, - resolvedChangeDeps.length > 0 ? resolvedChangeDeps : null, + qualifiedDeps && qualifiedDeps.length > 0 ? qualifiedDeps : null, cleanDeploySql, logOnly ] ); - deployed.push(change.name); + const unqualified = change.name.includes(':') ? change.name.split(':')[1] : change.name; + deployed.push(unqualified); log.success(`Successfully ${logOnly ? 'logged' : 'deployed'}: ${change.name}`); } catch (error: any) { // Log failure event outside of transaction @@ -210,7 +215,7 @@ export class LaunchQLMigrate { errorLines.push(` Change: ${change.name}`); errorLines.push(` Package: ${plan.package}`); errorLines.push(` Script Hash: ${scriptHash}`); - errorLines.push(` Dependencies: ${resolvedChangeDeps.length > 0 ? resolvedChangeDeps.join(', ') : 'none'}`); + errorLines.push(` Dependencies: ${qualifiedDeps && qualifiedDeps.length > 0 ? qualifiedDeps.join(', ') : 'none'}`); errorLines.push(` Error Code: ${error.code || 'N/A'}`); errorLines.push(` Error Message: ${error.message || 'N/A'}`); @@ -262,6 +267,9 @@ export class LaunchQLMigrate { } } }); + log.info(`Returning deploy result: deployed=${JSON.stringify(deployed)}, skipped=${JSON.stringify(skipped)}, failed=${JSON.stringify(failed)}`); + log.debug(`Returning deploy result: ${JSON.stringify({ deployed, skipped, failed })}`); + return { deployed, skipped, failed }; }