feat(store): billing subscription and checkout reads skip soft-deleted rows - #1963
Draft
rohilsurana wants to merge 2 commits into
Draft
rohilsurana wants to merge 2 commits into
rohilsurana wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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 |
Coverage Report for CI Build 36725725341Coverage increased (+0.4%) to 53.41%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
This branch was successfully deployed
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
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
The billing subscription and checkout repositories read rows without checking
deleted_at. They now skip deleted rows, using the sameliveandfromLivehelpers as the rest of the store.These are the last two of the five repositories the org delete cascade touches. #1962 does the other three and is still open, so all five only filter once both land. Nothing writes
deleted_atto either table yet, so every read returns the same rows as before.Changes
GetByID,GetByName,GetByProviderIDandListnow read fromfromLive.GetByID,GetByNameandListnow read fromfromLive.Technical Details
The two correlated subqueries on
billing_customersin the subscription repository are left alone on purpose. They only fill the org id and customer name into theRETURNINGclause ofCreateandUpdateByID, for the audit record. Filtering them would do more than blank two columns: both fields are plain strings, so a NULL would fail the struct scan and abort the whole write. An audit record should say which org a subscription belonged to even after the customer is deleted.GetByNameon both repositories is unreachable. Neither table has anamecolumn and neither method has a caller, so calling either fails with a postgres error beforedeleted_atmatters. I filtered them to keep the files consistent, but they are dead code and deleting them would be a fair follow-up.The update paths and the hard deletes are untouched, matching #1962.
No migration. Subscriptions have carried
deleted_atsince the table was created, and checkouts got it in20260916100000_soft_delete_columns.Test Plan
go test -run 'TestBillingSubscriptionRepositoryPG|TestBillingCheckoutRepositoryPG' ./internal/store/postgres/passesmaingolangci-lint run ./internal/store/postgres/...reports no issuesgo test ./internal/store/postgres/ ./billing/...passesEach suite seeds a live row and a deleted one, and I watched all five tests fail before adding the filters. Reverting the seven
fromLivecalls was also checked: every test fails, and each test exercises exactly one read, so each one catches its own change.SQL Safety (if your PR touches
*_repository.goorgoqu.*)?placeholders,goqu.Ex{}, orgoqu.Record{}— neverfmt.Sprintfor+building a query that gets executed.ToSQL()callers capture and forward params (query, params, err := stmt.ToSQL(); db.…Context(ctx, …, query, params...)). Neverquery, _, err := ….?placeholders inside single-quoted SQL literals ingoqu.L(usemake_interval(hours => ?)-style functions instead).//nolint:forbidigoor// #nosec G20xannotation has a one-line justification on the same line that a reviewer can verify.The added predicates come from the existing
live()helper and bind no values. The onlyfmt.Sprintfin the diff buildsTRUNCATEin the test teardowns from package table constants, the same as the other suites here.