Conversation
- Delete orphaned database schema files (audit-schema.sql, migrations.sql, rbac-schema.sql) - Rename initial migration to 000_initial_schema.sql for clarity - Update DatabaseService to load only numbered migrations instead of base schema files - Move all schema definitions to the migrations directory as single source of truth - Add documentation in copilot-instructions.md explaining migration-first approach - Create internal task documentation for database schema cleanup process - Eliminate duplicate table definitions that existed across base schemas and migrations
…a file - Update COPY instruction to include all database files and migrations - Change from copying only schema.sql to copying entire src/database/ directory - Ensures future database-related files are automatically included in builds - Improves maintainability by avoiding need to update Dockerfile when new database files are added
There was a problem hiding this comment.
Pull request overview
This PR consolidates the backend SQLite schema management into a migration-first workflow, removing legacy/orphan schema files and updating build/runtime packaging plus documentation to match the new approach.
Changes:
- Refactors
DatabaseServiceinitialization to run only numbered SQL migrations viaMigrationRunner. - Removes legacy standalone schema files (
rbac-schema.sql,audit-schema.sql,migrations.sql) and updates build/Docker packaging to include migrations. - Updates documentation/instructions to describe the migration-first policy.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/development/BACKEND_CODE_ANALYSIS.md | Updates backend DB documentation to reflect migration-first schema management. |
| backend/src/database/rbac-schema.sql | Deletes legacy RBAC base schema file (now covered by migrations). |
| backend/src/database/audit-schema.sql | Deletes legacy audit schema file (now covered by migrations). |
| backend/src/database/migrations.sql | Deletes legacy monolithic migration file (superseded by numbered migrations). |
| backend/src/database/migrations/000_initial_schema.sql | Adds header comments describing migration 000 purpose. |
| backend/src/database/DatabaseService.ts | Removes base-schema loading logic; runs migrations only via MigrationRunner. |
| backend/package.json | Updates build step to copy only the migrations directory into dist/. |
| Dockerfile | Updates runtime image to include database SQL/migration files. |
| CLAUDE.md | Updates repo guidance to match migration-first DB approach. |
| .kiro/todo/database-schema-cleanup-task.md | Adds a task write-up documenting the cleanup and final state. |
| .kiro/database-cleanup-prompt.md | Adds a prompt document for the cleanup effort (currently contains outdated “current state” text). |
| .github/copilot-instructions.md | Updates Copilot instructions to describe migration-first DB schema workflow. |
Comments suppressed due to low confidence (1)
backend/src/database/migrations/000_initial_schema.sql:6
revoked_tokensis created in migration 000 without a foreign key tousers, but migration 001 also definesrevoked_tokenswith a FK. Because both useCREATE TABLE IF NOT EXISTS, a fresh DB will keep the migration 000 version and silently miss the FK (and may duplicate indexes). Consolidaterevoked_tokensinto a single migration (preferably afterusersexists) and, if needed, add a follow-up migration to rebuild/upgrade existing databases safely.
| * Initialize database schema using migration-first approach | ||
| * | ||
| * Schema Management Policy: | ||
| * - ALL schema definitions are in numbered migrations (migrations/*.sql) | ||
| * - Migration 000: Initial schema (executions, revoked_tokens) | ||
| * - Migration 001: RBAC tables (users, roles, permissions, groups) | ||
| * - Migration 002+: All subsequent schema changes |
There was a problem hiding this comment.
With the switch to migration-first (and removal of schema.sql), there are still backend tests/utilities that load src/database/schema.sql directly. Those will fail and should be updated to apply migration 000 (or run MigrationRunner) to set up the in-memory DB schema for tests.
Dockerfile
Outdated
| # Copy database directory with all SQL files and migrations (not copied by TypeScript compiler) | ||
| # This ensures schema files, migrations, and any future database-related files are included | ||
| COPY --from=backend-builder --chown=pabawi:pabawi /app/backend/src/database/ ./dist/database/ |
There was a problem hiding this comment.
This COPY brings the entire src/database/ directory (including .ts sources) into the runtime image under dist/database/. If the intent is only to ship SQL migrations, copy just dist/database/migrations (already produced by the build) or src/database/migrations into dist/database/migrations to avoid bloating the image and mixing source with runtime artifacts.
| # Copy database directory with all SQL files and migrations (not copied by TypeScript compiler) | |
| # This ensures schema files, migrations, and any future database-related files are included | |
| COPY --from=backend-builder --chown=pabawi:pabawi /app/backend/src/database/ ./dist/database/ | |
| # Copy only database migrations (not copied by TypeScript compiler) | |
| # This avoids copying TypeScript sources into the runtime image | |
| COPY --from=backend-builder --chown=pabawi:pabawi /app/backend/src/database/migrations ./dist/database/migrations |
| The project uses SQLite with a hybrid approach: base schema files + migration system. This has led to duplicate definitions and orphaned files that need cleanup. | ||
|
|
||
| ## Current File Structure | ||
|
|
||
| ``` | ||
| backend/src/database/ | ||
| ├── schema.sql # Base schema: executions table | ||
| ├── rbac-schema.sql # Base schema: RBAC tables (DUPLICATE of 001) | ||
| ├── audit-schema.sql # ⚠️ ORPHANED - never loaded, duplicates 004 | ||
| ├── migrations.sql # ⚠️ ORPHANED - never loaded, superseded | ||
| ├── DatabaseService.ts # Loads schema.sql + rbac-schema.sql, then runs migrations | ||
| ├── MigrationRunner.ts # Runs numbered migrations from migrations/ |
There was a problem hiding this comment.
This prompt document describes the current state as a hybrid schema.sql + migrations approach, but the PR removes those base schema files and moves to migration-first. Please update this section to reflect the new migration-first layout (or clearly label it as the pre-cleanup state) so readers aren’t pointed at deleted files.
.kiro/database-cleanup-prompt.md
Outdated
| - ✅ No orphaned files in the database directory | ||
| - ✅ Clear documentation of the schema management approach | ||
| - ✅ All tests pass | ||
| - ✅ Docker deployment works with cl |
There was a problem hiding this comment.
The file ends with a truncated sentence ("Docker deployment works with cl"), which looks like an accidental cut-off. Please complete the sentence or remove the incomplete line to avoid confusing readers.
| - ✅ Docker deployment works with cl | |
| - ✅ Docker deployment works correctly |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@alvagante I've opened a new pull request, #34, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: alvagante <283804+alvagante@users.noreply.github.com>
No description provided.