Parameterized triggers, and support for application_fee events#1463
Parameterized triggers, and support for application_fee events#1463mnorth-stripe wants to merge 1 commit intomasterfrom
Conversation
7804968 to
eccf8e0
Compare
vzhang-stripe
left a comment
There was a problem hiding this comment.
Minor Observations:
- Parameter parsing: The code assumes --param follows the fixtureName:path=value format. Consider what happens if someone uses --param with an event
that doesn't match the fixture name (e.g., stripe trigger application_fee.created --param invoice:...). The error handling might be confusing. - Help text alignment: The vertical alignment logic in EventList() looks good but could be fragile if event names or parameter descriptions get very
long. Consider max-width handling. - Metadata loading: getRequiredParamsForEvent() loads and parses the fixture file. Consider whether this could fail gracefully if a fixture is
malformed (though this is likely fine for a CLI tool).
vzhang-stripe
left a comment
There was a problem hiding this comment.
1. Required --param values are ignored by RPC callers
In pkg/rpcservice/trigger.go, the RPC path hardcodes []string{} for the new param argument instead of forwarding anything
from the request:
req.Override,
[]string{}, // param - not yet supported in RPC, can be added later
req.Add,
That means application_fee.* triggers can work from the CLI but will always fail validation when invoked through RPC,
because those events now require charge:transfer_data.destination. This creates inconsistent behavior across entry points
and can break any existing RPC-based integrations that call Trigger.
Why this matters: the feature is user-facing, but only partially wired through the product surface. If RPC intentionally
doesn’t support params yet, the validation layer probably shouldn’t apply there, or the RPC schema should be updated in the
same change.
2. --override is applied twice, which can change behavior unexpectedly
BuildFromFixtureFile still calls NewFixtureFromFile(..., override, add, remove, ...), and then later merges override again
together with param:
fixture, err := NewFixtureFromFile(..., override, add, remove, edit)
...
mergedOverrides = append(mergedOverrides, override...)
mergedOverrides = append(mergedOverrides, param...)
if len(mergedOverrides) > 0 {
if err := fixture.Override(mergedOverrides); err != nil {
return nil, err
}
}
So overrides are now processed once inside NewFixtureFromFile and then a second time afterward. For many values that’s
harmless, but it’s still a semantic change and can become problematic for nested merges or future override behaviors. If
the goal is “params take precedence over overrides,” the cleaner fix is to either:
- stop passing override into NewFixtureFromFile and apply the merged list once afterward, or
- keep the existing flow and apply only param afterward.
tomer-stripe
left a comment
There was a problem hiding this comment.
Was going through the pr -- I wonder if it makes more sense to use --add in this case instead of creating a new flag (for --param) but still keeping the pre-flight validation.
Semantically --override assumes you're squashing over an existing value but we added --add to supplement the trigger with your own data. It feels similar to what you're doing with --param minus the pre-flight validation.
| @@ -0,0 +1,161 @@ | |||
| ## Summary | |||
There was a problem hiding this comment.
can you remove this?
2b0d776 to
f54ceec
Compare
Adds a --param flag to `stripe trigger` with pre-flight validation for events that require user-provided configuration. Application fee events are the first to use this, requiring a Connect account ID. The --param flag validates required parameters before making API calls, providing early feedback and clear error messages. It uses the same syntax as --override (fixtureName:path.to.field=value) and is backed by a new required_params metadata block in fixture JSON files. New triggers: application_fee.created, application_fee.refunded, application_fee.refund.updated.
f54ceec to
f6ca7ba
Compare
Reviewers
r? @
cc @stripe/developer-products
Important
There are some critical decisions in this PR description. It's long, but intentionally detailed for a reason
Summary
Adds support for triggering application fee events and introduces a
--paramflag with pre-flight validation for triggers that require user-provided configuration.Background
Some Stripe events require configuration that can't be pre-filled in fixtures. For example, application fee events require a Connect account ID with the
transferscapability enabled. Previously, these fixtures would:This PR addresses this by adding a
--paramflag that validates required parameters before making API calls, providing early feedback and clear error messages.What's Included
New
--paramFlagAdds pre-flight validation for trigger parameters:
Features:
--override:fixtureName:path.to.field=valuerequired_paramsmetadata in fixture files=, empty values)--overridevaluesNew Application Fee Triggers
Adds 3 new Connect-related event triggers:
application_fee.created- When an application fee is created on a chargeapplication_fee.refunded- When an application fee is fully refundedapplication_fee.refund.updated- When an application fee refund is updatedAll three require a Connect account ID via the new
--paramflag.Implementation Details
required_paramsarray to_metablock in fixture JSONexampletoplaceholderfor semantic clarityValidateRequiredParams()function with comprehensive error messages--paramcolumn tight at 82 chars instead of 93.Output Changes
stripe trigger --help: Event listThree new application fee events appear in the event list. Events that require parameters show the
--paramsyntax inline, vertically aligned. Alignment is calculated only from events with params (30 chars), keeping lines at 82 chars instead of 93:stripe trigger --help: Examples sectionThe examples section now includes descriptive comments, a parameterized usage example, and blank line separation between examples:
stripe trigger --help: New--paramflagA new
--paramflag for pre-flight parameter validation appears between--overrideand--raw:Spicy Decision: Params as a Layer on Top of Overrides
This PR deliberately does not introduce a formal concept of "params" in the fixture system itself. Instead,
--paramis a validation layer on top of the existing--overridemechanism — params use the exact same syntax and are merged into overrides before execution.This means fixture files remain unchanged at the execution level. The
_meta.required_paramsblock is only read by the trigger command for validation and help text. The fixture runner doesn't know the difference between a--paramand an--override.The question for reviewers: Is this the right approach, or should we formalize parameterization as a first-class concept in the fixture system?
Arguments for the current approach:
--param/--overridedistinction is a UX concern, not a data model concernArguments for formalizing params in fixtures:
stripe trigger) could benefit from the same validationtriggers.go— if fixtures supported params natively, this would move to the fixture layer where it arguably belongsWhat can be deferred vs. what needs to be decided now?
The refactoring question — whether validation lives in
triggers.goor moves down into the fixture runner — is entirely internal. If we later want fixtures to support parameterization as a general concept, that refactoring can be done without breaking users who have already started using--param. The flag, its syntax, and its behavior all stay the same; only the internal plumbing moves.However, there are decisions in this PR that are hard to change later because they form the user-facing contract:
Is
--paramthe right name for this concept? Once users start scripting against it, renaming is a breaking change.Should params use fixture JSON paths (
charge:transfer_data.destination) or semantic names (connected_account_id)? I considered semantic names earlier — they're shorter and describe the purpose of an input (similar to function argument names). But the downside is you lose the relationship to where that value is placed in the API call, and that relationship is a fairly central part of the fixture mental model. This PR uses fixture paths, which means users can look at the fixture JSON and see exactly where their value ends up — same as--override.Testing
All tests pass. The
--paramflag provides robust validation before API calls, catching configuration errors early with actionable error messages.