feat(db): integrity CHECK constraints, financial-FK Restrict & index right-sizing#53
Open
kashkoool wants to merge 1 commit into
Open
feat(db): integrity CHECK constraints, financial-FK Restrict & index right-sizing#53kashkoool wants to merge 1 commit into
kashkoool wants to merge 1 commit into
Conversation
…right-sizing
Defence-in-depth schema hardening from the DB audit (no data loss; additive migration):
- CHECK constraints backing the domain rules: money >= 0 (trip/booking/payment/refund/promo),
review.Rating BETWEEN 1 AND 5, currency ~ '^[A-Z]{3}$', and status enums limited to their
valid set (trip/booking/payment/refund/company).
- Protect financial history: payment->booking and refund->payment FKs are now Restrict, and the
refund.BookingId column gains a real FK (Restrict) + IX_refund_BookingId.
- Index right-sizing on the write-heavy booking table and reports: add (TripId, Status),
(CreatedBy, CreatedAtUtc), trip(CompanyId, DepartureUtc), AspNetUsers(CompanyId, StaffType);
drop the redundant single-column booking TripId/Status/CreatedBy and standalone CompanyId
indexes (each a strict prefix of a new composite) -> faster reads, no extra write cost.
- DbConstraintsTests assert the CHECKs reject bad values and the Restrict FK blocks deleting a
booking that has a payment. Full suite green: 65 unit + 65 integration.
📝 WalkthroughWalkthroughThis PR hardens the PostgreSQL schema with database-level integrity constraints and optimizes indexes across hot tables. It adds CHECK constraints for non-negative amounts, valid currency format (3 uppercase letters), allowed status/rating ranges, and valid enum values; changes payment and refund FK delete behavior from CASCADE to RESTRICT to preserve financial history; introduces a new refund-booking FK; and replaces single-column indexes with right-sized composites for reporting and company/trip query patterns. ChangesDatabase Integrity Hardening and Index Optimization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@PROJECT_STATUS.md`:
- Around line 131-136: The summary overstates test coverage by saying "Covered
by `DbConstraintsTests`" even though `DbConstraintsTests` only verifies a subset
(e.g., RESTRICT delete behavior, payment amount CHECK, review rating CHECK);
update the wording in the `AddIntegrityConstraintsAndIndexTuning` section to
indicate partial rather than full coverage (e.g., "Partial coverage by
`DbConstraintsTests` — tests validate RESTRICT delete, payment amount CHECK, and
review rating CHECK"), and mention remaining untested items (currency regex,
status enums, refund→booking FK, index changes) so readers know what
`DbConstraintsTests` actually covers.
In
`@src/TransportPlatform.Infrastructure/Persistence/Migrations/20260608041228_AddIntegrityConstraintsAndIndexTuning.cs`:
- Around line 46-155: This migration adds multiple validating AddCheckConstraint
and AddForeignKey calls (see AddCheckConstraint for "CK_*" constraints and
AddForeignKey calls "FK_payment_booking_BookingId",
"FK_refund_booking_BookingId") which will fail if existing data violates them;
change the AddCheckConstraint/AddForeignKey work to create constraints NOT VALID
(use raw SQL via migrationBuilder.Sql to run ALTER TABLE ... ADD CONSTRAINT
<name> ... NOT VALID for each "CK_*" and ALTER TABLE ... ADD CONSTRAINT <fkname>
FOREIGN KEY (...) REFERENCES ... NOT VALID for the FK pairs), ensure any needed
indexes (e.g., IX_refund_BookingId, IX_booking_TripId_Status) are created first,
then add a separate follow-up migration that runs ALTER TABLE ... VALIDATE
CONSTRAINT <name> (and validates FKs) after data backfill/cleanup so the rollout
is phased and won’t fail on existing bad rows.
In `@tests/TransportPlatform.IntegrationTests/DbConstraintsTests.cs`:
- Around line 37-38: Replace the broad
Assert.ThrowsAnyAsync<DbUpdateException>(() => db.SaveChangesAsync()) checks
with an Assert.ThrowsAsync<DbUpdateException> that captures the exception (e.g.,
var ex = await Assert.ThrowsAsync<DbUpdateException>(...)) and then assert the
specific constraint/foreign-key identifier in the underlying DB error
message/inner exception (e.g., Assert.Contains("EXPECTED_CONSTRAINT_NAME",
ex.InnerException?.Message ?? ex.Message)); apply the same change for the other
occurrences referenced (lines shown as 53-54 and 68-69) so each test explicitly
verifies the expected constraint/FK name rather than any DB update error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: afb97c1c-9fb6-4b48-8a40-c1950cf972b2
⛔ Files ignored due to path filters (2)
src/TransportPlatform.Infrastructure/Persistence/Migrations/20260608041228_AddIntegrityConstraintsAndIndexTuning.Designer.csis excluded by!**/Migrations/*.Designer.cssrc/TransportPlatform.Infrastructure/Persistence/Migrations/ApplicationDbContextModelSnapshot.csis excluded by!**/Migrations/*ModelSnapshot.cs
📒 Files selected for processing (11)
PROJECT_STATUS.mdsrc/TransportPlatform.Infrastructure/Persistence/Configurations/ApplicationUserConfiguration.cssrc/TransportPlatform.Infrastructure/Persistence/Configurations/BookingConfiguration.cssrc/TransportPlatform.Infrastructure/Persistence/Configurations/CompanyConfiguration.cssrc/TransportPlatform.Infrastructure/Persistence/Configurations/PaymentConfiguration.cssrc/TransportPlatform.Infrastructure/Persistence/Configurations/PromoCodeConfiguration.cssrc/TransportPlatform.Infrastructure/Persistence/Configurations/RefundConfiguration.cssrc/TransportPlatform.Infrastructure/Persistence/Configurations/ReviewConfiguration.cssrc/TransportPlatform.Infrastructure/Persistence/Configurations/TripConfiguration.cssrc/TransportPlatform.Infrastructure/Persistence/Migrations/20260608041228_AddIntegrityConstraintsAndIndexTuning.cstests/TransportPlatform.IntegrationTests/DbConstraintsTests.cs
Comment on lines
+131
to
+136
| > **DB integrity hardening** (migration `AddIntegrityConstraintsAndIndexTuning`): DB-level **CHECK | ||
| > constraints** now back the domain rules (money ≥ 0, `review.Rating` 1–5, currency `^[A-Z]{3}$`, | ||
| > status enums in their valid set); **payment/refund FKs are `Restrict`** + a real `refund→booking` | ||
| > FK so financial history can't be deleted; and `booking`/`trip`/`AspNetUsers` indexes were | ||
| > **right-sized** (report composites added, redundant single-column indexes dropped — faster reads, | ||
| > no extra write cost). Covered by `DbConstraintsTests`. |
There was a problem hiding this comment.
Avoid overstating test coverage in the hardening summary.
At Line 136, “Covered by DbConstraintsTests” reads as full coverage, but tests currently validate a subset (RESTRICT delete, payment amount CHECK, review rating CHECK). Consider clarifying this as partial coverage.
Suggested wording
-> **right-sized** (report composites added, redundant single-column indexes dropped — faster reads,
-> no extra write cost). Covered by `DbConstraintsTests`.
+> **right-sized** (report composites added, redundant single-column indexes dropped — faster reads,
+> no extra write cost). Partially covered by `DbConstraintsTests` (RESTRICT + representative CHECKs).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PROJECT_STATUS.md` around lines 131 - 136, The summary overstates test
coverage by saying "Covered by `DbConstraintsTests`" even though
`DbConstraintsTests` only verifies a subset (e.g., RESTRICT delete behavior,
payment amount CHECK, review rating CHECK); update the wording in the
`AddIntegrityConstraintsAndIndexTuning` section to indicate partial rather than
full coverage (e.g., "Partial coverage by `DbConstraintsTests` — tests validate
RESTRICT delete, payment amount CHECK, and review rating CHECK"), and mention
remaining untested items (currency regex, status enums, refund→booking FK, index
changes) so readers know what `DbConstraintsTests` actually covers.
Comment on lines
+46
to
+155
| migrationBuilder.AddCheckConstraint( | ||
| name: "CK_trip_currency_format", | ||
| table: "trip", | ||
| sql: "\"Currency\" ~ '^[A-Z]{3}$'"); | ||
|
|
||
| migrationBuilder.AddCheckConstraint( | ||
| name: "CK_trip_price_nonneg", | ||
| table: "trip", | ||
| sql: "\"Price\" >= 0"); | ||
|
|
||
| migrationBuilder.AddCheckConstraint( | ||
| name: "CK_trip_status_valid", | ||
| table: "trip", | ||
| sql: "\"Status\" IN ('Scheduled', 'InProgress', 'Completed', 'Cancelled')"); | ||
|
|
||
| migrationBuilder.AddCheckConstraint( | ||
| name: "CK_review_rating_range", | ||
| table: "review", | ||
| sql: "\"Rating\" BETWEEN 1 AND 5"); | ||
|
|
||
| migrationBuilder.CreateIndex( | ||
| name: "IX_refund_BookingId", | ||
| table: "refund", | ||
| column: "BookingId"); | ||
|
|
||
| migrationBuilder.AddCheckConstraint( | ||
| name: "CK_refund_amount_nonneg", | ||
| table: "refund", | ||
| sql: "\"Amount\" >= 0"); | ||
|
|
||
| migrationBuilder.AddCheckConstraint( | ||
| name: "CK_refund_currency_format", | ||
| table: "refund", | ||
| sql: "\"Currency\" ~ '^[A-Z]{3}$'"); | ||
|
|
||
| migrationBuilder.AddCheckConstraint( | ||
| name: "CK_refund_status_valid", | ||
| table: "refund", | ||
| sql: "\"Status\" IN ('Pending', 'Completed', 'Failed')"); | ||
|
|
||
| migrationBuilder.AddCheckConstraint( | ||
| name: "CK_promo_code_discount_nonneg", | ||
| table: "promo_code", | ||
| sql: "\"DiscountValue\" >= 0"); | ||
|
|
||
| migrationBuilder.AddCheckConstraint( | ||
| name: "CK_payment_amount_nonneg", | ||
| table: "payment", | ||
| sql: "\"Amount\" >= 0"); | ||
|
|
||
| migrationBuilder.AddCheckConstraint( | ||
| name: "CK_payment_currency_format", | ||
| table: "payment", | ||
| sql: "\"Currency\" ~ '^[A-Z]{3}$'"); | ||
|
|
||
| migrationBuilder.AddCheckConstraint( | ||
| name: "CK_payment_status_valid", | ||
| table: "payment", | ||
| sql: "\"Status\" IN ('Pending', 'Completed', 'Failed', 'Refunded')"); | ||
|
|
||
| migrationBuilder.AddCheckConstraint( | ||
| name: "CK_company_status_valid", | ||
| table: "company", | ||
| sql: "\"Status\" IN ('Pending', 'Active', 'Suspended')"); | ||
|
|
||
| migrationBuilder.CreateIndex( | ||
| name: "IX_booking_CreatedBy_CreatedAtUtc", | ||
| table: "booking", | ||
| columns: new[] { "CreatedBy", "CreatedAtUtc" }); | ||
|
|
||
| migrationBuilder.CreateIndex( | ||
| name: "IX_booking_TripId_Status", | ||
| table: "booking", | ||
| columns: new[] { "TripId", "Status" }); | ||
|
|
||
| migrationBuilder.AddCheckConstraint( | ||
| name: "CK_booking_amounts_nonneg", | ||
| table: "booking", | ||
| sql: "\"TotalAmount\" >= 0 AND \"DiscountAmount\" >= 0"); | ||
|
|
||
| migrationBuilder.AddCheckConstraint( | ||
| name: "CK_booking_currency_format", | ||
| table: "booking", | ||
| sql: "\"Currency\" ~ '^[A-Z]{3}$'"); | ||
|
|
||
| migrationBuilder.AddCheckConstraint( | ||
| name: "CK_booking_status_valid", | ||
| table: "booking", | ||
| sql: "\"Status\" IN ('PendingPayment', 'Confirmed', 'Cancelled', 'Expired')"); | ||
|
|
||
| migrationBuilder.CreateIndex( | ||
| name: "IX_AspNetUsers_CompanyId_StaffType", | ||
| table: "AspNetUsers", | ||
| columns: new[] { "CompanyId", "StaffType" }); | ||
|
|
||
| migrationBuilder.AddForeignKey( | ||
| name: "FK_payment_booking_BookingId", | ||
| table: "payment", | ||
| column: "BookingId", | ||
| principalTable: "booking", | ||
| principalColumn: "Id", | ||
| onDelete: ReferentialAction.Restrict); | ||
|
|
||
| migrationBuilder.AddForeignKey( | ||
| name: "FK_refund_booking_BookingId", | ||
| table: "refund", | ||
| column: "BookingId", | ||
| principalTable: "booking", | ||
| principalColumn: "Id", | ||
| onDelete: ReferentialAction.Restrict); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the migration file and print relevant parts with line numbers
FILE="src/TransportPlatform.Infrastructure/Persistence/Migrations/20260608041228_AddIntegrityConstraintsAndIndexTuning.cs"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "=== File: $FILE ==="
wc -l "$FILE"
sed -n '1,220p' "$FILE" | cat -n
echo
echo "=== Search for NOT VALID / VALIDATE in this migration ==="
rg -n "NOT VALID|VALIDATE CONSTRAINT|ValidateConstraint|NOT VALID|not valid|NotValid" "$FILE" || true
echo
echo "=== Search for any project-wide pattern for phased constraint rollout ==="
# Look for similar phrases in all migrations (bounded output by filtering)
rg -n "NOT VALID|VALIDATE CONSTRAINT|ValidateConstraint|notValid" src/TransportPlatform.Infrastructure/Persistence/Migrations || trueRepository: kashkoool/Transport
Length of output: 9830
🏁 Script executed:
[capability:shell]
#!/bin/bash
set -euo pipefail
FILE="src/TransportPlatform.Infrastructure/Persistence/Migrations/20260608041228_AddIntegrityConstraintsAndIndexTuning.cs"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "=== File: $FILE ==="
wc -l "$FILE"
sed -n '1,220p' "$FILE" | cat -n
echo
echo "=== Search for NOT VALID / VALIDATE in this migration ==="
rg -n "NOT VALID|VALIDATE CONSTRAINT|ValidateConstraint|NOT VALID|not valid|NotValid" "$FILE" || true
echo
echo "=== Search for any project-wide pattern for phased constraint rollout ==="
rg -n "NOT VALID|VALIDATE CONSTRAINT|ValidateConstraint|notValid" src/TransportPlatform.Infrastructure/Persistence/Migrations || trueRepository: kashkoool/Transport
Length of output: 9905
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/TransportPlatform.Infrastructure/Persistence/Migrations/20260608041228_AddIntegrityConstraintsAndIndexTuning.cs"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "=== File: $FILE ==="
wc -l "$FILE"
sed -n '1,220p' "$FILE" | cat -n
echo
echo "=== Search for NOT VALID / VALIDATE in this migration ==="
rg -n "NOT VALID|VALIDATE CONSTRAINT|ValidateConstraint|NOT VALID|not valid|NotValid" "$FILE" || true
echo
echo "=== Search for any project-wide pattern for phased constraint rollout ==="
rg -n "NOT VALID|VALIDATE CONSTRAINT|ValidateConstraint|notValid" src/TransportPlatform.Infrastructure/Persistence/Migrations || trueRepository: kashkoool/Transport
Length of output: 9830
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/TransportPlatform.Infrastructure/Persistence/Migrations/20260608041228_AddIntegrityConstraintsAndIndexTuning.cs"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "=== File: $FILE ==="
wc -l "$FILE"
sed -n '1,220p' "$FILE" | cat -n
echo
echo "=== Search for NOT VALID / VALIDATE in this migration ==="
rg -n "NOT VALID|VALIDATE CONSTRAINT|ValidateConstraint|NOT VALID|not valid|NotValid" "$FILE" || true
echo
echo "=== Search for any project-wide pattern for phased constraint rollout ==="
rg -n "NOT VALID|VALIDATE CONSTRAINT|ValidateConstraint|notValid" src/TransportPlatform.Infrastructure/Persistence/Migrations || trueRepository: kashkoool/Transport
Length of output: 9830
Major rollout risk: validating CHECK constraints + FKs in a single migration
Up(...) adds multiple validating AddCheckConstraints (trip/review/refund/promo_code/payment/company/booking) and also adds/validates foreign keys with Restrict (payment.BookingId -> booking.Id, refund.BookingId -> booking.Id, refund.PaymentId -> payment.Id) in one step, with no phased NOT VALID + later VALIDATE (or preflight/backfill) path—any existing non-conforming row/orphaned reference will fail the migration at apply time.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/TransportPlatform.Infrastructure/Persistence/Migrations/20260608041228_AddIntegrityConstraintsAndIndexTuning.cs`
around lines 46 - 155, This migration adds multiple validating
AddCheckConstraint and AddForeignKey calls (see AddCheckConstraint for "CK_*"
constraints and AddForeignKey calls "FK_payment_booking_BookingId",
"FK_refund_booking_BookingId") which will fail if existing data violates them;
change the AddCheckConstraint/AddForeignKey work to create constraints NOT VALID
(use raw SQL via migrationBuilder.Sql to run ALTER TABLE ... ADD CONSTRAINT
<name> ... NOT VALID for each "CK_*" and ALTER TABLE ... ADD CONSTRAINT <fkname>
FOREIGN KEY (...) REFERENCES ... NOT VALID for the FK pairs), ensure any needed
indexes (e.g., IX_refund_BookingId, IX_booking_TripId_Status) are created first,
then add a separate follow-up migration that runs ALTER TABLE ... VALIDATE
CONSTRAINT <name> (and validates FKs) after data backfill/cleanup so the rollout
is phased and won’t fail on existing bad rows.
Comment on lines
+37
to
+38
| await Assert.ThrowsAnyAsync<DbUpdateException>(() => db.SaveChangesAsync()); | ||
| } |
There was a problem hiding this comment.
Tighten exception assertions to prove the exact constraint under test.
Assert.ThrowsAnyAsync<DbUpdateException> is too broad here; these tests can pass on unrelated DB errors and still look green. Assert the expected constraint/FK name per case.
Suggested change
- await Assert.ThrowsAnyAsync<DbUpdateException>(() => db.SaveChangesAsync());
+ var ex = await Assert.ThrowsAsync<DbUpdateException>(() => db.SaveChangesAsync());
+ Assert.Contains("FK_payment_booking_BookingId", ex.InnerException?.Message ?? ex.Message);
- await Assert.ThrowsAnyAsync<DbUpdateException>(() => db.SaveChangesAsync());
+ var ex = await Assert.ThrowsAsync<DbUpdateException>(() => db.SaveChangesAsync());
+ Assert.Contains("CK_payment_amount_nonneg", ex.InnerException?.Message ?? ex.Message);
- await Assert.ThrowsAnyAsync<DbUpdateException>(() => db.SaveChangesAsync());
+ var ex = await Assert.ThrowsAsync<DbUpdateException>(() => db.SaveChangesAsync());
+ Assert.Contains("CK_review_rating_range", ex.InnerException?.Message ?? ex.Message);Also applies to: 53-54, 68-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/TransportPlatform.IntegrationTests/DbConstraintsTests.cs` around lines
37 - 38, Replace the broad Assert.ThrowsAnyAsync<DbUpdateException>(() =>
db.SaveChangesAsync()) checks with an Assert.ThrowsAsync<DbUpdateException> that
captures the exception (e.g., var ex = await
Assert.ThrowsAsync<DbUpdateException>(...)) and then assert the specific
constraint/foreign-key identifier in the underlying DB error message/inner
exception (e.g., Assert.Contains("EXPECTED_CONSTRAINT_NAME",
ex.InnerException?.Message ?? ex.Message)); apply the same change for the other
occurrences referenced (lines shown as 53-54 and 68-69) so each test explicitly
verifies the expected constraint/FK name rather than any DB update error.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Defence-in-depth schema hardening from the full DB audit. Additive migration, no data loss.
Integrity (DB-level CHECK constraints)
Back the domain rules at the database so impossible values are rejected even if app code is bypassed:
>= 0ontrip.Price,booking.Total/Discount,payment.Amount,refund.Amount,promo_code.DiscountValuereview.RatingBETWEEN 1 AND 5~ '^[A-Z]{3}$'on the money tablesProtect financial history
payment → bookingandrefund → paymentFKs changed from Cascade → Restrict (deleting a booking can no longer silently erase its payment/refund records)refund.BookingIdwas an unconstrained column → now a real FK (Restrict) +IX_refund_BookingIdIndex right-sizing (faster reads, no extra write cost)
booking(TripId, Status),booking(CreatedBy, CreatedAtUtc),trip(CompanyId, DepartureUtc),AspNetUsers(CompanyId, StaffType)bookingTripId/Status/CreatedBy and the standalonetrip/AspNetUsersCompanyId — trims the write-heavybookingtable from 7 → 6 indexesTests
New
DbConstraintsTestsassert the CHECKs reject bad values and the Restrict FK blocks deleting a booking that has a payment. Full suite green: 65 unit + 65 integration.Notes
Summary by CodeRabbit
Chores
Tests