cli
cli copied to clipboard
Shopify
Automatically retry partners and app management requests that fail with 401
WHY are these changes introduced?
- Unexpected HTTP 401 responses are one of the leading causes of unhandled exceptions in the CLI.
- For the most part, we handle unauthenticated users gracefully by various
ensureAuthenticatedmethods (i.e.ensureAuthenticatedPartners). - There must be either some race conditions or some other server side conditions that lead to HTTP 401 responses on occasion, and there are enough of these that they add up to one of the most common forms of unhandled errors in the CLI. One possible situation is that a token is valid, is validated in the CLI with
ensureAuthenticated, then becomes invalid before the next request is made.
WHAT is this pull request doing?
- Started this PR as a clone of this PR, but made some tweaks and a few extensions
- We could have gone two ways with this:
- Convert unauthorized errors to "handled" in the base handler layer, and show the user some kind of graceful message.
- Attempt to re-auth the user mid-process. This PR takes this approach, which arguably produces a better UX since the user doesn't have to manually re-run any command.
- The main change from the original PR is that I added retry logic in the GraphQL layer instead of having the low level
apilayer be responsible for orchestrating all retry code. Basically, this is an attempt to address this comment. I do think the code ended up simpler (less new types, less things to pass around) in this approach.- Keeping authN-specific retries in the GraphQL layer felt right since that layer is already aware of the token concept (via
GraphQLRequestBaseOptions), so this keeps the "token concern" at the same level of abstraction while still allowing injectability of the token refresh concept.
- Keeping authN-specific retries in the GraphQL layer felt right since that layer is already aware of the token concept (via
- I also refactored the
createUnauthorizedHandlerfunction into theDeveloperPlatformClient, and used the refactoring to implement the same retry behaviour for app management apps too (note: tests are still within the partners-client for this so this might have to change)
How to test your changes?
- It's hard to test this against production since if you delete or invalidate a token during the CLI flow, the CLI usually catches it because ensureAuthenticated is called very frequently.
- You can test using the app management server by updating this function. You need to be running the local environment (identity + BP + core) so the CLI can talk to local dev. Update that function to check a bogus permission (instead of
develop_apps) and return:unauthorizedinstead of:forbiddenand you will trigger the appropriate response to the CLI and can verify retry behaviour.
Measuring impact
- These bugsnags should dramatically decrease (to zero probably)
- Our CLI SLO should be more stable since HTTP 401s are the biggest cause of instability
Checklist
- [X] I've considered possible cross-platform impacts (Mac, Linux, Windows)
- [X] I've considered possible documentation changes
-
#5740
👈 (View in Graphite)
-
main
This stack of pull requests is managed by Graphite. Learn more about stacking.
We detected some changes at packages/*/src and there are no updates in the .changeset. If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Coverage report
St.:grey_question: |
Category | Percentage | Covered / Total |
|---|---|---|---|
| 🟡 | Statements | 76.73% (+0.08% 🔼) |
9648/12574 |
| 🟡 | Branches | 72.07% (+0.13% 🔼) |
4774/6624 |
| 🟡 | Functions | 76.59% (+0.02% 🔼) |
2493/3255 |
| 🟡 | Lines | 77.24% (+0.08% 🔼) |
9119/11806 |
Show files with reduced coverage 🔻
St.:grey_question: |
File | Statements | Branches | Functions | Lines |
|---|---|---|---|---|---|
| 🔴 | ... / app-management-client.ts |
42.03% (-0.14% 🔻) |
39.34% | 39.6% (-0.4% 🔻) |
40.66% (-0.15% 🔻) |
Test suite run success
2281 tests passing in 987 suites.
Report generated by 🧪jest coverage report action from 2aceb18c8371327808754c2e0bfa37bc361efb83
I agree, it doesn't seem much better and feels a little OOP-y in a largely FP style codebase? You can do mixins in typescript if you like though: https://www.typescriptlang.org/docs/handbook/mixins.html
I think what you've done is fine and gets simpler when there are fewer clients. You could switch things around and have the function that gives this capability also manage the state:
type Refreshable = {
refreshToken: () => Promise<string>
}
const inProgressRefreshes = new WeakMap<Refreshable, Promise<string>>()
export async function waitForRefreshedToken(client: Refreshable) {
let refreshInProgress = inProgressRefreshes.get(client)
if (refreshInProgress) {
return refreshInProgress
} else {
try {
refreshInProgress = client.refreshToken()
inProgressRefreshes.set(client, refreshInProgress)
return refreshInProgress
} finally {
inProgressRefreshes.delete(client)
}
}
}
We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run pnpm changeset add to track your changes and include them in the next release CHANGELOG.
[!CAUTION] DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release.
Differences in type declarations
We detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
- Some seemingly private modules might be re-exported through public modules.
- If the branch is behind
mainyou might see odd diffs, rebasemaininto this branch.
New type declarations
We found no new type declarations in this PR
Existing type declarations
packages/cli-kit/dist/public/node/api/app-management.d.ts
@@ -1,4 +1,4 @@
-import { CacheOptions, GraphQLResponse } from './graphql.js';
+import { CacheOptions, GraphQLResponse, UnauthorizedHandler } from './graphql.js';
import { RequestModeInput } from '../http.js';
import { TypedDocumentNode } from '@graphql-typed-document-node/core';
import { Variables } from 'graphql-request';
@@ -21,9 +21,10 @@ export interface RequestOptions {
* @param variables - GraphQL variables to pass to the query.
* @param cacheOptions - Cache options for the request. If not present, the request will not be cached.
* @param requestOptions - Preferred behaviour for the request.
+ * @param unauthorizedHandler - Optional handler for unauthorized requests.
* @returns The response of the query of generic type <T>.
*/
-export declare function appManagementRequestDoc<TResult, TVariables extends Variables>(orgId: string, query: TypedDocumentNode<TResult, TVariables>, token: string, variables?: TVariables, cacheOptions?: CacheOptions, requestOptions?: RequestOptions): Promise<TResult>;
+export declare function appManagementRequestDoc<TResult, TVariables extends Variables>(orgId: string, query: TypedDocumentNode<TResult, TVariables>, token: string, variables?: TVariables, cacheOptions?: CacheOptions, requestOptions?: RequestOptions, unauthorizedHandler?: UnauthorizedHandler): Promise<TResult>;
/**
* Sets the next deprecation date from [GraphQL response extensions](https://www.apollographql.com/docs/resources/graphql-glossary/#extensions)
* if objects contain a (ISO 8601-formatted string).
packages/cli-kit/dist/public/node/api/graphql.d.ts
@@ -17,6 +17,19 @@ export interface CacheOptions {
cacheExtraKey?: string;
cacheStore?: LocalStorage<ConfSchema>;
}
+interface RefreshedTokenOnAuthorizedResponse {
+ token?: string;
+}
+export type RefreshTokenOnAuthorizedResponse = Promise<RefreshedTokenOnAuthorizedResponse>;
+interface SimpleUnauthorizedHandler {
+ type: 'simple';
+ handler: () => Promise<void>;
+}
+interface TokenRefreshHandler {
+ type: 'token_refresh';
+ handler: () => RefreshTokenOnAuthorizedResponse;
+}
+export type UnauthorizedHandler = SimpleUnauthorizedHandler | TokenRefreshHandler;
interface GraphQLRequestBaseOptions<TResult> {
api: string;
url: string;
@@ -31,7 +44,7 @@ interface GraphQLRequestBaseOptions<TResult> {
export type GraphQLRequestOptions<T> = GraphQLRequestBaseOptions<T> & {
query: RequestDocument;
variables?: Variables;
- unauthorizedHandler?: () => Promise<void>;
+ unauthorizedHandler?: UnauthorizedHandler;
requestBehaviour?: RequestModeInput;
};
export type GraphQLRequestDocOptions<TResult, TVariables> = GraphQLRequestBaseOptions<TResult> & {
@@ -39,7 +52,7 @@ export type GraphQLRequestDocOptions<TResult, TVariables> = GraphQLRequestBaseOp
[key: string]: never;
}>>;
variables?: TVariables;
- unauthorizedHandler?: () => Promise<void>;
+ unauthorizedHandler?: UnauthorizedHandler;
requestBehaviour?: RequestModeInput;
};
export interface GraphQLResponseOptions<T> {
packages/cli-kit/dist/public/node/api/partners.d.ts
@@ -1,4 +1,4 @@
-import { GraphQLVariables, GraphQLResponse, CacheOptions } from './graphql.js';
+import { GraphQLVariables, GraphQLResponse, CacheOptions, UnauthorizedHandler } from './graphql.js';
import { RequestModeInput } from '../http.js';
import { Variables } from 'graphql-request';
import { TypedDocumentNode } from '@graphql-typed-document-node/core';
@@ -10,9 +10,10 @@ import { TypedDocumentNode } from '@graphql-typed-document-node/core';
* @param variables - GraphQL variables to pass to the query.
* @param cacheOptions - Cache options.
* @param preferredBehaviour - Preferred behaviour for the request.
+ * @param unauthorizedHandler - Optional handler for unauthorized requests.
* @returns The response of the query of generic type <T>.
*/
-export declare function partnersRequest<T>(query: string, token: string, variables?: GraphQLVariables, cacheOptions?: CacheOptions, preferredBehaviour?: RequestModeInput): Promise<T>;
+export declare function partnersRequest<T>(query: string, token: string, variables?: GraphQLVariables, cacheOptions?: CacheOptions, preferredBehaviour?: RequestModeInput, unauthorizedHandler?: UnauthorizedHandler): Promise<T>;
export declare const generateFetchAppLogUrl: (cursor?: string, filters?: {
status?: string;
source?: string;
@@ -24,9 +25,10 @@ export declare const generateFetchAppLogUrl: (cursor?: string, filters?: {
* @param token - Partners token.
* @param variables - GraphQL variables to pass to the query.
* @param preferredBehaviour - Preferred behaviour for the request.
+ * @param unauthorizedHandler - Optional handler for unauthorized requests.
* @returns The response of the query of generic type <TResult>.
*/
-export declare function partnersRequestDoc<TResult, TVariables extends Variables>(query: TypedDocumentNode<TResult, TVariables>, token: string, variables?: TVariables, preferredBehaviour?: RequestModeInput): Promise<TResult>;
+export declare function partnersRequestDoc<TResult, TVariables extends Variables>(query: TypedDocumentNode<TResult, TVariables>, token: string, variables?: TVariables, preferredBehaviour?: RequestModeInput, unauthorizedHandler?: UnauthorizedHandler): Promise<TResult>;
/**
* Sets the next deprecation date from [GraphQL response extensions](https://www.apollographql.com/docs/resources/graphql-glossary/#extensions)
* if objects contain a (ISO 8601-formatted string).