DanielLeens commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-6329655732
Thanks @nzw921rx for re-checking the current head (`82fc5e61ad`) so
carefully, and thanks @luozihen for the patience through these rounds. I
re-read both points against the code.
**1. Empty resolved key list: you are right, and I should have caught this.
Sorry for missing it in my earlier approvals.**
`JdbcMultiTableConfigConditionExtension` only checks that the `primary_keys`
map itself is non-empty. For `"^table1$" = []`, `toPrimaryKeyList()` returns an
empty list, `expandPrimaryKeyPlaceholder()` returns it unchanged, and
`applyPrimaryKeys()` calls `validatePrimaryKeyColumns()`, whose loop is a no-op
for an empty list. The sink then builds a `PrimaryKey` with no columns, and
`JdbcSink.createWriter()` reaches `getColumnNames().get(0)`
(`JdbcSink.java:145-146`), which throws `IndexOutOfBoundsException`. The `null`
element case is also real: `String.valueOf(element)` turns it into the column
name `"null"`, which `validatePrimaryKeyColumns()` does not reject.
This is an accepted config that fails with an obscure runtime error, so I
agree it should be fixed before merge. @luozihen, a small change should be
enough:
- Reject an empty resolved list for a matched table with `JDBC-12` (for
example at the top of `validatePrimaryKeyColumns()`, or earlier in
`toPrimaryKeyList()`), naming the pattern/table in the message.
- Reject non-string or `null` list elements in `toPrimaryKeyList()` with
`JDBC-12` instead of `String.valueOf`.
- Add unit tests in `JdbcSinkFactoryTest` for `[]`, `[""]` and a list
containing `null`.
**2. REST JSON submit endpoint.**
Confirmed: `ConfigBuilder.of(Map)` (`ConfigBuilder.java:107-110`) still uses
`ConfigFactory.parseMap(objectMap)`, so the JSON body of the submit-job REST
API rejects regex keys with `BadPath`. This PR documents it as a known
limitation (`docs/en/connectors/sink/Jdbc.md`, and the zh counterpart), and the
follow-up is tracked in #12434 ("REST submit-job with the JSON body rejects
regex option keys"). The PR already fixes the file-based path and the
`ConfigShadeUtils` rebuild through `rebuildWithLiteralKeys`, so the `of(Map)`
fix would be a small extension of the same helper.
On whether it should block this PR: that is a scoping call. My view is that
the documented limitation plus #12434 is acceptable, since config-file and
HOCON-body submission work and the failure is explicit rather than silent. If
you or the committers prefer to close the REST gap here, it is a one-line
change in `ConfigBuilder.of(Map)`, but the helper would then need a test for
the JSON-map path, so I would keep it in #12434 to keep this PR focused. I will
defer to the committers on this one; point 1 is the part I consider a must-fix.
@luozihen, once the empty/null validation and its tests are pushed, I will
re-review promptly.
--
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]
