feat(customers): list filter by plan id #3730
Conversation
📝 WalkthroughWalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP as HTTP Driver
participant Service
participant Adapter
participant DB as Database
Client->>HTTP: GET /customers?planId=<ULID>
HTTP->>Service: ListCustomersRequest{PlanID:<ULID>, ...}
Service->>Adapter: ListCustomersInput{PlanID, PlanKey, Expands, ...}
Adapter->>Adapter: build activeSubscriptionFilterPredicates(now)
alt PlanID provided
Adapter->>Adapter: append predicate: subscription.ID == PlanID
else PlanKey provided
Adapter->>Adapter: append predicate: subscription.PlanKey == PlanKey
end
Adapter->>DB: query customers with HasSubscriptionWith(predicate set)
DB-->>Adapter: matching customers (+ optional expanded subscriptions)
Adapter-->>Service: result set
Service-->>HTTP: formatted response
HTTP-->>Client: 200 OK with customers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
test/customer/customer.go (1)
558-625: Good test coverage for the new feature!The test covers the key scenarios: filtering by plan ID, filtering by plan key, unknown plan key returning empty results, and the mutual exclusivity validation.
A couple of small nits:
- Line 563:
cServiceis redundant – you already haveservicefrom line 562.- Line 566: Typo
testCutomer→testCustomer.🔎 Suggested cleanup
func (s *CustomerHandlerTestSuite) TestListWithSubscription(ctx context.Context, t *testing.T) { s.setupNamespace(t) service := s.Env.Customer() - cService := s.Env.Customer() // Create a customer with mandatory fields - testCutomer, err := cService.CreateCustomer(ctx, customer.CreateCustomerInput{ + testCustomer, err := service.CreateCustomer(ctx, customer.CreateCustomerInput{ Namespace: s.namespace, CustomerMutate: customer.CustomerMutate{ Name: TestName, UsageAttribution: &customer.CustomerUsageAttribution{ SubjectKeys: TestSubjectKeys, }, }, }) require.NoError(t, err, "Creating customer must not return error") - require.NotNil(t, testCutomer, "Customer must not be nil") - require.Equal(t, TestName, testCutomer.Name, "Customer name must match") - require.Equal(t, TestSubjectKeys, testCutomer.UsageAttribution.SubjectKeys, "Customer usage attribution subject keys must match") + require.NotNil(t, testCustomer, "Customer must not be nil") + require.Equal(t, TestName, testCustomer.Name, "Customer name must match") + require.Equal(t, TestSubjectKeys, testCustomer.UsageAttribution.SubjectKeys, "Customer usage attribution subject keys must match") - plan, _ := s.startMockSubscription(ctx, t, testCutomer.GetID()) + plan, _ := s.startMockSubscription(ctx, t, testCustomer.GetID())And update the remaining references to
testCutomer→testCustomerin the assertions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
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/aio/operations/_operations.pyis excluded by!**/_generated/**,!api/client/**api/client/python/openmeter/_generated/operations/_operations.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 (7)
api/api.gen.goapi/spec/src/customer/customer.tspopenmeter/customer/adapter/customer.goopenmeter/customer/customer.goopenmeter/customer/httpdriver/customer.gotest/customer/customer.gotest/customer/customer_test.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/customer.gotest/customer/customer_test.goopenmeter/customer/adapter/customer.gotest/customer/customer.goopenmeter/customer/httpdriver/customer.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.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 (1)
📚 Learning: 2025-03-25T09:13:03.300Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 2522
File: api/spec/src/productcatalog/routes.tsp:163-314
Timestamp: 2025-03-25T09:13:03.300Z
Learning: ULIDOrKey is defined in api/spec/src/types.tsp as a scalar type with a pattern that validates against both ULID and Key formats, not as a type alias like PlanIdOrKey.
Applied to files:
api/spec/src/customer/customer.tsp
🧬 Code graph analysis (3)
openmeter/customer/customer.go (3)
openmeter/productcatalog/plan/plan.go (1)
Plan(18-30)e2e/productcatalog_test.go (1)
PlanKey(22-22)pkg/models/errors.go (1)
NewGenericValidationError(138-140)
openmeter/customer/adapter/customer.go (1)
openmeter/customer/customer.go (2)
Expands(20-20)ExpandSubscriptions(24-24)
test/customer/customer.go (2)
openmeter/productcatalog/subscription/plan.go (2)
Plan(50-53)Phase(100-104)pkg/models/id.go (1)
NamespacedID(7-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). (9)
- GitHub Check: Artifacts / Container image
- GitHub Check: Test
- GitHub Check: Artifacts / Benthos Collector Container image
- GitHub Check: Code Generators
- GitHub Check: Lint
- GitHub Check: Build
- GitHub Check: Repository Scan
- GitHub Check: SAST (semgrep)
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
openmeter/customer/httpdriver/customer.go (1)
59-59: Looks good!The
Planfilter is correctly mapped from the API params to the request input. The mutual exclusivity validation withPlanKeyis handled inListCustomersInput.Validate(), so no additional checks are needed here.api/spec/src/customer/customer.tsp (1)
166-180: Nice addition to the API!The spec clearly documents both filter options with their mutual exclusivity. Using
ULIDtype for the plan ID is appropriate here, and the documentation makes it clear thatplanKeyuses case-sensitive exact matching whileplanfilters by the specific plan version ID.openmeter/customer/adapter/customer.go (1)
756-769: Clean refactoring!Extracting
activeSubscriptionFilterPredicatesinto a reusable helper is a solid improvement. It centralizes the active subscription logic and makes it easier to maintain consistency acrossWithActiveSubscriptionsand the new subscription-based filters.test/customer/customer_test.go (1)
38-40: LGTM!The new test case is properly integrated into the test suite, following the established pattern.
openmeter/customer/customer.go (2)
311-311: Field addition looks good.The
Planfield is correctly added as*stringto match the existingPlanKeypattern.
328-330: Validation is solid.The mutual exclusivity check is appropriately placed in
Validate()and provides a clear error message. This aligns with the API spec documentation.test/customer/customer.go (2)
981-1070: Nice helper functions!The
startMockSubscription,createMockPlan, andstartSubscriptionFromPlanhelpers are well-structured and will make it easier to set up subscription scenarios in future tests. Good use of the existingPlanFromPlanutility.
373-373: Good refactor to use the new helper!Replacing the inline subscription setup with
startMockSubscriptionkeeps the test focused on its actual purpose.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/customer/customer.go (1)
562-563: Redundant variable: Use a single variable name.Both
serviceandcServiceare assigned tos.Env.Customer(), but onlycServiceis used in the test. You can remove theservicedeclaration and just usecService(or rename it toservicefor consistency with other tests).🔎 Proposed fix
- service := s.Env.Customer() - cService := s.Env.Customer() + service := s.Env.Customer()Then update line 585 and 596:
- list, err := service.ListCustomers(ctx, customer.ListCustomersInput{ + list, err := service.ListCustomers(ctx, customer.ListCustomersInput{(Already using
service, so no change needed for those lines if you remove cService and update line 566 to useserviceinstead)Or simply remove the unused
servicedeclaration and keepcService.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
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/aio/operations/_operations.pyis excluded by!**/_generated/**,!api/client/**api/client/python/openmeter/_generated/operations/_operations.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 (6)
api/api.gen.goapi/spec/src/customer/customer.tspopenmeter/customer/adapter/customer.goopenmeter/customer/customer.goopenmeter/customer/httpdriver/customer.gotest/customer/customer.go
🚧 Files skipped from review as they are similar to previous changes (3)
- openmeter/customer/httpdriver/customer.go
- openmeter/customer/customer.go
- api/spec/src/customer/customer.tsp
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/customer/customer.goopenmeter/customer/adapter/customer.go
🧬 Code graph analysis (2)
test/customer/customer.go (3)
openmeter/productcatalog/plan.go (2)
Plan(41-46)PlanMeta(198-224)openmeter/productcatalog/subscription/plan.go (3)
Plan(50-53)Phase(100-104)RateCard(134-137)openmeter/subscription/subscriptionspec.go (2)
NewSpecFromPlan(1033-1125)CreateSubscriptionCustomerInput(42-52)
openmeter/customer/adapter/customer.go (1)
openmeter/customer/customer.go (2)
Expands(20-20)ExpandSubscriptions(24-24)
⏰ 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). (8)
- GitHub Check: Artifacts / Container image
- GitHub Check: Build
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Migration Checks
- GitHub Check: Code Generators
- GitHub Check: SAST (semgrep)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
openmeter/customer/adapter/customer.go (4)
36-36: Nice readability improvement!Extracting the
expandSubscriptionsflag makes the intent clearer and avoids repeating theslices.Containscheck. Good call!
81-102: Looking good – the subscription filter logic is solid!The predicate-based filtering approach is clean, and I see the query assignment issue from the previous review (line 99) has been properly fixed. The logic correctly applies both Plan ID and Plan Key filters when present, and based on the test coverage, mutual exclusivity is enforced at the validation layer (as it should be).
756-770: Great refactoring – centralized predicate logic!Extracting the active subscription predicates into a reusable function is a solid move. The logic correctly handles the time-based filtering (active from/to, deleted at, created at), and using it consistently for both filtering and expansion keeps things DRY.
749-754: Clean update to use the centralized predicates!
WithActiveSubscriptionsnow leverages the sharedactiveSubscriptionFilterPredicatesfunction, which makes the codebase more maintainable and consistent.test/customer/customer.go (3)
373-373: Nice cleanup – helper usage simplifies the test!Using
startMockSubscriptionhere keeps the test focused on the update logic rather than subscription setup details.
981-1043: Great test helpers – clean and reusable!The
startMockSubscriptionandcreateMockPlanhelpers significantly reduce duplication and make the tests more maintainable. The plan creation includes proper error checks and a realistic structure.
1045-1070: Solid helper for subscription lifecycle testing!
startSubscriptionFromPlanencapsulates the subscription creation flow nicely. The clock advancement is a good touch for ensuring time-based logic is properly tested.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openmeter/customer/httpdriver/customer.go (1)
42-78: Consider adding mutual exclusivity check forPlanIdandPlanKeyAccording to the PR description and AI summary,
planIdandplanKeyshould be mutually exclusive filters. Right now both validations run independently, so if a caller provides both params, the code will make two plan lookups before proceeding.While this might be enforced at the OpenAPI validation layer, adding a defensive check here would make the intent clear and prevent unnecessary database calls if both are somehow provided.
🔎 Proposed fix
+ // Check mutual exclusivity of PlanId and PlanKey + if params.PlanId != nil && params.PlanKey != nil { + return ListCustomersRequest{}, models.NewGenericValidationError( + fmt.Errorf("planId and planKey are mutually exclusive"), + ) + } + // Check if the plan by ID exists if params.PlanId != nil {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
openmeter/customer/httpdriver/customer.goopenmeter/customer/httpdriver/handler.goopenmeter/server/router/router.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/httpdriver/handler.goopenmeter/customer/httpdriver/customer.goopenmeter/server/router/router.go
🧬 Code graph analysis (1)
openmeter/customer/httpdriver/customer.go (5)
openmeter/productcatalog/plan/service.go (1)
GetPlanInput(304-319)pkg/models/id.go (1)
NamespacedID(7-10)pkg/models/errors.go (2)
IsGenericNotFoundError(57-65)NewGenericPreConditionFailedError(233-235)api/v3/handlers/customers/list.go (1)
ListCustomersRequest(22-22)e2e/productcatalog_test.go (1)
PlanKey(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). (10)
- GitHub Check: E2E
- GitHub Check: Quickstart
- GitHub Check: Build
- GitHub Check: Test
- GitHub Check: Code Generators
- GitHub Check: Lint
- GitHub Check: Migration Checks
- GitHub Check: Repository Scan
- GitHub Check: SAST (semgrep)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
openmeter/customer/httpdriver/customer.go (3)
42-59: LGTM on plan ID validation logicThe validation correctly checks for plan existence and returns a precondition failed error with a clear message when not found. Other errors are properly propagated. Nice and clean!
61-78: LGTM on plan key validation logicSame pattern as the ID check - consistent error handling approach. 👍
99-100: LGTM - PlanID correctly wired into requestThe new
PlanIDfield is properly mapped fromparams.PlanIdalongside the existingPlanKeyfield.openmeter/customer/httpdriver/handler.go (2)
33-40: LGTM - Handler struct updated correctlyClean addition of the
planServicefield. The struct field ordering matches the constructor parameter order nicely.
51-66: LGTM - Constructor updated with new dependencyThe
planServiceis properly injected and initialized. Everything lines up correctly with the struct definition.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
openmeter/server/router/router.go (1)
400-402: Remove redundant nil check.Since
Config.Validate()now checks for nil Plan (lines 189-191) and is called at the start ofNewRouter(line 257), this check is redundant and can be removed.🔎 Proposed cleanup
- // Product Catalog - if config.Plan == nil { - return nil, errors.New("plan service is required") - } - router.planHandler = planhttpdriver.New(test/customer/customer.go (2)
562-563: Remove redundant variable declaration.Both
serviceandcServiceare assigned tos.Env.Customer(). You can just use one variable throughout the test.🔎 Proposed fix
- service := s.Env.Customer() - cService := s.Env.Customer() + service := s.Env.Customer()Then use
serviceconsistently throughout (e.g., line 566).
1000-1078: Great test helpers!Both
createMockPlanandstartSubscriptionFromPlanare well-structured and will be reusable across tests. The time advancement (line 1072) is a nice touch to simulate realistic subscription lifecycle.Minor style note: Consider using
require.NoError()instead ofrequire.Nil()for error checks (lines 1037, 1047, 1070, 1075) for more idiomatic Go test assertions.🔎 Optional style improvement
- require.Nil(t, err) + require.NoError(t, err, "Creating plan must not return error")Apply similar changes to lines 1047, 1070, and 1075 for consistency with the rest of the test suite.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
openmeter/customer/customer.goopenmeter/server/router/router.gotest/customer/customer.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/customer/customer.goopenmeter/server/router/router.goopenmeter/customer/customer.go
🧬 Code graph analysis (2)
test/customer/customer.go (4)
pkg/models/errors.go (1)
IsGenericValidationError(160-168)openmeter/productcatalog/plan/service.go (1)
CreatePlanInput(105-110)pkg/clock/clock.go (2)
Now(14-21)SetTime(23-27)openmeter/productcatalog/subscription/service/plan.go (1)
PlanFromPlan(76-81)
openmeter/server/router/router.go (6)
openmeter/productcatalog/plan.go (1)
Plan(41-46)openmeter/subscription/plan.go (1)
Plan(59-69)openmeter/productcatalog/subscription/plan.go (1)
Plan(50-53)openmeter/productcatalog/plan/plan.go (1)
Plan(18-30)app/common/productcatalog.go (1)
Plan(37-39)openmeter/customer/httpdriver/handler.go (1)
New(51-67)
⏰ 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). (9)
- GitHub Check: Artifacts / Container image
- GitHub Check: Code Generators
- GitHub Check: Lint
- GitHub Check: Migration Checks
- GitHub Check: Build
- GitHub Check: Test
- GitHub Check: SAST (semgrep)
- GitHub Check: Repository Scan
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
openmeter/server/router/router.go (2)
189-191: Nice fix! Plan service validation is now in the right place.Moving the Plan nil-check into
Config.Validate()ensures it's caught early, before any handler construction. This addresses the previous review concern cleanly.
352-359: Customer handler wiring looks good!The Plan service is properly threaded through to the customer handler, and since
Config.Validate()now checks for nil Plan, this is safe.openmeter/customer/customer.go (2)
311-311: PlanID field addition looks good.The new field is properly typed as an optional pointer and positioned logically next to the PlanKey filter.
319-376: Solid validation implementation!The new
Validate()method is comprehensive and covers all the important cases:
- Mutual exclusivity between PlanID and PlanKey (which makes sense since they serve different filtering purposes)
- Empty string checks for all optional filters
- Clear, descriptive error messages
The validation is a bit verbose, but that's fine—explicit validation is better than clever shortcuts here.
test/customer/customer.go (3)
373-373: Nice cleanup using the new helper!Replacing the inline subscription setup with
startMockSubscriptionmakes the test much more readable.
581-633: Excellent test coverage!The test scenarios are thorough and cover all the important cases:
- Filtering by PlanID works ✓
- Filtering by PlanKey works ✓
- Empty string validation for both filters ✓
- Mutual exclusivity validation ✓
The assertions are clear and use appropriate error-checking helpers.
989-998: Clean helper composition!The
startMockSubscriptionhelper nicely orchestrates plan creation and subscription lifecycle for tests.
Currently, we only allow filtering the customer list by plan key, and also allow filtering by plan ID to be able to filter for specific plan versions.
Summary by CodeRabbit
New Features
Validation / Error Handling
Bug Fixes / Docs
Tests
✏️ Tip: You can customize this high-level summary in your review settings.