Repository navigation
fix: enforce database integrity and batch atomic operations - #8
Merged
Merged
Conversation
- Added 0001_integrity_indexes.sql to push core invariants down to the
database level (e.g., one billing period per property per month, one
bill per tenancy per period) preventing duplicate record races.
- Replaced read-then-write rate limiters with atomic INSERT...ON CONFLICT
counters for photo uploads and meter reading submissions.
- Wrapped leaf-to-root cascading deletes in property-cleanup.ts and
properties.ts inside db.batch() to guarantee atomicity and prevent
orphaned records.
- Surfaced JSON parsing errors in recalculateChain by passing { cause }
instead of silently swallowing them.
- Updated pnpm seed:fresh to handle upload_daily_count and
reading_daily_count.
- Added comprehensive unit tests for rate limits and D1 recalculations.
Contributor
📝 WalkthroughWalkthroughThe PR adds database integrity indexes and daily counter tables. Upload and meter-reading limits now use atomic counters with rollback paths. Recalculation removes stale bills and surfaces JSON errors. Property cleanup uses batched, foreign-key-safe deletions. ChangesIntegrity and processing updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant UploadsRoute
participant UploadDailyCount
participant R2
participant Database
Client->>UploadsRoute: submit upload
UploadsRoute->>UploadDailyCount: atomically reserve daily count
UploadDailyCount->>Database: upsert and read user/date counter
Database-->>UploadsRoute: counter value
UploadsRoute->>R2: store photo
UploadsRoute->>Database: save upload metadata
UploadsRoute->>UploadDailyCount: decrement on rejection or failure
UploadsRoute-->>Client: upload result or 429 response
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/routes/uploads.ts (1)
125-244: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRollback logic does not cover exceptions thrown by
formData(),validateUploadedFile(), orr2.put().Only the explicit validation branches (missing fields, invalid purpose, unauthorized, invalid file) call the counter rollback before returning. Between the reservation at Line 93 and the first try block at Line 247, three calls run outside any try/catch:
c.req.formData()at Line 126 can throw on a malformed multipart body.validateUploadedFile()at Line 200 can throw for reasons other than returning{ valid: false }.r2.put()at Line 235 can throw on an R2 failure.If any of these throws, the handler propagates an unhandled exception, and the daily counter increment from Line 93-98 is never rolled back. The user's upload quota is then permanently reduced by one for the rest of the UTC day, even though no upload succeeded.
Wrap this span in a try/catch that rolls back the counter (and deletes the R2 object if it was already created) on any exception, mirroring the existing catch block at Lines 289-296.
🐛 Proposed fix to close the rollback gap
+ let objectKey: string | undefined; + try { // --- Parse multipart --- const formData = await c.req.formData(); const photo = formData.get("photo") as File | null; ... const validation = await validateUploadedFile( photo, purpose === "bill_document" ? "bill-document" : "meter-photo" ); if (!validation.valid) { await db.run( sql`UPDATE upload_daily_count SET count = count - 1 WHERE id = ${counterId}` ); return c.json({ ... }, 400); } // --- Store in R2 --- const timestamp = Date.now(); ... - const objectKey = `${propertyId}/${periodId}/${user.id}/${timestamp}.${ext}`; + objectKey = `${propertyId}/${periodId}/${user.id}/${timestamp}.${ext}`; await r2.put(objectKey, await photo.arrayBuffer(), { ... }); // --- Cap enforcement in DB (Max 3 per period) --- - try { const MAX_PHOTOS = 3; ... if (existing.length >= MAX_PHOTOS) { await r2.delete(objectKey).catch(() => {}); await db.run( sql`UPDATE upload_daily_count SET count = count - 1 WHERE id = ${counterId}` ); return c.json({ ... }, 400); } ... await db.insert(billPhotos).values({ ... }); - } catch (err) { - await r2.delete(objectKey).catch(() => {}); - await db.run( - sql`UPDATE upload_daily_count SET count = count - 1 WHERE id = ${counterId}` - ); - throw err; - } + } catch (err) { + if (objectKey) await r2.delete(objectKey).catch(() => {}); + await db + .run(sql`UPDATE upload_daily_count SET count = count - 1 WHERE id = ${counterId}`) + .catch(() => {}); + throw err; + }🤖 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/api/routes/uploads.ts` around lines 125 - 244, Wrap the upload flow from c.req.formData() through r2.put(), including validateUploadedFile(), in the same try/catch rollback path used by the existing handler catch. Track the generated objectKey and, on exceptions, decrement the daily counter and delete the R2 object if it was created, while preserving the existing explicit validation responses.
🧹 Nitpick comments (2)
src/api/lib/recalculation.test.ts (1)
16-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the missing-reading fallback branch.
The tests cover the cascade-update path and the malformed-JSON path. They do not cover the branch in
recalculateChainwhereprevReadingorcurrentReadingis absent (the plaindb.delete(bills)fallback, without the batched start-value update). Add a test that removes or omits a meter reading for one period in the chain and asserts that bills for that period are deleted without a start-value update.🤖 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/api/lib/recalculation.test.ts` around lines 16 - 180, The recalculation tests lack coverage for the missing-reading fallback in recalculateChain. Add a test in the recalculateChain suite that removes or omits a meter reading for a chain period, invokes recalculateChain, and verifies that the period’s bills are deleted while no batched start-value update is performed; keep the existing cascade and malformed-JSON tests unchanged.src/api/routes/uploads.ts (1)
1-1: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse Drizzle's query builder with
.onConflictDoUpdate().returning()instead of raw SQL INSERT + SELECT.Both files reserve a daily counter with a raw
INSERT ... ON CONFLICT ... DO UPDATE, then issue a separateSELECTto read the resultingcount. SQLite's UPSERT syntax supports aRETURNINGclause, and Drizzle exposes this through.onConflictDoUpdate().returning(). Combining the write and the read into one statement removes an extra database round trip on every upload and every reading submission.
src/api/routes/uploads.ts#L90-111: replace theINSERT+ separateSELECTagainstupload_daily_countwith a single.insert(uploadDailyCount).values(...).onConflictDoUpdate(...).returning(...)call.src/api/routes/meter-readings.ts#L492-512: apply the same change to thereading_daily_countupsert.♻️ Proposed refactor (uploads.ts shown; apply the same shape to meter-readings.ts)
- await db.run(sql` - INSERT INTO upload_daily_count (id, user_id, date_key, count) - VALUES (${counterId}, ${user.id}, ${todayKey}, 1) - ON CONFLICT (user_id, date_key) - DO UPDATE SET count = count + 1 - `); - - const [counter] = await db - .select({ count: uploadDailyCount.count }) - .from(uploadDailyCount) - .where(eq(uploadDailyCount.id, counterId)); - - const newCount = counter?.count ?? 1; + const [counter] = await db + .insert(uploadDailyCount) + .values({ id: counterId, userId: user.id, dateKey: todayKey, count: 1 }) + .onConflictDoUpdate({ + target: [uploadDailyCount.userId, uploadDailyCount.dateKey], + set: { count: sql`${uploadDailyCount.count} + 1` }, + }) + .returning({ count: uploadDailyCount.count }); + + const newCount = counter?.count ?? 1;🤖 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/api/routes/uploads.ts` at line 1, Replace the raw SQL upsert and follow-up select in the upload counter flow with Drizzle’s `uploadDailyCount` insert builder using `.onConflictDoUpdate().returning()` to obtain the updated count in one statement. Apply the same pattern to the `readingDailyCount` flow in the meter-reading submission logic, preserving the existing conflict key, increment behavior, and returned count usage.
🤖 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 `@src/api/lib/recalculation.ts`:
- Around line 42-90: The recalculation loop deletes existing bills before JSON
validation can fail, leaving periods empty and stopping the cascade. Validate
each period’s oneOffCharges and rateOverride before any deletion, preferably for
all periods in periodsToProcess before entering the loop; reuse the same
parsing/validation behavior expected by recalculateBillsForPeriod, then retain
the existing deletion and recalculation flow.
In `@src/api/routes/uploads.test.ts`:
- Around line 234-271: Update the concurrent reservation test around the two
app.request calls so both upload requests are dispatched before awaiting either
result, using Promise.all or equivalent. Keep the existing request payloads and
assert that exactly one response is successful while the other returns 429,
without relying on fixed request ordering.
In `@src/db/migrations/0001_integrity_indexes.sql`:
- Around line 49-65: Add explicit unique index creation statements after the
daily counter table definitions for upload_daily_count and reading_daily_count,
using the exact names upload_daily_count_user_date_unique and
reading_daily_count_user_date_unique and indexing (user_id, date_key). Preserve
the existing inline UNIQUE constraints and use IF NOT EXISTS as specified.
- Around line 1-27: Add a pre-index migration step that detects and resolves or
explicitly validates duplicate rows for billing_periods (property_id,
period_month), bills (tenancy_id, billing_period_id), and active tenancies
(tenant_id, property_id). Ensure duplicates are handled before the three CREATE
UNIQUE INDEX statements execute, while preserving the partial-index rule
excluding non-active or NULL-tenant rows.
---
Outside diff comments:
In `@src/api/routes/uploads.ts`:
- Around line 125-244: Wrap the upload flow from c.req.formData() through
r2.put(), including validateUploadedFile(), in the same try/catch rollback path
used by the existing handler catch. Track the generated objectKey and, on
exceptions, decrement the daily counter and delete the R2 object if it was
created, while preserving the existing explicit validation responses.
---
Nitpick comments:
In `@src/api/lib/recalculation.test.ts`:
- Around line 16-180: The recalculation tests lack coverage for the
missing-reading fallback in recalculateChain. Add a test in the recalculateChain
suite that removes or omits a meter reading for a chain period, invokes
recalculateChain, and verifies that the period’s bills are deleted while no
batched start-value update is performed; keep the existing cascade and
malformed-JSON tests unchanged.
In `@src/api/routes/uploads.ts`:
- Line 1: Replace the raw SQL upsert and follow-up select in the upload counter
flow with Drizzle’s `uploadDailyCount` insert builder using
`.onConflictDoUpdate().returning()` to obtain the updated count in one
statement. Apply the same pattern to the `readingDailyCount` flow in the
meter-reading submission logic, preserving the existing conflict key, increment
behavior, and returned count usage.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e96ff10-757e-4bf0-963d-6cc5c8b28691
📒 Files selected for processing (21)
src/api/lib/property-cleanup.tssrc/api/lib/recalculation.test.tssrc/api/lib/recalculation.tssrc/api/routes/meter-readings-rate-limit.test.tssrc/api/routes/meter-readings.test.tssrc/api/routes/meter-readings.tssrc/api/routes/properties.tssrc/api/routes/uploads.test.tssrc/api/routes/uploads.tssrc/db/migrations/0001_integrity_indexes.sqlsrc/db/migrations/meta/_journal.jsonsrc/db/schema/billing-periods.tssrc/db/schema/bills.tssrc/db/schema/custom-charges.tssrc/db/schema/daily-counters.tssrc/db/schema/edit-requests.tssrc/db/schema/index.tssrc/db/schema/meter-reading-edits.tssrc/db/schema/properties.tssrc/db/schema/property-rates.tssrc/db/schema/tenancies.ts
Comment on lines
42
to
90
| for (let i = 0; i < periodsToProcess.length; i++) { | ||
| const period = periodsToProcess[i]; | ||
|
|
||
| // Never recalculate a confirmed period in a cascade (i > 0) | ||
| // Note: The target period (i === 0) ALWAYS recalculates, even if confirmed, | ||
| // Note: The target period (i === 0) ALWAYS recalculates, even if confirmed, | ||
| // because this function is called explicitly by the owner to override a confirmed period. | ||
| if (i > 0 && period.status === 'confirmed') { | ||
| console.warn('[Recalc] Stopping cascade at confirmed period', period.id); | ||
| break; | ||
| if (i > 0 && period.status === "confirmed") { | ||
| console.warn("[Recalc] Stopping cascade at confirmed period", period.id); | ||
| break; | ||
| } | ||
|
|
||
| // Update start values from previous period's end values (except for the first in the chain) | ||
| if (i > 0) { | ||
| const prevPeriod = periodsToProcess[i - 1]; | ||
| const [prevReading] = await db.select().from(meterReadings).where(eq(meterReadings.billingPeriodId, prevPeriod.id)).limit(1); | ||
| const [currentReading] = await db.select().from(meterReadings).where(eq(meterReadings.billingPeriodId, period.id)).limit(1); | ||
|
|
||
| const [prevReading] = await db | ||
| .select() | ||
| .from(meterReadings) | ||
| .where(eq(meterReadings.billingPeriodId, prevPeriod.id)) | ||
| .limit(1); | ||
| const [currentReading] = await db | ||
| .select() | ||
| .from(meterReadings) | ||
| .where(eq(meterReadings.billingPeriodId, period.id)) | ||
| .limit(1); | ||
|
|
||
| if (prevReading && currentReading) { | ||
| await db.update(meterReadings) | ||
| .set({ | ||
| solarGenerationStart: prevReading.solarGenerationEnd, | ||
| exportStart: prevReading.exportEnd, | ||
| importStart: prevReading.importEnd, | ||
| }) | ||
| .where(eq(meterReadings.id, currentReading.id)); | ||
| // Batch: update start values + delete stale bills atomically. | ||
| await db.batch([ | ||
| db | ||
| .update(meterReadings) | ||
| .set({ | ||
| solarGenerationStart: prevReading.solarGenerationEnd, | ||
| exportStart: prevReading.exportEnd, | ||
| importStart: prevReading.importEnd, | ||
| }) | ||
| .where(eq(meterReadings.id, currentReading.id)), | ||
| db.delete(bills).where(eq(bills.billingPeriodId, period.id)), | ||
| ]); | ||
| } else { | ||
| await db.delete(bills).where(eq(bills.billingPeriodId, period.id)); | ||
| } | ||
| } else { | ||
| await db.delete(bills).where(eq(bills.billingPeriodId, period.id)); | ||
| } | ||
|
|
||
| // Recalculate bills for this period | ||
| // Recalculate bills for this period (delete is already handled above) | ||
| await recalculateBillsForPeriod(db, period.id); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Bills are deleted before recalculateBillsForPeriod validates its inputs.
For every period in the loop, bills are deleted first (lines 78, 81, or 84), then recalculateBillsForPeriod runs and can now throw on malformed oneOffCharges or rateOverride JSON (lines 158-192). If that throw happens, the period ends up with zero bills instead of the stale bills it had before recalculation started, and the exception propagates out of recalculateChain, so the loop stops and any remaining periods in periodsToProcess are never processed either.
Validate period.oneOffCharges and period.rateOverride JSON for the period before deleting its bills, or validate all periods in periodsToProcess up front before the loop starts deleting anything. This prevents a single malformed JSON value from leaving a billing period with no bills at all.
🤖 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/api/lib/recalculation.ts` around lines 42 - 90, The recalculation loop
deletes existing bills before JSON validation can fail, leaving periods empty
and stopping the cascade. Validate each period’s oneOffCharges and rateOverride
before any deletion, preferably for all periods in periodsToProcess before
entering the loop; reuse the same parsing/validation behavior expected by
recalculateBillsForPeriod, then retain the existing deletion and recalculation
flow.
Comment on lines
+234
to
+271
| it("concurrent reservation does not bypass the limit", async () => { | ||
| const todayKey = new Date().toISOString().slice(0, 10); | ||
| await testDb.insert(uploadDailyCount).values({ | ||
| id: `owner-id:${todayKey}`, | ||
| userId: "owner-id", | ||
| dateKey: todayKey, | ||
| count: 1, // Mock MAX_UPLOADS_PER_DAY is 2, so 1 more is allowed | ||
| }); | ||
|
|
||
| currentUser = { id: "owner-id" }; | ||
|
|
||
| const req1 = new FormData(); | ||
| req1.append("photo", createMockFile("test1.jpg", "image/jpeg")); | ||
| req1.append("propertyId", propId); | ||
| req1.append("periodId", bpId); | ||
| req1.append("purpose", "import_meter"); | ||
|
|
||
| const req2 = new FormData(); | ||
| req2.append("photo", createMockFile("test2.jpg", "image/jpeg")); | ||
| req2.append("propertyId", propId); | ||
| req2.append("periodId", bpId); | ||
| req2.append("purpose", "export_meter"); | ||
|
|
||
| const res1 = await app.request( | ||
| "/uploads/bill-photo", | ||
| { method: "POST", body: req1 }, | ||
| mockEnv as unknown as Parameters<typeof app.request>[2] | ||
| ); | ||
|
|
||
| const res2 = await app.request( | ||
| "/uploads/bill-photo", | ||
| { method: "POST", body: req2 }, | ||
| mockEnv as unknown as Parameters<typeof app.request>[2] | ||
| ); | ||
|
|
||
| expect(res1.status).toBe(200); | ||
| expect(res2.status).toBe(429); | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Test does not exercise concurrent requests.
res1 is awaited to completion before res2 starts. This makes the two requests sequential, not concurrent. The test name states it verifies that concurrent reservation does not bypass the limit, but sequential execution cannot detect a race in the counter-increment logic; it would pass even if the atomic reservation returned to a read-then-write pattern.
Dispatch both requests together so their internal awaits can interleave, and confirm exactly one succeeds and one is rate-limited.
🧪 Proposed fix to dispatch requests concurrently
- const res1 = await app.request(
- "/uploads/bill-photo",
- { method: "POST", body: req1 },
- mockEnv as unknown as Parameters<typeof app.request>[2]
- );
-
- const res2 = await app.request(
- "/uploads/bill-photo",
- { method: "POST", body: req2 },
- mockEnv as unknown as Parameters<typeof app.request>[2]
- );
-
- expect(res1.status).toBe(200);
- expect(res2.status).toBe(429);
+ const [res1, res2] = await Promise.all([
+ app.request(
+ "/uploads/bill-photo",
+ { method: "POST", body: req1 },
+ mockEnv as unknown as Parameters<typeof app.request>[2]
+ ),
+ app.request(
+ "/uploads/bill-photo",
+ { method: "POST", body: req2 },
+ mockEnv as unknown as Parameters<typeof app.request>[2]
+ ),
+ ]);
+
+ const statuses = [res1.status, res2.status].sort();
+ expect(statuses).toEqual([200, 429]);📝 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.
Suggested change
| it("concurrent reservation does not bypass the limit", async () => { | |
| const todayKey = new Date().toISOString().slice(0, 10); | |
| await testDb.insert(uploadDailyCount).values({ | |
| id: `owner-id:${todayKey}`, | |
| userId: "owner-id", | |
| dateKey: todayKey, | |
| count: 1, // Mock MAX_UPLOADS_PER_DAY is 2, so 1 more is allowed | |
| }); | |
| currentUser = { id: "owner-id" }; | |
| const req1 = new FormData(); | |
| req1.append("photo", createMockFile("test1.jpg", "image/jpeg")); | |
| req1.append("propertyId", propId); | |
| req1.append("periodId", bpId); | |
| req1.append("purpose", "import_meter"); | |
| const req2 = new FormData(); | |
| req2.append("photo", createMockFile("test2.jpg", "image/jpeg")); | |
| req2.append("propertyId", propId); | |
| req2.append("periodId", bpId); | |
| req2.append("purpose", "export_meter"); | |
| const res1 = await app.request( | |
| "/uploads/bill-photo", | |
| { method: "POST", body: req1 }, | |
| mockEnv as unknown as Parameters<typeof app.request>[2] | |
| ); | |
| const res2 = await app.request( | |
| "/uploads/bill-photo", | |
| { method: "POST", body: req2 }, | |
| mockEnv as unknown as Parameters<typeof app.request>[2] | |
| ); | |
| expect(res1.status).toBe(200); | |
| expect(res2.status).toBe(429); | |
| }); | |
| it("concurrent reservation does not bypass the limit", async () => { | |
| const todayKey = new Date().toISOString().slice(0, 10); | |
| await testDb.insert(uploadDailyCount).values({ | |
| id: `owner-id:${todayKey}`, | |
| userId: "owner-id", | |
| dateKey: todayKey, | |
| count: 1, // Mock MAX_UPLOADS_PER_DAY is 2, so 1 more is allowed | |
| }); | |
| currentUser = { id: "owner-id" }; | |
| const req1 = new FormData(); | |
| req1.append("photo", createMockFile("test1.jpg", "image/jpeg")); | |
| req1.append("propertyId", propId); | |
| req1.append("periodId", bpId); | |
| req1.append("purpose", "import_meter"); | |
| const req2 = new FormData(); | |
| req2.append("photo", createMockFile("test2.jpg", "image/jpeg")); | |
| req2.append("propertyId", propId); | |
| req2.append("periodId", bpId); | |
| req2.append("purpose", "export_meter"); | |
| const [res1, res2] = await Promise.all([ | |
| app.request( | |
| "/uploads/bill-photo", | |
| { method: "POST", body: req1 }, | |
| mockEnv as unknown as Parameters<typeof app.request>[2] | |
| ), | |
| app.request( | |
| "/uploads/bill-photo", | |
| { method: "POST", body: req2 }, | |
| mockEnv as unknown as Parameters<typeof app.request>[2] | |
| ), | |
| ]); | |
| const statuses = [res1.status, res2.status].sort(); | |
| expect(statuses).toEqual([200, 429]); | |
| }); |
🤖 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/api/routes/uploads.test.ts` around lines 234 - 271, Update the concurrent
reservation test around the two app.request calls so both upload requests are
dispatched before awaiting either result, using Promise.all or equivalent. Keep
the existing request payloads and assert that exactly one response is successful
while the other returns 429, without relying on fixed request ordering.
Comment on lines
+1
to
+27
| -- 1a. UNIQUE constraint: one billing period per property per month. | ||
| -- Replaces the plain index so the DB engine rejects a second row | ||
| -- for the same (propertyId, periodMonth) pair, not just the code. | ||
| DROP INDEX IF EXISTS idx_billing_periods_property_month; | ||
| --> statement-breakpoint | ||
| CREATE UNIQUE INDEX idx_billing_periods_property_month | ||
| ON billing_periods (property_id, period_month); | ||
| --> statement-breakpoint | ||
|
|
||
| -- 1b. UNIQUE constraint: one bill per tenancy per billing period. | ||
| -- Recalculation always deletes before re-inserting, so this is | ||
| -- safe. The index makes the invariant enforcement database-level. | ||
| DROP INDEX IF EXISTS idx_bills_tenancy_period; | ||
| --> statement-breakpoint | ||
| CREATE UNIQUE INDEX idx_bills_tenancy_period | ||
| ON bills (tenancy_id, billing_period_id); | ||
| --> statement-breakpoint | ||
|
|
||
| -- 1c. UNIQUE constraint: prevent duplicate active tenancies. | ||
| -- Only one row with status='active' may exist for a given | ||
| -- (tenant_id, property_id) pair. NULL tenant_id rows (invited | ||
| -- but not yet joined) are excluded via WHERE to allow multiple | ||
| -- pending invites for the same email across properties. | ||
| -- SQLite partial indexes use a WHERE clause for this. | ||
| CREATE UNIQUE INDEX IF NOT EXISTS idx_tenancies_active_unique | ||
| ON tenancies (tenant_id, property_id) | ||
| WHERE status = 'active' AND tenant_id IS NOT NULL; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for pre-existing duplicate rows against the new unique constraints.
# Requires the production/staging D1 database file or a wrangler d1 execute connection; adjust as needed.
set -euo pipefail
echo "--- Checking for prior migrations/snapshots that might already assume uniqueness ---"
fd _journal.json src/db/migrations/meta --exec cat {}
fd '.*snapshot.*\.json' src/db/migrations/meta
echo "--- Searching for any existing dedup/cleanup scripts in the repo ---"
rg -n 'duplicate' -g '*.sql' -g '*.ts' src/dbRepository: VaibhavDaveDev/Meterly
Length of output: 806
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- Candidate migration/meta files ---"
find src/db/migrations -maxdepth 2 -type f | sort
echo
echo "--- 0000 snapshot relevant schema/index summaries ---"
python3 - <<'PY'
import json, pathlib, re
for f in sorted(pathlib.Path('src/db/migrations').rglob('*.json')):
if 'meta' in f.parts or f.name in ('0000_snapshot.json', 'snapshots'):
continue
try:
data = json.loads(f.read_text())
except Exception as e:
print(f"{f}: bad json: {e}")
continue
print(f"--- {f} ---")
# Print concise occurrences of target columns and any uniqueness artifacts.
s = json.dumps(data, separators=(',', ':'))
if any(s.find(part)>=0 for part in ['property_id', 'period_month', 'tenancy_id', 'billing_period_id', 'tenant_id', 'billing_periods', 'bills', 'tenancies']):
print("target mentions: yes")
for key in ['tables', 'indexes', 'sequences', 'enums']:
if key in data:
print(f"top-level {key}: {len(data[key]) if isinstance(data[key], dict) else len(data[key])}")
PY
echo "--- 0000_snapshot.json tables ---"
python3 - <<'PY'
import json, pathlib
p=pathlib.Path('src/db/migrations/meta/0000_snapshot.json')
data=json.loads(p.read_text())
targets=['billing_periods','bills','tenancies']
for path in data.get('tables', {}).keys():
if any(t in path for t in targets):
print(f"\n--- tables.{path} ---")
tbl=data['tables'][path]
print("columns:", list(tbl.get('columns',{}).keys()))
print("uniqueConstraints:", tbl.get('uniqueConstraints'))
print("primaryKey:", tbl.get('primaryKey'))
PY
echo
echo "--- 0001_integrity_indexes.sql full ---"
cat -n src/db/migrations/0001_integrity_indexes.sql
echo
echo "--- Search for unique indexes/constraints in migration files ---"
rg -n 'idx_billing_periods_property_month|idx_bills_tenancy_period|idx_tenancies_active_unique|uniqueIndex|uniqueConstraints|UNIQUE|CREATE TABLE' src/db/migrations src/db -g '*.sql' -g '*.ts' -g '*.json'Repository: VaibhavDaveDev/Meterly
Length of output: 16365
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- Schema declarations for affected unique indexes/usages ---"
for f in src/db/schema/billing-periods.ts src/db/schema/bills.ts src/db/schema/tenancies.ts src/db/schema/daily-counters.ts; do
echo "--- $f ---"
cat -n "$f"
done
echo
echo "--- Programmatic uniqueness summary from migration files ---"
python3 - <<'PY'
import pathlib, re, json
migs = {}
for p in sorted(pathlib.Path('src/db/migrations').rglob('*.sql')):
s = p.read_text()
migs[str(p)] = {
"CREATE UNIQUE INDEX": len(re.findall(r'(?i)(CREATE\s+UNIQUE\s+INDEX|CREATE\s+UNIQUE\s+INDEX\s+IF\s+NOT\s+EXISTS)\b', s)),
"plain CREATE INDEX": len(re.findall(r'(?i)CREATE\s+INDEX\s+(?:IF\s+NOT\s+EXISTS\s+)?[A-Z0-9_]+\s+ON\b', s)),
"table UNIQUE constraints": len(re.findall(r'(?i)\bUNIQUE\s*\(\s*[A-Z0-9_]+,\s*[A-Z0-9_]+\s*\)', s)),
}
print(json.dumps(migs, indent=2, sort_keys=True))
targets = {
"idx_billing_periods_property_month": "(property_id, period_month)",
"idx_bills_tenancy_period": "(tenancy_id, billing_period_id)",
"idx_tenancies_active_unique": "(tenant_id, property_id)",
"upload_daily_count_user_date_unique": "upload_daily_count (user_id, date_key)",
"reading_daily_count_user_date_unique": "reading_daily_count (user_id, date_key)",
}
for target, wanted_col in targets.items():
matched = sorted(p for p, s in migs.items() if target in p.read_text())
print(f"{target}: file count={len(matched)} files={matched}, columns_wanted={wanted_col}")
for p in matched:
s=p.read_text()
print(" contains index as UNIQUE?", bool(re.search(r'(?i)CREATE\s+UNIQUE\s+INDEX(?:\s+IF\s+NOT\s+EXISTS)?\s+' + re.escape(target), s)))
schema_snapshot = json.loads(pathlib.Path('src/db/migrations/meta/0000_snapshot.json').read_text())
print("--- 0000 snapshot index types for target names ---")
for name in targets:
i = schema_snapshot.get("indexes", {}).get(name)
print(f"{name}: unique={i.get('unique') if i else None}, columns={i.get('columns') if i else None}")
PYRepository: VaibhavDaveDev/Meterly
Length of output: 8915
Handle existing duplicates before creating the unique indexes.
CREATE UNIQUE INDEX fails if billing_periods, bills, or tenancies already violate the new uniqueness rules, which blocks deployment. Add a pre-migration cleanup/validation path for duplicate (property_id, period_month), duplicate (tenancy_id, billing_period_id), and multiple active rows for the same (tenant_id, property_id) before creating these indexes.
🤖 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/db/migrations/0001_integrity_indexes.sql` around lines 1 - 27, Add a
pre-index migration step that detects and resolves or explicitly validates
duplicate rows for billing_periods (property_id, period_month), bills
(tenancy_id, billing_period_id), and active tenancies (tenant_id, property_id).
Ensure duplicates are handled before the three CREATE UNIQUE INDEX statements
execute, while preserving the partial-index rule excluding non-active or
NULL-tenant rows.
Comment on lines
+49
to
+65
| CREATE TABLE IF NOT EXISTS upload_daily_count ( | ||
| id TEXT PRIMARY KEY, | ||
| user_id TEXT NOT NULL REFERENCES user(id) ON DELETE CASCADE, | ||
| date_key TEXT NOT NULL, -- 'YYYY-MM-DD' | ||
| count INTEGER NOT NULL DEFAULT 0, | ||
| UNIQUE (user_id, date_key) | ||
| ); | ||
| --> statement-breakpoint | ||
|
|
||
| -- reading_daily_count: same shape for meter-reading submissions. | ||
| CREATE TABLE IF NOT EXISTS reading_daily_count ( | ||
| id TEXT PRIMARY KEY, | ||
| user_id TEXT NOT NULL REFERENCES user(id) ON DELETE CASCADE, | ||
| date_key TEXT NOT NULL, | ||
| count INTEGER NOT NULL DEFAULT 0, | ||
| UNIQUE (user_id, date_key) | ||
| ); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm the exact index names declared in daily-counters.ts and compare with the migration.
set -euo pipefail
fd daily-counters.ts src/db/schema --exec cat -n {}
echo "--- migration snapshot (if present) ---"
fd '.*snapshot.*\.json' src/db/migrations/meta --exec cat {}Repository: VaibhavDaveDev/Meterly
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- migration relevant lines ---"
fd '0001_integrity_indexes\.sql' src/db/migrations --exec cat -n {} | sed -n '35,75p'
echo "--- schema dailycounter declarations ---"
fd daily-counters.ts src/db/schema --exec cat -n {} | sed -n '1,50p'
echo "--- SQLite index metadata comparison (read-only behavioral probe) ---"
command -v sqlite3 >/dev/null 2>&1 || { echo "sqlite3 unavailable"; exit 0; }
tmp=/tmp/drizzle_index_probe_$$.db
rm -f "$tmp"
sqlite3 "$tmp" <<'SQL'
CREATE TABLE upload_daily_count (
id TEXT PRIMARY KEY,
user_id TEXT NOT NULL,
date_key TEXT NOT NULL,
count INTEGER NOT NULL DEFAULT 0,
UNIQUE (user_id, date_key)
);
CREATE TABLE reading_daily_count (
id TEXT PRIMARY KEY,
user_id TEXT NOT NULL,
date_key TEXT NOT NULL,
count INTEGER NOT NULL DEFAULT 0,
UNIQUE (user_id, date_key)
);
SQL
echo "-- created indexes for daily-counts tables --"
sqlite3 "$tmp" "SELECT name, tbl_name, sql FROM sqlite_master WHERE type='index' AND tbl_name IN ('upload_daily_count','reading_daily_count') ORDER BY tbl_name, name;"
echo "-- pragma integrity and indexes for each table --"
sqlite3 "$tmp" "PRAGMA index_list(upload_daily_count);"
sqlite3 "$tmp" "PRAGMA index_list(reading_daily_count);"
rm -f "$tmp"Repository: VaibhavDaveDev/Meterly
Length of output: 3227
Create the named indexes for the daily counter tables.
src/db/schema/daily-counters.ts declares uniqueIndex("upload_daily_count_user_date_unique") and uniqueIndex("reading_daily_count_user_date_unique"), but src/db/migrations/0001_integrity_indexes.sql only defines inline UNIQUE (user_id, date_key) constraints. The database table will only have UNIQUE constraints, so Drizzle schema-as-code and the migration drift. Add explicit CREATE UNIQUE INDEX IF NOT EXISTS ... ON upload_daily_count (user_id, date_key) and the matching index on reading_daily_count to match the expected index metadata.
🤖 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/db/migrations/0001_integrity_indexes.sql` around lines 49 - 65, Add
explicit unique index creation statements after the daily counter table
definitions for upload_daily_count and reading_daily_count, using the exact
names upload_daily_count_user_date_unique and
reading_daily_count_user_date_unique and indexing (user_id, date_key). Preserve
the existing inline UNIQUE constraints and use IF NOT EXISTS as specified.
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.
Summary by CodeRabbit
New Features
Bug Fixes
Tests