DanielLeens commented on PR #12443: URL: https://github.com/apache/seatunnel/pull/12443#issuecomment-6362899267
Thanks @SEZ9 for the careful follow-up. Since that note was written from the PR description, I checked each point directly against the current head `509fddabe` (only a merge of `dev` on top of `3d6c855f`; the 13 PR files are unchanged). This is code reading only, nothing was compiled or run by me. 1. **`ConfigShadeUtils.decryptConfig()` re-parse:** it is covered. `ConfigShadeUtils.java:229` is now `ConfigFactory.parseMap(ConfigMapPathUtils.quoteInvalidPathKeys(configMap))`, so the second materialised-Map parse on the REST path goes through the same guard. The other `Map -> Config` boundaries are `ReadonlyConfig.java:79` and `ConfigBuilder.java:111, 243, 264, 278, 292`. 2. **"Quote only on BadPath" contract:** yes, that is the intended contract, and it is now stated explicitly. `docs/en/engines/zeta/rest-api-v2.md` (Body section) says parsable keys keep the existing semantics (`a->b` becomes nested config, quotes are removed, surrounding unquoted whitespace is trimmed) and only keys the path parser rejects become literal. `docs/en/introduction/concepts/config.md` and both ZH counterparts carry the same text. It is deliberately not byte-for-byte key preservation, so existing jobs that rely on `->` keep working. 3. **Tests:** `ReadableConfigTest` and `ConfigBuilderTest#testInvalidPathKeysSurviveConfigShadeRoundTrip` cover the `^t_nova_.*$` key through the shade round trip. `RestUtilTest` covers `a->b` (line 56) and the `"`/`\` escaped key (line 79). `RestApiSubmitJobConfigShadeDecryptTest` submits through the real REST path including decrypt, and `RestApiIT` has the HOCON quoted-literal and JSON submission cases. One small nit you may want to see closed: the whitespace test at `ConfigBuilderTest.java:75` still uses the value label `"dotted"` for the `" job.mode "` key; it now exercises trimming rather than a separator, so a clearer label would avoid the confusion you raised. That is cosmetic and non-blocking. 4. **Docs:** done in EN and ZH, as above. 5. **Title:** the diff touches `seatunnel-api` and `seatunnel-core-starter` for production code (engine changes are test-only), so `[Fix][Core]` fits. There is no new commit since my last review, so I am not repeating a full pass. CI-wise, `Build` for this head is still pending in the contributor's fork run, so I cannot say it passes yet. @hutiefang76 thanks for the merge of `dev`; no action is needed from you on these points. Over to you @SEZ9 for the final pass over the current head. -- 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]
