ref(api): type fetchMutation urls with getApiUrl - #124862
Conversation
`fetchMutation` took its `url` as a plain `string`, so every call site built a path by interpolation and nothing checked that the result was a real endpoint. Narrow the field to the branded `ApiUrl` that `getApiUrl()` returns and convert the remaining call sites, so a typo or a renamed route is a type error rather than a 404 at runtime. Two call sites carried a query parameter inside the path string - `?_=<signature>` on the unsubscribe pages and `?rerun=true` on the preprod build comparison. Those move to `options.query`, since appending to a branded url widens it back to `string`. Delete `getExternalActorEndpointDetails`, which took a collection endpoint and appended the mapping id for `PUT`. That only works on an opaque string, so each `getBaseFormEndpoint` now returns the finished url for its own resource and the form only chooses the method. The admin delete-billing-metric-history call previously requested `/api/0/customers/...`, which double-prefixed the client's own base url; it now uses the same unprefixed path as the rest of the admin views.
…points `getBaseFormEndpoint` was being handed the original mapping row instead of the resolved form data, so it could not see which team or user had just been picked. The team endpoint derives its slug from that selection, so creating a mapping from the modal resolved against an unsubstituted url template, and the callback also lost the id it needs to address an existing mapping. Pass the built mutation data back in, as the endpoint callbacks expect, and give the external-users callback the same id-in-path branch the teams one already has, so editing a user mapping targets its own resource rather than the collection.
📊 Type Coverage Diff
🔍 2 new type safety issues introducedType assertions (
This is informational only and does not block the PR. |
| const {url} = parseQueryKey(props.intentDataQueryKey); | ||
| const {intentData, isLoading, isError, error} = useSetupIntentData({ | ||
| endpoint: url, | ||
| queryKey: props.intentDataQueryKey, | ||
| }); |
There was a problem hiding this comment.
Parsing the queryKey moves into the useIntentData hook, so we can use ApiQueryKey as the prop type instead of the old string.
Co-authored-by: Ryan Albrecht <ryan@ryanalbrecht.ca>
| type ApiMutationVariables = { | ||
| method: 'PUT' | 'POST' | 'PATCH' | 'DELETE'; | ||
| url: string; | ||
| url: ReturnType<typeof getApiUrl>; |
There was a problem hiding this comment.
Here's where the type is enforced going forward!
| const {apiEndpoint, apiMethod} = getExternalActorEndpointDetails( | ||
| getBaseFormEndpoint(fullData as ExternalActorMappingOrSuggestion), | ||
| fullData as ExternalActorMappingOrSuggestion | ||
| ); | ||
| return fetchMutation<ExternalActorMapping>({ | ||
| url: apiEndpoint, | ||
| method: apiMethod, | ||
| url: getBaseFormEndpoint(fullData as ExternalActorMappingOrSuggestion), | ||
| method: isValidMapping ? 'PUT' : 'POST', | ||
| data: fullData, | ||
| }); |
There was a problem hiding this comment.
This is the place with the 'most' change in the PR.
Before we were accepting mapping and getBaseFormEndpoint() and calling them ourselves. Then we'd call getExternalActorEndpointDetails() which could potentially append an id to the end of the url.
All that gets streamlined. Now it's the job of each getBaseFormEndpoint() function to return a correct url, with or without the id appended. Therefore we don't need getExternalActorEndpointDetails anymore either.
there's a only two real files that implement getBaseFormEndpoint. They each accept the mapping parameter and append an id if it's passed in.
fetchMutationtook itsurlas a plainstring, so every call site built apath by interpolation and nothing checked that the result was a real endpoint.
This narrows the field to the branded
ApiUrlthatgetApiUrl()returns andconverts the remaining call sites, which turns a typo or a renamed route into a
type error instead of a 404 at runtime.
The brand is what does the work. It is not assignable from
string, so thecompiler finds every call site that still interpolates — including the two that
carried a query parameter inside the path (
?_=<signature>on the unsubscribepages,
?rerun=trueon the preprod build comparison). Those move tooptions.query, since appending to a branded url widens it back tostringandno longer compiles.
getExternalActorEndpointDetailsis deleted. It took a collection endpoint andappended the mapping id for
PUT, which only works on an opaque string. EachgetBaseFormEndpointnow returns the finished url for its own resource and theform only chooses the method.
Two entries are added to
knownGetsentryApiUrlsfor the adminbilling-platform-migration and delete-billing-metric-history endpoints, which had
no typed path yet. The latter had also been requesting
/api/0/customers/...,which double-prefixes the client's own
/api/0base; it now uses the sameunprefixed path as every other admin view.
No feature flags are involved — this is a type change plus the call sites it
forces.
Review focus
The external actor mapping forms are where behaviour can actually move, because
url construction shifted out of the shared helper and into each caller. The
second commit is the interesting one:
getBaseFormEndpointmust be called withthe resolved mutation data, not the original mapping row, since the team
endpoint derives its slug from whichever team was just selected and the callback
needs the id to address an existing mapping. Worth confirming that reading — the
existing suite covers both the create-via-modal and inline-edit paths.
IntegrationExternalUserMappingshad no id-in-path branch at all, so it gainedthe one the teams side already had. There is no spec file for that view, so that
particular path is covered by reading rather than by a test.