feat(Execute Workflow Node): Move Type Conversion functionality to ResourceMapper #12268
Conversation
n8n-assistant
bot
added
core
packages/editor-ui/src/components/ResourceMapper/ResourceMapper.vue
Outdated
Show resolved
Hide resolved
| @@ -2729,6 +2730,9 @@ export type ResourceMapperValue = { | |||
| value: { [key: string]: string | number | boolean | null } | null; | |||
| matchingColumns: string[]; | |||
| schema: ResourceMapperField[]; | |||
| ignoreTypeMismatchErrors: boolean; | |||
There was a problem hiding this comment.
Can you update our RMC documentation to include these new options once we merge this? I will also update the doc to include the new localResourceMapping method
…o resourceMapperTypeConversion
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
| @@ -1,3 +1,4 @@ | |||
| import { loadWorkflowInputMappings } from 'n8n-nodes-base/dist/utils/workflowInputsResourceMapping/GenericFunctions'; | |||
There was a problem hiding this comment.
My main reason for keeping these separate is for us to be able to customize them for the two nodes. Now realizing that they should behave the same and there is a bit of logic there to maintain to makes sense to unify it.
There was a problem hiding this comment.
Cool, I probably should have asked first, this just got caught up in the large merge conflict in general :D If we do modify behavior in the future it should probably still call a shared util in the end, so let's go with this 👍
Summary
Move
ignoreTypeMismatchErrorsandattemptToConvertTypesto the resourceMapper. This enables us to drop type checking in both node implementations, which kicked of some included refactoring.The ugly part here is the inclusion of "constant"
showTypeConversionOptionsfield in the resourceMapper schema, which indicates to parse strings rather than accept anything [1], which I believe to be much saner behavior.I had to introduce this property to avoid changing existing use cases of the resourceMapper. Any mapper adopting the type options added here will also adopt this stricter string conversion automatically.
One change to call out here compared to the feature branch is that we're adopting resourceMapper's behavior of silently changing
nulltoundefinedwhen type-checking (though the sub-workflow trigger then replaces missing/undefined fields withnull).Previously (on the feature branch) we carried
nullandundefinedover explicitly to support future default values forundefined, whereasnullwould be kept. However I suggest we keep the resourceMappper's behavior here in an attempt to stay in line. The implementation would be trivial in case the opposite is preferred.Also includes an unrelated notice about node version for ExecuteWorkflow V1 and V1.1.
[1] I.e. resourceMappers elsewhere would accept
7as a string and keep it a number, whereas new mappers would turn it into"7"under the hood.Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/ADO-3017/feature-add-type-conversion-options-to-the-resourcemapper
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)