fix(core): Map NodeOperationErrors to BadRequestError in /resource-locator-results #9073
Conversation
…cator-results UI calls the /dynamic-node-parameters/resource-locator-results endpoint to resolve resources to show in the NDV resource picker. For example the user can select an n8n workflow from a list in the n8n node. However, if the credentials are invalid or missing, the resolution will throw a NodeOperationError. This results in HTTP 500 response from the endpoint. Since this is an expected error due to invalid configuration, it makes more sense to respond with 400 Bad Request.
n8n-assistant
bot
added
core
| if (error instanceof NodeOperationError) { | ||
| throw new BadRequestError(error.message); | ||
| } |
There was a problem hiding this comment.
Can this throw a NodeOperationError unrelated to invalid credentials? Is it safe to assume this mapping is always true?
There was a problem hiding this comment.
That seems to depend on the Node that is being used. In case of the n8n-nodes-base.n8n, it ends up in NodeExecuteFunctions.getCredentials, which does throw a NodeOperationError if the creds have not been defined. What's the convention of using errors in the nodes code? Do you think we should instead inherit a new Error class from NodeOperationError for credentials and catch only that?
There was a problem hiding this comment.
Tbh I don't know what the conventions are either - @michael-radency what would you suggest?
There was a problem hiding this comment.
for nodes if error comes from external api request it's in most cases would be NodeApiError, if error happened during operation, like invalid json parse, it would be NodeOperationError
so in case of invalid credentials it probably should be NodeApiError, but there are exceptions
not sure why we need to add a new error class as BadRequestError is a specialized wrapper to throw error with status code 400
There was a problem hiding this comment.
So would it make sense then to change the NodeExecuteFunctions.getCredentials to throw NodeApiError instead? Even tho the error is not coming from an external API. I wouldn't change it to BadRequestError as that wouldn't make sense conceptually in a function like getCredentials.
There was a problem hiding this comment.
maybe better to add errorCode optional parameter to NodeOperationErrorOptions that would set error code on NodeOperationError
|
I'm gonna close this since this is not a must have and the implications of this change are not clear |
Summary
While investigating https://linear.app/n8n/issue/ADO-1361/dynamic-options-fail-to-load-if-the-node-contains-an-expression-in-an I noticed this:
UI calls the /dynamic-node-parameters/resource-locator-results endpoint to resolve resources to show in the NDV resource picker. For example the user can select an n8n workflow from a list in the n8n node. However, if the credentials are invalid or missing, the resolution will throw a NodeOperationError. This results in HTTP 500 response from the endpoint. Since this is an expected error due to invalid configuration, it makes more sense to respond with 400 Bad Request.
Related tickets and issues
https://linear.app/n8n/issue/ADO-1361/dynamic-options-fail-to-load-if-the-node-contains-an-expression-in-an
Review / Merge checklist
(no-changelog)otherwise. (conventions)