feat: subscription sync allow disabling continuous line logic #3226
Conversation
📝 WalkthroughWalkthroughIntroduces a new billing annotation constant and refactors subscription sync to use generic annotation helpers with GetBool. Broadly migrates ENT entities/builders from map[string]interface{} to models.Annotations, updates mutation/runtime utilities, adjusts customer/productcatalog mappings, and adds SQL migrations to set/augment ignore and force-continuous-lines annotations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
turip
force-pushed
the
feat/allow-disabling-continous-line-logic
branch
from
bb6ec73 to
1d8fede
Compare
turip
force-pushed
the
feat/allow-disabling-continous-line-logic
branch
from
1d8fede to
3eec6cc
Compare
turip
changed the title
turip
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (16)
openmeter/ent/db/subscriptionitem_update.go (1)
783-786: Typed annotations for UpdateOne: consistent with the new APIMatches the builder API change above.
openmeter/ent/db/addon_create.go (2)
936-940: Same UpsertOne consideration for annotations encodingApplies the same as above for AddonUpsertOne.SetAnnotations.
1355-1359: Same UpsertBulk consideration for annotations encodingApplies the same as above for AddonUpsertBulk.SetAnnotations.
openmeter/ent/db/usagereset_create.go (2)
458-462: UpsertOne: typed annotations setterSame as above; looks fine.
747-751: UpsertBulk: typed annotations setterSame note as above.
openmeter/ent/db/planaddon_create.go (2)
573-577: UpsertOne: typed annotations setterConsistent with above.
919-923: UpsertBulk: typed annotations setterConsistent with above.
openmeter/ent/db/entitlement_create.go (2)
1116-1120: UpsertOne: same annotations encoding concernSame as above; applies to EntitlementUpsertOne.SetAnnotations.
1555-1559: UpsertBulk: same annotations encoding concernSame as above; applies to EntitlementUpsertBulk.SetAnnotations.
openmeter/ent/db/mutation.go (7)
16452-16454: Same note: consider copying maps on SetAnnotationsSame consideration as for AddonMutation.SetAnnotations to avoid unintended external mutation effects.
See earlier comment on Lines 819-821.
29599-29601: Same note: consider copying maps on SetAnnotationsSame consideration as for AddonMutation.SetAnnotations.
See earlier comment on Lines 819-821.
32505-32507: Same note: consider copying maps on SetAnnotationsSame consideration as for AddonMutation.SetAnnotations.
See earlier comment on Lines 819-821.
38449-38451: Same note: consider copying maps on SetAnnotationsSame consideration as for AddonMutation.SetAnnotations.
See earlier comment on Lines 819-821.
42258-42260: Same note: consider copying maps on SetAnnotationsSame consideration as for AddonMutation.SetAnnotations.
See earlier comment on Lines 819-821.
50044-50046: Same note: consider copying maps on SetAnnotationsSame consideration as for AddonMutation.SetAnnotations.
See earlier comment on Lines 819-821.
53169-53171: Same note: consider copying maps on SetAnnotationsSame consideration as for AddonMutation.SetAnnotations.
See earlier comment on Lines 819-821.
🧹 Nitpick comments (21)
tools/migrate/migrations/20250731160524_billing-second-resolution.up.sql (1)
5-5: Clarify the warning and fix minor wordingThe intent is clear. Consider making the warning more explicit (e.g., “also set billing.subscription.sync.force-continuous-lines: true on the affected lines”) and, while you’re here, correcting “continous” to “continuous” in the header comments above for consistency.
tools/migrate/migrations/20250815072132_force-continuous-lines-annotation.up.sql (1)
1-4: Make the migration null-safe and idempotent with COALESCEAs written, the UPDATE skips rows with null annotations. Using COALESCE in both SET and WHERE makes this robust and safe to re-run.
Apply this diff:
-UPDATE billing_invoice_lines - SET annotations = annotations || '{"billing.subscription.sync.force-continuous-lines": true}' - WHERE - (annotations is not null and annotations <> 'null'::jsonb) and (annotations -> 'billing.subscription.sync.ignore' = 'true'::jsonb); +UPDATE billing_invoice_lines +SET annotations = COALESCE(annotations, '{}'::jsonb) || jsonb_build_object('billing.subscription.sync.force-continuous-lines', true) +WHERE COALESCE(annotations -> 'billing.subscription.sync.ignore', 'false'::jsonb) = 'true'::jsonb;openmeter/customer/adapter/entitymapping.go (1)
31-33: Avoid retaining the entire db.Customer by taking the address of its field; copy first (or use lo.ToPtr) for consistency and tighter escape scope.Using
&e.Annotationsforceseto escape and keeps the whole struct alive. Copying the map header to a local before taking its address (or usinglo.ToPtr) avoids retaining unrelated fields and aligns with the Metadata pattern above.Apply one of the following diffs:
Option A (copy to local before taking address):
- annotations = &e.Annotations + anns := e.Annotations + annotations = &annsOption B (match the Metadata pattern using lo.ToPtr):
- annotations = &e.Annotations + annotations = lo.ToPtr(e.Annotations)openmeter/billing/annotations.go (2)
4-7: Doc nit: clarify “hierarchy” scope for ignoreConsider clarifying whether “hierarchy” means subscription → item → line, and whether annotations set at higher levels override/merge with lower levels during sync.
- // AnnotationSubscriptionSyncIgnore is used to mark a line or hierarchy as ignored in subscription syncing. - // Should be used in case there is a breaking change in the subscription synchronization process, preventing billing - // from issuing credit notes for the past periods. + // AnnotationSubscriptionSyncIgnore marks a line or its parent hierarchy (e.g., subscription → item → line) as ignored during subscription syncing. + // Use it to shield against retroactive corrections during breaking changes in the synchronization process, preventing billing + // from issuing credit notes for past periods.
9-14: Doc nit: define interaction when both flags are presentMinor: add a note on precedence/interaction when both Ignore and ForceContinuousLines are present (e.g., ignore gates patching, while force only affects start-time adjustment).
// AnnotationSubscriptionSyncForceContinuousLines is used to force the creation of continuous subscription item lines. // If the sync process finds a previously existing line with this annotation, and the next line generated will not start at the end of the previously // found line, the sync process will adjust the start of the next line to the end of the previously found line, so that we don't have gaps in the // invoices. +// When both this and AnnotationSubscriptionSyncIgnore are present, ignore controls whether the line participates in sync at all, +// while force-continuous-lines only applies to start-time adjustments of lines that do participate.tools/migrate/migrations/20250807075408_usageperiod-duration-calculations.up.sql (1)
467-484: Migration filter: prefer NOT EXISTS over NOT IN; consider coupling the force flagNOT EXISTS avoids surprises on NULL semantics and is generally friendlier to the planner. Also, the comment suggests pairing this with the new force-continuous-lines flag; consider either referencing the dedicated migration explicitly or adding a companion UPDATE here if that’s the intended one-shot operation.
-UPDATE billing_invoice_lines +UPDATE billing_invoice_lines bil SET annotations = CASE WHEN annotations IS NULL OR annotations = 'null'::jsonb THEN '{}'::jsonb ELSE annotations END || jsonb_build_object('billing.subscription.sync.ignore', true) WHERE -- Line type usage based "type" = 'usage_based' -- Status valid AND "status" = 'valid' -- InvoiceID belongs to a not gathering invoice - AND "invoice_id" NOT IN ( - SELECT "id" FROM billing_invoices - WHERE "status" = 'gathering' - ); + AND NOT EXISTS ( + SELECT 1 + FROM billing_invoices bi + WHERE bi.id = bil.invoice_id AND bi.status = 'gathering' + );Alternatively, add a follow-up UPDATE to set 'billing.subscription.sync.force-continuous-lines' = true for the same selection (if desired), or explicitly reference the separate migration that introduces it to avoid future copy/paste reuse discrepancies.
pkg/models/annotation.go (1)
5-21: Tighten GetBool and consider exposing presence-sensitive variants.
- The len(a) check is redundant; a nil map lookup is safe and returns ok=false.
- Consider adding a presence-sensitive helper (e.g., GetBoolOk or GetBoolDefault) to distinguish “absent/wrong type” from “present=false” when needed.
Apply this minor simplification:
func (a Annotations) GetBool(key string) bool { - if len(a) == 0 { - return false - } - val, ok := a[key] if !ok { return false } boolVal, ok := val.(bool) if !ok { return false } return boolVal }If you foresee call sites needing to differentiate absence vs false, add one of these:
// Optional additions (same file) func (a Annotations) GetBoolDefault(key string, def bool) bool { if v, ok := a[key].(bool); ok { return v } return def } func (a Annotations) GetBoolOk(key string) (bool, bool) { v, ok := a[key] if !ok { return false, false } b, ok := v.(bool) return b, ok }openmeter/billing/worker/subscription/sync_test.go (1)
3502-3504: DRY up repeated annotation literals in tests.To reduce duplication and prevent typos, consider a helper or shared var for the common annotation set used in multiple tests.
For example, near the top of the test file or as a suite helper:
var ignoreAndForceAnnotations = models.Annotations{ billing.AnnotationSubscriptionSyncIgnore: true, billing.AnnotationSubscriptionSyncForceContinuousLines: true, }Then replace occurrences with:
line.Annotations = ignoreAndForceAnnotationsAlso applies to: 3519-3521, 3563-3565, 3679-3681, 4521-4523
openmeter/productcatalog/http/mapping.go (3)
905-912: *DRY suggestion: factor common “map → api.Annotations” conversionFromValidationAttributes is correct. You can eliminate duplication with FromAnnotations by introducing a tiny generic helper.
Apply this diff to use a shared helper:
func FromValidationAttributes(attrs models.Attributes) *api.Annotations { - if len(attrs) == 0 { - return nil - } - return lo.ToPtr((api.Annotations)(attrs)) + return toAPIAnnotations(attrs) }Add this helper (place near these functions):
// toAPIAnnotations converts any map[string]any-like type to *api.Annotations, returning nil when empty. func toAPIAnnotations[T ~map[string]any](m T) *api.Annotations { if len(m) == 0 { return nil } a := api.Annotations(m) return &a }
913-919: Unify with the same helper used aboveSame logic as FromValidationAttributes; route through a common converter to keep behavior consistent and testable.
func FromAnnotations(annotations models.Annotations) *api.Annotations { - if len(annotations) == 0 { - return nil - } - return lo.ToPtr((api.Annotations)(annotations)) + return toAPIAnnotations(annotations) }
933-934: LGTM: correct converter used for validation attributesSwitching to FromValidationAttributes(issue.Attributes()) makes the intent clear and keeps attribute-to-API conversion centralized.
If you want to further improve consistency, consider using the same FromMetadata helper everywhere in this file (some places still use lo.EmptyableToPtr(api.Metadata(...))).
openmeter/billing/worker/subscription/sync.go (2)
341-345: Update the comment to reflect the new gating with “force-continuous-lines”The comment still states period correction happens when the line “has billing.subscription.sync.ignore annotation,” but the code now also requires the previous line to have billing.subscription.sync.force-continuous-lines.
Apply this doc-only change:
-// The adjustment only happens if the line is subscription managed and has billing.subscription.sync.ignore annotation. This esentially -// allows for reanchoring if the period calculation changes. +// The adjustment only happens if: +// - the target is subscription-managed, and +// - the previous line has the billing.subscription.sync.ignore annotation, and +// - the previous line has the billing.subscription.sync.force-continuous-lines annotation. +// This allows explicit re-anchoring when the period calculation changes, decoupled from the ignore behavior.
422-441: Generic annotation helpers are clear; scope to subscription-managed lines is appropriate
- lineOrHierarchyHasAnnotation cleanly abstracts per-type checks.
- lineHasAnnotation wisely limits evaluation to subscription-managed lines; prevents manual edits from influencing automation.
- hierarchyHasAnnotation inspecting the last-in-scope child makes sense for progressive lines.
Consider a debug log (at very low verbosity) when gating causes a skip; it helps clarify why corrections didn’t apply during investigations.
Also applies to: 443-452, 454-465
openmeter/ent/db/mutation.go (8)
18585-18588: Accept both typed and plain map in SetField for BillingInvoiceLineMirror the compatibility guard suggested for Addon.
Apply this diff:
- v, ok := value.(models.Annotations) - if !ok { - return fmt.Errorf("unexpected type %T for field %s", value, name) - } + var v models.Annotations + switch val := value.(type) { + case models.Annotations: + v = val + case map[string]interface{}: + v = models.Annotations(val) + default: + return fmt.Errorf("unexpected type %T for field %s", value, name) + }
30335-30338: Accept both typed and plain map in Customer SetFieldBack-compat aid analogous to other entities.
Apply this diff:
- v, ok := value.(models.Annotations) - if !ok { - return fmt.Errorf("unexpected type %T for field %s", value, name) - } + var v models.Annotations + switch val := value.(type) { + case models.Annotations: + v = val + case map[string]interface{}: + v = models.Annotations(val) + default: + return fmt.Errorf("unexpected type %T for field %s", value, name) + }
33159-33162: Accept both typed and plain map in Entitlement SetFieldSame rationale for back-compat.
Apply this diff:
- v, ok := value.(models.Annotations) - if !ok { - return fmt.Errorf("unexpected type %T for field %s", value, name) - } + var v models.Annotations + switch val := value.(type) { + case models.Annotations: + v = val + case map[string]interface{}: + v = models.Annotations(val) + default: + return fmt.Errorf("unexpected type %T for field %s", value, name) + }
38730-38733: Accept both typed and plain map in NotificationEvent SetFieldSame back-compat improvement.
Apply this diff:
- v, ok := value.(models.Annotations) - if !ok { - return fmt.Errorf("unexpected type %T for field %s", value, name) - } + var v models.Annotations + switch val := value.(type) { + case models.Annotations: + v = val + case map[string]interface{}: + v = models.Annotations(val) + default: + return fmt.Errorf("unexpected type %T for field %s", value, name) + }
42805-42808: Accept both typed and plain map in PlanAddon SetFieldSame compatibility enhancement.
Apply this diff:
- v, ok := value.(models.Annotations) - if !ok { - return fmt.Errorf("unexpected type %T for field %s", value, name) - } + var v models.Annotations + switch val := value.(type) { + case models.Annotations: + v = val + case map[string]interface{}: + v = models.Annotations(val) + default: + return fmt.Errorf("unexpected type %T for field %s", value, name) + }
51237-51240: Accept both typed and plain map in SubscriptionItem SetFieldSame compatibility improvement.
Apply this diff:
- v, ok := value.(models.Annotations) - if !ok { - return fmt.Errorf("unexpected type %T for field %s", value, name) - } + var v models.Annotations + switch val := value.(type) { + case models.Annotations: + v = val + case map[string]interface{}: + v = models.Annotations(val) + default: + return fmt.Errorf("unexpected type %T for field %s", value, name) + }
53641-53644: Accept both typed and plain map in UsageReset SetFieldSame compatibility enhancement.
Apply this diff:
- v, ok := value.(models.Annotations) - if !ok { - return fmt.Errorf("unexpected type %T for field %s", value, name) - } + var v models.Annotations + switch val := value.(type) { + case models.Annotations: + v = val + case map[string]interface{}: + v = models.Annotations(val) + default: + return fmt.Errorf("unexpected type %T for field %s", value, name) + }
819-821: Confirm conversion required: API exposes api.Annotations (map[string]interface{}) while ent setters expect models.Annotations — ensure explicit conversions at mapping layersShort summary:
- api/api.gen.go declares: type Annotations map[string]interface{} (API).
- ent setters (SetAnnotations) and mutation code expect models.Annotations.
- Adapter/repo code frequently calls query.SetAnnotations(*params.Annotations) or cmd.SetAnnotations(input.Annotations) without an explicit conversion visible in the grep hits.
Locations to inspect/fix (representative findings from the verification run):
- API type:
- api/api.gen.go:1008 — type Annotations map[string]interface{}
- api/client/go/client.gen.go:923 — same
- Adapter/repo call sites that pass API/plain-map values into SetAnnotations:
- openmeter/productcatalog/planaddon/adapter/planaddon.go:377 — query = query.SetAnnotations(*params.Annotations)
- openmeter/productcatalog/addon/adapter/addon.go:469 — query = query.SetAnnotations(*params.Annotations)
- openmeter/entitlement/adapter/entitlement.go:128 — cmd.SetAnnotations(ent.Annotations)
- openmeter/customer/adapter/customer.go:191,493 — query = query.SetAnnotations(*input.Annotation)
- openmeter/subscription/repo/subscriptionitemrepo.go:111 — cmd.SetAnnotations(input.Annotations)
- Ent / generated DB expectations:
- Generated setters: many files under openmeter/ent/db/* have SetAnnotations(v models.Annotations)
- mutation dispatch (openmeter/ent/db/mutation.go) calls m.SetAnnotations(v) after asserting value type to models.Annotations in multiple places.
- DB create/update code shows serialization logic (sometimes to JSON/string) for annotations fields (examples in ent/db/*create.go and *update.go).
Actionable next steps:
- In each mapping/adapter/repo/handler that takes API-layer Annotations (api.Annotations or plain map[string]interface{}) and calls SetAnnotations(...) convert explicitly to models.Annotations, e.g.:
- query = query.SetAnnotations(models.Annotations(*params.Annotations))
- or add a small helper: func ToModelAnnotations(a api.Annotations) models.Annotations { return models.Annotations(a) } to centralize conversion and add validation where necessary.
- Verify handlers that accept raw JSON bodies (HTTP) cast incoming map[string]interface{} -> api.Annotations -> models.Annotations before passing to DB layer.
- Optionally, consider changing the generated API type to alias models.Annotations (type Annotations = models.Annotations) if you want assignability without conversions — but weigh coupling across packages.
If you want, I can:
- Prepare a small patch (or helper) to add explicit conversions at the adapter boundaries (pick a representative adapter to change), or
- Search & produce a list of all files that currently call SetAnnotations(...) with non-model values so you can apply conversions in bulk.
2 participants
Overview
The
billing.subscription.sync.ignorewas doing two things before this PR:This patch splits the continuous logic into a seperate flag, so that we can govern these behaviors independently.
Summary by CodeRabbit
New Features
Refactor
Chores