[Fix-18389][DataX] Read job definition from attached resource file when custom json is empty - #18434
[Fix-18389][DataX] Read job definition from attached resource file when custom json is empty#18434nikhiln64 wants to merge 6 commits into
Conversation
|
Thanks for opening this pull request! Please check out our contributing guidelines. (https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/contribute/join/pull-request.md) |
SbloodyS
left a comment
There was a problem hiding this comment.
The resource-file fallback is not reached for the workflow reported in #18389.
The new logic reads the resource only when StringUtils.isEmpty(dataXParameters.getJson()) is true. However, the issue’s actual task parameters contain:
"customConfig": 1,
"json": "{}",
"resourceList": [...]Since "{}" is not empty, the task will continue writing {} to the generated DataX job file instead of reading the attached resource.
The current UI also requires the JSON field to be non-empty even when a resource is selected, so the resource-only case covered by DataxParametersTest cannot be created through the UI.
Please make the UI and worker follow consistent source-selection rules, handle existing tasks that use {} as the placeholder, and add a DataxTask-level regression test using a real resource file and ResourceContext. The current test only validates checkParameters() and does not verify that the resource content is written to the generated job file.
|
Thank you for the careful review @SbloodyS, all three points are addressed and pushed. The worker now treats the UI placeholder {} the same as an empty json field through a shared isInlineJsonAbsent check, so the workflow from the issue reaches the resource fallback. checkParameters follows the same rule, meaning {} plus an attached resource is valid while {} alone is still rejected. The UI json field validation in use-datax.ts now accepts an empty json editor when a resource file is selected, so the UI and worker follow the same source selection rules and the resource only case can actually be created through the UI. And there is a new DataxTask level regression test that runs handle() with customConfig, json set to {} and a real temp resource file wired through ResourceContext, asserting the generated job file contains the resource content rather than the placeholder. The full module suite passes on JDK 8 with spotless applied. The fresh CI run will need an approval when you have a moment. |
|
The UI and worker validation are still inconsistent for the
The exact string comparison also does not recognize semantically equivalent empty objects such as Please determine whether the inline JSON is an empty object semantically instead of comparing it with the literal |
|
Thanks @SbloodyS, all four points are addressed and pushed, and the branch is rebased onto the latest dev. The check is now centralized in a single DataxParameters.isInlineJsonAbsent() method that both checkParameters and DataxTask.buildDataxJsonFile call, so the duplicated literal comparison is gone. It treats the inline json as absent when it is blank or when it parses to a JSON object with no fields, so {}, { } and a formatted multi line empty object are all recognized, while malformed json still counts as present so the existing format validation reports it. The UI validator in use-datax.ts applies the same semantic rule. A parsed empty object without a resource file is now rejected with the same message as an empty editor, and it is accepted when a resource file is selected, so the UI and the worker agree on every combination. DataxParametersTest now covers null, blank, {}, { } and a formatted empty object, each both with and without a resource, plus the malformed json case and the inline definition winning when both are present. The task level test now feeds a formatted empty object through the real ResourceContext path and asserts the generated job file carries the resource content. The full module suite passes locally, 12 tests across DataxTaskTest and DataxParametersTest. |
SbloodyS
left a comment
There was a problem hiding this comment.
Do not treat the first generic resource as the DataX job definition.
resourceList is a multi-select field and is already used for auxiliary files such as Kerberos keytabs and XML configuration files. However, when inline JSON is absent, the new validation accepts any non-empty resource list, and the worker blindly reads resourceList.get(0) as the DataX job definition.
For example, if a task attaches [keytab, job.json], or if the resource selection order changes, the worker will read the keytab/XML file as the DataX JSON. A task containing only auxiliary resources can also pass validation even though no job definition was provided.
Please explicitly identify the job-definition resource, or enforce an unambiguous contract such as exactly one designated JSON resource when inline JSON is absent. Auxiliary resources should remain separate. Please also add test coverage for multiple resources and for a non-JSON resource appearing before the job file.
|
Good catch, thank you, you are right that reading resourceList.get(0) is unsafe. resourceList is multi select and holds auxiliary files like keytabs and xml, so a task with keytab and job.json would have read the keytab as the DataX json, and a task carrying only auxiliary resources passed validation with no job definition at all. I have changed the contract to identify the job definition as the single resource whose name ends with .json, in a new DataxParameters.getJobDefinitionResource, rather than the first entry in the list. checkParameters now rejects the cases where there is no json resource and where there is more than one json resource, so a keytab only task and an ambiguous two json task are both invalid, and auxiliary resources stay separate. The worker reads that designated resource and throws a clear error if it is absent rather than falling back to a wrong file. For coverage I added a DataxParametersTest case for a keytab before the job file, two json resources, and an auxiliary only list, and a DataxTaskTest that attaches keytab and job.json and asserts the generated job file carries the job.json content and not the keytab. The datax module tests pass on Java 8 and the branch is rebased onto the latest dev. Let me know if you would prefer a stricter contract than the single .json convention, for example an explicit designated field. |
SbloodyS
left a comment
There was a problem hiding this comment.
Validate the designated JSON resource consistently in the UI
dolphinscheduler-ui/src/views/projects/task/components/node/fields/use-datax.ts:202-228
The backend now requires exactly one resource whose name ends with .json when the inline definition is absent. However, the UI still accepts any non-empty resourceList.
Therefore, configurations containing only a keytab/XML resource, or containing multiple JSON resources, pass UI validation but are rejected during worker initialization with the generic datax task params is not valid error.
Please apply the same rule in the UI: when inline JSON is absent, require exactly one .json resource. Please also add UI validator coverage for auxiliary-only, one JSON with auxiliary resources, and multiple-JSON cases.
|
Good point, the UI should not accept a config the worker will reject. I applied the same rule in the json validator in use-datax.ts. When the inline json is empty it now requires exactly one resource whose name ends with .json, using the same fullName list the resource picker already binds to model.resourceList. An auxiliary only selection like a lone keytab, and a selection with more than one json, both fail validation now, while one json with auxiliary files alongside it passes, which matches DataxParameters.getJobDefinitionResource on the backend. I also added a dedicated message key datax_custom_json_resource_tips in the en and zh locales instead of the old generic empty tip, so the reason is clear in the form. On the validator test coverage, I looked for a place to add it and the ui module has no frontend unit test harness at the moment. There is no vitest or jest config, no test script in package.json, and no .test.ts or .spec.ts under dolphinscheduler-ui/src, so validator behavior is not unit testable here the way the backend is, it is exercised through the selenium e2e suite. I did not want to pull a whole test framework into this fix. I ran the project prettier 2.7.1 over the three changed files and they pass. Happy to add e2e coverage for the datax resource cases in a follow up, or to wire up a validator unit test harness separately if you would like that as its own change. |
SbloodyS
left a comment
There was a problem hiding this comment.
Treat whitespace-only inline JSON as absent in the UI
DataxParameters.isInlineJsonAbsent() uses StringUtils.isBlank(), so whitespace-only values such as " " or newlines are treated as absent and correctly fall back to the attached JSON resource.
However, the UI only treats "", undefined, and null as absent. A whitespace-only value reaches utils.isJson() and is rejected even when exactly one valid .json resource is attached. Therefore, a configuration accepted by the backend cannot be submitted through the UI.
Please trim/check blank input before JSON validation and apply the resource fallback rule consistently. For example, derive inlineJsonAbsent from model.json == null || model.json.trim() === '' before calling utils.isJson().
…en custom json is empty
…ign UI validation, add task-level regression test
…the check in DataxParameters
…source, not resourceList.get(0) resourceList is multi-select and also carries auxiliary files such as Kerberos keytabs and xml configs. Taking resourceList.get(0) as the DataX job definition could read a keytab as the job when it is listed first, and a task carrying only auxiliary resources passed validation with no job definition at all. The job definition is now identified as the single resource whose name ends with .json. Validation rejects the no-json and ambiguous multi-json cases, and the worker reads that designated resource, failing loudly if it is absent. Added coverage for a keytab before the job file, multiple json resources, and an auxiliary-only list.
…ator when inline json is absent The backend now identifies the datax job definition as the single .json resource in resourceList and rejects the no-json and ambiguous multi-json cases. The UI validator still accepted any non-empty resourceList, so an auxiliary-only or multi-json config passed UI validation and was only rejected later at worker init with a generic error. Apply the same rule in the UI json validator: when the inline json is absent, require exactly one resource whose name ends with .json. Add an explicit message key in the en and zh locales.
|
Good catch, you are right that the UI was stricter than the backend here. The absence check only treated empty, undefined and null as absent, so a whitespace only value reached utils.isJson and was rejected even with exactly one valid .json resource attached, which the worker would have accepted. I now derive inlineJsonAbsent from model.json being undefined or null or trimming to empty, before the json validation, so a blank inline json falls back to the attached resource the same way the backend isInlineJsonAbsent does with StringUtils.isBlank. Pushed. |
Was this PR generated or assisted by AI?
YES. The implementation and tests were written with AI assistance (Claude Code), reviewed and driven by a human. The root cause analysis was posted on the issue before starting.
Purpose of the pull request
Fixes #18389. When a DataX task uses custom config with the job definition attached as a resource file, the worker downloads the resource but the plugin never reads it and runs with the empty inline json, so the job fails. This PR makes the plugin read the job definition from the first attached resource file whenever the inline json is empty, mirroring the pattern the SeaTunnel task plugin already uses through ResourceContext.
Brief change log
Verify this pull request
This change added tests and can be verified as follows: