Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
83d40a1
Add undefined target handling for deploy/revert/verify operations
devin-ai-integration[bot] Jul 21, 2025
1a74d3c
Fix redundant condition logic in moduleToChange assignment
devin-ai-integration[bot] Jul 21, 2025
9fdf379
Remove unnecessary parentheses from ternary condition
devin-ai-integration[bot] Jul 21, 2025
3d4a532
Refactor module selection and extension collection logic
devin-ai-integration[bot] Jul 21, 2025
4b9ef2d
Fix moduleToChange logic: remove incorrect null handling
devin-ai-integration[bot] Jul 22, 2025
c71f305
Refactor getAllWorkspaceExtensions to use proper dependency ordering
devin-ai-integration[bot] Jul 24, 2025
3eda30a
Refactor workspace dependency ordering to use virtual module pattern
devin-ai-integration[bot] Jul 24, 2025
0dc54d2
Add CHANGES.md with detailed summary
devin-ai-integration[bot] Jul 25, 2025
88e16eb
Add tests for getWorkspaceExtensionsInDependencyOrder
devin-ai-integration[bot] Jul 25, 2025
17bb30a
Fix duplicate entries in getWorkspaceExtensionsInDependencyOrder
devin-ai-integration[bot] Jul 28, 2025
95b9122
Rename extDeps to resolveExtensionDependencies
devin-ai-integration[bot] Jul 28, 2025
b3484d8
Simplify getWorkspaceExtensionsInDependencyOrder to use filtered reso…
devin-ai-integration[bot] Jul 28, 2025
b914b47
Add RECOMMENDATION.md with simplification analysis
devin-ai-integration[bot] Jul 28, 2025
c9fdcde
Rename method to resolveWorkspaceExtensionDependencies
devin-ai-integration[bot] Jul 28, 2025
36f423c
Fix CI failure: Update test references to renamed method
devin-ai-integration[bot] Jul 28, 2025
5682aa7
rm
pyramation Jul 28, 2025
48969d6
rm
pyramation Jul 28, 2025
7805d68
Add comprehensive documentation to findTopDependentModule method
devin-ai-integration[bot] Jul 28, 2025
7270ae8
Add EXPLAIN_FIND_TOP.md with analysis of findTopDependentModule
devin-ai-integration[bot] Jul 29, 2025
2f22b88
Remove findTopDependentModule and replace with consistent dependency …
devin-ai-integration[bot] Jul 29, 2025
edc6837
Fix inconsistent reverse() usage in revert logic
devin-ai-integration[bot] Jul 29, 2025
766fe83
reverse
pyramation Jul 29, 2025
55f1db2
Fix revert logic to match verify pattern
devin-ai-integration[bot] Jul 29, 2025
b2daefc
Fix CI failure: Restore toChange special case in revert for proper de…
devin-ai-integration[bot] Jul 29, 2025
7948034
Add comprehensive documentation for toChange special case in revert
devin-ai-integration[bot] Jul 29, 2025
bf768f7
Add revert truncation logic and tests
devin-ai-integration[bot] Jul 29, 2025
e8a7ea7
Fix truncation example in documentation
devin-ai-integration[bot] Jul 29, 2025
416e28f
Add revert truncation logic and tests
devin-ai-integration[bot] Jul 29, 2025
5518237
Fix pg-cache lifecycle issue in test cleanup
devin-ai-integration[bot] Jul 29, 2025
53604e6
Fix revert logic to always use workspace-wide resolution
devin-ai-integration[bot] Jul 29, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
164 changes: 164 additions & 0 deletions REVERT_TOCHANGE_EXPLANATION.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
# Explanation of toChange Special Case in Revert Method

## Executive Summary

The `toChange` special case in the `LaunchQLProject.revert()` method uses workspace-wide resolution to ensure database safety when reverting specific changes. This document explains why this approach is necessary and correct, addressing concerns about why it differs from the verify method's approach.

## The Problem: Dependency Chain Violations

### Test Fixture Analysis

From the test fixtures in `__fixtures__/sqitch/simple-w-tags/`:

1. **my-first** module has tags:
- `@v1.0.0` (after table_users)
- `@v1.1.0` (after table_products)

2. **my-third:create_schema** depends on `my-first:@v1.1.0` (which includes table_products)

3. **Test scenario**: `revertModule('my-first:@v1.0.0')` tries to revert my-first back to before table_products existed

4. **The Problem**: If we only revert my-first using module-specific resolution:
- my-first gets reverted to @v1.0.0 (table_products is removed)
- my-third:create_schema still exists in the database
- my-third:create_schema depends on table_products that no longer exists
- Result: Database constraint violation "Cannot revert table_products: required by my-third:create_schema"

### Why Workspace-Wide Resolution Solves This

The workspace-wide resolution (`this.resolveWorkspaceExtensionDependencies()`) ensures:

