SEZ9 commented on PR #12443:
URL: https://github.com/apache/seatunnel/pull/12443#issuecomment-5862052306
Thanks @hutiefang76 for the detailed update on `ddca53b7a`.
The approach you describe for F1/F5 — moving the guard into
`ConfigBuilder.of(Map)`, its adapter path, `ConfigShadeUtils.processConfig`,
and `ReadonlyConfig.toConfig()` — is the shape I was hoping for, since it
closes the re-parse in `ConfigShadeUtils.decryptConfig` after
`RestUtil.buildConfig` rather than patching around it in `RestUtil`. Keeping
`MetadataProviderManager` and `AssertSink` out is fine as long as the
description continues to say so. I'll confirm both once I've gone through the
updated diff.
Likewise for F4/F6: the `RestUtilTest` cases you list (top-level regex key,
nested `$`, `:`, `${FOO}`, quote and backslash keys through encrypt+decrypt,
plus the `->` and batch-submit cases) and
`RestApiSubmitJobConfigShadeDecryptTest#testSubmitJobWithRegexFieldInJson`
against `/submit-job` sound like they cover what I asked for. I'll verify
against the changed test files.
A few things before I do a final pass:
1. **F3:** your comment appears to have been cut off at "quote removal and
whitespace trimmin…" — could you finish that thought? I follow the reasoning
for quoting only on `BadPath` to keep `->` behaviour intact; what I'd like
written down is the intended rule for keys that do parse today but get
rewritten (embedded quotes stripped, surrounding whitespace trimmed). If the
answer is "accepted pre-existing behaviour, out of scope", that's fine, but
please state it explicitly so it's captured here.
2. **F2 (docs):** I don't see a mention of the `docs/en` / `docs/zh`
`rest-api-v2.md` update. Please add a short note describing the key semantics
for REST JSON bodies (reserved HOCON characters are literal; `->` remains a
path separator), or point me to it if it's already in.
3. **F7 (title):** since the change now also touches
`ConfigBuilder`/`ReadonlyConfig` and not only the engine server, `[Fix][Core]`
may well be the right tag after all. Could you confirm which modules the final
diff touches so we can settle the prefix?
Thanks again for the thorough work here.
<!-- streview-comment:1360 -->
--
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]
