SEZ9 commented on PR #12443:
URL: https://github.com/apache/seatunnel/pull/12443#issuecomment-6355834450
Thanks @hutiefang76 for the direct links — that makes it much easier to
follow.
From your description of `3d6c855f` (a shared `ConfigMapPathUtils` guard
used from `ReadonlyConfig.toConfig()` and the `ConfigBuilder` REST-map /
config-adapter / variable-backfill paths), that is the shape I was hoping for
on the "fix lives only in RestUtil" point — moving it into the shared
Map→Config path is the right direction. A few things I still need to close out
the earlier points:
1. **`ConfigShadeUtils.decryptConfig()` re-parse** — this was what undid the
fix on the real REST submit path. Does `decryptConfig` now go through one of
the guarded entry points, or does it still call `ConfigFactory.parseMap`
directly on its own materialised Map? If it's covered, please point me at
where; if not, it still needs wiring.
2. **"Quote only on BadPath" semantics** — since the guard quotes only keys
the path parser rejects, keys that *parse* but get rewritten (`->` split into
nested paths, embedded quotes stripped, surrounding whitespace trimmed) are
still not preserved literally. Is that the intended contract? If so, it should
be stated explicitly; if not, the guard needs to compare the parsed path back
against the original key rather than relying solely on `BadPath`.
3. **Tests** — please confirm the new cases include: a regex key
(`^t_nova_.*$`) through the full REST submit path including `decryptConfig`; a
key containing `"` and `\` round-tripping; a top-level key (not just nested); a
key whose only offending token is `$`, `:` or `${...}`; and a `->` key. An
`RestApiIT` case for the submit path that motivated the PR would be ideal. I'd
also drop or rename the "dotted key" test, since `.` isn't a separator in
SeaTunnel's fork and so it doesn't hit the new code.
4. **Docs** — `docs/en` and `docs/zh` `rest-api-v2.md` still need a short
note on the resulting key semantics (reserved HOCON characters now literal;
whatever we decide for `->`).
5. **PR title** — if the change now touches `seatunnel-api` and
`seatunnel-core-starter` as well, `[Fix][Core]` may be the right tag after all;
I'll confirm against the diff. Please make sure the PR description reflects the
modules touched.
Once I have an answer on (1) and (2), I'll do a full pass over `3d6c855f`.
<!-- streview-comment:1959 -->
--
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]
