feat: add add-on API #2590
feat: add add-on API #2590
Conversation
chrisgacsal
added
area/api
release-note/feature
📝 WalkthroughWalkthroughThe changes add comprehensive support for managing add-ons and add-on rate cards throughout the system. New entities, CRUD operations, query builders, mutations, schema definitions, client integrations, service layers, HTTP drivers, event definitions, and corresponding tests have been introduced. The modifications extend both the database layer and product catalog modules, integrating add-on functionality into the server wiring, transactional clients, and API endpoints. Changes
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ 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: 5
🧹 Nitpick comments (58)
openmeter/ent/db/predicate/predicate.go (2)
9-21: Addon predicate and error handling properly implementedThe Addon predicate type and its corresponding OrErr function follow the established pattern in the codebase for creating predicates and error handlers. Note the typo in the comment where "nit" should be "nil", but this is consistent with the existing comments throughout the file.
There's a small typo in the comment - "if the error is not nit" should be "if the error is not nil". However, this appears to be consistent with similar comments throughout the file for other entities, so it's not critical to change unless fixing all occurrences.
23-35: AddonRateCard predicate and error handling properly implementedThe AddonRateCard predicate type and its corresponding OrErr function are correctly implemented, matching the pattern used for other entities in the codebase.
Same typo as above in the comment: "if the error is not nit" should be "if the error is not nil", but this is consistent with existing patterns.
openmeter/productcatalog/addon/assert.go (4)
28-52: Inconsistent nil pointer handling in Description field.While most fields are checked with direct comparison after nil check, the Description field uses a special handling with
lo.FromPtrOr(). Consider using the same approach for all fields to avoid potential nil pointer panics.if i.Name != nil { - assert.Equalf(t, *i.Name, a.Name, "update input: name mismatch") + assert.Equalf(t, *i.Name, a.Name, "update input: name mismatch") } if i.Description != nil { assert.Equalf(t, lo.FromPtrOr(i.Description, ""), lo.FromPtrOr(a.Description, ""), "update input: description mismatch") } if i.Metadata != nil { - assert.Equalf(t, *i.Metadata, a.Metadata, "metadata mismatch") + assert.Equalf(t, *i.Metadata, a.Metadata, "update input: metadata mismatch") } if i.Annotations != nil { - assert.Equalf(t, *i.Annotations, a.Annotations, "annotations mismatch") + assert.Equalf(t, *i.Annotations, a.Annotations, "update input: annotations mismatch") }
54-63: Missing RateCards comparison and namespace check.This function doesn't compare
RateCardsor check theNamespacefield, which is inconsistent with the other assertion functions. Consider either adding these comparisons or adding a comment explaining why they're intentionally omitted.func AssertAddonEqual(t *testing.T, expected, actual Addon) { t.Helper() + assert.Equalf(t, expected.Namespace, actual.Namespace, "namespace mismatch") assert.Equalf(t, expected.Key, actual.Key, "key mismatch") assert.Equalf(t, expected.Name, actual.Name, "name mismatch") assert.Equalf(t, expected.Description, actual.Description, "description mismatch") assert.Equalf(t, expected.Currency, actual.Currency, "currency mismatch") assert.Equalf(t, expected.Metadata, actual.Metadata, "metadata mismatch") assert.Equalf(t, expected.Annotations, actual.Annotations, "annotations mismatch") + + // Compare RateCards if needed + AssertAddonRateCardsEqual(t, expected.RateCards, actual.RateCards) }
65-102: Improve the map creation and error messages for better readability.The current implementation uses immediately-invoked function expressions which are unusual in Go, and the error messages for missing keys don't indicate which key is missing.
func AssertAddonRateCardsEqual(t *testing.T, r1, r2 productcatalog.RateCards) { t.Helper() assert.Equalf(t, len(r1), len(r2), "number of RateCards mismatch") - r1Map := func() map[string]productcatalog.RateCard { - m := make(map[string]productcatalog.RateCard, len(r1)) - for _, v := range r1 { - m[v.Key()] = v - } - - return m - }() + r1Map := make(map[string]productcatalog.RateCard, len(r1)) + for _, v := range r1 { + r1Map[v.Key()] = v + } - r2Map := func() map[string]productcatalog.RateCard { - m := make(map[string]productcatalog.RateCard, len(r2)) - for _, v := range r2 { - m[v.Key()] = v - } - - return m - }() + r2Map := make(map[string]productcatalog.RateCard, len(r2)) + for _, v := range r2 { + r2Map[v.Key()] = v + } visited := make(map[string]struct{}) for phase1Key, rateCard1 := range r1Map { rateCard2, ok := r2Map[phase1Key] - require.Truef(t, ok, "missing RateCard key") + require.Truef(t, ok, "missing RateCard key %s in second slice", phase1Key) AssertRateCardEqual(t, rateCard1, rateCard2) visited[phase1Key] = struct{}{} } for phase2Key := range r2Map { _, ok := visited[phase2Key] - require.Truef(t, ok, "missing RateCard key") + require.Truef(t, ok, "missing RateCard key %s in first slice", phase2Key) } }
104-155: Add default case to type switches and consider refactoring to reduce duplication.The current implementation doesn't handle unexpected RateCard types, and duplicates the type switch logic for both rate cards.
func AssertRateCardEqual(t *testing.T, r1, r2 productcatalog.RateCard) { t.Helper() assert.Equalf(t, r1.Type(), r2.Type(), "type mismatch") m1 := r1.AsMeta() m2 := r2.AsMeta() assert.Equalf(t, m1.Key, m2.Key, "key mismatch") assert.Equalf(t, m1.Name, m2.Name, "name mismatch") assert.Equalf(t, lo.FromPtr(m1.Description), lo.FromPtr(m2.Description), "description mismatch") assert.Truef(t, m1.Metadata.Equal(m2.Metadata), "metadata mismatch") f1 := lo.FromPtr(m1.Feature) f2 := lo.FromPtr(m2.Feature) assert.Equalf(t, f1.Key, f2.Key, "feature key mismatch") assert.Equalf(t, f1.Namespace, f2.Namespace, "feature namespace mismatch") assert.Truef(t, m1.EntitlementTemplate.Equal(m2.EntitlementTemplate), "entitlement template mismatch") assert.Truef(t, m1.TaxConfig.Equal(m2.TaxConfig), "tax config mismatch") assert.Truef(t, m1.Price.Equal(m2.Price), "price mismatch") - var billingCadence1, billingCadence2 isodate.Period - - switch vv := r1.(type) { - case *productcatalog.FlatFeeRateCard: - billingCadence1 = lo.FromPtr(vv.BillingCadence) - case *FlatFeeRateCard: - billingCadence1 = lo.FromPtr(vv.FlatFeeRateCard.BillingCadence) - case *productcatalog.UsageBasedRateCard: - billingCadence1 = vv.BillingCadence - case *UsageBasedRateCard: - billingCadence1 = vv.UsageBasedRateCard.BillingCadence - } - - switch vv := r2.(type) { - case *productcatalog.FlatFeeRateCard: - billingCadence2 = lo.FromPtr(vv.BillingCadence) - case *FlatFeeRateCard: - billingCadence2 = lo.FromPtr(vv.FlatFeeRateCard.BillingCadence) - case *productcatalog.UsageBasedRateCard: - billingCadence2 = vv.BillingCadence - case *UsageBasedRateCard: - billingCadence2 = vv.UsageBasedRateCard.BillingCadence - } - - assert.Equal(t, billingCadence1, billingCadence2) + // Helper function to extract billing cadence from a RateCard + getBillingCadence := func(r productcatalog.RateCard) (isodate.Period, bool) { + switch vv := r.(type) { + case *productcatalog.FlatFeeRateCard: + return lo.FromPtr(vv.BillingCadence), true + case *FlatFeeRateCard: + return lo.FromPtr(vv.FlatFeeRateCard.BillingCadence), true + case *productcatalog.UsageBasedRateCard: + return vv.BillingCadence, true + case *UsageBasedRateCard: + return vv.UsageBasedRateCard.BillingCadence, true + default: + return isodate.Period{}, false + } + } + + billingCadence1, ok1 := getBillingCadence(r1) + billingCadence2, ok2 := getBillingCadence(r2) + + require.Truef(t, ok1, "unsupported RateCard type for first argument: %T", r1) + require.Truef(t, ok2, "unsupported RateCard type for second argument: %T", r2) + assert.Equal(t, billingCadence1, billingCadence2, "billing cadence mismatch") }openmeter/ent/db/mutation.go (2)
29512-29565: Managing 'addon_ratecard' in FeatureMutation.
- The new methods
AddAddonRatecardIDs,RemoveAddonRatecardIDs, andClearAddonRatecardfollow the ent mutation handling pattern.- Ensure adequate test coverage to confirm correct handling of add/remove vs. clear operations.
Consider adding logging or debug statements that clarify the difference between removing edge IDs and clearing the edge entirely.
29914-29920: Handling removed 'addon_ratecard' edge references.
These lines extend the existing removal logic for theFeatureMutation, allowing for explicit removal from the relationship. This is standard ent usage.A brief explanatory comment might help future maintainers distinguish between removing specific IDs vs. clearing the whole edge.
Also applies to: 29933-29935
openmeter/productcatalog/addon/event.go (5)
56-64: Redundant error slice usage inAddonCreateEvent.Validate().
Currently,errsis declared but never appended to. Either remove the slice or append any validation errors to it before joining.func (e AddonCreateEvent) Validate() error { - var errs []error if e.Addon == nil { return fmt.Errorf("add-on is required") } - return errors.Join(errs...) + return nil }
99-107: Redundant error slice usage inAddonUpdateEvent.Validate().
Same pattern asAddonCreateEvent.Validate().func (e AddonUpdateEvent) Validate() error { - var errs []error if e.Addon == nil { return fmt.Errorf("add-on is required") } - return errors.Join(errs...) + return nil }
142-154: Redundant error slice usage inAddonDeleteEvent.Validate().
Although this method performs additional checks onDeletedAt, it still does not use the declared errors slice.func (e AddonDeleteEvent) Validate() error { - var errs []error if e.Addon == nil { return fmt.Errorf("add-on is required") } if e.Addon.DeletedAt == nil { return fmt.Errorf("add-on deleted at is required") } - return errors.Join(errs...) + return nil }
189-197: Redundant error slice usage inAddonPublishEvent.Validate().
Remove unusederrsor consider appending errors if needed.
232-240: Redundant error slice usage inAddonArchiveEvent.Validate().
Same feedback regarding the unused error slice.openmeter/productcatalog/addon/httpdriver/errors.go (1)
12-16: Consider expanding error handling for addon-specific errors.The error encoder currently only handles feature-not-found errors. As this is a new addon API, consider expanding the error handling to include addon-specific errors that might occur during API operations (e.g., addon not found, invalid addon configuration, etc.).
func errorEncoder() encoder.ErrorEncoder { return func(ctx context.Context, err error, w http.ResponseWriter, r *http.Request) bool { - return commonhttp.HandleErrorIfTypeMatches[*feature.FeatureNotFoundError](ctx, http.StatusBadRequest, err, w) + return commonhttp.HandleErrorIfTypeMatches[*feature.FeatureNotFoundError](ctx, http.StatusBadRequest, err, w) || + commonhttp.HandleErrorIfTypeMatches[*AddonNotFoundError](ctx, http.StatusNotFound, err, w) || + commonhttp.HandleErrorIfTypeMatches[*InvalidAddonConfigError](ctx, http.StatusBadRequest, err, w) } }openmeter/productcatalog/addon/httpdriver/driver.go (3)
14-16: Provide a brief interface overview.Adding a short docstring for the
Handlerinterface would help other developers quickly grasp the overall responsibilities and usage context.
18-26: Evaluate interface granularity.
AddonHandlerhas several methods, all related to add-on lifecycle operations. Consider splitting them based on operations (read vs. write) or responsibilities to adhere to Interface Segregation Principle if the interface grows further.
36-43: Improve error context inresolveNamespace.Currently, the returned error is quite generic. Including details about missing or invalid namespaces in the error message might aid troubleshooting.
openmeter/ent/db/feature_create.go (1)
173-186: Defensive checks inAddAddonRatecard.Within the loop extracting IDs from
AddonRateCardpointers, consider gracefully handling any unexpected nil references to avoid potential runtime panics.cmd/server/wire_gen.go (2)
161-162: Consider validations forappsConfiguration.Before initializing
appService, ensureappsConfigurationhas valid data. Current error checks seem fine but defensive checks can reduce runtime surprises.
186-187: Progress manager concurrency.Initialization is sound. Double-check concurrency handling if multiple goroutines invoke progress manager methods simultaneously.
openmeter/productcatalog/addon/errors.go (1)
19-49: Consider simplifying message building.
Using multipleifstatements for constructing the error string is clear, but astrings.Builderor a small helper could streamline message assembly and avoid repeatedm += .... This is only a minor style preference.openmeter/productcatalog/addon/adapter/adapter.go (2)
19-40: Leverage extended validation if necessary.
Validate()checks only fornilreferences. If more nuanced checks (e.g., verifying DB connectivity) are needed, consider extending or clarifying validation.
72-83: Transactional continuity is well-handled.
WithTx()returns a newadapterinstance bound to the existing transaction driver, andSelf()provides a straightforward way for code to reference the current adapter. Consider adding doc comments onSelf()for clarity, although it’s self-explanatory.openmeter/productcatalog/addon/adapter/mapping.go (2)
60-61: Rename or clarify error message to reflect 'Add-on' instead of 'plan phase'.The error message here references a "plan phase" rather than an Add-on, which can be confusing in this context. Consider updating it to more closely describe the actual entity being validated.
- return nil, fmt.Errorf("invalid plan phase %s: %w", aa.ID, err) + return nil, fmt.Errorf("invalid addon %s: %w", aa.ID, err)
67-146: Consider refactoring thefromAddonRateCardRowfunction for readability.The function contains several distinct responsibilities, such as parsing metadata, converting billing cadence, and building multiple rate card types. Consider extracting these steps into smaller helper functions to improve readability and maintainability.
openmeter/productcatalog/addon/ratecard_test.go (1)
19-174: Add test coverage for a missing or nilFeaturefield.The JSON round-trip tests currently assume that a
Featureobject is present. Adding a test case with a nil or absentFeaturewould help ensure serialization and deserialization handle this scenario correctly.openmeter/productcatalog/addon/adapter/adapter_test.go (2)
195-196: Consider renaming references to "plan".Within this test block, the variable name and logs reference "plan" instead of "addon", which may create confusion. Rename these references to keep the naming consistent with "addon".
-assert.NoErrorf(t, err, "getting plan by key and version must not fail") -assert.NotNilf(t, getAddonV1, "plan must not be nil") +assert.NoErrorf(t, err, "getting add-on by key and version must not fail") +assert.NotNilf(t, getAddonV1, "add-on must not be nil")
361-361: Rename function for clarity.The helper function is called
testListPlanStatusFilter, but it concerns add-on status filtering. Rename it to maintain clarity and consistency.-func testListPlanStatusFilter(ctx context.Context, t *testing.T, repo *adapter) { +func testListAddonStatusFilter(ctx context.Context, t *testing.T, repo *adapter) {openmeter/productcatalog/addon/httpdriver/addon.go (3)
75-78: Fix the typo in the error message.The word "pahse" seems like a typo. Consider correcting it to "phase."
return ListAddonsResponse{}, fmt.Errorf("failed to cast add-on pahse: %w", err) +return ListAddonsResponse{}, fmt.Errorf("failed to cast add-on phase: %w", err)
323-324: Use consistent type in JSON response encoder.Currently,
PublishAddonusescommonhttp.JSONResponseEncoderWithStatus[GetAddonResponse]instead ofPublishAddonResponse(aliased toapi.Addon). Consider updating for clarity and consistent usage of the response type.-commonhttp.JSONResponseEncoderWithStatus[GetAddonResponse](http.StatusOK), +commonhttp.JSONResponseEncoderWithStatus[PublishAddonResponse](http.StatusOK),
367-368: Align type in JSON response encoder for archiving.Similar to the Publish handler,
ArchiveAddoncurrently usescommonhttp.JSONResponseEncoderWithStatus[GetAddonResponse]. Switch toArchiveAddonResponsefor consistency.-commonhttp.JSONResponseEncoderWithStatus[GetAddonResponse](http.StatusOK), +commonhttp.JSONResponseEncoderWithStatus[ArchiveAddonResponse](http.StatusOK),openmeter/productcatalog/addon.go (1)
153-156: Avoid repeated description checks.The description equality check appears twice, at lines 153–156 and again at 170–172, which is redundant and could introduce confusion or inconsistent future modifications. Consolidate into a single check.
--- a/addon.go +++ b/addon.go @@ -152,4 +152,0 @@ - if lo.FromPtr(m.Description) != lo.FromPtr(v.Description) { - return false - } ... - if lo.FromPtr(m.Description) != lo.FromPtr(v.Description) { - return false - }Also applies to: 170-172
openmeter/productcatalog/addon/adapter/addon.go (2)
24-152: Consider splitting the query-building logic into smaller functions.The
ListAddonsmethod merges multiple filtering paths and pagination in a single, relatively large function body. Splitting out pieces of the query-building—for instance, constructing predicates for IDs, keys, statuses, or currencies in helper functions—can make the code more modular and easier to maintain.
401-504: Recreating ratecards on update introduces a potential gap.Deleting and recreating ratecards can briefly remove the link between the add-on and its ratecards within a transaction. Because it’s wrapped in
TransactingRepo, it’s likely safe from partial updates. However, for high-traffic scenarios, consider a more granular update approach to minimize churn, if relevant.openmeter/ent/db/feature_query.go (2)
26-37: AddedwithAddonRatecardfield in FeatureQuery struct.This new pointer to an
AddonRateCardQueryextends the struct’s capabilities to load or filterAddonRateCardrelationships. Ensure that the rest of the code consistently checks this field to avoid nil pointer dereferences when it’s not set.
359-368: Use ofWithAddonRatecardfor eager loading.Providing a functional option slice to configure the subquery is flexible. This pattern is consistent with Ent’s typical approach. Keep in mind that large eager loads might impact performance if many features reference numerous addon ratecards.
openmeter/productcatalog/addon/service.go (5)
25-83:ListAddonsInputvalidation.While the validation method is currently empty, consider adding checks for invalid page parameters, misformatted currency codes, or contradictory filters. This will help catch early user input errors before hitting deeper logic in your repositories.
85-95:ListAddonsStatusFilterstruct is overshadowed.Currently,
ListAddonsStatusFilteris not referenced insideListAddonsInput, and a separateStatus []productcatalog.AddonStatusis used. Clarify whether both are needed, or unify them to simplify the filter approach.
122-196:UpdateAddonInputlogic and partial updates.
- The
Equalmethod helps check if any meaningful fields have changed. This is a neat approach to skip unnecessary DB updates.- The
Validatelogic is robust, ensuringNameand effective periods are correct.- Consider if you need more checks for
RateCardsreferences here, or if that is delegated solely to the repository layer.
198-230:GetAddonInputgracefully handles ID or Key usage.Requiring at least one of
IDorKeyis sensible. However, if a user provides both, consider clarifying which field has precedence or whether that should be disallowed. This can prevent unexpected results in ambiguous queries.
316-341:NextAddonInputfor rolling versions.This method strikes a balance: reusing the same ID or key logic as in
GetAddonInput, but serving the context of “finding the next version.” Consider clarifying usage scenarios in doc comments if they differ significantly fromGetAddonInput.openmeter/ent/db/addonratecard.go (1)
69-70: Clarify edge naming for the “features” field.
The edge is labeled “features” but is mapped to a singleFeaturepointer, which may confuse developers into expecting multiple features. Consider renaming it to “feature” or returning a slice ofFeatureobjects if multiples are intended.openmeter/productcatalog/addon/service/addon.go (2)
21-21: Replace “plan” references with “add-on” for consistency.
Multiple lines still reference “plan” instead of “add-on,” causing naming inconsistencies. Renaming these references will make the code more readable and consistent with the actual domain concept.Apply this diff to fix naming in doc strings and log messages:
--- a/original +++ b/modified @@ -21 +21 @@ - return pagination.PagedResponse[addon.Addon]{}, fmt.Errorf("invalid list Plans params: %w", err) + return pagination.PagedResponse[addon.Addon]{}, fmt.Errorf("invalid list Addons params: %w", err) @@ -108,5 +108,5 @@ - // * check their statuses to ensure that new plan with the same key is created only - // if there is no version in draft status - // * calculate the version number for the new Plan + // * check their statuses to ensure that new add-on with the same key is created only + // if there is no version in draft status + // * calculate the version number for the new add-on @@ -142 +142 @@ - // Emit plan created event + // Emit add-on created event @@ -165 +165 @@ - logger.Debug("deleting Plan") + logger.Debug("deleting add-on") @@ -211 +211 @@ - // Emit plan deleted event + // Emit add-on deleted event @@ -265 +265 @@ - logger.Debug("updating Plan") + logger.Debug("updating add-on") @@ -305 +305 @@ - // Emit plan updated event + // Emit add-on updated event @@ -475 +475 @@ - return nil, fmt.Errorf("failed to publish plan archived event: %w", err) + return nil, fmt.Errorf("failed to publish add-on archived event: %w", err)Also applies to: 108-112, 142-142, 165-165, 211-211, 265-265, 305-305, 475-475
108-124: Consider enforcing uniqueness for draft versions to avoid concurrency issues.
When multiple processes create new add-on versions concurrently, there is a risk of multiple draft versions being created. Consider a unique constraint or a database lock to enforce the “only one draft version” policy consistently.openmeter/productcatalog/addon/ratecard.go (1)
23-24: Remove or update the outdated “PhaseID” comment.
The comment on line 23 refers to “PhaseID,” but the code field is named “AddonID.” Renaming or removing this comment will remove confusion.- // PhaseID + // AddonIDopenmeter/productcatalog/addon/httpdriver/mapping.go (2)
66-202: Consider splitting up the large rate card mapping
This function handles multiple branches for different rate card types; refactoring into smaller, focused helpers could improve readability and make future changes less error-prone.
982-1055: Clarify usagePeriod fallback logic
Falling back to billingCadence when usagePeriod is nil may lead to unexpected behavior in certain scenarios. Re-evaluate whether additional configuration or explicit validation is needed.openmeter/ent/db/addon/addon.go (1)
85-109: Validators and Defaults SetupThe validator and default function patterns (e.g.,
NamespaceValidator,DefaultCreatedAt) are standard for Ent. Ensure you have relevant tests or integration checks to confirm each validator’s correctness. For example, verifying currency values or version bounds would help catch potential bad inputs during runtime.openmeter/ent/db/addonratecard/addonratecard.go (1)
132-140:TypeValidatorEnum ChecksUsing an explicit check for legitimate rate card types (
"flat_fee","usage_based") helps ensure data integrity. If you plan to add new rate card types later, remember to update this validator method. This is a good place to consider test coverage for negative test cases, ensuring it rejects unknown types.openmeter/ent/db/addonratecard_query.go (2)
111-131: Ensure proper handling of theFirstXpanic path.Methods like
FirstXautomatically panic on errors. While this can be convenient, consider whether you need more graceful error handling to prevent abrupt terminations in production environments.
211-235: Evaluate logging or instrumentation for bulk fetch operations.The
AllandAllXmethods can potentially return large result sets. Consider adding debug logging, metrics instrumentation, or rate limiting if these calls are expected to influence performance at scale.openmeter/ent/db/addon_query.go (1)
287-309: Consider user-defined validators onWithRatecards.Eager loading can introduce performance overhead. If you anticipate large volumes of
AddonRateCardentries, consider implementing constraints and validations (e.g., limiting queries, applying pagination) to maintain system responsiveness.openmeter/ent/db/addonratecard_update.go (1)
917-974: Assess partial updates logic forfeaturesedge.When updating the
Featureassociation, partial updates (i.e., clearing and adding) can lead to potential data integrity issues if multiple rate cards share a feature. Ensure you have the correct locking or transactional strategies to prevent conflicts.openmeter/ent/db/addon_update.go (1)
218-232: Consider implementing optimistic concurrency checks.If concurrent updates to add-ons might happen, an optimistic concurrency control (checking if
versionchanged between read and update) could help prevent overwriting each other's changes.openmeter/ent/db/addon_create.go (3)
223-241: Validate theaddon.DefaultID()uniqueness approach.While setting a custom default ID is fine, confirm that the generated IDs won't collide at scale. A UUID-based approach might provide safer uniqueness in high-volume scenarios.
400-421: Be mindful of database support forON CONFLICT.Different databases have varying levels of support for
DO NOTHINGand upsert logic. Confirm the chosen dialect fully supports these conflict resolution strategies.
872-961: Bulk creation is convenient, but consider partial success handling.When using
AddonCreateBulk, a single error rolls back the entire batch. If partial success is desired, you might need a different strategy (e.g., a custom transaction per entity).openmeter/ent/db/addonratecard_create.go (1)
451-467: M2O edge handling may discard extra IDs.
Currently, if multiple Addon or Feature IDs are set (e.g., through mutation.AddonIDs() or mutation.FeaturesIDs()), only the first element is used and stored. This is standard Ent behavior for M2O edges, but it could lead to confusion if multiple IDs are inadvertently passed.You may want to return an explicit error if more than one ID is provided for a single M2O edge to avoid silent discarding of extra IDs.
Also applies to: 468-484
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (58)
app/common/productcatalog.go(4 hunks)cmd/server/main.go(1 hunks)cmd/server/wire.go(2 hunks)cmd/server/wire_gen.go(14 hunks)openmeter/ent/db/addon.go(1 hunks)openmeter/ent/db/addon/addon.go(1 hunks)openmeter/ent/db/addon/where.go(1 hunks)openmeter/ent/db/addon_create.go(1 hunks)openmeter/ent/db/addon_delete.go(1 hunks)openmeter/ent/db/addon_query.go(1 hunks)openmeter/ent/db/addon_update.go(1 hunks)openmeter/ent/db/addonratecard.go(1 hunks)openmeter/ent/db/addonratecard/addonratecard.go(1 hunks)openmeter/ent/db/addonratecard/where.go(1 hunks)openmeter/ent/db/addonratecard_create.go(1 hunks)openmeter/ent/db/addonratecard_delete.go(1 hunks)openmeter/ent/db/addonratecard_query.go(1 hunks)openmeter/ent/db/addonratecard_update.go(1 hunks)openmeter/ent/db/client.go(13 hunks)openmeter/ent/db/ent.go(2 hunks)openmeter/ent/db/expose.go(1 hunks)openmeter/ent/db/feature.go(3 hunks)openmeter/ent/db/feature/feature.go(4 hunks)openmeter/ent/db/feature/where.go(1 hunks)openmeter/ent/db/feature_create.go(3 hunks)openmeter/ent/db/feature_query.go(8 hunks)openmeter/ent/db/feature_update.go(7 hunks)openmeter/ent/db/hook/hook.go(1 hunks)openmeter/ent/db/migrate/schema.go(3 hunks)openmeter/ent/db/mutation.go(10 hunks)openmeter/ent/db/paginate.go(1 hunks)openmeter/ent/db/predicate/predicate.go(1 hunks)openmeter/ent/db/runtime.go(2 hunks)openmeter/ent/db/setorclear.go(1 hunks)openmeter/ent/db/tx.go(3 hunks)openmeter/ent/schema/addon.go(1 hunks)openmeter/ent/schema/feature.go(1 hunks)openmeter/event/metadata/resourcepath.go(1 hunks)openmeter/productcatalog/addon.go(1 hunks)openmeter/productcatalog/addon/adapter/adapter.go(1 hunks)openmeter/productcatalog/addon/adapter/adapter_test.go(1 hunks)openmeter/productcatalog/addon/adapter/addon.go(1 hunks)openmeter/productcatalog/addon/adapter/mapping.go(1 hunks)openmeter/productcatalog/addon/addon.go(1 hunks)openmeter/productcatalog/addon/assert.go(1 hunks)openmeter/productcatalog/addon/errors.go(1 hunks)openmeter/productcatalog/addon/errors_test.go(1 hunks)openmeter/productcatalog/addon/event.go(1 hunks)openmeter/productcatalog/addon/httpdriver/addon.go(1 hunks)openmeter/productcatalog/addon/httpdriver/driver.go(1 hunks)openmeter/productcatalog/addon/httpdriver/errors.go(1 hunks)openmeter/productcatalog/addon/httpdriver/mapping.go(1 hunks)openmeter/productcatalog/addon/ratecard.go(1 hunks)openmeter/productcatalog/addon/ratecard_test.go(1 hunks)openmeter/productcatalog/addon/repository.go(1 hunks)openmeter/productcatalog/addon/service.go(1 hunks)openmeter/productcatalog/addon/service/addon.go(1 hunks)openmeter/productcatalog/addon/service/service.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (40)
openmeter/productcatalog/addon/service/service.go (3)
openmeter/productcatalog/addon/repository.go (1)
Repository(12-20)openmeter/productcatalog/addon/adapter/adapter.go (2)
New(42-51)Config(19-22)openmeter/productcatalog/addon/service.go (1)
Service(35-44)
openmeter/productcatalog/addon/repository.go (3)
openmeter/productcatalog/addon/service.go (5)
ListAddonsInput(48-79)CreateAddonInput(98-101)DeleteAddonInput(231-233)GetAddonInput(198-211)UpdateAddonInput(122-142)openmeter/productcatalog/addon.go (1)
Addon(181-186)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)
openmeter/ent/db/feature/where.go (3)
openmeter/ent/db/predicate/predicate.go (2)
Feature(120-120)AddonRateCard(24-24)openmeter/ent/db/addonratecard/addonratecard.go (1)
Table(59-59)openmeter/ent/db/feature/feature.go (3)
Table(44-44)AddonRatecardTable(60-60)AddonRatecardColumn(65-65)
openmeter/ent/db/feature.go (3)
openmeter/ent/db/addonratecard.go (2)
AddonRateCard(21-63)AddonRateCard(99-122)openmeter/ent/db/ent.go (1)
NotLoadedError(316-318)openmeter/ent/db/addonratecard_query.go (1)
AddonRateCardQuery(22-34)
openmeter/ent/db/ent.go (2)
openmeter/ent/db/addon/addon.go (2)
Table(47-47)ValidColumn(76-83)openmeter/ent/db/addonratecard/addonratecard.go (2)
Table(59-59)ValidColumn(99-106)
openmeter/ent/db/tx.go (4)
openmeter/ent/db/predicate/predicate.go (2)
Addon(10-10)AddonRateCard(24-24)openmeter/ent/db/addon.go (2)
Addon(17-51)Addon(72-91)openmeter/ent/db/client.go (3)
AddonClient(511-513)AddonRateCardClient(660-662)NewAddonClient(516-518)openmeter/ent/db/addonratecard.go (2)
AddonRateCard(21-63)AddonRateCard(99-122)
openmeter/productcatalog/addon/httpdriver/driver.go (1)
openmeter/productcatalog/addon/httpdriver/addon.go (7)
ListAddonsHandler(28-28)CreateAddonHandler(102-102)DeleteAddonHandler(197-197)GetAddonHandler(247-247)UpdateAddonHandler(149-149)PublishAddonHandler(291-291)ArchiveAddonHandler(336-336)
openmeter/ent/db/expose.go (2)
openmeter/ent/schema/addon.go (10)
Addon(17-19)Addon(21-25)Addon(27-49)Addon(51-58)Addon(60-75)AddonRateCard(77-79)AddonRateCard(81-85)AddonRateCard(87-139)AddonRateCard(141-153)AddonRateCard(155-168)openmeter/ent/db/client.go (2)
NewAddonClient(516-518)NewAddonRateCardClient(665-667)
openmeter/ent/db/feature_create.go (3)
openmeter/ent/db/addonratecard/where.go (1)
ID(16-18)openmeter/ent/db/addonratecard/addonratecard.go (2)
Table(59-59)FieldID(19-19)openmeter/ent/db/feature/feature.go (3)
Table(44-44)AddonRatecardColumn(65-65)FieldID(16-16)
openmeter/ent/db/paginate.go (5)
openmeter/ent/db/addon_query.go (1)
AddonQuery(22-33)openmeter/ent/db/predicate/predicate.go (2)
Addon(10-10)AddonRateCard(24-24)openmeter/ent/db/addon.go (2)
Addon(17-51)Addon(72-91)openmeter/ent/db/addonratecard_query.go (1)
AddonRateCardQuery(22-34)openmeter/ent/db/addonratecard.go (2)
AddonRateCard(21-63)AddonRateCard(99-122)
openmeter/ent/db/runtime.go (2)
openmeter/ent/db/addon.go (2)
Addon(17-51)Addon(72-91)openmeter/ent/db/addonratecard.go (2)
AddonRateCard(21-63)AddonRateCard(99-122)
openmeter/productcatalog/addon/adapter/adapter.go (2)
openmeter/productcatalog/addon/service/service.go (2)
Config(12-17)New(19-42)openmeter/productcatalog/addon/repository.go (1)
Repository(12-20)
openmeter/ent/db/feature/feature.go (2)
openmeter/ent/db/addon/addon.go (1)
Table(47-47)openmeter/ent/db/addonratecard/addonratecard.go (1)
Table(59-59)
cmd/server/wire_gen.go (5)
openmeter/productcatalog/feature/connector.go (1)
NewFeatureConnector(84-100)app/common/progressmanager.go (2)
ProgressManager(14-16)NewProgressManager(19-39)app/common/streaming.go (1)
NewStreamingConnector(21-43)app/common/entitlement.go (1)
NewEntitlementRegistry(22-40)openmeter/app/service/service.go (1)
Service(12-15)
openmeter/productcatalog/addon/adapter/mapping.go (4)
openmeter/productcatalog/addon.go (3)
Addon(181-186)AddonMeta(94-117)AddonEffectivePeriod(38-44)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)openmeter/productcatalog/ratecard.go (5)
RateCards(350-350)RateCard(28-38)RateCardMeta(49-77)FlatFeeRateCard(161-168)UsageBasedRateCard(256-262)openmeter/productcatalog/addon/ratecard.go (4)
RateCardManagedFields(19-25)FlatFeeRateCard(102-105)UsageBasedRateCard(200-203)ManagedRateCard(57-59)
openmeter/ent/db/hook/hook.go (1)
openmeter/ent/db/mutation.go (2)
AddonMutation(112-138)AddonRateCardMutation(1338-1366)
openmeter/ent/schema/addon.go (4)
openmeter/ent/db/addon.go (2)
Addon(17-51)Addon(72-91)openmeter/ent/db/predicate/predicate.go (3)
Addon(10-10)AddonRateCard(24-24)Feature(120-120)openmeter/ent/db/addonratecard.go (2)
AddonRateCard(21-63)AddonRateCard(99-122)openmeter/ent/schema/feature.go (5)
Feature(13-15)Feature(17-23)Feature(25-34)Feature(36-45)Feature(47-59)
openmeter/ent/db/migrate/schema.go (2)
openmeter/ent/db/addon/addon.go (2)
Table(47-47)Columns(58-73)openmeter/ent/db/addonratecard/addonratecard.go (2)
Table(59-59)Columns(77-96)
openmeter/productcatalog/addon/assert.go (4)
openmeter/productcatalog/addon/service.go (2)
CreateAddonInput(98-101)UpdateAddonInput(122-142)openmeter/productcatalog/addon.go (1)
Addon(181-186)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)openmeter/productcatalog/ratecard.go (4)
RateCards(350-350)RateCard(28-38)FlatFeeRateCard(161-168)UsageBasedRateCard(256-262)
openmeter/productcatalog/addon/adapter/adapter_test.go (6)
openmeter/productcatalog/addon/service.go (4)
CreateAddonInput(98-101)GetAddonInput(198-211)ListAddonsInput(48-79)UpdateAddonInput(122-142)openmeter/productcatalog/addon.go (4)
Addon(181-186)AddonMeta(94-117)AddonEffectivePeriod(38-44)AddonStatus(22-22)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)openmeter/productcatalog/addon/ratecard.go (3)
UsageBasedRateCard(200-203)RateCardManagedFields(19-25)FlatFeeRateCard(102-105)openmeter/productcatalog/addon/service/service_test.go (1)
MonthPeriod(304-304)openmeter/productcatalog/addon/assert.go (3)
AssertAddonCreateInputEqual(14-26)AssertAddonEqual(54-63)AssertAddonUpdateInputEqual(28-52)
openmeter/ent/db/predicate/predicate.go (2)
openmeter/ent/db/addon.go (2)
Addon(17-51)Addon(72-91)openmeter/ent/db/addonratecard.go (2)
AddonRateCard(21-63)AddonRateCard(99-122)
openmeter/ent/db/addon_delete.go (5)
openmeter/ent/db/ent.go (3)
IsConstraintError(353-359)ConstraintError(337-340)NotFoundError(270-272)openmeter/ent/db/mutation.go (1)
AddonMutation(112-138)openmeter/ent/db/predicate/predicate.go (1)
Addon(10-10)openmeter/ent/schema/addon.go (5)
Addon(17-19)Addon(21-25)Addon(27-49)Addon(51-58)Addon(60-75)openmeter/ent/db/addon/addon.go (3)
Table(47-47)FieldID(17-17)Label(15-15)
openmeter/productcatalog/addon.go (2)
openmeter/productcatalog/ratecard.go (2)
RateCards(350-350)RateCard(28-38)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)
openmeter/ent/db/feature_query.go (8)
openmeter/ent/db/addonratecard/addonratecard.go (4)
OrderOption(143-143)Table(59-59)FieldID(19-19)FieldFeatureID(51-51)openmeter/ent/db/feature/feature.go (5)
OrderOption(111-111)Table(44-44)FieldID(16-16)AddonRatecardTable(60-60)AddonRatecardColumn(65-65)openmeter/ent/db/predicate/predicate.go (2)
Feature(120-120)AddonRateCard(24-24)openmeter/ent/db/addonratecard_query.go (1)
AddonRateCardQuery(22-34)openmeter/ent/db/client.go (1)
AddonRateCardClient(660-662)openmeter/ent/db/addonratecard.go (2)
AddonRateCard(21-63)AddonRateCard(99-122)openmeter/ent/db/addonratecard/where.go (2)
ID(16-18)FeatureID(122-124)openmeter/ent/db/feature/where.go (1)
ID(14-16)
openmeter/productcatalog/addon/httpdriver/addon.go (1)
openmeter/productcatalog/addon/httpdriver/mapping.go (3)
FromAddon(23-62)AsCreateAddonRequest(478-507)AsUpdateAddonRequest(1071-1092)
openmeter/ent/db/mutation.go (1)
openmeter/ent/db/predicate/predicate.go (2)
Addon(10-10)AddonRateCard(24-24)
openmeter/ent/db/addon.go (7)
openmeter/ent/db/addonratecard.go (2)
AddonRateCard(21-63)AddonRateCard(99-122)openmeter/ent/db/predicate/predicate.go (2)
AddonRateCard(24-24)Addon(10-10)openmeter/ent/db/ent.go (2)
NotLoadedError(316-318)Value(60-60)openmeter/ent/db/feature/feature.go (7)
FieldID(16-16)FieldNamespace(26-26)FieldName(28-28)FieldKey(30-30)FieldCreatedAt(18-18)FieldUpdatedAt(20-20)FieldDeletedAt(22-22)openmeter/ent/db/addonratecard_query.go (1)
AddonRateCardQuery(22-34)openmeter/ent/db/client.go (1)
NewAddonClient(516-518)openmeter/ent/db/addon_update.go (1)
AddonUpdateOne(386-391)
openmeter/ent/db/addonratecard_delete.go (4)
openmeter/ent/db/ent.go (1)
Hook(59-59)openmeter/ent/db/mutation.go (1)
AddonRateCardMutation(1338-1366)openmeter/ent/db/predicate/predicate.go (1)
AddonRateCard(24-24)openmeter/ent/db/addonratecard/addonratecard.go (3)
Table(59-59)FieldID(19-19)Label(17-17)
openmeter/productcatalog/addon/service/addon.go (2)
openmeter/productcatalog/addon/service.go (8)
ListAddonsInput(48-79)CreateAddonInput(98-101)OrderBy(33-33)GetAddonInput(198-211)UpdateAddonInput(122-142)PublishAddonInput(245-250)ArchiveAddonInput(288-294)NextAddonInput(316-326)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)
openmeter/ent/db/addonratecard.go (8)
openmeter/ent/db/addonratecard/where.go (12)
ID(16-18)Namespace(71-73)CreatedAt(76-78)UpdatedAt(81-83)DeletedAt(86-88)Name(91-93)Description(96-98)Key(101-103)FeatureKey(106-108)BillingCadence(111-114)AddonID(117-119)FeatureID(122-124)openmeter/ent/db/ent.go (2)
NotFoundError(270-272)NotLoadedError(316-318)openmeter/ent/db/addonratecard/addonratecard.go (19)
Label(17-17)FieldMetadata(23-23)FieldID(19-19)FieldNamespace(21-21)FieldName(31-31)FieldDescription(33-33)FieldKey(35-35)FieldType(37-37)FieldFeatureKey(39-39)FieldBillingCadence(45-45)FieldAddonID(49-49)FieldFeatureID(51-51)FieldCreatedAt(25-25)FieldUpdatedAt(27-27)FieldDeletedAt(29-29)FieldEntitlementTemplate(41-41)FieldTaxConfig(43-43)FieldPrice(47-47)FieldDiscounts(53-53)openmeter/ent/db/feature/feature.go (9)
Label(14-14)FieldMetadata(24-24)FieldID(16-16)FieldNamespace(26-26)FieldName(28-28)FieldKey(30-30)FieldCreatedAt(18-18)FieldUpdatedAt(20-20)FieldDeletedAt(22-22)openmeter/ent/db/addon_query.go (1)
AddonQuery(22-33)openmeter/ent/db/client.go (1)
NewAddonRateCardClient(665-667)openmeter/ent/db/feature_query.go (1)
FeatureQuery(24-37)openmeter/ent/db/addonratecard_update.go (1)
AddonRateCardUpdateOne(493-498)
openmeter/ent/db/addonratecard/addonratecard.go (3)
openmeter/ent/db/addon/addon.go (20)
FieldID(17-17)FieldNamespace(19-19)FieldMetadata(21-21)FieldCreatedAt(23-23)FieldUpdatedAt(25-25)FieldDeletedAt(27-27)FieldName(29-29)FieldDescription(31-31)FieldKey(33-33)Columns(58-73)ByID(114-116)OrderOption(111-111)ByNamespace(119-121)ByCreatedAt(124-126)ByUpdatedAt(129-131)ByDeletedAt(134-136)ByName(139-141)ByDescription(144-146)ByKey(149-151)Table(47-47)openmeter/ent/db/feature/feature.go (18)
FieldID(16-16)FieldNamespace(26-26)FieldMetadata(24-24)FieldCreatedAt(18-18)FieldUpdatedAt(20-20)FieldDeletedAt(22-22)FieldName(28-28)FieldKey(30-30)Columns(69-81)ByID(114-116)OrderOption(111-111)ByNamespace(134-136)ByCreatedAt(119-121)ByUpdatedAt(124-126)ByDeletedAt(129-131)ByName(139-141)ByKey(144-146)Table(44-44)openmeter/ent/db/migrate/schema.go (1)
FeaturesTable(1306-1330)
openmeter/ent/db/client.go (4)
openmeter/ent/db/addon/where.go (1)
ID(14-16)openmeter/ent/db/addonratecard/where.go (1)
ID(16-18)openmeter/ent/db/addon/addon.go (4)
Table(47-47)FieldID(17-17)RatecardsTable(49-49)RatecardsColumn(54-54)openmeter/ent/db/addonratecard/addonratecard.go (6)
Table(59-59)FieldID(19-19)AddonTable(61-61)AddonColumn(66-66)FeaturesTable(68-68)FeaturesColumn(73-73)
openmeter/ent/db/addonratecard_query.go (4)
openmeter/ent/db/predicate/predicate.go (3)
AddonRateCard(24-24)Addon(10-10)Feature(120-120)openmeter/ent/db/addon_query.go (1)
AddonQuery(22-33)openmeter/ent/db/feature_query.go (1)
FeatureQuery(24-37)openmeter/ent/db/addonratecard/where.go (5)
Name(91-93)AddonID(117-119)IDIn(31-33)ID(16-18)FeatureID(122-124)
openmeter/ent/db/addon_query.go (4)
openmeter/ent/db/predicate/predicate.go (2)
Addon(10-10)AddonRateCard(24-24)openmeter/ent/db/addonratecard_query.go (1)
AddonRateCardQuery(22-34)openmeter/ent/db/client.go (1)
AddonRateCardClient(660-662)openmeter/ent/db/addonratecard.go (2)
AddonRateCard(21-63)AddonRateCard(99-122)
openmeter/ent/db/addon_update.go (3)
openmeter/ent/db/mutation.go (1)
AddonMutation(112-138)openmeter/ent/db/predicate/predicate.go (2)
Addon(10-10)AddonRateCard(24-24)openmeter/ent/db/addonratecard.go (2)
AddonRateCard(21-63)AddonRateCard(99-122)
openmeter/ent/db/addonratecard_update.go (3)
openmeter/ent/db/mutation.go (1)
AddonRateCardMutation(1338-1366)openmeter/ent/db/addonratecard.go (2)
AddonRateCard(21-63)AddonRateCard(99-122)openmeter/ent/db/predicate/predicate.go (3)
AddonRateCard(24-24)Addon(10-10)Feature(120-120)
openmeter/ent/db/addon/where.go (5)
openmeter/ent/db/addonratecard/where.go (19)
ID(16-18)IDEQ(21-23)IDNEQ(26-28)IDIn(31-33)IDNotIn(36-38)IDGT(41-43)IDGTE(46-48)IDLT(51-53)IDLTE(56-58)IDEqualFold(61-63)IDContainsFold(66-68)Namespace(71-73)CreatedAt(76-78)UpdatedAt(81-83)DeletedAt(86-88)Name(91-93)Description(96-98)Key(101-103)Not(972-974)openmeter/ent/db/predicate/predicate.go (2)
Addon(10-10)AddonRateCard(24-24)openmeter/ent/db/addon/addon.go (13)
FieldID(17-17)FieldNamespace(19-19)FieldCreatedAt(23-23)FieldUpdatedAt(25-25)FieldDeletedAt(27-27)FieldName(29-29)FieldDescription(31-31)FieldKey(33-33)FieldVersion(35-35)FieldCurrency(37-37)FieldEffectiveFrom(39-39)FieldEffectiveTo(41-41)FieldMetadata(21-21)openmeter/ent/db/addonratecard/addonratecard.go (9)
FieldID(19-19)FieldNamespace(21-21)FieldCreatedAt(25-25)FieldUpdatedAt(27-27)FieldDeletedAt(29-29)FieldName(31-31)FieldDescription(33-33)FieldKey(35-35)FieldMetadata(23-23)openmeter/ent/db/feature/feature.go (8)
FieldID(16-16)FieldNamespace(26-26)FieldCreatedAt(18-18)FieldUpdatedAt(20-20)FieldDeletedAt(22-22)FieldName(28-28)FieldKey(30-30)FieldMetadata(24-24)
openmeter/ent/db/addon_create.go (3)
openmeter/ent/db/mutation.go (1)
AddonMutation(112-138)openmeter/ent/db/addonratecard.go (2)
AddonRateCard(21-63)AddonRateCard(99-122)openmeter/ent/db/addon.go (2)
Addon(17-51)Addon(72-91)
openmeter/ent/db/addonratecard/where.go (2)
openmeter/ent/db/predicate/predicate.go (3)
AddonRateCard(24-24)Addon(10-10)Feature(120-120)openmeter/ent/db/addonratecard/addonratecard.go (6)
FieldFeatureKey(39-39)FieldBillingCadence(45-45)FieldAddonID(49-49)FieldFeatureID(51-51)FieldPrice(47-47)FieldDiscounts(53-53)
openmeter/ent/db/addonratecard_create.go (2)
openmeter/ent/db/mutation.go (1)
AddonRateCardMutation(1338-1366)openmeter/ent/db/addonratecard.go (2)
AddonRateCard(21-63)AddonRateCard(99-122)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: E2E
- GitHub Check: CI
- GitHub Check: Lint
- GitHub Check: Developer environment
- GitHub Check: Commit hooks
- GitHub Check: Test
- GitHub Check: Analyze (go)
🔇 Additional comments (172)
openmeter/ent/db/expose.go (1)
92-94: New client types for Addon and AddonRateCard properly integratedThe additions follow the established pattern in the codebase for initializing client types in a transaction context. The new fields are properly placed in alphabetical order among the existing client types.
openmeter/ent/db/tx.go (3)
17-20: Properly declared and documented client fieldsThe new fields for Addon and AddonRateCard are appropriately added with proper documentation comments that match the style of existing fields.
220-221: Client initialization properly addedThe initialization of the Addon and AddonRateCard clients follows the established pattern and is correctly integrated in alphabetical order with the other client initializations.
265-265: Documentation example updated to reference new entityGood practice to update the example in the documentation to reference one of the new entities (Addon) rather than leaving the old reference to App.
openmeter/ent/db/ent.go (2)
15-16: New imports for addon-related packagesThe imports for the new addon-related packages are correctly added in alphabetical order.
114-115: Column validation correctly added for new entitiesThe column validation for the new Addon and AddonRateCard entities is properly integrated into the columnCheck map, enabling the same validation functionality that exists for other entities.
openmeter/ent/db/runtime.go (3)
8-9: Imports foraddonandaddonratecardlook fine.These import statements reference the newly introduced schemas. This is consistent with Ent’s generated code, and there are no apparent issues.
55-94: New fields and validators forAddonare correctly set up; proceed with caution for manual edits.This section auto-registers validators, default values, and type scanners for the
Addonschema. Everything here follows the typical Ent pattern. As this file is auto-generated, any direct modifications may be overwritten. Ensure updates happen in the schema definition if behavior changes are required. Also consider adding tests or verifying coverage for all new validators (e.g., currency, version, key) to confirm they behave as intended.
95-137:AddonRateCardschema initialization is consistent with Ent’s auto-generation.Similar to the
Addonsetup, these assignments correctly register validators, default timestamps, ID generation, and type scanners for fields likeEntitlementTemplate,TaxConfig, andPrice. No issues detected. Confirm that the default and validator logic align with your intended business rules (e.g., verifyingaddon_idreferences). Again, any changes should be made in the schema, not here, to avoid being overwritten.openmeter/ent/db/hook/hook.go (2)
12-22: Well implemented adapter for Addon mutation functionsThe implementation follows the established pattern in the file for other entity hook adapters. The
AddonFunctype and itsMutatemethod are correctly implemented to adapt regular functions into entity mutators, with proper type checking and error handling.
24-34: Well implemented adapter for AddonRateCard mutation functionsThe implementation follows the established pattern in the file for other entity hook adapters. The
AddonRateCardFunctype and itsMutatemethod are correctly implemented to adapt regular functions into entity mutators, with proper type checking and error handling.openmeter/productcatalog/addon/addon.go (3)
11-21: Good structure for the Addon entityThe
Addonstruct is well-designed, embedding the necessary base models (NamespacedIDandManagedModel) and implementing themodels.Validatorinterface. The struct correctly includes both the metadata fields and a relationship to rate cards.
23-43: Thorough validation implementationThe validation method properly checks all components of the addon and aggregates errors. It follows the repository's pattern for validation by checking the embedded structs and the addon-specific fields.
45-54: Properly handles deleted addons in conversion methodThe
AsProductCatalogAddonmethod correctly checks if the addon is deleted before converting it, preventing access to deleted addons. This ensures data integrity in the product catalog.openmeter/ent/db/feature/feature.go (3)
41-65: Consistently defined constants for AddonRatecard edgeThe constants for the AddonRatecard edge follow the same naming and documentation patterns as existing edges. The table and column references are properly defined to establish the relationship between Features and AddonRateCards.
186-198: Well-implemented ordering functions for AddonRatecardThe ordering functions
ByAddonRatecardCountandByAddonRatecardfollow the established pattern for entity relationship ordering, creating a consistent API for querying features by their associated addon rate cards.
213-219: Properly defined step function for AddonRatecard relationshipThe
newAddonRatecardStepfunction correctly establishes the one-to-many relationship from Features to AddonRateCards, matching the existing relationship definitions in the file and ensuring proper SQL generation.openmeter/ent/db/migrate/schema.go (6)
12-28: Ensure currency default aligns with multi-currency requirements.
The default"USD"may be suitable now, but consider whether multi-currency usage is anticipated in the product roadmap. If yes, you might want to confirm that this default doesn’t conflict with other regions or future expansions.
29-74: Verify partial index conditions for soft deletion.
These indexes, such as"addon_namespace_key_version"withdeleted_at IS NULL, are common for soft-delete logic. Just confirm they operate correctly across the application code (i.e., no collisions if multiple rows share the same key once older ones are logically deleted).
75-93: Looks good!
Storingdiscountsandpriceas JSONB allows flexibility. The enum"flat_fee" | "usage_based"is clear and easy to extend if more billing types are needed in the future.
96-152: Index design and foreign keys are consistent.
All references, especially the unique constraints on[addon_id, key]and[addon_id, feature_key], appear logically sound for linking toaddonsandfeatures.
2095-2097: Inclusion in the overall schema is correct.
Thanks for addingAddonsTableandAddonRateCardsTabletoTables. Ensure corresponding migration files are properly generated and applied in the correct order.
2138-2139: Foreign key references established properly.
LinkingAddonRateCardsTabletoAddonsTableandFeaturesTablelooks coherent with the intended relationship.openmeter/productcatalog/addon/assert.go (2)
1-12: LGTM: Appropriate imports for testing and assertions.The imports look suitable for a test helper file, using standard testing libraries and relevant internal packages.
14-26: LGTM: Well-structured test helper with clear assertions.The function does a good job of comparing all relevant fields between a
CreateAddonInputand anAddonwith descriptive error messages, and properly callst.Helper()to improve test output.openmeter/ent/db/mutation.go (7)
19-20: Relevant imports for Addon and AddonRateCard.
These additions look correct for referencing the newaddonandaddonratecardentities used in the newly introduced mutation structs.
73-74: New node type constants.
Defining"TypeAddon"and"TypeAddonRateCard"aligns with the existing pattern for ent node type constants in this project.
28867-28869: Added edge-tracking fields in Feature for 'addon_ratecard'.
These new maps and booleans for tracking add/remove/clear states of theaddon_ratecardedge mirror existing patterns for feature edges.
29885-29892: Including the new 'addon_ratecard' edge in FeatureMutation’s AddedEdges.
Listing this edge ensures that ent’s internal builder handles it during mutation. This is consistent with existing logic for other Feature edges.
29955-29961: Edge clearing logic for 'addon_ratecard' in FeatureMutation.
By enumerating the new edge inClearedEdges, the mutation properly reflects when the entire edge is cleared.Also applies to: 29974-29976
29988-29989: Switch case for cleared 'addon_ratecard'.
Supporting the new edge inEdgeClearedensures that ent accurately detects when the relationship is fully cleared.
30012-30014: Resetting the 'addon_ratecard' edge.
Addingm.ResetAddonRatecard()finalizes the ent mutation API for this new relationship, letting developers revert to the initial state as needed.openmeter/productcatalog/addon/event.go (5)
23-29: No issues found forNewAddonCreateEvent.
The helper function correctly initializes the event with theAddonand user ID from context.
66-72: No issues found forNewAddonUpdateEvent.
Similar creation logic as previous events.
109-115: No issues found forNewAddonDeleteEvent.
Code is consistent with other event initializers.
156-162: No issues found forNewAddonPublishEvent.
Implementation is straightforward and matches established patterns.
199-205: No issues found forNewAddonArchiveEvent.
Implementation follows the same pattern, no concerns.openmeter/ent/db/addon/where.go (1)
1-785: Auto-generated file.
This file appears to be generated by Ent. Manual modifications are usually discouraged. No issues found in the generated predicates.openmeter/ent/db/addonratecard/where.go (1)
1-975: Auto-generated file.
Likewise, this file is standard Ent-generated predicate code. No issues found, and no manual edits are recommended.cmd/server/wire.go (2)
29-29: Good addition of the addon package import.The import for the addon package has been properly added to support the new addon service integration.
44-44: Well-structured service integration.The Addon service field has been appropriately added to the Application struct, following the same pattern as other services in the application. This change enables dependency injection of the addon service through the wire framework.
openmeter/event/metadata/resourcepath.go (1)
20-20: Clean addition of EntityAddon constant.The EntityAddon constant is properly added, following the same naming convention as other entity constants. This addition will allow the system to properly identify and handle addon resources in event metadata.
openmeter/ent/schema/feature.go (1)
51-51: Good relationship definition between Feature and AddonRateCard.The edge from Feature to AddonRateCard is properly defined, establishing the necessary relationship between these entities. This will allow features to be associated with addon rate cards in the database schema.
cmd/server/main.go (1)
151-151: LGTM: Properly integrating the Addon service into the router configuration.The addition of the
Addonfield to theRouterConfigstructure follows the established pattern and enables the server to handle addon-related API requests, which aligns with the introduction of the new add-on API.openmeter/ent/db/feature.go (4)
53-54: LGTM: Proper extension of the Feature entity to support addon rate cards.The addition of the
AddonRatecardfield to theFeatureEdgesstruct creates the necessary relationship between features and addon rate cards, enabling features to reference their associated addon rate cards.
57-57: LGTM: Updated array size to accommodate the new edge.The expansion of the
loadedTypesarray from size 2 to 3 is necessary to track loading status for the new edge relationship.
78-85: LGTM: Well-implemented helper method for edge access.The
AddonRatecardOrErrmethod follows the established pattern for edge access methods, providing proper error handling when attempting to access an unloaded edge.
209-212: LGTM: Consistent implementation of the query method.The
QueryAddonRatecardmethod properly delegates to theFeatureClientfor handling queries on the "addon_ratecard" edge, maintaining consistency with other edge query methods in the entity.openmeter/ent/db/feature/where.go (2)
624-633: LGTM: Well-implemented predicate for checking addon rate card relationships.The
HasAddonRatecardfunction correctly implements a predicate that checks for the existence of related addon rate cards, following the established pattern for edge predicates in the codebase.
635-645: LGTM: Enhanced querying capabilities with conditional filtering.The
HasAddonRatecardWithfunction properly extends the basic relationship check by allowing additional filter conditions on the related addon rate cards, enabling more sophisticated queries.openmeter/ent/db/paginate.go (4)
12-55: LGTM: Well-implemented pagination support for addon queries.The
Paginatemethod forAddonQueryfollows the established pagination pattern in the codebase, properly handling counting, offset/limit settings, and returning a consistent response structure. This enables efficient retrieval of addon data with pagination support.
58-58: LGTM: Type safety verification.The type check ensures that
AddonQuerycorrectly implements thepagination.Paginatorinterface, preventing potential compilation issues.
60-103: LGTM: Consistent pagination implementation for addon rate cards.The
Paginatemethod forAddonRateCardQueryproperly mirrors the pagination pattern used throughout the codebase, ensuring consistent behavior across all entity types.
106-106: LGTM: Type safety verification.The type check ensures that
AddonRateCardQuerycorrectly implements thepagination.Paginatorinterface, providing compile-time guarantees.openmeter/productcatalog/addon/service/service.go (4)
1-11: Package structure and imports look good.The imports are well-organized and contain all the necessary dependencies for the service implementation.
12-17: Clear and well-structured configuration struct.The Config struct properly defines all the dependencies required for the addon service, following the dependency injection pattern used elsewhere in the codebase.
19-42: Thorough validation of dependencies in the New function.Good implementation of the factory pattern with proper validation of all required dependencies. The error messages are clear and descriptive.
44-51: Good type assertion and service struct implementation.The
var _ addon.Service = (*service)(nil)assertion ensures the service struct properly implements the interface. The service struct mirrors the Config fields, which is consistent with the approach used elsewhere.openmeter/productcatalog/addon/errors_test.go (1)
11-47: Comprehensive test coverage for the IsNotFound error function.Tests cover all the important scenarios: direct error, wrapped error, and invalid error cases.
openmeter/productcatalog/addon/repository.go (2)
10-10: TODO for future enhancement noted.Good practice to leave a TODO comment for planned bulk API functionality.
12-20: Well-defined Repository interface with appropriate methods.The Repository interface is clearly defined with all necessary CRUD operations, following the same pattern as other repositories in the codebase. The interface extends
entutils.TxCreatorwhich indicates proper transaction support.app/common/productcatalog.go (4)
13-15: New imports added correctly for addon functionality.The required imports for the addon module have been properly added.
26-26: Addon wire set added to ProductCatalog.The Addon module is correctly integrated into the ProductCatalog wire set.
37-39: New Addon wire set defined.The Addon wire set is properly defined following the same pattern as other wire sets in the file.
74-94: Well-implemented NewAddonService factory function.The NewAddonService function follows the same pattern as other service factory functions, properly initializing the adapter and service with appropriate error handling and consistent logging configuration.
openmeter/ent/db/client.go (10)
18-19: Well-organized import additions.The new imports for addon and addonratecard modules are properly placed with the rest of the entity imports, maintaining consistency with the existing codebase structure.
63-66: Addon clients properly integrated into Client struct.New Addon and AddonRateCard clients are correctly added to the Client struct, following the same pattern as other entity clients.
146-147: Correct initialization of new clients.The initialization of Addon and AddonRateCard clients in the init() method follows the established pattern for client initialization.
274-275: Proper transaction handling for new clients.Addon and AddonRateCard clients are correctly integrated into the transaction handling methods, ensuring they work properly in transactional operations.
Also applies to: 329-330
394-404: Consistent hooks and interceptors integration.New clients are appropriately added to the client iteration methods (Use and Intercept), ensuring that hooks and interceptors can be applied to them.
Also applies to: 414-424
433-436: Proper mutation handler integration.The mutation handler is correctly updated to handle the new entity types, following the same pattern as existing entity types.
510-643: Well-implemented AddonClient.The AddonClient implementation follows the same structure and patterns as other entity clients, providing all necessary CRUD operations and relationship queries.
659-822: Well-implemented AddonRateCardClient.The AddonRateCardClient is correctly implemented with all required methods for CRUD operations and relationship queries.
709-710: hooks and interceptors structures are correctly updated.The hooks and interceptors structures are properly updated to include the new entity types, ensuring consistency with the rest of the codebase.
Also applies to: 720-721
552-566:Details
✅ Verification successful
Verify table references in QueryAddonRatecard implementation.
The implementation of the relationship query follows the correct pattern, but there appears to be a potential issue with table references in the underlying AddonRateCard schema definition.
Based on the relevant code snippets, AddonTable and FeaturesTable in addonratecard.go are both set to "addon_rate_cards", which seems incorrect. Typically, these should reference the tables of the related entities (addons and features respectively).
🏁 Script executed:
#!/bin/bash # Check the table definitions in addonratecard.go and addon.go echo "Checking table definitions in addonratecard.go:" grep -A 5 "Table =" openmeter/ent/db/addonratecard/addonratecard.go echo -e "\nChecking AddonTable definition in addonratecard.go:" grep -A 5 "AddonTable =" openmeter/ent/db/addonratecard/addonratecard.go echo -e "\nChecking FeaturesTable definition in addonratecard.go:" grep -A 5 "FeaturesTable =" openmeter/ent/db/addonratecard/addonratecard.go echo -e "\nChecking Table definition in addon.go:" grep -A 5 "Table =" openmeter/ent/db/addon/addon.go echo -e "\nChecking RatecardsTable definition in addon.go:" grep -A 5 "RatecardsTable =" openmeter/ent/db/addon/addon.goLength of output: 3302
Table references verified as correct.
After reviewing the definitions in
addonratecard.goandaddon.go, the join table schema is implementing the intended pattern: bothAddonTableandFeaturesTablecorrectly point to"addon_rate_cards"(the join table), while the inverse table references (AddonInverseTableandFeaturesInverseTable) properly point to the related entity tables ("addons"and"features"respectively). No changes to the QueryAddonRatecard implementation are necessary.openmeter/productcatalog/addon/httpdriver/driver.go (1)
45-55: Factory construction looks good.The
Newfactory function is well-structured, returning a clearly initializedhandler. The optionalHandlerOptionslice is a nice extension point for future enhancements.openmeter/ent/db/feature_create.go (2)
15-15: Import aligns with AddonRateCard logic.The new
addonratecardimport is correct for referencing the new entity, ensuring proper edge handling for add-on rate cards.
377-392:Details
✅ Verification successful
Validate AddonRateCard edge constraints.
Appending the
addon_ratecardedge spec is consistent. Verify that no additional uniqueness constraints are necessary, especially if multiple features can share the same rate card or vice versa.
🏁 Script executed:
#!/bin/bash # Check for unique or foreign-key constraints in your schema definitions rg -A 5 'addon_rate_cards'Length of output: 4973
AddonRateCard Edge Constraints Validated
The schema searches confirm that for the addon_rate_cards table, the only uniqueness constraint is on the primary key (the “id” column). The foreign key set on the “feature_id” column (used in the edge spec) does not enforce extra uniqueness, which is consistent with a one-to-many design where a feature can have multiple addon rate cards. Please confirm that this relationship behavior matches the intended domain design.
cmd/server/wire_gen.go (9)
26-26: Newaddonimport.Importing the add-on package here is aligned with the new add-on service integration.
90-98: Ensure cleanup on meter adapter failures.The error handling block systematically cleans up deferred resources. This pattern helps prevent resource leaks on partial initialization failures.
150-151: Initialize feature connector and add-on service in order.
featureConnectormust be ready before creating theaddonServiceif the latter depends on it. The current approach appears correct, but keep an eye on cyclical dependencies.
172-173: Tracer initialization.Defining a new tracer is consistent. Confirm that all modules requiring tracing have access to this instance.
197-197: Streaming connector initialization.Looks good. If initialization partially succeeds, confirm that leftover resources are still released.
207-207: Check entitlement usage.
entitlementregistry creation is correct. Ensure it’s consistently referenced wherever entitlements are enforced.
313-314: Sandbox provisioner setup.Proper error handling and cleanup sequence. This approach helps prevent inconsistent states if provisioning fails mid-process.
367-367: Meter event service creation.Constructing
metereventServicewithconnectorandserviceis coherent with the existing architecture. No issues found.
424-425: Updated Application struct.The added
AddonandAppfields neatly integrate new services into the broader application. Verify that they're utilized in the appropriate endpoints or handlers.openmeter/productcatalog/addon/errors.go (3)
12-17: Ensure parameters are optional or validated as needed.
NotFoundErrorParamsaccommodates multiple optional fields (Namespace, ID, Key, Version). If these fields are mandatory in certain scenarios, consider adding validation or documentation to clarify usage.
53-63: Error composition is well-structured.
NotFoundErrorneatly wraps amodels.GenericNotFoundError, ensuring compatibility with existing error-handling mechanisms. This is a solid error type design.
65-72: Robust error check witherrors.As.
IsNotFoundcorrectly leverages type assertion viaerrors.As, simplifying checks for this specific error type.openmeter/productcatalog/addon/adapter/adapter.go (2)
42-51: Configuration injection is clear.
New(config Config)properly validates inputs and returns anadapterimplementingaddon.Repository. No issues found.
61-70: Transaction management is consistent.
Tx()appropriately hijacks and wraps the Ent transaction logic. Error handling is sound, returning contextual information if the transaction fails.openmeter/ent/schema/addon.go (7)
17-25: Consistent schema definition with mixins.
AddoninheritsUniqueResourceMixinfor namespaced uniqueness. This approach is aligned with common Ent patterns.
27-49: Field definitions are appropriate.
Each field (likeversionwithMin(1),currencyas immutable, etc.) carefully enforces domain constraints. The JSONB-backedannotationsfield is also set up cleanly for Postgres.
60-75: Efficient indexing strategy.
Using a unique index on(namespace, key, version)combined with a partial index (deleted_at IS NULL) is a robust design. The GIN index onannotationseffectively supports flexible querying.
77-85: Second entity Mixin usage.
AddonRateCardsimilarly employs theUniqueResourceMixin, ensuring consistent handling of resource identity across both entities.
87-139: Flexible, extensible fields.
Fields likeentitlement_template,tax_config, andpriceare convertible to JSONB, offering flexibility for a wide range of add-on rate card data.
141-153: Clear relationship definitions.
Edges fromAddonRateCardtoAddon(Ref("ratecards")) and toFeature(Ref("addon_ratecard")) are well-specified.
155-168: Appropriate composite indexing.
Defining combined unique constraints on(addon_id, key)and(addon_id, feature_key)ensures logical uniqueness for rate cards, again filtered bydeleted_at IS NULL.openmeter/ent/db/addon_delete.go (1)
1-89: Auto-generated file by Ent
No manual modifications are recommended. If you need to alter its functionality, consider adjusting the Ent schema and re-generating the code.openmeter/ent/db/addonratecard_delete.go (11)
1-4: Initialization and package declaration look good.
All standard boilerplate code generated by Ent. No concerns here.
5-13: Imports are appropriate.
Libraries fromentgo.io/entand local packages are imported correctly.
15-20: Struct definition is aligned with Ent's conventions.
TheAddonRateCardDeletestruct and its fields follow established patterns for delete builders in Ent.
22-26: Where() method usage is straightforward.
Appending predicates to the mutation is standard Ent usage.
28-31: Exec() method correctness confirmed.
It delegates execution to thewithHookswrapper, maintaining Ent’s hook lifecycle. No issues found.
33-40: ExecX() maintains the conventional “panic on error” approach.
This is consistent with Ent’s standard “X” methods. No concerns.
42-57: sqlExec() properly handles constraint errors.
The usage ofsqlgraph.DeleteNodesand wrapping inConstraintErroris aligned with Ent’s recommended approach.
59-62: AddonRateCardDeleteOne struct mirrors the multi-entity variant.
Good structure for single-record deletes.
64-68: Where() method for single deletion also looks correct.
Reuses the same approach to append predicates.
70-81: Exec() handles the “not found” scenario with a custom error.
ReturningNotFoundErrorif no record is deleted is consistent with Ent patterns.
83-88: ExecX() for AddonRateCardDeleteOne is consistent with standard naming.
Again, relies on panic for errors, matching other Ent code.openmeter/ent/db/addon.go (11)
16-50: Definition of the Addon entity is well-structured.
All fields, including nested JSON fields (Metadata,Annotations), are handled with clear types.
53-60: AddonEdges struct properly marks loaded types.
This helps distinguish whether the related edges have been eagerly loaded.
62-69: RatecardsOrErr() edge-check is a standard approach for eager-loaded data.
By returningNotLoadedError, it cleanly differentiates between “empty slice” and “not loaded.”
71-91: scanValues() sets up field scanning types efficiently.
All field cases—strings, JSON, times—are covered. No issues noted.
93-196: assignValues() carefully unmarshals JSON fields.
Good error handling for unexpected types. This method accurately updates the struct’s fields.
198-202: Value() retrieves selected fields or modifiers.
This method ensures dynamic select queries are properly handled.
204-207: QueryRatecards() method returns a dedicated query builder.
Preserves Ent’s recommended pattern for chaining queries on edges.
209-214: Update() method returns the proper update builder.
This approach is consistent with typical Ent patterns.
216-225: Unwrap() logic ensures transactional consistency.
It correctly reverts driver references to the original driver if the entity came from a transaction.
227-280: String() method provides useful debugging info.
This is a clear format for logging entity fields, including optional fields.
282-284: Addons type alias is a handy list-like convenience.
No concerns.openmeter/ent/db/feature_update.go (7)
14-14: New import for the addonratecard package is necessary and valid.
This is required for referencing the added “addon_ratecard” edge.
148-161: AddAddonRatecardIDs() and AddAddonRatecard() integrate add-on rate cards into Feature updates.
These methods parallel existing edge-management patterns for other related entities, enabling easy linking ofAddonRateCardrecords.
210-229: ClearAddonRatecard() & RemoveAddonRatecard() methods ensure clean removal flows.
They align with the existing pattern for clearing and removing edges, maintaining consistency.
412-456: Updating sqlSave() with the “addon_ratecard” edge.
The logic for clearing and adding edges follows Ent’s typical O2M pattern. No obvious issues found.
591-604: AddAddonRatecardIDs() and AddAddonRatecard() for FeatureUpdateOne.
Mirrors the multi-entity version, preserving consistency in the single-entity update builder.
653-672: ClearAddonRatecard() and RemoveAddonRatecard() in the single-entity context.
Straightforward edge-clearing logic, consistent with Ent’s “XOne” builder approach.
885-929: FeatureUpdateOne.sqlSave() correctly wires in the AddonRatecard edges.
Edge definitions (cleared, removed, added) match the existing logic for other edges.openmeter/productcatalog/addon/adapter/adapter_test.go (1)
305-325: Verify post-deletion expectations.This test block verifies that an add-on can still be retrieved after deletion, suggesting soft-delete semantics. If the expected behavior is to deny retrieval of a deleted record, consider revisiting the logic and updating the test to reflect a “not found” scenario instead.
Do you expect the add-on to remain retrievable after deletion? If not, please confirm the correct post-deletion behavior by checking for a “not found” error.
openmeter/productcatalog/addon/adapter/addon.go (4)
218-253: Validate references for rate cards, if applicable.The
rateCardBulkCreatefunction effectively creates multiple rate cards in bulk. Make sure that any feature references (e.g.,FeatureID) orAddonIDforeign keys are validated or enforced by the schema to prevent orphaned or invalid references.
255-300: Marking the add-on as deleted is well-handled.The soft-delete approach using a
deletedAtfield is straightforward. By returning an error if the add-on doesn't exist, you’re ensuring consistent state. This deletion flow looks good.
302-399: Check edge cases in retrieving the latest version.The
GetAddonmethod includes logic for returning the addon’s latest version ifVersion == 0andIncludeLatestis set, or an "active" version otherwise. This branching can grow complicated if more states are introduced. Keep a watch for new status categories or reworks that might break these conditions.
506-511: Confirm if you should include ratecards marked for future deletion.
AddonEagerLoadRateCardsFnincludes ratecards not yet past theirDeletedAttimes. This is efficient for normal usage. However, if archived or future-dated items should be excluded from some contexts, be sure to handle that logic upstream or adjust this function.openmeter/ent/db/feature_query.go (5)
16-16: New import for AddonRateCard.The addition of
addonratecardensures that theFeatureQuerycan now referenceAddonRateCardentities. This import aligns with the new edges and query methods introduced below.
114-134: New methodQueryAddonRatecard().This method parallels existing relationships (like
QueryRatecard) and avoids confusion by using a dedicated edge. The logic to build a join step withsqlgraph.NewStepis consistent, and error handling looks straightforward.
445-452: Tracking loaded types with an index for the new field.Adding the third boolean for
withAddonRatecardeffectively signals the presence of an eager-loaded relation. This approach is aligned with how Ent maintains edges for O2M or M2M relationships.
489-495: Conditional loadAddonRatecard check.The if-statement ensures that
fq.withAddonRatecardis not nil before loading the relationship. Make sure that your data remains consistent by verifying that all references (i.e., foreign keys in AddonRateCard) point to valid features.
562-594:loadAddonRatecardfunction.The function populates an
AddonRatecardedge in each queried Feature. The approach of storing references innodeIDsand appending to the correct Feature entity in memory is a standard Ent pattern. The error checking for nilFeatureIDis especially helpful for diagnosing data mismatches.openmeter/productcatalog/addon/service.go (5)
1-19: Clear package structure and constants.Defining the package
addonand referencing standard time and error functionalities sets the stage well. The named constants (timeJitter, sorting orders) are suitably placed for broad usage across the file.
96-116:CreateAddonInputmerges namespaced model with an add-on.Bundling
models.NamespacedModelandproductcatalog.Addonis straightforward. The combined validation ensures users cannot skip either entity’s constraints. The approach is suitable for a single-step creation flow.
231-243:DeleteAddonInputrequires a valid NamespacedID.The soft validation is enough here. Because the actual deletion is performed by your repository, the function remains succinct and focused.
245-286:PublishAddonInputensures future activation windows.You’re requiring
EffectiveFromto be non-past, with jitter. This is helpful for preventing accidental backdating. Keep an eye out for time zone issues or user confusion around the “past” threshold if you have global usage.
288-314:ArchiveAddonInputensures future deactivation windows.Similar to
PublishAddonInput, requiring a non-pastEffectiveToprevents immediate retroactive changes. This is consistent with any usage flow that disallows backdated archiving.openmeter/productcatalog/addon/httpdriver/mapping.go (4)
23-62: Clean and thorough status handling
This function validates add-on statuses meticulously and returns an error for unknown statuses, improving reliability and clarity. Good job.
204-321: Thorough coverage of usage-based price types
This switch statement comprehensively addresses all price variants. The error handling is consistent and clear.
478-507: Add validations for required fields
Consider validating mandatory fields (e.g., key, name, and rate cards) to avoid creating partial or inconsistent add-ons.
700-743: Check discount precision and boundaries
Since discount quantities and percentages are parsed from decimal strings, ensure there’s no risk of negative or excessive values through proper validation.openmeter/ent/db/setorclear.go (1)
16-238: Overall Consistency & Test Coverage for 'SetOrClear' PatternAll of these functions follow the same conditional pattern: if the pointer argument is
nil, the correspondingClear<Field>method is called; otherwise, the relevantSet<Field>method is used with the dereferenced value. This is a common and appropriate approach for optional fields in Ent-generated code.No obvious correctness, concurrency, or security issues appear. However, ensure you have adequate test coverage for the newly introduced methods (both
UpdateandUpdateOne) so that each field’s set-or-clear behavior is validated, preventing future regressions.Would you like to run a quick check searching for test references to these new methods? I can generate a script to gauge coverage if needed.
openmeter/ent/db/addon/addon.go (2)
13-83: Consistent Definition of 'Addon' Fields and ColumnsThe constants and columns appear aligned with the schema definitions (e.g.,
FieldID,FieldNamespace, etc.). The usage ofColumnsfor validation and listing is standard Ent practice. No issues are apparent with naming or structure, and there are no concurrency or security concerns at this level. Good job keeping them clearly organized.
110-198: Ordering Helpers for Querying 'Addon'Providing ordering options (e.g.,
ByID,ByNamespace,ByEffectiveFrom) is a clean, extensible approach for building dynamic queries. The usage ofOrderOptionis idiomatic in Ent. Just confirm that any client code (e.g., your service layer) leveraging these ordering helpers passes valid fields, especially for the neighbor-based orderings likeByRatecards.openmeter/ent/db/addonratecard/addonratecard.go (3)
1-106: Definition of 'AddonRateCard' Fields & ColumnsThe top-level constants mirror those in the
addonpackage but specialized for rate cards. The standardColumnsslice and theValidColumnfunction look correct and maintainable. Nice job ensuring consistency with how columns are enumerated in other Ent entities.
108-130: Validators, Defaults, and Custom ScannersDefining field-specific validators (e.g.,
AddonIDValidator,TypeValidator) alongside default ID generators is a best practice. TheValueScannerstruct for typed fields is beneficial for domain-level safety. All method signatures and naming are consistent with Ent’s generated approach.
142-257: Ordering Options & Relationship StepsThe ordering methods (
ByID,ByNamespace, etc.) and the neighbor-based ordering (e.g.,ByAddonField,ByFeaturesField) appear correct. The use ofsqlgraph.OrderByNeighborTermsis consistent with Ent’s recommended patterns for advanced ordering across edges. Everything here should integrate smoothly with any query builders in your code.openmeter/ent/db/addonratecard_query.go (1)
67-87: Confirm correct edge relationship forQueryAddon.This method constructs an M2O edge from
AddonRateCardtoAddon. Ensure that the database schema and the Ent schema definition accurately reflect the intended relationship (i.e., eachAddonRateCardbelongs to exactly oneAddon).openmeter/ent/db/addon_query.go (2)
66-86: Validate edge cardinality forQueryRatecards.This method indicates a one-to-many edge (
sqlgraph.O2M), suggesting anAddoncan have multipleAddonRateCardentries. Confirm that the corresponding Ent schema matches this design to avoid unexpected query results.
88-108: Confirm usage of panic inFirstXandFirstIDX.Similar to other Ent-generated code, these methods panic on errors. Ensure this aligns with your team’s error-handling conventions and that you have adequate coverage or fallback in non-test environments.
openmeter/ent/db/addonratecard_update.go (2)
308-331: Review uniqueness constraint when clearing theAddonedge.Clearing a required unique edge (
Addon) throws an error if new nodes are also being added. Double-check business logic to ensure whether removing an existingAddonassignment without replacement is allowable in your domain model.
422-451: VerifyM2Oedge specification forAddonupdates.The code replaces or clears the add-on relationship by directly manipulating the foreign key. Confirm that the domain strictly requires a single
AddonperAddonRateCard. If multiple associations are possible, reevaluate the relationship type.openmeter/ent/db/addon_update.go (4)
19-24:AddonUpdatestruct definition looks clear and aligned with Ent conventions.This section provides a well-structured foundation for handling addon record updates. No major concerns here.
177-216: Confirm the One-to-Many relationship forratecards.These methods to add/remove
ratecardssuggest a one-to-many relationship. If you intended a many-to-many association, ensure the schema is accurately modeled, because Ent is generating an O2M approach here.
247-262: Validation and default logic are solid.Setting default timestamps and validating the
versionfield helps safeguard against malformed updates.
264-383:sqlSavemethod usage is consistent with Ent best practices.Leveraging
sqlgraph.UpdateNodesand systematically handling each cleared or set field ensures proper partial updates.openmeter/ent/db/addon_create.go (4)
19-25:AddonCreatestruct is neatly defined.The fields, hooks, and conflict options form a standard Ent builder pattern for creating addon records.
243-287: Field validation is thorough.All required fields (
namespace,name,key,version,currency, etc.) are enforced, reducing the chance of inconsistent data.
289-313:sqlSavemethod is consistent with Ent best practices.This method invokes
createSpecand gracefully handles constraint errors, aligning well with Ent’s generated patterns.
315-397:createSpeclogic is straightforward and well-organized.Fields are assigned to
_nodeproperly, and the newly created entity structure is ready for database insertion.openmeter/ent/db/addonratecard_create.go (5)
1-2: Auto-generated notice.
This file is a product of the Ent code generator. Generally, you should avoid manual edits to generated files, as changes will be overwritten by subsequent schema regenerations.
259-273: Sensible default assignments.
The default assignment ofCreatedAt,UpdatedAt, andIDfields ensures proper timestamps and IDs when these fields are omitted. This pattern is consistent with best practices and helps maintain data integrity.
275-337: Effective field validation checks.
All required fields/edges (e.g.,namespace,key,type,addon_id, and theaddonedge) are validated before save. This ensures that invalid or incomplete data cannot be persisted. Nicely handled.
339-363: Standard node creation flow.
ThesqlSavemethod correctly orchestrates the creation of the new node, including final ID assignment and constraint error handling. No concurrency or blocking issues identified.
803-1569: Bulk creation and upsert logic.
The bulk creation flow properly chains mutations and concludes with a single batch insert. The upsert configuration with optional conflict resolution is also well-structured and consistent with Ent patterns.
chrisgacsal
force-pushed
the
feat/addons-api
branch
2 times, most recently
from
18b405f to
2ec8639
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (25)
openmeter/productcatalog/addon/ratecard_test.go (3)
19-174: Test coverage is solid; consider stabilizing time usage.The JSON serialization tests for both FlatFee and UsageBased RateCards appear thorough, covering serialization and roundtrip deserialization. However, using
time.Now()inline might introduce small timing discrepancies between serialization and deserialization in certain environments. Consider freezing or mocking time values to avoid potential flakiness.
176-315: Tests for FlatFeeRateCard are adequate.The table-driven tests validate both valid/invalid scenarios. You might consider marking sub-tests with
t.Parallel()to speed up test execution where feasible, but otherwise these tests look solid.
318-474: Usage-based tests mirror the flat fee structure well.The parallel approach for valid and invalid usage-based rate cards is clear. Similar to the FlatFee tests, consider using
t.Parallel()for sub-tests if concurrency is safe.openmeter/productcatalog/addon/adapter/adapter_test.go (5)
65-67: Fix grammar in error message commentThere's a grammatical issue in the error message. The comment is missing a verb.
- t.Errorf("failed to postgres driver: %v", err) + t.Errorf("failed to close postgres driver: %v", err)
312-312: Fix typo in error messageThere's a small typo in the error message - it says "ad-on" instead of "add-on".
- require.NoErrorf(t, err, "deleting ad-on must not fail") + require.NoErrorf(t, err, "deleting add-on must not fail")
361-362: Rename function to correctly reference add-onsThe function name still references "Plan" rather than "Addon", which appears to be a copy-paste error.
-func testListPlanStatusFilter(ctx context.Context, t *testing.T, repo *adapter) { +func testListAddonStatusFilter(ctx context.Context, t *testing.T, repo *adapter) {
327-329: Update function call after renamingUpdate the function call to match the renamed function.
- t.Run("ListStatusFilter", func(t *testing.T) { - testListPlanStatusFilter(ctx, t, addonRepo) - }) + t.Run("ListStatusFilter", func(t *testing.T) { + testListAddonStatusFilter(ctx, t, addonRepo) + })
364-364: Update variable name for consistencyThe namespace variable name should be updated to be consistent with the function's purpose.
- ns := "list-plan-status-filter" + ns := "list-addon-status-filter"openmeter/productcatalog/addon/httpdriver/addon.go (3)
31-97: Typo fix in error message and optional page parameters usage.
- There's a minor typographical error at line 77: "pahse" → "phase".
- Consider verifying that using
notification.DefaultPageSizeandnotification.DefaultPageNumberis acceptable and does not create an unintended dependency. If decoupling is desired, you might introduce pagination defaults directly in this package.Possible fix for the typo:
- return ListAddonsResponse{}, fmt.Errorf("failed to cast add-on pahse: %w", err) + return ListAddonsResponse{}, fmt.Errorf("failed to cast add-on phase: %w", err)
288-331: Enhance clarity by aligning response type naming and address the TODO.
- The handler currently returns
PublishAddonResponsebut usescommonhttp.JSONResponseEncoderWithStatus[GetAddonResponse]. Even though both areapi.Addon, consider renaming the generic parameter to match for consistency.- Address the TODO comment on lines 302–303 if you need to support setting EffectivePeriod from the API request.
Would you like help in updating the TypeSpec definition for full EffectivePeriod support?
333-374: Handle TODO for EffectivePeriod.To.Similarly to the PublishAddon endpoint, there's a TODO in lines 347–348 about supporting EffectivePeriod settings. Finalizing that will help unify the approach.
Do you want me to open an issue or draft a follow-up PR to address the effective period handling?
openmeter/productcatalog/addon.go (2)
37-44: Fix or remove the FIXME to unify effective period logic.The comment hints at a plan to merge
AddonEffectivePeriodwith a broaderEffectivePeriodstructure. If you plan to keep them separate, consider removing or clarifying the FIXME.
54-81: Environment-based time usage note.
StatusandStatusAtrely ontime.Now(). For improved testability, consider using a clock interface (similar to other parts of the code) instead of directly callingtime.Now().openmeter/productcatalog/addon/service.go (1)
176-196: FIXME note on ratecards validation.There's a FIXME at line 193 referencing missing ratecard validation. If partial updates can include new or modified ratecards, consider addressing the incomplete validation to prevent inconsistent states.
Would you like me to generate a potential approach for partial ratecard validation?
openmeter/productcatalog/addon/service/addon.go (7)
18-28: Double-check error message text naming.
In line 21, the error message references "invalid list Plans params" while the function is namedListAddons. Consider aligning the wording to avoid confusion and improve consistency.
80-151: Inconsistency in success messages vs. entity name.
Logging statements and error strings ("plan created event") seem to reference "plan" instead of "add-on" (e.g., line 144:"failed to publish plan created event"). Consider renaming to ensure consistent naming throughout the logic and logs.
153-222: Rename logging for clarity.
Line 165 logs"deleting Plan", but all references and function names revolve around "add-on." Consider updating for consistency and clarity.
191-191: Resolve the "FIXME" to return the deleted add-on.
You have a// FIXME: make delete endpoint to return deleted addoncomment. If needed, I can help implement the returning logic and relevant tests.
224-252: Handle potential not-found scenario.
Line 242 has a// FIXME: not found error. If theGetAddoncall returns a not-found error, consider returning a typed error (e.g., 404) to provide better context for downstream consumers.
254-317: Adjust "plan" references in logs for naming consistency.
Similar to the Create and Delete methods, line 265 logs"updating Plan"even though the function deals with add-ons. Aligning the terminology will avoid confusion.
302-302: Correct typo in error message.
Typo in"failed to udpate aa-on"makes logs less clear. Replace with"failed to update add-on".- return nil, fmt.Errorf("failed to udpate aa-on: %w", err) + return nil, fmt.Errorf("failed to update add-on: %w", err)openmeter/productcatalog/addon/ratecard.go (1)
100-196: Optional enhancement to unify repeated logic.
TheFlatFeeRateCardstruct has repetitive patterns (e.g., in Marshal/Unmarshal) similar toUsageBasedRateCard. Consider extracting shared code to reduce duplication.openmeter/productcatalog/addon/event.go (3)
46-47: ValidateAddonreference before composing resource path.Each
EventMetadata()method assumese.Addonis non-nil when callingmetadata.ComposeResourcePath(...). Although there's aValidate()method, if it's never invoked, a nilAddonhere would cause a runtime panic. Consider adding a protective check if there's any chance the caller might skip validation before populating metadata.Also applies to: 89-90, 132-133, 179-180, 222-223
57-64: Remove or utilize the unusederrsaggregator.In each
Validate()method, you create anerrsslice but never append to it, soerrors.Join(errs...)is effectivelynilwhene.Addonis present or immediately returning an error when absent. Either remove the slice or consistently aggregate all validation errors to leverageerrors.Join.Also applies to: 100-107, 143-154, 190-197, 233-240
14-21: Consider extracting event version into a shared constant.The
"v1"string literal is repeated across all event definitions. For maintainability, define a single constant (e.g.const addonEventVersion = "v1") and reference it in eachEventName()call to avoid version mismatches in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
app/common/productcatalog.go(4 hunks)cmd/server/main.go(1 hunks)cmd/server/wire.go(2 hunks)cmd/server/wire_gen.go(14 hunks)openmeter/event/metadata/resourcepath.go(1 hunks)openmeter/productcatalog/addon.go(1 hunks)openmeter/productcatalog/addon/adapter/adapter.go(1 hunks)openmeter/productcatalog/addon/adapter/adapter_test.go(1 hunks)openmeter/productcatalog/addon/adapter/addon.go(1 hunks)openmeter/productcatalog/addon/adapter/mapping.go(1 hunks)openmeter/productcatalog/addon/addon.go(1 hunks)openmeter/productcatalog/addon/assert.go(1 hunks)openmeter/productcatalog/addon/errors.go(1 hunks)openmeter/productcatalog/addon/errors_test.go(1 hunks)openmeter/productcatalog/addon/event.go(1 hunks)openmeter/productcatalog/addon/httpdriver/addon.go(1 hunks)openmeter/productcatalog/addon/httpdriver/driver.go(1 hunks)openmeter/productcatalog/addon/httpdriver/errors.go(1 hunks)openmeter/productcatalog/addon/httpdriver/mapping.go(1 hunks)openmeter/productcatalog/addon/ratecard.go(1 hunks)openmeter/productcatalog/addon/ratecard_test.go(1 hunks)openmeter/productcatalog/addon/repository.go(1 hunks)openmeter/productcatalog/addon/service.go(1 hunks)openmeter/productcatalog/addon/service/addon.go(1 hunks)openmeter/productcatalog/addon/service/service.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- openmeter/event/metadata/resourcepath.go
- openmeter/productcatalog/addon/httpdriver/errors.go
- cmd/server/main.go
- cmd/server/wire.go
- openmeter/productcatalog/addon/httpdriver/driver.go
- openmeter/productcatalog/addon/repository.go
- openmeter/productcatalog/addon/errors.go
- openmeter/productcatalog/addon/adapter/adapter.go
- openmeter/productcatalog/addon/assert.go
🧰 Additional context used
🧬 Code Definitions (9)
openmeter/productcatalog/addon/addon.go (2)
openmeter/productcatalog/addon.go (2)
Addon(181-186)AddonMeta(94-117)openmeter/productcatalog/ratecard.go (1)
RateCards(350-350)
openmeter/productcatalog/addon/errors_test.go (1)
openmeter/productcatalog/addon/errors.go (3)
NewNotFoundError(19-49)NotFoundErrorParams(12-17)IsNotFound(65-72)
openmeter/productcatalog/addon/ratecard_test.go (3)
openmeter/productcatalog/addon/ratecard.go (4)
FlatFeeRateCard(102-105)RateCardManagedFields(19-25)UsageBasedRateCard(200-203)NewRateCardFrom(61-98)openmeter/productcatalog/addon/adapter/adapter.go (1)
Config(19-22)openmeter/productcatalog/addon/service/service.go (1)
Config(12-17)
openmeter/productcatalog/addon/adapter/adapter_test.go (6)
openmeter/productcatalog/addon/service.go (5)
CreateAddonInput(98-101)GetAddonInput(198-211)ListAddonsInput(48-79)UpdateAddonInput(122-142)DeleteAddonInput(231-233)openmeter/productcatalog/addon.go (4)
Addon(181-186)AddonMeta(94-117)AddonEffectivePeriod(38-44)AddonStatus(22-22)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)openmeter/productcatalog/addon/ratecard.go (3)
UsageBasedRateCard(200-203)RateCardManagedFields(19-25)FlatFeeRateCard(102-105)openmeter/productcatalog/addon/service/service_test.go (1)
MonthPeriod(303-303)openmeter/productcatalog/addon/assert.go (3)
AssertAddonCreateInputEqual(14-26)AssertAddonEqual(54-63)AssertAddonUpdateInputEqual(28-52)
cmd/server/wire_gen.go (10)
openmeter/productcatalog/feature/connector.go (1)
NewFeatureConnector(84-100)app/common/streaming.go (1)
NewStreamingConnector(21-43)app/common/entitlement.go (1)
NewEntitlementRegistry(22-40)openmeter/app/stripe/service.go (2)
BillingService(44-50)Service(11-16)openmeter/app/sandbox/app.go (1)
App(35-39)openmeter/billing/service.go (1)
Service(9-19)openmeter/customer/service.go (1)
Service(10-13)openmeter/app/service/service.go (1)
Service(12-15)openmeter/meter/service/service.go (1)
Service(13-15)openmeter/progressmanager/service.go (1)
Service(9-11)
openmeter/productcatalog/addon/httpdriver/addon.go (1)
openmeter/productcatalog/addon/httpdriver/mapping.go (3)
FromAddon(23-62)AsCreateAddonRequest(478-507)AsUpdateAddonRequest(1071-1092)
openmeter/productcatalog/addon.go (2)
openmeter/productcatalog/ratecard.go (2)
RateCards(350-350)RateCard(28-38)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)
openmeter/productcatalog/addon/service/addon.go (3)
openmeter/productcatalog/addon/service.go (8)
ListAddonsInput(48-79)CreateAddonInput(98-101)OrderBy(33-33)DeleteAddonInput(231-233)GetAddonInput(198-211)UpdateAddonInput(122-142)PublishAddonInput(245-250)ArchiveAddonInput(288-294)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)openmeter/productcatalog/addon/event.go (5)
NewAddonCreateEvent(24-29)NewAddonDeleteEvent(110-115)NewAddonUpdateEvent(67-72)NewAddonPublishEvent(157-162)NewAddonArchiveEvent(200-205)
openmeter/productcatalog/addon/event.go (3)
openmeter/productcatalog/addon.go (1)
Addon(181-186)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)openmeter/event/metadata/resourcepath.go (2)
ComposeResourcePath(28-30)EntityAddon(20-20)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test
- GitHub Check: Quickstart
- GitHub Check: Lint
- GitHub Check: E2E
- GitHub Check: Developer environment
- GitHub Check: CI
- GitHub Check: Build
- GitHub Check: Commit hooks
- GitHub Check: Analyze (go)
🔇 Additional comments (76)
openmeter/productcatalog/addon/addon.go (3)
1-21: Struct definition and fields look good.No issues found in the package declaration, imports, and primary
Addonstruct definition. The embeddings (NamespacedID, ManagedModel, AddonMeta) and theRateCardsfield are well organized.
23-43: Validation logic is comprehensive.The
Validatemethod correctly aggregates validation errors from embedded structs and returns a joined error. Implementation is clear and consistent with typical Go validation patterns.
45-54: Conversion method is well-structured.The
AsProductCatalogAddonmethod properly checks theDeletedAtfield to handle deleted add-ons. The returned object is correctly constructed. No major concerns here.openmeter/productcatalog/addon/adapter/addon.go (6)
24-152: Potential empty OR filter leads to zero results.When no IDs, Keys, KeyVersions, or Currencies are specified, the
orFiltersslice is empty, which might cause the query to return no rows. Verify whether this behavior is intentional or if a fallback to fetch all add-ons is desired.
154-216: Reminder: enforce composite uniqueness.Creation logic is straightforward. However, as previously noted, consider ensuring a composite uniqueness constraint (e.g.,
(namespace, key, version)) in your schema to prevent duplicates when multiple concurrent calls create the same add-on.
218-253: Bulk creation of rate cards is straightforward.The helper function correctly maps
RateCardobjects to database entities and appends them to a bulk create list. No issues spotted.
255-300: Soft-deletion flow is well-handled.The
DeleteAddonmethod marks add-ons as deleted by settingDeletedAt. The logic to handle already-deleted or non-existent add-ons is clear.
302-399: Check subselect correctness for fetching latest version.The subselect logic for retrieving the latest add-on version by
(namespace, key)is advanced but looks plausible. However, please confirm it aligns with your design requirements, especially for use cases where multiple versions exist.
506-511: Rate card eager loading is clean.Filtering out future-deleted rate cards is a sensible approach. No comments on concurrency or performance concerns.
openmeter/productcatalog/addon/adapter/adapter_test.go (4)
320-324: Verify soft deletion behaviorAfter calling
DeleteAddon, the test verifies the add-on can still be retrieved and has the same data, suggesting a soft deletion approach. Ensure this aligns with the intended behavior for handling deleted add-ons.
38-53: Well-structured test data setupThe addon input is well-structured with appropriate test data including metadata, annotations, and currency. The nested structure follows the domain model correctly.
89-147: Comprehensive test setup with proper test dataThe test setup is thorough, creating prerequisite data (feature) before testing add-on operations. The rate card configuration is detailed and covers various pricing options including tiers, commitments, and tax configurations.
148-155: Good validation in Create testThe Create test properly validates the creation operation and uses the helper function to assert equality between input and created entity.
app/common/productcatalog.go (4)
13-15: Well-organized imports for add-on functionalityThe imports are properly organized, bringing in the necessary packages for the add-on functionality.
23-27: Good integration into existing wire setupThe Addon wire set is correctly integrated into the ProductCatalog wire set.
37-39: Clean wire set declarationThe Addon wire set is declared consistently with other wire sets in the file.
74-94: Well-implemented service initialization functionThe NewAddonService function follows the same pattern as other service initialization functions in the file. It properly configures the adapter, handles errors, and sets up appropriate logging with subsystem context.
openmeter/productcatalog/addon/errors_test.go (3)
11-47: Comprehensive test cases for IsNotFound functionThe test cases for the IsNotFound function are comprehensive, covering valid NotFoundError, wrapped NotFoundError, and invalid error scenarios.
49-94: Complete test coverage for error message formattingThe test cases verify error message formatting with different combinations of parameters (ID, Key, Version), ensuring the error messages are correctly constructed.
61-62: Correctly updated error messagesError messages now properly reference "add-on" instead of "plan", addressing the issue highlighted in previous reviews.
openmeter/productcatalog/addon/service/service.go (4)
12-17: Well-structured configuration structThe Config struct is well-structured with all necessary dependencies for the service.
19-42: Thorough validation in service initializationThe New function properly validates all required dependencies and returns appropriate errors if any are missing.
44-44: Good practice using type assertionUsing a type assertion to verify that the service implements the addon.Service interface is a good practice to catch interface implementation issues at compile time.
46-51: Clean service struct implementationThe service struct is cleanly implemented with all the necessary dependencies.
cmd/server/wire_gen.go (14)
26-26: Add-on import statement
The added import for theaddonpackage is correct and aligns with the subsequent usage ofaddonService.
90-98: Initialize meter adapter with cleanup
The new code snippet properly createsadapterviacommon.NewMeterAdapterand follows established cleanup patterns for error handling.
150-151: Feature and Add-on services
Initializing the newfeatureConnectorandaddonServicelooks consistent and correctly referencescommon.NewAddonService.
161-162: App configuration and service
FetchingappsConfigurationfromconfand creatingappServicemaintains consistency with other service initializations.
172-174: Tracer and configuration references
These lines properly set up the tracer and retrieveentitlementsConfigurationandaggregationConfiguration, matching the project's pattern.
186-187: Progress manager service
DefiningprogressManagerConfigurationand initializing theprogressmanagerServicefits seamlessly into the existing setup.
197-198: Streaming connector creation
Implementingcommon.NewStreamingConnectorwithprogressmanagerServiceand handling errors is consistent with the inject-and-cleanup strategy.
207-208: Entitlement and customer services
Introducingentitlementfollowed bycustomerServiceis structurally correct and leverages the new streaming connector.
261-262: Billing service injection
Usingcommon.BillingServicewith all relevant dependencies (e.g.,appService,customerService,featureConnector) is well-structured.
271-272: AppStripe service initialization
Incorporating the newly createdsecretserviceServiceintocommon.NewAppStripeServiceensures secure data handling.
313-314: App sandbox provisioner
The newappSandboxProvisionerintegrates seamlessly, following the defined error handling flow.
367-368: Meter event service
CreatingmetereventServicewithconnectorandserviceensures event-driven operations are properly linked.
424-425: Embed new services in Application
AssigningaddonServiceandappServiceto theApplicationstruct fields keeps the dependency references centralized.
475-475: New Addon field in Application struct
DeclaringAddon addon.Servicealigns with its usage at line 424 for maintaining the add-on service reference.openmeter/productcatalog/addon/adapter/mapping.go (4)
1-14: Package and imports
All imported packages appear necessary for the mapping logic and align with the file’s functionality.
16-65: Function: fromAddonRow
This function neatly converts theentdb.Addonentity into anaddon.Addonmodel, with thorough validation and graceful handling of rate cards.
67-146: Function: fromAddonRateCardRow
The logic for constructingRateCardfromentdb.AddonRateCardis well-structured, uses explicit type checks, and ensures billing cadence is validated.
148-189: Function: asAddonRateCardRow
This reverse mapping toentdb.AddonRateCardcovers all rate card types via a clean type switch, with proper error handling for unsupported variants.openmeter/productcatalog/addon/httpdriver/addon.go (4)
99-144: Looks good for add-on creation flow.The
CreateAddonfunction properly decodes and validates the request body, then delegates work to the service layer. No issues found.
146-192: Partial update approach looks solid.The
UpdateAddonfunction consistently applies partial updates (pointers) and delegates them to the service. Appropriate error handling is in place.
194-230: Delete handler is clear and concise.The handler correctly constructs the input, calls the service, and returns a 204 status upon successful deletion.
232-286: Namespace and ID/Key resolution logic is clear.The
GetAddonhandler properly interprets ID or key inputs, factoring in the version and the IncludeLatest flag. Implementation is straightforward.openmeter/productcatalog/addon.go (5)
46-52: Valid range checks look appropriate.The
Validatemethod forAddonEffectivePeriodcorrectly flags invalid date ordering, returningAddonStatusInvalidwhenEffectiveTois beforeEffectiveFrom.
83-88: Equality check is straightforward.The
Equalmethod properly compares both EffectiveFrom and EffectiveTo for equality.
119-139: Validation logic is comprehensive.The
AddonMeta.Validate()method handles key, name, and currency requirements, and also delegates toAddonEffectivePeriod.Validate().
188-213: Rate card validation logic is sound.The loop checks for duplicate keys and individual validation errors. Ensure that referencing a different namespace in rate cards is intentionally permitted or further restricted if needed.
215-221: Addon equality check is neatly delegated.Combining metadata and rate cards for an equality check is straightforward. Good partitioning of responsibilities between
AddonMeta.EqualandRateCards.Equal.openmeter/productcatalog/addon/service.go (10)
35-44: Service interface design is well structured.The methods comprehensively cover the add-on lifecycle (CRUD + publish/archive). This lays a good foundation for future expansion.
46-84: ListAddonsInput is flexible and well-defined.No immediate issues. The filters and pagination fields look consistent with typical usage scenarios.
85-95: No concerns on status filter struct.The
ListAddonsStatusFilteris straightforward and does not introduce any apparent pitfalls.
98-115: Creation input effectively reuses Addon validation.
CreateAddonInputproperly composesNamespacedModelandAddon, delegating to each for thorough checks.
122-174: UpdateAddonInput: partial update approach is well-designed.Pointers to optional fields (Name, Description, etc.) allow incremental updates. Equality logic is clear.
198-229: GetAddonInput validation is correct.The input requires either Namespace + ID or Namespace + Key, enforcing that at least one reference is provided.
231-243: Deletion input is straightforward.The required
NamespacedIDprevents accidental deletion outside a valid namespace.
245-286: Time-based publish validations are robust.Checks ensure
EffectiveFromis in the future (withintimeJittertolerance). This prevents users from publishing an add-on in the past.
288-314: ArchiveAddonInput has analogous validations.
EffectiveTomust be set and not too far past. The approach is consistent withPublishAddonInput.
316-340: NextAddonInput design is aligned with the rest of the handlers.The required fields mirror those in
GetAddonInput, ensuring either ID or Key is present.openmeter/productcatalog/addon/service/addon.go (4)
30-78: Validate concurrency usage of in-place feature expansion.
TheexpandFeaturesmethod updates rate cards in-place, which may have concurrency implications if multiple goroutines call this function on shared data. Verify that usage patterns of this method do not introduce data-race conditions.
319-421: Consider verifying alignment across multiple rate cards.
Currently, if any card is misaligned, publishing is blocked. Ensure this logic is well-tested with mixed card scenarios to avoid partial publishes.
423-482: Good separation of archiving logic.
The steps to retrieve, validate, update, and publish an event for the archived add-on appear consistent and maintainable.
484-602: Prevent duplication of draft versions.
Lines 555-559 already check for an existing draft. This logic is critical to maintain version integrity. Confirm test coverage for corner cases, such as concurrent calls creating multiple drafts.openmeter/productcatalog/addon/ratecard.go (4)
19-55: Validation approach looks good.
TheRateCardManagedFields.Validate()method adequately checks required fields. No issues detected.
57-59: Interface naming is consistent.
ManagedRateCardis straightforward and aligns with the code’s usage.
61-98: Robust deserialization logic.
NewRateCardFromcleanly differentiates between raw JSON,FlatFeeRateCard, andUsageBasedRateCard. The typed approach helps avoid confusion during runtime.
198-295: UsageBasedRateCard implementation appears consistent.
Methods for JSON handling, equality checks, and merging align well with the FlatFeeRateCard pattern.openmeter/productcatalog/addon/httpdriver/mapping.go (7)
64-64: [Duplicate from prior feedback] Address the "FIXME" consolidation.
This echoes a past review comment suggesting that HTTP mappers be centralized to reduce duplication.
23-62: Clear transformation logic inFromAddon.
The function systematically maps core fields and gracefully handles invalid statuses. Implementation detail looks solid.
204-321: Good layering inFromRateCardUsageBasedPrice.
Each price type is neatly handled with custom error messages, making debugging easier. This is a well-structured approach.
478-507: Comprehensive input validation inAsCreateAddonRequest.
Covers both currency code validation and rate card conversion, preventing malformed requests from proceeding.
745-939: Sufficient error handling for complex price conversions.
AsPricethoroughly deals with each price type. The error messages are descriptive and should help troubleshoot issues quickly.
982-1055: UsagePeriod fallback logic is helpful.
By defaultingUsagePeriodtobillingCadenceif not specified, you reduce boilerplate for most use cases. Be sure to test corner scenarios.
1071-1092: Conditional update ofRateCardsis well-implemented.
AsUpdateAddonRequestgracefully handles the scenario where RateCards may or may not be present in the payload.
chrisgacsal
force-pushed
the
feat/addons-api
branch
2 times, most recently
from
ef4e225 to
371a487
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
♻️ Duplicate comments (1)
openmeter/productcatalog/addon.go (1)
154-156: 🛠️ Refactor suggestionRemove duplicate comparison of
m.Description.The equality check for
m.Descriptionappears at lines 154-156 and again at lines 170-172, causing redundant logic. Removing one of these checks would streamline theEqualmethod and avoid confusion.154-156 (keeping): if lo.FromPtr(m.Description) != lo.FromPtr(v.Description) { return false } -if lo.FromPtr(m.Description) != lo.FromPtr(v.Description) { - return false -}Also applies to: 170-172
🧹 Nitpick comments (38)
openmeter/productcatalog/addon/assert.go (2)
54-63: Consider adding RateCards comparison to AssertAddonEqualWhile this function compares the basic fields of two Addon objects, it doesn't compare the RateCards collection, unlike the other assertion functions. Consider adding RateCards comparison for complete equality verification.
func AssertAddonEqual(t *testing.T, expected, actual Addon) { t.Helper() assert.Equalf(t, expected.Key, actual.Key, "key mismatch") assert.Equalf(t, expected.Name, actual.Name, "name mismatch") assert.Equalf(t, expected.Description, actual.Description, "description mismatch") assert.Equalf(t, expected.Currency, actual.Currency, "currency mismatch") assert.Equalf(t, expected.Metadata, actual.Metadata, "metadata mismatch") assert.Equalf(t, expected.Annotations, actual.Annotations, "annotations mismatch") + + // Compare RateCards + AssertAddonRateCardsEqual(t, expected.RateCards, actual.RateCards) }
104-155: Consider refactoring the type-switching logic for billing cadence extractionThe current implementation uses type switching twice (once for each RateCard) to extract the billing cadence. This could be simplified if the RateCard interface had a method to directly access the billing cadence, reducing duplicate code and making future type additions easier to maintain.
You could create a helper function to extract the billing cadence:
+// getBillingCadence extracts billing cadence from different RateCard types +func getBillingCadence(r productcatalog.RateCard) isodate.Period { + switch vv := r.(type) { + case *productcatalog.FlatFeeRateCard: + return lo.FromPtr(vv.BillingCadence) + case *FlatFeeRateCard: + return lo.FromPtr(vv.FlatFeeRateCard.BillingCadence) + case *productcatalog.UsageBasedRateCard: + return vv.BillingCadence + case *UsageBasedRateCard: + return vv.UsageBasedRateCard.BillingCadence + } + return isodate.Period{} +} func AssertRateCardEqual(t *testing.T, r1, r2 productcatalog.RateCard) { // ...existing code... - var billingCadence1, billingCadence2 isodate.Period - - switch vv := r1.(type) { - case *productcatalog.FlatFeeRateCard: - billingCadence1 = lo.FromPtr(vv.BillingCadence) - case *FlatFeeRateCard: - billingCadence1 = lo.FromPtr(vv.FlatFeeRateCard.BillingCadence) - case *productcatalog.UsageBasedRateCard: - billingCadence1 = vv.BillingCadence - case *UsageBasedRateCard: - billingCadence1 = vv.UsageBasedRateCard.BillingCadence - } - - switch vv := r2.(type) { - case *productcatalog.FlatFeeRateCard: - billingCadence2 = lo.FromPtr(vv.BillingCadence) - case *FlatFeeRateCard: - billingCadence2 = lo.FromPtr(vv.FlatFeeRateCard.BillingCadence) - case *productcatalog.UsageBasedRateCard: - billingCadence2 = vv.BillingCadence - case *UsageBasedRateCard: - billingCadence2 = vv.UsageBasedRateCard.BillingCadence - } + billingCadence1 := getBillingCadence(r1) + billingCadence2 := getBillingCadence(r2) assert.Equal(t, billingCadence1, billingCadence2) }openmeter/productcatalog/addon.go (2)
37-37: Address theFIXMEcomment.There's a
FIXMEat line 37 suggesting a consolidation ofAddonEffectivePeriodwithEffectivePeriod. If there's a plan to unify or refactor these classes, performing it soon can help avoid duplicated logic and maintain consistency across the codebase.
54-54: Update docstring reference to reflect "Addon" instead of "Plan".The docstring for
Statusat line 54 mentions "Plan", but the function is defined onAddonEffectivePeriod. Consider renaming it to avoid confusion.-// Status returns the current status of the Plan +// Status returns the current status of the Addonopenmeter/productcatalog/addon/httpdriver/mapping.go (1)
64-64: Plan next steps for consolidation.A
FIXMEat line 64 notes the possibility of consolidating HTTP mappers into theproductcatalogpackage. Consider addressing this soon to minimize duplication and keep the codebase easier to maintain.openmeter/productcatalog/addon/event.go (1)
56-64: Streamline error handling.Each
Validate()method uses avar errs []errorbut ultimately checks only one or two conditions before returning. If there are no further validations to accumulate inerrs, consider removing the slice and returning errors immediately to simplify the code.func (e AddonCreateEvent) Validate() error { - var errs []error if e.Addon == nil { return fmt.Errorf("add-on is required") } - return errors.Join(errs...) + return nil }Also applies to: 99-107, 142-154, 189-197, 232-240
openmeter/productcatalog/addon/adapter/adapter.go (1)
17-22: Configuration struct is straightforward.Fields for the PostgreSQL client and logger are cleanly defined. Consider adding doc comments describing intended usage for future maintainability.
openmeter/productcatalog/addon/adapter/mapping.go (2)
16-65: fromAddonRow function.
- Properly populates the
addon.Addonfields from the Ent row.- Calls
aa.Validate()at the end (line 60) to ensure correctness.- Minor naming mismatch in the error message at line 61: it says “invalid plan phase” when referencing add-ons. Consider updating for clarity.
- return nil, fmt.Errorf("invalid plan phase %s: %w", aa.ID, err) + return nil, fmt.Errorf("invalid add-on %s: %w", aa.ID, err)
67-146: fromAddonRateCardRow function.
- Nicely consolidates metadata, feature references, and billing cadence.
- Switch-case on
r.Typebuilds the correct rate card variant.- On line 138: returning an error with
%wincludeserr, buterrmay benilin that context. Suggest constructing a fresh error or removing the%w.- return nil, fmt.Errorf("invalid AddonRateCard type %s: %w", r.Type, err) + return nil, fmt.Errorf("invalid AddonRateCard type %s", r.Type)openmeter/productcatalog/addon/ratecard_test.go (1)
19-174: Consider freezing times used in tests for predictability
Usingtime.Now()within tests can lead to flaky outcomes due to system time variance. Consider using fixed timestamps or a time-freezing library to ensure consistent, reproducible test runs.openmeter/ent/db/addonratecard_delete.go (1)
15-57: Caution withExecXpanic
Auto-generated ent code typically providesExecXmethods for convenience, but be mindful that it panics on error. Consider usingExecwhen a controlled error return is desired.openmeter/productcatalog/addon/adapter/adapter_test.go (2)
240-303: Consider testing partial rate card updates.The update test replaces existing rate cards entirely. If partial updates or incremental changes are anticipated, adding specialized tests would help ensure correctness under those scenarios.
305-325: Validate exclusion of soft-deleted add-ons whenIncludeDeletedis false.Although the delete test confirms that the add-on remains retrievable post-deletion (soft-delete), it's worth confirming that the same add-on does not appear in list results when
IncludeDeleted = false. Consider adding a test case to validate the intended behavior.openmeter/productcatalog/addon/httpdriver/addon.go (4)
75-77: Fix the typo in the error message.The error message has a minor spelling mistake: "pahse" should be "phase."
- return ListAddonsResponse{}, fmt.Errorf("failed to cast add-on pahse: %w", err) + return ListAddonsResponse{}, fmt.Errorf("failed to cast add-on phase: %w", err)
304-304: Address the TODO for EffectivePeriod.There's a placeholder indicating that the
PublishAddonRequestdoesn't yet allow fully setting the EffectivePeriod. If this is needed soon, consider implementing or tracking in a new issue.
349-349: Follow up on the TODO for setting EffectivePeriod.To.Similarly, there's another placeholder for archiving that relies on a future enhancement to the API schema. Make sure to handle this before release to avoid runtime confusion.
324-324: Use matching response type for PublishAddon.This handler returns
PublishAddonResponsebut usesGetAddonResponsein the response encoder. Although both map toapi.Addon, aligning the types helps avoid confusion.- commonhttp.JSONResponseEncoderWithStatus[GetAddonResponse](http.StatusOK), + commonhttp.JSONResponseEncoderWithStatus[PublishAddonResponse](http.StatusOK),openmeter/productcatalog/addon/service.go (2)
81-83: Empty validation method for ListAddonsInput.Currently,
Validate()returns nil. If future constraints are needed (e.g., page bounds), consider adding them here.
122-196: UpdateAddonInput: handle “FIXME: validate ratecards”.There’s a comment noting a missing validation step. Ensure it’s implemented when ratecards are fully integrated.
openmeter/ent/db/addonratecard.go (2)
65-70: Clarify the naming for theFeaturesedge.
Despite being named "features," this edge is typed as a single*Feature. If multiple features are expected, you may want to rename it toFeatureor switch the field type to a slice.
124-251: Be cautious with large or sensitive data stored inmetadata.
metadatais stored as a JSON object. If it can include sensitive/PII or large data, consider implementing validations, encryption, or alternative storage.Want help creating an approach to sanitize or encrypt metadata before saving?
openmeter/ent/db/mutation.go (1)
111-2839: Comprehensive mutation logic introduced for Addon and AddonRateCard entities.
- The generated mutation methods (e.g., setters, getters, clearers) follow the standard ent pattern.
- Concurrency aspects: the
sync.Onceusage for old-value retrieval is a common ent pattern, but ensure that concurrent updates rely on transactions to prevent stale reads or lost updates if multiple threads mutate the same entity at once.- Validation: consider adding hooks or constraints if certain fields (e.g.,
namespace,version, ordeleted_at) require specific conditions or if certain states should not coexist (e.g., disallow clearingdeleted_atwhile an entity is active, etc.).- Testing: ensure thorough unit tests cover the new edges (
ratecards) for addition, removal, clearing, and updating, especially around partial or simultaneous updates.Everything else aligns well with ent’s recommended approach.
openmeter/productcatalog/addon/service/addon.go (9)
21-21: Use consistent naming in error message.
The string"invalid list Plans params"should be adjusted to"invalid list Addons params"for consistency with theListAddonsfunction.- return pagination.PagedResponse[addon.Addon]{}, fmt.Errorf("invalid list Plans params: %w", err) + return pagination.PagedResponse[addon.Addon]{}, fmt.Errorf("invalid list Addons params: %w", err)
139-141: Inconsistent log comment ("plan" vs "add-on").
Line 141 says “Emit plan created event” while line 139 logs "add-on created." Use “add-on” for both references to keep naming consistent.
165-165: Rename log message to “deleting add-on.”
Currently, the log says"deleting Plan"but this code deals with add-ons.- logger.Debug("deleting Plan") + logger.Debug("deleting add-on")
199-199: Fix comment reference for deleted entity.
This comment references “deleted Plan,” but it should read “deleted add-on” for consistency.- // Get the deleted Plan to emit the event + // Get the deleted add-on to emit the event
210-210: Inconsistent comment referencing “plan” instead of “add-on.”
Use “add-on” here for consistency with the rest of the code.- // Emit plan deleted event + // Emit add-on deleted event
242-242: Graceful handling of not found errors.
There's aFIXMEcomment to handle a “not found” error properly. You can use theIsNotFoundfunction fromerrors.goto return a more specific error.Do you want me to open a new issue for refining the error handling here?
300-300: Typographical error.
Change"failed to udpate aa-on"to"failed to update add-on".- return nil, fmt.Errorf("failed to udpate aa-on: %w", err) + return nil, fmt.Errorf("failed to update add-on: %w", err)
304-304: Inconsistent log message referencing “Plan.”
Use “add-on” to match the resource being updated.- logger.Debug("Plan updated") + logger.Debug("add-on updated")
361-361: Minor grammar improvement.
Replace “then” with “than” to improve clarity.- // the add-on to be published has higher version then 1 + // the add-on to be published has higher version than 1openmeter/productcatalog/addon/ratecard.go (2)
169-176: CallRateCardManagedFields.Validate()for comprehensive validation.
Currently,FlatFeeRateCard.Validate()invokesr.FlatFeeRateCard.Validate()but does not validater.RateCardManagedFields. Consider also callingr.RateCardManagedFields.Validate()to catch empty namespace, ID, or add-on ID.func (r *FlatFeeRateCard) Validate() error { var errs []error + if err := r.RateCardManagedFields.Validate(); err != nil { + errs = append(errs, err) + } if err := r.FlatFeeRateCard.Validate(); err != nil { errs = append(errs, err) } return models.NewNillableGenericValidationError(errors.Join(errs...)) }
268-276: CallRateCardManagedFields.Validate()for usage-based rate cards.
Similarly, include validation ofr.RateCardManagedFieldsinsideUsageBasedRateCard.Validate()so the namespace, ID, and add-on ID are verified.func (r *UsageBasedRateCard) Validate() error { var errs []error + if err := r.RateCardManagedFields.Validate(); err != nil { + errs = append(errs, err) + } if err := r.UsageBasedRateCard.Validate(); err != nil { errs = append(errs, err) } return models.NewNillableGenericValidationError(errors.Join(errs...)) }openmeter/ent/db/addon_update.go (1)
254-262: Consider extending validations incheck()for additional fields if needed.
Currently, only theversionfield is strictly validated. For completeness, you might add boundary checks or structured constraints for other fields (e.g.,name,effective_from,effective_to). This can prevent unexpected data states and enhance reliability.openmeter/ent/db/addonratecard_update.go (1)
308-327: Validate completeness of field-level checks.
Thecheck()method appropriately validates fields likeentitlement_template,tax_config,price, andaddon_id. If additional constraints or business rules (e.g., name or feature key constraints) exist, consider adding them here to centralize validation.openmeter/ent/db/addonratecard_create.go (3)
22-29: Ensure any additional builder logic is added in custom code rather than the generated builder.
This struct (AddonRateCardCreate) is automatically generated by Ent. If you need complex or domain-specific validations or transformations before inserting records, consider configuring them either in custom templates or in your own helper functions instead of editing this generated file.Would you like me to open a new issue or provide an example of how to incorporate custom logic in Ent code generation templates?
202-206: Avoid accidental divergence between SetAddon and SetAddonID.
TheSetAddon(... *Addon)method simply delegates toSetAddonID(a.ID). If there's any logic required when linking an existingAddon, ensure that both calling patterns remain in sync. Otherwise, consumers of these methods may inadvertently bypass future checks if one method is modified.You might reduce the potential for confusion by removing one of the methods or by adding clarifying documentation about their intended usage.
365-486: Efficient specification assembly but consider large JSON field implications.
TheMetadata,EntitlementTemplate,TaxConfig,Price, andDiscountsfields are likely stored in JSON columns. If these objects grow large or need partial querying, you might consider additional indexing or a more structured schema. Otherwise, performance is fine for smaller payloads.If you expect these fields to be large or frequently queried by sub-field, consider using a dedicated table or a specialized search index for filtering.
🛑 Comments failed to post (5)
openmeter/productcatalog/addon/httpdriver/mapping.go (1)
548-556:
⚠️ Potential issueCorrect erroneous error message.
When casting a
UsageBasedRateCardin theAsRateCardfunction (lines 548–556), the error string still referencesFlatFeeRateCard, which is misleading and likely a copy-paste leftover. Update it to refer toUsageBasedRateCard.- if err != nil { - return nil, fmt.Errorf("failed to cast FlatFeeRateCard: %w", err) - } + if err != nil { + return nil, fmt.Errorf("failed to cast UsageBasedRateCard: %w", err) + }📝 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.usage, err := r.AsRateCardUsageBased() if err != nil { return nil, fmt.Errorf("failed to cast FlatFeeRateCard: %w", err) } usageBasedRateCard, err := AsUsageBasedRateCard(usage) if err != nil { return nil, fmt.Errorf("failed to cast UsageBasedRateCard: %w", err) }openmeter/productcatalog/addon/adapter/addon.go (2)
55-55: 🛠️ Refactor suggestion
Handle empty
orFiltersto avoid unintended filtering behavior.If no filter conditions (IDs, Keys, KeyVersions, Currencies) are specified,
orFiltersremains empty, causingquery.Where(addondb.Or(orFilters...))to evaluate to an empty predicate. This might result in a no-op or unexpected behavior. Consider skipping theOr(...)call whenorFiltersis empty, or adding a default true-filter if intended.
113-114:
⚠️ Potential issueUse the correct ordering predicate for
OrderByKey.Currently, the
OrderByKeycase appliesaddondb.ByVersion(order...)instead of ordering byKey. This appears to be a logical error. Update the query to useaddondb.ByKey(order...)for theOrderByKeycase.-case addon.OrderByKey: - query = query.Order(addondb.ByVersion(order...)) +case addon.OrderByKey: + query = query.Order(addondb.ByKey(order...))📝 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.case addon.OrderByKey: query = query.Order(addondb.ByKey(order...))openmeter/ent/db/setorclear.go (1)
156-161: 💡 Verification agent
🧩 Analysis chain
Potential nil-pointer dereference in double-pointer fields
Methods handling**productcatalog.EntitlementTemplate,**productcatalog.TaxConfig,**productcatalog.Price, and**productcatalog.Discountsrisk a nil-pointer panic ifvalue != nilbut*value == nil. Consider adding an extra check to handle that edge case safely, for example:if value == nil || *value == nil { - return u.ClearEntitlementTemplate() + // Ensures we gracefully handle a pointer-to-nil scenario return u.ClearEntitlementTemplate() } return u.SetEntitlementTemplate(*value)Would you like to run a quick code search to confirm actual usage patterns of these double-pointer functions?
Also applies to: 163-168, 170-175, 177-182, 198-203, 205-210, 226-231, 233-238
🏁 Script executed:
#!/bin/bash # Searching usage of the double-pointer methods rg -A 5 "SetOrClearEntitlementTemplate\\(|SetOrClearTaxConfig\\(|SetOrClearPrice\\(|SetOrClearDiscounts\\("Length of output: 10228
Action: Add Additional Nil Check in Double-Pointer Methods
The current implementations in functions like
SetOrClearEntitlementTemplate,SetOrClearTaxConfig,SetOrClearPrice, andSetOrClearDiscountsonly check whether the pointer itself is nil. This can lead to a nil-pointer panic ifvalueis not nil but*valueis nil. To safeguard against this, please update the condition to check for bothvalue == niland*value == nil.For example, change:
if value == nil { return u.ClearEntitlementTemplate() } return u.SetEntitlementTemplate(*value)to:
if value == nil || *value == nil { // Gracefully handle pointer-to-nil scenario return u.ClearEntitlementTemplate() } return u.SetEntitlementTemplate(*value)Affected functions (all in
openmeter/ent/db/setorclear.go):
- In both
AddonRateCardUpdateandAddonRateCardUpdateOnemethods for:
SetOrClearEntitlementTemplateSetOrClearTaxConfigSetOrClearPriceSetOrClearDiscounts- In both
PlanRateCardUpdateandPlanRateCardUpdateOnemethods for the same fields.- In both
SubscriptionItemUpdateandSubscriptionItemUpdateOnemethods for the same fields.Please apply similar changes to all double-pointer methods to prevent unexpected nil-pointer dereferences.
openmeter/ent/db/addon_create.go (1)
127-153: 🛠️ Refactor suggestion
Consider enforcing a logical relationship between
EffectiveFromandEffectiveTo.Currently there is no domain check ensuring that
EffectiveFrom≤EffectiveTo, or that these fields obey any particular ordering (e.g., ensuringEffectiveTois in the future). If such a constraint is required by your business rules, implementing a validation before saving will help avoid erroneous data in the database.I can help you add a concise validation in
ac.check()to ensureEffectiveTois chronologically greater than or equal toEffectiveFrom. Would you like me to propose a snippet?Also applies to: 365-395
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (25)
app/common/productcatalog.go (1)
74-95: Consider wrapping the error from addonservice.New
While the code properly handles errors fromaddonadapter.New, ifaddonservice.Newitself fails, it’s currently returned without context. To aid debugging, consider wrapping that error with additional context. Example:func NewAddonService( // ... ) (addon.Service, error) { adapter, err := addonadapter.New(...) if err != nil { return nil, fmt.Errorf("failed to initialize add-on adapter: %w", err) } - return addonservice.New(addonservice.Config{ ... }) + srv, err := addonservice.New(addonservice.Config{ ... }) + if err != nil { + return nil, fmt.Errorf("failed to initialize addonservice: %w", err) + } + return srv, nil }openmeter/productcatalog/addon/adapter/mapping.go (1)
60-62: Misleading error message
The error says "invalid plan phase" instead of referencing the add-on. For clarity, the message should align with the actual entity (add-on).- return nil, fmt.Errorf("invalid plan phase %s: %w", aa.ID, err) + return nil, fmt.Errorf("invalid add-on %s: %w", aa.ID, err)openmeter/productcatalog/addon/assert.go (1)
54-63: Include RateCards comparison for holistic equality.
Currently, this function compares only the basic Addon metadata but not RateCards. If full equality checks are needed, consider invokingAssertAddonRateCardsEqualhere for completeness.func AssertAddonEqual(t *testing.T, expected, actual Addon) { t.Helper() ... assert.Equalf(t, expected.Annotations, actual.Annotations, "annotations mismatch") + AssertAddonRateCardsEqual(t, expected.RateCards, actual.RateCards) }openmeter/productcatalog/addon.go (1)
54-57: Status method’s reliance on current time is convenient.
This approach is fine for runtime checks, but note that test scenarios might require injecting time dependencies. You may want to mock or pass a time source for advanced scenarios.openmeter/productcatalog/addon/httpdriver/mapping.go (2)
745-939: Consider refactoring this large switch-case for better maintainability.The function
AsPricehandles many price types in a single block, making it cumbersome to read and maintain. Splitting these cases into smaller helper functions or using a data-driven approach can improve clarity and testability.Would you like me to open a new issue or draft a potential refactor for this function?
1-1093: Ensure comprehensive test coverage for these mapping functions.Many of these mappers (e.g.,
FromAddon,FromRateCard, etc.) contain complex logic and numerous error paths. Verifying all major branches and error conditions can prevent subtle bugs.Would you like help writing tests or setting up a coverage report for these new functions?
openmeter/productcatalog/addon/event.go (2)
57-64: Remove or utilize the unused slice of errors in the validation method.Although
errsis declared, it’s never populated, and you return immediately on error. Either removeerrsanderrors.Join, or accumulate multiple validation errors in the slice for a comprehensive report.
67-71: Use conventional lowercase parameter naming.The parameter
Addonis capitalized here, which is unusual for function parameters in Go. Following Go naming conventions (e.g.,addon) improves readability and consistency.openmeter/productcatalog/addon/repository.go (1)
12-20: Enhance documentation and testing for repository interface methods.Adding doc comments clarifying the expected repository behavior can help team members implement these methods. Also, consider adding unit or integration tests to validate the repository’s CRUD operations.
openmeter/productcatalog/addon/httpdriver/driver.go (3)
14-16: Consider adding doc comments for clarity.The
Handlerinterface extendsAddonHandlerwithout additional documentation. Adding short doc comments can improve readability and comprehension for other developers, especially when expanding this interface in future iterations.
18-26: Interface size is acceptable, but consider grouping methods logically.The
AddonHandlerinterface enumerates many methods. While it appears consistent with the domain, you might group these methods or split them into smaller interfaces if they grow further, promoting maintainability.
36-43: Consider returning 4xx instead of 5xx for missing namespace.When the namespace is missing, you return a 500 (Internal Server Error). Typically, missing or invalid request parameters are treated as client errors, e.g., 400 (Bad Request) or 422 (Unprocessable Entity). Re-check if this scenario aligns better with a 4xx status code.
-return "", commonhttp.NewHTTPError(http.StatusInternalServerError, errors.New("internal server error")) +return "", commonhttp.NewHTTPError(http.StatusBadRequest, errors.New("namespace not provided"))openmeter/productcatalog/addon/ratecard_test.go (1)
19-174: Enhance coverage with negative JSON scenarios.
TestRateCard_JSONtests successful serialization and deserialization for both "FlatFee" and "UsageBased" rate cards. Consider adding negative tests (e.g., invalid JSON structure) to ensure error handling paths are also verified.openmeter/productcatalog/addon/httpdriver/addon.go (4)
75-77: Fix typo in error message.In the error message, the word "pahse" seems to be a typographical error and should be "phase" (or simply "add-on").
- return ListAddonsResponse{}, fmt.Errorf("failed to cast add-on pahse: %w", err) + return ListAddonsResponse{}, fmt.Errorf("failed to cast add-on phase: %w", err)
302-303: Address the TODO items for EffectivePeriod.These TODO markers reference adding support for
EffectivePeriodfields within the API request. If this enhancement is still needed, consider creating an issue or implementing it soon.Do you want me to open a separate issue to track these TODOs?
Also applies to: 347-348
324-330: Unify handler response type naming.For publishing an add-on, the request/response types are
PublishAddonRequestandPublishAddonResponse, yet the encoder is using[GetAddonResponse]. While they may share the same underlying struct, using a clearly matching type (e.g.,[PublishAddonResponse]) can reduce confusion.Consider updating the encoder to keep naming consistently aligned with the publish action or unify the naming throughout.
367-374: Ensure consistent response type in the archiving endpoint.Similar to the publish handler,
ArchiveAddonHandlerreturns anArchiveAddonResponsebut encodes using[GetAddonResponse]. For clarity, either directly reuse the same type or rename the encoder’s type to match the action’s response alias consistently.openmeter/productcatalog/addon/service.go (2)
81-83: Add validations for list parameters.The
Validate()method forListAddonsInputis currently empty. Consider adding checks for valid page sizes, page numbers, or other fields, to ensure users' inputs are within expected ranges.
193-193: Implement the missing ratecard validation.There's a FIXME indicating the need to validate ratecards in the
UpdateAddonInput.Validate()method. Failing to validate ratecards could introduce unexpected behavior.Would you like me to create a follow-up PR or issue to address and implement this validation logic?
openmeter/productcatalog/addon/service/addon.go (2)
139-139: Remove leftover “Plan” references in log messages and comments.The code consistently refers to "add-ons" except in these lines, where it references "Plan." This can create confusion and inconsistency.
Example fix:
- logger.Debug("deleting Plan") + logger.Debug("deleting add-on") - // Emit plan deleted event + // Emit add-on deleted event - logger.Debug("Plan updated") + logger.Debug("add-on updated") - // Emit plan updated event + // Emit add-on updated eventAlso applies to: 163-163, 197-197, 209-209, 263-263, 303-303, 306-306
300-300: Correct spelling in error message."udpate aa-on" should be "update add-on."
- return nil, fmt.Errorf("failed to udpate aa-on: %w", err) + return nil, fmt.Errorf("failed to update add-on: %w", err)openmeter/productcatalog/addon/ratecard.go (4)
61-98: Consider additional checks for empty or malformed input.When
visjson.RawMessageor[]byte, it may be beneficial to verify that the raw data is not empty (or invalid JSON) to proactively catch malformed inputs before unmarshal.Below is an example of how you might insert a brief check near line 70 for completeness:
case json.RawMessage, []byte: b := any(v).([]byte) + if len(b) == 0 { + return nil, errors.New("empty rate card data provided") + } serde := &productcatalog.RateCardSerde{} ...
150-167: Remove redundant add-on ID equality check.Line 153 already invokes
RateCardManagedFields.Equal(...), which comparesAddonID. Re-checkingr.AddonID != vv.AddonIDon line 157 is redundant. You can safely remove it to avoid duplication and reduce noise in the equality logic.if !r.RateCardManagedFields.Equal(vv.RateCardManagedFields) { return false } - -if r.AddonID != vv.AddonID { - return false -} return r.FlatFeeRateCard.Equal(&vv.FlatFeeRateCard)
169-177: Validate the managed fields.Currently,
(*FlatFeeRateCard).Validateonly invokesr.FlatFeeRateCard.Validate(). Consider also callingr.RateCardManagedFields.Validate()to ensure consistency with the requiredNamespace,ID, andAddonIDchecks.func (r *FlatFeeRateCard) Validate() error { var errs []error + if err := r.RateCardManagedFields.Validate(); err != nil { + errs = append(errs, err) + } if err := r.FlatFeeRateCard.Validate(); err != nil { errs = append(errs, err) } ... }
249-266: Remove redundant add-on ID equality check, and validate managed fields.
- As with
FlatFeeRateCard,r.RateCardManagedFields.Equal(...)already comparesAddonID, making the repeated check on line 256 redundant.(*UsageBasedRateCard).Validate()omitsr.RateCardManagedFields.Validate(). Including it ensures consistency when validating critical fields.func (r *UsageBasedRateCard) Equal(v productcatalog.RateCard) bool { switch vv := v.(type) { case *UsageBasedRateCard: if !r.RateCardManagedFields.Equal(vv.RateCardManagedFields) { return false } - - if r.AddonID != vv.AddonID { - return false - } return r.UsageBasedRateCard.Equal(&vv.UsageBasedRateCard) ... } func (r *UsageBasedRateCard) Validate() error { var errs []error + if err := r.RateCardManagedFields.Validate(); err != nil { + errs = append(errs, err) + } if err := r.UsageBasedRateCard.Validate(); err != nil { errs = append(errs, err) } ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
app/common/productcatalog.go(4 hunks)cmd/server/main.go(1 hunks)cmd/server/wire.go(2 hunks)cmd/server/wire_gen.go(14 hunks)openmeter/event/metadata/resourcepath.go(1 hunks)openmeter/productcatalog/addon.go(1 hunks)openmeter/productcatalog/addon/adapter/adapter.go(1 hunks)openmeter/productcatalog/addon/adapter/adapter_test.go(1 hunks)openmeter/productcatalog/addon/adapter/addon.go(1 hunks)openmeter/productcatalog/addon/adapter/mapping.go(1 hunks)openmeter/productcatalog/addon/addon.go(1 hunks)openmeter/productcatalog/addon/assert.go(1 hunks)openmeter/productcatalog/addon/errors.go(1 hunks)openmeter/productcatalog/addon/errors_test.go(1 hunks)openmeter/productcatalog/addon/event.go(1 hunks)openmeter/productcatalog/addon/httpdriver/addon.go(1 hunks)openmeter/productcatalog/addon/httpdriver/driver.go(1 hunks)openmeter/productcatalog/addon/httpdriver/errors.go(1 hunks)openmeter/productcatalog/addon/httpdriver/mapping.go(1 hunks)openmeter/productcatalog/addon/ratecard.go(1 hunks)openmeter/productcatalog/addon/ratecard_test.go(1 hunks)openmeter/productcatalog/addon/repository.go(1 hunks)openmeter/productcatalog/addon/service.go(1 hunks)openmeter/productcatalog/addon/service/addon.go(1 hunks)openmeter/productcatalog/addon/service/service.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- cmd/server/wire.go
- cmd/server/main.go
- openmeter/productcatalog/addon/httpdriver/errors.go
- openmeter/productcatalog/addon/errors_test.go
- openmeter/productcatalog/addon/adapter/adapter.go
- openmeter/productcatalog/addon/addon.go
- openmeter/productcatalog/addon/errors.go
- openmeter/event/metadata/resourcepath.go
- openmeter/productcatalog/addon/service/service.go
🧰 Additional context used
🧬 Code Definitions (13)
app/common/productcatalog.go (2)
openmeter/productcatalog/feature/connector.go (1)
FeatureConnector(27-34)openmeter/productcatalog/feature/feature.go (1)
Feature(87-110)
openmeter/productcatalog/addon/repository.go (3)
openmeter/productcatalog/addon/service.go (5)
ListAddonsInput(48-79)CreateAddonInput(98-101)DeleteAddonInput(231-233)GetAddonInput(198-211)UpdateAddonInput(122-142)openmeter/productcatalog/addon.go (1)
Addon(177-182)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)
openmeter/productcatalog/addon/ratecard_test.go (3)
openmeter/productcatalog/addon/ratecard.go (4)
FlatFeeRateCard(102-105)RateCardManagedFields(19-25)UsageBasedRateCard(200-203)NewRateCardFrom(61-98)openmeter/productcatalog/addon/adapter/adapter.go (1)
Config(19-22)openmeter/productcatalog/addon/service/service.go (1)
Config(12-17)
openmeter/productcatalog/addon/httpdriver/driver.go (1)
openmeter/productcatalog/addon/httpdriver/addon.go (7)
ListAddonsHandler(28-28)CreateAddonHandler(102-102)DeleteAddonHandler(197-197)GetAddonHandler(247-247)UpdateAddonHandler(149-149)PublishAddonHandler(291-291)ArchiveAddonHandler(336-336)
openmeter/productcatalog/addon/httpdriver/addon.go (1)
openmeter/productcatalog/addon/httpdriver/mapping.go (3)
FromAddon(23-62)AsCreateAddonRequest(478-507)AsUpdateAddonRequest(1071-1092)
openmeter/productcatalog/addon.go (2)
openmeter/productcatalog/ratecard.go (2)
RateCards(350-350)RateCard(28-38)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)
openmeter/productcatalog/addon/assert.go (4)
openmeter/productcatalog/addon/service.go (2)
CreateAddonInput(98-101)UpdateAddonInput(122-142)openmeter/productcatalog/addon.go (1)
Addon(177-182)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)openmeter/productcatalog/ratecard.go (4)
RateCards(350-350)RateCard(28-38)FlatFeeRateCard(161-168)UsageBasedRateCard(256-262)
openmeter/productcatalog/addon/adapter/addon.go (3)
openmeter/productcatalog/addon/service.go (10)
ListAddonsInput(48-79)OrderBy(33-33)OrderByCreatedAt(29-29)OrderByUpdatedAt(30-30)OrderByKey(27-27)OrderByID(26-26)CreateAddonInput(98-101)DeleteAddonInput(231-233)GetAddonInput(198-211)UpdateAddonInput(122-142)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)openmeter/productcatalog/addon/errors.go (3)
IsNotFound(65-72)NewNotFoundError(19-49)NotFoundErrorParams(12-17)
cmd/server/wire_gen.go (12)
cmd/server/wire.go (1)
Application(40-76)app/common/productcatalog.go (3)
NewFeatureConnector(41-49)NewAddonService(74-94)Addon(37-39)openmeter/productcatalog/feature/connector.go (1)
NewFeatureConnector(84-100)app/common/progressmanager.go (2)
ProgressManager(14-16)NewProgressManager(19-39)app/common/streaming.go (1)
NewStreamingConnector(21-43)app/common/entitlement.go (1)
NewEntitlementRegistry(22-40)openmeter/app/sandbox/app.go (1)
App(35-39)openmeter/billing/service.go (1)
Service(9-19)openmeter/customer/service.go (1)
Service(10-13)openmeter/app/service/service.go (1)
Service(12-15)openmeter/meter/service/service.go (1)
Service(13-15)openmeter/progressmanager/service.go (1)
Service(9-11)
openmeter/productcatalog/addon/adapter/adapter_test.go (6)
openmeter/productcatalog/addon/service.go (5)
CreateAddonInput(98-101)GetAddonInput(198-211)ListAddonsInput(48-79)UpdateAddonInput(122-142)DeleteAddonInput(231-233)openmeter/productcatalog/addon.go (4)
Addon(177-182)AddonMeta(94-117)AddonEffectivePeriod(38-44)AddonStatus(22-22)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)openmeter/productcatalog/addon/ratecard.go (3)
UsageBasedRateCard(200-203)RateCardManagedFields(19-25)FlatFeeRateCard(102-105)openmeter/productcatalog/addon/service/service_test.go (1)
MonthPeriod(303-303)openmeter/productcatalog/addon/assert.go (3)
AssertAddonCreateInputEqual(14-26)AssertAddonEqual(54-63)AssertAddonUpdateInputEqual(28-52)
openmeter/productcatalog/addon/httpdriver/mapping.go (2)
openmeter/productcatalog/addon/httpdriver/driver.go (1)
New(45-55)openmeter/productcatalog/addon/httpdriver/addon.go (2)
CreateAddonRequest(100-100)UpdateAddonRequest(147-147)
openmeter/productcatalog/addon/ratecard.go (1)
openmeter/productcatalog/ratecard.go (4)
FlatFeeRateCard(161-168)UsageBasedRateCard(256-262)RateCard(28-38)RateCardSerde(40-42)
openmeter/productcatalog/addon/event.go (3)
openmeter/productcatalog/addon.go (1)
Addon(177-182)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)openmeter/event/metadata/resourcepath.go (2)
ComposeResourcePath(28-30)EntityAddon(20-20)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (56)
app/common/productcatalog.go (3)
13-15: No issues found with the new add-on imports
The import statements appear correct and align with the new add-on functionality.
26-26: Addon injection set
IncludingAddon,within theProductCatalogwire set looks appropriate and consistent with the existing plan and feature sets.
37-39: Wire set creation
Definingvar Addon = wire.NewSet(NewAddonService)is consistent with the established wire pattern. No concerns here.cmd/server/wire_gen.go (14)
26-26: Import of theaddonpackage
This import is necessary to reference the new add-on functionality. No issues found.
90-98: Meter adapter creation
The snippet properly handles errors by invoking cleanup functions before returning. Looks good.
150-151: Initializing the FeatureConnector
Constructing thefeatureConnectorwith the required parameters is coherent with the existing pattern. No issues found.
161-162: InitializeappService
No immediate concerns. Error handling is consistent with the rest of the application’s service creation.
173-174: Tracer setup
The tracer is created following the existing telemetry pattern. No concerns.
186-187: Progress manager initialization
The function checks if the configuration is enabled and handles errors appropriately.
197-198: Streaming connector creation
Error cases are well-managed with cleanup calls. Implementation is consistent with other connectors.
207-207: Entitlement registry instantiation
The approach aligns with existing patterns. No issues found.
261-262: BillingService initialization
Constructing the billing service with multiple dependencies is handled gracefully. No concurrency or error-handling issues noted.
271-271: AppStripe service
The snippet ensures proper cleanup on errors, consistent with the pattern used in other services.
313-314: AppSandboxProvisioner
Dependencies are injected appropriately. Error handling and cleanup calls match the established approach.
367-367: MeterEventService
ConstructingmetereventServicewith(connector, service)follows the same injection pattern. No concerns.
424-425: Add-on and App injection into Application
AssigningaddonServiceandappServiceto the application struct is correct for enabling new add-on capabilities.
475-475: NewAddonfield in theApplicationstruct
This addition is consistent with the introduced add-on service.openmeter/productcatalog/addon/adapter/mapping.go (4)
1-14: New adapter mapping file
All necessary imports for the add-on mapping logic appear correct and sufficiently scoped.
16-59:fromAddonRowfunction
Creates anaddon.Addonobject from a database row. The loop overEdges.Ratecardsis clear, and the validation call is a good practice.
67-146:fromAddonRateCardRowfunction
Thoroughly constructs theRateCardfrom row data. Error conditions for billing cadence and unknown card types are properly handled.
148-189:asAddonRateCardRowfunction
Executes the reverse mapping fromRateCardto database entities. The type-switch ensures correct handling of each rate card subtype.openmeter/productcatalog/addon/assert.go (4)
14-26: Well-structured field-by-field comparison for Addon creation.
This function comprehensively checks each field in both the input and the resulting Addon, including RateCards. It provides clarity and is a sound approach for ensuring data integrity.
28-52: Good use of pointer logic for partial updates.
Only checking non-nil fields respects partial updates, preventing overwriting of unspecified fields. This is a neat approach to test real-world update scenarios.
65-102: Thorough RateCards comparison approach.
Mapping each slice into a map by key and verifying all entries ensures correct one-to-one matching. This is efficient and avoids misaligned index issues in a direct slice comparison.
104-155: Comprehensive RateCard equality checks.
Verifying type, metadata, feature, tax config, and billing cadence covers the critical fields. This ensures minimal risk of unnoticed discrepancies between RateCard instances.openmeter/productcatalog/addon/adapter/adapter_test.go (8)
55-88: Well-organized test setup.
The use of test fixtures to initialize Postgres and the logging infrastructure is clear and maintainable. It provides a solid base for testing repository methods.
89-102: Feature creation step is logically placed.
Creating a feature before creating the addon ensures all dependencies are set up correctly. This aligns well with the test flow.
148-155: Test coverage for addon creation is excellent.
Validating the newly created addon withAssertAddonCreateInputEqualthoroughly checks correctness right after insertion.
157-201: Robust approach testing multiple retrieval paths.
Fetching add-ons by ID, Key, and Key+Version ensures comprehensive coverage of read scenarios. This test suite effectively verifies that all lookup mechanisms are correct.
203-239: Listing queries correctly validate filtering criteria.
Ensuring filters by IDs and keys individually or in combination helps confirm that the pagination and filtering logic behaves as expected.
241-303: Update test covers effective period and optional fields thoroughly.
Testing the entire update flow, including optional fields like Metadata and RateCards, helps guarantee that partial updates are applied correctly.
305-325: Confirming soft-delete behavior.
After deletion, the test retrieves the same addon and checks equality. If the intended design is for a "soft delete", this is correct. Otherwise, consider asserting the record is truly gone if a hard-delete is intended.Is the deletion meant to be logical (soft-delete) or physical removal from the database? If physical, the test should confirm no record is retrievable afterward.
327-462: Status filter testing is quite comprehensive.
Testing draft, active, archived, and invalid states at different timeline scenarios validates the correctness of scheduling and versioned states. The thorough coverage of these cases is commendable.openmeter/productcatalog/addon.go (10)
37-44: Clear definition of AddonEffectivePeriod.
The struct is straightforward, allowing optional from/to times. It's a good foundation for implementing versioned or time-bound add-on behaviors.
46-52: Validation handles invalid periods gracefully.
Returning an error whenEffectiveTois beforeEffectiveFromis a good safeguard against illogical time ranges.
59-81: Flexible time-relative status checks.
StatusAtis a nice extension, supporting scenarios where historical or future statuses need to be known. Keep an eye on edge cases (e.g., if onlyEffectiveTois set) to confirm behavior matches user expectations.Would you like a script to seek occurrences where only
EffectiveTois set without anEffectiveFromto ensure consistent status logic?
83-87: Equality logic for effective periods is concise.
Comparing the zero values as defaults works well, ensuring that absent times are considered logically equal to zero.
94-117: AddonMeta fields adequately capture addon metadata.
Key, version, name, and currency are well-defined, and descriptions are optional. The design fosters flexibility with minimal overhead.
119-139: Robust AddonMeta validation.
These checks prevent empty keys, names, or invalid currency. Aggregating errors withNewNillableGenericValidationErroris a clean approach.
141-175: Comprehensive equality checks in AddonMeta.
All relevant fields (versioning, metadata, periods, etc.) are compared carefully. Usingmaps.Equalfor annotations is a neat solution to compare map contents.
177-182: Struct composition fosters clarity.
EmbeddingAddonMetainsideAddonis a solid design choice that avoids repetitive fields and keeps the domain model consistent.
184-209: Validation ensures RateCards integrity.
Preventing duplicate RateCard keys and validating each entry helps reduce runtime errors. This is vital for a payment or subscription system.
211-217: Concat equality check for metadata and RateCards.
The combinedMeta(viaAddonMeta) andRateCards.Equalcheck ensures a thorough comparison for the entire Addon struct.openmeter/productcatalog/addon/httpdriver/mapping.go (1)
64-64: Address the existing FIXME to consolidate common HTTP mappers.This is the same reminder from a previous review: refactoring shared logic into a common package (e.g.,
productcatalog) will reduce duplication, improve consistency, and simplify maintainability.openmeter/productcatalog/addon/httpdriver/driver.go (2)
30-34: Struct initialization appears straightforward.The
handlerstruct neatly aggregates theservice,namespaceDecoder, andoptions. This design facilitates a clean separation of concerns, and no concurrency or correctness issues are apparent.
45-55: Factory function implementation is solid.
Newcorrectly instantiates a privatehandlerthat implements theHandlerinterface. It's a clean pattern aligning with Go's recommendation to return interfaces from constructor functions.openmeter/productcatalog/addon/ratecard_test.go (2)
176-316: Validation checks appear thorough.
TestFlatFeeRateCardrightly covers both valid and invalid rate card scenarios. The sub-tests adequately validate negative amounts, invalid payment terms, and questionable JSON configurations.
318-474: Usage-based tests mirror flat fee logic effectively.
TestUsageBasedRateCardprovides strong coverage for valid and invalid states, including boundary checks (e.g., negative amounts, inverted commitments). The approach is consistent and effective.openmeter/productcatalog/addon/adapter/addon.go (7)
24-153: Ordered, paginated list logic is well structured.Aside from the potential empty filter concern, the rest of
ListAddonseffectively applies status-based filtering, eager loading of rate cards, sorting, and pagination. The approach aligns with best practices inent.
154-216: Composite uniqueness for add-ons remains unenforced.Creating add-ons does not appear to enforce
(namespace, key, version)uniqueness. This was previously flagged in earlier reviews. Without a composite index or constraint, duplicates can still be inserted.
218-253: Rate card bulk creation implementation is straightforward.The
rateCardBulkCreatefunction correctly constructs a series ofentdb.AddonRateCardCreatebuilders and returns them for bulk insertion, with no apparent issues.
255-300: Soft-delete logic is properly handled.
DeleteAddonsetsDeletedAtinstead of permanently removing the record, consistent with typical soft-delete patterns. Error handling for not-found cases is also well-structured.
302-399: Robust retrieval with fallback logic.
GetAddonhandles multiple retrieval modes (by ID, by key/version, latest active) and includes thorough error handling. Eager loading of rate cards is integrated seamlessly.
462-479: Rate card updates overwrite existing entries.This logic fully deletes existing rate cards before re-creating them, matching prior feedback about a "replace-all" approach. If partial updates or concurrency checks are required, further changes are recommended.
506-511: Eager loading with futureDeletedAtsupport.
AddonEagerLoadRateCardsFnrecognizes rate cards whoseDeletedAtis either null or in the future, enabling planned inactivation. This approach appears aligned with scheduled removal patterns.openmeter/productcatalog/addon/ratecard.go (1)
27-37: Looks Good: Cohesive field-based equality checks.The
RateCardManagedFields.Equalmethod correctly ensures thatNamespace,ID, andAddonIDare identical before returningtrue. This is a clear and straightforward approach to equality checks.
chrisgacsal
force-pushed
the
feat/addons-api
branch
from
371a487 to
1f7c3c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (21)
openmeter/productcatalog/addon/adapter/mapping.go (3)
42-58: Add debug logging for skippednilratecard edges.When iterating over
a.Edges.Ratecards, anyniledges are simply skipped. Consider logging or tracing that skip to aid troubleshooting if a partially loaded or missing related record leads to unexpected results.for _, edge := range a.Edges.Ratecards { if edge == nil { + // log skipped nil edge for tracing continue } ...
60-65: Ensure external callers handle wraparound validation errors.The code calls
aa.Validate()before returning. This consolidates errors nicely. However, ensure external or higher-level callers wrap or handle the returned error so that domain-related errors can be surfaced accurately to users or logs.
148-188: Consolidate repeated billing cadence extraction inasAddonRateCardRow.In the switch statement for casting
RateCard, each case callsv.<Variant>.BillingCadence.ISOStringPtrOrNil(). Although not harmful, it's slightly repetitive. You could unify the logic in a helper method (similar togetBillingCadencefor reading) to ensure readability and consistency.func asAddonRateCardRow(r productcatalog.RateCard) (entdb.AddonRateCard, error) { ... switch v := r.(type) { case *productcatalog.FlatFeeRateCard, *addon.FlatFeeRateCard, *productcatalog.UsageBasedRateCard, *addon.UsageBasedRateCard: - ratecard.BillingCadence = v.XXX.BillingCadence.ISOStringPtrOrNil() + ratecard.BillingCadence = extractBillingCadencePtrOrNil(v) ... }openmeter/productcatalog/addon/assert.go (4)
14-26: Improve negative test coverage for create input validation.
AssertAddonCreateInputEqualchecks matches for normal fields but doesn’t explicitly test negative or edge cases (e.g., empty currency). It might be worth adding test scenarios within your suite to ensure that error states or unexpected data are caught early.
28-52: Handle optional fields more consistently inAssertAddonUpdateInputEqual.The code correctly checks optional fields by verifying non-
nilpointers. Consider adding a consistent fallback mechanism or a direct check if a field is set to default to prevent subtle mismatches, particularly around empty strings or cleared metadata.
54-65: Consider factoring repeated RateCard assertion logic into a helper.
AssertAddonEqualinvokesAssertAddonRateCardsEqual, which in turn callsAssertRateCardEqual. If more assertion flows appear, you may centralize some of the repeated mapping logic (like building ther1Mapandr2Map) to reduce code duplication.Also applies to: 67-104
121-151: Ensure robust “metadata mismatch” coverage inAssertRateCardEqual.This function uses
.Equalchecks for metadata, tax configurations, and more, which is great. For thorough coverage, consider adding specialized tests for each mismatch scenario (e.g., missing metadata keys, different tax structures) so that test failures remain highly actionable.openmeter/productcatalog/addon.go (1)
177-217: Expand checks forRateCardsconcurrency or integrity.The
Addonstruct’sValidate()ensures no duplicate keys, but doesn’t check concurrency or ordering constraints if multiple rate cards are updated in parallel. If multiple writes could conflict, consider applying concurrency controls or version checks to avoid data races or conflict overwrites.openmeter/productcatalog/addon/event.go (3)
22-64: Robust creation event definition.
- The
NewAddonCreateEventfunction andAddonCreateEventstruct properly associate the session'sUserIDwith the newly created add-on.Validate()ensuresAddonis non-nil, preventing potential nil-pointer dereferences.Everything looks good here, though consider adding unit tests for each event type to strengthen reliability.
155-196: Publish event logic is coherent.
- Using the add-on’s
UpdatedAttimestamp for event emission is consistent with other event definitions.- Validation ensures the add-on is non-nil.
Optionally, if you track a dedicated "PublishedAt" in the data model, you could reference that instead for more granularity.
198-239: Archiving event is well-implemented.
- Reliability is improved by reusing
UpdatedAtto capture the time of archival.- Maintains consistent event generation and metadata usage.
A small note: throughout the code, references to “add-on” sometimes differ in hyphenation from “addon.” Standardizing this across event definitions and error messages could enhance readability.
openmeter/productcatalog/addon/httpdriver/addon.go (1)
294-331: PublishAddon handler
Implementation references the newPublishAddonflow. Small typo in error message: "failed to punlish add-on".Proposed fix:
- return PublishAddonResponse{}, fmt.Errorf("failed to punlish add-on [namespace=%s id=%s]: %w", ...) + return PublishAddonResponse{}, fmt.Errorf("failed to publish add-on [namespace=%s id=%s]: %w", ...)openmeter/productcatalog/addon/service.go (3)
20-21: Use consistent naming in error messages.Here, the error message states
"invalid list Plans params", although the code deals with listing add-ons. To prevent confusion, consider renaming it to "invalid list Addons params."- return pagination.PagedResponse[Addon]{}, fmt.Errorf("invalid list Plans params: %w", err) + return pagination.PagedResponse[Addon]{}, fmt.Errorf("invalid list Addons params: %w", err)
49-51: Consider handling pagination as optional.The TODO comment suggests making pagination optional. If desired, you could adopt default pagination values or allow zero values for page size and page number to indicate unbounded queries. This might simplify the API for use cases where pagination is unnecessary.
Do you want me to open a new issue or propose a helper function for optional pagination?
193-193: Add ratecard validation logic.There's a
FIXMEindicating ratecards aren't fully validated. Omitting validation may allow inconsistent ratecards to slip through. Consider implementing checks to ensure ratecards meet business rules (e.g., billing cadence alignment, feature references, etc.).Would you like me to draft a ratecards validation method?
openmeter/productcatalog/addon/service/addon.go (1)
21-21: Eliminate inconsistent references to "Plan" and fix spelling errors.Several lines still reference "Plan" instead of "Add-on" or contain typos:
- Line 21: "invalid list Plans params"
- Lines 108–109: "if there is no version in draft status for new plan"
- Lines 139–142:
"failed to publish plan created event"- Line 163:
"logger.Debug(\"deleting Plan\")"- Line 300:
"failed to udpate aa-on"Using consistent naming improves readability and avoids confusion:
- return pagination.PagedResponse[addon.Addon]{}, fmt.Errorf("invalid list Plans params: %w", err) + return pagination.PagedResponse[addon.Addon]{}, fmt.Errorf("invalid list Add-ons params: %w", err) - // if there is no version in draft status for new plan + // if there is no version in draft status for the new add-on - return nil, fmt.Errorf("failed to publish plan created event: %w", err) + return nil, fmt.Errorf("failed to publish add-on created event: %w", err) - logger.Debug("deleting Plan") + logger.Debug("deleting add-on") - return nil, fmt.Errorf("failed to udpate aa-on: %w", err) + return nil, fmt.Errorf("failed to update add-on: %w", err)Also applies to: 108-109, 139-142, 163-163, 300-300
openmeter/productcatalog/addon/ratecard.go (1)
57-59: Consider unifying or clarifying the role ofManagedRateCard.The
ManagedRateCardinterface exposes a single methodManagedFields() RateCardManagedFields. If future ratecard variants share similar managed fields, consider integrating it into the primaryRateCardinterface, or expanding its purpose. This can simplify the type hierarchy, but be mindful of changes required across implementations.openmeter/productcatalog/addon/httpdriver/mapping.go (4)
1-1: Consider adding tests for these mappings.
This file introduces multiple mapping functions, each covering complex logic with extensive type conversions. It's crucial to have robust unit test coverage to ensure that all edge cases and error paths are tested, particularly where custom error handling is performed.Do you need help creating targeted unit tests for these mapping functions?
338-347: Clarify default payment term behavior.
InFromPaymentTerm, unrecognized payment term types fall back toPricePaymentTermInAdvance, which may lead to silent misconfigurations. Consider returning an error for unknown types or deliberately handling them with a dedicated case.
405-476: Unify discount logic.
The logic inFromDiscountsandFromDiscountPercentagesoverlaps significantly. Combining or refactoring them could reduce duplication and simplify code maintenance, especially around type checks and conversions.
745-939: Refactor nested switch inAsPrice.
This function handles multiple price types, each with its own conversion steps. Consider factoring out each case into smaller, dedicated helper functions or using a type-based strategy. This approach enhances readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
app/common/productcatalog.go(4 hunks)cmd/server/main.go(1 hunks)cmd/server/wire.go(2 hunks)cmd/server/wire_gen.go(14 hunks)openmeter/event/metadata/resourcepath.go(1 hunks)openmeter/productcatalog/addon.go(1 hunks)openmeter/productcatalog/addon/adapter/adapter.go(1 hunks)openmeter/productcatalog/addon/adapter/adapter_test.go(1 hunks)openmeter/productcatalog/addon/adapter/addon.go(1 hunks)openmeter/productcatalog/addon/adapter/mapping.go(1 hunks)openmeter/productcatalog/addon/addon.go(1 hunks)openmeter/productcatalog/addon/assert.go(1 hunks)openmeter/productcatalog/addon/errors.go(1 hunks)openmeter/productcatalog/addon/errors_test.go(1 hunks)openmeter/productcatalog/addon/event.go(1 hunks)openmeter/productcatalog/addon/httpdriver/addon.go(1 hunks)openmeter/productcatalog/addon/httpdriver/driver.go(1 hunks)openmeter/productcatalog/addon/httpdriver/errors.go(1 hunks)openmeter/productcatalog/addon/httpdriver/mapping.go(1 hunks)openmeter/productcatalog/addon/ratecard.go(1 hunks)openmeter/productcatalog/addon/ratecard_test.go(1 hunks)openmeter/productcatalog/addon/repository.go(1 hunks)openmeter/productcatalog/addon/service.go(1 hunks)openmeter/productcatalog/addon/service/addon.go(1 hunks)openmeter/productcatalog/addon/service/service.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- cmd/server/wire.go
- openmeter/productcatalog/addon/httpdriver/errors.go
- cmd/server/main.go
- openmeter/event/metadata/resourcepath.go
- openmeter/productcatalog/addon/service/service.go
- openmeter/productcatalog/addon/httpdriver/driver.go
- openmeter/productcatalog/addon/repository.go
- openmeter/productcatalog/addon/addon.go
- openmeter/productcatalog/addon/adapter/adapter.go
- openmeter/productcatalog/addon/errors.go
- openmeter/productcatalog/addon/adapter/addon.go
🧰 Additional context used
🧬 Code Definitions (7)
app/common/productcatalog.go (2)
openmeter/productcatalog/feature/connector.go (1)
FeatureConnector(27-34)openmeter/productcatalog/feature/feature.go (1)
Feature(87-110)
openmeter/productcatalog/addon/httpdriver/addon.go (1)
openmeter/productcatalog/addon/httpdriver/mapping.go (3)
FromAddon(23-62)AsCreateAddonRequest(478-507)AsUpdateAddonRequest(1071-1092)
openmeter/productcatalog/addon/assert.go (4)
openmeter/productcatalog/addon/service.go (2)
CreateAddonInput(98-101)UpdateAddonInput(122-142)openmeter/productcatalog/addon.go (1)
Addon(177-182)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)openmeter/productcatalog/ratecard.go (4)
RateCards(350-350)RateCard(28-38)FlatFeeRateCard(161-168)UsageBasedRateCard(256-262)
openmeter/productcatalog/addon/ratecard_test.go (2)
openmeter/productcatalog/ratecard.go (4)
RateCard(28-38)FlatFeeRateCard(161-168)RateCardMeta(49-77)UsageBasedRateCard(256-262)openmeter/productcatalog/addon/ratecard.go (4)
FlatFeeRateCard(102-105)RateCardManagedFields(19-25)UsageBasedRateCard(204-207)NewRateCardFrom(61-98)
openmeter/productcatalog/addon/adapter/adapter_test.go (6)
openmeter/productcatalog/addon/service.go (5)
CreateAddonInput(98-101)GetAddonInput(198-211)ListAddonsInput(48-79)UpdateAddonInput(122-142)DeleteAddonInput(231-233)openmeter/productcatalog/addon.go (4)
Addon(177-182)AddonMeta(94-117)AddonEffectivePeriod(38-44)AddonStatus(22-22)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)openmeter/productcatalog/addon/ratecard.go (3)
UsageBasedRateCard(204-207)RateCardManagedFields(19-25)FlatFeeRateCard(102-105)openmeter/productcatalog/addon/service/service_test.go (1)
MonthPeriod(303-303)openmeter/productcatalog/addon/assert.go (3)
AssertAddonCreateInputEqual(14-26)AssertAddonEqual(54-65)AssertAddonUpdateInputEqual(28-52)
openmeter/productcatalog/addon/ratecard.go (3)
openmeter/productcatalog/addon/adapter/adapter.go (1)
New(42-51)openmeter/productcatalog/addon/service/service.go (1)
New(19-42)openmeter/productcatalog/ratecard.go (6)
FlatFeeRateCard(161-168)UsageBasedRateCard(256-262)RateCard(28-38)RateCardSerde(40-42)FlatFeeRateCardType(15-15)UsageBasedRateCardType(16-16)
openmeter/productcatalog/addon/httpdriver/mapping.go (2)
openmeter/productcatalog/addon/httpdriver/driver.go (1)
New(45-55)openmeter/productcatalog/addon/httpdriver/addon.go (2)
CreateAddonRequest(100-100)UpdateAddonRequest(147-147)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Quickstart
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: E2E
- GitHub Check: Developer environment
- GitHub Check: CI
- GitHub Check: Commit hooks
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (43)
openmeter/productcatalog/addon/adapter/mapping.go (3)
16-40: Consider validating Edges fields more defensively.The
fromAddonRowfunction initializes anaddon.Addonby mapping fields fromentdb.Addon. While it handlesnilchecks at the loop level, it may be worth confirming that otherEdgesfields (or nested references) are notnilbefore usage to avoid potential panics in less common scenarios.Would you like a quick script to check for
nilreferences in the adjacency edges across the codebase?
67-95: Review feature attachment logic for potential expansions.The
fromAddonRateCardRowfunction attaches a single feature if available (r.Edges.Features). This may suffice for now, but if multiple features per rate card are eventually required, further expansions could break the one-feature assumption. Keep this in view if future product requirements change.Would you like a script to verify that no existing logic attempts to attach multiple features to a single rate card?
98-146:Details
✅ Verification successful
Proactively handle potential unknown or less common rate card types.
The switch on
r.Typeonly handlesFlatFeeRateCardTypeandUsageBasedRateCardType, and defaults to an error for anything else. This is good for strictness; just ensure your broader application code doesn’t attempt to store or parse new types without an accompanying update here.
🏁 Script executed:
#!/bin/bash # Check for references to potential unknown rate card types in the codebase rg 'RateCardType\s+=\s+' -A 3Length of output: 577
Attention: Rate Card Type Handling Verified
The verification confirmed that only
flat_feeandusage_basedrate card types are defined across the codebase, so handling unknown types by erroring out in the default case remains appropriate. Just ensure that any future rate card type additions are explicitly supported here to avoid runtime errors.openmeter/productcatalog/addon.go (1)
94-175: Add unit tests for boundary currency validations.
AddonMeta.Validate()enforcesm.Currency.Validate(). Ensure test coverage includes invalid or unsupported currency codes, verifying that the system gracefully handles or rejects them.Would you like me to generate a quick script to grep for existing currency tests and confirm coverage of invalid codes?
openmeter/productcatalog/addon/event.go (4)
1-11: Great foundational setup for the add-on package.Defining a dedicated package for add-on events helps keep concerns modular and maintainable. This includes proper imports for context, errors, ULID generation, and the session package.
13-20: Clear and consistent event naming constants.Declaring subsystem and event names in a single block ensures a canonical reference for all event types. Helps maintain clarity in the codebase.
65-107: Good practice in the update event logic.
NewAddonUpdateEventcaptures theUserIDcontext and populates the event.Validate()ensures anAddonis set before proceeding, aligning with best practices for data consistency.Implementation appears sound with no obvious issues.
108-153: Appropriate handling of the delete event.
Validate()enforcesDeletedAtto be non-nil, ensuring that a delete event isn’t generated prematurely.- The event’s time references the actual deletion timestamp for accurate record-keeping.
Approach is correct and thorough.
app/common/productcatalog.go (4)
13-15: Well-structured imports.Including
addon,addonadapter, andaddonservicein a single section clarifies the new add-on dependency. No issues noted here.
26-26: Logical extension of the wire sets.Adding
Addonto theProductCatalogset ensures smooth integration alongsideFeatureandPlan.
37-39: Dedicated wire set for add-ons.Creating a separate
var Addonwire set aligns with the existing pattern forFeatureandPlan. It keeps dependencies organized and easier to maintain.
74-94: Initialization of the add-on service seems robust.
- The
NewAddonServicefunction properly validates dependencies withaddonadapter.Newandaddonservice.New.- Clear error messages guide the user when adapter creation fails.
This approach follows the established pattern for the plan and feature services, ensuring consistency.
openmeter/productcatalog/addon/errors_test.go (1)
1-95: Test coverage for the NotFoundError is comprehensive.
TestIsNotFoundErrorcovers valid, wrapped, and invalid error scenarios, verifying theIsNotFoundfunction.TestIsNotFoundError_Stringaccurately checks the error message format for various parameter combinations.Thank you for addressing the prior feedback about referencing “addon” rather than “plan.” This file now looks well-aligned with add-on error definitions.
cmd/server/wire_gen.go (14)
26-26: Import used for add-on support
This newly introduced import references theaddonpackage for the add-on functionality. Broadly looks correct, as it wires the new service into the application.
90-98: Consistent error-handling and resource cleanup
This block follows the established cleanup pattern for failures in the wire initialization. The logic is consistent and ensures resources are cleaned properly on error. No issues found.
150-151: New feature connector and add-on service instantiation
The instantiation sequence forfeatureConnectorandaddonServicelooks coherent. The dependency order (feature connector before add-on creation) is logical.
161-162: App service creation
LoadingappsConfigurationfromconf.Appsand then creating theappServiceis consistent with the standard approach in this file.
172-174: Tracer and entitlements initialization
Establishing the tracer before reading further configuration matches the existing pattern. This helps ensure instrumentation is available early.
187-187: Progress manager configuration
AssigningprogressmanagerConfiguration := conf.ProgressManagercleanly separates configuration concerns. No issues here.
198-198: Creating new streaming connector
The streaming connector is constructed with the relevant services, ensuring the event pipeline is available. No concurrency issues evident.
207-207: Entitlement registry references
Passingconnector, service, eventbusPublisherinto the entitlement registry helps unify the domain logic. No concerns noted.
261-261: Billing service with new service references
Incorporatingservice(MeterService) andconnectorensures billing logic can access meter data flows. This wiring looks correct.
271-271: AppStripe service creation
The newappstripeServicerelies onappServiceand other components. The ordering avoids dependency cycles.
313-313: App sandbox provisioner
Provisioning the sandbox environment for new apps is consistent with the approach used for other provisioning steps in this file.
367-367: MeterEventService references
metereventServicecorrectly depends onconnectorandservicefor event streaming. Alignment with existing patterns is good.
424-425: Including Addon and App in Application return
StoringAddon: addonServiceandApp: appServicein the returnedApplicationstruct ensures these services are available downstream.
475-475: New Addon field in Application struct
ExposingAddon addon.Servicein theApplicationstruct is consistent with other top-level services. No issues with naming or structure.openmeter/productcatalog/addon/ratecard_test.go (3)
19-174: Comprehensive JSON serialization tests
These tests forTestRateCard_JSONeffectively validate both serialization and deserialization flows, ensuring the original and round-tripped RateCards are equivalent. Consider adding edge case tests (e.g. empty/partial JSON) in future expansions.
176-316: Flat fee rate card tests
TheTestFlatFeeRateCardfunction covers both valid and invalid RateCard scenarios, ensuring robust validation checks. The test coverage is thorough.
318-474: Usage-based rate card tests
TestUsageBasedRateCardeffectively covers the usage-based RateCard structure, verifying correct handling of billing cadence and commitments. Test coverage looks solid.openmeter/productcatalog/addon/adapter/adapter_test.go (3)
55-331: TestPostgresAdapter with thorough sub-tests
Your sub-tests for create/get/list/update/delete comprehensively cover the lifecycle of an add-on in PostgreSQL. Validation steps using theAssertAddon*methods are well-structured.
333-359: Version creation helper
createAddonVersionensures new versions are both created and updated with the correctEffectivePeriod. This approach is consistent with multi-version add-on steps.
361-462: Status filter coverage
testListPlanStatusFilterthoroughly tests the filtering based on add-on status (draft, active, archived). The timeline-based logic is verified with multiple test cases. Nicely done.openmeter/productcatalog/addon/httpdriver/addon.go (6)
31-98: ListAddons HTTP handler
The request struct construction, filtering, and response pagination are clearly handled. Error paths are correctly surfaced.
105-144: CreateAddon HTTP handler
Decoding the JSON request and constructing aCreateAddonRequestaligns with the service expectations. The response uses a neat converter method. No conversion issues spotted.
152-192: UpdateAddon HTTP handler
The flow for reading request JSON, mapping toUpdateAddonRequest, and returning updated data is direct and consistent. Looks solid.
200-230: DeleteAddon HTTP handler
The deletion logic is straightforward, with a 204 No Content response. No issues with the fallback error path.
250-286: GetAddon HTTP handler
Resolving ID vs. key inidOrKeyis a nice detail. This handler properly updates the request struct for retrieving a specific version or the latest.
339-374: ArchiveAddon handler
Similar structure to publishing. Setting anEffectiveTotime is inline with the archiving approach. No other issues noted.openmeter/productcatalog/addon/ratecard.go (1)
61-98: Nicely done leveraging generics inNewRateCardFrom.This approach cleanly handles multiple ratecard representations (
FlatFeeRateCard,UsageBasedRateCard, or raw JSON) in a single function. It reduces boilerplate and keeps the code flexible.openmeter/productcatalog/addon/httpdriver/mapping.go (3)
64-64: ** Address the FIXME to consolidate mappers.**
Consolidating HTTP mappers under a common package would reduce duplication and improve maintainability.
982-1055: Validate usage period alignment earlier.
When a metered entitlement template doesn’t define aUsagePeriod, the code infers it frombillingCadence. If both are missing, an error is thrown late in the parsing process. Consider validating such constraints closer to input parsing to fail fast and simplify this function’s logic.
478-507: Double-check currency parsing during creation.
AsCreateAddonRequestsets the currency code and validates it. While this is good practice, ensure that upstream calling code properly handles validation errors and surfaces meaningful messages to users.Would you like me to verify references to
invalid CurrencyCodein your codebase to confirm that errors are handled appropriately?
chrisgacsal
force-pushed
the
feat/addons-api
branch
from
1f7c3c7 to
2e8b13c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openmeter/productcatalog/addon.go (1)
170-172:⚠️ Potential issueFix inverted logic in annotation comparison.
The method returns
falseif annotations are equal, which contradicts the intended equality check. Switch the condition to ensure equality is handled correctly.- if !maps.Equal(m.Annotations, v.Annotations) { + if !maps.Equal(m.Annotations, v.Annotations) { return false }
🧹 Nitpick comments (26)
app/common/productcatalog.go (1)
74-94: Provide minimal doc comments forNewAddonServiceThe function is well-structured and correctly initializes the adapter and service. Consider adding a doc comment to clarify its purpose and parameters.
+// NewAddonService provides a fully initialized add-on service instance +// by creating an adapter and passing required dependencies. func NewAddonService( logger *slog.Logger, ... )cmd/server/wire_gen.go (3)
90-98: Consider consolidating consecutive cleanup callsIf
NewMeterAdapterfails, multiple cleanup functions are invoked one after another. You could wrap these calls in a helper to reduce repetition.if err != nil { - cleanup5() - cleanup4() - cleanup3() - cleanup2() - cleanup() - return Application{}, nil, err + cleanupAll(cleanup5, cleanup4, cleanup3, cleanup2, cleanup) + return Application{}, nil, err }
197-197: Check naming consistency forservice
entitlement := common.NewEntitlementRegistry(... service, ...)might be confusing given multiple services in scope. Renaming the variable tometerServiceor a more specific descriptor can help clarify usage.
367-367: MeterEventService referencingserviceSimilar to the entitlement registry comment, consider renaming
serviceto avoid ambiguity if multiple services are in scope.openmeter/productcatalog/addon/adapter/mapping.go (2)
16-65: ImplementingfromAddonRowBuilding the core
addon.Addonentity from a database row is cleanly structured. Validation after constructing the object helps detect invalid data early. Consider verifying if rate card collections may need concurrency handling (e.g., large sets) in the future.
148-189: ImplementingasAddonRateCardRowThe type switch for different rate card types is clear and expressive. You might add brief doc comments to help maintainers understand usage.
+// asAddonRateCardRow converts a domain RateCard to the corresponding +// entdb.AddonRateCard entity, handling both flat fee and usage-based types. func asAddonRateCardRow(r productcatalog.RateCard) (entdb.AddonRateCard, error) { ... }openmeter/productcatalog/addon.go (3)
37-44: Track the consolidation ofAddonEffectivePeriodwithEffectivePeriod.The FIXME comment indicates there's potential duplication with a broader
EffectivePeriodconcept. Consider creating a shared implementation to maintain DRY principles and ensure consistent behavior across similar time-based status concepts in the codebase.
70-72: Simplify the archive status condition.The current condition for determining archived status is complex and could be simplified for better readability.
- if from.Before(t) && (to.Before(t) && from.Before(to)) { + if from.Before(t) && to.Before(t) && from.Before(to) { return AddonStatusArchived }
191-195: Complete the comment for RateCard validation checks.The comment starts with "Check for" but doesn't complete the sentence properly. Consider restructuring for clarity.
- // Check for - // * duplicated rate card keys - // * namespace mismatch - // * invalid RateCards + // Check for: + // * duplicated rate card keys + // * namespace mismatch + // * invalid RateCardsopenmeter/productcatalog/addon/event.go (1)
66-70: Standardize parameter naming in event constructors.In this function and subsequent ones (lines 109, 156, 199), the parameter is named
Addonwith an uppercase A, while inNewAddonCreateEventit's lowercaseaddon. Standardize parameter naming for consistency.-func NewAddonUpdateEvent(ctx context.Context, Addon *Addon) AddonUpdateEvent { +func NewAddonUpdateEvent(ctx context.Context, addon *Addon) AddonUpdateEvent { return AddonUpdateEvent{ - Addon: Addon, + Addon: addon, UserID: session.GetSessionUserID(ctx), } }openmeter/productcatalog/addon/ratecard_test.go (2)
19-24: Remove unusedExpectedErrorfield in test struct.The
ExpectedErrorfield is defined in the test struct but never used in the actual test cases. The test always expects both serialization and deserialization to succeed (lines 164, 169).tests := []struct { Name string RateCard productcatalog.RateCard - ExpectedError bool }{
163-171: Consider testing error cases for JSON serialization/deserialization.The test only covers the happy path where both serialization and deserialization succeed. Consider adding test cases that verify error handling for malformed JSON or incompatible types.
openmeter/productcatalog/addon/assert.go (1)
22-23: Maintain consistent error message prefixes.The error messages for metadata and annotations comparison don't have the "create input: " prefix that other comparisons use, which makes error output inconsistent.
- assert.Equalf(t, i.Metadata, a.Metadata, "metadata mismatch") - assert.Equalf(t, i.Annotations, a.Annotations, "annotations mismatch") + assert.Equalf(t, i.Metadata, a.Metadata, "create input: metadata mismatch") + assert.Equalf(t, i.Annotations, a.Annotations, "create input: annotations mismatch")openmeter/productcatalog/addon/httpdriver/addon.go (2)
317-319: Fix minor spelling error: "punlish".There's a typo in the error message:
- return PublishAddonResponse{}, fmt.Errorf("failed to punlish add-on [namespace=%s id=%s]: %w", request.Namespace, request.ID, err) + return PublishAddonResponse{}, fmt.Errorf("failed to publish add-on [namespace=%s id=%s]: %w", request.Namespace, request.ID, err)
302-303: Assist with TODO for effective periods.You have TODO comments referencing updates to
api.RequestforEffectivePeriod. Let me know if you'd like assistance or if you want to track these steps in a new issue.Also applies to: 347-348
openmeter/productcatalog/addon/service.go (5)
21-21: Use "add-ons" instead of "Plans" in error message.
This error message references "Plans" even though the method is for listing add-ons.- return pagination.PagedResponse[addon.Addon]{}, fmt.Errorf("invalid list Plans params: %w", err) + return pagination.PagedResponse[addon.Addon]{}, fmt.Errorf("invalid list add-ons params: %w", err)
163-163: Use "add-on" instead of "Plan" in the debug log.
This debug statement references "Plan" when deleting an add-on, which might cause confusion.- logger.Debug("deleting Plan") + logger.Debug("deleting add-on")
241-241: Provide a dedicated not-found error.
Currently, the code has a FIXME comment regarding the not-found scenario. Consider gracefully returning a well-defined "not found" error (e.g., a typed error or 404 equivalent) for clarity and consistency.Would you like help implementing a structured "NotFound" error type and returning it here?
300-300: Fix the typo "udpate aa-on".
There's a small spelling mismatch in the error message.- return nil, fmt.Errorf("failed to udpate aa-on: %w", err) + return nil, fmt.Errorf("failed to update add-on: %w", err)
553-557: Extract single-draft-version check into a shared helper.
Both “CreateAddon” (line 108 onward) and “NextAddon” (line 553 onward) enforce the single-draft-version rule. Refactoring this into a helper function would reduce duplication and improve maintainability.// Example extracted helper (in a shared location): func ensureNoDraftVersionExists(versions []addon.Addon) error { for _, v := range versions { if v.DeletedAt == nil && v.Status() == productcatalog.AddonStatusDraft { return models.NewGenericValidationError( fmt.Errorf("only a single draft version is allowed for add-on"), ) } } return nil }openmeter/productcatalog/addon/service/addon.go (5)
21-21: Use "add-ons" instead of "Plans" in error message.
This error message references "Plans" even though the method is for listing add-ons.- return pagination.PagedResponse[addon.Addon]{}, fmt.Errorf("invalid list Plans params: %w", err) + return pagination.PagedResponse[addon.Addon]{}, fmt.Errorf("invalid list add-ons params: %w", err)
163-163: Use "add-on" instead of "Plan" in the debug log.
This debug statement references "Plan" when deleting an add-on, which might cause confusion.- logger.Debug("deleting Plan") + logger.Debug("deleting add-on")
241-241: Provide a dedicated not-found error.
Currently, the code has a FIXME comment regarding the not-found scenario. Consider gracefully returning a well-defined "not found" error (e.g., a typed error or 404 equivalent) for clarity and consistency.Would you like help implementing a structured "NotFound" error type and returning it here?
300-300: Fix the typo "udpate aa-on".
There's a small spelling mismatch in the error message.- return nil, fmt.Errorf("failed to udpate aa-on: %w", err) + return nil, fmt.Errorf("failed to update add-on: %w", err)
553-557: Extract single-draft-version check into a shared helper.
Both “CreateAddon” and “NextAddon” in the service logic enforce the single-draft-version rule. Refactoring this into a helper function would reduce duplication and improve maintainability.// Example extracted helper (in a shared location): func ensureNoDraftVersionExists(versions []addon.Addon) error { for _, v := range versions { if v.DeletedAt == nil && v.Status() == productcatalog.AddonStatusDraft { return models.NewGenericValidationError( fmt.Errorf("only a single draft version is allowed for add-on"), ) } } return nil }openmeter/productcatalog/addon/ratecard.go (1)
61-98: Consider centralizing marshalling/unmarshalling logic for RateCards.
Multiple code paths inNewRateCardFromhandle JSON deserialization forFlatFeeRateCardandUsageBasedRateCard. You might reduce duplication by introducing a central function or bridging logic that can manage the unmarshal steps for all supported types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
app/common/productcatalog.go(4 hunks)cmd/server/main.go(1 hunks)cmd/server/wire.go(2 hunks)cmd/server/wire_gen.go(14 hunks)openmeter/event/metadata/resourcepath.go(1 hunks)openmeter/productcatalog/addon.go(1 hunks)openmeter/productcatalog/addon/adapter/adapter.go(1 hunks)openmeter/productcatalog/addon/adapter/adapter_test.go(1 hunks)openmeter/productcatalog/addon/adapter/addon.go(1 hunks)openmeter/productcatalog/addon/adapter/mapping.go(1 hunks)openmeter/productcatalog/addon/addon.go(1 hunks)openmeter/productcatalog/addon/assert.go(1 hunks)openmeter/productcatalog/addon/errors.go(1 hunks)openmeter/productcatalog/addon/errors_test.go(1 hunks)openmeter/productcatalog/addon/event.go(1 hunks)openmeter/productcatalog/addon/httpdriver/addon.go(1 hunks)openmeter/productcatalog/addon/httpdriver/driver.go(1 hunks)openmeter/productcatalog/addon/httpdriver/errors.go(1 hunks)openmeter/productcatalog/addon/httpdriver/mapping.go(1 hunks)openmeter/productcatalog/addon/ratecard.go(1 hunks)openmeter/productcatalog/addon/ratecard_test.go(1 hunks)openmeter/productcatalog/addon/repository.go(1 hunks)openmeter/productcatalog/addon/service.go(1 hunks)openmeter/productcatalog/addon/service/addon.go(1 hunks)openmeter/productcatalog/addon/service/service.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- cmd/server/main.go
- cmd/server/wire.go
- openmeter/productcatalog/addon/httpdriver/errors.go
- openmeter/event/metadata/resourcepath.go
- openmeter/productcatalog/addon/errors_test.go
- openmeter/productcatalog/addon/httpdriver/driver.go
- openmeter/productcatalog/addon/repository.go
- openmeter/productcatalog/addon/adapter/adapter.go
- openmeter/productcatalog/addon/errors.go
- openmeter/productcatalog/addon/service/service.go
- openmeter/productcatalog/addon/addon.go
🧰 Additional context used
🧬 Code Definitions (8)
openmeter/productcatalog/addon/httpdriver/addon.go (1)
openmeter/productcatalog/addon/httpdriver/mapping.go (3)
FromAddon(23-62)AsCreateAddonRequest(478-507)AsUpdateAddonRequest(1071-1092)
openmeter/productcatalog/addon/ratecard_test.go (3)
openmeter/productcatalog/addon/ratecard.go (4)
FlatFeeRateCard(102-105)RateCardManagedFields(19-25)UsageBasedRateCard(204-207)NewRateCardFrom(61-98)openmeter/productcatalog/addon/adapter/adapter.go (1)
Config(19-22)openmeter/productcatalog/addon/service/service.go (1)
Config(12-17)
openmeter/productcatalog/addon/adapter/adapter_test.go (6)
openmeter/productcatalog/addon/service.go (4)
CreateAddonInput(98-101)GetAddonInput(198-211)ListAddonsInput(48-79)UpdateAddonInput(122-142)openmeter/productcatalog/addon.go (4)
Addon(177-182)AddonMeta(94-117)AddonEffectivePeriod(38-44)AddonStatus(22-22)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)openmeter/productcatalog/addon/ratecard.go (2)
UsageBasedRateCard(204-207)FlatFeeRateCard(102-105)openmeter/productcatalog/addon/service/service_test.go (1)
MonthPeriod(303-303)openmeter/productcatalog/addon/assert.go (3)
AssertAddonCreateInputEqual(14-26)AssertAddonEqual(54-65)AssertAddonUpdateInputEqual(28-52)
openmeter/productcatalog/addon/assert.go (4)
openmeter/productcatalog/addon/service.go (2)
CreateAddonInput(98-101)UpdateAddonInput(122-142)openmeter/productcatalog/addon.go (1)
Addon(177-182)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)openmeter/productcatalog/ratecard.go (4)
RateCards(350-350)RateCard(28-38)FlatFeeRateCard(161-168)UsageBasedRateCard(256-262)
openmeter/productcatalog/addon/service/addon.go (2)
openmeter/productcatalog/addon/service.go (7)
ListAddonsInput(48-79)CreateAddonInput(98-101)OrderBy(33-33)UpdateAddonInput(122-142)PublishAddonInput(245-250)ArchiveAddonInput(288-294)NextAddonInput(316-326)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)
openmeter/productcatalog/addon.go (2)
openmeter/productcatalog/ratecard.go (2)
RateCards(350-350)RateCard(28-38)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)
openmeter/productcatalog/addon/event.go (3)
openmeter/productcatalog/addon.go (1)
Addon(177-182)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)openmeter/event/metadata/resourcepath.go (2)
ComposeResourcePath(28-30)EntityAddon(20-20)
openmeter/productcatalog/addon/httpdriver/mapping.go (2)
openmeter/productcatalog/addon/httpdriver/driver.go (1)
New(45-55)openmeter/productcatalog/addon/httpdriver/addon.go (2)
CreateAddonRequest(100-100)UpdateAddonRequest(147-147)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: E2E
- GitHub Check: CI
- GitHub Check: Quickstart
- GitHub Check: Developer environment
- GitHub Check: Lint
- GitHub Check: Commit hooks
- GitHub Check: Test
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (26)
app/common/productcatalog.go (3)
13-15: Importing add-on dependencies looks good.These imports are neatly organized, and the naming aligns well with the existing patterns for feature and plan modules.
26-26: Wiring Addon into ProductCatalogAdding
Addonto theProductCatalogwire set is a straightforward change, ensuring the new functionality is properly injected.
37-39: New wire set for AddonDefining
Addonas awire.NewSetfollows the established structure for feature and plan. Consistent naming makes dependency injection more transparent.cmd/server/wire_gen.go (10)
26-26: Import for add-on moduleAdding the import for the add-on package clearly indicates usage within server initialization.
150-151: Addon service initializationCreating the
featureConnectorand thenaddonServiceis in the correct sequence. This ensures the feature connector is available before the add-on is instantiated.
161-162: IntroducingappServiceThese lines correctly wire the new
appServiceinto the initialization flow.
172-174: Tracer, entitlements, & aggregation setupLoading these configurations in sequence is consistent with the rest of the initialization process.
186-188: Progress manager integrationUsing
NewProgressManageraligns with the approach used for other services. Proper error checking is in place.
261-262: Billing service with new referencesThese lines integrate the new
appServiceand look correct for the billing flow.
271-272: NewAppStripeService usagePassing in the newly created
appServiceplus necessary dependencies is aligned with the approach used for other services in the codebase.
313-314: Sandbox provisioningThe
appSandboxProvisioneris created consistently with the rest of the initialization steps, referencing the newly added services.
424-425: AugmentingApplicationwith Addon & AppIncluding these fields in the
Applicationstruct ensures that add-on functionality is exposed in the server configuration.
475-475: NewAddon addon.ServicefieldThis field straightforwardly binds the new add-on functionality to the
Applicationstruct.openmeter/productcatalog/addon/adapter/mapping.go (2)
1-15: Package & importsThe package name is consistent with typical Go patterns, and imports are relevant for add-on logic, particularly the
lolibrary for convenience methods.
67-146: ImplementingfromAddonRateCardRowThis function handles multiple rate card types with proper error checks and calls
Validate(). The robust error messages aid investigation when encountering invalid data.openmeter/productcatalog/addon/assert.go (1)
106-119: Validate coverage for fallback return ingetBillingCadence.
getBillingCadencereturns an emptyisodate.Period{}in the default case, which might mask unexpected or unhandled rate card types. Ensure there's a test scenario verifying that the default path is never triggered, or handle it more explicitly with a log/error if it's genuinely unexpected.Consider handling the default case by logging a warning or adding a panic in development mode:
func getBillingCadence(r productcatalog.RateCard) isodate.Period { switch vv := r.(type) { case *productcatalog.FlatFeeRateCard: return lo.FromPtr(vv.BillingCadence) case *FlatFeeRateCard: return lo.FromPtr(vv.FlatFeeRateCard.BillingCadence) case *productcatalog.UsageBasedRateCard: return vv.BillingCadence case *UsageBasedRateCard: return vv.UsageBasedRateCard.BillingCadence } - return isodate.Period{} + // This should never happen, log and return empty period + log.Printf("WARNING: Unknown rate card type: %T", r) + return isodate.Period{} }openmeter/productcatalog/addon/adapter/addon.go (3)
55-57: Confirm empty filter behavior.When
orFiltersis empty, the code does not apply thequery.Where()call, resulting in all rows being returned if no filter is provided. Double-check that this behavior aligns with your intended logic (returning every add-on for an empty filter set).
158-220: Revisit concurrency and composite uniqueness constraints.Creation logic doesn't appear to enforce a uniqueness constraint for
(namespace, key, version), potentially allowing concurrent operations to insert identical records. This issue was mentioned in a past review.
466-483: Consider partial updates for rate cards rather than a full overwrite.The existing approach deletes all rate cards and recreates them for updates, which was also highlighted in a past review comment. If partial updates are required in the future, a more granular approach may be preferable.
openmeter/productcatalog/addon/adapter/adapter_test.go (2)
55-309: Good test coverage for core adapter logic.These test cases provide comprehensive coverage for all primary CRUD operations, effectively validating both happy paths and edge cases.
284-304: Verify soft-deletion retrieval behavior.After deleting the add-on, the test confirms that
GetAddonstill retrieves it. Ensure this soft-delete approach is intentional, as many users expect a "not found" result for deleted records.openmeter/productcatalog/addon/ratecard.go (1)
183-200:Details
❓ Verification inconclusive
Verify test coverage for Merge operations on rate cards.
TheMergemethods for bothFlatFeeRateCardandUsageBasedRateCardhandle partial updates. It's worth confirming these code paths are tested, especially edge cases like merging different rate card types.Please ensure that all Merge branches are covered by tests. If needed, you can run coverage commands similar to:
Also applies to: 286-303
🏁 Script executed:
#!/bin/bash # Searching for tests of ratecard merge logic rg -A 12 'func Test.*Merge.*RateCard'Length of output: 39
Action: Please manually verify that comprehensive tests exist for all Merge branches of rate cards.
The current grep search for tests covering Merge operations returned no results. As a reminder:
- Confirm that tests are in place for both
FlatFeeRateCardandUsageBasedRateCardMerge scenarios, covering various edge cases including merging different rate card types.- Verify that modifications in lines 183-200 (and similarly in lines 286-303) are exercised by tests.
If tests are not found, please add the necessary test cases before merging this change.
openmeter/productcatalog/addon/httpdriver/mapping.go (4)
64-64: Address the FIXME to consolidate mappers.As previously suggested in the past reviews, unifying the HTTP mappers (e.g., from
FromAddon,FromRateCard, etc.) in a common package or layer would help reduce duplication and simplify maintenance.
66-202: Refactor the lengthy switch statements.These switches for different
RateCardtypes are fairly repetitive (e.g., for FlatFee vs. UsageBased). Consider extracting shared logic or using a more centralized approach (e.g., a registry pattern) to reduce duplication and improve readability.
629-637: Validate discount percentage range.If these discount percentages are limited to a specific range (e.g., 0–100), consider adding validation here to prevent invalid discount values:
+ if d.Percentage < 0 || d.Percentage > 100 { + return rc, fmt.Errorf("invalid discount percentage: must be between 0 and 100") + }
999-1015: Ensure test coverage for nil UsagePeriod or BillingCadence.When
metered.UsagePeriodis absent (and we fall back tobillingCadence), errors arise if both are unset or empty. Please confirm that your test suite covers this edge case and gracefully handles invalid or missing values.
chrisgacsal
force-pushed
the
feat/addons-api
branch
from
2e8b13c to
0d026b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (18)
openmeter/productcatalog/addon/assert.go (1)
67-104: Consider simplifying map creation for better readability.The map creation using immediately-invoked function expressions (IIFEs) is unnecessarily complex for Go. Consider extracting them to separate functions or simplifying the implementation.
- r1Map := func() map[string]productcatalog.RateCard { - m := make(map[string]productcatalog.RateCard, len(r1)) - for _, v := range r1 { - m[v.Key()] = v - } - - return m - }() + r1Map := make(map[string]productcatalog.RateCard, len(r1)) + for _, v := range r1 { + r1Map[v.Key()] = v + } - r2Map := func() map[string]productcatalog.RateCard { - m := make(map[string]productcatalog.RateCard, len(r2)) - for _, v := range r2 { - m[v.Key()] = v - } - - return m - }() + r2Map := make(map[string]productcatalog.RateCard, len(r2)) + for _, v := range r2 { + r2Map[v.Key()] = v + }openmeter/productcatalog/addon/adapter/addon.go (1)
405-508:UpdateAddonmethod functionality.
- Validates the request, fetches the existing add-on, and updates only changed attributes.
- Deletes existing rate cards entirely and recreates them if
RateCardsare provided. This “replace all rate cards” pattern can be valid if partial updates are not required but may risk concurrency overwrites.- Returns an error if the updated add-on is not found, which aligns with the code’s approach in other methods.
If partial rate card updates are desirable, consider an approach that merges changes instead of blanket deletion. Otherwise, keep the “replace all” semantics but ensure concurrency control if multiple updates can happen in parallel.
openmeter/productcatalog/addon/adapter/adapter_test.go (2)
84-88: Consider concurrency or connection pooling tests.The test initializes and uses an adapter directly but doesn't explore potential concurrency issues or connection pool limits. Concurrent creation/update of addons might reveal edge cases.
306-441: Ensure comprehensive coverage of scheduled transitions.The
testListPlanStatusFiltersubtest provides robust coverage of different status transitions. Consider adding edge case tests for effective periods that straddle boundary conditions (e.g., immediate transition, large time gaps, invalid periods).openmeter/productcatalog/addon/service.go (2)
50-51: Clarify pagination requirement.A
TODOindicates that pagination might be optional. Consider updating the code or documentation to reflect whether pagination is mandatory or truly optional, and ensure callers handle the results accordingly.
291-291: Consider clarifying time-based behavior forArchiveAddonInput.This struct only defines
EffectiveTo, which indicates unpublishing at a specific time. If you plan multiple transitions (draft → active → archived) or partial archives, consider also storingEffectiveFromor clarifying how partial intervals are handled.openmeter/productcatalog/addon.go (1)
55-57: Useclock.Now()for consistency.While
Statuscallstime.Now(), other parts of the codebase rely onclock.Now(). Switching toclock.Now()ensures consistent and testable time references across the application.openmeter/productcatalog/addon/ratecard.go (1)
19-37: EnsureAddonIDcomparison isn't duplicated elsewhere.
RateCardManagedFields.Equal()already comparesAddonID. Double-check ifFlatFeeRateCard.Equal()andUsageBasedRateCard.Equal()re-compare the same field, which could be redundant.openmeter/productcatalog/addon/ratecard_test.go (1)
176-316: Validation test coverage is good.Both valid and invalid scenarios are nicely tested for
FlatFeeRateCard. Consider additional edge cases (e.g., extremely largeAmount), if relevant to domain constraints.openmeter/productcatalog/addon/httpdriver/mapping.go (1)
66-202: Switch statement is lengthy but clear.
FromRateCardleverages a type-based switch. Though repetitive, it’s still readable. A registry-based approach could reduce duplication. This aligns with the existing refactor note.openmeter/productcatalog/addon/event.go (3)
31-56: Consider defensive checks inEventMetadata.
AddonCreateEvent.EventMetadata()may be called beforeValidate(), which could lead to a runtime error ife.Addonisnil. ThoughValidate()ensuresAddonis non-nil, defensive checks or clear documentation to callValidate()first could prevent unexpected usage.
131-140: Zero-time fallback in delete event metadata.
Usinglo.FromPtr(e.Addon.DeletedAt)prevents nil pointer dereferences, which is good. However, ifDeletedAtis unexpectedly nil, this defaults to Unix epoch (zero time). Consider logging or clearly documenting this fallback behavior or returning an error to avoid silently assigning an unintended timestamp.
221-230: Validatee.Addonbefore referencing its fields.
Similar to other events,AddonArchiveEvent.EventMetadata()referencese.Addon.Namespaceunconditionally. AlthoughValidate()checks fornil, ensuring thatEventMetadata()does not get called whene.Addonisnilrelies on convention. A short guard clause can protect against accidental misuse.openmeter/productcatalog/addon/httpdriver/addon.go (2)
296-315: Revisit the TODO forPublishAddon.
There's aTODO(chrisgacsal)suggesting a design improvement for settingEffectivePeriod. If left unresolved, the published add-ons might not reflect the intended effective date range. Consider whether we should open a separate tracking issue or implement the fix now.
319-319: Typographical error in the error message.
There is a small spelling mistake: "failed to punlish add-on" instead of "publish". This could potentially confuse error logs.- return PublishAddonResponse{}, fmt.Errorf("failed to punlish add-on [namespace=%s id=%s]: %w", request.Namespace, request.ID, err) + return PublishAddonResponse{}, fmt.Errorf("failed to publish add-on [namespace=%s id=%s]: %w", request.Namespace, request.ID, err)openmeter/productcatalog/addon/service/addon.go (3)
30-78: Handle concurrency for multi-rate-card feature expansion.
expandFeaturesmerges and expands feature data for multiple rate cards. If multiple goroutines process expansions concurrently for the same features, consider concurrency control or clarifying that it is safe under concurrent use scenarios.
317-419: Potential concurrency for publish and archive operations.
WithinPublishAddon, you check for a previously active add-on and then archive it. If two concurrent publish requests happen for the same key, data consistency issues could arise. It may be beneficial to add concurrency controls or unique constraints at the DB level.
482-600: Improve clarity in next version creation logic.
WhileNextAddonsuccessfully enforces no multiple drafts, the approach withsourceAddonFilterFuncand multiple iteration checks can be slightly complex. Extracting filter logic into a helper method or clarifying the branching paths could enhance readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
app/common/productcatalog.go(4 hunks)cmd/server/main.go(1 hunks)cmd/server/wire.go(2 hunks)cmd/server/wire_gen.go(14 hunks)openmeter/event/metadata/resourcepath.go(1 hunks)openmeter/productcatalog/addon.go(1 hunks)openmeter/productcatalog/addon/adapter/adapter.go(1 hunks)openmeter/productcatalog/addon/adapter/adapter_test.go(1 hunks)openmeter/productcatalog/addon/adapter/addon.go(1 hunks)openmeter/productcatalog/addon/adapter/mapping.go(1 hunks)openmeter/productcatalog/addon/addon.go(1 hunks)openmeter/productcatalog/addon/assert.go(1 hunks)openmeter/productcatalog/addon/errors.go(1 hunks)openmeter/productcatalog/addon/errors_test.go(1 hunks)openmeter/productcatalog/addon/event.go(1 hunks)openmeter/productcatalog/addon/httpdriver/addon.go(1 hunks)openmeter/productcatalog/addon/httpdriver/driver.go(1 hunks)openmeter/productcatalog/addon/httpdriver/errors.go(1 hunks)openmeter/productcatalog/addon/httpdriver/mapping.go(1 hunks)openmeter/productcatalog/addon/ratecard.go(1 hunks)openmeter/productcatalog/addon/ratecard_test.go(1 hunks)openmeter/productcatalog/addon/repository.go(1 hunks)openmeter/productcatalog/addon/service.go(1 hunks)openmeter/productcatalog/addon/service/addon.go(1 hunks)openmeter/productcatalog/addon/service/service.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- openmeter/productcatalog/addon/httpdriver/errors.go
- openmeter/event/metadata/resourcepath.go
- cmd/server/main.go
- cmd/server/wire.go
- openmeter/productcatalog/addon/service/service.go
- openmeter/productcatalog/addon/adapter/adapter.go
- openmeter/productcatalog/addon/httpdriver/driver.go
- openmeter/productcatalog/addon/addon.go
- openmeter/productcatalog/addon/errors.go
- openmeter/productcatalog/addon/adapter/mapping.go
- openmeter/productcatalog/addon/errors_test.go
- openmeter/productcatalog/addon/repository.go
🧰 Additional context used
🧬 Code Definitions (6)
openmeter/productcatalog/addon/adapter/adapter_test.go (4)
openmeter/productcatalog/addon/service.go (3)
CreateAddonInput(98-101)GetAddonInput(198-211)UpdateAddonInput(122-142)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)openmeter/productcatalog/addon/service/service_test.go (1)
MonthPeriod(303-303)openmeter/productcatalog/addon/assert.go (3)
AssertAddonCreateInputEqual(14-26)AssertAddonEqual(54-65)AssertAddonUpdateInputEqual(28-52)
cmd/server/wire_gen.go (5)
openmeter/productcatalog/feature/connector.go (1)
NewFeatureConnector(84-100)app/common/progressmanager.go (2)
ProgressManager(14-16)NewProgressManager(19-39)app/common/streaming.go (1)
NewStreamingConnector(21-43)app/common/entitlement.go (1)
NewEntitlementRegistry(22-40)openmeter/app/service/service.go (1)
Service(12-15)
openmeter/productcatalog/addon/assert.go (4)
openmeter/productcatalog/addon/service.go (2)
CreateAddonInput(98-101)UpdateAddonInput(122-142)openmeter/productcatalog/addon.go (1)
Addon(177-182)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)openmeter/productcatalog/ratecard.go (4)
RateCards(350-350)RateCard(28-38)FlatFeeRateCard(161-168)UsageBasedRateCard(256-262)
openmeter/productcatalog/addon/ratecard.go (3)
openmeter/productcatalog/addon/adapter/adapter.go (1)
New(42-51)openmeter/productcatalog/addon/service/service.go (1)
New(19-42)openmeter/productcatalog/ratecard.go (6)
FlatFeeRateCard(161-168)UsageBasedRateCard(256-262)RateCard(28-38)RateCardSerde(40-42)FlatFeeRateCardType(15-15)UsageBasedRateCardType(16-16)
openmeter/productcatalog/addon/service.go (3)
openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)openmeter/productcatalog/ratecard.go (1)
RateCards(350-350)openmeter/productcatalog/addon/service/service.go (1)
New(19-42)
openmeter/productcatalog/addon.go (2)
openmeter/productcatalog/ratecard.go (2)
RateCards(350-350)RateCard(28-38)openmeter/productcatalog/addon/addon.go (1)
Addon(13-21)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: CI
- GitHub Check: Developer environment
- GitHub Check: Commit hooks
- GitHub Check: Test
- GitHub Check: Build
- GitHub Check: Analyze (go)
- GitHub Check: Lint
🔇 Additional comments (55)
openmeter/productcatalog/addon/assert.go (6)
1-12: LGTM: Clear and concise imports.The imports for this test assertion utility are well-organized and include all necessary packages for test assertions, utilities, and internal dependencies.
14-26: Good implementation of the helper function for testing create inputs.The function properly compares all fields between
CreateAddonInputandAddoninstances, with clear error messages. The use oft.Helper()is correct for test utilities.
28-52: Appropriate handling of optional fields in update assertions.The function correctly handles nil pointers in the
UpdateAddonInputby only making assertions when fields are present, and properly useslo.FromPtrOrto handle default values.
54-65: LGTM: Addon comparison is thorough and well-structured.This function properly compares all relevant fields between two
Addoninstances and delegates rate card comparison to a specialized function.
106-119: Validate coverage for fallback return ingetBillingCadence.
getBillingCadencereturns an emptyisodate.Period{}in the default case, which might mask unexpected or unhandled rate card types. Ensure there's a test scenario verifying that the default path is never triggered, or handle it more explicitly with a log/error if it's genuinely unexpected.
121-151: LGTM: Comprehensive rate card equality implementation.The function properly checks all relevant fields from both rate cards, including metadata, feature information, and billing cadence. It handles optionality well using
lo.FromPtr.app/common/productcatalog.go (4)
13-15: Imports appear correct and consistent with module structure.
They properly reference the newly introduced add-on packages required for adapting and handling add-on logic, which aligns with the newly created wire set and service below.
26-26: Addition ofAddonto theProductCatalogwire set.
LinkingAddoninto theProductCatalogcomposition ensures the add-on service is initialized alongside other product catalog functionality.
37-39: Creation of theAddonwire set.
Defining a dedicated wire set for add-ons isolates the new service from the existing plan and feature sets, mirroring the structure used forPlanandFeature. This encourages modular design.
73-94: NewNewAddonServicefunction.
The function correctly initializes the adapter and service, returning an error if adapter creation fails. This design follows the same pattern used forNewPlanService, promoting consistency in initialization logic.cmd/server/wire_gen.go (14)
26-26: Added import for the newaddonpackage.
This import is necessary for declaring theAddonfield in theApplicationstruct and wiring the add-on service.
90-99: Meter adapter and service creation logic.
The error handling sequence with multiple cleanups is consistent with the rest of the file. Initializing the meter adapter first, then passing it intoNewMeterService, logically separates adapter creation from higher-level service usage.
150-151: Instantiation offeatureConnectorandaddonService.
These calls are properly sequenced to ensurefeatureConnectoris constructed before being passed toNewAddonService. The error handling block is thorough, matching the existing cleanup pattern.
161-171:appsConfigurationextraction andappServiceinitialization.
The approach is consistent with other top-level services: retrieve configuration, pass it into the service builder, and handle errors with staged cleanups.
172-185: Telemetry and ClickHouse aggregator initialization.
The code thoroughly handles potential failures by cascading cleanups. This ensures no partial resource leaks if any step fails.
186-196: Progress manager initialization.
Properly checks theProgressManagerConfiguration, then gracefully falls back or errors out as needed. The code’s structure is consistent with other initializations.
197-206: Creates the streaming connector withNewStreamingConnector.
Again, the staged approach ensures no leftover resources upon error. This is aligned with best practices for dependency injection in this codebase.
207-217: Entitlement registry creation.
Attaches the existingmeterServiceandeventbusPublisher. This helps unify the newly integrated modules (such as add-ons) with the rest of the system’s entitlement logic.
261-270: Billing service creation references newappServiceand add-on dependencies.
The interplay betweenbillingService,featureConnector, and the newserviceensures add-on data can flow through existing billing flows.
271-280:appstripeServiceinitialization.
Accepts the newappServicedependency for integration with Stripe. The approach mirrors the existing pattern for third-party service modules.
313-321:appSandboxProvisionercreation.
Includes bothappServiceandbillingService, ensuring that sandbox environments can now also be provisioned with add-on capabilities if needed.
367-367:metereventServicereferences the newservice.
Ensures meter events can be integrated with the final, fully configured meter service instance—this preserves a consistent data flow for event ingestion.
421-425: Populating theApplicationstruct with the new fields.
The new fieldsAddon: addonServiceandApp: appServiceare consistent with the rest of the struct’s pattern. Including them inApplicationunifies usage under a single interface.
475-475: Declaration ofAddon addon.ServicewithinApplication.
The newly introduced service is now fully integrated into the top-level application scope, ensuring it can be injected or consumed wherever necessary throughout the system.openmeter/productcatalog/addon/adapter/addon.go (6)
1-22: New file for add-on adapter package initialization.
This file introduces the core logic and imports needed to support add-ons, providing an independent adapter layer. The package structure and imports appear consistent with existing product catalog patterns.
158-220:CreateAddonmethod implementation.
- Validates input parameters and sets a default version if none is specified.
- The logic inserts the main add-on entity, then bulk-creates associated rate cards.
- Refetches the newly created add-on with eager-loaded rate cards, ensuring a fully populated result.
- As noted in past reviews, consider enforcing uniqueness constraints for
(namespace, key, version)if required to prevent concurrency issues and duplicates.Would you like to check schema files to confirm that
(namespace, key, version)is indeed enforced as unique? I can provide a script to search the schema for composite uniqueness if so.
222-257:rateCardBulkCreateutility function.
- Transforms user-provided rate card definitions into the ent creation statements in bulk, which is efficient.
- Consider partial updates if you wish to preserve existing rate cards, as the current logic in
UpdateAddonfully deletes & re-inserts them.
259-304:DeleteAddonmethod implementation.
- Marks the add-on as deleted by setting
DeletedAt, rather than fully removing the record. This is a safer “soft delete” approach.- The function reuses
GetAddonto ensure the entity exists before deletion.
306-403:GetAddonmethod implementation.
- Handles lookup either by explicit ID, or by
(key, version)(or “latest/active” if version is zero).- Utilizes custom SQL selectors for retrieving the latest version or an active add-on. This ensures flexible queries.
- Eager-loads rate cards for a complete view of the add-on.
510-515:AddonEagerLoadRateCardsFneager load configuration.
- Filters out all rate cards that have a
DeletedAtset in the past, effectively skipping rate cards considered “deleted.”- This ensures the returned add-on only includes currently valid or future-dated rate cards.
openmeter/productcatalog/addon/adapter/adapter_test.go (1)
285-304: Verify soft-delete behavior.After calling
DeleteAddon, the test still retrieves the same addon without error. This suggests a soft-delete or "archived" logic rather than full removal. If the intent is to remove the addon completely, ensure the repository methods match that expectation.openmeter/productcatalog/addon/ratecard.go (5)
39-55: Validation logic looks solid.The structured approach to accumulating errors is good. Consider clarifying whether partial validation is acceptable if multiple fields are missing to improve error traceability.
57-59: Interface is clear.Having a dedicated
ManagedRateCardinterface clarifies design intent and fosters extensibility.
61-98: Watch for pointer vs. value usage in generics.
NewRateCardFrom[T …](v T)uses a type switch with non-pointer cases (FlatFeeRateCard,UsageBasedRateCard). This is fine but ensure external calls don’t pass pointers inadvertently. A runtime mismatch could lead to subtle bugs.Would you like to check the entire codebase to confirm we only pass values?
100-200: Consider removing the repeatedAddonIDcheck.
FlatFeeRateCard.Equal()repeatsAddonIDcomparison, althoughRateCardManagedFields.Equal()does the same. Removing the duplicate comparison would simplify maintenance.
[ suggest_nitpick ]- if r.AddonID != vv.AddonID { - return false - }
202-303: Usage-based rate card mirroring is consistent.The mirroring of logic from
FlatFeeRateCardtoUsageBasedRateCardis consistent. Watch out for duplication inEqual()andMerge()methods that partially replicate checks already done by embedded fields.openmeter/productcatalog/addon/ratecard_test.go (2)
19-174: Comprehensive JSON serialization tests.The test covers both flat-fee and usage-based JSON (de)serialization thoroughly. This ensures resilience against future encoding changes.
318-473: Usage-based tests parallel flat-fee coverage.The usage-based validation testing follows a similar pattern. This consistency makes test maintenance simpler. No major issues.
openmeter/productcatalog/addon/httpdriver/mapping.go (14)
23-62: Solid mapping from domain model to API.
FromAddonproperly handles fields and funneling rate-card conversions. The status check and error handling are sound.
64-64: Refactor is already noted.The “FIXME” echoes the past suggestion to consolidate common mappers.
204-321: Usage-based price decomposition is well-structured.
FromRateCardUsageBasedPricesystematically branches each price type. Potential expansions (like new price types) will be easy to integrate.
323-347: Tax config and payment term mappings are straightforward.Both functions are concise and robust. The fallback to
InAdvanceis sensible when uncertain.
349-476: Entitlement & discount mapping clarifications.Functions
FromEntitlementTemplate,FromDiscounts, andFromDiscountPercentageshandle multiple subtypes. Error messages are clear, ensuring easier debugging.
478-526: Create request object is well-validated.
AsCreateAddonRequestensures currency is valid and rate cards are parsed. This helps prevent invalid data from entering the system.
528-562: Controlled type switching inAsRateCard.The approach neatly funnels into specialized transformation functions. This design fosters maintainability.
564-640: Flat-fee mapping is well-tested.
AsFlatFeeRateCardgracefully handles the optional fields (BillingCadence,FeatureKey, etc.) with minimal branching. Looks good.
642-698: Usage-based mapping matches domain usage.Checks for
BillingCadenceand usage-specific fields are consistent withFlatFeeRateCard. Great alignment between the two.
700-743: Discount array transformations.
AsDiscountspicks the correct discount sub-type, ensuring domain rules are upheld. Errors on invalid types are explicit.
745-939: Extensive price variant handling.
AsPricecovers many price types. The code remains comprehensible, though it’s fairly long. The approach is thorough.
941-980: Tier logic is concise.
AsPriceTiersets up tier boundaries and optional flat or unit price. The decimal parsing is done carefully, avoiding silent failures.
982-1069: Entitlement template transformations.
AsEntitlementTemplatenicely merges domain constraints (e.g.,billingCadencefallback). The error messaging is robust.
1071-1092: Update request object patterns mirror creation.
AsUpdateAddonRequestextends or overrides only the fields changed. Good approach for partial updates.openmeter/productcatalog/addon/event.go (1)
178-187: Consistency check for publish event timestamp.
AddonPublishEventsets the metadata time toe.Addon.UpdatedAtwhile other events set it to their respective fields (CreatedAt,DeletedAt). Ife.Addon.UpdatedAtcan be zero or out of date, consider whether a real-time timestamp or a safer fallback (time.Now()) is needed.openmeter/productcatalog/addon/httpdriver/addon.go (1)
31-63: Validate query parameters thoroughly.
WithinListAddons, parameters such asparams.Status,params.KeyVersion, etc., are directly mapped into slice fields. Although they are sanitized here, ensure that the underlying repository or service layer also validates them to prevent invalid or malicious inputs from impacting the system.openmeter/productcatalog/addon/service/addon.go (1)
80-149: ValidateVersionincrements more robustly.
When creating a new add-on, the logic incrementsparams.Versionif an existing version is higher. Ensure there are no race conditions where two requests create new add-ons simultaneously, leading to conflicting version numbers.
| field.String("addon_id"). | ||
| NotEmpty(). | ||
| Comment("The add-on identifier the ratecard is assigned to."), | ||
| field.String("feature_id"). |
There was a problem hiding this comment.
I will be posing this question once again, shouldn't we instead be referencing the feature only by key (&then if we purchase the Addon then resolve it to an ID?)
I know this is not how it works for plan, just stirring the pot here...
| from := lo.FromPtrOr(p.EffectiveFrom, time.Time{}) | ||
| to := lo.FromPtrOr(p.EffectiveTo, time.Time{}) |
There was a problem hiding this comment.
You could just do the nil checks as they're already nillable & then you wouldn't need IsZero's, eg then line 70 wouldn't need (to.Before(t) && from.Before(to)) just to.Before(t)
| } | ||
|
|
||
| result := make([]addon.Addon, 0, len(paged.Items)) | ||
| for _, item := range paged.Items { |
There was a problem hiding this comment.
FYI i've written a util for this on my branch, i'll rewrite these to use that once checked in
| // WHERE namespace = '$1' and key = '$2' | ||
| // GROUP BY (namespace, key) | ||
| // ) | ||
| query = query.Where(func(s *sql.Selector) { |
There was a problem hiding this comment.
As much as I dislike that pattern, should we use the query templating used for clickhouse too? (e.g. mySubQuery... then you can test how it templates, so we don't rely on comments?)
| } | ||
|
|
||
| // If there are add-on versions with the same key do: | ||
| // * check their statuses to ensure that new plan with the same key is created only |
| } | ||
|
|
||
| if aa.Version >= params.Version { | ||
| params.Version = aa.Version + 1 |
There was a problem hiding this comment.
Shouldn't we error here? I think we should
| return nil, fmt.Errorf("failed to get add-on: %w", err) | ||
| } | ||
|
|
||
| allowedStatuses := []productcatalog.AddonStatus{ |
There was a problem hiding this comment.
just a thought, we could use a simple statemachine with transitions for it (similar to billing & subscriptions)
There was a problem hiding this comment.
Agreed. I just wanted to do the refactor to use statemachine for addon and plan services in one go as they are pretty much the smae when it comes to business logic.
| @@ -365,6 +365,22 @@ func (c RateCards) Billables() RateCards { | |||
| return billables | |||
| } | |||
|
|
|||
| func (c RateCards) IsAligned() bool { | |||
There was a problem hiding this comment.
Let's not call this alignment as Alignment means something else (aligning the periods to a shared phase start compared to item activations). This is a prerequisite of alignment.
|
@GAlexIHU Thanks for your review. I'll address your comments in a follow-up PR. |
3 participants
Overview
Add initial/experimental version of the
add-onAPI. Expect follow-up PRs with improvement, fixes, breaking changes.Summary by CodeRabbit