SEZ9 commented on PR #12101:
URL: https://github.com/apache/seatunnel/pull/12101#issuecomment-5564618770
Thanks @CryoThrust for the follow-up in `0fec6dd35`, and @DanielLeens for
the full re-review.
On PR12101-F1: from both of your descriptions, the static-validation failure
branch now reuses `sanitizedMessage` for the thrown `ConfigCheckException`, and
`testJsonStaticFailureSanitizesThrownException` covers credentials embedded in
a JDBC URL. That is exactly the shape of fix I was looking for; I'll confirm it
against the diff before closing the finding. I also agree the
`InMemory`/multi-table test-compile issue is pre-existing and not something
this PR needs to carry.
The thread so far only covers F1, so could you confirm the status of the
remaining points? A one-liner per item ("done in `0fec6dd35`" / "not yet" /
"won't fix, because…") is fine:
- **F2 (JSON escaping):** are the sanitized messages emitted through a real
JSON serializer so quotes/newlines/control characters in error text can't break
the document?
- **F3 (null message):** is `sanitize(message)` guarded against a null
exception message before it is used for the thrown `ConfigCheckException`, not
only for the JSON payload?
- **F4 (stdout purity in JSON mode):** is stdout guaranteed to contain only
the JSON document on the failure path, or can the propagated
`ConfigCheckException` / text-mode logging still interleave?
- **F5 (`FileUtils.getConfigPath()` moved into the try):** does default text
mode still surface the same exception type and message for a
missing/unresolvable config file?
- **F6 (success JSON emitted inside the try):** has the success emission
moved out so a serialization failure isn't reported as a config validation
failure and can't throw again inside the catch?
- **F7 (docs):** is the `schemaVersion` / `valid` / `phase` / `errors`
contract documented in `user-command.md`?
- **F8 (style):** are the duplicated failure-JSON blocks consolidated and
the mixed `&&`/`||` condition explicitly grouped?
If any of these are already handled, just point me at where and I'll take a
look. I'd like at least F2, F3 and F4 addressed before merge since they affect
the correctness of the machine-readable output; F5–F8 can land here or in a
small follow-up, whichever you prefer.
<!-- streview-comment:865 -->
--
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]
