feat(billing): add SearchOrgInvoices RQL endpoint#1536
feat(billing): add SearchOrgInvoices RQL endpoint#1536paanSinghCoder wants to merge 10 commits intomainfrom
Conversation
Introduce a new SearchOrgInvoices endpoint with RQL support while keeping ListInvoices behavior unchanged for existing consumers, and wire backend auth/service/repository plus regenerated frontier protobuf bindings. Made-with: Cursor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an organization-scoped invoice search endpoint and implementation across API handlers, service, repository, models, mocks, tests, and authorization; also updates the Makefile's PROTON_COMMIT constant. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 29242d54-19b4-4ba1-872f-e89c7f7065a6
⛔ Files ignored due to path filters (2)
proto/v1beta1/frontier.pb.gois excluded by!**/*.pb.go,!proto/**proto/v1beta1/frontierv1beta1connect/frontier.connect.gois excluded by!proto/**
📒 Files selected for processing (8)
Makefilebilling/invoice/invoice.gobilling/invoice/service.gointernal/api/v1beta1connect/billing_invoice.gointernal/api/v1beta1connect/interfaces.gointernal/api/v1beta1connect/mocks/invoice_service.gointernal/store/postgres/billing_invoice_repository.gopkg/server/connect_interceptors/authorization.go
Coverage Report for CI Build 24447565038Coverage decreased (-0.01%) to 41.717%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
| if customerID == "" { | ||
| return nil, 0, errors.New("customer id is required") | ||
| } | ||
| if rqlQuery == nil { | ||
| return nil, 0, fmt.Errorf("%w: query is required", ErrBadInput) | ||
| } | ||
| if len(rqlQuery.GroupBy) > 0 { | ||
| return nil, 0, fmt.Errorf("%w: group_by is not supported", ErrBadInput) | ||
| } |
There was a problem hiding this comment.
Validations can go in proto. (Caution: check if rqlQuery is validated there)
There was a problem hiding this comment.
customerID and GroupBy are not present on proto level thus makes sense to keep here.
Remove the query check.
| if rqlQuery.Limit <= 0 { | ||
| rqlQuery.Limit = utils.DefaultLimit | ||
| } | ||
| if rqlQuery.Offset < 0 { | ||
| rqlQuery.Offset = utils.DefaultOffset | ||
| } |
There was a problem hiding this comment.
check if this happens automatically in utils
There was a problem hiding this comment.
Replaced the manual check and added utils.AddRQLPaginationInQuery(...)
| return nil, 0, fmt.Errorf("%w: %w", invoice.ErrBadInput, err) | ||
| } | ||
|
|
||
| countQuery, countParams, err := withSort.Select(goqu.COUNT("*")).ToSQL() |
There was a problem hiding this comment.
To get total count, we can use the filter/search applied query
There was a problem hiding this comment.
Done. replaced with withSearch
paanSinghCoder
requested review from
AmanGIT07,
rohilsurana,
rsbh and
whoAbhishekSah
and removed request for
rohilsurana,
rsbh and
whoAbhishekSah
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/store/postgres/billing_invoice_repository.go (1)
314-333:⚠️ Potential issue | 🟠 MajorAdd a deterministic default order before paginating.
Line 330 paginates
withSorteven when the request does not specify any sort, so page boundaries are nondeterministic and clients can see duplicates/missing invoices across pages.Suggested fix
withSort, err := frontierutils.AddRQLSortInQuery(withSearch, rqlQuery) if err != nil { return nil, 0, fmt.Errorf("%w: %w", invoice.ErrBadInput, err) } + + if len(rqlQuery.Sort) == 0 { + withSort = withSort.OrderAppend( + goqu.I(TABLE_BILLING_INVOICES+".created_at").Desc(), + goqu.I(TABLE_BILLING_INVOICES+".id").Desc(), + ) + } countQuery, countParams, err := withSearch.Select(goqu.COUNT("*")).ToSQL()
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 04d90ada-f210-4e7d-a6e5-293e01496ae8
📒 Files selected for processing (3)
billing/invoice/service.gointernal/api/v1beta1connect/billing_invoice.gointernal/store/postgres/billing_invoice_repository.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/api/v1beta1connect/billing_invoice.go
| if filter.CustomerID == "" { | ||
| return nil, errors.New("customer id is required") | ||
| return nil, errors.New("customer id not found") |
There was a problem hiding this comment.
Use ErrBadInput for missing customer IDs instead of "not found".
Line 310 and Line 317 are validating request input, but they currently return a plain not-found error. That makes invalid requests look like missing resources and is inconsistent with the ErrBadInput handling already used for the other SearchOrgInvoices validation branches.
Suggested fix
func (s *Service) List(ctx context.Context, filter Filter) ([]Invoice, error) {
if filter.CustomerID == "" {
- return nil, errors.New("customer id not found")
+ return nil, fmt.Errorf("%w: customer id is required", ErrBadInput)
}
return s.repository.List(ctx, filter)
}
func (s *Service) SearchOrgInvoices(ctx context.Context, customerID string, nonzeroOnly bool, rqlQuery *rql.Query) ([]Invoice, int64, error) {
if customerID == "" {
- return nil, 0, errors.New("customer id not found")
+ return nil, 0, fmt.Errorf("%w: customer id is required", ErrBadInput)
}
if rqlQuery == nil {
return nil, 0, fmt.Errorf("%w: query is required", ErrBadInput)
}Also applies to: 316-320
There was a problem hiding this comment.
If this is the method used in the older RPC then we can skip changing the error message. For our newer methods we can make the change
There was a problem hiding this comment.
@AmanGIT07, understood! Agreed — List is an existing/older RPC and its error message change can be left as-is. For SearchOrgInvoices (the new method in this PR), the customerID == "" check should be consistent with the other validation branches in the same function. Here's the scoped fix:
func (s *Service) SearchOrgInvoices(ctx context.Context, customerID string, nonzeroOnly bool, rqlQuery *rql.Query) ([]Invoice, int64, error) {
if customerID == "" {
- return nil, 0, errors.New("customer id not found")
+ return nil, 0, fmt.Errorf("%w: customer id is required", ErrBadInput)
}
if rqlQuery == nil {
return nil, 0, fmt.Errorf("%w: query is required", ErrBadInput)
}This keeps all three validation errors in SearchOrgInvoices consistently wrapped with ErrBadInput, which allows the connect handler to correctly map them to connect.CodeInvalidArgument. No change needed for List.
✏️ Learnings added
Learnt from: AmanGIT07
Repo: raystack/frontier PR: 1536
File: billing/invoice/service.go:309-310
Timestamp: 2026-04-14T21:14:04.534Z
Learning: In the `billing/invoice` package (billing/invoice/service.go, Go), the `List` method is an older RPC whose error messages should not be changed. Only new methods (e.g., `SearchOrgInvoices`) should use `ErrBadInput`-wrapped errors for input validation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
billing/invoice/service_search_org_invoices_test.go (1)
84-93: PreferErrorIsfor wrapped validation errors.For the nil-query and
group_bycases, asserting only message substrings is brittle. Addassert.ErrorIs(t, err, ErrBadInput)to validate the contract and keep the message checks as secondary.Proposed test hardening
_, _, err = svc.SearchOrgInvoices(context.Background(), "cust-1", true, nil) assert.Error(t, err) + assert.ErrorIs(t, err, ErrBadInput) assert.Contains(t, err.Error(), "query is required") @@ _, _, err = svc.SearchOrgInvoices(context.Background(), "cust-1", true, &rql.Query{ GroupBy: []string{"state"}, }) assert.Error(t, err) + assert.ErrorIs(t, err, ErrBadInput) assert.Contains(t, err.Error(), "group_by is not supported")internal/api/v1beta1connect/billing_invoice_test.go (1)
750-754: Avoid early return before mock expectation checks.Line 753 returns before
AssertExpectations, so error-path cases can skip mock verification.Suggested restructuring
got, err := handler.SearchOrgInvoices(context.Background(), tt.request) if tt.wantCode != 0 { assert.Error(t, err) assert.Equal(t, tt.wantCode, connect.CodeOf(err)) - return - } - - assert.NoError(t, err) - if tt.assertResp != nil { - tt.assertResp(t, got) + assert.Nil(t, got) + } else { + assert.NoError(t, err) + if tt.assertResp != nil { + tt.assertResp(t, got) + } } mockInvoiceService.AssertExpectations(t) mockCustomerService.AssertExpectations(t)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b292a96f-57e4-4776-9c10-9c5151698a4e
📒 Files selected for processing (2)
billing/invoice/service_search_org_invoices_test.gointernal/api/v1beta1connect/billing_invoice_test.go
| return nil, 0, fmt.Errorf("%w: %w", invoice.ErrBadInput, err) | ||
| } | ||
|
|
||
| withSort, err := frontierutils.AddRQLSortInQuery(withSearch, rqlQuery) |
There was a problem hiding this comment.
Lets add default sort order in case its not specified
There was a problem hiding this comment.
Added createAt default sort
| filterColumns := []string{ | ||
| "id", | ||
| "provider_id", | ||
| "customer_id", | ||
| "state", | ||
| "currency", | ||
| "amount", | ||
| "hosted_url", | ||
| "due_at", | ||
| "effective_at", | ||
| "created_at", | ||
| "period_start_at", | ||
| "period_end_at", | ||
| } |
There was a problem hiding this comment.
Let's put these supported columns at top of the file for better visibility. Same for search etc columns
| } | ||
| invoices = append(invoices, inv) | ||
| } | ||
| return invoices, totalCount, nil |
There was a problem hiding this comment.
why we need to send totalCount separately? It should be embedded in the response itself as per proto definition
There was a problem hiding this comment.
Added struct SearchOrgInvoicesPagination and SearchOrgInvoicesResult
|
@paanSinghCoder Please add some sample request and response |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/api/v1beta1connect/billing_invoice.go (1)
104-135:⚠️ Potential issue | 🟠 MajorValidate and normalize RQL before
GetByOrgIDearly-return paths.Right now, invalid RQL can return
200 OKforcustomer.ErrNotFound, and default pagination is skipped in that branch because validation/defaulting happens later.🛠️ Proposed fix
func (h *ConnectHandler) SearchOrgInvoices(ctx context.Context, request *connect.Request[frontierv1beta1.SearchOrgInvoicesRequest]) (*connect.Response[frontierv1beta1.SearchOrgInvoicesResponse], error) { errorLogger := NewErrorLogger() queryMsg := request.Msg.GetQuery() if queryMsg == nil { queryMsg = &frontierv1beta1.RQLRequest{} } + rqlQuery, err := utils.TransformProtoToRQL(queryMsg, invoice.SearchOrgInvoice{}) + if err != nil { + return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("failed to read rql query: %v", err)) + } + if err = rql.ValidateQuery(rqlQuery, invoice.SearchOrgInvoice{}); err != nil { + return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("failed to validate rql query: %v", err)) + } + if rqlQuery.Limit <= 0 { + rqlQuery.Limit = utils.DefaultLimit + } + if rqlQuery.Offset < 0 { + rqlQuery.Offset = utils.DefaultOffset + } + cust, err := h.customerService.GetByOrgID(ctx, request.Msg.GetOrgId()) if err != nil { if errors.Is(err, customer.ErrNotFound) { return connect.NewResponse(&frontierv1beta1.SearchOrgInvoicesResponse{ Invoices: []*frontierv1beta1.Invoice{}, Pagination: &frontierv1beta1.RQLQueryPaginationResponse{ - Offset: queryMsg.GetOffset(), - Limit: queryMsg.GetLimit(), + Offset: uint32(rqlQuery.Offset), + Limit: uint32(rqlQuery.Limit), TotalCount: 0, }, }), nil } @@ - rqlQuery, err := utils.TransformProtoToRQL(queryMsg, invoice.SearchOrgInvoice{}) - if err != nil { - return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("failed to read rql query: %v", err)) - } - if err = rql.ValidateQuery(rqlQuery, invoice.SearchOrgInvoice{}); err != nil { - return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("failed to validate rql query: %v", err)) - }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1302b844-eedb-4f66-b9e1-83a8b3ad8be5
📒 Files selected for processing (9)
billing/invoice/invoice.gobilling/invoice/service.gobilling/invoice/service_search_org_invoices_test.gointernal/api/v1beta1connect/billing_invoice.gointernal/api/v1beta1connect/billing_invoice_test.gointernal/api/v1beta1connect/interfaces.gointernal/api/v1beta1connect/mocks/invoice_service.gointernal/store/postgres/billing_invoice_repository.gopkg/server/connect_interceptors/authorization.go
✅ Files skipped from review due to trivial changes (2)
- billing/invoice/service_search_org_invoices_test.go
- billing/invoice/invoice.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/server/connect_interceptors/authorization.go
- internal/store/postgres/billing_invoice_repository.go
- internal/api/v1beta1connect/billing_invoice_test.go
- internal/api/v1beta1connect/interfaces.go
- billing/invoice/service.go
3 participants
Summary
SearchOrgInvoicesRPC in FrontierService that supportsRQLRequestand pagination responseSearchOrgInvoices RQL Support Matrix
org_idnonzero_amount_onlytrueappliesamount > 0filter.query.limit50when missing/invalid.query.offset0when missing/invalid.query.searchquery.sortquery.filtersquery.group_bygroup_by is not supported).Filterable / Sortable Fields
idprovider_idcustomer_idstatecurrencyamounthosted_urldue_ateffective_atcreated_atperiod_start_atperiod_end_atSearchable Fields
idprovider_idstatecurrencyhosted_urlOperator Support (practical)
stringeq,neq,like,notlike,in,notin,empty,notemptynumbereq,neq,gt,gte,lt,ltedatetimeeq,neq,gt,gte,lt,lteTest plan
make protoand verify no generated-file diffs remainListInvoicesconsumers still behave unchangedSearchOrgInvoiceswith filters/sort/search/limit/offset