nzw921rx commented on PR #11095: URL: https://github.com/apache/seatunnel/pull/11095#issuecomment-4764697292
> Thanks for the update. I re-reviewed the latest head from scratch. > > +1 to @zhangshenghang's earlier DynamicCompile point: the conditional-required regression is fixed on the current head by `DynamicCompileTransformFactory.java:42-57`, and the new negative tests at `DynamicCompileTransformFactoryTest.java:83-104` now cover the missing-key paths that were previously slipping through. > > I still see one blocker before merge, plus one follow-up that is worth tightening up. > > ## What this PR solves > * Pain point: several transform plugins were still discovering config errors too late, with validation split across constructors, runtime logic, and ad hoc checks. > * Approach: move more of that validation into declarative `OptionRule` rules and add focused factory-level tests around the migrated contracts. > * One-line summary: the direction is good, but the current head still leaves a compatibility-documentation gap, and the new "ConditionExtension aggregation" cleanup is only partial. > > ## Full runtime path I rechecked > ``` > job config > -> transform factory optionRule() > -> ConfigValidator.validate(optionRule) > -> required / optional / conditional checks > -> Conditions.notBlank / notEmpty / greaterThan > -> Conditions.extension(..., ConditionExtension) > > if submission-time validation passes > -> createTransform(context) > -> transform instance is created and enters the runtime path > > current head specifics > -> DynamicCompileTransformFactory [DynamicCompileTransformFactory.java:38-60] > -> SOURCE_CODE / ABSOLUTE_PATH are again conditionally required > -> CopyFieldTransformFactory / TableFilterTransformFactory > -> now return false from the extension validator, which allows aggregation to continue > -> DefineSinkType / JsonPath / RegexExtract / DataValidator > -> still throw OptionValidationException directly from ConditionExtension > ``` > > ## Findings > ### Issue 1: the newly tightened validation contracts are still undocumented as incompatible changes > * Location: > > * `seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/adaptsink/DefineSinkTypeTransformFactory.java:46-55,79-96` > * `seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/encrypt/FieldEncryptTransformFactory.java:40-55` > * `seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/dynamiccompile/DynamicCompileTransformFactory.java:38-60` > * `docs/en/introduction/concepts/incompatible-changes.md:6-104` > * `docs/zh/introduction/concepts/incompatible-changes.md` > * Why this is a problem: > > * This PR is doing more than just moving existing failures earlier. It also tightens several submission-time contracts that can now reject configs that previously passed. > * Concrete examples on the current head include duplicate-column rejection in `DefineSinkType`, `max_field_length > 0` in `FieldEncrypt`, and the restored conditional-required checks in `DynamicCompile`. > * Per the project's compatibility policy, those user-visible rejections need to be recorded in the incompatible-changes docs with migration guidance. That documentation is still missing. > * Risk: > > * Users upgrading into this behavior will hit new submission-time failures without release-note coverage or a clear migration path. > * It also makes it harder for maintainers to distinguish an intentional contract tightening from an accidental regression later on. > * Best fix: > > * Add entries to both `docs/en/introduction/concepts/incompatible-changes.md` and `docs/zh/introduction/concepts/incompatible-changes.md`. > * Please spell out which transforms changed, which old configs are now rejected, and how users should update those configs. > * Severity: High > * Raised by others already: Yes. This is directly related to @zhangshenghang's earlier compatibility note; I am adding the code-level evidence for the current head. > > ### Issue 2: the ConditionExtension aggregation cleanup is still only partial > * Location: > > * `seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/adaptsink/DefineSinkTypeTransformFactory.java:67-99` > * `seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/jsonpath/JsonPathTransformFactory.java:65-102` > * `seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/regexextract/RegexExtractTransformFactory.java:67-87` > * `seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/validator/DataValidatorTransformFactory.java:67-99` > * contract reference: `docs/en/introduction/concepts/incompatible-changes.md:40-60` > * Why this matters: > > * The latest follow-up commit clearly tries to improve aggregation by converting `CopyField` and `TableFilter` to `description() + return false`. > * But the factories above still throw `OptionValidationException` directly from `ConditionExtension`, so validation still aborts on the first extension error there. > * In other words, the current head still has mixed behavior: plain constraints aggregate, but several extension validators remain fail-fast. > * Risk: > > * The migrated transforms still do not present a consistent validation UX. > * The commit message / PR scope now reads more complete than the actual behavior on the current head. > * Best fix: > > * Option A: convert the remaining extension validators to boolean/description-style failures as well, so the aggregation behavior is actually consistent. > * Option B: if that is outside the intended scope for this PR, narrow the stated scope and description so it does not imply the aggregation problem is solved across the board. > * Severity: Medium > * Raised by others already: Yes. This is related to @zhangshenghang's earlier aggregation point; I am confirming that it is still not fully closed on the latest head. > > ## Tests and CI > * The new `DynamicCompileTransformFactoryTest.java:83-104` coverage is valuable and directly closes the missing-key regression proof I was looking for on that path. > * The visible PR-level `Build` check is still shown as failed right now, but the check output points to the contributor fork run `nzw921rx/seatunnel#27736094115`, and that rerun is currently in progress. So I cannot call the current head green yet, but I also cannot attribute a fresh current-head failure to the transform code at this moment. > > ## Merge conclusion > ### Conclusion: can merge after fixes > 1. Blockers > > * Issue 1 above: please document the newly tightened transform validation behavior as incompatible changes, with migration notes, before merge. > > 2. Non-blocking suggestions > > * Issue 2 above: either finish the remaining aggregation cleanup in this PR, or narrow the claimed scope so it matches the actual implementation. > > Overall, the DynamicCompile regression that @zhangshenghang flagged is fixed now, so this head is materially better than the one I reviewed before. The remaining blocker is no longer in the code path itself; it is the release / compatibility contract that still needs to be documented explicitly. @DanielLeens Thanks for the review, The `OptionValidationException` thrown by extension operators is now handled uniformly at the framework layer in PR #11121. -- 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]
