DanielLeens commented on PR #11496:
URL: https://github.com/apache/seatunnel/pull/11496#issuecomment-5476957547
Thanks for the independent re-review, @SEZ9 — this is genuinely useful,
since it traces the same head (`122e4a84c5df`) from a different angle than my
own review posted a few hours earlier. I went and checked each of your 8
findings against the actual current source (not from memory) before replying.
Summary: two are real, new, and not in my review — nice catch, especially Issue
6. Two overlap directly with what I already flagged. Two I believe are
incorrect once you trace the full call chain, and I'm including the concrete
evidence below rather than just asserting that. The rest are already-assessed
trade-offs from earlier rounds.
**Issue 1 (duplicate `-i` key) — the crash part is a dupe of my own Issue 2,
the NPE part doesn't reproduce.**
Agreed on the `Collectors.toMap` duplicate-key crash
(`ConfigBuilder.java:273-276`) — same root cause I flagged as my own Issue 2
this round, +1. On the NPE claim for `-i key=null`, though: `pair[1]` comes
from `variable.split("=", 2)` filtered to `pair.length == 2`
(`ConfigBuilder.java:261-264`). `String.split` never produces a `null` element,
so for `-i key=null` the literal three-character string `"null"` is what
reaches `parseUserValue`, not Java `null`. `parseUserValue`'s `value == null`
branch (line 314) is therefore dead code for this call site, and `.unwrapped()`
on the parsed result returns the string `"null"`, not `null` — no NPE. I
checked this same question in my own review this round too (Section 1.2), so
happy to compare notes if you have a repro that hits it a different way, but I
don't think `Collectors.toMap` NPEs here.
**Issue 2 (secret exposure via `withFallback`) — I think this is already
handled, and by design.**
I traced the full path again: `filterSourceConfig`
(`ConfigBuilder.java:434-446`) doesn't build `cleanSourceConfig` from the whole
`sourceConfig` — it only pulls in the keys that were actually collected into
`placeholders` during the `processConfigObject`/`processLeafValue` walk (i.e.
keys genuinely referenced via `${...}` somewhere in the job config). And
critically, right after the `withFallback(cleanSourceConfig).resolve(...)`
call, there's a cleanup pass:
```java
// ConfigBuilder.java:301-306
for (String key : placeholders) {
if (!config.hasPath(key) && resolvedConfig.hasPath(key)) {
resolvedConfig = resolvedConfig.withoutPath(key);
}
}
```
For every placeholder key that wasn't already present in the *original*
config (i.e. every key that only got there via the `withFallback` merge), this
strips it back out of `resolvedConfig` before it's returned. So `${db_pwd}`
substituting into `db.password` doesn't leave a root-level `db_pwd` key behind
— it gets removed because `config.hasPath("db_pwd")` is false. This mechanism
is unchanged across the many rounds I've reviewed, and it's specifically why I
didn't flag this in any prior round. If you can find a case where this cleanup
doesn't fire — e.g. a placeholder key that collides with a real config path, or
a nested-path edge case I'm missing — I'd genuinely like to see it, but from
the current source I can't reproduce the leak.
**Issue 3 (bracket exclusion in `PLACEHOLDER_PATTERN` default group) —
confirmed, this is real and new to me.**
Checked `PlaceholderUtils.java:32`: the default-value group is
`([^{}\[\]]*)`, which excludes `[`/`]`. Walking the regex engine against
`${key:[a,b]}` by hand: after matching `key`, the optional `(?::([^{}\[\]]*))?`
group tries to consume `:[a,b]`, but `[` isn't in the allowed default-value
class, so that group can only match zero characters after the `:`, and the
required trailing `\}` then fails to match `[` — so the entire optional group
backtracks out, then the bare `\}` fails to match `:` either. Net effect: the
whole pattern doesn't match `${key:[a,b]}` at all, `extractPlaceholderKeys`
returns nothing for it, and `replaceAllPlaceholders` leaves it as a literal,
unresolved string in the final config. The pre-PR
`ConfigBuilder.PLACEHOLDER_REGEX` this replaces used `[^}]*` for the default
group, which did allow brackets. This is a genuine, unflagged regression for
anyone using an array-shaped default (`${field_list:[id,name]}` and similar) —
good find, and I'll fold this in
to the blocker list rather than treat it as a nice-to-have, since it silently
leaves the raw placeholder text in the config rather than failing or warning.
**Issue 4 & 5 (`-i` values no longer exported via `System.setProperty`) —
already assessed and I still land the same way, but flag me if I'm missing a
concrete downstream consumer.**
This is the same removal I evaluated in my 2026-08-04 round: I called it "a
welcome change in isolation" because the old behavior leaked every submitted
job's `-i` variables into the JVM-wide system properties table, which is a real
cross-job contamination risk in Zeta's long-running server process (two jobs
submitted to the same JVM with a colliding `-i` key would stomp each other's
system property). The blocker I raised that round was about the *replacement
wiring* being broken, not about the removal itself, and that's been correctly
threaded ever since. If you have a specific downstream code path (a connector,
a plugin, or a second config-resolution pass) that actually reads
`System.getProperty(...)` for one of these keys today, that would change my
assessment — but as a general design point I think dropping the process-wide
side effect is the right call, not a regression to fix.
**Issue 6 (`insideQuotes` can get stuck true) — confirmed, and this is a
real gap in my own trace. Thank you.**
I checked `ParameterSplitter.java:67-71`:
```java
if (isStartWrapper) {
insideQuotes = true;
} else if (isEndWrapper) {
insideQuotes = false;
}
```
There's no `else` branch, so a `"` that matches neither `isStartWrapper` nor
`isEndWrapper` — e.g. a closing quote immediately followed by a non-delimiter
character like your `a="x"y,b=2` example — leaves `insideQuotes` completely
unchanged. Since the quote there was opened (`isStartWrapper` fired on the
first `"`), it stays `true` for the rest of the string, and the top-level comma
before `b=2` never splits. I traced this by hand against the current source and
it holds up. This is a real splitting regression I missed across all my prior
rounds on this class — folding it into the blocker list alongside my own Issue
1, since both are in the same file and both would ideally be fixed together
with one shared test class update.
**Issue 7 (brace-suppressed splitting is a behavior change for existing
users) — agreed it's a real behavior change, but I'd call it the intended
effect of this PR rather than a separate bug.**
Making `{`/`}` suppress comma-splitting (so `-i props={"a":"1","b":"2"}`
survives as one token instead of being torn apart at the internal comma) is
literally what this PR sets out to do — it's not a side effect of an unrelated
change. I agree it deserves a documentation callout for anyone whose existing
`-i foo={a,b}` invocation relied on the old two-token split, and I'll keep that
as a Low doc-only follow-up rather than a blocking behavior concern, since the
new behavior is the documented, intended one.
**Issue 8 (unbalanced opener silently swallows everything after it) — same
root cause as my own Issue 1, dedup.**
This is the general form of the exact bug I traced end-to-end with the
`key1={a:1,key2=val2` example in my own review (removed in `8c5b179cabec`, the
end-of-loop `braceDepth != 0 || bracketDepth != 0 || insideQuotes` guard). +1,
no new evidence needed on top of what's already in the blocker list.
**Where this leaves the blocker list:** combining both reviews and removing
the dupes/incorrect item, the confirmed blockers for this head are: (1)
`ParameterSplitter` unbalanced-delimiter validation removed (mine, matches your
Issue 8), (2) `Collectors.toMap` duplicate-key crash (yours, matches my Issue 2
— NPE claim not reproduced), (3) `insideQuotes` can get stuck true on a
non-wrapper-adjacent closing quote (yours, Issue 6, confirmed), and (4) the
bracket-excluding default-value regex in `PlaceholderUtils` (yours, Issue 3,
confirmed). I don't think the secret-exposure concern (your Issue 2) holds up
against the current cleanup logic, but please push back again if I'm missing
something there.
As always, I only have comment-level review rights here, so a write-capable
maintainer will need to do the final approve/merge once these land.
--
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]
