DanielLeens commented on PR #11032:
URL: https://github.com/apache/seatunnel/pull/11032#issuecomment-5203583830
@SEZ9 thanks for the detailed re-review. I went back through both of your
reviews on the current head (`22504262e84b`) and traced the exact runtime chain
again before replying, since your latest review's Issue 1 makes a factual claim
I want to check carefully rather than just take on faith.
## Pushing back on Issue 1 in your 2026-08-06 review ("ConfigValidator is
never invoked on these nested ReadonlyConfig instances")
I don't think this is accurate on the current head. The `tables_configs`
path is wired into the same `ConfigValidator` run as the root config, via a
`valueConstraint`:
- `FakeSourceFactory.java:195-198` registers
`.valueConstraint(extension(ConnectorCommonOptions.TABLE_CONFIGS, new
TableConfigsValidationExtension()))` on the factory's `OptionRule`.
- `FakeSourceFactory.java:202-236` —
`TableConfigsValidationExtension.evaluate(...)` loops every entry in
`tables_configs` and explicitly calls
`ConfigValidator.of(ReadonlyConfig.fromMap(childConfig)).validate(childRule)`,
where `childRule` is `new FakeSourceFactory().optionRule()` — i.e. the *same*
rule with all 14 min/max range constraints, applied recursively to each child.
- `ConfigValidator.java:230-249` (`collectErrors`) iterates
`rule.getValueConstraints()` and evaluates each one, so this constraint is not
skipped.
- `ConditionEvaluators.java:168-173` shows the `EXTENSION` operator
delegates straight to `ConditionExtension#evaluate(config, value)`, which is
exactly the extension above.
So the call chain for a `tables_configs` child is:
`FactoryUtil.createAndPrepareSource()` →
`ConfigValidator.validate(factory.optionRule())` → constraint loop →
`TableConfigsValidationExtension.evaluate()` → nested
`ConfigValidator.validate(childRule)` per child. A `tinyint.min = 999` inside
`tables_configs[0]` fails at the *same* top-level validation step as a
root-level violation, before `FakeSource`/`FakeConfig.buildWithConfig` is ever
reached.
This is also covered by a passing test in this PR:
`FakeFactoryTest.invalidTinyintMinInTablesConfigsShouldFailValidation()` builds
a `tables_configs` config with `tinyint.min = 200`, calls
`ConfigValidator.of(...).validate(new FakeSourceFactory().optionRule())` (the
same entry point the factory uses), and asserts it throws
`OptionValidationException` whose message contains both `"tables_configs[0]"`
and `"tinyint.min"`. That's the exact scenario your Issue 1 says passes
silently.
For context: this was the very first blocker I raised on this PR back on
2026-06-09 (multi-table `table_configs` losing validation), and I re-verified
the fix in three follow-up rounds (2026-06-12, and again in my full review on
2026-07-26) using this same trace. I'd suggest we not re-block on this one
unless there's a concrete reproduction that gets past the extension — happy to
be shown a config that slips through if I'm missing something.
## Points I agree with (+1, still open, non-blocking)
- **`valueConstraint` doesn't verify the referenced option is registered**
(your Issue 4 in the 2026-07-27 review, Issue 2 wording in the 2026-08-06
review) — confirmed, this is the same gap I flagged as my own Issue 1 in the
2026-07-26 review. Looking at `OptionRule.java:375-384`, `valueConstraint(...)`
just appends to `valueConstraints` with no check against
`required`/`optional`/`exclusive`, unlike the sibling `optional(option,
condition, ...)` overload right above it which calls
`verifyOptionOptionsDuplicate(...)`. Worth fixing before more connectors adopt
this API, but it doesn't affect `FakeSource` today since `TABLE_CONFIGS` is
already registered via `exclusive(...)`.
- **Varargs elements in `valueConstraint` aren't null-checked** — confirmed
at `OptionRule.java:382`, `Collections.addAll(this.valueConstraints,
conditions)` will happily insert a null and defer the failure to a later NPE
inside `ConfigValidator`. Agreed this should fail fast at build time instead.
- **No test for `valueConstraint` referencing an unregistered option /
multiple varargs conditions** — confirmed, `OptionRuleTest.java` only exercises
the single-condition, already-registered case (`TEST_TOPIC_PATTERN` via
`.exclusive(...)`). Agreed this is a real coverage gap once the registration
check above is added.
- **Javadoc on `valueConstraint` is thin/slightly misleading** ("already
registered by another rule" should say "registered on this same builder via
`required`/`optional`/`exclusive`") — agreed, same as above.
## One clarification on your 2026-07-27 review, Issue 1/3 (buildWithConfig
losing its own defensive checks / exception type change)
I traced this too: in the supported path, `FakeConfig.buildWithConfig(...)`
is only ever reached after `FactoryUtil.createAndPrepareSource()` has already
run `ConfigValidator.validate(factory.optionRule())` — for both the root config
and, per the extension above, every `tables_configs` child. So on the normal
user path there's no window where an out-of-range value reaches
`buildWithConfig` unvalidated, and the `FakeConnectorException` →
`OptionValidationException` change is a real but expected consequence of moving
validation earlier, not a silent gap. I agree it's worth a one-line callout in
the PR description for anyone matching on the old exception type/message, but I
wouldn't treat it as blocking — `buildWithConfig` being usable as an
unvalidated internal constructor for direct/test callers is a reasonable
trade-off for a connector-internal method, not part of the public factory SPI.
Given the above, my read is that the current head is behavior-preserving on
the path that matters (including `tables_configs`), and what's left is
legitimate but non-blocking API-hygiene work on the new `valueConstraint`
builder method. Let me know if you have a concrete config that reproduces the
Issue 1 scenario on this head — I'd want to fix my own understanding if so.
--
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]
