zhangshenghang commented on PR #11095: URL: https://github.com/apache/seatunnel/pull/11095#issuecomment-4716092501
Thanks for the cleanup — moving the scattered imperative checks to declarative `OptionRule + Conditions` is a nice direction, and the per-transform factory tests are a solid addition. I reviewed the diff against `dev` and found one real regression plus a few things worth addressing before merge. ## 🔴 DynamicCompile lost its conditional-required validation In `DynamicCompileTransformFactory`: ```java // dev: source_code is REQUIRED when compile_pattern = SOURCE_CODE .conditional(COMPILE_PATTERN, SOURCE_CODE, DynamicCompileTransformConfig.SOURCE_CODE) // this PR: only a not-blank value constraint, no longer required .conditional(COMPILE_PATTERN, SOURCE_CODE, Conditions.notBlank(SOURCE_CODE)) ``` These two overloads are not equivalent. The old `conditional(opt, value, Option)` registers `source_code` as a **conditionally-required option**. The new `conditional(opt, value, Condition)` only adds a value constraint, and `ConfigValidator.isConstraintApplicable()` **skips a constraint whose option key is absent**. So: | config | dev | this PR | |---|---|---| | `source_code = " "` (blank) | error | error ✅ | | `source_code` key **entirely missing** | **error** | **silently passes** ❌ | Since `compile_pattern` defaults to `SOURCE_CODE`, this is a common misconfig path. The job now passes submission-time validation and fails much later with an NPE / compile error — exactly what this refactor is supposed to prevent. `absolute_path` has the same issue. I verified this locally with a probe test (`compile_pattern=SOURCE_CODE`, `source_code` omitted) — `assertDoesNotThrow` passes on this branch. The existing `testSourceCodeBlankFails` only covers the **blank** case, not the **missing** case, which is why CI is green. **Suggested fix** — keep it required and add the not-blank check: ```java .conditional(COMPILE_PATTERN, CompilePattern.SOURCE_CODE, SOURCE_CODE) .conditional(COMPILE_PATTERN, CompilePattern.SOURCE_CODE, Conditions.notBlank(SOURCE_CODE)) .conditional(COMPILE_PATTERN, CompilePattern.ABSOLUTE_PATH, ABSOLUTE_PATH) .conditional(COMPILE_PATTERN, CompilePattern.ABSOLUTE_PATH, Conditions.notBlank(ABSOLUTE_PATH)) ``` and please add negative tests for the **missing-key** case (and for "neither source_code nor absolute_path set"). ## 🟡 The "errors are aggregated" claim doesn't hold for extension validators The PR description says validation errors are now aggregated instead of failing on the first error. But the `ConditionExtension` implementations used here (DefineSinkType, JsonPath, DataValidator, RegexExtract, TableFilter, CopyField) `throw new OptionValidationException(...)` on failure, which propagates straight out of `collectErrors()` and **aborts aggregation**. So plain `Conditions` aggregate, but the moment an extension throws it degrades to fail-fast — inconsistent within the same transform. Either return `false` from the extensions (compose the message from `description()`) to stay aggregatable, or adjust the PR description to say structural extension checks are fail-fast. ## 🟡 New rules / tightening beyond a pure migration — please document for backward-compat A few changes go beyond equivalent migration and can reject configs that worked before: - **DefineSinkType**: new duplicate-column-name check (dev only checked column/type non-null). - **FieldEncrypt**: new `max_field_length > 0` (dev didn't validate it at all). - **`optional → required` tightening**: FieldMapper, Metadata, JsonPath, SQL core options, and DynamicCompile's `compile_language` (no default) are now mandatory. These are mostly reasonable "fail earlier", but per the project's backward-compatibility policy they should be called out in the PR description and recorded in `docs/en/introduction/concepts/incompatible-changes.md` with migration notes. ## 🟢 Minor - `TableFilterTransformFactory` uses a fully-qualified `java.util.regex.Pattern.compile(...)` instead of a top-of-file import — inconsistent with the project style. The near-identical `RegexValidator` / column-structure validator inner classes across factories could also be pulled into a shared util. - FYI (not a problem): removing the "at least one pattern" check in `TableFilterConfig.of()` is fine — `PATTERN_MODE` has a default (`INCLUDE`), so `patternMode != null` was always true and that `checkArgument` was effectively dead code. No behavior change. - `RowKindExtractorTransformFactory` adding `.optional(TRANSFORM_TYPE)` is a genuine fix — that key was usable by the transform but undeclared, so it would have been rejected by unknown-key validation. 👍 Overall: good direction and good test effort — just please fix the DynamicCompile conditional-required regression (with missing-key tests) and document the tightened/added rules before merge. -- 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]
