TMS Migration (to develop) #367
Conversation
imashkanov
changed the title
imashkanov
force-pushed
the
feature/EPMRPP-migrate-tms-to-develop
branch
from
6f8b9c8 to
e72cc3f
Compare
imashkanov
force-pushed
the
feature/EPMRPP-migrate-tms-to-develop
branch
from
de2a21c to
8576ded
Compare
imashkanov
force-pushed
the
feature/EPMRPP-migrate-tms-to-develop
branch
from
d9f46fb to
bae82c6
Compare
imashkanov
force-pushed
the
feature/EPMRPP-migrate-tms-to-develop
branch
from
202a17d to
600f7a4
Compare
imashkanov
force-pushed
the
feature/EPMRPP-migrate-tms-to-develop
branch
from
600f7a4 to
21fe65d
Compare
WalkthroughAdds a comprehensive Test Management System schema (many enums, ~30 tables, indexes, triggers, functions), alters Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
migrations/103_tms_initial.up.sql (10)
202-217: Index FK tms_test_case_version.test_case_id.Partial unique index on defaults won’t help general joins. Add a regular index.
CREATE UNIQUE INDEX idx_tms_test_case_version_default ON tms_test_case_version (test_case_id) WHERE is_default = true; + +CREATE INDEX idx_tms_test_case_version_test_case_id + ON tms_test_case_version (test_case_id);
254-263: Make step → steps_manual_scenario mandatory.A step without a parent scenario is likely invalid; enforce NOT NULL.
- steps_manual_scenario_id bigint + steps_manual_scenario_id bigint NOT NULL CONSTRAINT tms_step_fk_steps_manual_scenario REFERENCES tms_steps_manual_scenario
86-101: Add date integrity check for milestones.Prevent inverted date ranges.
end_date TIMESTAMP, type varchar(255), @@ test_plan_id bigint CONSTRAINT tms_milestone_fk_test_plan REFERENCES tms_test_plan ); +ALTER TABLE tms_milestone + ADD CONSTRAINT chk_tms_milestone_dates + CHECK (end_date IS NULL OR start_date IS NULL OR end_date >= start_date);
279-296: Guard attachment file_size and avoid truncation risk.
- Enforce non-negative size.
- path_to_file can exceed 255; prefer text.
- file_size bigint, - path_to_file varchar(255), + file_size bigint CHECK (file_size IS NULL OR file_size >= 0), + path_to_file text,
66-84: Index foreign keys on tms_test_plan.Typical join/delete paths will benefit from indexes.
); +CREATE INDEX idx_tms_test_plan_project_id ON tms_test_plan (project_id); +CREATE INDEX idx_tms_test_plan_environment_id ON tms_test_plan (environment_id); +CREATE INDEX idx_tms_test_plan_product_version_id ON tms_test_plan (product_version_id); +CREATE INDEX idx_tms_test_plan_launch_id ON tms_test_plan (launch_id);
52-64: Cover common hierarchy and M2M filters.
- Add index on test_folder.parent_id for tree navigation.
- Add secondary index on tms_environment_dataset(dataset_id) to complement the unique (environment_id, dataset_id).
CREATE INDEX idx_tms_test_folder_project_id ON tms_test_folder (project_id, id); +CREATE INDEX idx_tms_test_folder_parent_id ON tms_test_folder (parent_id); @@ CONSTRAINT tms_environment_dataset_unique UNIQUE (environment_id, dataset_id) ); + +CREATE INDEX idx_tms_environment_dataset_dataset_id + ON tms_environment_dataset (dataset_id);Also applies to: 118-118
168-186: Auto-maintain updated_at; consider generated search_vector.
- Currently updated_at won’t refresh on UPDATE.
- Optional: replace trigger-managed tsvector with a generated column for simpler maintenance (PG12+).
Option A (minimal): add timestamp trigger.
CREATE OR REPLACE FUNCTION set_updated_at() RETURNS TRIGGER AS $$ BEGIN NEW.updated_at := now(); RETURN NEW; END; $$ LANGUAGE plpgsql; CREATE TRIGGER tms_test_case_set_updated_at BEFORE UPDATE ON tms_test_case FOR EACH ROW EXECUTE FUNCTION set_updated_at();Option B (generated column, replaces trigger/function):
-- In table definition: -- search_vector tsvector GENERATED ALWAYS AS ( -- to_tsvector('simple', -- coalesce(name,'')||' '||coalesce(description,'')||' '||coalesce(priority,'')) -- ) STORED -- and drop trigger/function.Also applies to: 138-139
8-17: Project-level uniqueness on human-readable names (confirm).Do you require uniqueness for:
- product_version.version within project,
- dataset.name within project,
- environment.name within project?
If yes, add unique constraints accordingly.-- Examples: ALTER TABLE tms_product_version ADD CONSTRAINT uq_tms_product_version_proj_ver UNIQUE (project_id, version); ALTER TABLE tms_dataset ADD CONSTRAINT uq_tms_dataset_proj_name UNIQUE (project_id, name); ALTER TABLE tms_environment ADD CONSTRAINT uq_tms_environment_proj_name UNIQUE (project_id, name);Also applies to: 21-29, 42-50
218-233: One-to-one manual scenario shape (confirm).Model enforces a single manual_scenario per test_case_version and either TEXT or STEPS via sub-tables. If that’s intended, great; if you need to allow both types simultaneously, remove the UNIQUE on test_case_version_id and add a composite unique (test_case_version_id, type).
- test_case_version_id bigint - UNIQUE + test_case_version_id bigint CONSTRAINT tms_manual_scenario_fk_test_case_version REFERENCES tms_test_case_version,-- Alternative: -- ALTER TABLE tms_manual_scenario -- ADD CONSTRAINT uq_manual_scenario_one_per_type -- UNIQUE (test_case_version_id, type);Also applies to: 234-250, 254-264, 265-275
1-6: Attributes key length/index (confirm).If tms_attribute.key is frequently filtered by prefix/exact match, consider adding a dedicated index (btree/trgm as applicable). 255 may be tight for some metadata keys; TEXT is safer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
migrations/103_tms_initial.down.sql(1 hunks)migrations/103_tms_initial.up.sql(1 hunks)
🔇 Additional comments (1)
migrations/103_tms_initial.down.sql (1)
1-50: Down looks consistent; remember to mirror any new indexes/constraints you add.The drop order and IF EXISTS usage are solid. If you adopt the suggested new indexes/constraints in the up migration, add corresponding DROP statements here to keep rollbacks clean.
| CREATE TABLE tms_dataset_data | ||
| ( | ||
| id BIGSERIAL | ||
| CONSTRAINT tms_dataset_data_pk PRIMARY KEY, | ||
| key varchar(255), | ||
| value varchar(255), | ||
| dataset_id bigint NOT NULL | ||
| CONSTRAINT tms_dataset_attribute_fk_tms_dataset | ||
| REFERENCES tms_dataset | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure dataset key uniqueness per dataset and fix FK name typo.
- Prevent duplicate keys within the same dataset.
- Current FK name looks copy-pasted (“attribute”); align with table naming.
- dataset_id bigint NOT NULL
- CONSTRAINT tms_dataset_attribute_fk_tms_dataset
- REFERENCES tms_dataset
+ dataset_id bigint NOT NULL
+ CONSTRAINT tms_dataset_data_fk_dataset
+ REFERENCES tms_dataset,
+ CONSTRAINT tms_dataset_data_unique_key_per_dataset UNIQUE (dataset_id, key)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CREATE TABLE tms_dataset_data | |
| ( | |
| id BIGSERIAL | |
| CONSTRAINT tms_dataset_data_pk PRIMARY KEY, | |
| key varchar(255), | |
| value varchar(255), | |
| dataset_id bigint NOT NULL | |
| CONSTRAINT tms_dataset_attribute_fk_tms_dataset | |
| REFERENCES tms_dataset | |
| ); | |
| CREATE TABLE tms_dataset_data | |
| ( | |
| id BIGSERIAL | |
| CONSTRAINT tms_dataset_data_pk PRIMARY KEY, | |
| key varchar(255), | |
| value varchar(255), | |
| dataset_id bigint NOT NULL | |
| CONSTRAINT tms_dataset_data_fk_dataset | |
| REFERENCES tms_dataset, | |
| CONSTRAINT tms_dataset_data_unique_key_per_dataset UNIQUE (dataset_id, key) | |
| ); |
🤖 Prompt for AI Agents
migrations/103_tms_initial.up.sql around lines 31 to 40: the tms_dataset_data
table needs a per-dataset uniqueness guarantee for key and the foreign key
constraint name should reflect this table (not “attribute”); update the CREATE
TABLE so that key is NOT NULL (to ensure meaningful uniqueness), add a UNIQUE
constraint on (dataset_id, key) to prevent duplicate keys within the same
dataset, and rename the FK constraint to something like
tms_dataset_data_fk_dataset referencing tms_dataset(id).
| id BIGSERIAL | ||
| CONSTRAINT tms_test_case_pk PRIMARY KEY, | ||
| created_at TIMESTAMP DEFAULT now() NOT NULL, | ||
| updated_at TIMESTAMP DEFAULT now() NOT NULL, | ||
| name varchar(255), | ||
| description varchar(255), | ||
| priority varchar(255), | ||
| search_vector tsvector, | ||
| external_id varchar(255), | ||
| test_folder_id bigint NOT NULL | ||
| CONSTRAINT tms_test_case_fk_test_folder | ||
| REFERENCES tms_test_folder, | ||
| dataset_id bigint | ||
| CONSTRAINT tms_test_case_fk_dataset | ||
| REFERENCES tms_dataset, | ||
| CONSTRAINT tms_test_case_name_folder_unique UNIQUE (name, test_folder_id) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add index for tms_test_case lookups by folder.
Queries “all cases in folder” will not use the (name, test_folder_id) unique index efficiently. Add a single-column index.
CREATE INDEX idx_tms_test_plan_test_case_test_plan_id ON tms_test_plan_test_case (test_plan_id);
CREATE INDEX idx_tms_test_plan_test_case_test_case_id ON tms_test_plan_test_case (test_case_id);
+
+-- Speed up folder-level listings and deletes
+CREATE INDEX idx_tms_test_case_test_folder_id ON tms_test_case (test_folder_id);Also applies to: 165-167
🤖 Prompt for AI Agents
In migrations/103_tms_initial.up.sql around lines 136-152 (and also apply the
same change for the table defined at lines 165-167), the schema lacks a
single-column index on test_folder_id which prevents efficient “all cases in
folder” lookups; add a standalone B-tree index for test_folder_id on the
tms_test_case table (e.g. CREATE INDEX ... ON tms_test_case(test_folder_id)) and
also create the equivalent single-column index on the table defined at lines
165-167 so folder-based queries use the index instead of scanning the unique
(name, test_folder_id) constraint.
migrations/1000_tms_initial.up.sql
Outdated
| CREATE TYPE LAUNCH_TYPE_ENUM AS ENUM ('AUTOMATION', 'MANUAL'); | ||
| ALTER TABLE launch | ||
| ADD COLUMN launch_type LAUNCH_TYPE_ENUM; | ||
| ALTER TABLE launch | ||
| ALTER COLUMN launch_type SET DEFAULT 'AUTOMATION'; | ||
| UPDATE launch | ||
| SET launch_type = 'AUTOMATION'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make launch.launch_type NOT NULL after backfill.
Nulls would complicate downstream logic and analytics. You already backfill and set default; lock in integrity with NOT NULL.
ALTER TABLE launch
ALTER COLUMN launch_type SET DEFAULT 'AUTOMATION';
UPDATE launch
SET launch_type = 'AUTOMATION';
+ALTER TABLE launch
+ ALTER COLUMN launch_type SET NOT NULL;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CREATE TYPE LAUNCH_TYPE_ENUM AS ENUM ('AUTOMATION', 'MANUAL'); | |
| ALTER TABLE launch | |
| ADD COLUMN launch_type LAUNCH_TYPE_ENUM; | |
| ALTER TABLE launch | |
| ALTER COLUMN launch_type SET DEFAULT 'AUTOMATION'; | |
| UPDATE launch | |
| SET launch_type = 'AUTOMATION'; | |
| CREATE TYPE LAUNCH_TYPE_ENUM AS ENUM ('AUTOMATION', 'MANUAL'); | |
| ALTER TABLE launch | |
| ADD COLUMN launch_type LAUNCH_TYPE_ENUM; | |
| ALTER TABLE launch | |
| ALTER COLUMN launch_type SET DEFAULT 'AUTOMATION'; | |
| UPDATE launch | |
| SET launch_type = 'AUTOMATION'; | |
| ALTER TABLE launch | |
| ALTER COLUMN launch_type SET NOT NULL; |
🤖 Prompt for AI Agents
In migrations/103_tms_initial.up.sql around lines 334 to 340, the launch_type
column is created, defaulted and backfilled but not constrained to NOT NULL; add
an ALTER TABLE launch ALTER COLUMN launch_type SET NOT NULL statement after the
backfill and default are applied so the column cannot contain NULLs going
forward and data integrity is enforced.
df2e346 to
7e1c575
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
migrations/103_tms_initial.up.sql (6)
247-259: Remove redundant unique index; the column constraint already creates it.manual_scenario_id has UNIQUE at column-level. The separate UNIQUE INDEX duplicates it.
-CREATE UNIQUE INDEX idx_tms_manual_scenario_preconditions_scenario_unique - ON tms_manual_scenario_preconditions(manual_scenario_id);
137-146: Add ON DELETE CASCADE to junction/mapping FKs to prevent orphans and ease cleanup.These are pure link tables; cascading deletions are expected.
Examples:
--- tms_test_folder_test_item - REFERENCES tms_test_folder, + REFERENCES tms_test_folder ON DELETE CASCADE, - REFERENCES test_item, + REFERENCES test_item ON DELETE CASCADE, --- tms_test_plan_test_case - REFERENCES tms_test_plan, + REFERENCES tms_test_plan ON DELETE CASCADE, - REFERENCES tms_test_case, + REFERENCES tms_test_case ON DELETE CASCADE, --- tms_test_case_test_item - REFERENCES tms_test_case, + REFERENCES tms_test_case ON DELETE CASCADE, - REFERENCES test_item, + REFERENCES test_item ON DELETE CASCADE, --- tms_step_test_item - REFERENCES tms_step, + REFERENCES tms_step ON DELETE CASCADE, - REFERENCES test_item, + REFERENCES test_item ON DELETE CASCADE, --- tms_step_attachment - REFERENCES tms_step, + REFERENCES tms_step ON DELETE CASCADE, - REFERENCES tms_attachment, + REFERENCES tms_attachment ON DELETE CASCADE, --- tms_text_manual_scenario_attachment - REFERENCES tms_text_manual_scenario, + REFERENCES tms_text_manual_scenario ON DELETE CASCADE, - REFERENCES tms_attachment, + REFERENCES tms_attachment ON DELETE CASCADE, --- tms_manual_scenario_preconditions_attachment - REFERENCES tms_manual_scenario_preconditions, + REFERENCES tms_manual_scenario_preconditions ON DELETE CASCADE, - REFERENCES tms_attachment, + REFERENCES tms_attachment ON DELETE CASCADE,Please confirm intended deletion semantics match this.
Also applies to: 171-180, 202-211, 291-300, 323-333, 338-348, 353-363
66-85: Consider nullifying optional references on parent delete.environment_id, product_version_id, launch_id look optional. Prefer ON DELETE SET NULL to avoid delete conflicts.
- CONSTRAINT tms_test_plan_fk_environment - REFERENCES tms_environment, + CONSTRAINT tms_test_plan_fk_environment + REFERENCES tms_environment ON DELETE SET NULL, ... - CONSTRAINT tms_test_plan_fk_product_version - REFERENCES tms_product_version, + CONSTRAINT tms_test_plan_fk_product_version + REFERENCES tms_product_version ON DELETE SET NULL, ... - CONSTRAINT tms_test_plan_fk_launch - REFERENCES launch + CONSTRAINT tms_test_plan_fk_launch + REFERENCES launch ON DELETE SET NULL
155-169: Keep updated_at accurate.Add a trigger to set updated_at = now() on UPDATE.
CREATE OR REPLACE FUNCTION set_updated_at() RETURNS TRIGGER AS $$ BEGIN NEW.updated_at := now(); RETURN NEW; END; $$ LANGUAGE plpgsql; CREATE TRIGGER tms_test_case_set_updated_at BEFORE UPDATE ON tms_test_case FOR EACH ROW EXECUTE FUNCTION set_updated_at();
305-318: Harden attachment metadata.Default created_at, and enforce non-negative file_size.
- file_size bigint, + file_size bigint, path_to_file varchar(255) NOT NULL, - created_at TIMESTAMP, + created_at TIMESTAMP DEFAULT now() NOT NULL, expires_at TIMESTAMP, ... + CONSTRAINT tms_attachment_file_size_nonnegative + CHECK (file_size IS NULL OR file_size >= 0),
103-119: Resolve the “many-to-many?” TODOs or track them.Milestone ↔ product_version / test_plan likely need junction tables. Create tasks or adjust now.
I can propose junction table DDLs if you confirm the intended relationships.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
migrations/103_tms_initial.down.sql(1 hunks)migrations/103_tms_initial.up.sql(1 hunks)
🔇 Additional comments (3)
migrations/103_tms_initial.up.sql (3)
151-169: Add index for folder-level test case lookups.Single-column index on test_folder_id avoids scans when listing/deleting cases by folder.
-- existing table definition... ); +-- Speed up folder-level listings and deletes +CREATE INDEX idx_tms_test_case_test_folder_id ON tms_test_case (test_folder_id);
31-40: Ensure per-dataset key uniqueness and correct FK name.Make key NOT NULL, enforce (dataset_id, key) uniqueness, and fix FK constraint name.
CREATE TABLE tms_dataset_data ( id BIGSERIAL CONSTRAINT tms_dataset_data_pk PRIMARY KEY, - key varchar(255), + key varchar(255) NOT NULL, value varchar(255), dataset_id bigint NOT NULL - CONSTRAINT tms_dataset_attribute_fk_tms_dataset - REFERENCES tms_dataset + CONSTRAINT tms_dataset_data_fk_dataset + REFERENCES tms_dataset(id), + CONSTRAINT tms_dataset_data_unique_key_per_dataset UNIQUE (dataset_id, key) );
405-411: Make launch.launch_type NOT NULL after backfill.You set default and backfill; lock integrity with NOT NULL.
ALTER TABLE launch ALTER COLUMN launch_type SET DEFAULT 'AUTOMATION'; UPDATE launch SET launch_type = 'AUTOMATION'; +ALTER TABLE launch + ALTER COLUMN launch_type SET NOT NULL;
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
migrations/103_tms_initial.up.sql (2)
452-463: Add NOT NULL constraint to launch.launch_type after backfill.The column is backfilled to 'AUTOMATION' and has a default, but is not constrained to NOT NULL. Nulls would complicate downstream logic and analytics.
Apply this diff:
CREATE TYPE LAUNCH_TYPE_ENUM AS ENUM ('AUTOMATION', 'MANUAL'); ALTER TABLE launch ADD COLUMN launch_type LAUNCH_TYPE_ENUM; ALTER TABLE launch ALTER COLUMN launch_type SET DEFAULT 'AUTOMATION'; UPDATE launch SET launch_type = 'AUTOMATION'; +ALTER TABLE launch + ALTER COLUMN launch_type SET NOT NULL; ALTER TABLE launch ADD COLUMN test_plan_id bigint;
46-55: Fix tms_dataset_data: add NOT NULL, composite uniqueness, and correct FK name.The table lacks data integrity constraints:
keyshould be NOT NULL to ensure meaningful uniqueness- Missing
UNIQUE (dataset_id, key)to prevent duplicate keys per dataset- FK name
tms_dataset_attribute_fk_tms_datasetis misnamed (should reflect the table being defined)Apply this diff:
CREATE TABLE tms_dataset_data ( id BIGSERIAL CONSTRAINT tms_dataset_data_pk PRIMARY KEY, - key varchar(255), + key varchar(255) NOT NULL, value varchar(255), dataset_id bigint NOT NULL - CONSTRAINT tms_dataset_attribute_fk_tms_dataset - REFERENCES tms_dataset + CONSTRAINT tms_dataset_data_fk_dataset + REFERENCES tms_dataset, + CONSTRAINT tms_dataset_data_unique_key_per_dataset UNIQUE (dataset_id, key) );migrations/103_tms_initial.down.sql (1)
1-9: Remove duplicate/incorrect index drops at lines 1-2 and consolidate.Lines 1-2 attempt to drop indexes with names that don't match the up migration. Specifically, the up migration creates indexes named
idx_preconditions_attachment_*(line 391-392), but lines 1-2 try to dropidx_tms_manual_scenario_preconditions_attachment_*. TheIF EXISTSguards prevent errors, but the unnecessary drops add noise and confusion.Compare against up migration:
- Up:
CREATE INDEX idx_preconditions_attachment_preconditions_id ON tms_manual_scenario_preconditions_attachment(preconditions_id);(line 391)- Down line 1 tries:
DROP INDEX IF EXISTS idx_tms_manual_scenario_preconditions_attachment_attachment_id;❌- Down line 5 correctly:
DROP INDEX IF EXISTS idx_preconditions_attachment_attachment_id;✓Apply this diff to consolidate:
-DROP INDEX IF EXISTS idx_tms_manual_scenario_preconditions_attachment_attachment_id; -DROP INDEX IF EXISTS idx_tms_manual_scenario_preconditions_attachment_preconditions_id; DROP TABLE IF EXISTS tms_manual_scenario_preconditions_attachment; -DROP INDEX IF EXISTS idx_preconditions_attachment_attachment_id; -DROP INDEX IF EXISTS idx_preconditions_attachment_preconditions_id; DROP INDEX IF EXISTS idx_tms_text_manual_scenario_attachment_scenario_id; DROP INDEX IF EXISTS idx_tms_text_manual_scenario_attachment_attachment_id; DROP TABLE IF EXISTS tms_text_manual_scenario_attachment; + +DROP INDEX IF EXISTS idx_preconditions_attachment_preconditions_id; +DROP INDEX IF EXISTS idx_preconditions_attachment_attachment_id; DROP INDEX IF EXISTS idx_tms_step_attachment_attachment_id;
🧹 Nitpick comments (1)
migrations/103_tms_initial.up.sql (1)
163-181: Add single-column index on test_folder_id for efficient folder queries.Queries like "all test cases in folder X" will scan the composite unique index
(name, test_folder_id)inefficiently. Add a dedicated B-tree index on the folder ID.Add this index after the table definition:
CREATE TABLE tms_test_case ( id BIGSERIAL CONSTRAINT tms_test_case_pk PRIMARY KEY, created_at TIMESTAMP DEFAULT now() NOT NULL, updated_at TIMESTAMP DEFAULT now() NOT NULL, name varchar(255), description varchar(255), priority varchar(255), search_vector tsvector, external_id varchar(255), test_folder_id bigint NOT NULL CONSTRAINT tms_test_case_fk_test_folder REFERENCES tms_test_folder, dataset_id bigint CONSTRAINT tms_test_case_fk_dataset REFERENCES tms_dataset, CONSTRAINT tms_test_case_name_folder_unique UNIQUE (name, test_folder_id) ); + +CREATE INDEX idx_tms_test_case_test_folder_id ON tms_test_case (test_folder_id);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
migrations/103_tms_initial.down.sql(1 hunks)migrations/103_tms_initial.up.sql(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
migrations/103_tms_initial.up.sql (3)
163-181: [DUPLICATE] Add single-column index on test_folder_id for efficient folder queries.Folder-level lookups (e.g., "all cases in folder") will not use the composite
(name, test_folder_id)unique index efficiently. Add a dedicated B-tree index ontest_folder_id.CREATE TABLE tms_test_case ( id BIGSERIAL CONSTRAINT tms_test_case_pk PRIMARY KEY, created_at TIMESTAMP DEFAULT now() NOT NULL, updated_at TIMESTAMP DEFAULT now() NOT NULL, name varchar(255), description varchar(255), priority varchar(255), search_vector tsvector, external_id varchar(255), test_folder_id bigint NOT NULL CONSTRAINT tms_test_case_fk_test_folder REFERENCES tms_test_folder, dataset_id bigint CONSTRAINT tms_test_case_fk_dataset REFERENCES tms_dataset, CONSTRAINT tms_test_case_name_folder_unique UNIQUE (name, test_folder_id) ); +CREATE INDEX idx_tms_test_case_test_folder_id ON tms_test_case (test_folder_id);
46-55: [DUPLICATE] Fix FK name typo and add dataset-key uniqueness constraint.The FK constraint name "tms_dataset_attribute_fk_tms_dataset" appears copy-pasted and should reflect the
tms_dataset_datatable. Additionally, to prevent duplicate keys within the same dataset, add a composite UNIQUE constraint on(dataset_id, key)and ensurekeyis NOT NULL.CREATE TABLE tms_dataset_data ( id BIGSERIAL CONSTRAINT tms_dataset_data_pk PRIMARY KEY, - key varchar(255), + key varchar(255) NOT NULL, value varchar(255), dataset_id bigint NOT NULL - CONSTRAINT tms_dataset_attribute_fk_tms_dataset + CONSTRAINT tms_dataset_data_fk_dataset REFERENCES tms_dataset + CONSTRAINT tms_dataset_data_unique_key_per_dataset UNIQUE (dataset_id, key) );
442-448: [DUPLICATE] Enforce NOT NULL on launch_type after backfill.After setting a default and backfilling all existing rows to 'AUTOMATION', add a NOT NULL constraint to prevent future nulls and strengthen data integrity.
CREATE TYPE LAUNCH_TYPE_ENUM AS ENUM ('AUTOMATION', 'MANUAL'); ALTER TABLE launch ADD COLUMN launch_type LAUNCH_TYPE_ENUM; ALTER TABLE launch ALTER COLUMN launch_type SET DEFAULT 'AUTOMATION'; UPDATE launch SET launch_type = 'AUTOMATION'; +ALTER TABLE launch + ALTER COLUMN launch_type SET NOT NULL; ALTER TABLE launch ADD COLUMN test_plan_id bigint;migrations/103_tms_initial.down.sql (1)
1-10: [DUPLICATE] Remove erroneous index drop names; align with UP migration.Lines 2-3 attempt to drop indexes with names that don't exist in the UP migration. The correct index names (created at UP lines 377-378) are
idx_preconditions_attachment_preconditions_idandidx_preconditions_attachment_attachment_id, which are already correctly dropped at lines 6-7. Remove the duplicate/incorrect drops at lines 2-3.-DROP INDEX IF EXISTS idx_tms_manual_scenario_preconditions_attachment_attachment_id; -DROP INDEX IF EXISTS idx_tms_manual_scenario_preconditions_attachment_preconditions_id; DROP TABLE IF EXISTS tms_manual_scenario_preconditions_attachment; DROP INDEX IF EXISTS idx_preconditions_attachment_attachment_id;
🧹 Nitpick comments (1)
migrations/103_tms_initial.up.sql (1)
259-270: Redundant UNIQUE constraint on tms_manual_scenario_preconditions.The table defines manual_scenario_id both as a UNIQUE FK constraint (line 263) and a duplicate UNIQUE index (lines 269-270). Keep only the constraint and remove the index.
CREATE TABLE tms_manual_scenario_preconditions ( id BIGSERIAL CONSTRAINT tms_manual_scenario_preconditions_pk PRIMARY KEY, manual_scenario_id bigint NOT NULL UNIQUE CONSTRAINT tms_manual_scenario_preconditions_fk_manual_scenario REFERENCES tms_manual_scenario, value varchar(255) ); -CREATE UNIQUE INDEX idx_tms_manual_scenario_preconditions_scenario_unique - ON tms_manual_scenario_preconditions(manual_scenario_id);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
migrations/103_tms_initial.down.sql(1 hunks)migrations/103_tms_initial.up.sql(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
migrations/103_tms_initial.up.sql (3)
46-55: Ensure dataset key uniqueness per dataset and fix FK name typo (duplicate of previous review).The FK constraint name "tms_dataset_attribute_fk_tms_dataset" appears copy-pasted from another table. Additionally, there is no UNIQUE constraint on (dataset_id, key) to prevent duplicate keys within the same dataset.
Apply this diff to fix the constraint name and add uniqueness guarantee:
CREATE TABLE tms_dataset_data ( id BIGSERIAL CONSTRAINT tms_dataset_data_pk PRIMARY KEY, key varchar(255), value varchar(255), dataset_id bigint NOT NULL - CONSTRAINT tms_dataset_attribute_fk_tms_dataset - REFERENCES tms_dataset + CONSTRAINT tms_dataset_data_fk_dataset + REFERENCES tms_dataset, + CONSTRAINT tms_dataset_data_unique_key_per_dataset UNIQUE (dataset_id, key) );
163-181: Add single-column index on test_folder_id for efficient folder lookups (duplicate of previous review).The UNIQUE constraint on (name, test_folder_id) cannot efficiently support queries filtering only by folder ID. A standalone index on test_folder_id is needed for folder-level listings and deletes.
Apply this diff to add the missing index:
CREATE TABLE tms_test_case ( id BIGSERIAL CONSTRAINT tms_test_case_pk PRIMARY KEY, created_at TIMESTAMP DEFAULT now() NOT NULL, updated_at TIMESTAMP DEFAULT now() NOT NULL, name varchar(255), description varchar(255), priority varchar(255), search_vector tsvector, external_id varchar(255), test_folder_id bigint NOT NULL CONSTRAINT tms_test_case_fk_test_folder REFERENCES tms_test_folder, dataset_id bigint CONSTRAINT tms_test_case_fk_dataset REFERENCES tms_dataset, CONSTRAINT tms_test_case_name_folder_unique UNIQUE (name, test_folder_id) ); +CREATE INDEX idx_tms_test_case_test_folder_id ON tms_test_case (test_folder_id); + CREATE TABLE tms_test_plan_test_case ( test_plan_id bigint
436-445: Critical: Add NOT NULL constraint to launch.launch_type after backfill (duplicate of previous review).The
launch_typecolumn is created, defaulted, and backfilled to 'AUTOMATION', but not constrained to NOT NULL. This allows future NULLs, which will complicate downstream logic and analytics. The column integrity must be enforced.Apply this diff to lock in the constraint after backfill:
ALTER TABLE launch ALTER COLUMN launch_type SET DEFAULT 'AUTOMATION'; UPDATE launch SET launch_type = 'AUTOMATION'; +ALTER TABLE launch + ALTER COLUMN launch_type SET NOT NULL; ALTER TABLE launch ADD COLUMN test_plan_id bigint;
🧹 Nitpick comments (1)
migrations/103_tms_initial.up.sql (1)
444-447: Consider adding FK constraint to launch.test_plan_id.The
test_plan_idcolumn is added without a foreign key constraint totms_test_plan. This leaves referential integrity to the application layer. If this column is meant to always reference a valid test plan, add a FK constraint.Apply this diff if FK constraint is needed:
ALTER TABLE launch ADD COLUMN test_plan_id bigint; +ALTER TABLE launch + ADD CONSTRAINT launch_fk_test_plan + FOREIGN KEY (test_plan_id) REFERENCES tms_test_plan;Alternatively, if the FK is intentionally deferred or conditional, document this decision in the migration or PR comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
migrations/103_tms_initial.up.sql(1 hunks)
🔇 Additional comments (3)
migrations/103_tms_initial.up.sql (3)
1-44: LGTM on attribute, product version, and dataset tables.Schema structure is sound with proper FKs, unique constraints, and search-vector infrastructure.
57-161: LGTM on environments, test plans, milestones, and folders.Proper FKs, bridge tables with uniqueness constraints, and search-vector indexing are all correctly implemented.
416-434: LGTM on tms_test_case_execution structure (note: test_case_id lacks FK).The execution table is well-indexed with multiple query patterns covered. However,
test_case_id(line 423) has no foreign key constraint totms_test_case. Verify this is intentional (e.g., if test case can be deleted independently or if the ID originates from external context).
| CREATE TABLE tms_text_manual_scenario | ||
| ( | ||
| manual_scenario_id bigint NOT NULL | ||
| CONSTRAINT tms_text_manual_scenario_pk PRIMARY KEY | ||
| CONSTRAINT tms_text_manual_scenario_fk_manual_scenario | ||
| REFERENCES tms_manual_scenario, | ||
| instructions varchar(255), | ||
| expected_result varchar(255) | ||
| ); |
There was a problem hiding this comment.
Critical: Fix duplicate CONSTRAINT syntax on tms_text_manual_scenario.
A column cannot have two inline CONSTRAINT definitions. The PRIMARY KEY and FOREIGN KEY constraints are both defined on the same column, which is invalid SQL syntax.
Apply this diff to separate the constraints:
CREATE TABLE tms_text_manual_scenario
(
manual_scenario_id bigint NOT NULL
- CONSTRAINT tms_text_manual_scenario_pk PRIMARY KEY
- CONSTRAINT tms_text_manual_scenario_fk_manual_scenario
+ CONSTRAINT tms_text_manual_scenario_pk PRIMARY KEY,
+ CONSTRAINT tms_text_manual_scenario_fk_manual_scenario FOREIGN KEY (manual_scenario_id)
REFERENCES tms_manual_scenario,
instructions varchar(255),
expected_result varchar(255)
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CREATE TABLE tms_text_manual_scenario | |
| ( | |
| manual_scenario_id bigint NOT NULL | |
| CONSTRAINT tms_text_manual_scenario_pk PRIMARY KEY | |
| CONSTRAINT tms_text_manual_scenario_fk_manual_scenario | |
| REFERENCES tms_manual_scenario, | |
| instructions varchar(255), | |
| expected_result varchar(255) | |
| ); | |
| CREATE TABLE tms_text_manual_scenario | |
| ( | |
| manual_scenario_id bigint NOT NULL | |
| CONSTRAINT tms_text_manual_scenario_pk PRIMARY KEY, | |
| CONSTRAINT tms_text_manual_scenario_fk_manual_scenario FOREIGN KEY (manual_scenario_id) | |
| REFERENCES tms_manual_scenario, | |
| instructions varchar(255), | |
| expected_result varchar(255) | |
| ); |
🤖 Prompt for AI Agents
In migrations/103_tms_initial.up.sql around lines 272 to 280, the
manual_scenario_id column currently has two inline CONSTRAINT clauses (PRIMARY
KEY and FOREIGN KEY) which is invalid SQL; separate them by declaring the column
normally (manual_scenario_id bigint NOT NULL) and then add table-level
constraints: one PRIMARY KEY constraint for manual_scenario_id and one FOREIGN
KEY constraint referencing tms_manual_scenario. Ensure the FOREIGN KEY has its
own CONSTRAINT name and correct syntax (REFERENCES tms_manual_scenario(id) or
the appropriate PK column).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
migrations/103_tms_initial.up.sql (4)
258-266: Critical: Fix invalid duplicate CONSTRAINT syntax on tms_text_manual_scenario.A column cannot have two inline CONSTRAINT definitions. The PRIMARY KEY and FOREIGN KEY constraints are both declared on the same column, which is invalid SQL syntax. Separate them into table-level constraints.
Apply this diff to fix the syntax:
CREATE TABLE tms_text_manual_scenario ( manual_scenario_id bigint NOT NULL - CONSTRAINT tms_text_manual_scenario_pk PRIMARY KEY - CONSTRAINT tms_text_manual_scenario_fk_manual_scenario + CONSTRAINT tms_text_manual_scenario_pk PRIMARY KEY, + CONSTRAINT tms_text_manual_scenario_fk_manual_scenario FOREIGN KEY (manual_scenario_id) REFERENCES tms_manual_scenario, instructions varchar(255), expected_result varchar(255) );
437-443: Add NOT NULL constraint to launch_type after backfill.The
launch_typecolumn is created, backfilled with 'AUTOMATION', and given a default, but is never constrained to NOT NULL. This allows future inserts to bypass the default via explicit NULL, complicating downstream logic and analytics.Apply this diff to enforce NOT NULL:
CREATE TYPE LAUNCH_TYPE_ENUM AS ENUM ('AUTOMATION', 'MANUAL'); ALTER TABLE launch ADD COLUMN launch_type LAUNCH_TYPE_ENUM; ALTER TABLE launch ALTER COLUMN launch_type SET DEFAULT 'AUTOMATION'; UPDATE launch SET launch_type = 'AUTOMATION'; +ALTER TABLE launch + ALTER COLUMN launch_type SET NOT NULL;
268-274: Critical: Fix invalid duplicate CONSTRAINT syntax on tms_steps_manual_scenario.Same issue as tms_text_manual_scenario: two inline CONSTRAINT definitions on the same column is invalid SQL syntax. Separate them into table-level constraints.
Apply this diff to fix the syntax:
CREATE TABLE tms_steps_manual_scenario ( manual_scenario_id bigint NOT NULL - CONSTRAINT tms_steps_manual_scenario_pk PRIMARY KEY - CONSTRAINT tms_steps_manual_scenario_fk_manual_scenario + CONSTRAINT tms_steps_manual_scenario_pk PRIMARY KEY, + CONSTRAINT tms_steps_manual_scenario_fk_manual_scenario FOREIGN KEY (manual_scenario_id) REFERENCES tms_manual_scenario );
46-55: Fix FK name typo and add per-dataset key uniqueness constraint on tms_dataset_data.The FK constraint name
tms_dataset_attribute_fk_tms_datasetappears copy-pasted from the attribute table and does not reflect this table. Additionally, thekeycolumn lacks a NOT NULL constraint, and there is no UNIQUE constraint preventing duplicate keys within the same dataset.Apply this diff to correct the FK name, enforce NOT NULL on key, and add per-dataset uniqueness:
CREATE TABLE tms_dataset_data ( id BIGSERIAL CONSTRAINT tms_dataset_data_pk PRIMARY KEY, - key varchar(255), + key varchar(255) NOT NULL, value varchar(255), dataset_id bigint NOT NULL - CONSTRAINT tms_dataset_attribute_fk_tms_dataset + CONSTRAINT tms_dataset_data_fk_dataset - REFERENCES tms_dataset + REFERENCES tms_dataset, + CONSTRAINT tms_dataset_data_unique_key_per_dataset UNIQUE (dataset_id, key) );
🧹 Nitpick comments (1)
migrations/103_tms_initial.up.sql (1)
163-181: Add single-column index on tms_test_case.test_folder_id for efficient folder-level queries.The unique composite index on (name, test_folder_id) does not optimize queries that filter by folder alone (e.g., "list all cases in folder" or "delete all cases in folder"). Add a dedicated single-column index.
Apply this diff to add the index:
CREATE TABLE tms_test_case ( id BIGSERIAL CONSTRAINT tms_test_case_pk PRIMARY KEY, created_at TIMESTAMP DEFAULT now() NOT NULL, updated_at TIMESTAMP DEFAULT now() NOT NULL, name varchar(255), description varchar(255), priority varchar(255), search_vector tsvector, external_id varchar(255), test_folder_id bigint NOT NULL CONSTRAINT tms_test_case_fk_test_folder REFERENCES tms_test_folder, dataset_id bigint CONSTRAINT tms_test_case_fk_dataset REFERENCES tms_dataset, CONSTRAINT tms_test_case_name_folder_unique UNIQUE (name, test_folder_id) ); + +CREATE INDEX idx_tms_test_case_test_folder_id ON tms_test_case (test_folder_id);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
migrations/103_tms_initial.down.sql(1 hunks)migrations/103_tms_initial.up.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- migrations/103_tms_initial.down.sql
imashkanov
force-pushed
the
feature/EPMRPP-migrate-tms-to-develop
branch
2 times, most recently
from
9559a6b to
ef60327
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
migrations/103_tms_initial.up.sql (4)
286-294: Critical: Fix duplicate CONSTRAINT syntax on tms_text_manual_scenario.A column cannot declare two inline CONSTRAINT definitions. Lines 289–290 have two
CONSTRAINTkeywords on the same column, which is invalid SQL syntax.Apply this diff:
CREATE TABLE tms_text_manual_scenario ( manual_scenario_id bigint NOT NULL - CONSTRAINT tms_text_manual_scenario_pk PRIMARY KEY - CONSTRAINT tms_text_manual_scenario_fk_manual_scenario + CONSTRAINT tms_text_manual_scenario_pk PRIMARY KEY, + CONSTRAINT tms_text_manual_scenario_fk_manual_scenario FOREIGN KEY (manual_scenario_id) REFERENCES tms_manual_scenario, instructions varchar(255), expected_result varchar(255) );
452-461: Add NOT NULL constraint to launch_type after backfill.The column is backfilled and defaulted but not constrained to NOT NULL. This allows NULLs to reappear and complicates downstream logic. Enforce integrity by adding the constraint.
Apply this diff:
CREATE TYPE LAUNCH_TYPE_ENUM AS ENUM ('AUTOMATION', 'MANUAL'); ALTER TABLE launch ADD COLUMN launch_type LAUNCH_TYPE_ENUM; ALTER TABLE launch ALTER COLUMN launch_type SET DEFAULT 'AUTOMATION'; UPDATE launch SET launch_type = 'AUTOMATION'; +ALTER TABLE launch + ALTER COLUMN launch_type SET NOT NULL; ALTER TABLE launch ADD COLUMN test_plan_id bigint;
46-55: Add NOT NULL to key column and add per-dataset uniqueness constraint.For meaningful uniqueness within a dataset, the
keycolumn must be NOT NULL. Additionally, the FK constraint name appears to be copy-pasted ("attribute"); it should reflect this table's purpose.Apply this diff:
CREATE TABLE tms_dataset_data ( id BIGSERIAL CONSTRAINT tms_dataset_data_pk PRIMARY KEY, - key varchar(255), + key varchar(255) NOT NULL, value varchar(255), dataset_id bigint NOT NULL - CONSTRAINT tms_dataset_attribute_fk_tms_dataset - REFERENCES tms_dataset + CONSTRAINT tms_dataset_data_fk_dataset + REFERENCES tms_dataset, + CONSTRAINT tms_dataset_data_unique_key_per_dataset UNIQUE (dataset_id, key) );
296-302: Critical: Fix duplicate CONSTRAINT syntax on tms_steps_manual_scenario.Same issue as tms_text_manual_scenario—two inline CONSTRAINT keywords on the same column is invalid SQL syntax.
Apply this diff:
CREATE TABLE tms_steps_manual_scenario ( manual_scenario_id bigint NOT NULL - CONSTRAINT tms_steps_manual_scenario_pk PRIMARY KEY - CONSTRAINT tms_steps_manual_scenario_fk_manual_scenario + CONSTRAINT tms_steps_manual_scenario_pk PRIMARY KEY, + CONSTRAINT tms_steps_manual_scenario_fk_manual_scenario FOREIGN KEY (manual_scenario_id) REFERENCES tms_manual_scenario );
🧹 Nitpick comments (3)
migrations/103_tms_initial.up.sql (3)
163-181: Add single-column index on test_folder_id for efficient folder-level queries.Queries filtering by
test_folder_idalone (e.g., "all cases in folder") cannot efficiently use the composite UNIQUE index on (name, test_folder_id). Add a standalone B-tree index to prevent sequential scans on large datasets.Add after line 181:
CONSTRAINT tms_test_case_name_folder_unique UNIQUE (name, test_folder_id) ); +CREATE INDEX idx_tms_test_case_test_folder_id ON tms_test_case (test_folder_id);
273-284: Remove redundant unique index on manual_scenario_id.Line 277 declares
manual_scenario_idwith inlineUNIQUE, and lines 283–284 create a redundant index on the same column. The inline constraint is sufficient; the explicit index adds maintenance overhead without benefit.Remove lines 283-284:
value varchar(255) ); - -CREATE UNIQUE INDEX idx_tms_manual_scenario_preconditions_scenario_unique - ON tms_manual_scenario_preconditions(manual_scenario_id);
260-271: Clarify constraint syntax on test_case_version_id for readability.Lines 267–269 mix inline
UNIQUEwithCONSTRAINTin an unusual way. While PostgreSQL accepts this, it's clearer to separate the constraint definition.Refactor for clarity:
CREATE TABLE tms_manual_scenario ( id BIGSERIAL CONSTRAINT tms_manual_scenario_pk PRIMARY KEY, execution_estimation_time integer, link_to_requirements varchar(255), test_case_version_id bigint - UNIQUE CONSTRAINT tms_manual_scenario_fk_test_case_version + UNIQUE REFERENCES tms_test_case_version, type tms_manual_scenario_type NOT NULL );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
migrations/103_tms_initial.down.sql(1 hunks)migrations/103_tms_initial.up.sql(1 hunks)
🔇 Additional comments (1)
migrations/103_tms_initial.down.sql (1)
79-95: No issues found—launch columns are safe to drop.Verification confirms
test_plan_idandlaunch_typecolumns contain no NOT NULL or FK constraints. Both DROP COLUMN IF EXISTS statements are idempotent and will safely complete the rollback.
migrations/103_tms_initial.down.sql
Outdated
| DROP INDEX IF EXISTS idx_tms_manual_scenario_preconditions_attachment_attachment_id; | ||
| DROP INDEX IF EXISTS idx_tms_manual_scenario_preconditions_attachment_preconditions_id; | ||
| DROP TABLE IF EXISTS tms_manual_scenario_preconditions_attachment; | ||
|
|
||
| DROP INDEX IF EXISTS idx_preconditions_attachment_attachment_id; | ||
| DROP INDEX IF EXISTS idx_preconditions_attachment_preconditions_id; | ||
| DROP INDEX IF EXISTS idx_tms_text_manual_scenario_attachment_scenario_id; | ||
| DROP INDEX IF EXISTS idx_tms_text_manual_scenario_attachment_attachment_id; | ||
| DROP TABLE IF EXISTS tms_text_manual_scenario_attachment; | ||
|
|
||
| DROP INDEX IF EXISTS idx_tms_step_attachment_attachment_id; | ||
| DROP INDEX IF EXISTS idx_tms_step_attachment_step_id; | ||
| DROP TABLE IF EXISTS tms_step_attachment; | ||
|
|
||
| DROP INDEX IF EXISTS idx_tms_attachment_path; | ||
| DROP INDEX IF EXISTS idx_tms_attachment_expires_at; | ||
| DROP TABLE IF EXISTS tms_attachment; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify attachment table drops match UP migration index naming.
Lines 1–9 drop indexes for attachment tables. Confirm these names exactly match those in the UP migration. From the UP file:
- Line 376:
idx_tms_text_manual_scenario_attachment_scenario_id - Line 377:
idx_tms_text_manual_scenario_attachment_attachment_id - Lines 1–2 drop old index names; lines 5–8 drop correct names.
This appears to have been fixed from the prior review, but verify all attachment-related indexes and tables are correctly paired.
Run this to cross-check index names:
🏁 Script executed:
#!/bin/bash
# Extract all index creation lines from UP migration
echo "=== Indexes in UP migration ==="
grep -n "CREATE INDEX" migrations/103_tms_initial.up.sql | grep -E "attachment|text_manual|step_attachment|preconditions"
echo "=== Index drops in DOWN migration ==="
grep -n "DROP INDEX" migrations/103_tms_initial.down.sql | grep -E "attachment|text_manual|step_attachment|preconditions"Length of output: 1711
Lines 1–2 drop incorrect index names that don't exist in the UP migration.
The DOWN migration has a critical mismatch:
- Lines 1–2 drop
idx_tms_manual_scenario_preconditions_attachment_*— these indexes are never created in the UP migration. - Lines 5–6 drop
idx_preconditions_attachment_*— these are the correct names created in the UP migration (line 391–392).
The UP/DOWN migrations are asymmetric. Remove lines 1–2 entirely, or verify if these old index names should have been created in the UP migration but were replaced with the correct names at lines 5–6.
All other attachment-related indexes (lines 7–16) correctly match the UP migration.
🤖 Prompt for AI Agents
In migrations/103_tms_initial.down.sql around lines 1 to 2, the DOWN migration
attempts to drop indexes named
idx_tms_manual_scenario_preconditions_attachment_attachment_id and
idx_tms_manual_scenario_preconditions_attachment_preconditions_id which are not
created in the UP migration; remove these two DROP INDEX lines (lines 1–2) so
the DOWN migration matches the actual index names created in UP (the correct
drops for idx_preconditions_attachment_* remain at lines 5–6), or if those old
names were intended, instead add corresponding CREATE INDEX statements to the UP
migration — choose one fix and keep UP/DOWN symmetric.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
migrations/1000_tms_initial.up.sql (4)
452-458: Add NOT NULL constraint on launch.launch_type after backfill.The column is created with a default value and backfilled, but lacks a NOT NULL constraint. This allows unexpected NULLs in downstream logic and analytics.
Apply this diff to enforce data integrity after the backfill:
ALTER TABLE launch ALTER COLUMN launch_type SET DEFAULT 'AUTOMATION'; UPDATE launch SET launch_type = 'AUTOMATION'; +ALTER TABLE launch + ALTER COLUMN launch_type SET NOT NULL;
46-55: Apply all three data integrity constraints to tms_dataset_data.The review comment is accurate. The migration contains:
- Nullable
keycolumn (should be NOT NULL)- Misnamed FK constraint "tms_dataset_attribute_fk_tms_dataset" (should reference table name "tms_dataset_data")
- Missing uniqueness constraint on (dataset_id, key) to prevent duplicates
Apply the suggested diff to fix all three issues:
CREATE TABLE tms_dataset_data ( id BIGSERIAL CONSTRAINT tms_dataset_data_pk PRIMARY KEY, - key varchar(255), + key varchar(255) NOT NULL, value varchar(255), dataset_id bigint NOT NULL - CONSTRAINT tms_dataset_attribute_fk_tms_dataset + CONSTRAINT tms_dataset_data_fk_dataset REFERENCES tms_dataset, + CONSTRAINT tms_dataset_data_unique_key_per_dataset UNIQUE (dataset_id, key) );
286-294: 🔴 Critical: Fix invalid duplicate CONSTRAINT syntax on tms_text_manual_scenario and tms_steps_manual_scenario.Lines 289–290 and 299–300 incorrectly declare two inline
CONSTRAINTkeywords on the same column without theFOREIGN KEYkeyword on the second constraint. This is invalid SQL syntax. A column definition can only have one inline constraint; the PRIMARY KEY and FOREIGN KEY must be properly separated.Apply this diff to both tables:
CREATE TABLE tms_text_manual_scenario ( manual_scenario_id bigint NOT NULL - CONSTRAINT tms_text_manual_scenario_pk PRIMARY KEY - CONSTRAINT tms_text_manual_scenario_fk_manual_scenario + CONSTRAINT tms_text_manual_scenario_pk PRIMARY KEY, + CONSTRAINT tms_text_manual_scenario_fk_manual_scenario FOREIGN KEY (manual_scenario_id) REFERENCES tms_manual_scenario, instructions varchar(255), expected_result varchar(255) );CREATE TABLE tms_steps_manual_scenario ( manual_scenario_id bigint NOT NULL - CONSTRAINT tms_steps_manual_scenario_pk PRIMARY KEY - CONSTRAINT tms_steps_manual_scenario_fk_manual_scenario + CONSTRAINT tms_steps_manual_scenario_pk PRIMARY KEY, + CONSTRAINT tms_steps_manual_scenario_fk_manual_scenario FOREIGN KEY (manual_scenario_id) REFERENCES tms_manual_scenario );
296-302: 🔴 Critical: Fix invalid duplicate CONSTRAINT syntax in two tables.Two tables have the same syntax error—dual
CONSTRAINTkeywords on the same column (lines 289–290 and 299–300):
tms_text_manual_scenario(lines 289–290)tms_steps_manual_scenario(lines 299–300)Apply this pattern to both tables:
CREATE TABLE tms_text_manual_scenario ( manual_scenario_id bigint NOT NULL - CONSTRAINT tms_text_manual_scenario_pk PRIMARY KEY - CONSTRAINT tms_text_manual_scenario_fk_manual_scenario + CONSTRAINT tms_text_manual_scenario_pk PRIMARY KEY, + CONSTRAINT tms_text_manual_scenario_fk_manual_scenario FOREIGN KEY (manual_scenario_id) REFERENCES tms_manual_scenario,CREATE TABLE tms_steps_manual_scenario ( manual_scenario_id bigint NOT NULL - CONSTRAINT tms_steps_manual_scenario_pk PRIMARY KEY - CONSTRAINT tms_steps_manual_scenario_fk_manual_scenario + CONSTRAINT tms_steps_manual_scenario_pk PRIMARY KEY, + CONSTRAINT tms_steps_manual_scenario_fk_manual_scenario FOREIGN KEY (manual_scenario_id) REFERENCES tms_manual_scenariomigrations/1000_tms_initial.down.sql (1)
1-9: Correct mismatched and duplicated index drop statements.Lines 1–2 attempt to drop indexes with the long "tms_manual_scenario_preconditions_attachment_" prefix that are never created in the up migration. The actual indexes for
tms_manual_scenario_preconditions_attachmentare namedidx_preconditions_attachment_*(lines 391–392 in the up migration). The down migration incorrectly tries to drop both the non-existent long-prefix versions (lines 1–2) and the correct short-prefix versions (lines 5–6) for the same table, creating confusion after dropping the table itself at line 3.Apply the suggested diff to remove the incorrect index drops and reorganize by table:
-DROP INDEX IF EXISTS idx_tms_manual_scenario_preconditions_attachment_attachment_id; -DROP INDEX IF EXISTS idx_tms_manual_scenario_preconditions_attachment_preconditions_id; -DROP TABLE IF EXISTS tms_manual_scenario_preconditions_attachment; - -DROP INDEX IF EXISTS idx_preconditions_attachment_attachment_id; -DROP INDEX IF EXISTS idx_preconditions_attachment_preconditions_id; -DROP INDEX IF EXISTS idx_tms_text_manual_scenario_attachment_scenario_id; -DROP INDEX IF EXISTS idx_tms_text_manual_scenario_attachment_attachment_id; -DROP TABLE IF EXISTS tms_text_manual_scenario_attachment; +-- tms_text_manual_scenario_attachment +DROP INDEX IF EXISTS idx_tms_text_manual_scenario_attachment_attachment_id; +DROP INDEX IF EXISTS idx_tms_text_manual_scenario_attachment_scenario_id; +DROP TABLE IF EXISTS tms_text_manual_scenario_attachment; + +-- tms_manual_scenario_preconditions_attachment +DROP INDEX IF EXISTS idx_preconditions_attachment_attachment_id; +DROP INDEX IF EXISTS idx_preconditions_attachment_preconditions_id; +DROP TABLE IF EXISTS tms_manual_scenario_preconditions_attachment;
🧹 Nitpick comments (1)
migrations/1000_tms_initial.up.sql (1)
163-181: Add single-column index on tms_test_case.test_folder_id for efficient folder-level queries.The table has a composite unique index on
(name, test_folder_id), but queries listing all test cases within a folder will not use it efficiently for the folder ID lookup alone. Add a dedicated index to speed up folder-level listings, deletions, and other folder-scoped operations.Apply this diff after the table definition:
CONSTRAINT tms_test_case_name_folder_unique UNIQUE (name, test_folder_id) ); +-- Speed up folder-level listings and deletes +CREATE INDEX idx_tms_test_case_test_folder_id ON tms_test_case (test_folder_id); + CREATE TABLE tms_test_plan_test_case
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
migrations/1000_tms_initial.down.sql(1 hunks)migrations/1000_tms_initial.up.sql(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Call feature Docker build / Build and export to AWS ECR
imashkanov
force-pushed
the
feature/EPMRPP-migrate-tms-to-develop
branch
from
6cdcdfb to
9559a6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
migrations/103_tms_initial.down.sql (1)
1-9: Remove duplicate/incorrect index drops for tms_manual_scenario_preconditions_attachment.Lines 1-2 attempt to drop indexes with names
idx_tms_manual_scenario_preconditions_attachment_*which are never created in the UP migration. Lines 5-6 correctly drop the actual indexes that exist in UP migration (lines 391-392). This creates redundancy and confusion.Remove lines 1-2 entirely since those index names do not exist in the UP migration.
Apply this diff:
-DROP INDEX IF EXISTS idx_tms_manual_scenario_preconditions_attachment_attachment_id; -DROP INDEX IF EXISTS idx_tms_manual_scenario_preconditions_attachment_preconditions_id; DROP TABLE IF EXISTS tms_manual_scenario_preconditions_attachment; DROP INDEX IF EXISTS idx_preconditions_attachment_attachment_id; DROP INDEX IF EXISTS idx_preconditions_attachment_preconditions_id;migrations/103_tms_initial.up.sql (3)
286-294: Critical: Fix duplicate CONSTRAINT syntax on tms_text_manual_scenario.A column cannot have two inline CONSTRAINT definitions on the same line. The PRIMARY KEY and FOREIGN KEY constraints are both specified, which is invalid SQL syntax.
Apply this diff to separate the constraints:
CREATE TABLE tms_text_manual_scenario ( manual_scenario_id bigint NOT NULL - CONSTRAINT tms_text_manual_scenario_pk PRIMARY KEY - CONSTRAINT tms_text_manual_scenario_fk_manual_scenario + CONSTRAINT tms_text_manual_scenario_pk PRIMARY KEY, + CONSTRAINT tms_text_manual_scenario_fk_manual_scenario FOREIGN KEY (manual_scenario_id) REFERENCES tms_manual_scenario, instructions varchar(255), expected_result varchar(255) );
46-55: Fix FK constraint name and add uniqueness constraint on tms_dataset_data.The foreign key constraint is named
tms_dataset_attribute_fk_tms_dataset(references "attribute" erroneously—this table is dataset_data). Additionally, there is no per-dataset uniqueness guarantee for keys, which should be enforced.Apply this diff:
CREATE TABLE tms_dataset_data ( id BIGSERIAL CONSTRAINT tms_dataset_data_pk PRIMARY KEY, - key varchar(255), + key varchar(255) NOT NULL, value varchar(255), dataset_id bigint NOT NULL - CONSTRAINT tms_dataset_attribute_fk_tms_dataset - REFERENCES tms_dataset + CONSTRAINT tms_dataset_data_fk_dataset + REFERENCES tms_dataset, + CONSTRAINT tms_dataset_data_unique_key_per_dataset UNIQUE (dataset_id, key) );
296-302: Critical: Fix duplicate CONSTRAINT syntax on tms_steps_manual_scenario.Same issue as tms_text_manual_scenario: two inline CONSTRAINT definitions on the same column is invalid SQL syntax.
Apply this diff to separate the constraints:
CREATE TABLE tms_steps_manual_scenario ( manual_scenario_id bigint NOT NULL - CONSTRAINT tms_steps_manual_scenario_pk PRIMARY KEY - CONSTRAINT tms_steps_manual_scenario_fk_manual_scenario + CONSTRAINT tms_steps_manual_scenario_pk PRIMARY KEY, + CONSTRAINT tms_steps_manual_scenario_fk_manual_scenario FOREIGN KEY (manual_scenario_id) REFERENCES tms_manual_scenario );
🧹 Nitpick comments (2)
migrations/103_tms_initial.up.sql (2)
163-181: Add single-column index on test_folder_id for efficient folder lookups.A UNIQUE constraint exists on
(name, test_folder_id), but queries for "all test cases in a folder" will not efficiently use this composite index. Add a dedicated single-column B-tree index ontest_folder_id.Apply this diff after line 181:
CONSTRAINT tms_test_case_name_folder_unique UNIQUE (name, test_folder_id) ); +-- Speed up folder-level listings and deletes +CREATE INDEX idx_tms_test_case_test_folder_id ON tms_test_case (test_folder_id);
452-458: Add NOT NULL constraint on launch_type after backfill.The column is created, defaulted, and backfilled, but remains nullable. This allows NULLs to exist in future inserts via explicit INSERTs, complicating downstream logic and analytics. Lock in the constraint after backfill completes.
Apply this diff:
CREATE TYPE LAUNCH_TYPE_ENUM AS ENUM ('AUTOMATION', 'MANUAL'); ALTER TABLE launch ADD COLUMN launch_type LAUNCH_TYPE_ENUM; ALTER TABLE launch ALTER COLUMN launch_type SET DEFAULT 'AUTOMATION'; UPDATE launch SET launch_type = 'AUTOMATION'; +ALTER TABLE launch + ALTER COLUMN launch_type SET NOT NULL;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
migrations/103_tms_initial.down.sql(1 hunks)migrations/103_tms_initial.up.sql(1 hunks)
imashkanov
force-pushed
the
feature/EPMRPP-migrate-tms-to-develop
branch
from
6447b89 to
c9a560c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
migrations/1000_tms_initial.up.sql (3)
298-304: Critical: Fix duplicate CONSTRAINT syntax on tms_steps_manual_scenario.Identical issue to tms_text_manual_scenario: two inline
CONSTRAINTdefinitions on the same column is invalid SQL syntax and will cause the migration to fail.Separate the constraints by moving the FOREIGN KEY to table level:
CREATE TABLE tms_steps_manual_scenario ( manual_scenario_id bigint NOT NULL - CONSTRAINT tms_steps_manual_scenario_pk PRIMARY KEY - CONSTRAINT tms_steps_manual_scenario_fk_manual_scenario - REFERENCES tms_manual_scenario + CONSTRAINT tms_steps_manual_scenario_pk PRIMARY KEY, + CONSTRAINT tms_steps_manual_scenario_fk_manual_scenario FOREIGN KEY (manual_scenario_id) + REFERENCES tms_manual_scenario );
46-55: Fix FK naming typo and add uniqueness constraint on tms_dataset_data.The foreign key constraint is named
tms_dataset_attribute_fk_tms_dataset(appears copy-pasted from the tms_attribute table), and there's no UNIQUE constraint to prevent duplicate keys within a dataset.Rename the FK to correctly reflect the tms_dataset_data table, make the key column NOT NULL, and add a UNIQUE constraint:
CREATE TABLE tms_dataset_data ( id BIGSERIAL CONSTRAINT tms_dataset_data_pk PRIMARY KEY, - key varchar(255), + key varchar(255) NOT NULL, value varchar(255), dataset_id bigint NOT NULL - CONSTRAINT tms_dataset_attribute_fk_tms_dataset - REFERENCES tms_dataset + CONSTRAINT tms_dataset_data_fk_dataset + REFERENCES tms_dataset, + CONSTRAINT tms_dataset_data_unique_key_per_dataset UNIQUE (dataset_id, key) );
288-296: Critical: Fix duplicate CONSTRAINT syntax on tms_text_manual_scenario.A single column cannot have two inline
CONSTRAINTdefinitions. This is invalid SQL syntax that will cause the migration to fail.Separate the PRIMARY KEY and FOREIGN KEY constraints by moving the FK to a table-level constraint:
CREATE TABLE tms_text_manual_scenario ( manual_scenario_id bigint NOT NULL - CONSTRAINT tms_text_manual_scenario_pk PRIMARY KEY - CONSTRAINT tms_text_manual_scenario_fk_manual_scenario - REFERENCES tms_manual_scenario, + CONSTRAINT tms_text_manual_scenario_pk PRIMARY KEY, + CONSTRAINT tms_text_manual_scenario_fk_manual_scenario FOREIGN KEY (manual_scenario_id) + REFERENCES tms_manual_scenario, instructions varchar(255), expected_result varchar(255) );
🧹 Nitpick comments (2)
migrations/1000_tms_initial.up.sql (2)
165-183: Add a single-column index on tms_test_case.test_folder_id for efficient folder lookups.Queries filtering test cases by folder will not use the existing composite UNIQUE constraint
(name, test_folder_id)efficiently. A dedicated single-column index will improve performance for folder-level queries and deletes.Add a B-tree index after the table definition:
CREATE TABLE tms_test_case ( id BIGSERIAL CONSTRAINT tms_test_case_pk PRIMARY KEY, created_at TIMESTAMP DEFAULT now() NOT NULL, updated_at TIMESTAMP DEFAULT now() NOT NULL, name varchar(255), description varchar(255), priority varchar(255), search_vector tsvector, external_id varchar(255), test_folder_id bigint NOT NULL CONSTRAINT tms_test_case_fk_test_folder REFERENCES tms_test_folder, dataset_id bigint CONSTRAINT tms_test_case_fk_dataset REFERENCES tms_dataset, CONSTRAINT tms_test_case_name_folder_unique UNIQUE (name, test_folder_id) ); + +CREATE INDEX idx_tms_test_case_test_folder_id ON tms_test_case (test_folder_id);
454-460: Add NOT NULL constraint to launch.launch_type after backfill.The column is created with a default, backfilled for existing rows, but lacks a NOT NULL constraint. This allows future NULLs and complicates downstream logic and analytics.
Enforce NOT NULL after the backfill and default are applied:
ALTER TABLE launch ALTER COLUMN launch_type SET DEFAULT 'AUTOMATION'; UPDATE launch SET launch_type = 'AUTOMATION'; +ALTER TABLE launch + ALTER COLUMN launch_type SET NOT NULL;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
migrations/1000_tms_initial.down.sql(1 hunks)migrations/1000_tms_initial.up.sql(1 hunks)
migrations/1000_tms_initial.down.sql
Outdated
| DROP INDEX IF EXISTS idx_tms_manual_scenario_preconditions_attachment_attachment_id; | ||
| DROP INDEX IF EXISTS idx_tms_manual_scenario_preconditions_attachment_preconditions_id; | ||
| DROP TABLE IF EXISTS tms_manual_scenario_preconditions_attachment; | ||
|
|
||
| DROP INDEX IF EXISTS idx_preconditions_attachment_attachment_id; | ||
| DROP INDEX IF EXISTS idx_preconditions_attachment_preconditions_id; | ||
| DROP INDEX IF EXISTS idx_tms_text_manual_scenario_attachment_scenario_id; | ||
| DROP INDEX IF EXISTS idx_tms_text_manual_scenario_attachment_attachment_id; | ||
| DROP TABLE IF EXISTS tms_text_manual_scenario_attachment; | ||
|
|
||
| DROP INDEX IF EXISTS idx_tms_step_attachment_attachment_id; | ||
| DROP INDEX IF EXISTS idx_tms_step_attachment_step_id; | ||
| DROP TABLE IF EXISTS tms_step_attachment; | ||
|
|
||
| DROP INDEX IF EXISTS idx_tms_attachment_path; | ||
| DROP INDEX IF EXISTS idx_tms_attachment_expires_at; | ||
| DROP TABLE IF EXISTS tms_attachment; | ||
|
|
||
| DROP INDEX IF EXISTS idx_tms_test_folder_test_item_test_folder_id; | ||
| DROP INDEX IF EXISTS idx_tms_test_folder_test_item_test_item_id; | ||
| DROP TABLE IF EXISTS tms_test_folder_test_item; | ||
|
|
||
| DROP INDEX IF EXISTS idx_tms_step_test_item_step_id; | ||
| DROP INDEX IF EXISTS idx_tms_step_test_item_test_item_id; | ||
| DROP TABLE IF EXISTS tms_step_test_item; | ||
|
|
||
| DROP INDEX IF EXISTS idx_tms_test_case_test_item_test_case_id; | ||
| DROP INDEX IF EXISTS idx_tms_test_case_test_item_test_item_id; | ||
| DROP TABLE IF EXISTS tms_test_case_test_item; | ||
|
|
||
| DROP INDEX IF EXISTS idx_tms_test_case_execution_snapshot; | ||
| DROP INDEX IF EXISTS idx_tms_test_case_execution_case_item; | ||
| DROP INDEX IF EXISTS idx_tms_test_case_execution_version_id; | ||
| DROP INDEX IF EXISTS idx_tms_test_case_execution_test_item_id; | ||
| DROP INDEX IF EXISTS idx_tms_test_case_execution_test_case_id; | ||
| DROP TABLE IF EXISTS tms_test_case_execution; | ||
|
|
||
| DROP INDEX IF EXISTS idx_tms_test_case_search_vector; | ||
| DROP INDEX IF EXISTS idx_tms_test_plan_search_vector; | ||
| DROP INDEX IF EXISTS idx_tms_attribute_search_vector; | ||
| DROP INDEX IF EXISTS idx_tms_test_folder_project_id; | ||
|
|
||
| DROP TRIGGER IF EXISTS tms_test_case_search_vector_trigger ON tms_test_case; | ||
| DROP TRIGGER IF EXISTS tms_test_plan_search_vector_trigger ON tms_test_plan; | ||
| DROP TRIGGER IF EXISTS tms_attribute_search_vector_trigger ON tms_attribute; | ||
|
|
||
| DROP FUNCTION IF EXISTS update_tms_test_case_search_vector(); | ||
| DROP FUNCTION IF EXISTS update_tms_test_plan_search_vector(); | ||
| DROP FUNCTION IF EXISTS update_tms_attribute_search_vector(); | ||
|
|
||
| DROP TABLE IF EXISTS tms_test_plan_attribute; | ||
| DROP TABLE IF EXISTS tms_test_case_attribute; | ||
| DROP TABLE IF EXISTS tms_manual_scenario_attribute; | ||
|
|
||
| DROP TABLE IF EXISTS tms_step; | ||
|
|
||
| DROP TABLE IF EXISTS tms_steps_manual_scenario; | ||
| DROP TABLE IF EXISTS tms_text_manual_scenario; | ||
|
|
||
| DROP INDEX IF EXISTS idx_tms_manual_scenario_preconditions_scenario_unique; | ||
| DROP TABLE IF EXISTS tms_manual_scenario_preconditions; | ||
|
|
||
| DROP INDEX IF EXISTS idx_tms_manual_scenario_type; | ||
| DROP TABLE IF EXISTS tms_manual_scenario; | ||
|
|
||
| DROP TYPE IF EXISTS tms_manual_scenario_type; | ||
|
|
||
| DROP INDEX IF EXISTS idx_tms_test_case_version_default; | ||
| DROP TABLE IF EXISTS tms_test_case_version; | ||
|
|
||
| DROP INDEX IF EXISTS idx_tms_test_plan_test_case_test_plan_id; | ||
| DROP INDEX IF EXISTS idx_tms_test_plan_test_case_test_case_id; | ||
| DROP TABLE IF EXISTS tms_test_plan_test_case; | ||
|
|
||
| DROP INDEX IF EXISTS idx_tms_test_case_launch_test_case_id; | ||
| DROP INDEX IF EXISTS idx_tms_test_case_launch_launch_id; | ||
| DROP TABLE IF EXISTS tms_test_case_launch; | ||
|
|
||
| DROP TABLE IF EXISTS tms_test_case; | ||
| DROP TABLE IF EXISTS tms_test_folder; | ||
| DROP TABLE IF EXISTS tms_milestone; | ||
| DROP TABLE IF EXISTS tms_test_plan; | ||
| DROP TABLE IF EXISTS tms_environment_dataset; | ||
| DROP TABLE IF EXISTS tms_environment; | ||
| DROP TABLE IF EXISTS tms_product_version; | ||
| DROP TABLE IF EXISTS tms_dataset_data; | ||
| DROP TABLE IF EXISTS tms_dataset; | ||
|
|
||
| DROP TYPE IF EXISTS tms_dataset_type; | ||
|
|
||
| DROP TABLE IF EXISTS tms_attribute; | ||
|
|
||
| ALTER TABLE launch DROP COLUMN IF EXISTS test_plan_id; | ||
| ALTER TABLE launch DROP COLUMN IF EXISTS launch_type; | ||
| DROP TYPE IF EXISTS LAUNCH_TYPE_ENUM; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify down migration completeness by cross-referencing the up migration.
The down migration structure appears sound and the prior attachment index/table naming issue seems corrected. However, idempotence and completeness require verification that every object created in the up migration has a corresponding drop statement.
🏁 Script executed:
#!/bin/bash
# Description: Extract CREATE and DROP statements to verify 1:1 mapping
# Expected: All CREATE statements have corresponding DROP statements
echo "=== Extracting CREATE statements from up migration ==="
grep -E "^CREATE (TABLE|INDEX|TYPE|FUNCTION|TRIGGER)" migrations/1000_tms_initial.up.sql | sed 's/^CREATE //' | sed 's/ .*//' | sort | uniq -c
echo ""
echo "=== Extracting DROP statements from down migration ==="
grep -E "^DROP (TABLE|INDEX|TYPE|FUNCTION|TRIGGER)" migrations/1000_tms_initial.down.sql | sed 's/^DROP //' | sed 's/ .*//' | sort | uniq -c
echo ""
echo "=== Checking for index name consistency ==="
echo "Indexes created (from up):"
grep "^CREATE INDEX" migrations/1000_tms_initial.up.sql | awk '{print $3}'
echo ""
echo "Indexes dropped (from down):"
grep "^DROP INDEX" migrations/1000_tms_initial.down.sql | awk '{print $4}' | sort | uniqLength of output: 1430
The script has a syntax issue: the awk command extracts field $4 which is "EXISTS" instead of the index name. Let me run a corrected verification:Now let me run a corrected verification script to properly extract and compare the indexes:
🏁 Script executed:
#!/bin/bash
# Corrected: Extract index names properly from both migrations for accurate comparison
echo "=== Indexes CREATED in up migration ==="
grep "^CREATE INDEX" migrations/1000_tms_initial.up.sql | sed 's/^CREATE INDEX //' | sed 's/ ON .*//' | sed 's/ USING .*//' | sort > /tmp/created_indexes.txt
cat /tmp/created_indexes.txt
echo ""
echo "Count: $(wc -l < /tmp/created_indexes.txt)"
echo ""
echo "=== Indexes DROPPED in down migration ==="
grep "^DROP INDEX" migrations/1000_tms_initial.down.sql | sed 's/^DROP INDEX IF EXISTS //' | sed 's/;$//' | sort > /tmp/dropped_indexes.txt
cat /tmp/dropped_indexes.txt
echo ""
echo "Count: $(wc -l < /tmp/dropped_indexes.txt)"
echo ""
echo "=== MISSING from DOWN (created but not dropped) ==="
comm -23 /tmp/created_indexes.txt /tmp/dropped_indexes.txt
echo ""
echo "=== EXTRA in DOWN (dropped but not created) ==="
comm -13 /tmp/created_indexes.txt /tmp/dropped_indexes.txtLength of output: 2766
Down migration contains 4 orphaned DROP INDEX statements for indexes never created in the up migration.
Verification reveals a migration synchronization issue:
- Indexes created in up migration: 28
- Indexes dropped in down migration: 32 (4 extra)
The following indexes are dropped but never created:
idx_tms_manual_scenario_preconditions_attachment_attachment_ididx_tms_manual_scenario_preconditions_attachment_preconditions_ididx_tms_manual_scenario_preconditions_scenario_uniqueidx_tms_test_case_version_default
These orphaned DROP statements should be removed from the down migration (lines 1-9 and line 67 in migrations/1000_tms_initial.down.sql).
🤖 Prompt for AI Agents
migrations/1000_tms_initial.down.sql lines 1-9 and line 67: the down migration
contains four DROP INDEX statements for indexes that were never created in the
up migration (idx_tms_manual_scenario_preconditions_attachment_attachment_id,
idx_tms_manual_scenario_preconditions_attachment_preconditions_id,
idx_tms_manual_scenario_preconditions_scenario_unique,
idx_tms_test_case_version_default); remove those orphaned DROP INDEX lines so
the down migration mirrors the up migration (delete the two DROP INDEX lines at
the top for the manual_scenario_preconditions_attachment indexes, the DROP INDEX
for manual_scenario_preconditions_scenario_unique, and the DROP INDEX for
idx_tms_test_case_version_default at line 67).
…s_test_folder_test_item
migrations/1000_tms_initial.down.sql
Outdated
| @@ -0,0 +1,95 @@ | |||
| DROP INDEX IF EXISTS idx_tms_manual_scenario_preconditions_attachment_attachment_id; | |||
There was a problem hiding this comment.
Can you please change the version of the file according to the latest version number in develop. I suppose it should me something near 218_..
…hes-to-migrate-tms-to-develop Feature/epmrpp manual launches to migrate tms to develop
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
migrations/1000_tms_initial.up.sql (5)
213-248: Add index for efficient folder-based queries.Queries filtering by
test_folder_idcannot efficiently use the composite unique constraint on(name, test_folder_id)and will require a full table scan or index scan.Recommended addition
CREATE INDEX idx_tms_test_case_search_vector ON tms_test_case USING gin (search_vector); +CREATE INDEX idx_tms_test_case_test_folder_id ON tms_test_case (test_folder_id);
66-75: Fix FK naming and add uniqueness constraint for dataset keys.The foreign key constraint name references "attribute" instead of "data", and duplicate keys within the same dataset are currently allowed.
Recommended fix
CREATE TABLE tms_dataset_data ( id BIGSERIAL CONSTRAINT tms_dataset_data_pk PRIMARY KEY, - key varchar(255), + key varchar(255) NOT NULL, value varchar(255), dataset_id bigint NOT NULL - CONSTRAINT tms_dataset_attribute_fk_tms_dataset - REFERENCES tms_dataset + CONSTRAINT tms_dataset_data_fk_dataset + REFERENCES tms_dataset, + CONSTRAINT tms_dataset_data_unique_key_per_dataset UNIQUE (dataset_id, key) );
320-328: Critical: Fix invalid duplicate CONSTRAINT syntax.A column cannot have two inline
CONSTRAINTdefinitions. The current syntax with bothPRIMARY KEYandFOREIGN KEYconstraints on the same line is invalid SQL and will fail execution.🔧 Required fix
CREATE TABLE tms_text_manual_scenario ( manual_scenario_id bigint NOT NULL - CONSTRAINT tms_text_manual_scenario_pk PRIMARY KEY - CONSTRAINT tms_text_manual_scenario_fk_manual_scenario - REFERENCES tms_manual_scenario, + CONSTRAINT tms_text_manual_scenario_pk PRIMARY KEY, + CONSTRAINT tms_text_manual_scenario_fk_manual_scenario + FOREIGN KEY (manual_scenario_id) REFERENCES tms_manual_scenario (id), instructions varchar(255), expected_result varchar(255) );
330-336: Critical: Fix invalid duplicate CONSTRAINT syntax.Same issue as
tms_text_manual_scenario: the column has two inlineCONSTRAINTdefinitions which is invalid SQL syntax.🔧 Required fix
CREATE TABLE tms_steps_manual_scenario ( manual_scenario_id bigint NOT NULL - CONSTRAINT tms_steps_manual_scenario_pk PRIMARY KEY - CONSTRAINT tms_steps_manual_scenario_fk_manual_scenario - REFERENCES tms_manual_scenario + CONSTRAINT tms_steps_manual_scenario_pk PRIMARY KEY, + CONSTRAINT tms_steps_manual_scenario_fk_manual_scenario + FOREIGN KEY (manual_scenario_id) REFERENCES tms_manual_scenario (id) );
534-542: Add NOT NULL constraint after backfill for data integrity.After backfilling all existing rows and setting a default, the
launch_typecolumn should be constrained toNOT NULLto prevent future NULL values and ensure data integrity.Recommended addition
ALTER TABLE launch ALTER COLUMN launch_type SET DEFAULT 'AUTOMATION'; UPDATE launch SET launch_type = 'AUTOMATION' WHERE launch_type IS NULL; + +ALTER TABLE launch + ALTER COLUMN launch_type SET NOT NULL;migrations/1000_tms_initial.down.sql (1)
1-222: Critical: Update migration version to match develop branch sequence.The migration version
1000will execute out of order. Based on the maintainer's comment, this should be sequentially numbered near218to match the latest version in the develop branch.Note: This issue also applies to
migrations/1000_tms_initial.up.sql— both files must be renamed with the correct sequential version number.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
migrations/1000_tms_initial.down.sql(1 hunks)migrations/1000_tms_initial.up.sql(1 hunks)
🔇 Additional comments (4)
migrations/1000_tms_initial.down.sql (3)
5-6: LGTM: Launch table rollback is correct.The column drops properly revert the additions made in the up migration and use
IF EXISTSfor idempotence.
8-189: LGTM: Proper reverse dependency order in rollback.The down migration correctly drops objects in reverse dependency order (most dependent first, base objects last) with appropriate
IF EXISTSguards for idempotence.
195-213: LGTM: Enum cleanup with documented limitation.The enum type drops correctly revert the types created in the up migration. The comment appropriately documents that removing values added to existing enums (
FULL_TEXT_SEARCHtofilter_condition_enumandTO_RUNtostatus_enum) is complex and omitted for development scenarios.migrations/1000_tms_initial.up.sql (1)
547-557: LGTM: Safe enum value additions with idempotence guard.The enum value additions use
IF NOT EXISTSto ensure idempotence and are correctly documented in the down migration as not being reverted (due to PostgreSQL limitations).
| CREATE TABLE tms_test_folder_test_item | ||
| ( | ||
| id BIGSERIAL PRIMARY KEY, | ||
| name varchar(255), | ||
| description varchar(255), | ||
| test_folder_id bigint NOT NULL, | ||
| launch_id bigint NOT NULL, | ||
| test_item_id bigint NOT NULL | ||
| CONSTRAINT tms_test_folder_test_item_fk_test_item | ||
| REFERENCES test_item | ||
| ); | ||
|
|
||
| CREATE INDEX idx_tms_test_folder_test_item_test_folder_id ON tms_test_folder_test_item (test_folder_id); | ||
| CREATE INDEX idx_tms_test_folder_test_item_test_item_id ON tms_test_folder_test_item (test_item_id); |
There was a problem hiding this comment.
Add missing foreign key constraints for referential integrity.
The tms_test_folder_test_item table has two columns without foreign key constraints:
test_folder_idshould referencetms_test_folderlaunch_idshould referencelaunch
This allows invalid references to non-existent folders or launches.
Recommended fix
CREATE TABLE tms_test_folder_test_item
(
id BIGSERIAL PRIMARY KEY,
name varchar(255),
description varchar(255),
test_folder_id bigint NOT NULL
+ CONSTRAINT tms_test_folder_test_item_fk_test_folder
+ REFERENCES tms_test_folder,
launch_id bigint NOT NULL
+ CONSTRAINT tms_test_folder_test_item_fk_launch
+ REFERENCES launch,
test_item_id bigint NOT NULL
CONSTRAINT tms_test_folder_test_item_fk_test_item
REFERENCES test_item
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CREATE TABLE tms_test_folder_test_item | |
| ( | |
| id BIGSERIAL PRIMARY KEY, | |
| name varchar(255), | |
| description varchar(255), | |
| test_folder_id bigint NOT NULL, | |
| launch_id bigint NOT NULL, | |
| test_item_id bigint NOT NULL | |
| CONSTRAINT tms_test_folder_test_item_fk_test_item | |
| REFERENCES test_item | |
| ); | |
| CREATE INDEX idx_tms_test_folder_test_item_test_folder_id ON tms_test_folder_test_item (test_folder_id); | |
| CREATE INDEX idx_tms_test_folder_test_item_test_item_id ON tms_test_folder_test_item (test_item_id); | |
| CREATE TABLE tms_test_folder_test_item | |
| ( | |
| id BIGSERIAL PRIMARY KEY, | |
| name varchar(255), | |
| description varchar(255), | |
| test_folder_id bigint NOT NULL | |
| CONSTRAINT tms_test_folder_test_item_fk_test_folder | |
| REFERENCES tms_test_folder, | |
| launch_id bigint NOT NULL | |
| CONSTRAINT tms_test_folder_test_item_fk_launch | |
| REFERENCES launch, | |
| test_item_id bigint NOT NULL | |
| CONSTRAINT tms_test_folder_test_item_fk_test_item | |
| REFERENCES test_item | |
| ); | |
| CREATE INDEX idx_tms_test_folder_test_item_test_folder_id ON tms_test_folder_test_item (test_folder_id); | |
| CREATE INDEX idx_tms_test_folder_test_item_test_item_id ON tms_test_folder_test_item (test_item_id); |
🤖 Prompt for AI Agents
In migrations/1000_tms_initial.up.sql around lines 194 to 207, the
tms_test_folder_test_item table defines test_folder_id and launch_id without
foreign key constraints; add foreign key constraints so test_folder_id
REFERENCES tms_test_folder(id) and launch_id REFERENCES launch(id) (choose
sensible constraint names, e.g. tms_test_folder_test_item_fk_test_folder and
tms_test_folder_test_item_fk_launch), and ensure an index exists on launch_id
(create idx_tms_test_folder_test_item_launch_id if missing); modify the CREATE
TABLE block to include these REFERENCES clauses or add ALTER TABLE ... ADD
CONSTRAINT statements immediately after the table creation.
| CREATE TABLE tms_step_execution | ||
| ( | ||
| id BIGSERIAL PRIMARY KEY, | ||
| test_case_execution_id bigint NOT NULL, | ||
| test_item_id bigint NOT NULL | ||
| CONSTRAINT tms_step_execution_test_item_fk_test_item | ||
| REFERENCES test_item, | ||
| launch_id bigint NOT NULL, | ||
| tms_step_id bigint | ||
| ); | ||
|
|
||
| CREATE INDEX idx_tms_step_execution_test_case ON tms_step_execution(test_case_execution_id); | ||
| CREATE INDEX idx_tms_step_execution_test_item ON tms_step_execution(test_item_id); | ||
| CREATE INDEX idx_tms_step_execution_launch ON tms_step_execution(launch_id); | ||
| CREATE INDEX idx_tms_step_execution_tms_step ON tms_step_execution(tms_step_id); |
There was a problem hiding this comment.
Add missing foreign key constraints for referential integrity.
The tms_step_execution table has three columns that should reference parent tables but lack foreign key constraints:
test_case_execution_id→tms_test_case_executionlaunch_id→launchtms_step_id→tms_step(optional/nullable)
Without these constraints, orphaned records can exist after parent deletions.
Recommended fix
CREATE TABLE tms_step_execution
(
id BIGSERIAL PRIMARY KEY,
test_case_execution_id bigint NOT NULL
+ CONSTRAINT tms_step_execution_fk_test_case_execution
+ REFERENCES tms_test_case_execution,
test_item_id bigint NOT NULL
CONSTRAINT tms_step_execution_test_item_fk_test_item
REFERENCES test_item,
launch_id bigint NOT NULL
+ CONSTRAINT tms_step_execution_fk_launch
+ REFERENCES launch,
tms_step_id bigint
+ CONSTRAINT tms_step_execution_fk_tms_step
+ REFERENCES tms_step
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CREATE TABLE tms_step_execution | |
| ( | |
| id BIGSERIAL PRIMARY KEY, | |
| test_case_execution_id bigint NOT NULL, | |
| test_item_id bigint NOT NULL | |
| CONSTRAINT tms_step_execution_test_item_fk_test_item | |
| REFERENCES test_item, | |
| launch_id bigint NOT NULL, | |
| tms_step_id bigint | |
| ); | |
| CREATE INDEX idx_tms_step_execution_test_case ON tms_step_execution(test_case_execution_id); | |
| CREATE INDEX idx_tms_step_execution_test_item ON tms_step_execution(test_item_id); | |
| CREATE INDEX idx_tms_step_execution_launch ON tms_step_execution(launch_id); | |
| CREATE INDEX idx_tms_step_execution_tms_step ON tms_step_execution(tms_step_id); | |
| CREATE TABLE tms_step_execution | |
| ( | |
| id BIGSERIAL PRIMARY KEY, | |
| test_case_execution_id bigint NOT NULL | |
| CONSTRAINT tms_step_execution_fk_test_case_execution | |
| REFERENCES tms_test_case_execution, | |
| test_item_id bigint NOT NULL | |
| CONSTRAINT tms_step_execution_test_item_fk_test_item | |
| REFERENCES test_item, | |
| launch_id bigint NOT NULL | |
| CONSTRAINT tms_step_execution_fk_launch | |
| REFERENCES launch, | |
| tms_step_id bigint | |
| CONSTRAINT tms_step_execution_fk_tms_step | |
| REFERENCES tms_step | |
| ); | |
| CREATE INDEX idx_tms_step_execution_test_case ON tms_step_execution(test_case_execution_id); | |
| CREATE INDEX idx_tms_step_execution_test_item ON tms_step_execution(test_item_id); | |
| CREATE INDEX idx_tms_step_execution_launch ON tms_step_execution(launch_id); | |
| CREATE INDEX idx_tms_step_execution_tms_step ON tms_step_execution(tms_step_id); |
🤖 Prompt for AI Agents
In migrations/1000_tms_initial.up.sql around lines 354 to 368, the
tms_step_execution table declares test_case_execution_id, launch_id and
tms_step_id but lacks foreign key constraints; add FOREIGN KEY constraints:
test_case_execution_id -> tms_test_case_execution(id) and launch_id ->
launch(id) (both NOT NULL) and tms_step_id -> tms_step(id) (nullable), and
include appropriate ON DELETE behavior (e.g., ON DELETE CASCADE for the NOT NULL
parents to avoid orphans, and ON DELETE SET NULL for the nullable tms_step_id)
so referential integrity is enforced.
| CREATE TABLE tms_test_case_execution | ||
| ( | ||
| id BIGSERIAL | ||
| CONSTRAINT tms_test_case_execution_pk PRIMARY KEY, | ||
| test_item_id bigint UNIQUE | ||
| CONSTRAINT tms_test_case_execution_fk_test_item | ||
| REFERENCES test_item, | ||
| priority varchar(255), | ||
| test_case_id bigint NOT NULL, | ||
| launch_id bigint NOT NULL, | ||
| test_case_version_id bigint NOT NULL, | ||
| test_case_snapshot jsonb NOT NULL | ||
| ); | ||
|
|
||
| CREATE INDEX idx_tms_test_case_execution_test_case_id ON tms_test_case_execution (test_case_id); | ||
| CREATE INDEX idx_tms_test_case_execution_test_item_id ON tms_test_case_execution (test_item_id); | ||
| CREATE INDEX idx_tms_test_case_execution_launch_id ON tms_test_case_execution (launch_id); | ||
| CREATE INDEX idx_tms_test_case_execution_version_id ON tms_test_case_execution (test_case_version_id); | ||
| CREATE INDEX idx_tms_test_case_execution_launch_case ON tms_test_case_execution (launch_id, test_case_id); | ||
| CREATE INDEX idx_tms_test_case_execution_snapshot ON tms_test_case_execution USING gin (test_case_snapshot); |
There was a problem hiding this comment.
Add missing foreign key constraints for execution records.
The tms_test_case_execution table has three critical columns without foreign key constraints:
test_case_idshould referencetms_test_caselaunch_idshould referencelaunchtest_case_version_idshould referencetms_test_case_version
These relationships are essential for maintaining execution data integrity and preventing orphaned records.
Recommended fix
CREATE TABLE tms_test_case_execution
(
id BIGSERIAL
CONSTRAINT tms_test_case_execution_pk PRIMARY KEY,
test_item_id bigint UNIQUE
CONSTRAINT tms_test_case_execution_fk_test_item
REFERENCES test_item,
priority varchar(255),
test_case_id bigint NOT NULL
+ CONSTRAINT tms_test_case_execution_fk_test_case
+ REFERENCES tms_test_case,
launch_id bigint NOT NULL
+ CONSTRAINT tms_test_case_execution_fk_launch
+ REFERENCES launch,
test_case_version_id bigint NOT NULL
+ CONSTRAINT tms_test_case_execution_fk_version
+ REFERENCES tms_test_case_version,
test_case_snapshot jsonb NOT NULL
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CREATE TABLE tms_test_case_execution | |
| ( | |
| id BIGSERIAL | |
| CONSTRAINT tms_test_case_execution_pk PRIMARY KEY, | |
| test_item_id bigint UNIQUE | |
| CONSTRAINT tms_test_case_execution_fk_test_item | |
| REFERENCES test_item, | |
| priority varchar(255), | |
| test_case_id bigint NOT NULL, | |
| launch_id bigint NOT NULL, | |
| test_case_version_id bigint NOT NULL, | |
| test_case_snapshot jsonb NOT NULL | |
| ); | |
| CREATE INDEX idx_tms_test_case_execution_test_case_id ON tms_test_case_execution (test_case_id); | |
| CREATE INDEX idx_tms_test_case_execution_test_item_id ON tms_test_case_execution (test_item_id); | |
| CREATE INDEX idx_tms_test_case_execution_launch_id ON tms_test_case_execution (launch_id); | |
| CREATE INDEX idx_tms_test_case_execution_version_id ON tms_test_case_execution (test_case_version_id); | |
| CREATE INDEX idx_tms_test_case_execution_launch_case ON tms_test_case_execution (launch_id, test_case_id); | |
| CREATE INDEX idx_tms_test_case_execution_snapshot ON tms_test_case_execution USING gin (test_case_snapshot); | |
| CREATE TABLE tms_test_case_execution | |
| ( | |
| id BIGSERIAL | |
| CONSTRAINT tms_test_case_execution_pk PRIMARY KEY, | |
| test_item_id bigint UNIQUE | |
| CONSTRAINT tms_test_case_execution_fk_test_item | |
| REFERENCES test_item, | |
| priority varchar(255), | |
| test_case_id bigint NOT NULL | |
| CONSTRAINT tms_test_case_execution_fk_test_case | |
| REFERENCES tms_test_case, | |
| launch_id bigint NOT NULL | |
| CONSTRAINT tms_test_case_execution_fk_launch | |
| REFERENCES launch, | |
| test_case_version_id bigint NOT NULL | |
| CONSTRAINT tms_test_case_execution_fk_version | |
| REFERENCES tms_test_case_version, | |
| test_case_snapshot jsonb NOT NULL | |
| ); | |
| CREATE INDEX idx_tms_test_case_execution_test_case_id ON tms_test_case_execution (test_case_id); | |
| CREATE INDEX idx_tms_test_case_execution_test_item_id ON tms_test_case_execution (test_item_id); | |
| CREATE INDEX idx_tms_test_case_execution_launch_id ON tms_test_case_execution (launch_id); | |
| CREATE INDEX idx_tms_test_case_execution_version_id ON tms_test_case_execution (test_case_version_id); | |
| CREATE INDEX idx_tms_test_case_execution_launch_case ON tms_test_case_execution (launch_id, test_case_id); | |
| CREATE INDEX idx_tms_test_case_execution_snapshot ON tms_test_case_execution USING gin (test_case_snapshot); |
…that should work by the part of the key
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
migrations/1000_tms_initial.down.sql (2)
94-94: Remove orphaned DROP INDEX statement.This index (
idx_tms_manual_scenario_preconditions_scenario_unique) is not created in the up migration, as confirmed by previous verification. Remove this line to ensure the down migration accurately mirrors the up migration.🔎 Suggested fix
-DROP INDEX IF EXISTS idx_tms_manual_scenario_preconditions_scenario_unique; DROP TABLE IF EXISTS tms_manual_scenario_preconditions;
108-108: Remove orphaned DROP INDEX statement.This index (
idx_tms_test_case_version_default) is not created in the up migration. Remove this line to maintain synchronization between the up and down migrations.🔎 Suggested fix
-DROP INDEX IF EXISTS idx_tms_test_case_version_default; DROP TABLE IF EXISTS tms_test_case_version;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
migrations/1000_tms_initial.down.sqlmigrations/1000_tms_initial.up.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- migrations/1000_tms_initial.up.sql
🔇 Additional comments (3)
migrations/1000_tms_initial.down.sql (3)
1-6: LGTM: Launch table reversion is correct.The column drops properly revert the additions made in the up migration, with appropriate
IF EXISTSguards for idempotence.
8-209: LGTM: Down migration structure and ordering are correct.The migration demonstrates proper dependency ordering:
- Indexes, triggers, and functions are dropped before their tables
- Child and junction tables are dropped before parent tables
- Enums/types are dropped last after all dependent tables
- Consistent use of
IF EXISTSguards ensures idempotence- Clear section organization aids maintainability
201-206: LGTM: Appropriate handling of shared extension.Correctly leaving the
pg_trgmextension drop commented out, as extensions are typically shared across the database and may be used by other schemas. The comment clearly marks this as optional.
| @@ -0,0 +1,209 @@ | |||
| -- ============================================================================ | |||
There was a problem hiding this comment.
Update migration version number to align with develop branch.
Based on past review comments, the file version should be changed from 1000 to approximately 218 to match the latest version in the develop branch. This affects the filename and ensures proper migration ordering.
🤖 Prompt for AI Agents
In migrations/1000_tms_initial.down.sql around line 1, the migration version in
the filename should be updated to align with the develop branch (use version
218); rename the file to migrations/218_tms_initial.down.sql and update any
internal version references or matching up/down pair filenames inside the repo
to use 218 so migrations are ordered correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
migrations/1000_tms_initial.up.sql (8)
66-75: Fix FK constraint name and add uniqueness guarantee for dataset keys.The FK constraint name refers to "attribute" but this is the tms_dataset_data table. Additionally, duplicate keys within the same dataset are currently allowed, which may lead to ambiguity.
Recommended changes
CREATE TABLE tms_dataset_data ( id BIGSERIAL CONSTRAINT tms_dataset_data_pk PRIMARY KEY, - key varchar(255), + key varchar(255) NOT NULL, value varchar(255), dataset_id bigint NOT NULL - CONSTRAINT tms_dataset_attribute_fk_tms_dataset - REFERENCES tms_dataset + CONSTRAINT tms_dataset_data_fk_dataset + REFERENCES tms_dataset, + CONSTRAINT tms_dataset_data_unique_key_per_dataset UNIQUE (dataset_id, key) );Based on past review comments (still unresolved).
194-207: Add missing foreign key constraints for referential integrity.The tms_test_folder_test_item table has test_folder_id and launch_id columns without FK constraints, allowing invalid references to non-existent folders or launches.
Recommended fix
CREATE TABLE tms_test_folder_test_item ( id BIGSERIAL PRIMARY KEY, name varchar(255), description varchar(255), - test_folder_id bigint NOT NULL, + test_folder_id bigint NOT NULL + CONSTRAINT tms_test_folder_test_item_fk_test_folder + REFERENCES tms_test_folder, - launch_id bigint NOT NULL, + launch_id bigint NOT NULL + CONSTRAINT tms_test_folder_test_item_fk_launch + REFERENCES launch, test_item_id bigint NOT NULL CONSTRAINT tms_test_folder_test_item_fk_test_item REFERENCES test_item );Based on past review comments (still unresolved).
319-327: Critical: Fix invalid SQL syntax with duplicate inline constraints.A column cannot have two inline CONSTRAINT definitions. The PRIMARY KEY and FOREIGN KEY constraints on manual_scenario_id must be separated.
Required fix
CREATE TABLE tms_text_manual_scenario ( manual_scenario_id bigint NOT NULL - CONSTRAINT tms_text_manual_scenario_pk PRIMARY KEY - CONSTRAINT tms_text_manual_scenario_fk_manual_scenario - REFERENCES tms_manual_scenario, + CONSTRAINT tms_text_manual_scenario_pk PRIMARY KEY, + CONSTRAINT tms_text_manual_scenario_fk_manual_scenario + FOREIGN KEY (manual_scenario_id) REFERENCES tms_manual_scenario, instructions TEXT, expected_result TEXT );Note: Past review marked this as addressed, but the issue persists in the current code.
329-335: Critical: Fix invalid SQL syntax with duplicate inline constraints.Same issue as tms_text_manual_scenario: two inline CONSTRAINT definitions on the same column is invalid SQL.
Required fix
CREATE TABLE tms_steps_manual_scenario ( manual_scenario_id bigint NOT NULL - CONSTRAINT tms_steps_manual_scenario_pk PRIMARY KEY - CONSTRAINT tms_steps_manual_scenario_fk_manual_scenario - REFERENCES tms_manual_scenario + CONSTRAINT tms_steps_manual_scenario_pk PRIMARY KEY, + CONSTRAINT tms_steps_manual_scenario_fk_manual_scenario + FOREIGN KEY (manual_scenario_id) REFERENCES tms_manual_scenario );Note: Past review marked this as addressed, but the issue persists in the current code.
353-368: Add missing foreign key constraints for execution records.The tms_step_execution table has three columns that should reference parent tables but lack FK constraints:
- test_case_execution_id → tms_test_case_execution
- launch_id → launch
- tms_step_id → tms_step (nullable)
Without these constraints, orphaned records can exist after parent deletions.
Recommended fix
CREATE TABLE tms_step_execution ( id BIGSERIAL PRIMARY KEY, - test_case_execution_id bigint NOT NULL, + test_case_execution_id bigint NOT NULL + CONSTRAINT tms_step_execution_fk_test_case_execution + REFERENCES tms_test_case_execution, test_item_id bigint NOT NULL CONSTRAINT tms_step_execution_test_item_fk_test_item REFERENCES test_item, - launch_id bigint NOT NULL, + launch_id bigint NOT NULL + CONSTRAINT tms_step_execution_fk_launch + REFERENCES launch, - tms_step_id bigint + tms_step_id bigint + CONSTRAINT tms_step_execution_fk_tms_step + REFERENCES tms_step );Based on past review comments (still unresolved).
480-500: Add missing foreign key constraints for execution records.The tms_test_case_execution table has three critical columns without FK constraints:
- test_case_id → tms_test_case
- launch_id → launch
- test_case_version_id → tms_test_case_version
These relationships are essential for maintaining execution data integrity and preventing orphaned records.
Recommended fix
CREATE TABLE tms_test_case_execution ( id BIGSERIAL CONSTRAINT tms_test_case_execution_pk PRIMARY KEY, test_item_id bigint UNIQUE CONSTRAINT tms_test_case_execution_fk_test_item REFERENCES test_item, priority varchar(255), - test_case_id bigint NOT NULL, + test_case_id bigint NOT NULL + CONSTRAINT tms_test_case_execution_fk_test_case + REFERENCES tms_test_case, - launch_id bigint NOT NULL, + launch_id bigint NOT NULL + CONSTRAINT tms_test_case_execution_fk_launch + REFERENCES launch, - test_case_version_id bigint NOT NULL, + test_case_version_id bigint NOT NULL + CONSTRAINT tms_test_case_execution_fk_version + REFERENCES tms_test_case_version, test_case_snapshot jsonb NOT NULL );Based on past review comments (still unresolved).
533-544: Add NOT NULL constraint on launch_type after backfill.The launch_type column is backfilled with 'AUTOMATION' and has a default, but nulls are still allowed. This complicates downstream logic and analytics.
Recommended addition
UPDATE launch SET launch_type = 'AUTOMATION' WHERE launch_type IS NULL; + +ALTER TABLE launch + ALTER COLUMN launch_type SET NOT NULL; ALTER TABLE launch ADD COLUMN IF NOT EXISTS test_plan_id bigint;Based on past review comments (still unresolved).
213-230: Add index for folder-based queries and consider unique constraint.Queries that list all test cases in a folder will not efficiently use the search_vector index. A single-column index on test_folder_id would improve performance. Additionally, the AI summary mentions a unique constraint on (name, test_folder_id), but it's missing from the schema.
Recommended additions
CREATE INDEX idx_tms_test_case_search_vector ON tms_test_case USING gin (search_vector); +CREATE INDEX idx_tms_test_case_test_folder_id ON tms_test_case (test_folder_id); + +-- If test case names should be unique per folder: +ALTER TABLE tms_test_case + ADD CONSTRAINT tms_test_case_name_folder_unique UNIQUE (name, test_folder_id);Based on past review comments and inconsistency with AI summary.
🧹 Nitpick comments (5)
migrations/1000_tms_initial.up.sql (5)
41-50: Consider adding ON DELETE behavior for referential integrity.The FK to project lacks an ON DELETE clause. If product versions should be removed when their project is deleted, add CASCADE; otherwise, add RESTRICT to make the intent explicit.
Suggested addition
project_id bigint NOT NULL CONSTRAINT tms_product_version_fk_project - REFERENCES project + REFERENCES project ON DELETE CASCADE
81-103: Consider adding ON DELETE CASCADE for project-scoped data.The tms_environment table references project without an ON DELETE clause. Adding CASCADE would ensure environments are removed when their project is deleted.
Suggested change
project_id bigint NOT NULL CONSTRAINT tms_environment_fk_project - REFERENCES project + REFERENCES project ON DELETE CASCADE
109-128: Consider ON DELETE behavior for milestone relationships.Both FKs (project and product_version) lack ON DELETE clauses. Consider adding CASCADE for project (milestones are project-scoped) and SET NULL or RESTRICT for product_version depending on business logic.
271-286: LGTM! Consider adding ON DELETE CASCADE.The partial unique index elegantly enforces the "one default version per test case" constraint. Consider adding ON DELETE CASCADE to the test_case FK so versions are automatically removed when their test case is deleted.
306-317: Remove redundant unique index.The manual_scenario_id column is already declared UNIQUE inline (line 310), making the separate unique index on lines 316-317 redundant.
Suggested change
manual_scenario_id bigint NOT NULL UNIQUE CONSTRAINT tms_manual_scenario_preconditions_fk_manual_scenario REFERENCES tms_manual_scenario, value varchar(255) ); - -CREATE UNIQUE INDEX idx_tms_manual_scenario_preconditions_scenario_unique - ON tms_manual_scenario_preconditions(manual_scenario_id);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
migrations/1000_tms_initial.up.sql
🔇 Additional comments (10)
migrations/1000_tms_initial.up.sql (10)
1-9: LGTM!The pg_trgm extension is properly guarded with IF NOT EXISTS and is required for the trigram-based search functionality used later in the schema.
11-16: LGTM!The enum definitions are well-structured and cover the necessary domain values for the TMS feature.
21-36: LGTM!The tms_attribute table has appropriate constraints, including ON DELETE CASCADE for the project reference and a unique constraint to prevent duplicate keys per project. The indexing strategy supports both trigram-based fuzzy search and efficient project-scoped lookups.
156-172: LGTM!The search vector implementation is correctly structured with a BEFORE trigger that maintains the tsvector, and the GIN index will efficiently support full-text search queries.
232-248: LGTM!The search vector trigger correctly indexes the test case name, description, and priority fields for full-text search.
253-266: LGTM!The many-to-many relationship is correctly implemented with a composite primary key and indexes supporting efficient queries from both directions.
373-435: LGTM!The attachment tables are well-structured with proper foreign key constraints, composite primary keys for join tables, and efficient indexing including a partial index on expires_at for non-null values.
440-474: LGTM!The attribute relationship tables correctly implement many-to-many associations with composite primary keys and proper foreign key constraints.
501-528: LGTM!The execution comment tables have proper foreign key constraints and indexing to support efficient comment and attachment lookups.
546-556: LGTM!The enum extensions properly use IF NOT EXISTS and add values that align with the new TMS functionality (full-text search filtering and manual test execution status).
4 participants
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.