You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@seatunnel.apache.org by ki...@apache.org on 2023/01/31 12:40:42 UTC
[incubator-seatunnel] branch dev updated: improve option duplicate check (#3968)
This is an automated email from the ASF dual-hosted git repository.
kirs pushed a commit to branch dev
in repository https://gitbox.apache.org/repos/asf/incubator-seatunnel.git
The following commit(s) were added to refs/heads/dev by this push:
new a37a777dc improve option duplicate check (#3968)
a37a777dc is described below
commit a37a777dc13b083ba4b785cd77a007996f7414b3
Author: Eric <ga...@gmail.com>
AuthorDate: Tue Jan 31 20:40:36 2023 +0800
improve option duplicate check (#3968)
---
.../api/configuration/util/OptionRule.java | 93 ++++++++++++++--------
.../api/configuration/util/OptionRuleTest.java | 11 ++-
2 files changed, 64 insertions(+), 40 deletions(-)
diff --git a/seatunnel-api/src/main/java/org/apache/seatunnel/api/configuration/util/OptionRule.java b/seatunnel-api/src/main/java/org/apache/seatunnel/api/configuration/util/OptionRule.java
index ee1e491f7..d9e600803 100644
--- a/seatunnel-api/src/main/java/org/apache/seatunnel/api/configuration/util/OptionRule.java
+++ b/seatunnel-api/src/main/java/org/apache/seatunnel/api/configuration/util/OptionRule.java
@@ -132,7 +132,7 @@ public class OptionRule {
*/
public Builder optional(@NonNull Option<?>... options) {
for (Option<?> option : options) {
- verifyDuplicate(option, "OptionsOption");
+ verifyOptionOptionsDuplicate(option, "OptionsOption");
}
this.optionalOptions.addAll(Arrays.asList(options));
return this;
@@ -142,11 +142,9 @@ public class OptionRule {
* Absolutely required options without any constraints.
*/
public Builder required(@NonNull Option<?>... options) {
- for (Option<?> option : options) {
- verifyDuplicate(option, "RequiredOption");
- //verifyRequiredOptionDefaultValue(option);
- }
- this.requiredOptions.add(RequiredOption.AbsolutelyRequiredOptions.of(options));
+ RequiredOption.AbsolutelyRequiredOptions requiredOption = RequiredOption.AbsolutelyRequiredOptions.of(options);
+ verifyRequiredOptionDuplicate(requiredOption);
+ this.requiredOptions.add(requiredOption);
return this;
}
@@ -157,20 +155,14 @@ public class OptionRule {
if (options.length <= 1) {
throw new OptionValidationException("The number of exclusive options must be greater than 1.");
}
- for (Option<?> option : options) {
- verifyDuplicate(option, "ExclusiveOption");
- //verifyRequiredOptionDefaultValue(option);
- }
- this.requiredOptions.add(RequiredOption.ExclusiveRequiredOptions.of(options));
+ RequiredOption.ExclusiveRequiredOptions exclusiveRequiredOption = RequiredOption.ExclusiveRequiredOptions.of(options);
+ verifyRequiredOptionDuplicate(exclusiveRequiredOption);
+ this.requiredOptions.add(exclusiveRequiredOption);
return this;
}
- public <T> Builder conditional(@NonNull Option<T> conditionalOption, @NonNull List<T> expectValues, @NonNull Option<?>... requiredOptions) {
- for (Option<?> o : requiredOptions) {
- verifyDuplicate(o, "ConditionalOption");
- //verifyRequiredOptionDefaultValue(o);
- }
-
+ public <T> Builder conditional(@NonNull Option<T> conditionalOption, @NonNull List<T> expectValues,
+ @NonNull Option<?>... requiredOptions) {
verifyConditionalExists(conditionalOption);
if (expectValues.size() == 0) {
@@ -188,27 +180,26 @@ public class OptionRule {
}
}
- this.requiredOptions.add(RequiredOption.ConditionalRequiredOptions.of(expression,
- new ArrayList<>(Arrays.asList(requiredOptions))));
+ RequiredOption.ConditionalRequiredOptions option = RequiredOption.ConditionalRequiredOptions.of(expression,
+ new ArrayList<>(Arrays.asList(requiredOptions)));
+ verifyRequiredOptionDuplicate(option);
+ this.requiredOptions.add(option);
return this;
}
- public <T> Builder conditional(@NonNull Option<T> conditionalOption, @NonNull T expectValue, @NonNull Option<?>... requiredOptions) {
- for (Option<?> o : requiredOptions) {
- // temporarily cancel this logic, need to find a way to verify that the options is duplicated
- // verifyDuplicate(o, "ConditionalOption");
- // verifyRequiredOptionDefaultValue(o);
- }
-
+ public <T> Builder conditional(@NonNull Option<T> conditionalOption, @NonNull T expectValue,
+ @NonNull Option<?>... requiredOptions) {
verifyConditionalExists(conditionalOption);
/**
* Each parameter can only be controlled by one other parameter
*/
Expression expression = Expression.of(Condition.of(conditionalOption, expectValue));
+ RequiredOption.ConditionalRequiredOptions conditionalRequiredOption = RequiredOption.ConditionalRequiredOptions.of(expression,
+ new ArrayList<>(Arrays.asList(requiredOptions)));
- this.requiredOptions.add(RequiredOption.ConditionalRequiredOptions.of(expression,
- new ArrayList<>(Arrays.asList(requiredOptions))));
+ verifyRequiredOptionDuplicate(conditionalRequiredOption);
+ this.requiredOptions.add(conditionalRequiredOption);
return this;
}
@@ -216,10 +207,9 @@ public class OptionRule {
* Bundled options, must be present or absent together.
*/
public Builder bundled(@NonNull Option<?>... requiredOptions) {
- for (Option<?> option : requiredOptions) {
- verifyDuplicate(option, "BundledOption");
- }
- this.requiredOptions.add(RequiredOption.BundledRequiredOptions.of(requiredOptions));
+ RequiredOption.BundledRequiredOptions bundledRequiredOption = RequiredOption.BundledRequiredOptions.of(requiredOptions);
+ verifyRequiredOptionDuplicate(bundledRequiredOption);
+ this.requiredOptions.add(bundledRequiredOption);
return this;
}
@@ -234,16 +224,51 @@ public class OptionRule {
}
}
- private void verifyDuplicate(@NonNull Option<?> option, @NonNull String currentOptionType) {
+ private void verifyDuplicateWithOptionOptions(@NonNull Option<?> option, @NonNull String currentOptionType) {
if (optionalOptions.contains(option)) {
throw new OptionValidationException(
String.format("%s '%s' duplicate in option options.", currentOptionType, option.key()));
}
+ }
+
+ private void verifyRequiredOptionDuplicate(@NonNull RequiredOption requiredOption) {
+ requiredOption.getOptions().forEach(option -> {
+ verifyDuplicateWithOptionOptions(option, requiredOption.getClass().getSimpleName());
+ requiredOptions.forEach(ro -> {
+ if (ro instanceof RequiredOption.ConditionalRequiredOptions &&
+ requiredOption instanceof RequiredOption.ConditionalRequiredOptions) {
+ Option<?> requiredOptionCondition =
+ ((RequiredOption.ConditionalRequiredOptions) requiredOption).getExpression().getCondition()
+ .getOption();
+
+ Option<?> roOptionCondition =
+ ((RequiredOption.ConditionalRequiredOptions) ro).getExpression().getCondition()
+ .getOption();
+
+ if (ro.getOptions().contains(option) && !requiredOptionCondition.equals(roOptionCondition)) {
+ throw new OptionValidationException(
+ String.format("%s '%s' duplicate in %s options.", requiredOption.getClass().getSimpleName(),
+ option.key(), ro.getClass().getSimpleName()));
+ }
+ } else {
+ if (ro.getOptions().contains(option)) {
+ throw new OptionValidationException(
+ String.format("%s '%s' duplicate in %s options.", requiredOption.getClass().getSimpleName(),
+ option.key(), ro.getClass().getSimpleName()));
+ }
+ }
+ });
+ });
+ }
+
+ private void verifyOptionOptionsDuplicate(@NonNull Option<?> option, @NonNull String currentOptionType) {
+ verifyDuplicateWithOptionOptions(option, currentOptionType);
requiredOptions.forEach(requiredOption -> {
if (requiredOption.getOptions().contains(option)) {
throw new OptionValidationException(
- String.format("%s '%s' duplicate in '%s'.", currentOptionType, option.key(), requiredOption.getClass().getName()));
+ String.format("%s '%s' duplicate in '%s'.", currentOptionType, option.key(),
+ requiredOption.getClass().getSimpleName()));
}
});
}
diff --git a/seatunnel-api/src/test/java/org/apache/seatunnel/api/configuration/util/OptionRuleTest.java b/seatunnel-api/src/test/java/org/apache/seatunnel/api/configuration/util/OptionRuleTest.java
index 1779d5542..73cf7aa1f 100644
--- a/seatunnel-api/src/test/java/org/apache/seatunnel/api/configuration/util/OptionRuleTest.java
+++ b/seatunnel-api/src/test/java/org/apache/seatunnel/api/configuration/util/OptionRuleTest.java
@@ -99,7 +99,7 @@ public class OptionRuleTest {
// test duplicate
assertEquals(
- "ErrorCode:[API-02], ErrorDescription:[Option item validate failed] - RequiredOption 'option.required-have-default' duplicate in option options.",
+ "ErrorCode:[API-02], ErrorDescription:[Option item validate failed] - AbsolutelyRequiredOptions 'option.required-have-default' duplicate in option options.",
assertThrows(OptionValidationException.class, executable).getMessage());
executable = () -> {
@@ -113,7 +113,7 @@ public class OptionRuleTest {
// test duplicate in RequiredOption$ExclusiveRequiredOptions
assertEquals(
- "ErrorCode:[API-02], ErrorDescription:[Option item validate failed] - RequiredOption 'option.test-duplicate' duplicate in 'org.apache.seatunnel.api.configuration.util.RequiredOption$ExclusiveRequiredOptions'.",
+ "ErrorCode:[API-02], ErrorDescription:[Option item validate failed] - AbsolutelyRequiredOptions 'option.test-duplicate' duplicate in ExclusiveRequiredOptions options.",
assertThrows(OptionValidationException.class, executable).getMessage());
executable = () -> {
@@ -140,11 +140,10 @@ public class OptionRuleTest {
.build();
};
- // temporary cancel this test case
// test parameter can only be controlled by one other parameter
- // assertEquals(
- // "ErrorCode:[API-02], ErrorDescription:[Option item validate failed] - ConditionalOption 'option.timestamp' duplicate in 'org.apache.seatunnel.api.configuration.util.RequiredOption$ConditionalRequiredOptions'.",
- // assertThrows(OptionValidationException.class, executable).getMessage());
+ assertEquals(
+ "ErrorCode:[API-02], ErrorDescription:[Option item validate failed] - ConditionalRequiredOptions 'option.timestamp' duplicate in ConditionalRequiredOptions options.",
+ assertThrows(OptionValidationException.class, executable).getMessage());
}
@Test