luozihen commented on PR #12015: URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-5770455151
Thanks for the very thorough pass — the log analysis, the CI breakdown and the four carryover findings are all clear. Here is how I handled each one; the changes are in 00acc23a582d76a589c00df956dded4b6f21da94. - Issue 1 (regex keys on the REST JSON path) — handled as your option A. You are right that the JSON body bypasses the fix: RestUtil.buildConfig / buildConfigList turn it into a map and hand it to ConfigBuilder.of(Map) before the shade rebuild, so ConfigFactory.parseMap(...) still rejects ^t_nova_.*$ with ConfigException.BadPath. To keep this PR focused I did not change that path here: I prototyped option B, verified it, and then reverted it so the diff stays scoped. Instead the limitation is now documented in the multi_table_config section of docs/en/connectors/sink/Jdbc.md and docs/zh/connectors/sink/Jdbc.md — regex keys work for config files and for the HOCON REST body, the JSON REST body is not supported yet and rejects the configuration while parsing it. I will open a follow-up issue for ConfigBuilder.of(Map) (rebuild the map with literal keys when a key is not a valid path, plus a test for the REST-shaped map) and link it here. For completeness: that change would only affect the of(Map) overload, which has exactly two production callers, both on the REST submit path (RestUtil.buildConfig / buildConfigList); every other ConfigBuilder.of(...) caller uses the Path/String overloads that this PR does not touch. - Issue 2 (legacy primary_keys branch) — fixed as you suggested. applyFallbackPrimaryKeys now rebuilds only the catalog primary key for the explicit top-level primary_keys case and leaves the option value untouched, exactly as on dev; the column validation and the comma-joined write happen only on the multi_table_config branch, where the value is computed. The shared catalog rebuild lives in buildCatalogTableWithPrimaryKey, so the two branches cannot drift. Guarded by JdbcSinkFactoryTest#testLegacyPrimaryKeysStillRebuildCatalogPrimaryKey. - Issue 3 (PR title) — fixed, the title now says multi_table_config. - Issue 4 (test for the processConfig round trip) — added. ConfigShadeTest#testDecryptPreservesOrdinaryConfigShapes covers an unquoted dotted key, a quoted dotted key inside a nested map and numeric values, and asserts that the env, source and sink blocks come back unchanged. I compare those blocks instead of the whole config because processConfig always writes an empty transform key (pre-existing behaviour), which would make a whole-config equality assertion fail for a reason unrelated to the round trip. - Issue 5 (mistyped static column) — left as a follow-up, as you suggested: validating names against the schema has to account for field_ide case conversion, so it is cleaner as its own change. It is recorded in the PR description. -- 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]
