luozihen commented on PR #12015: URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-6331227461
Thanks @nzw921rx for re-checking the head so carefully — both points were real, and both are fixed now. 1. Empty / malformed key columns. toPrimaryKeyList() now receives the pattern and rejects, with JDBC-12 naming the pattern: list elements that are not strings (including null, which used to be stringified into the column name "null"), and values that produce no column at all ([], "", " "). The per-table blank/comma checks in validatePrimaryKeyColumns() remain as the final guard, so [""] still fails with JDBC-12. New tests in JdbcSinkFactoryTest cover [] (both resolveMultiTablePrimaryKeys() and the createSink path that previously reached JdbcSink#createWriter()), "", [""], [null], [123], a non-list/non-string value, a null value, and a positive comma-separated string, asserting the JDBC-12 code. The negative tests fail with "nothing was thrown" against the previous implementation. 2. REST JSON body. Fixed here as well. ConfigBuilder.of(Map) — used by RestUtil.buildConfig() and buildConfigList() — now rebuilds with the same literal-key helper as the config-file and shade paths (ConfigBuilder#rebuildWithLiteralKeys) instead of ConfigFactory.parseMap, keeping the resolve / resolveWith(systemProperties) steps. ConfigBuilderTest#testOfMapPreservesLiteralRegexOptionKeys runs the exact REST sequence (map → of(Map) → ConfigShadeUtils.decryptConfig) and asserts the regex key and its values survive; it fails with ConfigException.BadPath before the change. The EN/ZH docs now state that regex keys work for config files and for both the JSON and HOCON REST bodies. On #12434 / #12443. With this change the REST JSON conversion is covered here, so #12434 is addressed by this PR. #12443 targets the same ConfigBuilder.of(Map) / ConfigShadeUtils conversion, so it would duplicate this fix and conflict in ConfigBuilder; it may be better to close it or put it on hold until this PR lands. If the maintainers would rather keep #12443 as the single REST fix, I can drop the REST commit from this PR instead — just tell me which way you prefer. CI is queued on the new head d9bb1574f (which includes a fresh merge of dev); I will post the result once it finishes. -- 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]
