Skip to content

Commit 704aae8

Browse files
isaacroldanisaacroldan
authored
Merge pull request #7125 from Shopify/03-30-improve_graphql_api_error_messages_for_access_denied_errors
Improve GraphQL API error messages for access denied errors
2 parents 22818cd + 5cecd5e commit 704aae8

2 files changed

Lines changed: 263 additions & 7 deletions

File tree

‎packages/cli-kit/src/private/node/api/graphql.test.ts‎

Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,192 @@
1+
import {extractGraphQLErrorMessages, errorHandler} from './graphql.js'
2+
import {GraphQLClientError} from './headers.js'
3+
import {AbortError} from '../../../public/node/error.js'
4+
import {ClientError} from 'graphql-request'
5+
import {describe, expect, test} from 'vitest'
6+
7+
describe('extractGraphQLErrorMessages', () => {
8+
test('returns undefined for undefined errors', () => {
9+
expect(extractGraphQLErrorMessages(undefined)).toBeUndefined()
10+
})
11+
12+
test('returns undefined for empty errors array', () => {
13+
expect(extractGraphQLErrorMessages([])).toBeUndefined()
14+
})
15+
16+
test('returns friendly message for access_denied app_errors', () => {
17+
const errors = [
18+
{
19+
message: 'Query errors',
20+
extensions: {
21+
app_errors: {
22+
errors: [
23+
{
24+
message: 'User does not have access to app_by_api_key.',
25+
category: 'access_denied',
26+
},
27+
],
28+
},
29+
},
30+
},
31+
]
32+
expect(extractGraphQLErrorMessages(errors)).toBe(
33+
"You don't have the necessary permissions to perform this action. Check that you're using the correct account or token.",
34+
)
35+
})
36+
37+
test('extracts raw message for non-access_denied app_errors', () => {
38+
const errors = [
39+
{
40+
message: 'Query errors',
41+
extensions: {
42+
app_errors: {
43+
errors: [{message: 'Something else went wrong.', category: 'validation'}],
44+
},
45+
},
46+
},
47+
]
48+
expect(extractGraphQLErrorMessages(errors)).toBe('Something else went wrong.')
49+
})
50+
51+
test('extracts multiple messages from extensions.app_errors', () => {
52+
const errors = [
53+
{
54+
message: 'Query errors',
55+
extensions: {
56+
app_errors: {
57+
errors: [{message: 'First error.'}, {message: 'Second error.'}],
58+
},
59+
},
60+
},
61+
]
62+
expect(extractGraphQLErrorMessages(errors)).toBe('First error.\nSecond error.')
63+
})
64+
65+
test('falls back to top-level error message when no app_errors', () => {
66+
const errors = [{message: 'Something went wrong'}]
67+
expect(extractGraphQLErrorMessages(errors)).toBe('Something went wrong')
68+
})
69+
70+
test('handles multiple top-level errors', () => {
71+
const errors = [{message: 'Error one'}, {message: 'Error two'}]
72+
expect(extractGraphQLErrorMessages(errors)).toBe('Error one\nError two')
73+
})
74+
75+
test('handles mix of app_errors and top-level errors', () => {
76+
const errors = [
77+
{
78+
message: 'Query errors',
79+
extensions: {
80+
app_errors: {
81+
errors: [{message: 'Something broke.', category: 'validation'}],
82+
},
83+
},
84+
},
85+
{message: 'Another error'},
86+
]
87+
expect(extractGraphQLErrorMessages(errors)).toBe('Something broke.\nAnother error')
88+
})
89+
90+
test('falls back to top-level message when app_errors.errors is empty', () => {
91+
const errors = [
92+
{
93+
message: 'Something went wrong',
94+
extensions: {
95+
app_errors: {
96+
errors: [],
97+
},
98+
},
99+
},
100+
]
101+
expect(extractGraphQLErrorMessages(errors)).toBe('Something went wrong')
102+
})
103+
104+
test('falls back to top-level message when app_errors entries have no extractable message', () => {
105+
const errors = [
106+
{
107+
message: 'Query errors',
108+
extensions: {
109+
app_errors: {
110+
errors: [{code: 'UNKNOWN'}],
111+
},
112+
},
113+
},
114+
]
115+
expect(extractGraphQLErrorMessages(errors)).toBe('Query errors')
116+
})
117+
118+
test('handles errors with no message', () => {
119+
const errors = [{code: 'UNKNOWN'}]
120+
expect(extractGraphQLErrorMessages(errors)).toBeUndefined()
121+
})
122+
})
123+
124+
describe('errorHandler', () => {
125+
function createClientError(errors: unknown[], status = 200): ClientError {
126+
const response = {
127+
status,
128+
headers: new Map(),
129+
errors,
130+
} as unknown as ClientError['response']
131+
const error = new ClientError(response, {query: 'query {}'})
132+
return error
133+
}
134+
135+
test('returns friendly message for access_denied app_errors', () => {
136+
const handler = errorHandler('App Management')
137+
const clientError = createClientError([
138+
{
139+
message: 'Query errors',
140+
extensions: {
141+
app_errors: {
142+
errors: [{message: 'User does not have access to app_by_api_key.', category: 'access_denied'}],
143+
},
144+
},
145+
},
146+
])
147+
const result = handler(clientError) as GraphQLClientError
148+
expect(result).toBeInstanceOf(GraphQLClientError)
149+
expect(result.message).toBe(
150+
"You don't have the necessary permissions to perform this action. Check that you're using the correct account or token.",
151+
)
152+
})
153+
154+
test('falls back to JSON dump when no extractable messages', () => {
155+
const handler = errorHandler('App Management')
156+
const clientError = createClientError([{code: 'UNKNOWN'}])
157+
const result = handler(clientError) as GraphQLClientError
158+
expect(result).toBeInstanceOf(GraphQLClientError)
159+
expect(result.message).toContain('App Management GraphQL API responded unsuccessfully')
160+
})
161+
162+
test('appends request ID when provided', () => {
163+
const handler = errorHandler('App Management')
164+
const clientError = createClientError([
165+
{
166+
message: 'Query errors',
167+
extensions: {
168+
app_errors: {
169+
errors: [{message: 'Something broke.', category: 'validation'}],
170+
},
171+
},
172+
},
173+
])
174+
const result = handler(clientError, 'req-123') as GraphQLClientError
175+
expect(result.message).toContain('Something broke.')
176+
expect(result.message).toContain('Request ID: req-123')
177+
})
178+
179+
test('returns AbortError for 5xx status with API context', () => {
180+
const handler = errorHandler('App Management')
181+
const clientError = createClientError([{message: 'Internal server error'}], 500)
182+
const result = handler(clientError) as AbortError
183+
expect(result).toBeInstanceOf(AbortError)
184+
expect(result.message).toBe('The App Management GraphQL API responded with HTTP status 500: Internal server error')
185+
})
186+
187+
test('passes through non-ClientError errors', () => {
188+
const handler = errorHandler('App Management')
189+
const error = new Error('something else')
190+
expect(handler(error)).toBe(error)
191+
})
192+
})

