SEZ9 commented on PR #12443:
URL: https://github.com/apache/seatunnel/pull/12443#issuecomment-6306766686
Thanks for the follow-up pushes (`f3f3a2d1`, `049f6e30`, and the current
head `3d6c855f`). I have not yet been able to verify the new diff myself, so
rather than mark anything resolved on description alone, could you help me
confirm a few points against the current head?
**F1 / F5 (fix undone by the `decryptConfig` re-parse; workaround private to
RestUtil):** The described approach — a shared helper that quotes only
parser-rejected keys, applied before each production `Map -> Config` conversion
— is exactly the shape I was hoping for. Could you point me to the helper and
each call site so I can confirm the second `parseMap` no longer loses keys like
`^t_nova_.*$`, and that REST and file loading now behave the same?
**F3 (parsable keys still rewritten — `->` split, quote stripping,
trimming):** If this is now documented as intended semantics ("keys the parser
accepts keep their meaning, rejected keys become literal"), I'm fine treating
it as such. Please confirm the docs state that `->` still splits and that users
must quote such keys in HOCON to keep them literal.
**F2 (REST docs):** Since the user-visible trigger is the REST JSON body,
could you confirm whether the REST API docs (en and zh) now describe or
cross-link the new key semantics?
**F4 / F6 (tests):** I can't tell from the thread what the current coverage
looks like. Concretely I'd like to see: (a) a regex key such as `^t_nova_.*$`
going through `ConfigShadeUtils.decryptConfig` end to end, (b) an escape
round-trip for keys containing `"` and `\`, (c) a top-level key, not only
nested ones, (d) a key whose only offending token is `$`, `:` or `${...}`, and
(e) a `->` key asserting the documented split behaviour. The existing "dotted
key" test doesn't hit the quoting path since `.` is not a separator in our
fork, so it can stay but shouldn't be the only case. An integration case for
the REST submit path that motivated the change would round this out.
**F7 (title prefix):** If production changes now span modules beyond
`seatunnel-engine-server`, `[Fix][Core]` is fine; please just confirm the
changed module set.
Once I can confirm the above and CI is green on the current head, I'm happy
to approve.
<!-- streview-comment:1909 -->
--
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]
