-
Notifications
You must be signed in to change notification settings - Fork 451
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
base: master
Are you sure you want to change the base?
Changes from 19 commits
e14fb4a
979ac05
e9933d3
8045a2d
5ec18b9
a23ff56
a5be5ce
48ae65e
7a397e7
c43fe83
4cf4721
67974ad
2ab655a
d06af85
d995508
d2c1a53
4db0b09
5ecaa06
b61728b
a39804e
6b665e0
3e2cb1b
869c651
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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({ | ||
|
@@ -98,7 +99,7 @@ export class Orchestrator { | |
return Ok(scheduleMap); | ||
} | ||
|
||
async triggerAction<T = any>({ | ||
async triggerAction<T = unknown>({ | ||
connection, | ||
actionName, | ||
input, | ||
|
@@ -117,13 +118,16 @@ export class Orchestrator { | |
'connection.provider_config_key': connection.provider_config_key, | ||
'connection.environment_id': connection.environment_id | ||
}; | ||
if (!connection.id) { | ||
throw new NangoError('invalid_input', { connection }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is going to throw the error instead of returning an Err There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in a39804e |
||
} | ||
const span = tracer.startSpan('execute.action', { | ||
tags: spanTags, | ||
...(activeSpan ? { childOf: activeSpan } : {}) | ||
}); | ||
const startTime = Date.now(); | ||
try { | ||
let parsedInput = null; | ||
let parsedInput: JsonValue = null; | ||
try { | ||
parsedInput = input ? JSON.parse(JSON.stringify(input)) : null; | ||
} catch (err) { | ||
|
@@ -136,7 +140,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 | ||
|
@@ -153,7 +157,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 } : {}) | ||
}) | ||
); | ||
}); | ||
|
||
|
@@ -241,7 +248,7 @@ export class Orchestrator { | |
} | ||
} | ||
|
||
async triggerWebhook<T = any>({ | ||
async triggerWebhook<T = unknown>({ | ||
account, | ||
environment, | ||
integration, | ||
|
@@ -370,7 +377,7 @@ export class Orchestrator { | |
} | ||
} | ||
|
||
async triggerOnEventScript<T = any>({ | ||
async triggerOnEventScript<T = unknown>({ | ||
connection, | ||
version, | ||
name, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,9 +44,13 @@ class ErrorManager { | |
if (err) { | ||
logger.error(`Response error`, errorToObject(err)); | ||
if (!err.message) { | ||
res.status(err.status || 500).send({ error: { code: err.type || 'unknown_error', payload: err.payload } }); | ||
res.status(err.status || 500).send({ | ||
error: { code: err.type || 'unknown_error', payload: err.payload, additional_properties: err.additional_properties } | ||
}); | ||
} else { | ||
res.status(err.status || 500).send({ error: { message: err.message, code: err.type, payload: err.payload } }); | ||
res.status(err.status || 500).send({ | ||
error: { message: err.message, code: err.type, payload: err.payload, additional_properties: err.additional_properties } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. additional properties can be undefined. should we not return it at all in this case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It just gets pruned when it's undefined, since it serializes:
|
||
}); | ||
} | ||
} else { | ||
res.status(500).json({ error: { code: 'unknown_empty_error' } }); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not the upstream attribute directly instead of the additional level of nesting:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could see three directions:
additional_properties
, like I built. In that case the error object stays consistent and we don't have to add all sorts of properties to it down the road. It's less pretty, but is also easier to deal with. And since it's aRecord<string,unknown>
it's easier to change going forward because it has to have nested properties.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 6b665e0, but also waiting for feedback from @bodinsamuel and @khaliqgant and it's an easy thing to revert if we decide to go a different direction.