fix(api/purchases): add scheduled_execution_at to GetPlannedExecutions SELECT - #1255
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesFix GetPlannedExecutions SQL projection and regression guard
Estimated code review effort: 1 (Trivial) | ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
1de7bb0 to
9ccf5ed
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
9ccf5ed to
8de0d5d
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
…s SELECT
GetPlannedExecutions projected 26 columns, but scanExecutionRows scans 27
(the 27th being scheduled_execution_at). Against a real PostgreSQL instance
this caused rows.Scan to fail ("expected 27 destination arguments in Scan,
not 26"), returning "failed to scan execution" and surfacing as a 500 on
the Planned Purchases list endpoint.
The regression drifted in via PR #804 (commit 8e5f139) which added
scheduled_execution_at to scanExecutionRows and every other SELECT feeding
it, but skipped GetPlannedExecutions.
Also tighten TestPGXMock_GetPlannedExecutions_ProjectsAllScanColumns:
the mock ExpectQuery regexp now requires both idempotency_key AND
scheduled_execution_at in the SQL so future column-count drift is caught
at test time rather than in production.
Closes #1247
…GetPlannedExecutions test The regression test already verified the scan column count (select+scan both have 27 columns). This tightens it to also assert that a NULL scheduled_execution_at comes back as a nil *time.Time pointer rather than a zero-value time, confirming applyNullTimesToExecution's Valid-guard.
…GetPlannedExecutions Extend TestPGXMock_GetPlannedExecutions_ProjectsAllScanColumns with a second row carrying a non-NULL scheduled_execution_at and assert it deserialises into the ScheduledExecutionAt pointer. The prior test only covered the NULL path (nil pointer), which passes even if a real value is silently dropped. This adds the direct regression guard for the fix: with the column absent from the SELECT projection the value never reaches ScheduledExecutionAt and every delayed execution reads back as unscheduled.
8de0d5d to
5d954f2
Compare
|
Rebased onto latest Post-rebase CI:
Scope of this PR remains a single production-code change: @coderabbitai review |
|
✅ Action performedReview finished.
|
Closes #1247
Symptom
On the Plans page, the "Planned Purchases" section fails to load with "Failed to load planned purchases: Internal server error" (HTTP 500). Reproducible by the Admin user, so this is not a permissions issue.
Root Cause
GetPlannedExecutionsininternal/config/store_postgres.goprojected only 26 columns in its SELECT, omittingscheduled_execution_at. The sharedscanExecutionRowshelper scans 27 columns, with&scheduledExecutionAtas the final (27th) scan target. Against a real PostgreSQL instance the row Scan fails with "expected 27 destination arguments in Scan, not 26", returning "failed to scan execution" which the handler surfaces as a 500.This drifted in via PR #804 (revocation-delay feature, commit 8e5f139) which added
scheduled_execution_attoscanExecutionRowsand every other SELECT feeding it (ListStuckExecutions,GetPendingExecutions,GetStaleApprovedExecutions), but skippedGetPlannedExecutions.Fix
scheduled_execution_atto theGetPlannedExecutionsSELECT in the correct position (afteridempotency_key, matching thescanExecutionRowsscan order and consistent with every sibling query).TestPGXMock_GetPlannedExecutions_ProjectsAllScanColumns: the mock ExpectQuery regexp now requires bothidempotency_keyANDscheduled_execution_atto appear in the issued SQL, so a future column-count drift is caught at test time instead of in production.Why the old test was a false positive
The previous matcher (
mock.ExpectQuery("idempotency_key")) matched any SQL containingidempotency_keyand returned a 27-column mock result regardless of the actual projection, so it passed even with the bug present. The tightened alternation regexp (idempotency_key.*scheduled_execution_at|scheduled_execution_at.*idempotency_key) fails to match a projection missing either column, the mock then returns no rows, and the test fails.Testing
go build ./...: passgo test ./internal/config/...: 585 passedscheduled_execution_at.Stacking
Stacked on #1254 (pre-commit repair); retarget to main when #1254 merges.
Summary by CodeRabbit