Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(server, orchestrator, jobs, runner): More descriptive action errors #3279

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
e14fb4a
Tweak runner output error type
nalanj Jan 8, 2025
979ac05
Add specialized handling for http errors in exec
nalanj Jan 8, 2025
e9933d3
Fix ratelimit header filter
nalanj Jan 8, 2025
8045a2d
Cleanup
nalanj Jan 8, 2025
5ec18b9
Go back to instanceof check
nalanj Jan 8, 2025
a23ff56
Fix some type errors
nalanj Jan 8, 2025
a5be5ce
Merge branch 'master' into alan/action-errors
nalanj Jan 9, 2025
48ae65e
Add comment about why I'm using isAxiosError
nalanj Jan 9, 2025
7a397e7
Fix all eslint warnings in exec.ts
nalanj Jan 9, 2025
c43fe83
Update jobs putTask to handle new error format in validation
nalanj Jan 9, 2025
4cf4721
WIP on figuring out how to get error to flow through orchestrator
nalanj Jan 9, 2025
67974ad
Merge branch 'master' into alan/action-errors
nalanj Jan 10, 2025
2ab655a
Rework error to have additional_properties
nalanj Jan 10, 2025
d06af85
Got upstream errors flowing out of action errors
nalanj Jan 10, 2025
d995508
Have additional_properties set to undefined by default
nalanj Jan 10, 2025
d2c1a53
Merge branch 'master' into alan/action-errors
nalanj Jan 10, 2025
4db0b09
Add 424 to openapi spec
nalanj Jan 10, 2025
5ecaa06
Re-add mystery code removal. I'm assuming it was eslint formatting wh…
nalanj Jan 13, 2025
b61728b
Remove note about not reaching instance
nalanj Jan 13, 2025
a39804e
Move connectionid assertion
nalanj Jan 13, 2025
6b665e0
Update error response to just have an upstream type
nalanj Jan 13, 2025
3e2cb1b
Clean up response handling
nalanj Jan 13, 2025
869c651
Merge branch 'master' into alan/action-errors
nalanj Jan 17, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions docs-v2/spec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1434,6 +1434,45 @@ paths:
message:
type: string

'424':
description: Action http error
content:
application/json:
schema:
type: object
properties:
error:
type: object
properties:
message:
type: string
code:
type: string
payload:
type: object
upstream:
type: object
properties:
status_code:
type: integer
headers:
type: object
properties:
content-type:
type: string
patternProperties:
'^x-rate':
type: string
body:
type: object
additionalProperties: true
required:
- message
- code
- payload
required:
- error

/environment-variables:
get:
description: Retrieve the environment variables as added in the Nango dashboard
Expand Down
13 changes: 4 additions & 9 deletions packages/jobs/lib/routes/tasks/putTask.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { z } from 'zod';
import type { ApiError, Endpoint } from '@nangohq/types';
import type { ApiError, Endpoint, RunnerOutputError } from '@nangohq/types';
import { validateRequest } from '@nangohq/utils';
import type { EndpointRequest, EndpointResponse, RouteHandler } from '@nangohq/utils';
import { handleError, handleSuccess } from '../../execution/operations/output.js';
Expand All @@ -17,13 +17,7 @@ type PutTask = Endpoint<{
};
Body: {
nangoProps?: NangoProps;
error?:
| {
type: string;
payload: Record<string, unknown>;
status: number;
}
| undefined;
error?: RunnerOutputError | undefined;
output: JsonValue;
};
Error: ApiError<'put_task_failed'>;
Expand Down Expand Up @@ -101,7 +95,8 @@ const validate = validateRequest<PutTask>({
.object({
type: z.string(),
payload: z.record(z.string(), z.unknown()).or(z.unknown().transform((v) => ({ message: v }))),
status: z.number()
status: z.number(),
additional_properties: z.record(z.string(), z.unknown()).optional()
})
.optional(),
output: jsonSchema.default(null)
Expand Down
4 changes: 3 additions & 1 deletion packages/orchestrator/lib/clients/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ export class OrchestratorClient {
retryIf: (res) => 'error' in res && Date.now() < retryUntil
}
})({ params: { taskId }, query: { longPolling: 30_000 } });

