DanielCarter-stack commented on PR #10592:
URL: https://github.com/apache/seatunnel/pull/10592#issuecomment-4044192037
<!-- code-pr-reviewer -->
<!-- cpr:pr_reply_v2_parts {"group": "apache/seatunnel#10592", "part": 1,
"total": 1} -->
### Issue 1: OptionRule.equals() and hashCode() do not include conditionRules
**Location**: `OptionRule.java:117-132`
**Related Context**:
- New field: `OptionRule.java:87`
- equals implementation: `OptionRule.java:117-127`
- hashCode implementation: `OptionRule.java:130-132`
**Problem Description**:
`OptionRule` adds a new `conditionRules` field, but the `equals()` and
`hashCode()` methods are not updated accordingly. This causes two `OptionRule`
objects containing different `conditionRules` to be incorrectly determined as
equal.
**Potential Risks**:
- Risk 1: When using `OptionRule` in `HashMap` or `HashSet`, duplicate key
issues will occur
- Risk 2: Duplicate detection for `Builder.verifyConditionalExists()` may
fail (because it traverses `requiredOptions`, not checking `conditionRules`)
- Risk 3: Unit tests based on `equals()` may produce false positives
**Impact Scope**:
- Direct impact: Comparison and hash operations of `OptionRule` objects
- Indirect impact: Any code that depends on `OptionRule.equals()` (none
currently found, but may exist in the future)
- Affected area: Core API layer
**Severity**: MAJOR
**Improvement Suggestion**:
```java
@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (!(o instanceof OptionRule)) {
return false;
}
OptionRule that = (OptionRule) o;
return Objects.equals(optionalOptions, that.optionalOptions)
&& Objects.equals(requiredOptions, that.requiredOptions)
&& Objects.equals(conditionRules, that.conditionRules); // Added
}
@Override
public int hashCode() {
return Objects.hash(optionalOptions, requiredOptions, conditionRules);
// Added
}
```
**Rationale**:
- `equals()` and `hashCode()` contract requires: equal objects must have the
same hash code
- New fields should participate in equality judgment
- `ConditionRule` implements `equals()` (through Lombok `@Getter` + `@Data`
equivalent), so direct comparison is possible
---
### Issue 2: ConditionRule lacks equals() and hashCode() implementation
**Location**: `ConditionRule.java:1-32`
**Related Context**:
- Usage location: `OptionRule.java:279` (`conditionRulesMap.put(...)`)
- Comparison location: `OptionRule.java:273`
(`conditionRulesMap.containsKey(expression)`)
**Problem Description**:
`ConditionRule` uses Lombok's `@Getter`, but does not generate `equals()`
and `hashCode()`. As a value type for `HashMap` (although the key is
`Expression`), if deduplication or comparison based on `ConditionRule` is
needed in the future, problems will arise.
**Potential Risks**:
- Risk 1: If future code logic depends on `ConditionRule.equals()` (e.g.,
comparing whether two rules are the same), incorrect results will be obtained
- Risk 2: When `ConditionRule` is used in `HashSet` or as a value in
`HashMap`, proper deduplication cannot be performed
**Impact Scope**:
- Direct impact: Comparison of `ConditionRule` objects
- Indirect impact: Rule comparison logic that may be extended in the future
- Affected area: Core API layer
**Severity**: MINOR
**Improvement Suggestion**:
```java
@Getter
@EqualsAndHashCode // Added
public class ConditionRule {
private final Expression expression;
private final OptionRule optionRule;
public ConditionRule(Expression expression, OptionRule optionRule) {
this.expression = expression;
this.optionRule = optionRule;
}
}
```
**Rationale**:
- Value objects should implement `equals()` and `hashCode()`
- Lombok's `@EqualsAndHashCode` automatically generates correct methods
- Although `ConditionRule` currently only serves as a value in `HashMap`, it
should be added for completeness and future extensibility
---
### Issue 3: Missing JavaDoc documentation
**Locations**:
- `ConditionRule.java:23` (class level)
- `OptionRule.java:106-108` (`getConditionRules()` method)
- `OptionRule.java:242-291` (`conditionalRule()` method)
**Related Context**:
- Existing documentation: `OptionRule.java:34-67` (JavaDoc example at top of
class)
**Problem Description**:
The newly added API lacks JavaDoc documentation, users cannot understand:
- The purpose and use cases of `ConditionRule`
- The difference between `conditionalRule()` and `conditional()`
- When to use nested rules
**Potential Risks**:
- Risk 1: Connector developers may not be aware of this new feature
- Risk 2: Users may choose incorrectly between `conditional()` and
`conditionalRule()`
- Risk 3: Reduced API usability
**Impact Scope**:
- Direct impact: API documentation
- Indirect impact: Connector development efficiency
- Affected area: All Connector developers
**Severity**: MINOR
**Improvement Suggestion**:
**1. ConditionRule class documentation**:
```java
/**
* A conditional rule that associates an expression with an {@link
OptionRule}.
*
* <p>Used for nested option validation. When the expression evaluates to
true,
* the associated {@code OptionRule} will be validated.
*
* <p>Example:
* <pre>{@code
* // When auth_mode == "password" and username == "admin", require
admin_token
* OptionRule adminRule = OptionRule.builder().required(ADMIN_TOKEN).build();
* OptionRule authRule = OptionRule.builder()
* .conditionalRule(USERNAME, "admin", adminRule)
* .build();
* }</pre>
*
* @see Expression
* @see OptionRule
*/
@Getter
@EqualsAndHashCode
public class ConditionRule {
// ...
}
```
**2. conditionalRule() method documentation**:
```java
/**
* Adds a conditional rule that will be validated when the given option
matches
* any of the expected values.
*
* <p>Unlike {@link #conditional(Option, List, Option...)}, which only allows
* specifying required options, this method accepts a full {@link OptionRule}
* that can contain nested conditions, bundled options, etc.
*
* <p>Use this method when you need complex nested validation logic.
*
* @param <T> the type of the conditional option
* @param conditionalOption the option to check
* @param expectValues the expected values that trigger validation of {@code
conditionalOptionRule}
* @param conditionalOptionRule the rule to validate when condition matches
* @return this builder
* @throws OptionValidationException if {@code expectValues} is empty
* @throws OptionValidationException if {@code conditionalOptionRule} has no
options
* @throws OptionValidationException if a rule for the same expression
already exists
* @see #conditional(Option, List, Option...)
*/
public <T> Builder conditionalRule(
@NonNull Option<T> conditionalOption,
@NonNull List<T> expectValues,
@NonNull OptionRule conditionalOptionRule) {
// ...
}
```
**3. Update example at top of OptionRule class**:
```java
/**
* Validation rule for {@link Option}.
*
* <p>The option rule is typically built in one of the following pattern:
*
* <pre>{@code
* // simple rule
* OptionRule simpleRule = OptionRule.builder()
* .optional(POLL_TIMEOUT, POLL_INTERVAL)
* .required(CLIENT_SERVICE_URL)
* .build();
*
* // basic full rule
* OptionRule fullRule = OptionRule.builder()
* .optional(POLL_TIMEOUT, POLL_INTERVAL, CURSOR_STARTUP_MODE)
* .required(CLIENT_SERVICE_URL, ADMIN_SERVICE_URL)
* .exclusive(TOPIC_PATTERN, TOPIC)
* .conditional(CURSOR_STARTUP_MODE, StartMode.TIMESTAMP,
CURSOR_STARTUP_TIMESTAMP)
* .build();
*
* // complex conditional rule
* // moot expression
* Expression expression = Expression.of(TOPIC_DISCOVERY_INTERVAL, 200)
* .and(Expression.of(Condition.of(CURSOR_STARTUP_MODE,
StartMode.EARLIEST)
* .or(CURSOR_STARTUP_MODE, StartMode.LATEST)))
* .or(Expression.of(Condition.of(TOPIC_DISCOVERY_INTERVAL, 100)))
*
* OptionRule complexRule = OptionRule.builder()
* .optional(POLL_TIMEOUT, POLL_INTERVAL, CURSOR_STARTUP_MODE)
* .required(CLIENT_SERVICE_URL, ADMIN_SERVICE_URL)
* .exclusive(TOPIC_PATTERN, TOPIC)
* .conditional(expression, CURSOR_RESET_MODE)
* .build();
*
* // nested conditional rule (new feature)
* // When auth_mode == "password", require username and password
* // When auth_mode == "password" and username == "admin", also require
admin_token
* OptionRule adminRule = OptionRule.builder()
* .required(ADMIN_TOKEN)
* .build();
* OptionRule passwordRule = OptionRule.builder()
* .required(USERNAME, PASSWORD)
* .conditionalRule(USERNAME, "admin", adminRule)
* .build();
* OptionRule authRule = OptionRule.builder()
* .required(AUTH_MODE)
* .conditionalRule(AUTH_MODE, "password", passwordRule)
* .conditionalRule(AUTH_MODE, "kerberos", kerberosRule)
* .build();
* }</pre>
*/
```
**Rationale**:
- JavaDoc is the contract of the API and must be complete
- Example code is more effective than abstract descriptions
- Distinguishing use cases between `conditional()` and `conditionalRule()`
is crucial
---
### Issue 4: OptionRule.Builder.verifyConditionalExists() does not check
conditionRules
**Location**: `OptionRule.java:414-428`
**Related Context**:
- Call location: `OptionRule.java:246` (in `conditionalRule()` method)
- Current check logic: only traverses `optionalOptions` and `requiredOptions`
**Problem Description**:
The `verifyConditionalExists()` method verifies whether conditional options
are defined, but it only checks `optionalOptions` and `requiredOptions`, not
the nested rules in `conditionRules`.
**Code Analysis**:
```java
private void verifyConditionalExists(@NonNull Option<?> option) {
boolean inOptions = optionalOptions.contains(option);
AtomicBoolean inRequired = new AtomicBoolean(false);
requiredOptions.forEach(requiredOption -> {
if (requiredOption.getOptions().contains(option)) {
inRequired.set(true);
}
});
// Did not check nested OptionRule in conditionRules
if (!inOptions && !inRequired.get()) {
throw new OptionValidationException(...);
}
}
```
**Potential Risks**:
- Risk 1: If a conditional option is only defined in nested `OptionRule`, it
will be incorrectly determined as "not found"
- Risk 2: Limits the flexibility of nested rules (although current design
may not need this scenario)
**Actual Impact**:
- Low (in current design, conditional options should be defined in
parent-level rules)
- But the code logic is incomplete and can mislead future maintainers
**Impact Scope**:
- Direct impact: Correctness of `Builder.verifyConditionalExists()`
- Indirect impact: Construction limitations of nested rules
- Affected area: Core API layer
**Severity**: MINOR
**Improvement Suggestion**:
```java
private void verifyConditionalExists(@NonNull Option<?> option) {
boolean inOptions = optionalOptions.contains(option);
AtomicBoolean inRequired = new AtomicBoolean(false);
requiredOptions.forEach(requiredOption -> {
if (requiredOption.getOptions().contains(option)) {
inRequired.set(true);
}
});
// Added: Check nested OptionRule in conditionRules
AtomicBoolean inConditionRules = new AtomicBoolean(false);
conditionRulesMap.values().forEach(nestedRule -> {
// Check optionalOptions of nested rules
if (nestedRule.getOptionalOptions().contains(option)) {
inConditionRules.set(true);
}
// Check requiredOptions of nested rules
nestedRule.getRequiredOptions().forEach(ro -> {
if (ro.getOptions().contains(option)) {
inConditionRules.set(true);
}
});
});
if (!inOptions && !inRequired.get() && !inConditionRules.get()) {
throw new OptionValidationException(
String.format("Conditional '%s' not found in options.",
option.key()));
}
}
```
**Rationale**:
- Logical completeness: should check all places that may contain the option
- Leave room for future expansion: although current design may not need it,
complete checking can avoid future bugs
- Alternatively: if this scenario is not supported, it should be clearly
stated in the documentation
**Alternative Approach**:
If the current design does not intend to support the scenario where
"conditional options are only defined in nested rules", a comment can be added:
```java
// Note: The conditional option must be defined in the parent rule's
optional/required options,
// not in nested ConditionRules. This is by design to keep the validation
hierarchy clear.
```
---
### Issue 5: Test cases do not cover edge cases
**Location**: `ConfigValidatorTest.java:298-350`
**Related Context**:
- Test method: `testNestedOptionRule()`
- Covered scenarios: Positive and negative cases for 3-layer nesting
**Problem Description**:
Test cases do not cover the following edge cases:
1. Empty `conditionalOptionRule` (already intercepted by `hasOptions()`
check)
2. Duplicate expressions (already intercepted by `containsKey()` check)
3. Scenario where conditional options are undefined
4. Short-circuit evaluation for multi-layer nesting (e.g., if the first
layer condition does not match, whether to skip the second layer)
**Potential Risks**:
- Risk 1: Behavior of edge cases is undefined and may cause runtime
exceptions
- Risk 2: Future refactoring may break these untested paths
**Impact Scope**:
- Direct impact: Test coverage
- Indirect impact: Code robustness
- Affected area: Test quality
**Severity**: MINOR
**Improvement Suggestion**:
```java
@Test
public void testConditionalRuleWithEmptyRule() {
OptionRule emptyRule = OptionRule.builder().build();
Executable executable = () -> OptionRule.builder()
.conditionalRule(SINGLE_CHOICE_VALUE_TEST, "A", emptyRule);
assertEquals(
"ErrorCode:[API-02], ErrorDescription:[Option item validate failed]
- " +
"conditional option rule for 'single_choice_test' must have
options.",
assertThrows(OptionValidationException.class,
executable).getMessage());
}
@Test
public void testDuplicateConditionalRule() {
OptionRule subRule = OptionRule.builder().required(KEY_USERNAME).build();
Executable executable = () -> OptionRule.builder()
.conditionalRule(SINGLE_CHOICE_VALUE_TEST, "A", subRule)
.conditionalRule(SINGLE_CHOICE_VALUE_TEST, "A", subRule); //
Duplicate
assertEquals(
"ErrorCode:[API-02], ErrorDescription:[Option item validate failed]
- " +
"conditional option rule for 'single_choice_test' with expression " +
"'['single_choice_test' == A]' already exists.",
assertThrows(OptionValidationException.class,
executable).getMessage());
}
@Test
public void testUndefinedConditionalOption() {
OptionRule subRule = OptionRule.builder().required(KEY_USERNAME).build();
Option<String> undefinedOption = Options.key("undefined").stringType()
.noDefaultValue().withDescription("undefined");
Executable executable = () -> OptionRule.builder()
.conditionalRule(undefinedOption, "value", subRule); //
undefinedOption is undefined
assertEquals(
"ErrorCode:[API-02], ErrorDescription:[Option item validate failed]
- " +
"Conditional 'undefined' not found in options.",
assertThrows(OptionValidationException.class,
executable).getMessage());
}
```
**Rationale**:
- Although these scenarios are intercepted by runtime checks, tests should
verify error messages
- Ensure edge case behavior meets expectations
- Improve code robustness and maintainability
---
--
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]
