SEZ9 commented on PR #12015: URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-6379498979
Thanks for the follow-up. The head is still `d9bb1574f0dfee9f6673533dff2616522a76e6c4`, so my latest review stands; this is a clarification on PR12015-F1 (and the related test gap in PR12015-F7), not a new review. **On the semantics table (F1).** The first row says `job.mode` / `shade.identifier` are kept as literal keys when the config is built from a map. My reading of the shaded config parser points the other way: keys supplied via a map are parsed as paths and split on `.`, including keys inside nested maps such as `env`. That is the same mechanism that makes a regex key like `^t_nova_.*$` fail with `BadPath`. I could not find a change in this PR that alters that parsing, so unless you can point me at the exact class that does, I have to treat expansion as the current behavior, which means the literal rebuild in `0c43f10dd` could stop an `env`-supplied `shade.identifier` from being resolved the way it is today. This is easiest to settle with a test rather than discussion, and you already proposed the right one: 1. Please add the REST-shaped test you described (map -> `RestUtil.buildConfig` -> `ConfigShadeUtils.decryptConfig`) with `env` containing `shade.identifier`, `job.mode`, `a.b` and one regex key, asserting which keys expand and which stay literal on the literal rebuild. If it is green I will withdraw F1 and say so here; if it is red, it tells us which direction to take. 2. Either way, please correct or confirm the first row of the semantics table so the doc and the test agree. 3. If the test is red, I'd lean toward the narrow option: keep every key that parses as a path today behaving exactly as it does now, and only rescue keys that fail path parsing. That keeps the change contained to the JDBC sink use case instead of altering `toConfig()` for every caller. Once the test and the table update are pushed, I'll re-check F1 and F7 against the new head. <!-- streview-comment:1995 --> -- 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]