1. **Dependency Discovery**: All modules in the workspace are analyzed for dependencies
2. **Proper Ordering**: my-third gets reverted BEFORE my-first
3. **Safe Revert**: table_products can be safely removed after dependent modules are gone

## Database Safety Requirements

### Revert Operations Modify State

Unlike verify operations, revert operations:
- **Modify database state** by removing tables, functions, schemas, etc.
- **Must respect dependency constraints** enforced by PostgreSQL
- **Require careful ordering** to prevent "Cannot drop X: required by Y" errors

### PostgreSQL Constraint Enforcement

PostgreSQL enforces referential integrity and dependency constraints:
- Foreign key constraints prevent dropping referenced tables
- Function dependencies prevent dropping functions used by other objects
- Schema dependencies prevent dropping schemas containing referenced objects

## Why Verify Doesn't Need This Special Case

### Verify Operations Are Read-Only

The verify method:
- **Only checks existence** of database objects
- **Doesn't modify state** so no dependency ordering concerns
- **Can safely use module-specific resolution** since it's just validation

### Different Use Cases

| Operation | Purpose | State Change | Dependency Concerns |
|-----------|---------|--------------|-------------------|
| **Verify** | Check if modules exist in database | None | None |
| **Revert** | Remove database objects | Yes | Critical |

## Alternative Approaches Considered

### Option 1: Module-Specific Resolution (Like Verify)
```typescript
// This would fail with dependency violations
const moduleProject = this.getModuleProject(name);
extensionsToRevert = moduleProject.getModuleExtensions();
```

**Problems**:
- Fails with "Cannot revert X: required by Y" errors
- Doesn't account for cross-module dependencies
- Unsafe for database operations

### Option 2: Targeted Dependent Module Resolution
```typescript
// Find only modules that depend on the target
const dependentModules = findModulesThatDependOn(name);
extensionsToRevert = resolveDependencies(dependentModules);
```

**Problems**:
- More complex to implement and maintain
- Similar results to workspace-wide resolution in practice
- Additional complexity without clear benefits

### Option 3: Dependency-Aware Revert Order
```typescript
// Track cross-module dependencies at specific versions
const versionAwareDeps = resolveVersionSpecificDependencies(name, toChange);
extensionsToRevert = orderByDependencies(versionAwareDeps);
```

**Problems**:
- Significantly more complex implementation
- Requires tracking version-specific cross-module dependencies
- Overkill for the current use case

## Code Implementation Details

### Current Implementation
```typescript
} else if (toChange) {
// When reverting to a specific change (toChange), we need workspace-wide resolution
// to prevent "Cannot revert X: required by Y" database dependency violations.
extensionsToRevert = this.resolveWorkspaceExtensionDependencies();
}
```

### Why This Works

1. **Comprehensive Scope**: `resolveWorkspaceExtensionDependencies()` creates a virtual module that depends on all workspace modules
2. **Proper Ordering**: Returns modules in dependency order (dependencies before dependents)
3. **Reverse Execution**: The revert loop processes in reverse order, ensuring dependents are reverted first

## Test Evidence

### Failing Test Without Special Case
```
LaunchQLError: Revert failed for module: my-first
Cannot revert table_products: required by my-third:create_schema
```

### Passing Test With Special Case
The workspace-wide resolution ensures my-third is reverted before my-first, allowing table_products to be safely removed.

## Conclusion

The `toChange` special case in the revert method is **correct and necessary** for database safety. While it may seem inconsistent with the verify pattern, the fundamental difference between state-modifying and read-only operations justifies the different approaches.

### Key Insights

1. **Revert operations require dependency-aware ordering** due to database constraints
2. **Verify operations don't need this complexity** since they're read-only
3. **Workspace-wide resolution is the most robust approach** for ensuring safe revert operations
4. **The special case prevents real database errors** that would occur with simpler approaches

### Recommendation

**Keep the current implementation** as it correctly handles the complex dependency scenarios that arise when reverting specific changes across modules with inter-dependencies.

## Truncation Optimization

### Performance Improvement

The revert method now includes truncation logic to avoid processing unnecessary modules beyond the target. When reverting with a toChange parameter:

1. **Workspace-wide resolution** is still used for dependency safety
2. **Truncation is applied** before reversal to remove modules that come after the target
3. **Reversal proceeds** with only the necessary modules

### Example

When reverting `my-second:@v2.0.0` in a workspace with `[my-first, my-second, my-third]`:
- **Before truncation**: Process all modules `[my-third, my-second, my-first]` (reversed)
- **After truncation**: Process only `[my-third, my-second]` (reversed)

This prevents unnecessary processing of `my-first` while maintaining database safety through proper dependency ordering. We must revert `my-third` first since it depends on `my-second`.
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`getWorkspaceExtensionsInDependencyOrder with complex existing fixtures handles different fixture types consistently 1`] = `
{
"external": [
"citext",
"plpgsql",
"pgcrypto",
],
"resolved": [
"citext",
"plpgsql",
"pgcrypto",
"my-first",
"my-second",
"my-third",
],
}
`;

