Surface actual parse error in MergeYaml via Validated API#6818
Surface actual parse error in MergeYaml via Validated API#6818
Conversation
When MergeYaml.parse() fails, the actual parsing error (stored in a ParseExceptionResult marker on the ParseError) was being silently discarded by the Yaml.Documents instanceof filter, resulting in an unhelpful "Could not parse as YAML" message. Now the ParseExceptionResult message is extracted and included in the IllegalArgumentException, making it possible to diagnose the root cause (e.g. SnakeYAML syntax errors, print-idempotence failures).
|
We saw this somehow pass validation but fail during execution, so rewrote the logic to capture and forward any parse failure message. |
| try { | ||
| incoming = MergeYaml.parse(yaml); | ||
| return true; | ||
| } catch (IllegalArgumentException e) { |
There was a problem hiding this comment.
It feels strange to have an exception to signal failure. Failure is nothing special its expected in validation.
Also, creating an exception causes stack walking to fill the stacktrace and JIT deopt, maybe not very strong arguments in our case but still.
We could omit the exception and instead return the parsing error or erroneous and check with instaceof?
There was a problem hiding this comment.
or turn the parse into a validateParseable method and return a validation result insteadt?
Extract validateParseable() returning Validated<Yaml> so that validate() no longer uses try/catch for control flow, addressing PR review feedback.
The validation message changed from "Must be valid YAML" to "Could not parse as YAML: <details>" to surface the actual parse error.
|
@MBoegers any chance you could review? Or should I ask someone else? |
MBoegers
left a comment
There was a problem hiding this comment.
Interesting style to use the Validated<> API. I like!
Summary
MergeYamlencountered a YAML parsing failure, the oldmaybeParsemethod silently discarded theParseErrorresult (via aYaml.Documentsinstanceof filter), producing only a generic "Must be valid YAML" messageParseExceptionResultmarker and surfaces it through theValidatedAPI, making failures diagnosable (e.g. SnakeYAML syntax errors, print-idempotence failures)validateParseable()returningValidated<Yaml>so thatvalidate()uses the Validated API instead of exception-based control flowparse()delegates tovalidateParseable(), eliminating duplicated parsing logicTest plan
invalidYaml()test updated to match new validation messagesourceNull()test still passes with existing "Must be valid YAML" messageMergeYamlTesttests pass