lasdf1234 commented on code in PR #13134:
URL: https://github.com/apache/gravitino/pull/13134#discussion_r4024025480
##########
maintenance/jobs/src/main/java/org/apache/gravitino/maintenance/jobs/iceberg/IcebergUpdateStatsAndMetricsJob.java:
##########
@@ -391,15 +400,22 @@ static Map<String, String> parseArguments(String[] args) {
@VisibleForTesting
static Map<String, String> parseCustomSparkConfigs(String sparkConfJson) {
Review Comment:
Thanks for your review.
Moved omitUnresolvedTemplateMap to the private section at the end of
JobManager, per the class member ordering rule.
##########
core/src/test/java/org/apache/gravitino/job/TestJobTemplate.java:
##########
@@ -458,4 +458,415 @@ public void
testCreateSparkRuntimeJobTemplateWithReplacements() throws IOExcepti
.collect(Collectors.toList());
Assertions.assertTrue(archiveNames.contains(archive1.getName()));
}
+
+ @Test
+ public void testOmitEmptyArguments() {
+ // Explicit "" after --updater-options is kept; name-matched --options
{{options}} is dropped;
+ // bare {{stream_results}} / {{slices}} are kept so missing positionals
stay visible.
+ Assertions.assertEquals(
+ Lists.newArrayList(
+ "--catalog",
+ "iceberg",
+ "--table",
+ "db.t",
+ "--updater-options",
+ "",
+ "--spark-conf",
+ "{\"k\":\"v\"}",
+ "{{stream_results}}",
+ " "),
+ JobManager.omitEmptyArguments(
+ Lists.newArrayList(
+ "--catalog",
+ "iceberg",
+ "--table",
+ "db.t",
+ "--updater-options",
+ "",
+ "--spark-conf",
+ "{\"k\":\"v\"}",
+ "--options",
+ "{{options}}",
+ "{{stream_results}}",
+ " ")));
+
+ Assertions.assertEquals(
+ Lists.newArrayList("{{slices}}"),
+ JobManager.omitEmptyArguments(Lists.newArrayList("{{slices}}")));
+
+ // Boolean flag + unrelated placeholder must not be dropped as a pair.
+ Assertions.assertEquals(
+ Lists.newArrayList("--verbose", "{{unset_flag}}"),
+ JobManager.omitEmptyArguments(Lists.newArrayList("--verbose",
"{{unset_flag}}")));
+
+ // Hyphen / underscore names still match for built-in optional flags.
+ Assertions.assertEquals(
+ Lists.newArrayList(),
+ JobManager.omitEmptyArguments(
+ Lists.newArrayList("--updater-options", "{{updater_options}}")));
+
+ Assertions.assertEquals(
+ Lists.newArrayList("--catalog", "iceberg", "--stream-results"),
+ JobManager.omitEmptyArguments(
+ Lists.newArrayList("--catalog", "iceberg", "--stream-results")));
+
+ Assertions.assertThrows(NullPointerException.class, () ->
JobManager.omitEmptyArguments(null));
+ Assertions.assertEquals(
+ Lists.newArrayList(),
JobManager.omitEmptyArguments(Lists.newArrayList()));
+ }
+
+ @Test
+ public void testCreateSparkRuntimeJobTemplateOmitsUnresolvedEnvironments()
throws IOException {
+ File executable = Files.createTempFile(tempDir.toPath(), "testSparkJob",
".jar").toFile();
+ SparkJobTemplate sparkJobTemplate =
+ SparkJobTemplate.builder()
+ .withName("testSparkJobAuthEnv")
+ .withExecutable(executable.toURI().toString())
+ .withClassName("org.apache.gravitino.TestSparkJob")
+ .withEnvironments(
+ ImmutableMap.of(
+ "GRAVITINO_AUTH_TYPE",
+ "{{gravitino_auth_type}}",
+ "GRAVITINO_AUTH_USERNAME",
+ "{{gravitino_auth_username}}",
+ "KEEP_ME",
+ "literal"))
+ .build();
+
+ JobTemplateEntity entity =
+ JobTemplateEntity.builder()
+ .withId(2L)
+ .withName(sparkJobTemplate.name())
+ .withNamespace(NamespaceUtil.ofJobTemplate("test"))
+ .withTemplateContent(
+
JobTemplateEntity.TemplateContent.fromJobTemplate(sparkJobTemplate))
+ .withAuditInfo(
+
AuditInfo.builder().withCreator("test").withCreateTime(Instant.now()).build())
+ .build();
+
+ JobTemplate omitted =
+ JobManager.createRuntimeJobTemplate(entity, ImmutableMap.of(),
tempStagingDir);
+ Assertions.assertEquals(ImmutableMap.of("KEEP_ME", "literal"),
omitted.environments());
+
+ JobTemplate withEmptyAuthType =
+ JobManager.createRuntimeJobTemplate(
+ entity, ImmutableMap.of("gravitino_auth_type", ""),
tempStagingDir);
+ Assertions.assertEquals(
+ ImmutableMap.of("GRAVITINO_AUTH_TYPE", "", "KEEP_ME", "literal"),
+ withEmptyAuthType.environments());
+
+ JobTemplate resolved =
+ JobManager.createRuntimeJobTemplate(
+ entity,
+ ImmutableMap.of(
+ "gravitino_auth_type", "basic",
+ "gravitino_auth_username", "admin"),
+ tempStagingDir);
+ Assertions.assertEquals(
+ ImmutableMap.of(
+ "GRAVITINO_AUTH_TYPE",
+ "basic",
+ "GRAVITINO_AUTH_USERNAME",
+ "admin",
+ "KEEP_ME",
+ "literal"),
+ resolved.environments());
+ }
+
+ @Test
+ public void testCreateSparkRuntimeJobTemplateOmitsUnresolvedConfigs() throws
IOException {
+ File executable = Files.createTempFile(tempDir.toPath(), "testSparkJob",
".jar").toFile();
+ SparkJobTemplate sparkJobTemplate =
+ SparkJobTemplate.builder()
+ .withName("testSparkJobConfigs")
+ .withExecutable(executable.toURI().toString())
+ .withClassName("org.apache.gravitino.TestSparkJob")
+ .withConfigs(
+ ImmutableMap.of(
+ "spark.executor.instances",
+ "{{spark_executor_instances}}",
+ "spark.executor.memory",
+ "{{spark_executor_memory}}",
+ "spark.master",
+ "local"))
+ .build();
+
+ JobTemplateEntity entity =
+ JobTemplateEntity.builder()
+ .withId(6L)
+ .withName(sparkJobTemplate.name())
+ .withNamespace(NamespaceUtil.ofJobTemplate("test"))
+ .withTemplateContent(
+
JobTemplateEntity.TemplateContent.fromJobTemplate(sparkJobTemplate))
+ .withAuditInfo(
+
AuditInfo.builder().withCreator("test").withCreateTime(Instant.now()).build())
+ .build();
+
+ JobTemplate omitted =
+ JobManager.createRuntimeJobTemplate(entity, ImmutableMap.of(),
tempStagingDir);
+ Assertions.assertEquals(
+ ImmutableMap.of("spark.master", "local"), ((SparkJobTemplate)
omitted).configs());
+
+ JobTemplate withEmptyInstances =
+ JobManager.createRuntimeJobTemplate(
+ entity, ImmutableMap.of("spark_executor_instances", ""),
tempStagingDir);
+ Assertions.assertEquals(
+ ImmutableMap.of("spark.executor.instances", "", "spark.master",
"local"),
+ ((SparkJobTemplate) withEmptyInstances).configs());
+
+ JobTemplate resolved =
+ JobManager.createRuntimeJobTemplate(
+ entity,
+ ImmutableMap.of(
+ "spark_executor_instances", "2",
+ "spark_executor_memory", "1g"),
+ tempStagingDir);
+ Assertions.assertEquals(
+ ImmutableMap.of(
+ "spark.executor.instances",
+ "2",
+ "spark.executor.memory",
+ "1g",
+ "spark.master",
+ "local"),
+ ((SparkJobTemplate) resolved).configs());
+ }
+
+ @Test
+ public void testCreateRuntimeJobTemplateFailsOnEmbeddedUnresolvedConfig()
throws IOException {
+ File executable = Files.createTempFile(tempDir.toPath(), "testSparkJob",
".jar").toFile();
+ SparkJobTemplate sparkJobTemplate =
+ SparkJobTemplate.builder()
+ .withName("testSparkJobEmbeddedConfig")
+ .withExecutable(executable.toURI().toString())
+ .withClassName("org.apache.gravitino.TestSparkJob")
+ .withConfigs(ImmutableMap.of("spark.sql.extensions",
"prefix-{{missing_ext}}"))
+ .build();
+
+ JobTemplateEntity entity =
+ JobTemplateEntity.builder()
+ .withId(7L)
+ .withName(sparkJobTemplate.name())
+ .withNamespace(NamespaceUtil.ofJobTemplate("test"))
+ .withTemplateContent(
+
JobTemplateEntity.TemplateContent.fromJobTemplate(sparkJobTemplate))
+ .withAuditInfo(
+
AuditInfo.builder().withCreator("test").withCreateTime(Instant.now()).build())
+ .build();
+
+ IllegalArgumentException thrown =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> JobManager.createRuntimeJobTemplate(entity,
ImmutableMap.of(), tempStagingDir));
+
Assertions.assertTrue(thrown.getMessage().contains("prefix-{{missing_ext}}"));
+ }
+
+ @Test
+ public void testCreateRuntimeJobTemplateFailsOnDuplicateEnvironmentKeys()
throws IOException {
+ File executable = Files.createTempFile(tempDir.toPath(), "testSparkJob",
".jar").toFile();
+ SparkJobTemplate sparkJobTemplate =
+ SparkJobTemplate.builder()
+ .withName("testSparkJobDupEnv")
+ .withExecutable(executable.toURI().toString())
+ .withClassName("org.apache.gravitino.TestSparkJob")
+ .withEnvironments(ImmutableMap.of("{{A}}", "v1", "{{B}}", "v2"))
+ .build();
+
+ JobTemplateEntity entity =
+ JobTemplateEntity.builder()
+ .withId(3L)
+ .withName(sparkJobTemplate.name())
+ .withNamespace(NamespaceUtil.ofJobTemplate("test"))
+ .withTemplateContent(
+
JobTemplateEntity.TemplateContent.fromJobTemplate(sparkJobTemplate))
+ .withAuditInfo(
+
AuditInfo.builder().withCreator("test").withCreateTime(Instant.now()).build())
+ .build();
+
+ IllegalStateException thrown =
+ Assertions.assertThrows(
+ IllegalStateException.class,
+ () ->
+ JobManager.createRuntimeJobTemplate(
+ entity, ImmutableMap.of("A", "SAME", "B", "SAME"),
tempStagingDir));
+ Assertions.assertTrue(thrown.getMessage().contains("Duplicate key SAME"));
+ }
+
+ @Test
+ public void testRejectEmbeddedUnresolvedPlaceholder() {
Review Comment:
Thanks for your review.
Added @Nullable on the reject helpers that accept null, and reduced the
redundant regex/matcher passes by matching once and branching on the result.
##########
core/src/main/java/org/apache/gravitino/job/JobManager.java:
##########
@@ -947,17 +960,143 @@ static String replacePlaceholder(String inputString,
Map<String, String> replace
String key = matcher.group(1);
String replacement = replacements.get(key);
if (replacement != null) {
- matcher.appendReplacement(result, replacement);
+ matcher.appendReplacement(result,
Matcher.quoteReplacement(replacement));
} else {
// If no replacement is found, keep the placeholder as is
- matcher.appendReplacement(result, matcher.group(0));
+ matcher.appendReplacement(result,
Matcher.quoteReplacement(matcher.group(0)));
}
}
matcher.appendTail(result);
return result.toString();
}
+ /**
+ * Drop unresolved optional {@code --flag}/value pairs after placeholder
substitution.
+ *
+ * <p>Built-in templates list optional flags as {@code --flag} + {@code
{{placeholder}}} with
+ * matching names (for example {@code --updater-options} / {@code
{{updater_options}}}). When the
+ * job conf omits that key, leaving the unresolved placeholder produces
dangling arguments such as
+ * {@code --updater-options {{updater_options}}}. This method removes a pair
only when the
+ * following token is an entire unresolved placeholder whose name matches
the flag (after
+ * normalizing {@code -} / {@code _} and case).
+ *
+ * <p>A boolean flag followed by an unrelated placeholder (for example
{@code --verbose} / {@code
+ * {{unset_flag}}}) is not treated as a pair. Bare / positional unresolved
placeholders (for
+ * example {@code {{slices}}} in SparkPi) are kept so a missing required
argument stays visible.
+ * An explicit empty string in {@code jobConf} is also kept.
+ *
+ * @param arguments arguments after {@link #replacePlaceholder(String,
Map)}; must not be null
+ * @return compacted argument list suitable for process execution; never null
+ */
+ @VisibleForTesting
+ static List<String> omitEmptyArguments(List<String> arguments) {
+ Preconditions.checkNotNull(arguments, "arguments");
+ if (arguments.isEmpty()) {
+ return arguments;
+ }
+
+ List<String> result = new ArrayList<>(arguments.size());
+ for (int i = 0; i < arguments.size(); i++) {
+ String arg = arguments.get(i);
+ if (arg.startsWith("--") && i + 1 < arguments.size()) {
+ String next = arguments.get(i + 1);
+ if (!next.startsWith("--") && isMatchingUnresolvedFlagValue(arg,
next)) {
+ i++;
+ continue;
+ }
+ }
+
+ result.add(arg);
+ }
+ return result;
+ }
+
+ /**
+ * Whether {@code value} is an unresolved {@code {{placeholder}}} whose name
matches {@code
+ * flagArg} (for example {@code --updater-options} matches {@code
{{updater_options}}}).
+ */
+ @VisibleForTesting
+ static boolean isMatchingUnresolvedFlagValue(String flagArg, String value) {
+ if (!flagArg.startsWith("--") || !isUnresolvedPlaceholder(value)) {
+ return false;
+ }
+ Matcher matcher = PLACEHOLDER_PATTERN.matcher(value);
+ if (!matcher.matches()) {
+ return false;
+ }
+ String normalizedFlag = flagArg.substring(2).replace('-',
'_').toLowerCase(Locale.ROOT);
+ String normalizedPlaceholder = matcher.group(1).replace('-',
'_').toLowerCase(Locale.ROOT);
+ return normalizedFlag.equals(normalizedPlaceholder);
+ }
+
+ /**
+ * Resolves optional template maps such as {@code environments} and Spark
{@code configs}. Entries
+ * whose keys or values are still an unresolved {@code {{placeholder}}}
after substitution are
+ * dropped so optional credentials / Spark confs do not become literal
placeholder strings.
+ *
+ * <p>Explicit empty strings from {@code jobConf} are kept. {@code
arguments} use {@link
+ * #omitEmptyArguments(List)}; {@code customFields} still keep unresolved
placeholders as literal
+ * text when the entire token is one placeholder.
+ *
+ * <p>If two entries resolve to the same key after placeholder substitution,
this method fails
+ * fast with {@link IllegalStateException}, matching {@link
Collectors#toMap}.
+ *
+ * <p>Composite values that still embed an unresolved placeholder (for
example {@code
+ * prefix-{{missing}}} or {@code {{a}}.{{b}}} with only one key supplied)
fail fast with {@link
+ * IllegalArgumentException} instead of being passed through as literal text.
+ *
+ * @param source template map before substitution
+ * @param jobConf replacement values
+ * @param context label used in error messages (for example {@code
environment} or {@code
+ * configs})
+ * @return resolved map without unresolved optional entries
+ */
+ private static Map<String, String> omitUnresolvedTemplateMap(
+ Map<String, String> source, Map<String, String> jobConf, String context)
{
+ Map<String, String> resolved = new LinkedHashMap<>();
+ for (Map.Entry<String, String> entry : source.entrySet()) {
+ String key = replacePlaceholder(entry.getKey(), jobConf);
+ String value = replacePlaceholder(entry.getValue(), jobConf);
+ if (isUnresolvedPlaceholder(key) || isUnresolvedPlaceholder(value)) {
+ continue;
+ }
+ rejectEmbeddedUnresolvedPlaceholder(key, context + " key");
+ rejectEmbeddedUnresolvedPlaceholder(value, context + " value");
+ if (resolved.containsKey(key)) {
+ throw new IllegalStateException(
+ String.format(
+ "Duplicate key %s (attempted merging values %s and %s)",
+ key, resolved.get(key), value));
+ }
+ resolved.put(key, value);
+ }
+ return resolved;
+ }
+
+ /**
+ * Whether {@code value} is exactly one unresolved {@code {{placeholder}}}
token.
+ *
+ * <p>Blank / empty strings are not unresolved placeholders: a present
{@code jobConf} key with
+ * value {@code ""} means the caller explicitly supplied an empty value.
+ */
+ @VisibleForTesting
+ static boolean isUnresolvedPlaceholder(String value) {
+ return value != null && PLACEHOLDER_PATTERN.matcher(value).matches();
+ }
+
+ @VisibleForTesting
+ static void rejectEmbeddedUnresolvedPlaceholder(String value, String
context) {
Review Comment:
Thanks for your review.
Added end-to-end createRuntimeJobTemplate failure-path coverage for the
reject call sites (executable, className, jar/file/archive, environment, config
value, customField, and script), not only the helper unit tests.
##########
core/src/main/java/org/apache/gravitino/job/JobManager.java:
##########
@@ -947,17 +960,143 @@ static String replacePlaceholder(String inputString,
Map<String, String> replace
String key = matcher.group(1);
String replacement = replacements.get(key);
if (replacement != null) {
- matcher.appendReplacement(result, replacement);
+ matcher.appendReplacement(result,
Matcher.quoteReplacement(replacement));
} else {
// If no replacement is found, keep the placeholder as is
- matcher.appendReplacement(result, matcher.group(0));
+ matcher.appendReplacement(result,
Matcher.quoteReplacement(matcher.group(0)));
}
}
matcher.appendTail(result);
return result.toString();
}
+ /**
+ * Drop unresolved optional {@code --flag}/value pairs after placeholder
substitution.
+ *
+ * <p>Built-in templates list optional flags as {@code --flag} + {@code
{{placeholder}}} with
+ * matching names (for example {@code --updater-options} / {@code
{{updater_options}}}). When the
+ * job conf omits that key, leaving the unresolved placeholder produces
dangling arguments such as
+ * {@code --updater-options {{updater_options}}}. This method removes a pair
only when the
+ * following token is an entire unresolved placeholder whose name matches
the flag (after
+ * normalizing {@code -} / {@code _} and case).
+ *
+ * <p>A boolean flag followed by an unrelated placeholder (for example
{@code --verbose} / {@code
+ * {{unset_flag}}}) is not treated as a pair. Bare / positional unresolved
placeholders (for
+ * example {@code {{slices}}} in SparkPi) are kept so a missing required
argument stays visible.
+ * An explicit empty string in {@code jobConf} is also kept.
+ *
+ * @param arguments arguments after {@link #replacePlaceholder(String,
Map)}; must not be null
+ * @return compacted argument list suitable for process execution; never null
+ */
+ @VisibleForTesting
+ static List<String> omitEmptyArguments(List<String> arguments) {
+ Preconditions.checkNotNull(arguments, "arguments");
+ if (arguments.isEmpty()) {
+ return arguments;
+ }
+
+ List<String> result = new ArrayList<>(arguments.size());
+ for (int i = 0; i < arguments.size(); i++) {
+ String arg = arguments.get(i);
+ if (arg.startsWith("--") && i + 1 < arguments.size()) {
+ String next = arguments.get(i + 1);
+ if (!next.startsWith("--") && isMatchingUnresolvedFlagValue(arg,
next)) {
+ i++;
+ continue;
+ }
+ }
+
+ result.add(arg);
+ }
+ return result;
+ }
+
+ /**
+ * Whether {@code value} is an unresolved {@code {{placeholder}}} whose name
matches {@code
+ * flagArg} (for example {@code --updater-options} matches {@code
{{updater_options}}}).
+ */
+ @VisibleForTesting
+ static boolean isMatchingUnresolvedFlagValue(String flagArg, String value) {
+ if (!flagArg.startsWith("--") || !isUnresolvedPlaceholder(value)) {
+ return false;
+ }
+ Matcher matcher = PLACEHOLDER_PATTERN.matcher(value);
+ if (!matcher.matches()) {
+ return false;
+ }
+ String normalizedFlag = flagArg.substring(2).replace('-',
'_').toLowerCase(Locale.ROOT);
+ String normalizedPlaceholder = matcher.group(1).replace('-',
'_').toLowerCase(Locale.ROOT);
+ return normalizedFlag.equals(normalizedPlaceholder);
+ }
+
+ /**
+ * Resolves optional template maps such as {@code environments} and Spark
{@code configs}. Entries
+ * whose keys or values are still an unresolved {@code {{placeholder}}}
after substitution are
+ * dropped so optional credentials / Spark confs do not become literal
placeholder strings.
+ *
+ * <p>Explicit empty strings from {@code jobConf} are kept. {@code
arguments} use {@link
+ * #omitEmptyArguments(List)}; {@code customFields} still keep unresolved
placeholders as literal
+ * text when the entire token is one placeholder.
+ *
+ * <p>If two entries resolve to the same key after placeholder substitution,
this method fails
+ * fast with {@link IllegalStateException}, matching {@link
Collectors#toMap}.
+ *
+ * <p>Composite values that still embed an unresolved placeholder (for
example {@code
+ * prefix-{{missing}}} or {@code {{a}}.{{b}}} with only one key supplied)
fail fast with {@link
+ * IllegalArgumentException} instead of being passed through as literal text.
+ *
+ * @param source template map before substitution
+ * @param jobConf replacement values
+ * @param context label used in error messages (for example {@code
environment} or {@code
+ * configs})
+ * @return resolved map without unresolved optional entries
+ */
+ private static Map<String, String> omitUnresolvedTemplateMap(
+ Map<String, String> source, Map<String, String> jobConf, String context)
{
+ Map<String, String> resolved = new LinkedHashMap<>();
+ for (Map.Entry<String, String> entry : source.entrySet()) {
+ String key = replacePlaceholder(entry.getKey(), jobConf);
+ String value = replacePlaceholder(entry.getValue(), jobConf);
+ if (isUnresolvedPlaceholder(key) || isUnresolvedPlaceholder(value)) {
+ continue;
+ }
+ rejectEmbeddedUnresolvedPlaceholder(key, context + " key");
+ rejectEmbeddedUnresolvedPlaceholder(value, context + " value");
+ if (resolved.containsKey(key)) {
+ throw new IllegalStateException(
+ String.format(
+ "Duplicate key %s (attempted merging values %s and %s)",
Review Comment:
Thanks for your review.
IcebergUpdateStatsAndMetricsJob now delegates to
IcebergJobUtils.parseArguments, so it shares the same boolean-flag /
empty-value behavior as the other Iceberg jobs.
##########
core/src/main/java/org/apache/gravitino/job/JobManager.java:
##########
@@ -947,17 +960,143 @@ static String replacePlaceholder(String inputString,
Map<String, String> replace
String key = matcher.group(1);
String replacement = replacements.get(key);
if (replacement != null) {
- matcher.appendReplacement(result, replacement);
+ matcher.appendReplacement(result,
Matcher.quoteReplacement(replacement));
} else {
// If no replacement is found, keep the placeholder as is
- matcher.appendReplacement(result, matcher.group(0));
+ matcher.appendReplacement(result,
Matcher.quoteReplacement(matcher.group(0)));
}
}
matcher.appendTail(result);
return result.toString();
}
+ /**
+ * Drop unresolved optional {@code --flag}/value pairs after placeholder
substitution.
+ *
+ * <p>Built-in templates list optional flags as {@code --flag} + {@code
{{placeholder}}} with
+ * matching names (for example {@code --updater-options} / {@code
{{updater_options}}}). When the
+ * job conf omits that key, leaving the unresolved placeholder produces
dangling arguments such as
+ * {@code --updater-options {{updater_options}}}. This method removes a pair
only when the
+ * following token is an entire unresolved placeholder whose name matches
the flag (after
+ * normalizing {@code -} / {@code _} and case).
+ *
+ * <p>A boolean flag followed by an unrelated placeholder (for example
{@code --verbose} / {@code
+ * {{unset_flag}}}) is not treated as a pair. Bare / positional unresolved
placeholders (for
+ * example {@code {{slices}}} in SparkPi) are kept so a missing required
argument stays visible.
+ * An explicit empty string in {@code jobConf} is also kept.
+ *
+ * @param arguments arguments after {@link #replacePlaceholder(String,
Map)}; must not be null
+ * @return compacted argument list suitable for process execution; never null
+ */
+ @VisibleForTesting
+ static List<String> omitEmptyArguments(List<String> arguments) {
+ Preconditions.checkNotNull(arguments, "arguments");
+ if (arguments.isEmpty()) {
+ return arguments;
+ }
+
+ List<String> result = new ArrayList<>(arguments.size());
+ for (int i = 0; i < arguments.size(); i++) {
+ String arg = arguments.get(i);
+ if (arg.startsWith("--") && i + 1 < arguments.size()) {
+ String next = arguments.get(i + 1);
+ if (!next.startsWith("--") && isMatchingUnresolvedFlagValue(arg,
next)) {
+ i++;
+ continue;
+ }
+ }
+
+ result.add(arg);
+ }
+ return result;
+ }
+
+ /**
+ * Whether {@code value} is an unresolved {@code {{placeholder}}} whose name
matches {@code
+ * flagArg} (for example {@code --updater-options} matches {@code
{{updater_options}}}).
+ */
+ @VisibleForTesting
+ static boolean isMatchingUnresolvedFlagValue(String flagArg, String value) {
+ if (!flagArg.startsWith("--") || !isUnresolvedPlaceholder(value)) {
+ return false;
+ }
+ Matcher matcher = PLACEHOLDER_PATTERN.matcher(value);
+ if (!matcher.matches()) {
+ return false;
+ }
+ String normalizedFlag = flagArg.substring(2).replace('-',
'_').toLowerCase(Locale.ROOT);
+ String normalizedPlaceholder = matcher.group(1).replace('-',
'_').toLowerCase(Locale.ROOT);
+ return normalizedFlag.equals(normalizedPlaceholder);
+ }
+
+ /**
+ * Resolves optional template maps such as {@code environments} and Spark
{@code configs}. Entries
+ * whose keys or values are still an unresolved {@code {{placeholder}}}
after substitution are
+ * dropped so optional credentials / Spark confs do not become literal
placeholder strings.
+ *
+ * <p>Explicit empty strings from {@code jobConf} are kept. {@code
arguments} use {@link
+ * #omitEmptyArguments(List)}; {@code customFields} still keep unresolved
placeholders as literal
+ * text when the entire token is one placeholder.
+ *
+ * <p>If two entries resolve to the same key after placeholder substitution,
this method fails
+ * fast with {@link IllegalStateException}, matching {@link
Collectors#toMap}.
+ *
+ * <p>Composite values that still embed an unresolved placeholder (for
example {@code
+ * prefix-{{missing}}} or {@code {{a}}.{{b}}} with only one key supplied)
fail fast with {@link
+ * IllegalArgumentException} instead of being passed through as literal text.
+ *
+ * @param source template map before substitution
+ * @param jobConf replacement values
+ * @param context label used in error messages (for example {@code
environment} or {@code
+ * configs})
+ * @return resolved map without unresolved optional entries
+ */
+ private static Map<String, String> omitUnresolvedTemplateMap(
Review Comment:
Thanks for your review.
Duplicate keys after placeholder substitution now throw
IllegalArgumentException instead of IllegalStateException, so the REST layer
returns HTTP 400 for this client input error.
--
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]
