ruanwenjun commented on code in PR #17711:
URL:
https://github.com/apache/dolphinscheduler/pull/17711#discussion_r2559361534
##########
dolphinscheduler-service/src/main/java/org/apache/dolphinscheduler/service/expand/CuringParamsServiceImpl.java:
##########
@@ -160,107 +161,177 @@ public Map<String, Property>
parseWorkflowFatherParam(@Nullable Map<String, Stri
}
/**
- * Generate prepare params include project params, global parameters,
local parameters, built-in parameters, varpool, start-up params.
- * <p> The priority of the parameters is as follows:
- * <p> varpool > command parameters > local parameters > global parameters
> project parameters > built-in parameters
- * todo: Use TaskRuntimeParams to represent this.
+ * Prepares the final map of task execution parameters by merging
parameters from multiple sources
+ * in a well-defined priority order. The resulting map is guaranteed to
contain only valid entries:
+ * <ul>
+ * <li>Keys are non-null and non-blank strings</li>
+ * <li>Values are non-null {@link Property} objects</li>
+ * </ul>
*
- * @param taskInstance
- * @param parameters
- * @param workflowInstance
- * @param projectName
- * @param workflowDefinitionName
- * @return
+ * <p><strong>Parameter Precedence (highest to lowest):</strong>
+ * <ol>
+ * <li>Business/scheduling time parameters (e.g., {@code
${system.datetime}})</li>
+ * <li>Command-line or runtime complement parameters</li>
+ * <li>Task-local parameters</li>
+ * <li>Workflow global parameters (solidified at instance creation)</li>
+ * <li>Project-level parameters</li>
+ * <li>Built-in system parameters (e.g., {@code ${task.id}})</li>
+ * </ol>
+ *
+ * <p><strong>Important Notes:</strong>
+ * <ul>
+ * <li>All parameter sources are sanitized via {@link #safePutAll(Map,
Map)} to prevent {@code null}
+ * or blank keys, which would cause JSON serialization failures
(e.g., Jackson's
+ * "Null key for a Map not allowed in JSON").</li>
+ * <li>Placeholders (e.g., {@code "${var}"}) in parameter values are
resolved after all sources
+ * are merged, using the consolidated parameter map. Global
parameters are already
+ * <em>solidified</em> (fully resolved at workflow instance
creation), so no recursive
+ * placeholder expansion is required.</li>
+ * <li>{@code VarPool} values (from upstream tasks) only override
parameters marked as
+ * {@link Direct#IN}; output or constant parameters remain
unchanged.</li>
+ * </ul>
+ *
+ * @param taskInstance the current task instance (must not be
null)
+ * @param parameters the parsed task-specific parameters (must
not be null)
+ * @param workflowInstance the parent workflow instance (must not be
null)
+ * @param projectName name of the project containing the
workflow
+ * @param workflowDefinitionName name of the workflow definition
+ * @return a safe, fully resolved map of parameter name to {@link
Property}, ready for task execution
*/
@Override
- public Map<String, Property> paramParsingPreparation(@NonNull TaskInstance
taskInstance,
+ public Map<String, Property> paramParsingPreparation(
+ @NonNull TaskInstance
taskInstance,
@NonNull
AbstractParameters parameters,
@NonNull
WorkflowInstance workflowInstance,
String projectName,
String
workflowDefinitionName) {
- Map<String, Property> prepareParamsMap = new HashMap<>();
- // assign value to definedParams here
- Map<String, Property> globalParams =
parseGlobalParamsMap(workflowInstance);
-
- // combining local and global parameters
- Map<String, Property> localParams =
parameters.getInputLocalParametersMap();
-
- // stream pass params
- List<Property> varPools = parseVarPool(taskInstance);
+ Map<String, Property> prepareParamsMap = new HashMap<>();
Review Comment:
Revert the unrelated changes.
##########
dolphinscheduler-service/src/main/java/org/apache/dolphinscheduler/service/expand/CuringParamsServiceImpl.java:
##########
@@ -160,107 +161,177 @@ public Map<String, Property>
parseWorkflowFatherParam(@Nullable Map<String, Stri
}
/**
- * Generate prepare params include project params, global parameters,
local parameters, built-in parameters, varpool, start-up params.
- * <p> The priority of the parameters is as follows:
- * <p> varpool > command parameters > local parameters > global parameters
> project parameters > built-in parameters
- * todo: Use TaskRuntimeParams to represent this.
+ * Prepares the final map of task execution parameters by merging
parameters from multiple sources
+ * in a well-defined priority order. The resulting map is guaranteed to
contain only valid entries:
+ * <ul>
+ * <li>Keys are non-null and non-blank strings</li>
+ * <li>Values are non-null {@link Property} objects</li>
+ * </ul>
*
- * @param taskInstance
- * @param parameters
- * @param workflowInstance
- * @param projectName
- * @param workflowDefinitionName
- * @return
+ * <p><strong>Parameter Precedence (highest to lowest):</strong>
+ * <ol>
+ * <li>Business/scheduling time parameters (e.g., {@code
${system.datetime}})</li>
+ * <li>Command-line or runtime complement parameters</li>
+ * <li>Task-local parameters</li>
+ * <li>Workflow global parameters (solidified at instance creation)</li>
+ * <li>Project-level parameters</li>
+ * <li>Built-in system parameters (e.g., {@code ${task.id}})</li>
+ * </ol>
+ *
+ * <p><strong>Important Notes:</strong>
+ * <ul>
+ * <li>All parameter sources are sanitized via {@link #safePutAll(Map,
Map)} to prevent {@code null}
+ * or blank keys, which would cause JSON serialization failures
(e.g., Jackson's
+ * "Null key for a Map not allowed in JSON").</li>
+ * <li>Placeholders (e.g., {@code "${var}"}) in parameter values are
resolved after all sources
+ * are merged, using the consolidated parameter map. Global
parameters are already
+ * <em>solidified</em> (fully resolved at workflow instance
creation), so no recursive
+ * placeholder expansion is required.</li>
+ * <li>{@code VarPool} values (from upstream tasks) only override
parameters marked as
+ * {@link Direct#IN}; output or constant parameters remain
unchanged.</li>
+ * </ul>
+ *
+ * @param taskInstance the current task instance (must not be
null)
+ * @param parameters the parsed task-specific parameters (must
not be null)
+ * @param workflowInstance the parent workflow instance (must not be
null)
+ * @param projectName name of the project containing the
workflow
+ * @param workflowDefinitionName name of the workflow definition
+ * @return a safe, fully resolved map of parameter name to {@link
Property}, ready for task execution
*/
@Override
- public Map<String, Property> paramParsingPreparation(@NonNull TaskInstance
taskInstance,
+ public Map<String, Property> paramParsingPreparation(
+ @NonNull TaskInstance
taskInstance,
@NonNull
AbstractParameters parameters,
@NonNull
WorkflowInstance workflowInstance,
String projectName,
String
workflowDefinitionName) {
- Map<String, Property> prepareParamsMap = new HashMap<>();
- // assign value to definedParams here
- Map<String, Property> globalParams =
parseGlobalParamsMap(workflowInstance);
-
- // combining local and global parameters
- Map<String, Property> localParams =
parameters.getInputLocalParametersMap();
-
- // stream pass params
- List<Property> varPools = parseVarPool(taskInstance);
+ Map<String, Property> prepareParamsMap = new HashMap<>();
- // if it is a complement,
- // you need to pass in the task instance id to locate the time
- // of the process instance complement
ICommandParam commandParam =
JSONUtils.parseObject(workflowInstance.getCommandParam(), ICommandParam.class);
- String timeZone = commandParam.getTimeZone();
+ String timeZone = commandParam != null ? commandParam.getTimeZone() :
null;
Review Comment:
`commandParam` shouldn't be null.
##########
dolphinscheduler-service/src/main/java/org/apache/dolphinscheduler/service/expand/CuringParamsServiceImpl.java:
##########
@@ -160,107 +161,177 @@ public Map<String, Property>
parseWorkflowFatherParam(@Nullable Map<String, Stri
}
/**
- * Generate prepare params include project params, global parameters,
local parameters, built-in parameters, varpool, start-up params.
- * <p> The priority of the parameters is as follows:
- * <p> varpool > command parameters > local parameters > global parameters
> project parameters > built-in parameters
- * todo: Use TaskRuntimeParams to represent this.
+ * Prepares the final map of task execution parameters by merging
parameters from multiple sources
+ * in a well-defined priority order. The resulting map is guaranteed to
contain only valid entries:
+ * <ul>
+ * <li>Keys are non-null and non-blank strings</li>
+ * <li>Values are non-null {@link Property} objects</li>
+ * </ul>
*
- * @param taskInstance
- * @param parameters
- * @param workflowInstance
- * @param projectName
- * @param workflowDefinitionName
- * @return
+ * <p><strong>Parameter Precedence (highest to lowest):</strong>
+ * <ol>
+ * <li>Business/scheduling time parameters (e.g., {@code
${system.datetime}})</li>
+ * <li>Command-line or runtime complement parameters</li>
+ * <li>Task-local parameters</li>
+ * <li>Workflow global parameters (solidified at instance creation)</li>
+ * <li>Project-level parameters</li>
+ * <li>Built-in system parameters (e.g., {@code ${task.id}})</li>
+ * </ol>
+ *
+ * <p><strong>Important Notes:</strong>
+ * <ul>
+ * <li>All parameter sources are sanitized via {@link #safePutAll(Map,
Map)} to prevent {@code null}
+ * or blank keys, which would cause JSON serialization failures
(e.g., Jackson's
+ * "Null key for a Map not allowed in JSON").</li>
+ * <li>Placeholders (e.g., {@code "${var}"}) in parameter values are
resolved after all sources
+ * are merged, using the consolidated parameter map. Global
parameters are already
+ * <em>solidified</em> (fully resolved at workflow instance
creation), so no recursive
+ * placeholder expansion is required.</li>
+ * <li>{@code VarPool} values (from upstream tasks) only override
parameters marked as
+ * {@link Direct#IN}; output or constant parameters remain
unchanged.</li>
+ * </ul>
+ *
+ * @param taskInstance the current task instance (must not be
null)
+ * @param parameters the parsed task-specific parameters (must
not be null)
+ * @param workflowInstance the parent workflow instance (must not be
null)
+ * @param projectName name of the project containing the
workflow
+ * @param workflowDefinitionName name of the workflow definition
+ * @return a safe, fully resolved map of parameter name to {@link
Property}, ready for task execution
*/
@Override
- public Map<String, Property> paramParsingPreparation(@NonNull TaskInstance
taskInstance,
+ public Map<String, Property> paramParsingPreparation(
+ @NonNull TaskInstance
taskInstance,
@NonNull
AbstractParameters parameters,
@NonNull
WorkflowInstance workflowInstance,
String projectName,
String
workflowDefinitionName) {
- Map<String, Property> prepareParamsMap = new HashMap<>();
- // assign value to definedParams here
- Map<String, Property> globalParams =
parseGlobalParamsMap(workflowInstance);
-
- // combining local and global parameters
- Map<String, Property> localParams =
parameters.getInputLocalParametersMap();
-
- // stream pass params
- List<Property> varPools = parseVarPool(taskInstance);
+ Map<String, Property> prepareParamsMap = new HashMap<>();
- // if it is a complement,
- // you need to pass in the task instance id to locate the time
- // of the process instance complement
ICommandParam commandParam =
JSONUtils.parseObject(workflowInstance.getCommandParam(), ICommandParam.class);
- String timeZone = commandParam.getTimeZone();
+ String timeZone = commandParam != null ? commandParam.getTimeZone() :
null;
- // built-in params
- Map<String, String> builtInParams =
- setBuiltInParamsMap(taskInstance, workflowInstance, timeZone,
projectName, workflowDefinitionName);
+ // 1. Built-in parameters (lowest precedence)
+ Map<String, String> builtInParams = setBuiltInParamsMap(
+ taskInstance, workflowInstance, timeZone, projectName,
workflowDefinitionName);
+ safePutAll(prepareParamsMap,
ParameterUtils.getUserDefParamsMap(builtInParams));
- // project-level params
+ // 2. Project-level parameters
Map<String, Property> projectParams =
getProjectParameterMap(taskInstance.getProjectCode());
+ safePutAll(prepareParamsMap, projectParams);
- if (MapUtils.isNotEmpty(builtInParams)) {
-
prepareParamsMap.putAll(ParameterUtils.getUserDefParamsMap(builtInParams));
- }
-
- if (MapUtils.isNotEmpty(projectParams)) {
- prepareParamsMap.putAll(projectParams);
- }
-
- if (MapUtils.isNotEmpty(globalParams)) {
- prepareParamsMap.putAll(globalParams);
- }
-
- if (MapUtils.isNotEmpty(localParams)) {
- prepareParamsMap.putAll(localParams);
- }
+ // 3. Workflow global parameters
+ Map<String, Property> globalParams =
parseGlobalParamsMap(workflowInstance);
+ safePutAll(prepareParamsMap, globalParams);
- if (CollectionUtils.isNotEmpty(commandParam.getCommandParams())) {
- prepareParamsMap.putAll(commandParam.getCommandParams().stream()
- .collect(Collectors.toMap(Property::getProp,
Function.identity())));
+ // 4. Task-local parameters
+ Map<String, Property> localParams =
parameters.getInputLocalParametersMap();
+ safePutAll(prepareParamsMap, localParams);
+
+ // 5. Command-line / complement parameters
+ if (commandParam != null &&
CollectionUtils.isNotEmpty(commandParam.getCommandParams())) {
+ Map<String, Property> commandParamsMap =
commandParam.getCommandParams().stream()
+ .filter(prop -> StringUtils.isNotBlank(prop.getProp()))
+ .collect(Collectors.toMap(
+ Property::getProp,
+ Function.identity(),
+ (v1, v2) -> v2 // retain last on duplicate key
+ ));
+ safePutAll(prepareParamsMap, commandParamsMap);
}
+ // 6. VarPool: override values only for existing IN-direction
parameters
+ List<Property> varPools = parseVarPool(taskInstance);
if (CollectionUtils.isNotEmpty(varPools)) {
- // overwrite the in parameter by varPool
for (Property varPool : varPools) {
- Property property = prepareParamsMap.get(varPool.getProp());
- if (property == null || property.getDirect() != Direct.IN) {
+ if (StringUtils.isBlank(varPool.getProp())) {
continue;
}
- property.setValue(varPool.getValue());
+ Property targetParam = prepareParamsMap.get(varPool.getProp());
+ if (targetParam != null &&
Direct.IN.equals(targetParam.getDirect())) {
+ targetParam.setValue(varPool.getValue());
+ }
}
}
- Iterator<Map.Entry<String, Property>> iter =
prepareParamsMap.entrySet().iterator();
- while (iter.hasNext()) {
- Map.Entry<String, Property> en = iter.next();
- Property property = en.getValue();
-
- if (StringUtils.isNotEmpty(property.getValue())
- &&
property.getValue().contains(Constants.FUNCTION_START_WITH)) {
- /**
- * local parameter refers to global parameter with the same
name
- * note: the global parameters of the process instance here
are solidified parameters,
- * and there are no variables in them.
- */
- String val = property.getValue();
-
- // handle some chain parameter assign, such as `{"var1":
"${var2}", "var2": 1}` should be convert to
- // `{"var1": 1, "var2": 1}`
- val = convertParameterPlaceholders(val, prepareParamsMap);
- property.setValue(val);
- }
+ // 7. Resolve placeholders (e.g., "${output_dir}") using the current
parameter context
+ resolvePlaceholders(prepareParamsMap);
Review Comment:
`preBuildBusinessParams` should before `resolvePlaceholders`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