‎packages/cli-kit/src/private/node/api/graphql.ts‎

Lines changed: 71 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -66,21 +66,85 @@ function sanitizeDeepVariables(value: unknown, sensitiveKeys: string[]): unknown
6666
return result
6767
}
6868

69+
/**
70+
* Extracts human-readable error messages from a GraphQL errors array.
71+
*
72+
* Some APIs (e.g. App Management) return structured errors nested inside
73+
* `extensions.app_errors.errors[].message`. When those are present, we extract
74+
* them so the CLI displays a clean message instead of a raw JSON dump.
75+
* Falls back to each error's top-level `message` field, and ultimately to
76+
* the full JSON representation if no messages can be extracted.
77+
*/
78+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
79+
export function extractGraphQLErrorMessages(errors: any[] | undefined): string | undefined {
80+
if (!errors || errors.length === 0) return undefined
81+
82+
const messages: string[] = []
83+
84+
for (const error of errors) {
85+
// Check for nested app_errors (App Management API pattern)
86+
const appErrors = error?.extensions?.app_errors?.errors
87+
if (Array.isArray(appErrors) && appErrors.length > 0) {
88+
const appMessages: string[] = []
89+
for (const appError of appErrors) {
90+
const friendlyMessage = friendlyAppErrorMessage(appError)
91+
if (friendlyMessage) {
92+
appMessages.push(friendlyMessage)
93+
} else if (typeof appError?.message === 'string') {
94+
appMessages.push(appError.message)
95+
}
96+
}
97+
// Fall back to top-level error message if no app_error messages were extracted
98+
if (appMessages.length > 0) {
99+
messages.push(...appMessages)
100+
} else if (typeof error?.message === 'string') {
101+
messages.push(error.message)
102+
}
103+
}
104+
// Fall back to top-level error message
105+
else if (typeof error?.message === 'string') {
106+
messages.push(error.message)
107+
}
108+
}
109+
110+
return messages.length > 0 ? messages.join('\n') : undefined
111+
}
112+
113+
/**
114+
* Maps known app_errors categories to user-friendly messages.
115+
* Returns undefined if the error doesn't match a known pattern.
116+
*/
117+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
118+
function friendlyAppErrorMessage(appError: any): string | undefined {
119+
if (appError?.category === 'access_denied') {
120+
return "You don't have the necessary permissions to perform this action. Check that you're using the correct account or token."
121+
}
122+
return undefined
123+
}
124+
69125
export function errorHandler(api: string): (error: unknown, requestId?: string) => unknown {
70126
return (error: unknown, requestId?: string) => {
71127
if (error instanceof ClientError) {
72128
const {status} = error.response
73-
let errorMessage = stringifyMessage(outputContent`
129+
130+
const extractedMessages = extractGraphQLErrorMessages(error.response.errors)
131+
132+
let errorMessage: string
133+
if (extractedMessages && status < 500) {
134+
errorMessage = extractedMessages
135+
} else if (extractedMessages && status >= 500) {
136+
errorMessage = `The ${api} GraphQL API responded with HTTP status ${status}: ${extractedMessages}`
137+
} else {
138+
errorMessage = stringifyMessage(outputContent`
74139
The ${outputToken.raw(api)} GraphQL API responded unsuccessfully with${
75-
status === 200 ? '' : ` the HTTP status ${status} and`
76-
} errors:
140+
status === 200 ? '' : ` the HTTP status ${status} and`
141+
} errors:
77142
78143
${outputToken.json(error.response.errors)}
79-
`)
144+
`)
145+
}
80146
if (requestId) {
81-
errorMessage += `
82-
Request ID: ${requestId}
83-
`
147+
errorMessage += `\n\nRequest ID: ${requestId}`
84148
}
85149
let mappedError: Error
86150
if (status < 500) {

0 commit comments

Comments
 (0)