Skip to content

Commit ac01ab8

Browse files
pregnantboypregnantboy
authored
fix: postman sms retriable error not properly thrown (#1017)
## Problem Postman SMS retriable error was re-wrapped with StepError resulting in no retries ## Solution Dont re-catch and wrap this error
1 parent cda7b56 commit ac01ab8

File tree

6 files changed

+206
-88
lines changed

6 files changed

+206
-88
lines changed

‎packages/backend/src/apps/postman-sms/__tests__/actions.send-sms.test.ts

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -37,15 +37,18 @@ const mocks = vi.hoisted(() => ({
3737
setActionItem: vi.fn(),
3838
logError: vi.fn(),
3939
authDataParseResult: vi.fn(() => ({
40-
env: 'test',
41-
campaignId: 'test-campaign',
42-
apiKey: 'test-api-key',
40+
success: true,
41+
data: {
42+
env: 'test',
43+
campaignId: 'test-campaign',
44+
apiKey: 'test-api-key',
45+
},
4346
})),
4447
}))
4548

4649
vi.mock('../auth/schema', () => ({
4750
authDataSchema: {
48-
parse: mocks.authDataParseResult,
51+
safeParse: mocks.authDataParseResult,
4952
},
5053
}))
5154

@@ -57,12 +60,15 @@ vi.mock('@/helpers/logger', () => ({
5760

5861
vi.mock('axios', async (importOriginal) => {
5962
const actualAxios = await importOriginal<typeof import('axios')>()
60-
const mockCreate: typeof actualAxios.default.create = (createConfig) =>
61-
actualAxios.default.create({
63+
const mockCreate: typeof actualAxios.default.create = (createConfig) => {
64+
const instance = actualAxios.default.create({
6265
...createConfig,
6366
adapter: mocks.axiosRequestAdapter,
6467
})
6568

69+
return instance
70+
}
71+
6672
return {
6773
default: {
6874
...actualAxios.default,
@@ -112,9 +118,12 @@ describe('Send SMS Action', () => {
112118

113119
it('uses the campaign ID in auth data', async () => {
114120
mocks.authDataParseResult.mockReturnValue({
115-
env: 'test',
116-
campaignId: 'my-campaign-id',
117-
apiKey: 'test-key',
121+
success: true,
122+
data: {
123+
env: 'test',
124+
campaignId: 'my-campaign-id',
125+
apiKey: 'test-key',
126+
},
118127
})
119128
await sendSmsAction.run($)
120129

‎packages/backend/src/apps/postman-sms/__tests__/request-error-handler.test.ts

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
import type { IGlobalVariable } from '@plumber/types'
2+
3+
import type { AxiosError } from 'axios'
4+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
5+
6+
import HttpError from '@/errors/http'
7+
import RetriableError from '@/errors/retriable-error'
8+
import StepError from '@/errors/step'
9+
10+
import requestErrorHandler from '../common/request-error-handler'
11+
12+
describe('Postman SMS request error handler', () => {
13+
let $: IGlobalVariable
14+
15+
beforeEach(() => {
16+
$ = {
17+
app: {
18+
name: 'postman-sms',
19+
},
20+
step: {
21+
position: 2,
22+
},
23+
} as unknown as IGlobalVariable
24+
})
25+
26+
afterEach(() => {
27+
vi.restoreAllMocks()
28+
})
29+
30+
describe('HTTP 429 (Too Many Requests)', () => {
31+
it('throws `step` RetriableError with default delay when retry-after header is missing', async () => {
32+
const axiosError = {
33+
isAxiosError: true,
34+
name: 'AxiosError',
35+
message: 'Request failed with status code 429',
36+
response: {
37+
status: 429,
38+
},
39+
} as unknown as AxiosError
40+
41+
const error = new HttpError(axiosError)
42+
43+
await expect(requestErrorHandler($, error)).rejects.toThrow(
44+
RetriableError,
45+
)
46+
await expect(requestErrorHandler($, error)).rejects.toMatchObject({
47+
delayType: 'step',
48+
delayInMs: 3000,
49+
})
50+
})
51+
})
52+
53+
describe('HTTP 400 with parameter_invalid', () => {
54+
it('throws StepError with campaign template setup message', async () => {
55+
const axiosError = {
56+
isAxiosError: true,
57+
name: 'AxiosError',
58+
message: 'Request failed with status code 400',
59+
response: {
60+
status: 400,
61+
data: {
62+
error: {
63+
code: 'parameter_invalid',
64+
},
65+
},
66+
},
67+
} as unknown as AxiosError
68+
69+
const error = new HttpError(axiosError)
70+
71+
await expect(requestErrorHandler($, error)).rejects.toThrow(StepError)
72+
})
73+
})
74+
75+
describe('Other errors', () => {
76+
it('throws StepError with generic error message', async () => {
77+
const axiosError = {
78+
isAxiosError: true,
79+
name: 'AxiosError',
80+
message: 'Internal server error',
81+
response: {
82+
status: 500,
83+
},
84+
} as unknown as AxiosError
85+
86+
const error = new HttpError(axiosError)
87+
88+
await expect(requestErrorHandler($, error)).rejects.toThrow(StepError)
89+
})
90+
})
91+
})

‎packages/backend/src/apps/postman-sms/actions/send-sms/index.ts

Lines changed: 47 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,7 @@
11
import type { IRawAction } from '@plumber/types'
22

33
import { DateTime } from 'luxon'
4-
import { ZodError } from 'zod'
54

6-
import HttpError from '@/errors/http'
75
import StepError, { GenericSolution } from '@/errors/step'
86
import logger from '@/helpers/logger'
97
import { ensureZodObjectKey, firstZodParseError } from '@/helpers/zod-utils'
@@ -56,89 +54,63 @@ const action = {
5654
getDataOutMetadata,
5755

5856
async run($) {
59-
try {
60-
const parsedParams = fieldSchema.parse($.step.parameters)
61-
const authData = authDataSchema.parse($.auth.data)
57+
const parsedParams = fieldSchema.safeParse($.step.parameters)
58+
if (parsedParams.success === false) {
59+
throw new StepError(
60+
`Configuration problem: ${firstZodParseError(parsedParams.error)}`,
61+
GenericSolution.ReconfigureInvalidField,
62+
$.step.position,
63+
$.app.name,
64+
)
65+
}
66+
const authData = authDataSchema.safeParse($.auth.data)
67+
if (authData.success === false) {
68+
throw new StepError(
69+
`Invalid connection data: ${firstZodParseError(authData.error)}`,
70+
GenericSolution.MalformedConnectionData,
71+
$.step.position,
72+
$.app.name,
73+
)
74+
}
6275

63-
const response = await $.http.post(
64-
'/campaigns/:campaignId/messages',
65-
{
66-
recipient: parsedParams.recipient,
67-
language: 'english',
68-
values: {
69-
body: parsedParams.message,
70-
},
76+
const response = await $.http.post(
77+
'/campaigns/:campaignId/messages',
78+
{
79+
recipient: parsedParams.data.recipient,
80+
language: 'english',
81+
values: {
82+
body: parsedParams.data.message,
7183
},
72-
{
73-
urlPathParams: {
74-
campaignId: authData.campaignId,
75-
},
84+
},
85+
{
86+
urlPathParams: {
87+
campaignId: authData.data.campaignId,
7688
},
77-
)
78-
const parsedResponse = postmanMessageSchema.safeParse(response.data)
89+
},
90+
)
91+
const parsedResponse = postmanMessageSchema.safeParse(response.data)
7992

80-
if (parsedResponse.success) {
81-
$.setActionItem({
82-
raw: {
83-
message: parsedResponse.data,
84-
},
85-
})
86-
87-
return
88-
}
89-
90-
//
91-
// Edge case: If we're here, then Postman's response has somehow changed
92-
// without us knowing about it.
93-
//
94-
// We'll allow the step to succeed as SMS has already been sent out, but
95-
// we don't expose any of the response data because we have no clue what
96-
// it contains now; we don't want to return gibberish to prevent the user
97-
// into a false sense of correctness.
98-
//
99-
100-
logger.error('Postman send single SMS response changed', {
101-
event: 'api-response-change',
102-
appName: 'postman-sms',
103-
eventName: 'sendSms',
104-
})
93+
if (parsedResponse.success) {
10594
$.setActionItem({
10695
raw: {
107-
// Signal to the user that an SMS has at least been created by now.
108-
createdAt: DateTime.now().toISO(),
96+
message: response.data,
10997
},
11098
})
111-
} catch (error) {
112-
if (error instanceof ZodError) {
113-
throw new StepError(
114-
`Configuration problem: '${firstZodParseError(error)}'`,
115-
GenericSolution.ReconfigureInvalidField,
116-
$.step.position,
117-
$.app.name,
118-
)
119-
}
120-
121-
// This happens if user did not create a template in the format we expect.
122-
if (
123-
error instanceof HttpError &&
124-
error.response.status === 400 &&
125-
error.response.data.error?.code === 'parameter_invalid'
126-
) {
127-
throw new StepError(
128-
'Campaign template was not set up correctly',
129-
'Ensure that you have followed the instructions in our guide to set up your campaign template.',
130-
$.step.position,
131-
$.app.name,
132-
)
133-
}
13499

135-
throw new StepError(
136-
'Error sending SMS',
137-
error.message,
138-
$.step.position,
139-
$.app.name,
140-
)
100+
return
141101
}
102+
103+
logger.error('Postman send single SMS response changed', {
104+
event: 'api-response-change',
105+
appName: 'postman-sms',
106+
eventName: 'sendSms',
107+
})
108+
$.setActionItem({
109+
raw: {
110+
// Signal to the user that an SMS has at least been created by now.
111+
createdAt: DateTime.now().toISO(),
112+
},
113+
})
142114
},
143115
} satisfies IRawAction
144116

‎packages/backend/src/apps/postman-sms/common/request-error-handler.ts

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,22 @@
11
import type { IApp } from '@plumber/types'
22

33
import RetriableError from '@/errors/retriable-error'
4+
import StepError from '@/errors/step'
45
import { parseRetryAfterToMs } from '@/helpers/parse-retry-after-to-ms'
56

67
type ThrowingHandler = (
78
...args: Parameters<IApp['requestErrorHandler']>
89
) => never
910

10-
const handle429: ThrowingHandler = ($, error) => {
11+
const handle429: ThrowingHandler = (_, error): never => {
1112
const retryAfterMs =
1213
parseRetryAfterToMs(error.response?.headers?.['retry-after']) ?? 'default'
1314

1415
throw new RetriableError({
1516
error: 'Retrying HTTP 429 from Postman SMS',
16-
delayType: 'group',
17+
// pausing the entire queue here is not a good idea because we wont be able to fully utilize
18+
// the throughput that the campaign supports, so we throw step error here instead of group
19+
delayType: 'step',
1720
delayInMs: retryAfterMs,
1821
})
1922
}
@@ -26,8 +29,24 @@ const requestErrorHandler: IApp['requestErrorHandler'] = async function (
2629
return handle429($, error)
2730
}
2831

29-
// No-op for non-429; our Axios interceptor will re-throw the original
30-
// HttpError
32+
if (
33+
error.response.status === 400 &&
34+
error.response.data.error?.code === 'parameter_invalid'
35+
) {
36+
throw new StepError(
37+
'Campaign template was not set up correctly',
38+
'Ensure that you have followed the instructions in our guide to set up your campaign template.',
39+
$.step.position,
40+
$.app.name,
41+
)
42+
}
43+
44+
throw new StepError(
45+
'Error sending SMS',
46+
error.message,
47+
$.step.position,
48+
$.app.name,
49+
)
3150
}
3251

3352
export default requestErrorHandler

‎packages/backend/src/errors/step.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import HttpError from './http'
77
//
88
export enum GenericSolution {
99
ReconfigureInvalidField = 'Click on set up action and reconfigure the invalid field. Error could also result from the variables used in the field.',
10+
MalformedConnectionData = 'Please reconnect and try again.',
1011
}
1112

1213
export default class StepError extends Error {

0 commit comments

Comments
 (0)