exports[`getWorkspaceExtensionsInDependencyOrder with complex existing fixtures returns extensions in dependency order for complex workspace 1`] = `
{
"external": [
"plpgsql",
"uuid-ossp",
"pgcrypto",
],
"resolved": [
"plpgsql",
"uuid-ossp",
"pg-utilities",
"pg-verify",
"pgcrypto",
"totp",
"utils",
"secrets",
],
}
`;

exports[`getWorkspaceExtensionsInDependencyOrder with complex existing fixtures verifies dependency ordering properties 1`] = `
{
"external": [
"citext",
"plpgsql",
"pgcrypto",
],
"resolved": [
"citext",
"plpgsql",
"pgcrypto",
"my-first",
"my-second",
"my-third",
],
}
`;

exports[`getWorkspaceExtensionsInDependencyOrder with existing fixtures handles workspace with minimal modules 1`] = `
{
"external": [
"citext",
"plpgsql",
"pgcrypto",
],
"resolved": [
"citext",
"plpgsql",
"pgcrypto",
"my-first",
"my-second",
"my-third",
],
}
`;

exports[`getWorkspaceExtensionsInDependencyOrder with existing fixtures returns extensions in dependency order for launchql workspace 1`] = `
{
"external": [
"plpgsql",
"uuid-ossp",
"pgcrypto",
],
"resolved": [
"plpgsql",
"uuid-ossp",
"pg-utilities",
"pg-verify",
"pgcrypto",
"totp",
"utils",
"secrets",
],
}
`;

exports[`getWorkspaceExtensionsInDependencyOrder with existing fixtures returns extensions in dependency order for simple-w-tags workspace 1`] = `
{
"external": [
"citext",
"plpgsql",
"pgcrypto",
],
"resolved": [
"citext",
"plpgsql",
"pgcrypto",
"my-first",
"my-second",
"my-third",
],
}
`;
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
process.env.LAUNCHQL_DEBUG = 'true';

import { LaunchQLProject } from '../../src/core/class/launchql';
import { TestFixture } from '../../test-utils';

let fixture: TestFixture;

beforeAll(() => {
fixture = new TestFixture('sqitch');
});

afterAll(() => {
fixture.cleanup();
});

describe('getWorkspaceExtensionsInDependencyOrder', () => {
describe('with existing fixtures', () => {
it('returns extensions in dependency order for simple-w-tags workspace', () => {
const workspacePath = fixture.getFixturePath('simple-w-tags');
const project = new LaunchQLProject(workspacePath);

const result = project.resolveWorkspaceExtensionDependencies();
expect(result).toMatchSnapshot();
});

it('returns extensions in dependency order for launchql workspace', () => {
const workspacePath = fixture.getFixturePath('launchql');
const project = new LaunchQLProject(workspacePath);

const result = project.resolveWorkspaceExtensionDependencies();
expect(result).toMatchSnapshot();
});

it('handles workspace with minimal modules', () => {
const workspacePath = fixture.getFixturePath('simple');
const project = new LaunchQLProject(workspacePath);

const result = project.resolveWorkspaceExtensionDependencies();
expect(result).toMatchSnapshot();

expect(result).toHaveProperty('resolved');
expect(result).toHaveProperty('external');
});
});

describe('with complex existing fixtures', () => {
it('returns extensions in dependency order for complex workspace', () => {
const workspacePath = fixture.getFixturePath('launchql');
const project = new LaunchQLProject(workspacePath);

const result = project.resolveWorkspaceExtensionDependencies();
expect(result).toMatchSnapshot();

expect(Array.isArray(result.resolved)).toBe(true);
expect(Array.isArray(result.external)).toBe(true);
});

it('verifies dependency ordering properties', () => {
const workspacePath = fixture.getFixturePath('simple-w-tags');
const project = new LaunchQLProject(workspacePath);

const result = project.resolveWorkspaceExtensionDependencies();
expect(result).toMatchSnapshot();

expect(result).toHaveProperty('resolved');
expect(result).toHaveProperty('external');
expect(Array.isArray(result.resolved)).toBe(true);
expect(Array.isArray(result.external)).toBe(true);

expect(result.resolved.length).toBeGreaterThanOrEqual(0);
expect(result.external.length).toBeGreaterThanOrEqual(0);
});

it('handles different fixture types consistently', () => {
const workspacePath = fixture.getFixturePath('simple');
const project = new LaunchQLProject(workspacePath);

const result = project.resolveWorkspaceExtensionDependencies();
expect(result).toMatchSnapshot();

expect(result.resolved.length).toBeGreaterThan(0);
});
});
});
Loading