Conversation
📝 WalkthroughWalkthroughAdds a soft-delete API to mark all grants owned by a specific owner as deleted, wires a new entitlement pre-delete hook to invoke that API, and standardizes several OwnerNotFoundError checks to use lo.ErrorsAs for type-safe error detection across multiple files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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)
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: 1
🧹 Nitpick comments (3)
openmeter/credit/grant/owner_connector.go (1)
59-65: LGTM! Constructor provides consistent error wrapping.The new
NewOwnerNotFoundErrorconstructor properly wraps the specific error type into a generic not-found error, providing a clean API boundary for error handling.Consider adding a blank line between line 58 and 59 for better readability:
func (e OwnerNotFoundError) Error() string { return fmt.Sprintf("Owner %s not found in namespace %s, attempted to find as %s", e.Owner.ID, e.Owner.Namespace, e.AttemptedOwner) } + func NewOwnerNotFoundError(owner models.NamespacedID, attemptedOwner string) error {openmeter/credit/adapter/grant.go (1)
66-72: Consider validating input parameters.The method doesn't validate that
ownerID.IDandownerID.Namespaceare non-empty. While this is a repository-layer method, defensive validation could prevent potential issues.Apply this diff to add validation:
func (g *grantDBADapter) DeleteOwnerGrants(ctx context.Context, ownerID models.NamespacedID) error { + if ownerID.ID == "" || ownerID.Namespace == "" { + return fmt.Errorf("ownerID and namespace must not be empty") + } command := g.db.Grant.Update(). SetDeletedAt(clock.Now()). Where(db_grant.OwnerID(ownerID.ID), db_grant.Namespace(ownerID.Namespace)) return command.Exec(ctx) }openmeter/credit/hook/entitlement_hook.go (1)
41-41: Consider adding context to the error.The error from
DeleteOwnerGrantsis returned directly. Adding context would improve debuggability.Apply this diff to wrap the error:
- return h.grantRepo.DeleteOwnerGrants(ctx, models.NamespacedID{Namespace: meteredEnt.Namespace, ID: meteredEnt.ID}) + if err := h.grantRepo.DeleteOwnerGrants(ctx, models.NamespacedID{Namespace: meteredEnt.Namespace, ID: meteredEnt.ID}); err != nil { + return fmt.Errorf("failed to delete grants for entitlement %s: %w", meteredEnt.ID, err) + } + return nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
openmeter/credit/adapter/grant.go(1 hunks)openmeter/credit/balance.go(2 hunks)openmeter/credit/grant/owner_connector.go(1 hunks)openmeter/credit/grant/repo.go(1 hunks)openmeter/credit/hook/entitlement_hook.go(1 hunks)openmeter/entitlement/metered/entitlement_grant.go(1 hunks)openmeter/entitlement/metered/grant_owner_adapter.go(2 hunks)openmeter/entitlement/metered/reset.go(2 hunks)openmeter/registry/builder/entitlement.go(2 hunks)openmeter/server/server_test.go(1 hunks)test/billing/subscription_suite.go(2 hunks)test/entitlement/regression/framework_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
openmeter/credit/grant/repo.go (1)
pkg/models/id.go (1)
NamespacedID(7-10)
openmeter/credit/grant/owner_connector.go (2)
pkg/models/id.go (1)
NamespacedID(7-10)pkg/models/errors.go (1)
NewGenericNotFoundError(38-40)
openmeter/entitlement/metered/grant_owner_adapter.go (1)
openmeter/credit/grant/owner_connector.go (1)
NewOwnerNotFoundError(60-65)
openmeter/credit/adapter/grant.go (4)
pkg/models/id.go (1)
NamespacedID(7-10)openmeter/ent/db/grant.go (2)
Grant(21-63)Grant(86-105)pkg/clock/clock.go (1)
Now(14-21)openmeter/ent/db/grant/where.go (3)
OwnerID(90-92)ID(15-17)Namespace(70-72)
openmeter/credit/balance.go (1)
openmeter/credit/grant/owner_connector.go (1)
OwnerNotFoundError(51-54)
openmeter/registry/builder/entitlement.go (1)
openmeter/credit/hook/entitlement_hook.go (1)
NewEntitlementHook(23-29)
openmeter/entitlement/metered/entitlement_grant.go (1)
openmeter/credit/grant/owner_connector.go (1)
OwnerNotFoundError(51-54)
openmeter/server/server_test.go (1)
pkg/models/id.go (1)
NamespacedID(7-10)
test/entitlement/regression/framework_test.go (1)
openmeter/credit/hook/entitlement_hook.go (1)
NewEntitlementHook(23-29)
openmeter/credit/hook/entitlement_hook.go (4)
pkg/models/servicehook.go (2)
ServiceHook(9-15)NoopServiceHook(167-167)openmeter/credit/grant/repo.go (1)
Repo(72-86)openmeter/entitlement/metered/entitlement.go (1)
ParseFromGenericEntitlement(73-126)pkg/models/id.go (1)
NamespacedID(7-10)
openmeter/entitlement/metered/reset.go (1)
openmeter/credit/grant/owner_connector.go (1)
OwnerNotFoundError(51-54)
test/billing/subscription_suite.go (1)
openmeter/credit/hook/entitlement_hook.go (1)
NewEntitlementHook(23-29)
⏰ 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 / Benthos Collector Container image
- GitHub Check: Artifacts / Container image
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Migration Checks
- GitHub Check: Code Generators
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (14)
openmeter/credit/balance.go (2)
228-234: Improved error handling with error unwrapping.The change from type assertion to
lo.ErrorsAsis a good improvement. Unlike type assertion which only checks for exact type matches,lo.ErrorsAs(wrapping Go'serrors.As) properly handles wrapped errors by unwrapping the error chain. This makes the error checking more robust when errors are wrapped with additional context (e.g.,fmt.Errorf("context: %w", err)).
18-18: Import andlo.ErrorsAsusage verified as correct.The
lo.ErrorsAsfunction is a shortcut forerrors.As, making it a type-safe wrapper that properly handles error wrapping chains. The import on line 18 is necessary and the usage on line 230 follows the correct pattern:lo.ErrorsAs[*ErrorType](err)returns a tuple that can be checked withok.This refactor improves error handling compared to direct type assertions, as
errors.Asproperly unwraps error chains while type assertions only match exact types. The pattern is consistently used throughout the codebase (20+ locations verified) with no conflicts.openmeter/registry/builder/entitlement.go (1)
13-13: LGTM! Hook registration follows the correct pattern.The new
EntitlementHookis properly imported and registered after the entitlement connector is initialized, consistent with the existing subscription hook registration pattern at line 114.Also applies to: 115-115
openmeter/entitlement/metered/reset.go (1)
17-17: LGTM! Error handling refactored to use type-safe approach.The change from type assertion to
lo.ErrorsAsprovides better type safety while maintaining the same behavior - mappingOwnerNotFoundErrortoentitlement.NotFoundError.Also applies to: 50-50
test/billing/subscription_suite.go (1)
15-15: LGTM! Test configuration mirrors production setup.The hook registration in tests ensures that the cascade deletion behavior will be exercised during test execution, maintaining consistency with the production configuration.
Also applies to: 298-298
openmeter/entitlement/metered/entitlement_grant.go (1)
83-83: LGTM! Consistent error handling refactor.The change aligns with the error handling pattern used in
reset.go, providing type-safe error checking across the codebase.openmeter/entitlement/metered/grant_owner_adapter.go (1)
61-61: LGTM! Consistent use of error constructor.Both call sites now use the centralized
grant.NewOwnerNotFoundErrorconstructor, improving maintainability by having a single point of error construction.Also applies to: 125-125
openmeter/server/server_test.go (1)
1338-1340: LGTM! No-op implementation maintains interface compliance.The no-op implementation of
DeleteOwnerGrantsis consistent with the existing test double pattern inNoopGrantRepo, allowing tests to compile without requiring a full repository implementation.openmeter/credit/grant/repo.go (1)
81-82: Soft deletion properly implemented with appropriate foreign key cascade.Verified that
DeleteOwnerGrantsperforms a soft delete by settingdeleted_attimestamp, confirmed in the implementation atopenmeter/credit/adapter/grant.go:66-72. The foreign key constraint on the grants table is configured withON DELETE CASCADE(migration20250605102416_subscription-cascade.up.sql), which means soft deletion is used for explicit grant cleanup operations while the database handles cascading cleanup of grants when entitlements are deleted. The implementation is correct and intentional.test/entitlement/regression/framework_test.go (2)
18-18: LGTM! Import correctly added for the new entitlement hook.The aliased import follows the existing pattern and is used appropriately at line 203.
201-204: LGTM! Hook registration correctly integrated.The entitlement hook is properly wired alongside the subscription hook, enabling grant cascade deletion in the test suite.
openmeter/credit/hook/entitlement_hook.go (2)
12-21: LGTM! Clean hook structure using composition.The type aliases and struct design follow the service hook pattern correctly. Embedding
NoopHookprovides default implementations while allowing selective override of lifecycle methods.
23-29: LGTM! Constructor follows standard patterns.The constructor properly initializes the hook with its dependency and returns the interface type.
openmeter/credit/adapter/grant.go (1)
66-72: Transaction coordination is already properly handled; code is correct.The implementation correctly coordinates transactions. The entitlement service wraps the entire deletion flow—including PreDelete hooks and grant deletion—within a single
transaction.Run()at line 172. This ensures that if the grant soft-delete fails, the entitlement deletion is rolled back automatically. The context passed through the hook chain retains transaction state from the service layer, making the grant adapter's soft-delete operation part of the same atomic transaction.
2 participants
Overview
Notes for reviewer
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests