Credit API Refactors (History Rewrite Part I)#2369
Conversation
GAlexIHU
force-pushed
the
galexi/history-rewrite
branch
from
9a07d7a to
ec3e9fb
Compare
GAlexIHU
force-pushed
the
galexi/history-rewrite
branch
from
ec3e9fb to
37b2c68
Compare
WalkthroughThe changes update the owner type used across multiple modules by replacing the custom type Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GrantConnector
participant OwnerConnector
participant Repository
Client->>GrantConnector: CreateGrant(ownerID, input)
GrantConnector->>OwnerConnector: DescribeOwner(ownerID)
OwnerConnector-->>GrantConnector: Owner {Namespace, Meter, QueryParams, ResetBehavior}
GrantConnector->>Repository: CreateGrant(owner, input)
Repository-->>GrantConnector: Grant & Response
GrantConnector-->>Client: Return Grant / Error
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
openmeter/credit/balance.go (1)
63-66: Inconsistency in error messageThe error message on line 65 still references
owner.IDbut should referenceownerID.IDto be consistent with the parameter name change.- return nil, fmt.Errorf("failed to describe owner %s: %w", owner.ID, err) + return nil, fmt.Errorf("failed to describe owner %s: %w", ownerID.ID, err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
openmeter/credit/adapter/balance_snapshot.go(3 hunks)openmeter/credit/adapter/grant.go(4 hunks)openmeter/credit/balance.go(12 hunks)openmeter/credit/balance/repository.go(1 hunks)openmeter/credit/grant.go(7 hunks)openmeter/credit/grant/events.go(1 hunks)openmeter/credit/grant/grant.go(2 hunks)openmeter/credit/grant/owner_connector.go(2 hunks)openmeter/credit/grant/repo.go(3 hunks)openmeter/credit/helper.go(7 hunks)openmeter/entitlement/balanceworker/worker.go(2 hunks)openmeter/entitlement/metered/balance.go(9 hunks)openmeter/entitlement/metered/balance_test.go(15 hunks)openmeter/entitlement/metered/connector.go(1 hunks)openmeter/entitlement/metered/entitlement_grant.go(3 hunks)openmeter/entitlement/metered/grant_owner_adapter.go(10 hunks)openmeter/entitlement/metered/grant_owner_adapter_test.go(8 hunks)openmeter/entitlement/metered/reset.go(1 hunks)openmeter/entitlement/metered/reset_test.go(19 hunks)openmeter/server/server_test.go(1 hunks)test/entitlement/regression/scenario_test.go(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: CI
- GitHub Check: Analyze (go)
🔇 Additional comments (51)
openmeter/credit/grant/events.go (1)
46-46: LGTM: Direct resource path construction.This change simplifies the code by passing the
e.OwnerIDdirectly to theComposeResourcePathfunction without type conversion, which aligns with the PR objective of standardizing ownership representation.openmeter/entitlement/metered/grant_owner_adapter_test.go (1)
62-65: Simplifies type usage by replacing custom owner type with standard model.The change replaces the custom
grant.NamespacedOwnertype with the standardmodels.NamespacedIDtype, which aligns with the PR objective of eliminating unnecessary type aliases. This simplifies the code and makes it more consistent with the rest of the codebase.Also applies to: 139-142, 186-189, 254-257, 313-316, 357-360, 413-416, 469-472
openmeter/entitlement/metered/reset.go (1)
20-23: Simplifies owner type representation.This change replaces the custom
grant.NamespacedOwnertype with the standardmodels.NamespacedIDtype, removing the unnecessary conversion throughgrant.Owner(). This aligns with the PR objective of eliminating unnecessary type aliases and simplifies the code.test/entitlement/regression/scenario_test.go (2)
66-69: Standardizes owner type across test scenarios.The change consistently replaces the custom
grant.NamespacedOwnertype with the standardmodels.NamespacedIDtype across all test scenarios, removing unnecessary type conversions. This aligns with the PR objective of eliminating unnecessary type aliases and makes the tests more consistent with the updated implementation.Also applies to: 84-87, 129-132, 186-189, 214-218, 254-257, 313-316, 359-362, 413-416, 498-501, 534-537
350-353: Standardizes owner type in balance snapshot operations.The change replaces the custom
grant.NamespacedOwnertype with the standardmodels.NamespacedIDtype in the balance snapshot repository call, which aligns with the standardization across the rest of the codebase.openmeter/entitlement/metered/reset_test.go (1)
214-214: Consistent ownership model simplificationThe changes in this file are part of a broader effort to simplify the ownership representation by removing unnecessary type abstractions. The code now directly uses string IDs instead of a custom
Ownertype and employsmodels.NamespacedIDinstead of the customNamespacedOwnertype.Also applies to: 231-234, 236-239, 286-287, 297-298, 325-328, 355-356, 366-367, 397-400, 427-428, 438-439, 469-472, 536-537, 547-548, 575-578, 588-589, 616-619, 650-651, 675-676, 722-723
openmeter/entitlement/metered/balance_test.go (1)
191-192: Consistent ownership model simplificationThe changes in this test file follow the same pattern as seen in other files, switching from custom ownership types to direct string IDs and standard
models.NamespacedIDstructures. This refactoring improves code clarity by removing unnecessary abstractions while maintaining the same functionality.Also applies to: 201-202, 243-246, 249-250, 323-326, 329-330, 477-478, 488-489, 499-500, 536-537, 588-589, 616-619, 650-651, 675-676, 722-723, 746-747, 756-758, 836-837, 846-848
openmeter/credit/grant/repo.go (1)
43-43: Core ownership model refactoringThese changes represent the core refactoring of the ownership model in the repository layer:
- Simplified the
OwnerIDfield types from customOwnertype to simple string- Updated method signatures to use standard
models.NamespacedIDinstead of customNamespacedOwnertypeThis refactoring reduces abstraction layers in the codebase, making it more straightforward and easier to maintain.
Also applies to: 57-57, 76-76
openmeter/credit/grant/grant.go (1)
20-20: Fundamental data model simplificationThese changes complete the ownership model refactoring by:
- Changing the core
Grantstruct to use a simple string forOwnerIDrather than a custom type- Updating the
GetNamespacedOwnermethod to return the standardmodels.NamespacedIDtypeThis simplification of the fundamental data model makes the code more maintainable and removes unnecessary abstraction layers while preserving the same functionality.
Also applies to: 118-123
openmeter/credit/grant.go (2)
18-19: Unified parameter usage for CreateGrant
Refactoring fromNamespacedOwnertomodels.NamespacedIDis consistent and straightforward. The switch toDescribeOwnerfor retrieving the owner and truncating to the meter’s window size is logically sound. No significant concerns—good work!Also applies to: 35-35, 44-44, 48-48, 53-53, 62-62, 67-68, 84-84, 89-89, 91-91, 94-94, 97-97
120-120: Updated approach for VoidGrant
The new usage ofownerIDand calls toDescribeOwner,LockOwnerForTx, andInvalidateAfteraligns with the refactored interface. Concurrency handling via owner-level locks is clearly addressed. No issues found.Also applies to: 128-133, 145-145, 151-151, 158-158
openmeter/credit/grant/owner_connector.go (2)
16-29: New Owner struct
Embeddingmodels.NamespacedIDand includingMeter,DefaultQueryParams, andResetBehaviorwithin theOwnerstruct neatly consolidates the data. TheGetSubjectKey()method is a clean, direct way to retrieve the subject. This greatly improves clarity.
46-46: Refined interface with DescribeOwner
Switching fromNamespacedOwnertomodels.NamespacedIDunifies the codebase. The addition ofDescribeOwnerto the interface provides a more direct and consistent way of fetching the owner’s details. No concerns here.Also applies to: 52-54, 56-57, 61-61
openmeter/entitlement/metered/grant_owner_adapter.go (3)
46-101: DescribeOwner implementation
Fetching the linked entitlement, meter, and subject key to constructgrant.Owneris streamlined. The error handling paths for missing or invalid data (feature, meter, etc.) are handled clearly, preventing null references. Nice work.
103-145: Usage period retrieval methods
Adapting these methods to usemodels.NamespacedID(e.g.,GetStartOfMeasurement,GetUsagePeriodStartAt,GetResetTimelineInclusive) is in line with the refactoring goals. The operations for computing and resetting usage periods remain clear and appear logically correct.Also applies to: 147-280
306-349: Usage period finalization
The approach to ending usage periods (EndCurrentUsagePeriod), updating the entitlement usage period, and locking the owner for transactional safety looks robust. Keeping everything within a single transaction ensures consistency. No issues noted.Also applies to: 351-378, 380-387
openmeter/server/server_test.go (1)
648-648: Type change aligns with standardized owner representationThe parameter type for
ownerhas been updated fromgrant.NamespacedOwnertomodels.NamespacedID, which is part of the larger standardization effort to consolidate ownership representation across the codebase.openmeter/entitlement/metered/connector.go (1)
209-212: Simplified owner ID representationThe change replaces
grant.NamespacedOwnerwithmodels.NamespacedIDand directly assignsmetered.IDto the ID field without unnecessary conversion, making the code more concise and consistent with the rest of the codebase.openmeter/entitlement/balanceworker/worker.go (2)
171-173: Streamlined owner ID handlingThe change eliminates unnecessary string conversion for
event.OwnerID, using the ID directly when constructing theNamespacedIDand resource path. This simplifies the code while maintaining the same functionality.
182-184: Streamlined owner ID handlingSimilar to the earlier change, this eliminates unnecessary string conversion for
event.OwnerID, using the ID directly when constructing theNamespacedIDand resource path for the void event handler.openmeter/entitlement/metered/entitlement_grant.go (3)
33-36: Simplified owner ID representationThe change replaces
grant.NamespacedOwnerwithmodels.NamespacedIDand assignsent.IDdirectly to the ID field, making the code more straightforward and consistent with the standardized approach to ownership representation.
74-75: Direct owner ID assignmentUpdated to use
ent.IDdirectly rather than converting throughgrant.Owner(ent.ID), which simplifies the code and aligns with the ownership type standardization.
119-120: Simplified entitlement ID assignmentDirectly assigns
grant.OwnerIDtoEntitlementIDwithout unnecessary type conversion, making the code more concise and easier to understand.openmeter/credit/adapter/grant.go (4)
37-37: Clean type handling in SetOwnerIDThe code now directly uses grant.OwnerID without string conversion, which aligns with the refactoring to standardize ownership representation.
76-76: Simplified owner ID filteringDirect use of the OwnerID parameter in the where clause improves code clarity by removing unnecessary type conversions.
168-170: Method signature improved with models.NamespacedIDReplacing
grant.NamespacedOwnerwithmodels.NamespacedIDstandardizes the owner representation across the codebase, making it more consistent and easier to maintain.
219-219: Simplified owner ID assignmentDirect assignment of entity.OwnerID removes unnecessary conversion through grant.Owner function, streamlining the code.
openmeter/credit/balance/repository.go (3)
9-9: Clean import additionAdded models package import to support the type changes in the interface.
13-16: Interface methods standardized with models.NamespacedIDAll SnapshotRepo interface methods now consistently use models.NamespacedID instead of the custom grant.NamespacedOwner type, which enhances API clarity and simplifies the contract.
23-23: Error type updated for consistencyThe Owner field in NoSavedBalanceForOwnerError now uses models.NamespacedID, maintaining type consistency across the error handling system.
openmeter/entitlement/metered/balance.go (8)
59-62: Simplified owner initializationConstructor for models.NamespacedID is now more direct, eliminating unnecessary type conversions and enhancing code readability.
71-73: Improved method naming and error handlingChanged from
GetMeterto more descriptiveDescribeOwnermethod and updated the error message to accurately reflect the operation being performed.
83-83: Direct access to owner query parametersNow directly accessing owner.DefaultQueryParams instead of using intermediate conversions, improving code clarity.
92-92: Simplified meter accessUsing owner.Meter directly to query the meter, which streamlines the code by removing unnecessary conversions.
143-149: Consistent owner description approachUsing DescribeOwner with models.NamespacedID maintains consistency with earlier changes in the file and improves error message clarity.
174-175: Updated query method parametersCorrectly passing owner namespace and meter when querying, maintaining functionality while using the new type structure.
252-253: Clean balance history retrievalNow using owner.NamespacedID directly when getting balance history, aligning with the standardized approach to owner identification.
314-314: Consistent query parameter usageUsing owner.NamespacedID.Namespace for meter queries maintains consistency with other parameter usage in the method.
openmeter/credit/adapter/balance_snapshot.go (4)
13-13: Clean import additionAdded models package import to support the type changes in method signatures.
27-31: Updated InvalidateAfter method signatureMethod now uses models.NamespacedID and correctly references owner.ID in the database query, maintaining functionality while using the standardized type.
34-38: Improved GetLatestValidAt implementationMethod signature now uses models.NamespacedID and the database query properly references owner.ID directly, simplifying the code.
55-61: Refactored Save method with consistent type usageThe Save method now uses models.NamespacedID and correctly sets owner ID in the database, maintaining consistency with other methods.
openmeter/credit/balance.go (4)
26-28: Interface signature changes look goodThe interface declaration has been updated to use
models.NamespacedIDinstead of the previous type. This aligns with the PR objective of eliminating unnecessary type aliases.
38-39: Parameter name changed to reflect new typeThe parameter has been appropriately renamed from
ownertoownerIDto better reflect its nature as an identifier rather than a complete owner object.
128-131: Error message correctly updatedThe error message on line 130 has been correctly updated to reference
ownerID.ID.
182-185: Method calls consistently updatedAll method calls to the owner connector have been updated to use the new parameter type, and the variable names have been appropriately adjusted.
openmeter/credit/helper.go (5)
24-24: Function signature updated correctlyThe function signature has been updated to use
models.NamespacedIDinstead of the previous type, maintaining consistency with the other changes in this PR.
56-66: Clean pattern for getting owner detailsThe code now uses a clean pattern of first retrieving the owner object with
DescribeOwnerand then using its properties. This is more maintainable than accessing properties through different method calls.
123-125: Owner-specific properties accessed correctlyThe code now consistently accesses owner-specific properties through the owner object returned by
DescribeOwner.
156-160: Consistent use of namespace from owner IDThe meter query now correctly uses the namespace from the owner ID, maintaining the functionality while simplifying the type structure.
203-203: Struct definition updated to match new typeThe
snapshotParamsstruct has been updated to use the new type for itsownerfield, ensuring consistency throughout the codebase.
2 participants
Overview
Internal API refactors, precursor to history PR, no changes in functionality.
OwnerConnectormethods into one for easier useSummary by CodeRabbit
Refactor
Tests