feat(multi-subject): entitlement subjects#3576
Conversation
📝 WalkthroughWalkthroughThis PR removes subject tracking from entitlements, deletes several subject/entitlement validator hooks and their wiring, introduces EntitlementResetEventV2 (customer-centric), adds subject-deletion cleanup (PostDelete) to customer hooks, updates APIs/tests for multi-subject usage attribution, and includes DB migrations and related caller updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor SubjectService as SubjectSvc
participant SubjectCustomerHook as SubjectCustomerHook (PostDelete)
participant CustomerService as CustomerSvc
Note over SubjectSvc,SubjectCustomerHook: Subject deletion triggers registered hooks
SubjectSvc->>SubjectCustomerHook: PostDelete(ctx, subject)
activate SubjectCustomerHook
SubjectCustomerHook->>CustomerSvc: GetCustomerByUsageAttribution(namespace, subjectKey)
alt customer not found or deleted
SubjectCustomerHook-->>SubjectSvc: return nil (no-op)
else customer found and active
SubjectCustomerHook->>CustomerSvc: UpdateCustomer(remove subjectKey from UsageAttribution)
CustomerSvc-->>SubjectCustomerHook: updated customer / error
SubjectCustomerHook-->>SubjectSvc: return success / propagate error
end
deactivate SubjectCustomerHook
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to focus during review:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/subject/service/service_test.go (1)
201-206: Verify customer UsageAttribution cleanup after subject deletion.The comment on line 205 nicely explains the behavior change, but the test doesn't verify what happens to the customer's
UsageAttributionafter the subject is deleted. According to the AI summary, there's aPostDeletehook that should remove the subject key from the customer's usage attribution.Consider adding assertions to verify:
- The subject is soft-deleted
- The customer's
UsageAttribution.SubjectKeysno longer includessub1.Key- The entitlement remains active and associated with the customer
err = env.SubjectService.Delete(t.Context(), models.NamespacedID{ Namespace: sub1.Namespace, ID: sub1.Id, }) require.NoErrorf(t, err, "We will not delete the entitlements as they belong to the customer not the subject") + +// Verify customer usage attribution was cleaned up +updatedCus, err := env.CustomerService.GetCustomer(t.Context(), customer.GetCustomerInput{ + Namespace: cus.Namespace, + ID: cus.ID, +}) +require.NoErrorf(t, err, "getting customer should not fail") +assert.NotContainsf(t, updatedCus.UsageAttribution.SubjectKeys, sub1.Key, "subject key should be removed from customer usage attribution") + +// Verify subject is soft-deleted +deletedSub, err := env.SubjectService.GetById(t.Context(), models.NamespacedID{ + Namespace: sub1.Namespace, + ID: sub1.Id, +}) +require.NoErrorf(t, err, "getting deleted subject should not fail") +assert.Truef(t, deletedSub.IsDeleted(), "subject should be soft-deleted")As per coding guidelines: "Make sure the tests are comprehensive and cover the changes."
🧹 Nitpick comments (6)
openmeter/subject/service/service_test.go (1)
207-239: Clarify the nested test structure.The nested
"Delete"test is a bit confusing because the subject was already deleted at line 201. CallingDeleteagain at line 225 doesn't reflect typical usage, and the test structure makes it unclear what scenario is being verified.Consider either:
- Renaming and restructuring to make the test intent clearer (e.g., "VerifySubjectRemainsSoftDeleted")
- Moving the soft-delete verification (lines 231-238) to immediately after the first deletion (after line 205)
- Removing this nested test if it's redundant
The current structure makes it look like you're testing deletion twice on the same subject, which might not be the intended scenario.
openmeter/entitlement/metered/reset.go (1)
56-66: Consider using clock abstraction for consistency.The v2 event structure looks good! One small observation: line 65 uses
time.Now()directly, but other parts of the codebase use a clock abstraction (likeclock.Now()inparser.go). For consistency and testability, consider using the same clock abstraction here.- ResetRequestedAt: time.Now(), + ResetRequestedAt: clock.Now(),openmeter/entitlement/driver/parser.go (2)
34-37: Silent error handling might mask issues.When
GetFirstSubjectKey()fails, the code defaults to an empty string. This might be confusing for debugging if a customer genuinely has no subject keys. Consider logging the error or checking if it's an expected "no keys" scenario versus an actual error.subjKey, err := metered.Customer.UsageAttribution.GetFirstSubjectKey() if err != nil { + // Log or handle expected vs unexpected errors subjKey = "" }
34-37: Consider extracting repeated pattern.The same
GetFirstSubjectKey()error handling pattern appears inToMetered,ToStatic, andToBoolean. Consider extracting this to a helper function to reduce duplication and make any future changes easier.func getSubjectKeyOrEmpty(ua customer.CustomerUsageAttribution) string { subjKey, err := ua.GetFirstSubjectKey() if err != nil { return "" } return subjKey }Then use:
subjKey := getSubjectKeyOrEmpty(metered.Customer.UsageAttribution)Also applies to: 76-79, 108-111
openmeter/customer/service/hooks/subjectcustomer.go (1)
62-133: Looks good overall! One thing to consider:The implementation correctly handles the happy path and gracefully deals with not-found/deleted customers. However, there's a subtle edge case: if the subject key isn't actually in the customer's
UsageAttribution.SubjectKeys, we'll still callUpdateCustomerwith the same list (sincelo.Filterwould return unchanged data).Consider adding a quick check before the update to skip the mutation if the subject key isn't present:
if !lo.Contains(cus.UsageAttribution.SubjectKeys, sub.Key) { span.AddEvent("subject key not in customer usage attribution") return nil }This would avoid unnecessary database operations and make the intent clearer. Otherwise, the error handling, tracing, and graceful nil returns are spot on!
openmeter/customer/customer.go (1)
220-230: Implementation looks good, but consider clarifying the deprecation note.The method correctly returns the first (lexicographically smallest) subject key after sorting, and properly clones the slice to avoid mutations. However, the deprecation note might be a bit confusing since this appears to be the new standard way to get a subject key across the codebase (replacing
GetSubjectKey()).Consider updating the deprecation comment to better explain the future direction:
// Deprecated: This is a transitional method for backwards compatibility as the codebase // migrates from subject-based to customer-based entitlements. In the future, direct // subject key lookups should be avoided in favor of customer-centric patterns.This would make it clearer that while the method is new, it's already marked for eventual removal.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (17)
openmeter/ent/db/client.gois excluded by!**/ent/db/**openmeter/ent/db/entitlement.gois excluded by!**/ent/db/**openmeter/ent/db/entitlement/entitlement.gois excluded by!**/ent/db/**openmeter/ent/db/entitlement/where.gois excluded by!**/ent/db/**openmeter/ent/db/entitlement_create.gois excluded by!**/ent/db/**openmeter/ent/db/entitlement_query.gois excluded by!**/ent/db/**openmeter/ent/db/entitlement_update.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**openmeter/ent/db/subject.gois excluded by!**/ent/db/**openmeter/ent/db/subject/subject.gois excluded by!**/ent/db/**openmeter/ent/db/subject/where.gois excluded by!**/ent/db/**openmeter/ent/db/subject_create.gois excluded by!**/ent/db/**openmeter/ent/db/subject_query.gois excluded by!**/ent/db/**openmeter/ent/db/subject_update.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (38)
app/common/customer.go(0 hunks)app/common/subject.go(0 hunks)cmd/server/wire.go(0 hunks)cmd/server/wire_gen.go(2 hunks)openmeter/customer/customer.go(3 hunks)openmeter/customer/errors.go(0 hunks)openmeter/customer/service/hooks/subjectcustomer.go(3 hunks)openmeter/customer/service/hooks/subjectvalidator.go(0 hunks)openmeter/customer/service/hooks/subjectvalidator_test.go(0 hunks)openmeter/ent/schema/entitlement.go(0 hunks)openmeter/ent/schema/subject.go(1 hunks)openmeter/entitlement/adapter/entitlement.go(2 hunks)openmeter/entitlement/adapter/entitlement_test.go(2 hunks)openmeter/entitlement/balanceworker/subject_customer.go(1 hunks)openmeter/entitlement/balanceworker/worker.go(1 hunks)openmeter/entitlement/driver/parser.go(6 hunks)openmeter/entitlement/entitlement.go(2 hunks)openmeter/entitlement/errors.go(1 hunks)openmeter/entitlement/events.go(0 hunks)openmeter/entitlement/metered/events.go(3 hunks)openmeter/entitlement/metered/reset.go(1 hunks)openmeter/entitlement/service/scheduling.go(1 hunks)openmeter/notification/consumer/entitlementbalancethreshold_test.go(0 hunks)openmeter/subject/adapter/subject.go(0 hunks)openmeter/subject/service/hooks/entitlementvalidator.go(0 hunks)openmeter/subject/service/service_test.go(1 hunks)openmeter/subscription/service/servicevalidation.go(1 hunks)openmeter/subscription/testutils/service.go(0 hunks)test/billing/suite.go(0 hunks)test/customer/customer.go(3 hunks)test/customer/customer_test.go(1 hunks)test/customer/subject.go(1 hunks)test/customer/testenv.go(4 hunks)test/entitlement/regression/scenario_test.go(1 hunks)test/notification/consumer_balance.go(2 hunks)test/notification/testenv.go(1 hunks)tools/migrate/migrations/20251104121422_remove-subject-from-entitlement.down.sql(1 hunks)tools/migrate/migrations/20251104121422_remove-subject-from-entitlement.up.sql(1 hunks)
💤 Files with no reviewable changes (13)
- openmeter/notification/consumer/entitlementbalancethreshold_test.go
- openmeter/subscription/testutils/service.go
- openmeter/entitlement/events.go
- openmeter/subject/adapter/subject.go
- app/common/subject.go
- openmeter/customer/errors.go
- app/common/customer.go
- test/billing/suite.go
- openmeter/ent/schema/entitlement.go
- cmd/server/wire.go
- openmeter/customer/service/hooks/subjectvalidator_test.go
- openmeter/subject/service/hooks/entitlementvalidator.go
- openmeter/customer/service/hooks/subjectvalidator.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them.Performance should be a priority in critical code paths. Anything related to event ingestion, message processing, database operations (regardless of database) should be vetted for potential performance bottlenecks.
Files:
openmeter/entitlement/metered/reset.goopenmeter/entitlement/driver/parser.goopenmeter/customer/service/hooks/subjectcustomer.goopenmeter/subscription/service/servicevalidation.goopenmeter/entitlement/balanceworker/worker.goopenmeter/entitlement/service/scheduling.goopenmeter/entitlement/balanceworker/subject_customer.gotest/notification/consumer_balance.goopenmeter/entitlement/adapter/entitlement.goopenmeter/subject/service/service_test.goopenmeter/customer/customer.goopenmeter/entitlement/adapter/entitlement_test.gotest/notification/testenv.gotest/customer/customer_test.gotest/entitlement/regression/scenario_test.goopenmeter/entitlement/errors.goopenmeter/entitlement/entitlement.goopenmeter/ent/schema/subject.gotest/customer/testenv.gotest/customer/subject.gotest/customer/customer.gocmd/server/wire_gen.goopenmeter/entitlement/metered/events.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests.
When appropriate, recommend e2e tests for critical changes.
Files:
openmeter/subject/service/service_test.goopenmeter/entitlement/adapter/entitlement_test.gotest/customer/customer_test.gotest/entitlement/regression/scenario_test.go
🧠 Learnings (6)
📓 Common learnings
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 3373
File: openmeter/subject/adapter/subject.go:62-65
Timestamp: 2025-09-12T09:37:57.052Z
Learning: In the OpenMeter subject module, soft-delete validation is handled at the service layer rather than the adapter layer, following a clean separation of concerns where business logic validation occurs at higher layers and the adapter focuses on data access operations.
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 3373
File: openmeter/subject/adapter/subject.go:62-65
Timestamp: 2025-09-12T09:37:57.052Z
Learning: In the OpenMeter subject module, soft-delete validation is handled at the service layer rather than the adapter layer, following a clean separation of concerns where business logic validation occurs at higher layers and the adapter focuses on data access operations.
📚 Learning: 2025-10-09T13:59:12.012Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 3486
File: openmeter/ingest/kafkaingest/serializer/serializer.go:105-107
Timestamp: 2025-10-09T13:59:12.012Z
Learning: In OpenMeter, the CloudEvents `subject` field is mandatory for the application's business logic, even though it's optional in the CloudEvents specification. The `ValidateKafkaPayloadToCloudEvent` function in `openmeter/ingest/kafkaingest/serializer/serializer.go` intentionally enforces this requirement.
Applied to files:
openmeter/entitlement/metered/reset.goopenmeter/entitlement/driver/parser.goopenmeter/subscription/service/servicevalidation.goopenmeter/entitlement/errors.goopenmeter/entitlement/entitlement.gotest/customer/testenv.goopenmeter/entitlement/metered/events.go
📚 Learning: 2025-03-07T12:17:43.129Z
Learnt from: GAlexIHU
Repo: openmeterio/openmeter PR: 2383
File: openmeter/entitlement/metered/lateevents_test.go:37-45
Timestamp: 2025-03-07T12:17:43.129Z
Learning: In the OpenMeter codebase, test files like `openmeter/entitlement/metered/lateevents_test.go` may use variables like `meterSlug` and `namespace` without explicit declarations visible in the same file. This appears to be an accepted pattern in their test structure.
Applied to files:
openmeter/entitlement/metered/reset.goopenmeter/entitlement/driver/parser.gotest/notification/consumer_balance.goopenmeter/entitlement/adapter/entitlement.goopenmeter/subject/service/service_test.goopenmeter/entitlement/adapter/entitlement_test.gotest/customer/testenv.gotest/customer/customer.go
📚 Learning: 2025-09-12T09:38:52.436Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 3373
File: openmeter/subject/adapter/subject.go:119-136
Timestamp: 2025-09-12T09:38:52.436Z
Learning: In OpenMeter subject adapter GetByIdOrKey method, ID-based lookups should return subjects even if soft-deleted, while Key-based lookups should be gated by DeletedAt filters. This is intentional design where IDs are treated as immutable references that can retrieve deleted entities, but Keys should only match active (non-deleted) subjects.
Applied to files:
openmeter/entitlement/adapter/entitlement_test.go
📚 Learning: 2025-04-20T11:15:07.499Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 2692
File: openmeter/productcatalog/plan/adapter/mapping.go:64-74
Timestamp: 2025-04-20T11:15:07.499Z
Learning: In the OpenMeter codebase, Ent's edge methods ending in "OrErr" (like AddonsOrErr()) only return NotLoadedError when the edge wasn't loaded, and cannot return DB errors. Simple err != nil checks are sufficient for these methods.
Applied to files:
openmeter/ent/schema/subject.go
📚 Learning: 2025-08-29T12:31:52.802Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 3291
File: app/common/customer.go:88-89
Timestamp: 2025-08-29T12:31:52.802Z
Learning: In Go projects using Google's wire dependency injection framework, named types (without =) should be used instead of type aliases (with =) to work around wire limitations. For example, use `type CustomerSubjectValidatorHook customerservicehooks.SubjectValidatorHook` instead of `type CustomerSubjectValidatorHook = customerservicehooks.SubjectValidatorHook` when wire is involved.
Applied to files:
test/customer/testenv.gocmd/server/wire_gen.go
🧬 Code graph analysis (16)
openmeter/entitlement/metered/reset.go (2)
openmeter/entitlement/metered/events.go (1)
EntitlementResetEventV2(73-81)api/api.gen.go (2)
Customer(2222-2272)CustomerUsageAttribution(2410-2413)
openmeter/entitlement/driver/parser.go (2)
openmeter/customer/customer.go (1)
Customer(41-53)api/api.gen.go (1)
Customer(2222-2272)
openmeter/customer/service/hooks/subjectcustomer.go (3)
openmeter/customer/customer.go (4)
GetCustomerByUsageAttributionInput(233-239)UpdateCustomerInput(330-333)CustomerID(147-147)CustomerMutate(112-122)pkg/models/errors.go (1)
IsGenericNotFoundError(57-65)pkg/clock/clock.go (1)
Now(14-21)
openmeter/entitlement/balanceworker/worker.go (6)
openmeter/watermill/grouphandler/grouphandler.go (1)
NewGroupEventHandler(29-31)openmeter/entitlement/metered/events.go (1)
EntitlementResetEventV2(73-81)pkg/models/id.go (1)
NamespacedID(7-10)openmeter/entitlement/balanceworker/entitlementhandler.go (3)
WithSource(49-53)WithEventAt(55-59)WithSourceOperation(61-65)openmeter/event/metadata/resourcepath.go (2)
ComposeResourcePath(29-31)EntityEntitlement(10-10)openmeter/entitlement/snapshot/event.go (1)
ValueOperationReset(22-22)
openmeter/entitlement/service/scheduling.go (1)
openmeter/entitlement/errors.go (1)
AlreadyExistsError(9-13)
test/notification/consumer_balance.go (4)
api/api.gen.go (2)
Customer(2222-2272)CustomerUsageAttribution(2410-2413)openmeter/streaming/query_params.go (2)
Customer(76-78)CustomerUsageAttribution(81-85)pkg/models/model.go (2)
ManagedResource(23-31)NamespacedModel(204-206)test/notification/testenv.go (2)
TestCustomerID(38-38)TestSubjectKey(36-36)
openmeter/entitlement/adapter/entitlement.go (3)
openmeter/ent/db/entitlement.go (2)
Entitlement(21-73)Entitlement(153-176)openmeter/ent/schema/entitlement.go (5)
Entitlement(20-22)Entitlement(24-31)Entitlement(33-71)Entitlement(73-84)Entitlement(86-111)openmeter/ent/db/customer_query.go (1)
CustomerQuery(27-43)
openmeter/customer/customer.go (3)
api/api.gen.go (2)
Customer(2222-2272)CustomerUsageAttribution(2410-2413)openmeter/streaming/query_params.go (2)
Customer(76-78)CustomerUsageAttribution(81-85)pkg/models/errors.go (1)
NewGenericValidationError(138-140)
test/customer/customer_test.go (1)
test/customer/customer.go (1)
CustomerHandlerTestSuite(54-58)
openmeter/entitlement/errors.go (1)
openmeter/ent/db/entitlement/where.go (1)
CustomerID(110-112)
openmeter/entitlement/entitlement.go (2)
api/api.gen.go (1)
Customer(2222-2272)openmeter/ent/db/customer.go (2)
Customer(20-64)Customer(142-157)
test/customer/testenv.go (4)
openmeter/customer/service/hooks/subjectcustomer.go (2)
NewSubjectCustomerHook(165-186)SubjectCustomerHookConfig(188-196)app/common/subject.go (2)
NewSubjectCustomerHook(35-53)Subject(18-21)openmeter/subject/service/hooks/customersubject.go (2)
NewCustomerSubjectHook(81-96)CustomerSubjectHookConfig(98-98)openmeter/billing/service.go (1)
CustomerOverrideService(35-42)
test/customer/subject.go (6)
test/customer/customer.go (1)
CustomerHandlerTestSuite(54-58)openmeter/customer/customer.go (7)
Customer(41-53)CreateCustomerInput(312-315)CustomerMutate(112-122)CustomerUsageAttribution(200-202)UpdateCustomerInput(330-333)CustomerID(147-147)GetCustomerInput(351-358)pkg/models/key.go (1)
NamespacedKey(5-8)pkg/models/id.go (1)
NamespacedID(7-10)openmeter/productcatalog/feature/connector.go (1)
CreateFeatureInputs(19-26)openmeter/entitlement/entitlement.go (1)
CreateEntitlementInputs(77-102)
test/customer/customer.go (4)
pkg/models/key.go (1)
NamespacedKey(5-8)openmeter/customer/customer.go (3)
CreateCustomerInput(312-315)CustomerMutate(112-122)CustomerUsageAttribution(200-202)openmeter/customer/errors.go (1)
IsSubjectKeyConflictError(88-96)pkg/models/errors.go (1)
IsGenericConflictError(87-95)
cmd/server/wire_gen.go (1)
app/common/telemetry.go (2)
NewTelemetryServer(261-268)TelemetryServer(259-259)
openmeter/entitlement/metered/events.go (2)
api/api.gen.go (2)
CustomerUsageAttribution(2410-2413)Subject(6898-6932)openmeter/event/metadata/resourcepath.go (2)
ComposeResourcePath(29-31)EntityEntitlement(10-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Artifacts / Benthos Collector Container image
- GitHub Check: Artifacts / Container image
- GitHub Check: Test
- GitHub Check: Migration Checks
- GitHub Check: Lint
- GitHub Check: Build
- GitHub Check: Code Generators
🔇 Additional comments (34)
openmeter/subscription/service/servicevalidation.go (1)
33-38: LGTM! Comment renumbering looks good.The step numbering has been updated correctly after removing the subject validation logic. The currency validation itself remains intact and properly handles the case where the customer has a currency set.
test/notification/testenv.go (1)
38-38: Nice addition! The constant fits right in.The new
TestCustomerIDconstant aligns well with the shift to a customer-centric model. The naming and value are consistent with the existing test constants, and placing it right afterTestSubjectIDmakes sense given the relationship between customers and subjects.test/notification/consumer_balance.go (1)
74-84: Nice update to the test fixture!The Customer object construction looks good and properly aligns with the customer-centric model. The nested structure with
ManagedResourceandUsageAttributionmatches the expected schema.cmd/server/wire_gen.go (1)
550-602: Auto-generated changes look consistent.The variable renumbering (v9 → v8) is expected since this file is auto-generated by Wire and reflects the removal of the validator hooks. The wiring structure remains sound.
openmeter/ent/schema/subject.go (1)
66-67: Verified: No code relies on the Subject→Entitlements edge.The search results confirm the removal is safe. All entitlements edge usage found is on the
Customerentity, notSubject. TheQueryEntitlements(),WithEntitlements(), andEntitlementsOrErr()patterns show zero results for Subject, and nothing else attempts to navigate from Subject to Entitlements.tools/migrate/migrations/20251104121422_remove-subject-from-entitlement.up.sql (1)
1-2: All clear—the migration is safe to deploy.I've verified your concerns and the good news: the codebase is ready. Here's what I found:
✅ No remaining code references — The search across all Go files (excluding tests and generated code) turned up zero active uses of
subject_keyorsubject_idin entitlements operations. No queries likeQuerySubject()orWithSubject()on the Entitlements entity.✅ Down migration is solid — The down migration file exists and is comprehensive. It not only restores the columns with the correct types but also recreates the indexes and the foreign key constraint to
subjects. It's production-ready for rollback if needed.✅ Schema already cleaned up — The current Entitlements schema definition has no references to subject columns—just the customer relationship. This confirms the code changes are complete and consistent.
The timing also makes sense: the migration is dated Nov 4, 2025, which aligns perfectly with OpenMeter's subject-based API removal date. You're good to ship this.
test/customer/customer_test.go (1)
63-71: LGTM! Nice test organization.The new "Subject" test subgroup follows the existing pattern well and integrates cleanly with the test suite structure.
openmeter/entitlement/balanceworker/worker.go (1)
295-306: LGTM! Clean v2 handler addition.The new
EntitlementResetEventV2handler mirrors the v1 handler nicely and maintains backward compatibility. The structure and parameter usage are consistent and correct.openmeter/entitlement/errors.go (1)
12-16: LGTM! Clean transition to customer-centric error.The field rename from
SubjectKeytoCustomerIDis consistent and the error message accurately reflects the new field. This aligns well with the customer-centric model.openmeter/entitlement/balanceworker/subject_customer.go (1)
30-33: LGTM! Proper error handling here.The update to
GetFirstSubjectKey()is correct, and I appreciate that this code properly propagates the error instead of silently defaulting to empty string. This makes debugging easier if a customer has no subject keys.openmeter/entitlement/service/scheduling.go (1)
99-99: LGTM! Consistent with error struct change.The update to use
CustomerID: conflict.Customer.IDcorrectly aligns with theAlreadyExistsErrorstruct change. TheCustomerfield should always be populated based on the entitlement construction flow, so this looks safe.test/customer/testenv.go (4)
10-10: LGTM! Imports look good.All new imports are properly utilized in the hook wiring below.
Also applies to: 13-13, 17-17, 28-28
159-169: LGTM! Subject customer hook is wired correctly.The hook configuration includes all required dependencies and is properly registered with the subject service. Using a noop tracer is appropriate for the test environment.
171-180: LGTM! Customer subject hook is wired correctly.The hook configuration and registration look good. Both hooks now establish the cross-service subject-customer lifecycle interactions needed for testing.
207-209: LGTM! Noop service is appropriate for tests.The noop implementation correctly embeds the interface, providing zero-value implementations for all methods. This is the right approach for a test environment that doesn't need actual billing interactions.
test/entitlement/regression/scenario_test.go (1)
631-631: LGTM! Method rename is correct.The update from
GetSubjectKey()toGetFirstSubjectKey()aligns with the API changes across the codebase. Since this test creates a customer with a single subject key, the "first" semantics (returning the smallest sorted key) won't affect the test behavior.openmeter/entitlement/adapter/entitlement_test.go (2)
250-250: LGTM! Subject-related assertion removed.The removal of subject-key assertions aligns with the broader PR changes to drop subject fields from entitlements. The test still properly validates the important fields after the upsert operation.
497-498: LGTM! Method rename is correct.The switch to
GetFirstSubjectKey()is consistent with the API changes. Error handling is properly in place, and the usage in subsequent assertions is appropriate.openmeter/customer/service/hooks/subjectcustomer.go (1)
13-13: LGTM! Necessary imports.Both imports are properly utilized in the new PostDelete method.
Also applies to: 26-26
test/customer/customer.go (4)
110-122: LGTM! Nice test addition.Verifying that subjects are created alongside customers is a good practice, especially with the new hook wiring. The subtest structure keeps things organized.
125-141: LGTM! Nice refactoring.Wrapping these conflict tests in subtests makes the test structure much clearer and easier to follow. The test logic itself remains solid.
Also applies to: 144-161, 164-181
207-207: LGTM! Clear test data.The new subject key name is descriptive and follows the existing pattern.
245-271: LGTM! Excellent test coverage.These subtests properly verify the subject lifecycle during customer updates: new subjects are created, and old subjects are left dangling rather than deleted. This is important behavior to test explicitly!
test/customer/subject.go (3)
1-65: LGTM! Great test setup.The first subtest nicely covers the dangling subject scenario: create a customer with a subject, remove it from usage attribution, then delete the subject. This is an important edge case to test!
67-109: LGTM! Core hook behavior tested.This subtest validates the main PostDelete hook behavior: when a subject is deleted while still in a customer's usage attribution, the customer is automatically updated to remove that subject key. The assertion properly checks for an empty list.
111-171: LGTM! Important edge case covered.This test addresses a potentially tricky scenario: a customer with active entitlements but no remaining subjects after deletion. The test name clearly communicates the expected behavior (no error), which is helpful for future maintainers.
openmeter/entitlement/metered/events.go (3)
7-7: LGTM! Clean deprecation path.The import is necessary for the v2 event, and the deprecation comment clearly guides users to the new version.
Also applies to: 18-18
73-91: LGTM! V2 event structure looks good.The new event properly replaces subject-based fields with customer-centric ones (
CustomerIDandCustomerUsageAttribution). The versioning is clear, and interface implementation is correctly declared.
93-130: LGTM! Event methods implemented correctly.The
EventName(),EventMetadata(), andValidate()methods are all properly implemented:
- Metadata subject path now references the customer instead of subject key (line 100)
- Validation covers all required fields including the nested
CustomerUsageAttribution- Error messages are clear and consistent
openmeter/customer/customer.go (2)
6-6: LGTM! Necessary import.The
slicesimport is used inGetFirstSubjectKey()for cloning and sorting operations.
55-68: LGTM! Handy conversion helper.The
AsCustomerMutate()method provides a clean way to convert aCustomerto aCustomerMutate, mapping all the relevant fields. This simplifies mutation workflows and reduces boilerplate.openmeter/entitlement/adapter/entitlement.go (3)
318-320: Nice work on the customer-centric model! 🎯The query properly loads customers with their subjects via the customer adapter, which is exactly right for the new model. The eager loading pattern is consistent with the rest of the file and should perform well.
830-832: Consistent pattern maintained! ✓The same customer-with-subjects loading pattern is applied here, keeping things nice and uniform across the codebase.
56-81: The DB consistency assumption here is solid – the migrations back it upGood catch on thinking about data consistency! Your original concern was totally valid during development, but here's what happened: The recent migration chain (especially
20250821121421_entitlement-customer-linkand20251104121422_remove-subject-from-entitlement) actually handled this with a deliberate backfill strategy.The migration backfills
customer_idfrom thecustomer_subjectsmapping by joining on namespace and subject_key, then adds a foreign key constraint fromentitlements.customer_idtocustomers.id. Since we just crossed the November 1st consolidation deadline (today's Nov 4th), the final cleanup migration already ran, removing the old subject columns.So the code's assumption that customer-subject data is consistent and properly linked? That's baked in by the migration strategy. The FK constraint ensures integrity going forward. You're all good here.
tools/migrate/migrations/20251110132128_remove-subject-from-entitlement.down.sql
Show resolved
Hide resolved
GAlexIHU
force-pushed
the
feat/multi-subject/entitlement-subject
branch
from
2bfdaae to
d728430
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/subscription/service/servicevalidation.go (1)
23-23: Fix typo in comment.Small typo here: "Valiate" should be "Validate".
Apply this diff:
- // 1. Valiate the spec + // 1. Validate the spec
🧹 Nitpick comments (2)
test/customer/subject.go (1)
108-108: Make the empty assertion tolerant to nil slices.Minor thing: once the backend clears a customer's subjects,
UsageAttribution.SubjectKeyscan come back asnil, andrequire.Equal([]string{}, …)will trip even though the data is effectively empty. Swapping torequire.Empty(or asserting len == 0) keeps the intent while handling both representations. Something like:- require.Equal(t, []string{}, cust.UsageAttribution.SubjectKeys, "Customer usage attribution subject keys must be empty") + require.Empty(t, cust.UsageAttribution.SubjectKeys, "Customer usage attribution subject keys must be empty")…and similarly in the later subtest.
Also applies to: 170-170
openmeter/customer/service/hooks/subjectcustomer.go (1)
102-130: Minor: Consider checking error before result.The logic is correct, but conventionally you'd check
errbefore checkingcus != nil. Currently you're logging the result before checking if the operation failed. Consider:cus, err = s.provisioner.customer.UpdateCustomer(ctx, customer.UpdateCustomerInput{ CustomerID: customer.CustomerID{ Namespace: cus.Namespace, ID: cus.ID, }, CustomerMutate: func() customer.CustomerMutate { mut := cus.AsCustomerMutate() mut.UsageAttribution.SubjectKeys = lo.Filter(mut.UsageAttribution.SubjectKeys, func(key string, _ int) bool { return key != sub.Key }) return mut }(), }) + if err != nil { + span.AddEvent("failed to update customer usage attribution", trace.WithAttributes( + attribute.String("error", err.Error()), + )) + return err + } + if cus != nil { span.AddEvent("updated customer usage attribution", trace.WithAttributes( attribute.String("customer.usage_attribution.subject_keys", strings.Join(cus.UsageAttribution.SubjectKeys, ", ")), )) } - - if err != nil { - span.AddEvent("failed to update customer usage attribution", trace.WithAttributes( - attribute.String("error", err.Error()), - )) - - return err - }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (17)
openmeter/ent/db/client.gois excluded by!**/ent/db/**openmeter/ent/db/entitlement.gois excluded by!**/ent/db/**openmeter/ent/db/entitlement/entitlement.gois excluded by!**/ent/db/**openmeter/ent/db/entitlement/where.gois excluded by!**/ent/db/**openmeter/ent/db/entitlement_create.gois excluded by!**/ent/db/**openmeter/ent/db/entitlement_query.gois excluded by!**/ent/db/**openmeter/ent/db/entitlement_update.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**openmeter/ent/db/subject.gois excluded by!**/ent/db/**openmeter/ent/db/subject/subject.gois excluded by!**/ent/db/**openmeter/ent/db/subject/where.gois excluded by!**/ent/db/**openmeter/ent/db/subject_create.gois excluded by!**/ent/db/**openmeter/ent/db/subject_query.gois excluded by!**/ent/db/**openmeter/ent/db/subject_update.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (38)
app/common/customer.go(0 hunks)app/common/subject.go(0 hunks)cmd/server/wire.go(0 hunks)cmd/server/wire_gen.go(2 hunks)openmeter/customer/customer.go(3 hunks)openmeter/customer/errors.go(0 hunks)openmeter/customer/service/hooks/subjectcustomer.go(3 hunks)openmeter/customer/service/hooks/subjectvalidator.go(0 hunks)openmeter/customer/service/hooks/subjectvalidator_test.go(0 hunks)openmeter/ent/schema/entitlement.go(0 hunks)openmeter/ent/schema/subject.go(1 hunks)openmeter/entitlement/adapter/entitlement.go(2 hunks)openmeter/entitlement/adapter/entitlement_test.go(2 hunks)openmeter/entitlement/balanceworker/subject_customer.go(1 hunks)openmeter/entitlement/balanceworker/worker.go(1 hunks)openmeter/entitlement/driver/parser.go(6 hunks)openmeter/entitlement/entitlement.go(2 hunks)openmeter/entitlement/errors.go(1 hunks)openmeter/entitlement/events.go(0 hunks)openmeter/entitlement/metered/events.go(3 hunks)openmeter/entitlement/metered/reset.go(1 hunks)openmeter/entitlement/service/scheduling.go(1 hunks)openmeter/notification/consumer/entitlementbalancethreshold_test.go(0 hunks)openmeter/subject/adapter/subject.go(0 hunks)openmeter/subject/service/hooks/entitlementvalidator.go(0 hunks)openmeter/subject/service/service_test.go(1 hunks)openmeter/subscription/service/servicevalidation.go(1 hunks)openmeter/subscription/testutils/service.go(0 hunks)test/billing/suite.go(0 hunks)test/customer/customer.go(3 hunks)test/customer/customer_test.go(1 hunks)test/customer/subject.go(1 hunks)test/customer/testenv.go(4 hunks)test/entitlement/regression/scenario_test.go(1 hunks)test/notification/consumer_balance.go(2 hunks)test/notification/testenv.go(1 hunks)tools/migrate/migrations/20251104121422_remove-subject-from-entitlement.down.sql(1 hunks)tools/migrate/migrations/20251104121422_remove-subject-from-entitlement.up.sql(1 hunks)
💤 Files with no reviewable changes (13)
- openmeter/subject/adapter/subject.go
- cmd/server/wire.go
- openmeter/customer/service/hooks/subjectvalidator_test.go
- openmeter/subscription/testutils/service.go
- openmeter/customer/errors.go
- openmeter/notification/consumer/entitlementbalancethreshold_test.go
- app/common/customer.go
- openmeter/customer/service/hooks/subjectvalidator.go
- test/billing/suite.go
- app/common/subject.go
- openmeter/ent/schema/entitlement.go
- openmeter/subject/service/hooks/entitlementvalidator.go
- openmeter/entitlement/events.go
🚧 Files skipped from review as they are similar to previous changes (12)
- test/customer/customer.go
- openmeter/entitlement/balanceworker/subject_customer.go
- test/customer/customer_test.go
- test/entitlement/regression/scenario_test.go
- openmeter/entitlement/service/scheduling.go
- openmeter/entitlement/metered/reset.go
- tools/migrate/migrations/20251104121422_remove-subject-from-entitlement.up.sql
- tools/migrate/migrations/20251104121422_remove-subject-from-entitlement.down.sql
- openmeter/entitlement/entitlement.go
- test/notification/testenv.go
- openmeter/entitlement/balanceworker/worker.go
- openmeter/entitlement/driver/parser.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them.Performance should be a priority in critical code paths. Anything related to event ingestion, message processing, database operations (regardless of database) should be vetted for potential performance bottlenecks.
Files:
openmeter/subscription/service/servicevalidation.gotest/notification/consumer_balance.goopenmeter/subject/service/service_test.goopenmeter/entitlement/adapter/entitlement.goopenmeter/customer/customer.goopenmeter/entitlement/adapter/entitlement_test.goopenmeter/customer/service/hooks/subjectcustomer.goopenmeter/ent/schema/subject.gotest/customer/subject.gocmd/server/wire_gen.goopenmeter/entitlement/metered/events.gotest/customer/testenv.goopenmeter/entitlement/errors.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests.
When appropriate, recommend e2e tests for critical changes.
Files:
openmeter/subject/service/service_test.goopenmeter/entitlement/adapter/entitlement_test.go
🧠 Learnings (7)
📓 Common learnings
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 3373
File: openmeter/subject/adapter/subject.go:119-136
Timestamp: 2025-09-12T09:38:52.436Z
Learning: In OpenMeter subject adapter GetByIdOrKey method, ID-based lookups should return subjects even if soft-deleted, while Key-based lookups should be gated by DeletedAt filters. This is intentional design where IDs are treated as immutable references that can retrieve deleted entities, but Keys should only match active (non-deleted) subjects.
📚 Learning: 2025-10-09T13:59:12.012Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 3486
File: openmeter/ingest/kafkaingest/serializer/serializer.go:105-107
Timestamp: 2025-10-09T13:59:12.012Z
Learning: In OpenMeter, the CloudEvents `subject` field is mandatory for the application's business logic, even though it's optional in the CloudEvents specification. The `ValidateKafkaPayloadToCloudEvent` function in `openmeter/ingest/kafkaingest/serializer/serializer.go` intentionally enforces this requirement.
Applied to files:
openmeter/subscription/service/servicevalidation.goopenmeter/entitlement/metered/events.gotest/customer/testenv.go
📚 Learning: 2025-03-07T12:17:43.129Z
Learnt from: GAlexIHU
Repo: openmeterio/openmeter PR: 2383
File: openmeter/entitlement/metered/lateevents_test.go:37-45
Timestamp: 2025-03-07T12:17:43.129Z
Learning: In the OpenMeter codebase, test files like `openmeter/entitlement/metered/lateevents_test.go` may use variables like `meterSlug` and `namespace` without explicit declarations visible in the same file. This appears to be an accepted pattern in their test structure.
Applied to files:
test/notification/consumer_balance.goopenmeter/subject/service/service_test.goopenmeter/entitlement/adapter/entitlement.goopenmeter/entitlement/adapter/entitlement_test.gotest/customer/testenv.go
📚 Learning: 2025-09-12T09:38:52.436Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 3373
File: openmeter/subject/adapter/subject.go:119-136
Timestamp: 2025-09-12T09:38:52.436Z
Learning: In OpenMeter subject adapter GetByIdOrKey method, ID-based lookups should return subjects even if soft-deleted, while Key-based lookups should be gated by DeletedAt filters. This is intentional design where IDs are treated as immutable references that can retrieve deleted entities, but Keys should only match active (non-deleted) subjects.
Applied to files:
openmeter/entitlement/adapter/entitlement_test.go
📚 Learning: 2025-04-20T11:15:07.499Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 2692
File: openmeter/productcatalog/plan/adapter/mapping.go:64-74
Timestamp: 2025-04-20T11:15:07.499Z
Learning: In the OpenMeter codebase, Ent's edge methods ending in "OrErr" (like AddonsOrErr()) only return NotLoadedError when the edge wasn't loaded, and cannot return DB errors. Simple err != nil checks are sufficient for these methods.
Applied to files:
openmeter/ent/schema/subject.go
📚 Learning: 2025-08-29T12:31:52.802Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 3291
File: app/common/customer.go:88-89
Timestamp: 2025-08-29T12:31:52.802Z
Learning: In Go projects using Google's wire dependency injection framework, named types (without =) should be used instead of type aliases (with =) to work around wire limitations. For example, use `type CustomerSubjectValidatorHook customerservicehooks.SubjectValidatorHook` instead of `type CustomerSubjectValidatorHook = customerservicehooks.SubjectValidatorHook` when wire is involved.
Applied to files:
cmd/server/wire_gen.gotest/customer/testenv.go
📚 Learning: 2025-08-29T12:31:46.048Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 3291
File: app/common/customer.go:59-60
Timestamp: 2025-08-29T12:31:46.048Z
Learning: In dependency injection with Google Wire, named types (e.g., `type CustomerSubjectHook customerservicehooks.SubjectCustomerHook`) are sometimes required instead of type aliases (e.g., `type CustomerSubjectHook = customerservicehooks.SubjectCustomerHook`) to work around Wire's limitations in recognizing and handling type aliases properly during code generation.
Applied to files:
cmd/server/wire_gen.go
🧬 Code graph analysis (9)
test/notification/consumer_balance.go (4)
api/api.gen.go (2)
Customer(2222-2272)CustomerUsageAttribution(2410-2413)openmeter/streaming/query_params.go (2)
Customer(76-78)CustomerUsageAttribution(81-85)pkg/models/model.go (2)
ManagedResource(23-31)NamespacedModel(204-206)test/notification/testenv.go (2)
TestCustomerID(38-38)TestSubjectKey(36-36)
openmeter/entitlement/adapter/entitlement.go (3)
openmeter/ent/db/entitlement.go (2)
Entitlement(21-73)Entitlement(153-176)openmeter/ent/schema/entitlement.go (5)
Entitlement(20-22)Entitlement(24-31)Entitlement(33-71)Entitlement(73-84)Entitlement(86-111)openmeter/ent/db/customer_query.go (1)
CustomerQuery(27-43)
openmeter/customer/customer.go (3)
api/api.gen.go (2)
Customer(2222-2272)CustomerUsageAttribution(2410-2413)openmeter/streaming/query_params.go (2)
Customer(76-78)CustomerUsageAttribution(81-85)pkg/models/errors.go (1)
NewGenericValidationError(138-140)
openmeter/customer/service/hooks/subjectcustomer.go (3)
openmeter/customer/customer.go (4)
GetCustomerByUsageAttributionInput(233-239)UpdateCustomerInput(330-333)CustomerID(147-147)CustomerMutate(112-122)pkg/models/errors.go (1)
IsGenericNotFoundError(57-65)pkg/clock/clock.go (1)
Now(14-21)
test/customer/subject.go (6)
test/customer/customer.go (1)
CustomerHandlerTestSuite(54-58)openmeter/customer/customer.go (7)
Customer(41-53)CreateCustomerInput(312-315)CustomerMutate(112-122)CustomerUsageAttribution(200-202)UpdateCustomerInput(330-333)CustomerID(147-147)GetCustomerInput(351-358)pkg/models/key.go (1)
NamespacedKey(5-8)pkg/models/id.go (1)
NamespacedID(7-10)openmeter/productcatalog/feature/connector.go (1)
CreateFeatureInputs(19-26)openmeter/entitlement/entitlement.go (1)
CreateEntitlementInputs(77-102)
cmd/server/wire_gen.go (1)
app/common/telemetry.go (2)
NewTelemetryServer(261-268)TelemetryServer(259-259)
openmeter/entitlement/metered/events.go (2)
api/api.gen.go (2)
CustomerUsageAttribution(2410-2413)Subject(6898-6932)openmeter/event/metadata/resourcepath.go (2)
ComposeResourcePath(29-31)EntityEntitlement(10-10)
test/customer/testenv.go (3)
openmeter/customer/service/hooks/subjectcustomer.go (2)
NewSubjectCustomerHook(165-186)SubjectCustomerHookConfig(188-196)openmeter/subject/service/hooks/customersubject.go (2)
NewCustomerSubjectHook(81-96)CustomerSubjectHookConfig(98-98)openmeter/billing/service.go (1)
CustomerOverrideService(35-42)
openmeter/entitlement/errors.go (1)
openmeter/ent/db/entitlement/where.go (1)
CustomerID(110-112)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (18)
openmeter/subscription/service/servicevalidation.go (3)
33-38: Currency validation looks good!The currency validation logic is clean and handles the mismatch case appropriately. Nice that it only checks currency when there are billables.
72-78: Currency validation consistent with create flow.Good that the currency validation here mirrors the logic in
validateCreate. Consistency is key!
15-81: Test coverage for removed subject validation is already well-established.The good news: the test suite in
service_test.goalready covers the validation logic comprehensively. I found tests that validate:
- Currency mismatch detection – "Should not allow creating a subscription with different currency compared to the customer" confirms the validation logic works
- Customer currency setting – "Should set customer currency based on subscription" covers the happy path
- Multiple scenarios – creation, cancellation, and update flows all exercise the validation path
Plus, the codebase shows no remaining subject-related validation code in the subscription validation functions. The removal appears complete, and the existing tests adequately cover the remaining currency validation logic that's still in place.
test/customer/testenv.go (1)
159-180: Hook wiring looks good!The cross-service hook setup between customer and subject services is properly implemented with appropriate error handling and registration. The use of noop tracer is perfect for the test environment.
cmd/server/wire_gen.go (1)
550-550: Generated wire code looks consistent.The TelemetryServer wiring has shifted from
v9tov8, which makes sense given the removal of the two validator hooks from the dependency graph. Since this is auto-generated code, no action needed—just confirming it matches the updatedwire.gosource!Also applies to: 602-602
test/notification/consumer_balance.go (2)
15-15: LGTM! Import aligns with the customer-centric refactor.This import is needed for the new Customer field on the entitlement, which fits perfectly with the PR's goal of moving away from direct subject references.
74-84: Verify if this test fixture validation is actually triggered.The original concern about missing
NameandManagedModelfields is structurally valid. I found that ManagedResource.Validate() explicitly checks if Name is empty, returning "name is required" error, and it also validates the ManagedModel timestamps.Your Customer at lines 74-84 has both of these omitted—it'll default to an empty Name string and zero timestamps.
That said, I didn't find any
.Validate()calls on this Entitlement or Customer within the test file itself (only a validation call on BalanceThreshold at line 232). So the real question is: does this code path actually trigger validation? The test may intentionally use minimal fixture setup if validation doesn't run on this particular flow.Worth checking: Does the test pass as-is? If it does, validation likely isn't being called. If it's failing or if you know this fixture gets validated downstream, then adding those fields would be the move.
openmeter/ent/schema/subject.go (1)
66-68: LGTM! Clean removal of the entitlements edge.The change correctly removes the Subject → Entitlement relationship, which aligns with the PR's goal to decouple subjects from entitlements. This is now handled at the Customer level instead.
openmeter/subject/service/service_test.go (1)
201-206: Test correctly validates the new behavior!The test now properly reflects that subjects can be deleted even when the customer has active entitlements. The message clearly documents that entitlements belong to the customer, not the subject. Nice work on updating the test to match the new behavioral contract.
openmeter/customer/service/hooks/subjectcustomer.go (1)
62-133: Great implementation of the PostDelete hook!The cleanup logic looks solid - you're properly handling all the edge cases (customer not found, customer deleted) and updating the usage attribution to remove the deleted subject. The telemetry and error handling are thorough.
openmeter/customer/customer.go (2)
55-68: LGTM! Clean helper method.The
AsCustomerMutatemethod provides a straightforward way to convert a Customer to CustomerMutate, which is useful for updates. All the relevant fields are correctly mapped.
220-230: Nice defensive implementation with deterministic behavior!The method correctly handles the multi-subject case by cloning, sorting, and returning the first key. The error handling for empty subject keys is good. The deprecation notice is clear about backwards compatibility.
One tiny suggestion: you could make the deprecation comment more explicit about what to use instead or when this will be removed, but it's not critical.
openmeter/entitlement/adapter/entitlement.go (2)
318-320: LGTM! Correctly updated to load subjects via customer.The query now loads subjects through the Customer relationship rather than directly, which aligns with the new data model where subjects are accessed via
Customer.UsageAttribution.SubjectKeys.
830-832: Consistent with the earlier change!Same pattern here - subjects are now loaded through the customer edge, maintaining consistency across the codebase.
openmeter/entitlement/adapter/entitlement_test.go (2)
497-498: Good update to use GetFirstSubjectKey!The test correctly adapts to the new API for getting subject keys from customer usage attribution.
505-527: Test assertions properly updated for the new model!The checks now correctly verify that subject keys are present in the customer's usage attribution, which is the right way to validate this relationship post-refactor. Nice use of
Containsto handle the multi-subject case.openmeter/entitlement/errors.go (1)
9-17: LGTM! Error payload correctly updated.The error now carries
CustomerIDinstead ofSubjectKey, which matches the new entitlement model where entitlements are tied to customers rather than subjects directly.openmeter/entitlement/metered/events.go (1)
73-130: Nice v2 event design!The new event structure correctly shifts to a customer-centric model while maintaining the deprecation-based migration path for v1. The validation and metadata generation look solid. Good use of versioning to allow gradual migration.
| var ( | ||
| _ marshaler.Event = EntitlementResetEvent{} | ||
|
|
||
| resetEntitlementEventNameV2 = metadata.GetEventName(metadata.EventType{ | ||
| Subsystem: EventSubsystem, | ||
| Name: "entitlement.reset", | ||
| Version: "v2", | ||
| }) | ||
| ) |
There was a problem hiding this comment.
Fix: Copy-paste error in var declaration.
Line 84 still references the old EntitlementResetEvent type when it should reference EntitlementResetEventV2.
Apply this fix:
var (
- _ marshaler.Event = EntitlementResetEvent{}
+ _ marshaler.Event = EntitlementResetEventV2{}
resetEntitlementEventNameV2 = metadata.GetEventName(metadata.EventType{
Subsystem: EventSubsystem,
Name: "entitlement.reset",
Version: "v2",
})
)📝 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.
| var ( | |
| _ marshaler.Event = EntitlementResetEvent{} | |
| resetEntitlementEventNameV2 = metadata.GetEventName(metadata.EventType{ | |
| Subsystem: EventSubsystem, | |
| Name: "entitlement.reset", | |
| Version: "v2", | |
| }) | |
| ) | |
| var ( | |
| _ marshaler.Event = EntitlementResetEventV2{} | |
| resetEntitlementEventNameV2 = metadata.GetEventName(metadata.EventType{ | |
| Subsystem: EventSubsystem, | |
| Name: "entitlement.reset", | |
| Version: "v2", | |
| }) | |
| ) |
🤖 Prompt for AI Agents
In openmeter/entitlement/metered/events.go around lines 83 to 91, the blank
identifier type assertion incorrectly references EntitlementResetEvent; change
it to EntitlementResetEventV2 so the var block reads the marshaler.Event
implementation check against the V2 type. Update the line to use
EntitlementResetEventV2{} (no other changes).
d728430 to
33e1c2c
Compare
GAlexIHU
requested review from
chrisgacsal,
gergely-kurucz-konghq,
tothandras and
turip
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/customer/testenv.go (1)
220-228: Nil embedded interface in noopCustomerOverrideService still unresolved.This is using
noopCustomerOverrideService{}which has a nil embedded interface (defined at lines 423-425). As noted in the previous review, if any test triggers code that calls methods onCustomerOverride(likeGetCustomerOverrideinCustomerProvisioner.EnsureStripeCustomer), you'll hit a nil pointer panic.Current tests work because they don't set
StripeCustomerId, but the struct needs proper noop implementations to be safe.
🧹 Nitpick comments (11)
openmeter/credit/grant.go (1)
51-51: Nice improvement to error context! 👍Wrapping the error with additional context makes debugging much easier. This follows Go best practices perfectly.
If you're in the mood for a bit more polish, the same treatment could be applied to other error returns in this function (lines 60, 69, 92) and in
VoidGrant(lines 135, 140, 146) for consistency. Totally optional though—this change is already a win!test/customer/testenv.go (1)
279-362: Nice comprehensive subscription setup!The subscription service wiring is thorough and properly handles all the dependencies (repos, adapters, hooks, addons, workflow). Everything looks correctly ordered and configured.
If you find this function getting unwieldy in the future, you could extract some of these service-creation blocks into helper functions like
setupSubscriptionServices()or similar, but that's totally optional at this stage.test/customer/subject.go (5)
31-80: Subject deletion “dangling” case is clear, could assert deletion more stronglyThis flow nicely captures the “dangling after removing from usage attribution” scenario and ensures deletion doesn’t error, which matches the new semantics.
If you want to lock it in a bit more, you could follow the delete with a
GetByKey/GetByIdOrKeycheck and assert it fails with a not‑found error. That would protect against regressions whereDeletebecomes a no‑op.
82-125: Be careful depending on[]string{}vsnilfor subject keysThe assertion
require.Equal(t, []string{}, cust.UsageAttribution.SubjectKeys, "Customer usage attribution subject keys must be empty")is very concrete about the internal representation. If the implementation ever sets
nilinstead of an empty slice, this will fail even though the observable behavior (“no subjects”) is the same.If you don’t need to enforce non‑nil here, you might make this a bit more robust with something like:
require.Empty(t, cust.UsageAttribution.SubjectKeys, "Customer usage attribution subject keys must be empty")or
require.Len(t, cust.UsageAttribution.SubjectKeys, 0, ...).
126-186: Nice coverage for “customer with entitlements loses all subjects”This subtest does a good job of pinning down the behavior that deleting the only subject:
- doesn’t error even when entitlements exist, and
- results in the customer having no remaining subject keys.
If you ever want to go further, you could also assert that the entitlement remains readable / behaves as expected post‑deletion, but as‑is it captures the critical regression surface.
189-213: Tiny nit: this equality check doesn’t assert anything meaningfulHere:
require.Equal(t, met.Key, met.Key, "meter key must match")you’re comparing the value to itself, so the assertion can’t fail.
If you want this to pull its weight, maybe assert against the expected key/slug, or validate another important attribute:
require.Equal(t, "integration-meter", met.Key) require.Equal(t, meter.MeterAggregationSum, met.Aggregation)(or whatever fields are relevant).
325-349: Subtest name vs assertion don’t quite line upThe subtest is called:
t.Run("Should have all subjects created as entitlements", ...)but the body only verifies that the subjects exist, not that entitlements were created for them.
Totally minor, but you might either:
- rename to something like “Should have all subjects created as subjects”, or
- add entitlement checks if the intent really is to assert entitlement creation.
Right now the name is a bit misleading for future readers.
e2e/multisubject_test.go (4)
25-63: Customer + subjects setup matches new multi-subject modelCreating the customer with
UsageAttribution.SubjectKeys(without explicitUpsertSubjectcalls) is a nice direct exercise of the new behavior, and the basic assertions look good.If you ever want to make this a bit stricter, you could assert the returned
SubjectKeysactually matchsubjectKeys(e.g. viaElementsMatch) rather than just length, but the current check is enough for the happy path.
67-185: Plan + feature wiring is thorough and realisticThe feature/plan setup is nicely representative of a real configuration (boolean feature + metered feature with
IssueAfterReset), and the assertions around plan creation/publishing are clear.There’s some overlap with the plan-building logic in
productcataloge2e tests, so if this pattern spreads further you might eventually factor out a helper to reduce duplication, but that’s just a future cleanup idea.
187-205: Subscription creation is minimal but effectiveUsing
SubscriptionTimingEnumImmediateand just asserting a 201 here is enough given the follow‑up tests rely on the resulting entitlements. If you see flakiness down the line, you might consider also asserting basic fields on the created subscription (status, attached plan ID), but not strictly necessary.
220-259: Aggregation & entitlement usage checks are great; a couple of robustness nitsLove this part of the test — it:
- ingests one event per subject,
- asserts the meter aggregates to
len(subjectKeys), and- then checks the customer’s metered entitlement usage also reflects that same aggregated count.
Two small robustness thoughts:
You’re using
context.Background()for ingest and query. Totally fine, but if you want cancellation to propagate from the test (and easier debugging when things hang), you could reusectxinstead.The
QueryMeterassertion assumes this meter is only used here:require.Len(t, resp.JSON200.Data, 1) assert.Equal(t, float64(len(subjectKeys)), resp.JSON200.Data[0].Value)If future tests reuse
multi_subject_meter, this could start to fail. Scoping the query by time range (and/or subject filters if available in the client) would make this more future‑proof.Overall though, this e2e flow is a really nice validation of the new multi‑subject behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
api/client/go/client.gen.gois excluded by!api/client/**api/client/javascript/src/client/schemas.tsis excluded by!api/client/**api/client/javascript/src/zod/index.tsis excluded by!api/client/**api/client/python/openmeter/_generated/models/_models.pyis excluded by!**/_generated/**,!api/client/**api/openapi.cloud.yamlis excluded by!**/openapi.cloud.yamlapi/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (10)
api/spec/src/customer/customer.tsp(1 hunks)e2e/config.yaml(1 hunks)e2e/multisubject_test.go(1 hunks)e2e/productcatalog_test.go(1 hunks)openmeter/credit/grant.go(1 hunks)openmeter/customer/adapter/customer.go(1 hunks)test/customer/customer.go(3 hunks)test/customer/customer_test.go(1 hunks)test/customer/subject.go(1 hunks)test/customer/testenv.go(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/customer/customer.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them.Performance should be a priority in critical code paths. Anything related to event ingestion, message processing, database operations (regardless of database) should be vetted for potential performance bottlenecks.
Files:
openmeter/customer/adapter/customer.gotest/customer/customer_test.goe2e/multisubject_test.gotest/customer/subject.goopenmeter/credit/grant.goe2e/productcatalog_test.gotest/customer/testenv.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests.
When appropriate, recommend e2e tests for critical changes.
Files:
test/customer/customer_test.goe2e/multisubject_test.goe2e/productcatalog_test.go
**/*.tsp
⚙️ CodeRabbit configuration file
**/*.tsp: Review the TypeSpec code for conformity with TypeSpec best practices. When recommending changes also consider the fact that multiple codegeneration toolchains depend on the TypeSpec code, each of which have their idiosyncrasies and bugs.The declared API should be accurate, in parity with the actual implementation, and easy to understand for the user.
Files:
api/spec/src/customer/customer.tsp
🧠 Learnings (3)
📚 Learning: 2025-10-09T13:59:12.012Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 3486
File: openmeter/ingest/kafkaingest/serializer/serializer.go:105-107
Timestamp: 2025-10-09T13:59:12.012Z
Learning: In OpenMeter, the CloudEvents `subject` field is mandatory for the application's business logic, even though it's optional in the CloudEvents specification. The `ValidateKafkaPayloadToCloudEvent` function in `openmeter/ingest/kafkaingest/serializer/serializer.go` intentionally enforces this requirement.
Applied to files:
openmeter/customer/adapter/customer.go
📚 Learning: 2025-03-07T12:17:43.129Z
Learnt from: GAlexIHU
Repo: openmeterio/openmeter PR: 2383
File: openmeter/entitlement/metered/lateevents_test.go:37-45
Timestamp: 2025-03-07T12:17:43.129Z
Learning: In the OpenMeter codebase, test files like `openmeter/entitlement/metered/lateevents_test.go` may use variables like `meterSlug` and `namespace` without explicit declarations visible in the same file. This appears to be an accepted pattern in their test structure.
Applied to files:
e2e/config.yamle2e/multisubject_test.gotest/customer/subject.goe2e/productcatalog_test.gotest/customer/testenv.go
📚 Learning: 2025-08-29T12:31:52.802Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 3291
File: app/common/customer.go:88-89
Timestamp: 2025-08-29T12:31:52.802Z
Learning: In Go projects using Google's wire dependency injection framework, named types (without =) should be used instead of type aliases (with =) to work around wire limitations. For example, use `type CustomerSubjectValidatorHook customerservicehooks.SubjectValidatorHook` instead of `type CustomerSubjectValidatorHook = customerservicehooks.SubjectValidatorHook` when wire is involved.
Applied to files:
test/customer/testenv.go
🧬 Code graph analysis (5)
openmeter/customer/adapter/customer.go (2)
openmeter/ent/schema/customer.go (5)
CustomerSubjects(72-74)CustomerSubjects(76-80)CustomerSubjects(82-100)CustomerSubjects(102-119)CustomerSubjects(121-138)openmeter/customer/errors.go (1)
NewSubjectKeyConflictError(67-71)
test/customer/customer_test.go (1)
test/customer/customer.go (1)
CustomerHandlerTestSuite(54-58)
e2e/multisubject_test.go (1)
api/api.gen.go (30)
CreateCustomerJSONRequestBody(9321-9321)Currency(1954-1966)CurrencyCode(1970-1970)Address(1017-1038)CustomerUsageAttribution(2411-2415)CreateFeatureJSONRequestBody(9345-9345)RateCardEntitlement(6380-6382)RateCardBooleanEntitlement(6369-6373)RateCardBooleanEntitlementType(6376-6376)RateCardMeteredEntitlement(6429-6452)RateCardMeteredEntitlementType(6455-6455)IssueAfterReset(4909-4915)RateCard(6364-6366)RateCardFlatFee(6385-6423)TaxConfig(7554-7566)RateCardFlatFeeType(6426-6426)Price(6311-6313)FlatPriceWithPaymentTerm(3726-3736)PricePaymentTerm(6317-6317)FlatPriceWithPaymentTermType(3739-3739)PlanCreate(6096-6126)Alignment(1041-1046)PlanPhase(6147-6165)SubscriptionTiming(7536-7538)SubscriptionTimingEnumImmediate(820-820)CreateSubscriptionJSONRequestBody(9414-9414)PlanSubscriptionCreate(6248-6280)CustomerId(2346-2349)Plan(5953-6015)PlanReferenceInput(6180-6186)
test/customer/subject.go (13)
test/customer/customer.go (1)
CustomerHandlerTestSuite(54-58)openmeter/customer/customer.go (7)
Customer(41-53)CreateCustomerInput(312-315)CustomerMutate(112-122)CustomerUsageAttribution(200-202)UpdateCustomerInput(330-333)CustomerID(147-147)GetCustomerInput(351-358)api/api.gen.go (14)
Customer(2223-2273)CustomerUsageAttribution(2411-2415)Subject(6939-6973)Feature(3494-3528)Entitlement(2610-2612)EntitlementType(3400-3400)Meter(5022-5071)Plan(5953-6015)Subscription(7001-7058)Currency(1954-1966)IssueAfterReset(4909-4915)Price(6311-6313)FlatPrice(3714-3720)Period(5944-5950)pkg/models/key.go (1)
NamespacedKey(5-8)pkg/models/id.go (1)
NamespacedID(7-10)openmeter/productcatalog/feature/connector.go (1)
CreateFeatureInputs(19-26)openmeter/entitlement/entitlement.go (2)
CreateEntitlementInputs(77-102)EntitlementTypeBoolean(321-321)openmeter/meter/service.go (2)
CreateMeterInput(107-117)GetMeterInput(38-41)pkg/datetime/testutils.go (1)
MustParseDuration(38-45)openmeter/productcatalog/subscription/service/plan.go (1)
PlanFromPlan(76-81)pkg/clock/clock.go (3)
Now(14-21)SetTime(23-27)ResetTime(29-31)openmeter/subscription/workflow/service.go (2)
CreateSubscriptionWorkflowInput(23-30)ChangeSubscriptionWorkflowInput(32-39)openmeter/billing/invoiceline.go (4)
NewFlatFeeLine(473-511)NewFlatFeeLineInput(434-458)WithFeatureKey(466-470)CreatePendingInvoiceLinesInput(701-706)
test/customer/testenv.go (8)
openmeter/meter/adapter/adapter.go (3)
Adapter(46-49)New(33-42)Config(16-19)openmeter/watermill/eventbus/eventbus.go (2)
NewMock(159-172)Publisher(67-79)openmeter/app/service.go (1)
AppService(13-35)openmeter/app/sandbox/mock.go (1)
NewMockableFactory(232-264)openmeter/billing/service.go (1)
CustomerOverrideService(35-42)openmeter/app/appbase.go (1)
AppID(137-140)openmeter/billing/profile.go (4)
CreateProfileInput(329-339)SupplierContact(150-155)CreateProfileAppsInput(365-365)Profile(224-229)pkg/models/model.go (2)
Address(220-228)CountryCode(231-231)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
openmeter/customer/adapter/customer.go (1)
303-328: Nice guard here!This is a clean improvement. By checking that
SubjectKeyshas entries before callingCreateBulk, you're avoiding an unnecessary database round-trip when there are no subjects to create. The error handling inside the guard is correct too—constraint errors only make sense when there are actually keys to conflict.This aligns well with the multi-subject model where subjects aren't always required.
api/spec/src/customer/customer.tsp (3)
270-271: Nice clarity in the docs!The updated wording accurately reflects the new behavior allowing customers without subjects. This aligns well with the PR objectives around multi-subject support and dangling subjects.
277-277: Good call making this explicit!Explicitly documenting that the array can be empty helps developers understand the new behavior at a glance. This kind of clarity prevents confusion down the line.
280-281: The concern about breaking changes isn't supported by the code evidence.The backend already fully supports empty
subjectKeysarrays:
- Explicit test exists (
test/customer/customer.go:187-193): A test case named "customer-no-subjects" already creates customers withSubjectKeys: []string{}- Validation allows it: The
Validate()method doesn't enforce a minimum count onsubjectKeys- Empty handling is intentional: The deprecated
GetFirstSubjectKey()method explicitly handles the empty case and returns a validation error- No unsafe access patterns: Production code iterates and builds
subjectKeyswithout assuming non-empty stateThis PR appears to be aligning the API spec with already-implemented backend behavior—not introducing a new breaking change. The multi-subject model you're enabling was already baked into the backend.
Likely an incorrect or invalid review comment.
test/customer/testenv.go (1)
427-497: Solid test helper functions!The three helper functions are well-designed:
minimalCreateProfileInputTemplate: provides a sensible default billing profile template with immediate collection (PT0S interval documented nicely).installSandboxApp: handles sandbox app creation and retrieval cleanly.createDefaultProfile: convenient wrapper for profile creation with namespace override.These will make writing tests much easier. Nice work!
e2e/productcatalog_test.go (1)
3-8: Switch to stdlibcontextlooks goodUsing
contextfrom the standard library instead ofgolang.org/x/net/contextis the right call and keeps things idiomatic. No behavior change here.test/customer/subject.go (1)
351-407: End‑to‑end flow looks solidThe sandbox app/profile install, subscription creation via the workflow service, clock manipulation, and billing line/invoicing path all hang together nicely. The use of
t.Cleanup(clock.ResetTime)is especially nice for keeping time drift localized to this test.test/customer/customer_test.go (1)
63-75: Nice separation of subject-specific flowsThe extra
"Subject"subtest group that reusesCustomerHandlerTestSuiteforTestSubjectDeletionandTestMultiSubjectIntegrationFlowfits cleanly with the existing pattern and keeps subject scenarios discoverable without mixing them into the core customer tests.e2e/config.yaml (1)
100-103: Multi-subject meter wiring matches the e2e test usageThe new
multi_subject_meterentry (eventTypemulti_subject, aggregationCOUNT) lines up with the constants and queries ine2e/multisubject_test.go. Nothing to tweak here.e2e/multisubject_test.go (1)
207-218: Per-subject entitlement checks nicely validate multi-subject accessLooping over
subjectKeysand verifyingGetEntitlementValueWithResponsereturnsHasAccess == trueis exactly the behavior we care about here. This is a clean way to prove that all subjects under the customer see the same feature access.
GAlexIHU
force-pushed
the
feat/multi-subject/entitlement-subject
branch
from
33cb307 to
2e96131
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
test/customer/testenv.go (1)
423-497: Re‑confirm thatnoopCustomerOverrideServiceis never called.
noopCustomerOverrideServicestill just embedsbilling.CustomerOverrideService:type noopCustomerOverrideService struct { billing.CustomerOverrideService }When you pass this into
SubjectCustomerHookasCustomerOverride, any method call on that interface will panic since the embedded value is nil.Earlier analysis suggested current tests don’t hit those code paths, but with the new billing + subscription + sandbox wiring, it’s easier to accidentally start exercising them.
Two options:
- implement the minimal no‑op methods on this type (especially
GetCustomerOverride), or- make sure
SubjectCustomerHooktreats a nilCustomerOverrideas “feature disabled” and never calls into it in tests.Might be worth giving this a quick sanity check before more tests start relying on it.
openmeter/entitlement/metered/events.go (1)
73-91: Fix the marshaler.Event assertion to target V2.Love the addition of
EntitlementResetEventV2with customer + usage attribution baked in; theEventMetadataandValidatelogic look aligned with the new contract.There’s still a small copy‑paste issue in the var block though:
var ( - _ marshaler.Event = EntitlementResetEvent{} + _ marshaler.Event = EntitlementResetEventV2{} resetEntitlementEventNameV2 = ... )Updating the assertion to use
EntitlementResetEventV2{}will ensure compile‑time coverage that the new event type continues to satisfymarshaler.Event.
🧹 Nitpick comments (7)
openmeter/subject/service/service_test.go (1)
201-206: Test update correctly reflects the new deletion semanticsChanging this to
require.NoErrorfnicely encodes the updated rule that deleting a subject with active entitlements is allowed and doesn’t try to clean up those entitlements. The follow‑up steps that deactivate the entitlement and then delete the customer/subject give good coverage of the full lifecycle. If you ever want clearer failure messaging, you could switch torequire.NoError(t, err, "...")instead of the*fvariant, but that’s purely cosmetic.openmeter/customer/service/hooks/subjectcustomer.go (1)
13-13: PostDelete cleanup logic is solid; consider aligning it withIgnoreErrorsbehaviorThe new
PostDeletehook looks good overall:
- Tracing + span events are helpful for debugging subject‑deletion flows.
- You correctly treat “customer not found” and “already deleted” as no‑ops while still cleaning up usage attribution when there’s a live customer.
- Filtering out the deleted subject key from
UsageAttribution.SubjectKeysmatches the new model where entitlements belong to the customer and subjects can be deleted without breaking things.Two small thoughts:
IgnoreErrorsisn’t applied here
SubjectCustomerHookConfig.IgnoreErrorsis currently only respected via theprovisionhelper used byPostCreate/PostUpdate. InPostDeleteyou always return errors fromGetCustomerByUsageAttributionandUpdateCustomer.
If the intent is “this hook should never block subject operations whenIgnoreErrorsis true,” you might want to mirror the same pattern here (log + span events, but swallow the error whenignoreErrorsis set). If the idea is that delete‑time cleanup must be strict, then this is fine as‑is—but it’s a divergence worth being explicit about.(Optional) Defensive nil‑guard on
sub
Right nowsub.Id/sub.Keyare used unconditionally. If there’s any chance a nil subject could slip through this hook in the future, a quickif sub == nil { return nil }would keep it safe. For the current call sites it’s probably not an issue, so feel free to ignore if you’re confident in the contract.Functionally, though, the new hook behavior looks consistent with the rest of the PR.
Also applies to: 26-27, 62-133
e2e/multisubject_test.go (1)
235-248: Consider using the shared context instead ofcontext.Background()(optional).For the ingest and query calls you’re using
context.Background(), while the rest of the test sharesctxthat’s cancelled at the end ofTestMultiSubject. For consistency and easier cancellation/debugging, you might want to passctxhere as well:- resp, err := client.IngestEventWithResponse(context.Background(), ev) + resp, err := client.IngestEventWithResponse(ctx, ev) ... - resp, err := client.QueryMeterWithResponse(context.Background(), MultiSubjectMeterSlug, nil) + resp, err := client.QueryMeterWithResponse(ctx, MultiSubjectMeterSlug, nil)Not a blocker, just a small polish for consistency.
openmeter/entitlement/adapter/entitlement.go (2)
318-338: Be mindful of the extra join cost inListEntitlements.Switching
ListEntitlementsto alwaysWithCustomer+WithSubjectsis great for correctness (it guaranteesent.Customeris loaded for mapping), but it does add an extra join (and subject join) to what might be a hot listing path.If most callers don’t actually need
Customer/SubjectKeysfrom the returned entitlements, it could be worth considering:
- a lighter code path (or flag) that skips the customer/subject join, vs.
- this “fully hydrated” path for callers that really need it.
Not a blocker, just something to keep in mind if this list becomes a bottleneck.
611-623: DropWithCustomerhere to avoid unnecessary work.In
UpsertEntitlementCurrentPeriods, you now query entitlements like this:entitlements, err := repo.db.Entitlement.Query(). Where(db_entitlement.IDIn(...)). WithCustomer(func(q *db.CustomerQuery) { customeradapter.WithSubjects(q, now) }). All(ctx)But the rest of the function never uses the customer or subject edges—only core entitlement fields (
Namespace,FeatureID, etc.) to build upserts.You can safely remove the
WithCustomer(...)call here to reduce query cost, especially since this function is designed to batch‑update a potentially large set of entitlements.test/customer/subject.go (2)
31-187: Subject deletion tests nicely pin down the new behavior (small naming nit).These three subtests do a great job of codifying the new semantics:
- dangling subjects can be deleted once removed from usage attribution
- deleting a non‑dangling subject automatically cleans it from the customer
- even when entitlements exist, subject deletion doesn’t blow up and leaves the customer with no subject keys
One small nit: in the last subtest you name the local entitlement variable
entitlement, which temporarily shadows the importedentitlementpackage on that line:entitlement, err := s.Env.Entitlement().CreateEntitlement(...)It’s harmless, but renaming the variable (e.g. to
entorcreatedEnt) would make the code a bit clearer and avoid the shadowing.
189-407: End‑to‑end multi‑subject flow test looks great (with one tiny assertion nit).This integration test is super valuable:
- spins up a real meter, two features, and a plan with both usage‑based and flat‑fee rate cards
- creates a customer with multiple subject keys and verifies subjects are actually created
- goes through sandbox app + billing profile setup, subscription workflow creation, time travel, pending line creation, and invoicing
That’s a really nice way to validate the new multi‑subject wiring under realistic conditions.
Only tiny nit I’d tweak:
require.Equal(t, met.Key, met.Key, "meter key must match")This assertion is tautologically true; if you meant to verify the key from a prior variable, compare against that instead, or just drop the line.
Otherwise, this test is spot‑on for catching regressions in the multi‑subject + billing pipeline.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (24)
api/client/go/client.gen.gois excluded by!api/client/**api/client/javascript/src/client/schemas.tsis excluded by!api/client/**api/client/javascript/src/zod/index.tsis excluded by!api/client/**api/client/python/openmeter/_generated/models/_models.pyis excluded by!**/_generated/**,!api/client/**api/openapi.cloud.yamlis excluded by!**/openapi.cloud.yamlapi/openapi.yamlis excluded by!**/openapi.yamlgo.sumis excluded by!**/*.sum,!**/*.sumopenmeter/ent/db/client.gois excluded by!**/ent/db/**openmeter/ent/db/entitlement.gois excluded by!**/ent/db/**openmeter/ent/db/entitlement/entitlement.gois excluded by!**/ent/db/**openmeter/ent/db/entitlement/where.gois excluded by!**/ent/db/**openmeter/ent/db/entitlement_create.gois excluded by!**/ent/db/**openmeter/ent/db/entitlement_query.gois excluded by!**/ent/db/**openmeter/ent/db/entitlement_update.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**openmeter/ent/db/subject.gois excluded by!**/ent/db/**openmeter/ent/db/subject/subject.gois excluded by!**/ent/db/**openmeter/ent/db/subject/where.gois excluded by!**/ent/db/**openmeter/ent/db/subject_create.gois excluded by!**/ent/db/**openmeter/ent/db/subject_query.gois excluded by!**/ent/db/**openmeter/ent/db/subject_update.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (44)
api/spec/src/customer/customer.tsp(1 hunks)app/common/customer.go(0 hunks)app/common/subject.go(0 hunks)cmd/server/wire.go(0 hunks)cmd/server/wire_gen.go(2 hunks)e2e/config.yaml(1 hunks)e2e/multisubject_test.go(1 hunks)e2e/productcatalog_test.go(1 hunks)openmeter/credit/grant.go(1 hunks)openmeter/customer/adapter/customer.go(1 hunks)openmeter/customer/customer.go(3 hunks)openmeter/customer/errors.go(0 hunks)openmeter/customer/service/hooks/subjectcustomer.go(3 hunks)openmeter/customer/service/hooks/subjectvalidator.go(0 hunks)openmeter/customer/service/hooks/subjectvalidator_test.go(0 hunks)openmeter/ent/schema/entitlement.go(0 hunks)openmeter/ent/schema/subject.go(1 hunks)openmeter/entitlement/adapter/entitlement.go(2 hunks)openmeter/entitlement/adapter/entitlement_test.go(2 hunks)openmeter/entitlement/balanceworker/subject_customer.go(1 hunks)openmeter/entitlement/balanceworker/worker.go(1 hunks)openmeter/entitlement/driver/parser.go(6 hunks)openmeter/entitlement/entitlement.go(2 hunks)openmeter/entitlement/errors.go(1 hunks)openmeter/entitlement/events.go(0 hunks)openmeter/entitlement/metered/events.go(3 hunks)openmeter/entitlement/metered/reset.go(1 hunks)openmeter/entitlement/service/scheduling.go(1 hunks)openmeter/notification/consumer/entitlementbalancethreshold_test.go(0 hunks)openmeter/subject/adapter/subject.go(0 hunks)openmeter/subject/service/hooks/entitlementvalidator.go(0 hunks)openmeter/subject/service/service_test.go(1 hunks)openmeter/subscription/service/servicevalidation.go(1 hunks)openmeter/subscription/testutils/service.go(0 hunks)test/billing/suite.go(0 hunks)test/customer/customer.go(3 hunks)test/customer/customer_test.go(1 hunks)test/customer/subject.go(1 hunks)test/customer/testenv.go(8 hunks)test/entitlement/regression/scenario_test.go(1 hunks)test/notification/consumer_balance.go(2 hunks)test/notification/testenv.go(1 hunks)tools/migrate/migrations/20251110132128_remove-subject-from-entitlement.down.sql(1 hunks)tools/migrate/migrations/20251110132128_remove-subject-from-entitlement.up.sql(1 hunks)
💤 Files with no reviewable changes (13)
- openmeter/notification/consumer/entitlementbalancethreshold_test.go
- app/common/customer.go
- openmeter/subscription/testutils/service.go
- openmeter/subject/adapter/subject.go
- cmd/server/wire.go
- app/common/subject.go
- openmeter/entitlement/events.go
- openmeter/customer/service/hooks/subjectvalidator_test.go
- openmeter/subject/service/hooks/entitlementvalidator.go
- openmeter/customer/errors.go
- test/billing/suite.go
- openmeter/ent/schema/entitlement.go
- openmeter/customer/service/hooks/subjectvalidator.go
🚧 Files skipped from review as they are similar to previous changes (12)
- openmeter/entitlement/driver/parser.go
- openmeter/customer/adapter/customer.go
- openmeter/entitlement/errors.go
- test/entitlement/regression/scenario_test.go
- e2e/config.yaml
- openmeter/ent/schema/subject.go
- tools/migrate/migrations/20251110132128_remove-subject-from-entitlement.up.sql
- openmeter/credit/grant.go
- test/customer/customer.go
- test/notification/testenv.go
- api/spec/src/customer/customer.tsp
- tools/migrate/migrations/20251110132128_remove-subject-from-entitlement.down.sql
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them.Performance should be a priority in critical code paths. Anything related to event ingestion, message processing, database operations (regardless of database) should be vetted for potential performance bottlenecks.
Files:
test/notification/consumer_balance.goe2e/productcatalog_test.goopenmeter/entitlement/adapter/entitlement.goopenmeter/entitlement/metered/reset.goopenmeter/subscription/service/servicevalidation.goopenmeter/customer/customer.goopenmeter/subject/service/service_test.goopenmeter/customer/service/hooks/subjectcustomer.gotest/customer/subject.goopenmeter/entitlement/metered/events.gotest/customer/testenv.gocmd/server/wire_gen.goopenmeter/entitlement/service/scheduling.goopenmeter/entitlement/entitlement.goopenmeter/entitlement/balanceworker/subject_customer.gotest/customer/customer_test.goe2e/multisubject_test.goopenmeter/entitlement/balanceworker/worker.goopenmeter/entitlement/adapter/entitlement_test.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests.
When appropriate, recommend e2e tests for critical changes.
Files:
e2e/productcatalog_test.goopenmeter/subject/service/service_test.gotest/customer/customer_test.goe2e/multisubject_test.goopenmeter/entitlement/adapter/entitlement_test.go
🧠 Learnings (4)
📚 Learning: 2025-03-07T12:17:43.129Z
Learnt from: GAlexIHU
Repo: openmeterio/openmeter PR: 2383
File: openmeter/entitlement/metered/lateevents_test.go:37-45
Timestamp: 2025-03-07T12:17:43.129Z
Learning: In the OpenMeter codebase, test files like `openmeter/entitlement/metered/lateevents_test.go` may use variables like `meterSlug` and `namespace` without explicit declarations visible in the same file. This appears to be an accepted pattern in their test structure.
Applied to files:
test/notification/consumer_balance.goe2e/productcatalog_test.goopenmeter/entitlement/adapter/entitlement.goopenmeter/entitlement/metered/reset.goopenmeter/subject/service/service_test.goopenmeter/entitlement/metered/events.gotest/customer/testenv.goe2e/multisubject_test.goopenmeter/entitlement/adapter/entitlement_test.go
📚 Learning: 2025-10-09T13:59:12.012Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 3486
File: openmeter/ingest/kafkaingest/serializer/serializer.go:105-107
Timestamp: 2025-10-09T13:59:12.012Z
Learning: In OpenMeter, the CloudEvents `subject` field is mandatory for the application's business logic, even though it's optional in the CloudEvents specification. The `ValidateKafkaPayloadToCloudEvent` function in `openmeter/ingest/kafkaingest/serializer/serializer.go` intentionally enforces this requirement.
Applied to files:
openmeter/entitlement/metered/reset.goopenmeter/subscription/service/servicevalidation.goopenmeter/entitlement/metered/events.goopenmeter/entitlement/entitlement.go
📚 Learning: 2025-08-29T12:31:52.802Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 3291
File: app/common/customer.go:88-89
Timestamp: 2025-08-29T12:31:52.802Z
Learning: In Go projects using Google's wire dependency injection framework, named types (without =) should be used instead of type aliases (with =) to work around wire limitations. For example, use `type CustomerSubjectValidatorHook customerservicehooks.SubjectValidatorHook` instead of `type CustomerSubjectValidatorHook = customerservicehooks.SubjectValidatorHook` when wire is involved.
Applied to files:
test/customer/testenv.gocmd/server/wire_gen.go
📚 Learning: 2025-09-12T09:38:52.436Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 3373
File: openmeter/subject/adapter/subject.go:119-136
Timestamp: 2025-09-12T09:38:52.436Z
Learning: In OpenMeter subject adapter GetByIdOrKey method, ID-based lookups should return subjects even if soft-deleted, while Key-based lookups should be gated by DeletedAt filters. This is intentional design where IDs are treated as immutable references that can retrieve deleted entities, but Keys should only match active (non-deleted) subjects.
Applied to files:
openmeter/entitlement/adapter/entitlement_test.go
🧬 Code graph analysis (14)
test/notification/consumer_balance.go (3)
api/api.gen.go (2)
Customer(2223-2273)CustomerUsageAttribution(2411-2415)pkg/models/model.go (2)
ManagedResource(23-31)NamespacedModel(204-206)test/notification/testenv.go (2)
TestCustomerID(40-40)TestSubjectKey(38-38)
openmeter/entitlement/adapter/entitlement.go (3)
openmeter/entitlement/entitlement.go (1)
Entitlement(232-246)openmeter/ent/schema/entitlement.go (5)
Entitlement(20-22)Entitlement(24-31)Entitlement(33-71)Entitlement(73-84)Entitlement(86-111)openmeter/entitlement/metered/entitlement.go (1)
Entitlement(22-50)
openmeter/entitlement/metered/reset.go (2)
openmeter/entitlement/metered/events.go (1)
EntitlementResetEventV2(73-81)api/api.gen.go (2)
Customer(2223-2273)CustomerUsageAttribution(2411-2415)
openmeter/customer/customer.go (3)
api/api.gen.go (2)
Customer(2223-2273)CustomerUsageAttribution(2411-2415)openmeter/streaming/query_params.go (2)
Customer(76-78)CustomerUsageAttribution(81-85)pkg/models/errors.go (1)
NewGenericValidationError(138-140)
openmeter/customer/service/hooks/subjectcustomer.go (3)
openmeter/customer/customer.go (4)
GetCustomerByUsageAttributionInput(233-239)UpdateCustomerInput(330-333)CustomerID(147-147)CustomerMutate(112-122)pkg/models/errors.go (1)
IsGenericNotFoundError(57-65)pkg/clock/clock.go (1)
Now(14-21)
test/customer/subject.go (13)
test/customer/customer.go (1)
CustomerHandlerTestSuite(54-58)openmeter/customer/customer.go (6)
Customer(41-53)CreateCustomerInput(312-315)CustomerMutate(112-122)CustomerUsageAttribution(200-202)UpdateCustomerInput(330-333)CustomerID(147-147)pkg/models/key.go (1)
NamespacedKey(5-8)pkg/models/id.go (1)
NamespacedID(7-10)openmeter/productcatalog/feature/connector.go (1)
CreateFeatureInputs(19-26)openmeter/meter/service.go (2)
CreateMeterInput(107-117)GetMeterInput(38-41)openmeter/productcatalog/plan/service.go (1)
CreatePlanInput(105-110)pkg/datetime/testutils.go (1)
MustParseDuration(38-45)openmeter/productcatalog/subscription/service/plan.go (1)
PlanFromPlan(76-81)pkg/clock/clock.go (3)
Now(14-21)SetTime(23-27)ResetTime(29-31)openmeter/subscription/workflow/service.go (2)
CreateSubscriptionWorkflowInput(23-30)ChangeSubscriptionWorkflowInput(32-39)openmeter/billing/invoiceline.go (4)
NewFlatFeeLine(473-511)NewFlatFeeLineInput(434-458)WithFeatureKey(466-470)CreatePendingInvoiceLinesInput(701-706)openmeter/billing/invoice.go (1)
InvoicePendingLinesInput(920-929)
openmeter/entitlement/metered/events.go (2)
api/api.gen.go (2)
CustomerUsageAttribution(2411-2415)Subject(6942-6976)openmeter/event/metadata/resourcepath.go (2)
ComposeResourcePath(29-31)EntityEntitlement(10-10)
test/customer/testenv.go (5)
openmeter/meter/adapter/adapter.go (3)
Adapter(46-49)New(33-42)Config(16-19)openmeter/watermill/eventbus/eventbus.go (2)
NewMock(159-172)Publisher(67-79)openmeter/app/service.go (1)
AppService(13-35)openmeter/billing/profile.go (8)
CreateProfileInput(329-339)InvoicingConfig(78-84)PaymentConfig(120-122)CollectionMethod(134-134)CollectionMethodChargeAutomatically(138-138)SupplierContact(150-155)CreateProfileAppsInput(365-365)Profile(224-229)pkg/models/model.go (2)
Address(220-228)CountryCode(231-231)
cmd/server/wire_gen.go (1)
app/common/telemetry.go (2)
NewTelemetryServer(261-268)TelemetryServer(259-259)
openmeter/entitlement/service/scheduling.go (2)
openmeter/entitlement/errors.go (1)
AlreadyExistsError(9-13)openmeter/customer/customer.go (1)
CustomerID(147-147)
openmeter/entitlement/entitlement.go (3)
app/common/customer.go (1)
Customer(23-25)api/api.gen.go (1)
Customer(2223-2273)openmeter/ent/schema/customer.go (5)
Customer(17-19)Customer(21-29)Customer(31-39)Customer(41-54)Customer(56-69)
test/customer/customer_test.go (1)
test/customer/customer.go (1)
CustomerHandlerTestSuite(54-58)
e2e/multisubject_test.go (1)
api/api.gen.go (30)
CreateCustomerJSONRequestBody(9324-9324)Currency(1954-1966)CurrencyCode(1970-1970)Address(1017-1038)CustomerUsageAttribution(2411-2415)CreateFeatureJSONRequestBody(9348-9348)RateCardEntitlement(6383-6385)RateCardBooleanEntitlement(6372-6376)RateCardBooleanEntitlementType(6379-6379)RateCardMeteredEntitlement(6432-6455)RateCardMeteredEntitlementType(6458-6458)IssueAfterReset(4912-4918)RateCard(6367-6369)RateCardFlatFee(6388-6426)TaxConfig(7557-7569)RateCardFlatFeeType(6429-6429)Price(6314-6316)FlatPriceWithPaymentTerm(3729-3739)PricePaymentTerm(6320-6320)FlatPriceWithPaymentTermType(3742-3742)PlanCreate(6099-6129)Alignment(1041-1046)PlanPhase(6150-6168)SubscriptionTiming(7539-7541)SubscriptionTimingEnumImmediate(820-820)CreateSubscriptionJSONRequestBody(9417-9417)PlanSubscriptionCreate(6251-6283)CustomerId(2346-2349)Plan(5956-6018)PlanReferenceInput(6183-6189)
openmeter/entitlement/balanceworker/worker.go (6)
openmeter/watermill/grouphandler/grouphandler.go (1)
NewGroupEventHandler(29-31)openmeter/entitlement/metered/events.go (1)
EntitlementResetEventV2(73-81)pkg/models/id.go (1)
NamespacedID(7-10)openmeter/entitlement/balanceworker/entitlementhandler.go (3)
WithSource(49-53)WithEventAt(55-59)WithSourceOperation(61-65)openmeter/event/metadata/resourcepath.go (2)
ComposeResourcePath(29-31)EntityEntitlement(10-10)openmeter/entitlement/snapshot/event.go (1)
ValueOperationReset(22-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Artifacts / Container image
- GitHub Check: Lint
- GitHub Check: Migration Checks
- GitHub Check: Test
- GitHub Check: Code Generators
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (15)
openmeter/entitlement/entitlement.go (1)
356-356: Customer pointer + validation line up with the new customer‑centric modelHaving
Customer *customer.CustomeronGenericPropertiesand enforcing it as non‑nil inValidatematches howEntitlement.AsCreateEntitlementInputsalready assumes a populated customer (viae.Customer.GetUsageAttribution()). As long as callers construct entitlements through the existing adapters that load the customer edge, this nicely encodes the invariant instead of relying on convention. I’m happy with this as‑is.Also applies to: 364-369
e2e/productcatalog_test.go (1)
4-4: Switch to stdlibcontextlooks goodNice cleanup moving to the standard
contextpackage; usages below are already compatible, so this is a no‑risk, more idiomatic import.openmeter/subscription/service/servicevalidation.go (1)
33-38: Leaned‑out validation matches the new subject semanticsWith the subject‑level checks removed elsewhere, keeping
validateCreatefocused on spec validation, state‑machine rules, and the currency consistency check feels right and matches the “subjects can be deleted / may be dangling” behavior described in the PR. I don’t see any extra validation needed at this layer.test/customer/customer_test.go (1)
63-75: Nice split of subject-specific tests into their own sub-suite.The new
Subjectgroup follows the existing pattern, reuses the same env, and keeps the customer tests organized. Looks good to me.openmeter/entitlement/balanceworker/subject_customer.go (1)
30-33: Update toGetFirstSubjectKeylooks correct; just double‑check semantics.Switching to
cus.UsageAttribution.GetFirstSubjectKey()matches the new multi-subject API and keeps the existing error handling intact.Given this worker now effectively picks a single (first) subject for the customer, it’d be good to confirm that:
UsageAttributionis always set for customers that reach this path, and- Using the first subject is the intended behavior when there are multiple subject keys.
If both hold, this change is spot on.
openmeter/entitlement/service/scheduling.go (1)
89-100: AlreadyExistsError payload now usingCustomerIDlooks aligned with the new model.Using
CustomerID: conflict.Customer.IDmatches the updatedAlreadyExistsErrorstruct and keeps the error focused on customer–feature uniqueness instead of subject-level uniqueness. This is consistent with how you later compareinput.UsageAttribution.IDwitholdEnt.Customer.IDin the same file.As long as
Customeris always populated on entitlements (which this code already assumes elsewhere), this change is good.openmeter/entitlement/balanceworker/worker.go (1)
287-298: V2 reset handler wiring looks consistent with existing entitlement event handling.The new
EntitlementResetEventV2handler plugs nicely into the balance worker:
- It uses the same
handleEntitlementEvent+PublishIfNoErrorpattern as theRecalculateEventhandler below.- Resource path and
ValueOperationResetare aligned with how v1 resets are ultimately mapped in the snapshot layer.- Using the namespace + entitlement ID pair keeps the NamespacedID consistent.
Only thing to keep in mind: v1 reset still goes through a
RecalculateEventindirection, while v2 now callshandleEntitlementEventdirectly. That’s fine if the intent is to simplify the new path, but worth double-checking that both paths produce equivalent snapshot behavior.openmeter/entitlement/adapter/entitlement_test.go (2)
239-253: Nice extra assertion on entitlement type.The additional
EntitlementTypeequality check is a good guardrail to ensure the upsert truly only touchesCurrentUsagePeriodand doesn’t silently mutate type. 👍
497-526: Good coverage of customer + subject preservation across mappings.Using
GetFirstSubjectKey()and asserting that the subject key survives throughGetEntitlementand the typed metered/static/boolean parsers is a solid end‑to‑end check of the new customer-centric model. This should catch regressions if edge loading or mapping changes later.test/notification/consumer_balance.go (1)
14-27: Snapshot test wiring matches the new customer‑centric entitlement shape.Populating
GenericProperties.Customerwith a minimalCustomer(ID, namespace, andUsageAttribution.SubjectKeys) is exactly what these notification tests need now that entitlements carry customer instead of a bare subject key. The rest of the test logic doesn’t have to change, which is nice.openmeter/customer/customer.go (2)
55-68: AsCustomerMutate helper looks good.The helper cleanly mirrors the mutable fields from
CustomerintoCustomerMutateand should simplify update flows (like the tests tweaking usage attribution). Just be aware the pointer fields (e.g.Metadata,Annotation) are shared, which is expected for this kind of DTO-style conversion.
220-230: The newGetFirstSubjectKeysemantics look solid and callers are happy with it.Verified the behavior against all 5 actual call sites:
- parser.go (3 calls at lines 34, 76, 108): All gracefully handle the error case by falling back to an empty string—they don't assume single-key semantics.
- subject_customer.go (line 30): Treats a key as required but passes it through to
GetByKey()for subject lookup—lexicographic determinism is totally fine here.- Data model: The codebase actively filters
SubjectKeyslists, confirming multiple keys are realistic, not edge cases.The lexicographic ordering gives you a deterministic, stable tie-breaker, which is exactly what you need during the API transition period. No caller breaks with this behavior.
openmeter/entitlement/adapter/entitlement.go (1)
823-874: Scheduled entitlements query wiring looks consistent.Adding
WithCustomer+WithSubjectsto theGetScheduledEntitlementsquery lines up nicely withmapEntitlementEntity’s expectations and the broader shift to customer/usage‑attribution‑aware entitlements. The ordering and filters stay the same, so behavior should remain stable.test/customer/testenv.go (1)
65-421: Test env wiring is ambitious but looks coherent.Nice job turning this into a mini “stack”:
- meter adapter + service
- entitlement registry with MeterService + publisher
- subject/customer hooks registered both ways
- subscription + workflow + addons + plan services
- billing + app + sandbox wiring
That gives the new multi‑subject tests a lot of realistic surface area to exercise without forcing external dependencies, and the
TestEnvinterface stays tidy. I don’t see any obvious lifecycle or setup issues here beyond what’s already been flagged for the noop override service below.cmd/server/wire_gen.go (1)
561-561: LGTM! Wire regeneration looks good.The variable renumbering (v9 → v8) is a mechanical change from Wire regenerating after the validator hooks were removed. The cleanup chain and field assignments are all correctly sequenced.
Also applies to: 613-613
| BillingAddress: &api.Address{ | ||
| City: lo.ToPtr("City"), | ||
| Country: lo.ToPtr("US"), | ||
| Line1: lo.ToPtr("Line 1"), | ||
| Line2: lo.ToPtr("Line 2"), | ||
| State: lo.ToPtr("State"), | ||
| PhoneNumber: lo.ToPtr("1234567890"), | ||
| PostalCode: lo.ToPtr("12345"), | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Fix Country field type: lo.ToPtr("US") doesn’t match *CountryCode.
Right now BillingAddress.Country expects *api.CountryCode, but lo.ToPtr("US") infers *string, which won’t compile.
You can make this type-safe by wrapping the constant in api.CountryCode:
- BillingAddress: &api.Address{
- City: lo.ToPtr("City"),
- Country: lo.ToPtr("US"),
+ BillingAddress: &api.Address{
+ City: lo.ToPtr("City"),
+ Country: lo.ToPtr(api.CountryCode("US")),(or create a local country := api.CountryCode("US") and pass &country).
I'll verify the type mismatch claim by examining the actual code and type definitions.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and examine the multisubject_test.go file
fd -t f "multisubject_test.go" --exec cat {}
Length of output: 8513
🏁 Script executed:
#!/bin/bash
# Search for api.Address type definition and Country field
rg -n -A10 "type Address struct" --type goLength of output: 1315
🏁 Script executed:
#!/bin/bash
# Search for CountryCode type definition
rg -n "type CountryCode" --type goLength of output: 93
Country field type mismatch: lo.ToPtr("US") returns *string, but the field expects *CountryCode.
Line 42 won't compile as-is. Since CountryCode is a type alias for string, the pointer types don't match (*string ≠ *CountryCode).
Wrap the constant to fix it:
- Country: lo.ToPtr("US"),
+ Country: lo.ToPtr(api.CountryCode("US")),📝 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.
| BillingAddress: &api.Address{ | |
| City: lo.ToPtr("City"), | |
| Country: lo.ToPtr("US"), | |
| Line1: lo.ToPtr("Line 1"), | |
| Line2: lo.ToPtr("Line 2"), | |
| State: lo.ToPtr("State"), | |
| PhoneNumber: lo.ToPtr("1234567890"), | |
| PostalCode: lo.ToPtr("12345"), | |
| }, | |
| BillingAddress: &api.Address{ | |
| City: lo.ToPtr("City"), | |
| Country: lo.ToPtr(api.CountryCode("US")), | |
| Line1: lo.ToPtr("Line 1"), | |
| Line2: lo.ToPtr("Line 2"), | |
| State: lo.ToPtr("State"), | |
| PhoneNumber: lo.ToPtr("1234567890"), | |
| PostalCode: lo.ToPtr("12345"), | |
| }, |
🤖 Prompt for AI Agents
In e2e/multisubject_test.go around lines 41 to 49, the BillingAddress.Country is
being set with lo.ToPtr("US") which returns *string but the field type is
*api.CountryCode; change the call to produce a *api.CountryCode, e.g. wrap the
literal with the CountryCode conversion before taking the pointer (use
lo.ToPtr(api.CountryCode("US"))) so the pointer types match.
| assert.Equal(t, MultiSubjectPlanKey, plan.Key) | ||
| require.NotNil(t, plan.Version) | ||
| assert.Equal(t, 1, plan.Version) |
There was a problem hiding this comment.
🧩 Analysis chain
Dereference plan.Version when asserting its value.
plan.Version is a *int, so assert.Equal(t, 1, plan.Version) compares an int to a *int and will always fail the assertion.
Suggest:
- require.NotNil(t, plan.Version)
- assert.Equal(t, 1, plan.Version)
+ require.NotNil(t, plan.Version)
+ assert.Equal(t, 1, *plan.Version)I'll verify this type assertion issue by checking the actual code and type definition.
---
🏁 Script executed:
```shell
#!/bin/bash
# Check the actual code at lines 170-172
sed -n '165,175p' e2e/multisubject_test.go
Length of output: 324
🏁 Script executed:
#!/bin/bash
# Find the plan variable type definition and plan.Version field
rg -n -B5 -A5 'plan\s*:=' e2e/multisubject_test.go | head -50Length of output: 410
🏁 Script executed:
#!/bin/bash
# Search for the type definition of the plan variable or struct with Version field
rg -n 'Version\s*\*int' --type goLength of output: 297
Dereference plan.Version when asserting its value.
plan.Version is a *int pointer, so assert.Equal(t, 1, plan.Version) compares an int to a pointer type and will always fail. The dereference is safe here since the previous line confirms it's not nil.
- require.NotNil(t, plan.Version)
- assert.Equal(t, 1, plan.Version)
+ require.NotNil(t, plan.Version)
+ assert.Equal(t, 1, *plan.Version)📝 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.
| assert.Equal(t, MultiSubjectPlanKey, plan.Key) | |
| require.NotNil(t, plan.Version) | |
| assert.Equal(t, 1, plan.Version) | |
| assert.Equal(t, MultiSubjectPlanKey, plan.Key) | |
| require.NotNil(t, plan.Version) | |
| assert.Equal(t, 1, *plan.Version) |
🤖 Prompt for AI Agents
In e2e/multisubject_test.go around lines 170 to 172, the test compares an int to
a *int pointer when asserting plan.Version which will always fail; since the
previous line ensures plan.Version is not nil, change the assertion to compare
the int to the dereferenced pointer (use *plan.Version) so the types match and
the value comparison is correct.
| event := EntitlementResetEventV2{ | ||
| EntitlementID: entitlementID.ID, | ||
| Namespace: eventmodels.NamespaceID{ | ||
| ID: entitlementID.Namespace, | ||
| }, | ||
| Subject: subject.SubjectKey{ | ||
| Key: ent.SubjectKey, | ||
| }, | ||
| ResetAt: params.At, | ||
| RetainAnchor: params.RetainAnchor, | ||
| ResetRequestedAt: time.Now(), | ||
| CustomerID: ent.Customer.ID, | ||
| CustomerUsageAttribution: ent.Customer.UsageAttribution, | ||
| ResetAt: params.At, | ||
| RetainAnchor: params.RetainAnchor, | ||
| ResetRequestedAt: time.Now(), | ||
| } |
There was a problem hiding this comment.
Guard against ent.Customer being nil when building the reset event.
Here you now rely on ent.Customer being populated:
CustomerID: ent.Customer.ID,
CustomerUsageAttribution: ent.Customer.UsageAttribution,If for any reason the adapter mapping fails to attach a customer (or future changes call ResetEntitlementUsage on an entitlement without a loaded customer), this will panic with a nil pointer deref.
I’d add a defensive check and fail fast with a clear error instead of panicking, e.g.:
- ent, err := e.entitlementRepo.GetEntitlement(ctx, entitlementID)
+ ent, err := e.entitlementRepo.GetEntitlement(ctx, entitlementID)
if err != nil {
return nil, fmt.Errorf("failed to get entitlement: %w", err)
}
+
+ if ent.Customer == nil {
+ return nil, fmt.Errorf("entitlement %s has no customer loaded, cannot emit reset event", entitlementID.ID)
+ }Optionally, you might also consider using clock.Now() instead of time.Now() for ResetRequestedAt so tests can fully control timing, but that’s more of a nice‑to‑have.
2 participants
Overview
Removes anything subject related from entitlements
db.Subjecttodb.CustomerSubjectwhich assumes our database is consistent(currently those two aren't hard linked in the db. if we want to be paranoid we can preemptively hard-link the two)
Changes how customers - subjects - entitlements interact, so its more in line with how it will work after multi-subjects. For the functional spec see
/tests/customer/subject, but TLDRSummary by CodeRabbit
Improvements
New Features
Bug Fixes
Tests