Add monetized subscription support to gateway#1708
Closed
nisan-abeywickrama wants to merge 2 commits intowso2:mainfrom
Closed
Add monetized subscription support to gateway#1708nisan-abeywickrama wants to merge 2 commits intowso2:mainfrom
nisan-abeywickrama wants to merge 2 commits intowso2:mainfrom
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 11 minutes and 39 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis PR adds two optional billing identifier fields ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gateway/gateway-controller/pkg/api/handlers/subscription_handler.go (1)
247-267:⚠️ Potential issue | 🟠 Major
SubscriptionUpdateRequestis missing billing fields from generated code, contradicting the OpenAPI schema.The
management-openapi.yamlspec definesbillingCustomerIdandbillingSubscriptionIdas updatable fields inSubscriptionUpdateRequest(lines 3887–3892), but the generated Go struct (line 1537–1539 ingenerated.go) contains only theStatusfield. This schema–code mismatch means clients can send these fields in update requests, but they are silently dropped during JSON unmarshaling, never reaching the handler at all.To fix this, either:
- Remove
billingCustomerIdandbillingSubscriptionIdfrom theSubscriptionUpdateRequestschema inmanagement-openapi.yamlto reflect that they are not updatable, or- Regenerate the Go types to include these fields and then handle them in
UpdateSubscription(and emit them inSubscriptionUpdatedEventPayloadto maintain consistency withCreateSubscription/subscriptionToResponse).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/api/handlers/subscription_handler.go` around lines 247 - 267, The generated SubscriptionUpdateRequest struct is missing billingCustomerId and billingSubscriptionId (schema/code mismatch); either remove those fields from the OpenAPI schema or regenerate the Go types to include them and then update the UpdateSubscription handler (where req is bound with s.bindRequestBody) to copy req.BillingCustomerId and req.BillingSubscriptionId into the subscription model, and ensure those values are included in SubscriptionUpdatedEventPayload and in subscriptionToResponse so updates are persisted and returned consistently with CreateSubscription.
🧹 Nitpick comments (3)
gateway/gateway-runtime/policy-engine/internal/analytics/dto/subscription.go (1)
21-26: Consideromitemptyon the subscription JSON tags.When
Event.Subscriptionis non-nil but some fields are empty (e.g. anonymous or non-billed traffic), this struct will serialize"billingCustomerId":"","billingSubscriptionId":"", etc. Adding,omitemptyon each tag would keep analytics payloads cleaner and symmetric with the Moesif publisher which already guards on non-empty. Not a correctness issue.Proposed change
- BillingCustomerID string `json:"billingCustomerId"` - BillingSubscriptionID string `json:"billingSubscriptionId"` - Status string `json:"status"` - PlanName string `json:"planName"` + BillingCustomerID string `json:"billingCustomerId,omitempty"` + BillingSubscriptionID string `json:"billingSubscriptionId,omitempty"` + Status string `json:"status,omitempty"` + PlanName string `json:"planName,omitempty"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-runtime/policy-engine/internal/analytics/dto/subscription.go` around lines 21 - 26, The Subscription struct currently always emits empty strings for missing fields; update the JSON tags on BillingCustomerID, BillingSubscriptionID, Status, and PlanName in the Subscription struct to include ",omitempty" so fields with empty values are omitted during JSON serialization (this affects places that marshal Event.Subscription). Ensure the change is applied to the Subscription type definition only.sdk/core/policyengine/subscription_store.go (1)
17-25: Naming inconsistency between store and xDS DTO (nit).
SubscriptionEntryusesBillingCustomerID/BillingSubscriptionID(Go idiom, uppercaseID) while the sourceSubscriptionDatainsdk/core/policyengine/subscription_xds.gousesBillingCustomerId/BillingSubscriptionId. Both compile, but aligning the two (preferably on the Go-idiomaticIDform, unless you want to match the existingAPIId/ApplicationIdstyle inSubscriptionData) avoids subtle developer confusion when searching the codebase.Also, the billing
*stringpointers are shared (not cloned) between the incomingSubscriptionDataand the storedSubscriptionEntry— fine given callers treat them as immutable, but worth keeping in mind if you ever start mutating fetched entries.Also applies to: 70-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/core/policyengine/subscription_store.go` around lines 17 - 25, Rename the billing fields in SubscriptionEntry to use the Go-idiomatic ID capitalization to match subscription_xds DTO (change BillingCustomerID and BillingSubscriptionID to the ID form if they are not already), update any mapping code that converts from SubscriptionData (which has BillingCustomerId/BillingSubscriptionId) to SubscriptionEntry to use the new names, and ensure the mapping either explicitly copies the string values (allocating new strings) if you want to avoid sharing the incoming *string pointers or documents that pointer sharing is intentional; refer to the SubscriptionEntry struct and the SubscriptionData DTO when making these changes.gateway/gateway-controller/pkg/controlplane/events.go (1)
333-340: Nit: struct field alignment is inconsistent.The added fields use an extra tab between name and type, so the column doesn't line up with neighboring payload structs.
gofmtwill rewrite this; worth running it to keep the diff clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/pkg/controlplane/events.go` around lines 333 - 340, The struct field alignment for fields APIID, SubscriptionID, ApplicationID, SubscriptionToken, SubscriptionPlanId, Status, BillingCustomerID, and BillingSubscriptionID is inconsistent (extra tab spacing); fix by normalizing spacing so each field name uses a single tab (or spaces) before the type to match neighboring payload structs and then run gofmt (or go fmt) on gateway-controller/pkg/controlplane/events.go to reformat the file and keep the diff clean.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gateway/gateway-controller/pkg/controlplane/events.go`:
- Around line 332-341: The SubscriptionUpdatedEventPayload is missing
BillingCustomerID and BillingSubscriptionID so billing changes can't
propagate—add those two fields (same names and json tags as in
SubscriptionCreatedEventPayload) to the SubscriptionUpdatedEventPayload struct
and wire them through the update path: accept and populate them in the
UpdateSubscription handler (subscription_handler.go), ensure the event published
to client.go includes them, have the DB upsert logic consume and persist them,
and make sure the xDS snapshot/updater reads the persisted billing IDs so
downstream consumers receive updated values.
In
`@gateway/gateway-runtime/policy-engine/internal/analytics/dto/unwrappedEvent.go`:
- Around line 73-76: DefaultResponseEvent gained billing/subscription fields
(BillingCustomerID, BillingSubscriptionID, SubscriptionStatus,
SubscriptionPlanName) but DefaultFaultEvent was not changed; add the same four
fields and JSON tags to DefaultFaultEvent so fault events (throttled,
ext_authz_denied, etc.) carry the same billing identifiers for downstream
analytics and billing. Locate the struct named DefaultFaultEvent and mirror the
field names and tags from DefaultResponseEvent, keeping types and JSON keys
identical.
In `@gateway/system-policies/analytics/analytics.go`:
- Around line 29-33: analytics.go defines metadata constants
(BillingCustomerIDMetadataKey, BillingSubscriptionIDMetadataKey,
SubscriptionStatusMetadataKey, SubscriptionPlanNameMetadataKey) that are read
from SharedContext.Metadata but there is no producer in this repo that writes
those keys; locate the subscription-validation or billing producer (or implement
one) and ensure it writes matching keys into SharedContext.Metadata using the
same constant names, or if the producer lives in another module, add a clear
integration note and unit/integration tests to verify the keys are populated
before analytics.go reads them; update the producer to set
SharedContext.Metadata["x-wso2-billing-customer-id"]/... (using the constants)
and add a test that analytics.go sees non-empty values.
---
Outside diff comments:
In `@gateway/gateway-controller/pkg/api/handlers/subscription_handler.go`:
- Around line 247-267: The generated SubscriptionUpdateRequest struct is missing
billingCustomerId and billingSubscriptionId (schema/code mismatch); either
remove those fields from the OpenAPI schema or regenerate the Go types to
include them and then update the UpdateSubscription handler (where req is bound
with s.bindRequestBody) to copy req.BillingCustomerId and
req.BillingSubscriptionId into the subscription model, and ensure those values
are included in SubscriptionUpdatedEventPayload and in subscriptionToResponse so
updates are persisted and returned consistently with CreateSubscription.
---
Nitpick comments:
In `@gateway/gateway-controller/pkg/controlplane/events.go`:
- Around line 333-340: The struct field alignment for fields APIID,
SubscriptionID, ApplicationID, SubscriptionToken, SubscriptionPlanId, Status,
BillingCustomerID, and BillingSubscriptionID is inconsistent (extra tab
spacing); fix by normalizing spacing so each field name uses a single tab (or
spaces) before the type to match neighboring payload structs and then run gofmt
(or go fmt) on gateway-controller/pkg/controlplane/events.go to reformat the
file and keep the diff clean.
In
`@gateway/gateway-runtime/policy-engine/internal/analytics/dto/subscription.go`:
- Around line 21-26: The Subscription struct currently always emits empty
strings for missing fields; update the JSON tags on BillingCustomerID,
BillingSubscriptionID, Status, and PlanName in the Subscription struct to
include ",omitempty" so fields with empty values are omitted during JSON
serialization (this affects places that marshal Event.Subscription). Ensure the
change is applied to the Subscription type definition only.
In `@sdk/core/policyengine/subscription_store.go`:
- Around line 17-25: Rename the billing fields in SubscriptionEntry to use the
Go-idiomatic ID capitalization to match subscription_xds DTO (change
BillingCustomerID and BillingSubscriptionID to the ID form if they are not
already), update any mapping code that converts from SubscriptionData (which has
BillingCustomerId/BillingSubscriptionId) to SubscriptionEntry to use the new
names, and ensure the mapping either explicitly copies the string values
(allocating new strings) if you want to avoid sharing the incoming *string
pointers or documents that pointer sharing is intentional; refer to the
SubscriptionEntry struct and the SubscriptionData DTO when making these changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c66c6193-acae-48c8-ba73-fa7e2a430831
📒 Files selected for processing (20)
gateway/gateway-controller/api/management-openapi.yamlgateway/gateway-controller/pkg/api/handlers/subscription_handler.gogateway/gateway-controller/pkg/api/management/generated.gogateway/gateway-controller/pkg/controlplane/client.gogateway/gateway-controller/pkg/controlplane/events.gogateway/gateway-controller/pkg/models/subscription.gogateway/gateway-controller/pkg/storage/gateway-controller-db.postgres.sqlgateway/gateway-controller/pkg/storage/gateway-controller-db.sqlgateway/gateway-controller/pkg/storage/sql_store.gogateway/gateway-controller/pkg/storage/sqlite.gogateway/gateway-controller/pkg/subscriptionxds/subscription_snapshot.gogateway/gateway-runtime/policy-engine/internal/analytics/analytics.gogateway/gateway-runtime/policy-engine/internal/analytics/constants.gogateway/gateway-runtime/policy-engine/internal/analytics/dto/event.gogateway/gateway-runtime/policy-engine/internal/analytics/dto/subscription.gogateway/gateway-runtime/policy-engine/internal/analytics/dto/unwrappedEvent.gogateway/gateway-runtime/policy-engine/internal/analytics/publishers/moesif.gogateway/system-policies/analytics/analytics.gosdk/core/policyengine/subscription_store.gosdk/core/policyengine/subscription_xds.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
This pull request introduces support for tracking billing information in API subscriptions throughout the gateway controller and runtime. The main changes add
billingCustomerIdandbillingSubscriptionIdfields to the subscription data model, API requests/responses, database schema, event payloads, and analytics. This enables enhanced analytics and integration with billing systems.Approach
Subscription Model and Database Schema Updates:
billingCustomerIdandbillingSubscriptionIdfields to theSubscriptionmodel, API request/response structs, and OpenAPI spec (management-openapi.yaml,subscription.go,generated.go). [1] [2] [3] [4] [5]subscriptionstable, and incremented schema version. [1] [2] [3] [4] [5] [6]API and Event Handling:
Analytics and Runtime Integration: