feat(frontier)!: move CreatePlan and UpdatePlan to AdminService and add ListAllPlans - #496
Conversation
|
The latest Buf updates on your PR. Results from workflow Validate / validate (pull_request).
|
51559d3 to
ae6d5a6
Compare
📝 WalkthroughWalkthroughThe protobuf contract moves plan creation and update operations from Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
raystack/frontier/v1beta1/admin.proto (2)
441-444: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRestrict
stateto known values.The comment documents that valid values are "active" or "disabled", or empty for all plans. The field has no validation enforcing this. An unrecognized value silently returns an empty result instead of surfacing an error. Add a
buf.validate.fieldconstraint, similar to theintervalfield onPlanRequestBody.♻️ Proposed validation
message ListAllPlansRequest { // filter by plan state, e.g. "active" or "disabled". an empty value returns all plans - string state = 1; + string state = 1 [(buf.validate.field).string = { + in: [ + "", + "active", + "disabled" + ] + }]; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@raystack/frontier/v1beta1/admin.proto` around lines 441 - 444, Update ListAllPlansRequest.state with a buf.validate.field string constraint limiting values to "active", "disabled", or the empty string, following the validation pattern used by PlanRequestBody.interval. Preserve empty state as the request for all plans and reject unrecognized values during validation.
450-471: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winApply the same validation rigor to
state,on_start_credits, andtrial_days.
interval(line 457) restricts input to a known set of values, butstate(line 468) accepts any string with no constraint, so it has the same silent-mismatch risk asListAllPlansRequest.state. Additionally,on_start_creditsandtrial_days(lines 465-466) accept negativeint64values with no lower-bound check, even though negative credits or negative trial days are not meaningful business values.♻️ Proposed validation
int64 on_start_credits = 6; - int64 trial_days = 7; + int64 trial_days = 7 [(buf.validate.field).int64.gte = 0]; - string state = 8; + string state = 8 [(buf.validate.field).string = { + in: [ + "active", + "disabled" + ] + }];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@raystack/frontier/v1beta1/admin.proto` around lines 450 - 471, Update PlanRequestBody fields state, on_start_credits, and trial_days to add validation consistent with the existing request validation: constrain state to the supported plan-state values using the established state validation, and require on_start_credits and trial_days to be non-negative. Keep the existing field types and interval validation unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@raystack/frontier/v1beta1/admin.proto`:
- Around line 441-444: Update ListAllPlansRequest.state with a
buf.validate.field string constraint limiting values to "active", "disabled", or
the empty string, following the validation pattern used by
PlanRequestBody.interval. Preserve empty state as the request for all plans and
reject unrecognized values during validation.
- Around line 450-471: Update PlanRequestBody fields state, on_start_credits,
and trial_days to add validation consistent with the existing request
validation: constrain state to the supported plan-state values using the
established state validation, and require on_start_credits and trial_days to be
non-negative. Keep the existing field types and interval validation unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b2e103a-8583-4771-80ec-ad38b1912a78
📒 Files selected for processing (2)
raystack/frontier/v1beta1/admin.protoraystack/frontier/v1beta1/frontier.proto
💤 Files with no reviewable changes (1)
- raystack/frontier/v1beta1/frontier.proto
|
Applied both validation nits in fc68fa3: |
634beca to
44039cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@raystack/frontier/v1beta1/admin.proto`:
- Around line 459-465: Update PlanRequestBody.state validation to include
IGNORE_IF_ZERO_VALUE so an omitted empty state passes validation and the server
can apply the active default. Keep UpdatePlanRequestBody.state using strict
active/inactive validation without this ignore option.
🪄 Autofix
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 Plus
Run ID: 56870105-6600-49a8-a5e4-b0cac1819e39
📒 Files selected for processing (1)
raystack/frontier/v1beta1/admin.proto
2 participants
What
Move the plan write APIs to
AdminServiceand add a newListAllPlansAPI.CreatePlanandUpdatePlanmove fromFrontierServicetoAdminService.ListAllPlansonAdminService. It returns every plan, including inactive ones. An empty state returns all plans; a set state filters to it.PlanRequestBody,CreatePlanRequest,CreatePlanResponse,UpdatePlanRequest,UpdatePlanResponse) move fromfrontier.protointoadmin.proto, soadmin.protodoes not importfrontier.proto.UpdatePlantakes a newUpdatePlanRequestBody(title, description, on_start_credits, trial_days, state, metadata). It has no name, interval, or products because UpdatePlan does not change those. It is a full write: an omitted field is cleared, not left unchanged.UpdatePlanRequest.bodyis required.ListPlansandGetPlan, and their messages, stay onFrontierServiceinfrontier.proto.ListPlansstill returns active plans only.statemust beactiveorinactiveon the write bodies (empty is rejected, so state is required); theListAllPlansfilter allows an empty state to mean "all";on_start_creditsandtrial_daysmust be zero or more.Why
Plan writes are admin only. Today they sit on
FrontierServiceand are gated to platform superusers by the server. Moving them toAdminServiceputs them on the admin surface where they belong.The billing reconcile flow in Frontier needs to read every plan for export, including inactive ones.
ListPlansreturns active plans only, so it cannot see an inactive plan.ListAllPlansfills that gap.The state values (
active/inactive) follow frontier's billing convention: prices usePriceStateInactive = "inactive"(billing/product/product.go), and no billing code usesdisabled.Breaking change and migration
CreatePlanandUpdatePlanare removed fromFrontierService.FrontierServicetoAdminService. The old paths returnUNIMPLEMENTED.UpdatePlanRequest.bodychanged fromPlanRequestBodyto the newUpdatePlanRequestBodyon a new field number (field 2 is reserved, body is now field 3). This is wire-safe, but the Go type ofUpdatePlanRequest.Bodychanges, which is a compile break for Go callers of UpdatePlan.AdminServicebut keeps its old JSON body will not get an error for the fields that no longer exist on the update body: the JSON codec discards unknown fields, soname,interval, andproductsare silently dropped from anUpdatePlanbody.CreatePlanorUpdatePlancaller that used to omitstate(the server defaulted it toactive) now getsINVALID_ARGUMENT, becausestateis validated toactive/inactive.buf breakingpasses at the WIRE level; the service surface change above is intentional and not covered by that gate.Testing
buf build,buf lint, andbuf breakingagainst main all pass.