luozihen commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-6355632069
Hi @hutiefang76 — thanks for the careful comparison. You are right that
these are two different key contracts rather than two copies of the same fix,
so it is worth agreeing on the contract and the ownership before either lands.
I ran the semantics against the shaded HOCON in this repo, because the
thread mixes upstream typesafe-config behaviour with this fork's:
| input key | ConfigFactory.parseMap | literal rebuild in this PR |
| --- | --- | --- |
| job.mode, shade.identifier | literal key kept, hasPath(...) is true |
identical |
| a->b | expanded to nested a -> b | one a->b key |
| " x " (quote characters in the key text) | normalized to ` x ` | kept as
`" x "` |
| ^t_.*$ | ConfigException$BadPath | kept as a literal key |
The first row matters for the thread: this repo patches the shaded parser
(#8975), so plain dotted keys are not expanded by parseMap here, and
env.hasPath("shade.identifier") is true both before and after this change. I
could not reproduce the REST shade consequence, but I agree it is exactly the
kind of thing that should be pinned rather than argued, so I will add the
REST-shaped test Daniel asked for (map -> RestUtil.buildConfig ->
ConfigShadeUtils.decryptConfig, with a regex key, job.mode, shade.identifier
and a->b).
The remaining deltas are the ones you named, and they are the contract
question. Items I would like us to agree on:
1. Key contract for the shared conversion: all keys literal (current
#12015), or quote only the keys that fail path parsing (#12443). I lean to the
second: it keeps a->b and the quoting behaviour byte-compatible with today and
only rescues keys that cannot be represented now.
2. Ownership: only one PR should change ConfigBuilder.of(Map),
backfillUserVariables and ConfigShadeUtils.processConfig; the other drops its
overlapping commit and rebases.
3. Scope: should ReadonlyConfig.toConfig(), the config-adapter path and the
variable-map conversions (currently only in #12443) follow the same contract,
or stay out of the shared change?
4. Merge order: whichever lands second removes its duplicate. 0c43f10dd here
is a single, self-contained commit and easy to drop.
5. Tests that pin the contract: the REST-shaped case above, asserting which
keys stay literal and which still expand.
Concretely, two ways to proceed:
- If you keep #12443 as the single shared fix with the quote-only contract,
I will drop 0c43f10dd from #12015 and rebase onto it once it lands; the JDBC
feature and its validation stay here.
- If the committers would rather have the REST path fixed in #12015
(@nzw921rx asked for that), I can switch of(Map) to quote-only-invalid here and
you rebase #12443 on top, keeping the extra conversions
(ReadonlyConfig.toConfig(), adapter, variable maps) in #12443.
Which of the two would you prefer? Once we agree on the contract and the
owner, I will make the change the same day.
--
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]