if ('error' in getOutput) {
return Err({
name: getOutput.error.code,
Expand Down Expand Up @@ -408,7 +409,8 @@ export class OrchestratorClient {
name: error.name,
type: 'type' in error ? (error.type as string) : 'unknown_error',
message: error.message,
payload: 'payload' in error ? (error.payload as any) : null
payload: 'payload' in error ? (error.payload as any) : null,
additional_properties: 'additional_properties' in error ? (error.additional_properties as any) : null
};
const res = await this.routeFetch(putTaskRoute)({
params: { taskId },
Expand Down
1 change: 1 addition & 0 deletions packages/orchestrator/lib/clients/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -203,4 +203,5 @@ export function TaskOnEvent(props: TaskCommonFields & OnEventArgs): TaskOnEvent
export interface ClientError extends Error {
name: string;
payload: JsonValue;
additional_properties?: Record<string, JsonValue>;
}
49 changes: 41 additions & 8 deletions packages/runner/lib/exec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { NangoProps } from '@nangohq/shared';
import { AxiosError } from 'axios';
import { isAxiosError } from 'axios';
import { ActionError, NangoSync, NangoAction, instrumentSDK, SpanTypes, validateData, NangoError } from '@nangohq/shared';
import { Buffer } from 'buffer';
import * as vm from 'node:vm';
Expand All @@ -13,6 +13,11 @@ import { errorToObject, metrics, truncateJson } from '@nangohq/utils';
import { logger } from './utils.js';
import type { RunnerOutput } from '@nangohq/types';

interface ScriptExports {
onWebhookPayloadReceived?: (nango: NangoAction, payload?: object) => Promise<unknown>;
default: (nango: NangoAction, payload?: object) => Promise<unknown>;
}

export async function exec(
nangoProps: NangoProps,
code: string,
Expand Down Expand Up @@ -76,7 +81,7 @@ export async function exec(
};

const context = vm.createContext(sandbox);
const scriptExports = script.runInContext(context);
const scriptExports = script.runInContext(context) as ScriptExports;

if (nangoProps.scriptType === 'webhook') {
if (!scriptExports.onWebhookPayloadReceived) {
Expand Down Expand Up @@ -174,16 +179,44 @@ export async function exec(
},
response: null
};
} else if (err instanceof AxiosError) {
} else if (isAxiosError<unknown, unknown>(err)) {
// isAxiosError lets us use something the shape of an axios error in
// testing, which is handy with how strongly typed everything is

span.setTag('error', err);
if (err.response?.data) {
const errorResponse = err.response.data.payload || err.response.data;
if (err.response) {
const maybeData = err.response.data;

let errorResponse: unknown = {};
if (maybeData && typeof maybeData === 'object' && 'payload' in maybeData) {
errorResponse = maybeData.payload as Record<string, unknown>;
} else {
errorResponse = maybeData;
}

const headers = Object.fromEntries(
Object.entries(err.response.headers)
.map<[string, string]>(([k, v]) => [k.toLowerCase(), String(v)])
.filter(([k]) => k === 'content-type' || k.startsWith('x-rate'))
);

const responseBody: Record<string, unknown> = truncateJson(
errorResponse && typeof errorResponse === 'object' ? (errorResponse as Record<string, unknown>) : { message: errorResponse }
);

return {
success: false,
error: {
type: 'script_http_error',
payload: truncateJson(typeof errorResponse === 'string' ? { message: errorResponse } : errorResponse),
status: err.response.status
payload: responseBody,
status: err.response.status,
additional_properties: {
upstream_response: {
status: err.response.status,
headers,
body: responseBody
}
}
},
response: null
};
Expand All @@ -192,7 +225,7 @@ export async function exec(
return {
success: false,
error: {
type: 'script_http_error',
type: 'script_network_error',
nalanj marked this conversation as resolved.
Show resolved Hide resolved
payload: truncateJson({ name: tmp.name || 'Error', code: tmp.code, message: tmp.message }),
status: 500
},
Expand Down
68 changes: 59 additions & 9 deletions packages/runner/lib/exec.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,12 @@ function getNangoProps(): NangoProps {
syncConfig: {} as DBSyncConfig,
debug: false,
startedAt: new Date(),
runnerFlags: {} as any,
runnerFlags: {
validateActionInput: false,
validateActionOutput: false,
validateSyncMetadata: false,
validateSyncRecords: false
},
stubbedMetadata: {},
endUser: null
};
Expand Down Expand Up @@ -127,26 +132,71 @@ describe('Exec', () => {
expect(res.success).toEqual(false);
});

it('should return a formatted error when receiving an AxiosError (without a body)', async () => {
it('should return a script_network_error when receiving an AxiosError (without a body)', async () => {
const nangoProps = getNangoProps();
const jsCode = `
fn = async (nango) => {
await nango.get({
endpoint: '/',
baseUrl: 'https://example.dev/'
})
const err = new Error("Something broke");
err.isAxiosError = true;
err.code = "ECONNREFUSED";

throw err;
nalanj marked this conversation as resolved.
Show resolved Hide resolved
};
exports.default = fn
`;
const res = await exec(nangoProps, jsCode);

// NB: it will fail because Nango is not running not because the website is not reachable
// NB2: the message is different depending on the system running Node
expect(res.error).toMatchObject({
payload: {
code: 'ECONNREFUSED'
},
status: 500,
type: 'script_network_error'
});
expect(res.success).toEqual(false);
});

it('should return a script_network_error when receiving an AxiosError (without a body)', async () => {
const nangoProps = getNangoProps();
const jsCode = `
fn = async (nango) => {
const err = new Error("Something broke");
err.isAxiosError = true;
err.code = "ERR_BAD_RESPONSE";
err.response = {
status: 404,
headers: {
'Content-Type': 'application/json',
'Content-Security-Policy': 'blech',
'X-RateLimit-Limit': '100',
},
data: { error: "Not found" }
}
throw err;
};
exports.default = fn
`;
const res = await exec(nangoProps, jsCode);

// NB: it will fail because Nango is not running not because the website is not reachable
// NB2: the message is different depending on the system running Node
bodinsamuel marked this conversation as resolved.
Show resolved Hide resolved
expect(res.error).toEqual({
payload: {
error: 'Not found'
},
status: 404,
additional_properties: {
upstream_response: {
body: {
error: 'Not found'
},
headers: {
'content-type': 'application/json',
'x-ratelimit-limit': '100'
},
status: 404
}
},
type: 'script_http_error'
});
expect(res.success).toEqual(false);
Expand Down Expand Up @@ -175,7 +225,7 @@ describe('Exec', () => {
expect(res.success).toEqual(false);
});

it('should redac Authorization', async () => {
it('should redact Authorization', async () => {
const nangoProps = getNangoProps();
const jsCode = `
fn = async (nango) => {
Expand Down
15 changes: 14 additions & 1 deletion packages/server/lib/controllers/sync.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,20 @@ class SyncController {
span.setTag('nango.error', actionResponse.error);
await logCtx.failed();

errorManager.errResFromNangoErr(res, actionResponse.error);
if (actionResponse.error.type === 'script_http_error') {
res.status(424).json({
error: {
payload: actionResponse.error.payload,
code: actionResponse.error.type,
...(actionResponse.error.additional_properties && 'upstream_response' in actionResponse.error.additional_properties
? { upstream: actionResponse.error.additional_properties['upstream_response'] }
: {})
}
});
} else {
errorManager.errResFromNangoErr(res, actionResponse.error);
}

span.finish();
return;
}
Expand Down
21 changes: 15 additions & 6 deletions packages/shared/lib/clients/orchestrator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import { getSyncConfigRaw, getSyncConfigBySyncId } from '../services/sync/config
import environmentService from '../services/environment.service.js';
import type { DBEnvironment, DBTeam } from '@nangohq/types';
import type { RecordCount } from '@nangohq/records';
import type { JsonValue } from 'type-fest';

export interface RecordsServiceInterface {
deleteRecordsBySyncId({
Expand Down Expand Up @@ -98,7 +99,7 @@ export class Orchestrator {
return Ok(scheduleMap);
}

async triggerAction<T = any>({
async triggerAction<T = unknown>({
connection,
actionName,
input,
Expand All @@ -117,13 +118,18 @@ export class Orchestrator {
'connection.provider_config_key': connection.provider_config_key,
'connection.environment_id': connection.environment_id
};

const span = tracer.startSpan('execute.action', {
tags: spanTags,
...(activeSpan ? { childOf: activeSpan } : {})
});
const startTime = Date.now();
try {
let parsedInput = null;
if (!connection.id) {
throw new NangoError('invalid_input', { connection });
}

let parsedInput: JsonValue = null;
try {
parsedInput = input ? JSON.parse(JSON.stringify(input)) : null;
} catch (err) {
Expand All @@ -136,7 +142,7 @@ export class Orchestrator {
const args = {
actionName,
connection: {
id: connection.id!,
id: connection.id,
connection_id: connection.connection_id,
provider_config_key: connection.provider_config_key,
environment_id: connection.environment_id
Expand All @@ -153,7 +159,10 @@ export class Orchestrator {
const res = actionResult.mapError((err) => {
return (
deserializeNangoError(err.payload) ||
new NangoError('action_script_failure', { error: err.message, ...(err.payload ? { payload: err.payload } : {}) })
new NangoError('action_script_failure', {
error: err.message,
...(err.payload ? { payload: err.payload } : {})
})
);
});

Expand Down Expand Up @@ -241,7 +250,7 @@ export class Orchestrator {
}
}

async triggerWebhook<T = any>({
async triggerWebhook<T = unknown>({
account,
environment,
integration,
Expand Down Expand Up @@ -370,7 +379,7 @@ export class Orchestrator {
}
}

async triggerOnEventScript<T = any>({
async triggerOnEventScript<T = unknown>({
connection,
version,
name,
Expand Down
Loading
Loading