Repository navigation
fix: misleading build error - #74
jakcinmarina wants to merge 6 commits into
Conversation
| expectLoadFailureNotice(result, 'Could not parse the input schema of actor nFJndFXA5zjCTuudP'); | ||
| expectLoadFailureNotice( | ||
| result, | ||
| 'Could not parse the input schema of actor nFJndFXA5zjCTuudP', |
There was a problem hiding this comment.
nit: "actor" missing capital letter
There was a problem hiding this comment.
Capitalised it in new strings only. I left the already-released error messages as they are, since users may match on them in their workflows.
a719a6b to
60f714d
Compare
|
I rebased onto master and had to fix an interaction with the review fixes there, so could you take another look just to be sure? @drobnikj @protoss70 |
|
@jakcinmarina can you please run https://github.com/apify/agent-skills-internal/blob/main/skills/apify-proofreader/SKILL.md skill to check content, it should fix as well the Actor capitalization 👍 |
|
@drobnikj ran it and applied fixes :) |
drobnikj
left a comment
drobnikj
left a comment
There was a problem hiding this comment.
LGTM, I have just two questions, can you check?
| ); | ||
| } | ||
| // Re-wrapping an existing NodeApiError returns it unchanged. | ||
| throw new NodeApiError(this.getNode(), error as JsonObject); |
There was a problem hiding this comment.
Question: this comment states the assumption the rest of the PR disproves — with a second n8n-workflow copy the instanceof pass-through fails, so a non-build 403 is re-wrapped here and loses its message and context again, or did I read it wrong?
There was a problem hiding this comment.
Yep, that comment was written before I found the two n8n-workflow copies and I never updated it. Here it re-wraps and loses context. I switched to toNodeApiError and removed the comment.
| * Wrap `error` in a NodeApiError unless it already is one. NodeApiError's own pass-through uses | ||
| * `instanceof`, so it would re-wrap one from n8n's copy and lose its message and `context`. | ||
| */ | ||
| function toNodeApiError(node: INode, error: any): Error { |
There was a problem hiding this comment.
Question: this isn't exported, and every operation that catches around apiRequest still ends in a bare throw new NodeApiError(this.getNode(), error) — get-run, get-actor-runs, get-user-runs-list, abort-run, datasets/get-items, scrape-single-url, web-fetch, get-last-run. Not sure if this is on purpose?
There was a problem hiding this comment.
Not on purpose. After the rebase I only fixed the wrap sites in genericFunctions.ts and missed the operations. Exported it, use it in all of them and added a test.
protoss70
left a comment
protoss70
left a comment
There was a problem hiding this comment.
Looks good, left a question
| const apifyError = error?.context?.data?.error; | ||
| if (apifyError?.type === MISSING_DEFAULT_BUILD_ERROR_TYPE) { | ||
| // The message must go in the options, or n8n replaces it with the status-code title. | ||
| const message = `Actor ${actorId} has no build for its default build tag`; |
There was a problem hiding this comment.
Question: Perhaps in the message we can link the builds page of the selected Actor as well. What do you think? Does it look ugly on the screen? Can we display it as link?
https://console.apify.com/actors/<ACT_ID>/builds
Running an Actor whose default build tag has no build (for example, a never-built Actor) failed with "Forbidden - perhaps check your credentials?". Apify answers this case with a 403, and n8n gives every 403 that title. The real reason was only in the description.
getDefaultBuildnow matches Apify's unknown-build-tag error type and throws a clear error instead:Title: "Actor … has no build for its default build tag"
Description: Apify's message, plus "Build the Actor in Apify Console, or set the Build Tag of an existing build."
Other 403s, such as real permission errors, are unchanged.
apiRequestnow also recognisesNodeApiErrorsfrom n8n's own copy of n8n-workflow and passes them on unchanged. When a second copy is installed next to the node, instanceof failed, the error was wrapped again, and Apify's response body was lost, so checks like the one above never matched. This happens when the node is linked into ~/.n8n/custom, and likely with some manual or Docker installs. Normal Community Nodes installs are unaffected, because they already took this path.closes https://github.com/orgs/apify/projects/18/views/2?pane=issue&itemId=271440647&issue=apify%7Cdx-heroes-team%7C25