DanielLeens commented on issue #11007: URL: https://github.com/apache/seatunnel/issues/11007#issuecomment-5137560820
Good question. I checked the current `connector-assert` path again. Yes, for this migration the first check can be removed from `AssertSink`: the top-level `rules` option is already declared as required in `AssertSinkFactory#optionRule()`, so keeping the same missing-option check again in the sink constructor is redundant after the declarative validation path is active. But the parsed-rule check should stay in `AssertSink`. That check is not just validating that `rules` exists. It protects the semantic case where `rules` is present but does not produce any effective assertion after parsing, for example no `row_rules`, no `field_rules`, no `catalog_table_rule`, and no `table_names`. `OptionRule` should not try to understand that nested Assert rule model. So the safe first slice is: 1. rely on `OptionRule.required(RULES)` for the top-level missing `rules` validation; 2. keep the existing `Assert rule config is empty` protection in `AssertSink`; 3. add/update focused tests for both cases: missing `rules` and present-but-empty/effectively-empty rules; 4. avoid changing the runtime assertion semantics in the same PR. That keeps the migration narrow and preserves the existing user-visible behavior. -- 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]
