luozihen commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-6406668724
Hi @hutiefang76 — following up on the contract question from 23 Oct. I have
not heard back for a few days, so rather than leave this open I will proceed in
the direction @SEZ9 and @DanielLeens converged on, and I wanted to tell you
explicitly first.
1. The disputed row is settled by the parser itself.
ConfigParseOptions.PATH_TOKEN_SEPARATOR is "->" in this repo
(seatunnel-config-shade/.../ConfigParseOptions.java:23), not ".". That is why
parseMap keeps job.mode / shade.identifier as literal keys (and
env.hasPath("shade.identifier") is true), while a->b is the form that expands,
and why ^t_.*$ still fails with BadPath. I will pin this with the REST-shaped
regression and a direct toConfig() case, and align the wording of the existing
test with it.
2. I will keep the REST fix in this PR, but narrow it. @nzw921rx asked for
the REST JSON path to work here, and @SEZ9 / @DanielLeens both prefer the
narrow contract, so I will replace the all-literal rebuild with
quote-only-invalid: a key is quoted only when it cannot be parsed as a path,
everything that parses today keeps its current behavior (a->b still expands,
dotted keys and quoting behave as before), and regex keys become representable.
This is the narrow option we discussed, not the broad literal rebuild.
3. Ownership. ConfigBuilder.of(Map), backfillUserVariables and
ConfigShadeUtils.processConfig will share one narrow helper, so the contract
lives in one place. For #12443, I suggest it rebases onto this and keeps the
parts that go beyond this PR (ReadonlyConfig.toConfig(), the config-adapter
path and both variable-map conversions), dropping the overlapping pieces. If
you would rather keep #12443 as the only owner of the whole conversion, tell me
within the next two days and I will drop 0c43f10dd instead and rebase once
#12443 lands.
4. Tests in this PR. The REST-shaped case (map -> RestUtil.buildConfig ->
ConfigShadeUtils.decryptConfig) with shade.identifier, job.mode, a.b, a->b and
a regex key, asserting which keys expand and which stay literal; plus a direct
toConfig() case in ReadableConfigTest so both entry points are pinned.
Thanks — I would like to settle this and move the PR forward; if you
disagree with either the contract or the ownership, a short reply is enough and
I will adapt.
--
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]
