davidzollo commented on PR #10977:
URL: https://github.com/apache/seatunnel/pull/10977#issuecomment-4598394379
## Re-review (after commits 728e3ee...894091d)
Apologies for being less thorough on the first pass — re-evaluating the
whole PR from scratch as required, and acknowledging that I had under-credited
the depth of the rewrite. Local verification this time: **146/146 tests pass**
in `seatunnel-api` (`ConfigValidatorTest` 143 + `ConditionTest` 3),
`spotless:check` clean.
### What this PR really does (concrete example)
Today, "port must be in [1, 65535]" or "start_ts must be less than end_ts"
lives as imperative `if/else` inside `*Config.java`, invisible to REST/CLI/Web
UI/AI tooling, and reported one-at-a-time. After this PR:
```java
import static org.apache.seatunnel.api.configuration.util.Conditions.*;
OptionRule.builder()
.required(PORT, greaterOrEqual(PORT, 1).and(lessOrEqual(PORT, 65535)))
.required(HOST, notBlank(HOST))
.required(START_TS, END_TS, lessThanField(START_TS, END_TS))
.build();
```
`ConfigValidator` runs every constraint at job-submission time, aggregates
**all** failures (structural + value) into a single `Option validation failed
(N errors): [1] ... [2] ...` message, REST `/option-rules` exposes both the
symbolic `compareOperator` and the structured `conditionOperator` /
`conditionOperatorCategory` so frontends/AI don't need to string-match.
### Verifying the previous round's blockers were fixed
All 8 items I raised in the previous review are addressed:
| # | Previous finding | Status | How |
|---|---|---|---|
| 1 | `optional(start, end, lessThanField(start, end))` threw `Cannot
compare null` when one side missing | **Fixed** | `isConstraintApplicable` now
splits the chain at OR boundaries and requires AND-segments to be fully
present; tests `testOptionalCrossFieldOnlyStartPresent` / `OnlyEndPresent` /
`NoFalsePositive` pin it down |
| 2 | `greaterOrEqual(PORT,1).or(notBlank(HOST))` broke short-circuit when
PORT null | **Fixed** | All numeric/FIELD evaluators are now null-safe (`v !=
null && ...`); covered by `testOrChainNumericNullWithStringFallback` (4
scenarios) |
| 3 | `greaterThan(longOpt, 0)` raised `Numeric type mismatch` because `int`
vs `Long` | **Fixed** | `compareNumberValues` now normalizes via `BigDecimal` /
`double` / `long` three-tier; `testNumericTypeMismatchIntVsLong` /
`DoubleVsInt` cover it |
| 4 | REST `ConditionNode` only had symbol string; AI/frontend forced to
string-match | **Fixed** | Added `conditionOperator` (e.g. `GREATER_OR_EQUAL`)
+ `conditionOperatorCategory` (e.g. `NUMERIC`) |
| 5 | Raw-type `new Condition(...)` factories | **Fixed** | Factories moved
to `Conditions.java` (clean generics); narrow `Condition.java` to data + chain
logic |
| 6 | Evaluator exceptions dropped option key | **Fixed** |
`ConditionEvaluators.evaluate` wraps inner exceptions as `Failed to evaluate
constraint '%s' on option '%s': %s` |
| 7 | `Condition.of(opt, null)` API narrowed without
`incompatible-changes.md` entry | **Fixed** | Entry added in both
`docs/{en,zh}/introduction/concepts/incompatible-changes.md` |
| 8 | Test coverage gaps (optional cross-field single-side, OR with numeric,
type mismatch) | **Fixed** | Test count went from 32 → **143** in
`ConfigValidatorTest`; nearly every scenario I called out has an explicit test |
Bonus improvement that wasn't in the previous review but is genuinely
production-grade: `collectErrors` aggregates structural + value errors into one
`OptionValidationException` instead of fail-fast. Big UX win for users
debugging large HOCON configs.
### New findings on this pass
#### Issue 1 (medium) — Asymmetric cross-field: `optional(head) +
required(compareField)` makes the optional head implicitly required
`isConstraintApplicable` collects `head + compareOption + all chain nodes`
and returns `true` as soon as **any** of them is `AbsolutelyRequiredOptions`.
Consider:
```java
.required(START_TS)
.optional(MAX, lessThanField(MAX, START_TS))
```
- `allOptions = {MAX, START_TS}`
- `START_TS` is absolutely required → first-stage hit → applicable
- `FIELD_LESS_THAN(null, START_TS)` → returns `false`
- Error reported: `constraint: 'max' < 'start_ts'`
User's clear intent: "MAX is optional; if set, must be less than START_TS."
Actual behavior: MAX is silently required. The symmetric case (`required(head)
+ optional(compareField)`) is already pinned down by
`testRequiredPrimaryWithAbsentOptionalCompareField` as "must fail", which is a
consistent but easy-to-trip design choice.
**Suggested fix (Option A, recommended):** keep "required head forces
applicable" but only inspect the **head** node, not the entire option set. That
makes both directions symmetric — the head decides. Adjust
`testRequiredPrimaryWithAbsentOptionalCompareField` accordingly (END_TS is
optional, so the constraint should skip and let structural validation handle
the absence).
**Option B (conservative):** keep both semantics, but add a Javadoc warning
on every cross-field factory in `Conditions.java` stating "if used together
with `optional(head, ...)`, the compareOption must also be optional".
#### Issue 2 (low) — `MetadataExportCommand` didn't pick up
`conditionOperator` / `conditionOperatorCategory`
`OptionRulesService.toConditionNode` now exposes the structured operator
metadata for the REST API, but `MetadataExportCommand.exportCondition` still
only emits `compareOperator` (symbol string). The PR description specifically
calls out AI-assisted config generation, and the CLI export is exactly the
surface AI tools consume offline. Quick fix: mirror the same fields via
`op.name()` / `op.getCategory().name()`.
#### Issue 3 (low) — Docs hint at length/size constraints that the PR didn't
ship
The previous revision had `LENGTH_*`, `COLLECTION_SIZE_*`,
`FIELD_EQUAL/NOT_EQUAL`, `FIELD_SIZE_EQUAL`, etc. Those have been (rightly)
trimmed — the surface is now 17 operators in 4 categories. But the architecture
doc still uses "string length limit" / "fixed collection size" as motivating
examples. Suggest either narrowing the docs or adding a "currently supported
operators" callout in `Conditions.java`'s class-level Javadoc so users know
what's actually available without grepping.
#### Issue 4 (medium) — `OptionValidationException` message format changed;
no `incompatible-changes.md` entry
Old format (per error):
```
ErrorCode:[API-02], ErrorDescription:[Option item validate failed] - There
are unconfigured options, the options('host') are required.
```
New format (aggregated):
```
ErrorCode:[API-02], ErrorDescription:[Option item validate failed] - Option
validation failed (2 errors):
[1] option: 'host'
type: required
constraint: required option is not configured
[2] option: 'port'
type: value
constraint: 'port' >= 1
```
This is strictly a better UX, but any consumer doing
`getMessage().contains("are required")` will break. Connector unit tests doing
string assertions on the message (the PR itself rewrote a handful of them) will
fail. Worth adding a second `incompatible-changes.md` entry next to the
`Condition.of(opt, null)` one.
#### Issue 5 (low) — `Condition.*` static factories were removed without a
deprecated bridge
`Condition.greaterThan(...)`, `Condition.notBlank(...)`, etc. moved to
`Conditions.*`. Anyone using them outside the main repo will get a compile
error. Either:
- Keep `@Deprecated public static <T> Condition<T> greaterThan(...) { return
Conditions.greaterThan(...); }` bridges, **or**
- Document the rename in `incompatible-changes.md` alongside the others.
#### Issue 6 (low) — `compareNumbers` error message includes raw values
```java
throw new OptionValidationException(
"Cannot compare null values in numeric comparison: left=%s, right=%s",
a, b);
```
For numeric options this is fine, but if a connector ever wires up a
sensitive option as `Number`, the raw value leaks into logs. Safer to swap to
`leftType=%s, rightType=%s`.
#### Issue 7 (low) — `Conditions.java` class Javadoc has a truncated example
```java
* <pre>{@code
* static *
* OptionRule.builder()
```
Looks like an incomplete `import static
org.apache.seatunnel.api.configuration.util.Conditions.*;` line.
#### Issue 8 (low) — `extractInnerMessage` couples to
`OptionValidationException` text format
Splitting on `" - "` to strip the `ErrorCode:[API-02] - ` prefix is fine
today, but ties the evaluator wrapper to an implementation detail of the
exception. A `getRawMessage()` accessor on `OptionValidationException` would be
cleaner. Not urgent.
#### Issue 9 (low) — Missing test for the Issue 1 scenario
`testRequiredPrimaryWithAbsentOptionalCompareField` pins down
`required(head) + optional(compareField)`. The mirror case (`optional(head) +
required(compareField)`, head absent) is not explicitly tested. Worth adding
one to make the chosen semantic regression-proof.
### Issue summary
| # | Issue | Location | Severity |
|---|---|---|---|
| 1 | `optional(head) + required(compareField)` implicitly forces head to be
required | `ConfigValidator#isConstraintApplicable` | medium |
| 4 | `OptionValidationException` message format change not documented |
`ConfigValidator#validate(OptionRule)` | medium |
| 2 | `MetadataExportCommand` doesn't expose `conditionOperator` /
`conditionOperatorCategory` | `MetadataExportCommand#exportCondition` | low |
| 3 | Docs imply length/size constraints that aren't shipped |
`Conditions.java`, configuration-and-option-system docs | low |
| 5 | `Condition.*` static factories removed without `@Deprecated` bridges
or doc | `Condition.java` | low |
| 6 | `compareNumbers` error message leaks raw values |
`ConditionEvaluators#compareNumbers` | low |
| 7 | `Conditions.java` class Javadoc has a truncated example |
`Conditions.java` | low |
| 8 | `extractInnerMessage` couples to exception text format |
`ConditionEvaluators#extractInnerMessage` | low |
| 9 | Missing test for `optional(head) + required(compareField) + head
absent` | `ConfigValidatorTest` | low |
### Verification commands
| Command | Result | Note |
|---|---|---|
| `./mvnw -B -pl seatunnel-api spotless:check -nsu
-Dmaven.gitcommitid.skip=true` | PASS | clean |
| `./mvnw -B -pl seatunnel-api test -Dtest=ConfigValidatorTest,ConditionTest
-nsu -Dmaven.gitcommitid.skip=true` | **PASS (146/146)** | |
| `./mvnw -B -pl seatunnel-engine/seatunnel-engine-server test
-Dtest=OptionRulesServiceTest -am ...` | not run | `seatunnel-config-shade`
failed to compile (missing `ConfigValue/Entry` symbols, unrelated to this PR —
typical when shade-relocate hasn't run) |
| `git grep "Condition\.of\([^,]*, *null"` in main repo | none | no current
production callers, backward-compat risk for `Condition.of(opt, null)`
narrowing is effectively zero |
### Conclusion: ready to merge after two doc additions; the rest can be
follow-ups
No runtime blockers remain. The remaining 9 items are either documentation
gaps (4, 5), low-severity polish (2, 3, 6, 7, 8, 9), or one semantic edge case
that I think deserves discussion before code change (1).
**Recommended pre-merge (≈30 min):**
- Add a single `incompatible-changes.md` block covering (a)
`OptionValidationException` message format change and (b) `Condition.*` static
factory relocation. Or alternatively keep `@Deprecated` bridges on
`Condition.*`.
**Recommended follow-up PR:**
- Decide on Issue 1's semantic (Option A: head-only
`isConstraintApplicable`, or Option B: Javadoc warning on cross-field
factories).
- Mirror the structured operator metadata in `MetadataExportCommand` (Issue
2).
- Tighten docs scope to currently-shipped operators (Issue 3).
- Polish 6/7/8 + add the missing test from Issue 9.
### Overall assessment
This is a meaningful step up from the previous revision. The contributor
turned every one of the 8 prior findings into a precise fix backed by tests,
plus added error aggregation and structured REST metadata that weren't
requested but are genuinely production-grade. The 17-operator scope is a
deliberate, healthy narrowing relative to the prior 30 — better to ship a tight
set with full evaluator + REST + AI parity than 30 operators in inconsistent
states.
Solid work. Looking forward to seeing this land.
--
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]
