SEZ9 commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-6431953601
@luozihen — apologies for the slow reply on the contract question; answering
it directly now.
1. Agreed on the parser point. Given `PATH_TOKEN_SEPARATOR` is `->` in this
repo, the row we disagreed on resolves the way you describe: `job.mode` /
`shade.identifier` stay literal through `parseMap`, `a->b` is the form that
expands, and `^t_.*$` is the one that actually breaks with `BadPath`. That
settles PR12015-F1 in favour of the narrow fix rather than the all-literal
rebuild, so I withdraw my objection to keeping the REST fix in this PR.
2. Quote-only-invalid is the contract I'm comfortable with: quote a key only
when it cannot be parsed as a path, and leave everything that parses today
untouched. Please make sure the implementation really is a no-op for every key
that parses today (dotted keys, already-quoted keys, `a->b`), so no existing
caller of `toConfig()` sees a change.
3. On tests (PR12015-F7), what I need to see before I can resolve this:
- the REST-shaped regression (`map -> RestUtil.buildConfig ->
ConfigShadeUtils.decryptConfig`) plus a direct `toConfig()` case, asserting
`shade.identifier`, `job.mode`, `a.b`, `a->b` and a regex key together in one
test so the final behavior is pinned in one place;
- the existing test's wording aligned with the `->` separator, as you
proposed.
4. Sequencing: since #12443 (head `509fddab`) carries the config-key
conversion, please land that first, then sync `dev` and drop `0c43f10dd` from
this branch so the two PRs don't ship competing conversions. The head here is
still `d9bb1574`, so my review stands until the rebased head is pushed; once it
is, I'll re-review the REST point and the test together.
@nzw921rx noted — fine to take this after 3.1.0; nothing here needs to rush
the release.
Nothing further is needed from you until #12443 merges. Ping me when the
rebased head is up.
<!-- streview-comment:2054 -->
--
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]
