Billing GraphQL#2883
Conversation
c65a777 to
8d1970c
Compare
|
|
||
| /// Fetch invoices older than `cursor` (or the newest invoices when `cursor` | ||
| /// is `None`). Returned rows are ordered newest-first. | ||
| pub async fn fetch_invoice_rows_forward( |
There was a problem hiding this comment.
I really tried to not have fetch_invoice_rows_forward and fetch_invoice_rows_backward since they're almost identical, but the alternative (dynamic query generation) was worse, so I kept it. If anyone has any better ideas I'm all ears
There was a problem hiding this comment.
i struggled with this a while ago when working on invite links and i think this duplication is just the price of admission for the query cache. At least it's easy to reason about...
| type Value = stripe::Invoice; | ||
| type Error = async_graphql::Error; | ||
|
|
||
| async fn load( |
There was a problem hiding this comment.
This could be parallelized.. wanted to KISS where possible tho. Happy to be convinced otherwise
| /// The port to listen on for API requests. | ||
| #[clap(long, default_value = "8080", env = "API_PORT")] | ||
| api_port: u16, | ||
| /// Stripe secret API key. When provided, the billing GraphQL queries and |
There was a problem hiding this comment.
This is Option so that a Stripe API key is not required to run locally.
| /// App is the wired application state of the control-plane API. | ||
| pub struct App { | ||
| pub _id_generator: std::sync::Mutex<models::IdGenerator>, | ||
| pub billing_provider: Option<Arc<dyn crate::billing::BillingProvider>>, |
There was a problem hiding this comment.
I really wanted this to be generic, but it would have exploded the complexity of this PR even further and it wasn't worth it
| // ensures that things still work correctly without it. | ||
| sqlx::query!(r#"delete from role_grants where subject_role = 'estuary_support/';"#) | ||
| .execute(&mut *txn) | ||
| control_plane_api::test_support::provision_test_tenant(&self.pool, tenant, &email, meta) |
There was a problem hiding this comment.
Moved this to control_plane_api so it could be shared, rather than duplicating it.
c00bc9f to
cc73e15
Compare
cc73e15 to
2184f2e
Compare
|
Not sure where to put these thoughts - so putting them on the PR. Should the concept of fetching the The UI will only ever fetch up to 5 tenant's payment methods at once. We do this so support doesn't spam calls. If it is easy (and more importantly makes sense) to allow us to search for an array of To be clear - none of these things are blockers... just thoughts. |
183cb12 to
e3ba37e
Compare
|
|
||
| /// Fetch invoices older than `cursor` (or the newest invoices when `cursor` | ||
| /// is `None`). Returned rows are ordered newest-first. | ||
| pub async fn fetch_invoice_rows_forward( |
There was a problem hiding this comment.
i struggled with this a while ago when working on invite links and i think this duplication is just the price of admission for the query cache. At least it's easy to reason about...
e3ba37e to
a8dea5d
Compare
Introduce a `BillingProvider` abstraction over the Stripe operations we need for customer, payment-method, invoice, and setup-intent handling, together with a Stripe-backed implementation and an in-memory test mock. The trait is deliberately scoped to outbound Stripe API calls so integration tests can stub them. * `BillingProvider` trait: Stripe primitives plus composed default methods (`find_customer`, `require_customer`, `find_or_create_customer`, `fetch_invoice`). * `StripeBillingProvider`: production implementation over `stripe::Client`. * `InMemoryBillingProvider`: stateful test mock used by integration tests and local agent startup. * `billing::db::fetch_invoice_rows`: typed read over the `invoices_ext` view with date-range and invoice-type filters.
dbfe8d5 to
ca57448
Compare
…L surface Add `BillingProvider` (optional) to the `App` struct and wire `--stripe-api-key` from CLI args. When absent, billing operations return "Billing is not configured". Add billing GraphQL mutations (`createBillingSetupIntent`, `setBillingPaymentMethod`, `deleteBillingPaymentMethod`) and `Tenant.billing` query with invoices, payment methods, and customer data. DataLoaders (`StripeInvoiceLoader`, `ChargeDataLoader`, `CustomerDataLoader`) are injected per-request when a billing provider is configured.
The agent refuses to start without either `STRIPE_API_KEY` or `BILLING_IN_MEMORY=true`, so every environment that starts the agent now has to supply one. * `deploy-agent-api.yaml`: inject `STRIPE_API_KEY` from Cloud Run secret manager so the deployed agent starts. * `platform-test.yaml`: conditionally run `graphql_billing_live_stripe` against `STRIPE_TESTMODE_API_KEY` when the secret is present. * `mise/tasks/local/control-plane`: forward a shell-provided `STRIPE_API_KEY` into `agent.env`, or fall back to `BILLING_IN_MEMORY=true` so local dev works without additional setup.
Instead of building queries ad-hoc, let's centralize it a bit
* `CustomerDataLoader` resolves tenant names to Stripe customers. All customer lookups across `payment_methods`, `primary_payment_method`, and invoice resolution share this single DataLoader per request. * `StripeInvoiceLoader` resolves invoices by customer ID, period, and type. Its keys now carry a `CustomerId` rather than a tenant name, so it has no customer-lookup logic of its own. Searches are parallelized via `join_all`. * `ChargeDataLoader` resolves charges by `PaymentIntentId`, only hit when `paymentDetails` is selected. Also moves `receipt_url` into `InvoicePaymentDetails` (both fields come from the charge) and adds a `ChargeStatus` enum so consumers can distinguish payment outcomes. Removes the unused `BillingProvider::fetch_invoice` default method.
ca57448 to
98a8373
Compare
…cy-Key `find_or_create_customer` and `get_or_create_customer_for_tenant` search Stripe by tenant metadata then create if the search misses, but `customers.search` is eventually consistent so two near-simultaneous calls can both miss and both create a duplicate customer row for the same tenant. Use a deterministic `Idempotency-Key` per tenant on `Customer::create` so concurrent or retried creations collapse inside Stripe's 24h window
| for tenant in keys { | ||
| if let Some(customer) = self | ||
| .0 | ||
| .find_customer(tenant) | ||
| .await | ||
| .map_err(|err| async_graphql::Error::new(err.to_string()))? | ||
| { | ||
| out.insert(tenant.clone(), customer); | ||
| } | ||
| } | ||
| Ok(out) |
There was a problem hiding this comment.
same parallelization treatment instead of sequential? maybe for the sake of consistency with the other loaders more than a real performance concern
| inv.status | ||
| .as_ref() | ||
| .and_then(|s| serde_json::to_value(s).ok()) | ||
| .and_then(|v| v.as_str().map(str::to_string)) |
There was a problem hiding this comment.
InvoiceStatus has an as_str() func
| inv.status | |
| .as_ref() | |
| .and_then(|s| serde_json::to_value(s).ok()) | |
| .and_then(|v| v.as_str().map(str::to_string)) | |
| inv.status.as_ref().map(|s| s.as_str().to_string()) |
GregorShear
left a comment
There was a problem hiding this comment.
sorry to leave a couple of raw agent comments - i'm rushing out the door and wanted to get this up before i leave
| pub fn from_str(s: &str) -> Option<Self> { | ||
| match s { | ||
| "final" => Some(InvoiceType::Final), | ||
| "preview" => Some(InvoiceType::Preview), | ||
| "manual" => Some(InvoiceType::Manual), | ||
| _ => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
agent says:
this shadows std::str::FromStr::from_str, so "final".parse::() won't work and the method signature is confusingly close to the trait's. Could either impl FromStr for InvoiceType (which gives you .parse() for free) or rename the inherent method to something like parse_str to avoid the collision.
|
|
||
| /// Fetch invoices older than `cursor` (or the newest invoices when `cursor` | ||
| /// is `None`). Returned rows are ordered newest-first. | ||
| pub async fn fetch_invoice_rows_forward( |
There was a problem hiding this comment.
also, do you mind mentioning in your comment that this is forward pagination? there's another gql api where pagination and ordering of the result set is conflated (and wrong) and i want to make sure we give the agent a chance to get it right if it follows this example
| } | ||
|
|
||
| /// Fetch invoices newer than `cursor`. Returned rows are ordered newest-first. | ||
| pub async fn fetch_invoice_rows_backward( |
There was a problem hiding this comment.
same here - add comment this is backward pagination
| let Some(primary_id) = billing::default_payment_method_id(&customer) else { | ||
| return Ok(None); | ||
| }; | ||
| let pm = self |
There was a problem hiding this comment.
agent says:
If the client selects both paymentMethods and primaryPaymentMethod in the same query (which the UI mocks suggest is the typical shape), this fetches primary_id via get_payment_method even though that PM is already in the list returned by list_payment_methods for the same customer. Worth either:
- Looking the primary up in the cached list, or
- Adding a PaymentMethodLoader keyed on (customer_id, payment_method_id) with a default method that batches via list_payment_methods.
The simpler "find in the list" approach would mirror what set_billing_payment_method already does in the mutations file. Not a hot path so it's fine to defer, but flagging.
|
|
||
| async fn stripe_invoice(&self, ctx: &Context<'_>) -> Result<Option<stripe::Invoice>> { | ||
| let customer_loader = ctx.data::<DataLoader<super::tenant::CustomerDataLoader>>()?; | ||
| let Some(customer) = customer_loader.load_one(self.tenant.clone()).await? else { |
There was a problem hiding this comment.
agent says:
Worth confirming the cache hits when both paymentDetails and TenantBilling.payment_methods are selected in the same query — the latter passes self.tenant from validate_tenant_name (a Prefix-validated string), and this one passes row.billed_prefix from the DB. If the two ever disagree on trailing-slash or case normalization, the per-request DataLoader silently issues two find_customer calls instead of one.
This PR moves the billing endpoints from edge functions into GraphQL.
Tenant.billingBilling is a field on
Tenantrather than a standalone top-level query, because tenant is the natural aggregation root and other fields will follow.This is a design choice I went with as it felt like we were starting to pollute the global namespace with queries that ought to be scoped under their natural owner. It's possible that we should move some other queries as well, but that's out of scope.
WRT permissions:
Tenant.billingis non-nullable which means that a request that selects it withoutAdminon the tenant gets an error, rather than a partial response containingnulland a path-scoped entry inerrors.Invoices
Invoicemerges two data sources. Database fields (dateStart,dateEnd,invoiceType,subtotal,lineItems,extra) come from theinvoices_extPostgres view. Stripe fields (amountDue,status,invoicePdf,hostedInvoiceUrl) are resolved directly from Stripe.Mutations
createBillingSetupIntentfinds or creates a Stripe customer for the tenant, then returns theclientSecretthe browser needs for the Stripe Elements flow.setBillingPaymentMethodupdatesinvoice_settings.default_payment_methodon the customer.deleteBillingPaymentMethoddetaches the method; if the deleted method was the primary, the first remaining method is auto-promoted as default.All three require
Adminon the tenant and validate that the payment method belongs to the tenant's Stripe customer before acting.billing-typesShared Stripe types and search-query builders (
InvoiceType,InvoiceSearch,InvoiceMetadata,customer_search_query,stripe_search) are extracted into abilling-typescrate.billing-integrationsnow imports these instead of defining its own copies.CI
deploy-agent-api.yamlinjectsSTRIPE_API_KEYfrom Cloud Run secret manager.platform-test.yamlconditionally runs thegraphql_billing_live_stripeintegration test whenSTRIPE_TESTMODE_API_KEYis available in the environment.Closes #2879
Part of #2877