YAML Error Handling #2747
YAML Error Handling #2747
Conversation
…ed contexts - Replaced `ParsingException` with `YamlParsingException` for improved error details. - Introduced `YamlErrorRenderer` for visual error markers in YAML reports. - Added support for nested parsing contexts in `YamlParsingException`. - Improved validation logic and error messages in `NodeValidationUtils` and `MethodSetter`. - Updated `GenericYamlParser` to enhance schema validation handling.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request overhauls YAML parsing and error reporting: introduces Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as RouterCLI
participant Parser as GenericYamlParser
participant Registry as BeanRegistry
participant Renderer as LineYamlErrorRenderer
participant Colors as TerminalColorsMini
CLI->>Parser: parse config (create ParsingContext)
Parser->>Registry: resolve beans / classes
Parser--x CLI: throws ConfigurationParsingException (with ParsingContext)
CLI->>Renderer: renderErrorReport(parsingContext)
Renderer->>Renderer: use JsonPath to mark wrong node
Renderer->>Colors: request color codes (RESET/RED/CYAN/BOLD)
Renderer-->>CLI: formatted colored YAML report
CLI->>CLI: print report to stderr (colored)
Note: colored boxes in renderer/terminal steps use color helpers internally (rgba used in implementation). Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java`:
- Around line 234-238: Remove the leftover debug assignments `var i = "dummy"`
in GenericYamlParser.java: locate the occurrences inside the method that throws
YamlParsingException (around the throw statement referencing
YamlParsingException(cause, node.get(key))) and simply delete those redundant
variable declarations so they no longer appear before or after the throw; ensure
no other logic is altered and the method still compiles.
- Around line 95-107: Rename the misspelled method validateAgaistSchema to
validateAgainstSchema throughout the class so callers and references match;
update the method declaration name and any invocations or commented-out
references (e.g., the call near the YAML parsing logic) as well as any
exceptions or Javadoc that mention the old name to use validateAgainstSchema,
ensuring the signature (Grammar grammar, JsonNode jsonNode, JsonLocationMap
jsonLocationMap) and behavior remain unchanged.
In `@annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java`:
- Around line 155-156: The thrown YamlParsingException in MethodSetter builds
its message with three "%s" placeholders but passes four arguments (key, key,
wanted.getSimpleName(), key.toUpperCase()), so the extra key.toUpperCase() is
ignored; fix by removing the surplus argument and ensure the message arguments
match the format string (update the throw in MethodSetter where new
YamlParsingException(...) is constructed to pass only key, key, and
wanted.getSimpleName()).
In `@annot/src/main/java/com/predic8/membrane/annot/yaml/ParsingContext.java`:
- Around line 69-71: The addPath method currently returns a raw ParsingContext
causing unchecked assignments and loss of the registry's generic type; change
its signature to return ParsingContext<T> (i.e. public ParsingContext<T>
addPath(String path)) and instantiate the new context with the diamond operator
to preserve generics (new ParsingContext<>(context, registry, grammar, topLevel,
this.path + path)); ensure the ParsingContext class declaration and constructor
are generic on T so the returned instance carries the same T as this.registry.
In `@annot/src/main/java/com/predic8/membrane/annot/yaml/YamlErrorRenderer.java`:
- Around line 133-193: The multi-line branch in YamlErrorRenderer (in the loop
that copies nested content after detecting MARKER_KEY) stops on blank lines
because getIndentation(blankLine) is 0, prematurely ending the while; modify the
while condition in the block handling the multi-line marker (the loop that uses
getIndentation(lines[i]) > markerIndent) to also accept blank lines as part of
the nested content (e.g. treat lines[i].trim().isEmpty() as continuing the
loop), and update the loop body so that when contentLine.trim().isEmpty() you
still emit an appropriately indented blank line (instead of trimming it away)
before continuing; references: MARKER_KEY, ERROR_POINTER, getIndentation, and
the multi-line handling code in YamlErrorRenderer.
In
`@annot/src/main/java/com/predic8/membrane/annot/yaml/YamlParsingException.java`:
- Around line 99-101: getFormattedReport() dereferences parsingContext and will
NPE when parsingContext is null; add a null guard in
YamlParsingException.getFormattedReport() that checks if parsingContext is null
and returns a safe default (e.g. a short message like "No parsing context
available" or the exception key) instead of calling
YamlErrorRenderer.renderErrorReport, otherwise continue to call
YamlErrorRenderer.renderErrorReport(parsingContext.getToplevel(),
parsingContext.path(), key); ensure this handles all instances created by the
constructors that don't set parsingContext.
In `@annot/src/main/java/com/predic8/membrane/common/TerminalColorsMini.java`:
- Around line 45-48: The detectAnsiSupport() logic is inverted: when
MEMBRANE_DISABLE_TERM_COLORS_PROPERTY is absent colors should be enabled. Update
detectAnsiSupport() so it returns true when the system property is not set or is
set to "false", and returns false only when the property is explicitly "true";
i.e., treat a null prop as "false". Modify the return logic in
detectAnsiSupport() (referenced by MEMBRANE_DISABLE_TERM_COLORS_PROPERTY) to
reflect that behavior.
🧹 Nitpick comments (8)
annot/src/main/java/com/predic8/membrane/common/TerminalColorsMini.java (2)
4-7: Acknowledged TODO: duplicated code from core.The comment flags this as a duplication of a class in core. Consider extracting a shared module or moving this to a common dependency to avoid drift between the two copies.
Would you like me to open an issue to track consolidating the duplicated
TerminalColorsimplementations?
25-43: Nit: Method names use constant-style ALL_CAPS.Java convention reserves
ALL_CAPSforstatic finalfields; methods typically usecamelCase(e.g.,red(),reset()). This is a minor readability nit — the current naming likely mimics constant usage at call sites, so feel free to keep if that's the intent.annot/src/main/java/com/predic8/membrane/annot/yaml/YamlParsingException.java (2)
14-14: Unused fieldcontext.The
contextfield is assigned in one constructor (line 21) but has no getter, no setter, and is never read. If it's not needed, remove it to avoid confusion withparsingContext.♻️ Proposed removal
private String nodeName; - private String context; private ParsingContext<?> parsingContext;And update the constructor accordingly:
- public YamlParsingException(String message, JsonNode node, String key, String context) { + public YamlParsingException(String message, JsonNode node, String key) { super(message); this.node = node; this.key = key; - this.context = context; }
71-79: Commented-out code ingetRelevantNode().The commented-out block on lines 75–77 should either be implemented or removed. Leaving it in creates ambiguity about the intended behavior.
annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (1)
86-98:getConfigElementNamehas issues — consider reusingMcYamlIntrospector.getElementName.Several concerns with this new helper:
- Raw type:
Classshould beClass<?>.MCChildElementis a method-level annotation, soclazz.getAnnotation(MCChildElement.class)will always returnnullfor a class — this branch is dead code..toString()on the annotation would return the annotation proxy's string representation (e.g.,@MCChildElement(…)), not a user-friendly name.- Duplication:
McYamlIntrospector.getElementName(Class<?>)(line ~200 inMcYamlIntrospector.java) already provides theMCElement.name()→getSimpleName()fallback.Proposed fix — delegate to existing utility
- private static String getConfigElementName(Class clazz) { - Annotation childAnnotation = clazz.getAnnotation(MCChildElement.class); - if (childAnnotation != null) { - return ((MCChildElement) childAnnotation).toString(); - } - - Annotation mcAnnotation = clazz.getAnnotation(MCElement.class); - if (mcAnnotation != null) { - return ((MCElement) mcAnnotation).name(); - } - - return clazz.getSimpleName(); - } + private static String getConfigElementName(Class<?> clazz) { + return McYamlIntrospector.getElementName(clazz); + }annot/src/main/java/com/predic8/membrane/annot/yaml/YamlErrorRenderer.java (1)
55-121: Good structure for the happy path; consider resilience of the catch-all.The deep-copy + marker-injection approach is sound. One concern: on line 118–120, wrapping every exception in a bare
RuntimeExceptioncould mask the original YAML parsing error that triggered reporting. If the renderer fails, the user loses the original error context.Consider logging the original error or letting it propagate instead:
Proposed fallback
} catch (Exception e) { - throw new RuntimeException("Failed to render error report: " + e.getMessage(), e); + // Rendering failure should not hide the original parse error + return "(Could not render error report: " + e.getMessage() + ")"; }annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
208-215:catch (Throwable)will also wrapOutOfMemoryErrorand similar inYamlParsingException.This is a common trade-off in config parsers. If you want to preserve the ability for JVM-fatal errors to propagate, consider narrowing to
catch (Exception)and re-adding the existingNoClassDefFoundErrorhandling.annot/src/main/java/com/predic8/membrane/annot/yaml/ParsingContext.java (1)
87-91: Remove commented-out code.Line 90 retains the old
RuntimeExceptionthrow. Since the migration toYamlParsingExceptionis intentional, this dead comment should be cleaned up.Proposed fix
if (clazz == null) { - var e = new YamlParsingException("Did not find java class for key '%s'.".formatted(key), null, key, context); - throw e; -// throw new RuntimeException("Did not find java class for key '%s'.".formatted(key)); + throw new YamlParsingException("Did not find java class for key '%s'.".formatted(key), null, key, context); }
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java
Outdated
Show resolved
Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java
Outdated
Show resolved
Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java
Outdated
Show resolved
Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/ParsingContext.java
Outdated
Show resolved
Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/YamlErrorRenderer.java
Outdated
Show resolved
Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/YamlParsingException.java
Outdated
Show resolved
Hide resolved
|
This pull request needs "/ok-to-test" from an authorized committer. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
323-330:⚠️ Potential issue | 🟠 MajorFix inconsistent
throwsclause in overloaded method.The 3-param overload (line 327) declares
throws ParsingException, but the private method it delegates to (parseListExcludingStartEvent, line 331) declaresthrows YamlParsingException. SinceYamlParsingExceptionextendsRuntimeException(unchecked), it propagates regardless, but the throws clause should be updated toYamlParsingExceptionfor consistency with the 2-param overload and to accurately reflect what the method actually throws.
🤖 Fix all issues with AI agents
In `@annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java`:
- Around line 229-233: The catch block in GenericYamlParser currently
unconditionally calls e.setNode(node) and e.setNodeName(key), which can
overwrite more specific location info set deeper (e.g., by resolveReference or
coerceTextual); change the logic to only set the node and nodeName when they are
not already present on the YamlParsingException (i.e., check
YamlParsingException::getNode()/getNodeName() for null/empty before calling
e.setNode(node) or e.setNodeName(key)) so that existing, more precise location
data is preserved while still populating missing context from the parent.
In `@annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java`:
- Around line 86-98: Update getConfigElementName: change the parameter type from
Class clazz to Class<?> clazz to avoid raw-type warnings; remove the unreachable
clazz.getAnnotation(MCChildElement.class) branch (MCChildElement is
method-targeted) and stop calling toString() on annotations—use MCElement.name()
when present, otherwise return clazz.getSimpleName(); if you ever need
MCChildElement handling, retrieve its .name() from the method-level annotation
rather than calling toString().
🧹 Nitpick comments (2)
annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (1)
73-73: Incomplete migration: line 73 still throwsRuntimeExceptioninstead ofYamlParsingException.The catch block on line 72–74 wraps grammar lookup failures in a plain
RuntimeException, while the analogous error at lines 77–81 correctly usesYamlParsingException. The Javadoc on line 48 also still documentsRuntimeException.Proposed fix
} catch (Exception e) { - throw new RuntimeException("Can't find method or bean for key '%s' in %s".formatted(key, getConfigElementName(clazz)), e); + throw new YamlParsingException("Can't find method or bean for key '%s' in %s".formatted(key, getConfigElementName(clazz)), null, key, ctx.context()); }And update the Javadoc on line 48:
- * Throws a {`@link` RuntimeException} if neither a matching setter nor a resolvable bean class can be found. + * Throws a {`@link` YamlParsingException} if neither a matching setter nor a resolvable bean class can be found.annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
159-163: Unnecessary intermediate variable.The
var eand blank line add noise without benefit.Proposed simplification
if (clazz == null) { - var e = new YamlParsingException("Did not find java class for kind '%s'.".formatted(kind), node); - - throw e; + throw new YamlParsingException("Did not find java class for kind '%s'.".formatted(kind), node); }
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java
Outdated
Show resolved
Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java
Outdated
Show resolved
Hide resolved
…onParsingException` and update related classes - Replaced `YamlParsingException` with `ConfigurationParsingException` for better clarity in exception usage. - Updated `GenericYamlParser` and test cases to align with the new exception class. - Simplified exception handling and removed redundant YAML-related logic.
…n parsing methods - Eliminated redundant parameters from `resolveSpelValue` and `replaceMarkerWithError` methods. - Improved clarity in conditionals flagged as always true or false, annotated with TODO comments for future review. - Streamlined exception handling in `GenericYamlParser` and optimized parsing utility logic.
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In
`@annot/src/main/java/com/predic8/membrane/annot/yaml/ConfigurationParsingException.java`:
- Around line 28-30: getFormattedReport() can NPE because parsingContext is set
via a setter and may be null; modify
ConfigurationParsingException.getFormattedReport() to check parsingContext for
null before calling YamlErrorRenderer.renderErrorReport(parsingContext) and
return a safe fallback (e.g. an empty string or a short message like "No parsing
context available") when null. Ensure you reference the parsingContext field and
the getFormattedReport() method, and keep
YamlErrorRenderer.renderErrorReport(parsingContext) only when parsingContext !=
null so callers such as RouterCLI.getRouter() no longer get an NPE.
- Around line 5-15: Add a new constructor to ConfigurationParsingException that
accepts (String message, Throwable cause) and forwards both to the superclass
(i.e., call super(message, cause)) so callers like
YamlParsingUtils.resolveSpelValue can preserve descriptive messages and the
original exception chain; update any constructors if necessary to keep the
existing parsingContext field behavior but do not remove or change other
constructors or fields.
In `@annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java`:
- Line 83: The code creates ParsingContext with a null registry in
GenericYamlParser (e.g., new ParsingContext<>("", null, grammar, jsonNode, "$")
and the raw-type instance at the second site), which is fragile; change these
sites to use a clear factory method such as ParsingContext.validationOnly(...)
(or a static helper on GenericYamlParser) that returns a ParsingContext with an
explicit "no-registry" sentinel and correct generic type, and replace the raw
new ParsingContext(...) at the second site with the diamond form new
ParsingContext<>(...) or, better, the same validationOnly(...) factory so
downstream ctx.registry() checks against the sentinel instead of NPEing. Ensure
the factory documents intent and the sentinel is handled safely by code that
calls ctx.registry().
In `@annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java`:
- Around line 70-80: Replace the RuntimeException thrown inside the catch in
MethodSetter (the block that calls getChildSetter(clazz, beanClass)) with a
ConfigurationParsingException: create the ConfigurationParsingException using
the same formatted message, call e.setParsingContext(ctx.key(key)), and throw
that exception with the original Exception as its cause so parsing context and
root cause are preserved; also update or remove the misleading TODO about
"Always false" since getAnySetter can return null and the condition beanClass ==
null && setter == null is reachable.
In `@annot/src/main/java/com/predic8/membrane/annot/yaml/YamlErrorRenderer.java`:
- Around line 74-80: In YamlErrorRenderer, do not silently fall back to the root
when an object field is missing: inside the block handling ObjectNode (where
parent instanceof ObjectNode parentNode and targetNode = parentNode.get(key)),
replace the temporary assignment targetNode = node with explicit error
handling—throw an IllegalArgumentException (or a custom exception) that includes
the missing key and jsonPath (e.g., "Key '<key>' not found in parent at:
<jsonPath>") so the calling code can surface the correct location;
alternatively, if a non-throwing path is required, return a clear
Optional/Result or set targetNode to null and handle that case downstream in the
render logic to avoid misplacing error markers.
- Line 59: Remove the debug System.out.println in YamlErrorRenderer (the line
printing "node = " + node + ", jsonPath = " + jsonPath + ", key = " + key);
either delete it or replace it with a proper logger call using LoggerFactory
(e.g., logger.debug(...)) in the YamlErrorRenderer class so stdout isn't
polluted in production; ensure you import and use the class-level logger
(LoggerFactory.getLogger(YamlErrorRenderer.class)) and log at an appropriate
level (debug/trace) if diagnostic output is still needed.
In `@annot/src/main/java/com/predic8/membrane/annot/yaml/YamlParsingUtils.java`:
- Around line 116-122: The catch block in resolveSpelValue currently throws new
ConfigurationParsingException with only a message and discards the original
RuntimeException (e), losing the stack trace from eval(expression, SPEL_CTX);
add a (String message, Throwable cause) constructor to
ConfigurationParsingException and update resolveSpelValue to pass e as the cause
(i.e., throw new ConfigurationParsingException("Invalid SpEL expression:
%s".formatted(e.getMessage()), e)) so the original exception and stack trace are
preserved.
In `@annot/src/test/java/com/predic8/membrane/annot/YAMLParsingReportTest.java`:
- Around line 356-373: The assertion expecting key "c" is wrong (copy-paste);
update the test in YAMLParsingReportTest (the try/catch that calls parseYAML and
uses getCause()/pc.getKey()) to assert the actual YAML key that causes the parse
error (replace assertEquals("c", pc.getKey()) with assertEquals("type",
pc.getKey()) so it matches the YAML input's offending key).
In `@annot/src/test/java/com/predic8/membrane/common/ExceptionUtil.java`:
- Around line 3-13: Remove the duplicate getRootCause method from annot's
ExceptionUtil class and instead add the core module as a test dependency
(org.membrane-soa:service-proxy-core) in annot/pom.xml; then update code that
used com.predic8.membrane.common.ExceptionUtil.getRootCause to import and call
com.predic8.membrane.core.util.ExceptionUtil.getRootCause so callers reference
the single implementation in the core module.
In `@core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java`:
- Around line 132-139: RouterCLI's catch for ConfigurationParsingException
currently risks losing the original parsing error when
getFormattedReport()/renderErrorReport() throws (NPE or JsonProcessingException)
and rethrows only the new exception; change the handler in RouterCLI (catching
ConfigurationParsingException e) to always log the original exception message
and stacktrace (use log.error with e), then attempt to produce the formatted
report via getFormattedReport()/renderErrorReport() inside a secondary try/catch
that catches Throwable (including NPE and JsonProcessingException) and, on
failure, logs a safe fallback message containing the original e.getMessage() and
the secondary exception as suppressed or additional context (do not replace or
discard the original exception), and avoid throwing a new RuntimeException that
hides the original parsing error.
In `@docs/ROADMAP.md`:
- Around line 28-29: Update the sentence "the apis.yaml is read last and
therefore overwrites previous definitions" to start with a capital letter and/or
format as a bullet, e.g., "The apis.yaml is read last and therefore overwrites
previous definitions (e.g., fruitshop.apis.yaml, dlp.apis.yaml)." Also rewrite
the unclear fragment "with global and matches all" to a clear explanation such
as "the final apis.yaml typically contains global rules that apply to all
services (i.e., it matches all routes/services)." Replace the original phrases
exactly so you update "the apis.yaml is read last and therefore overwrites
previous definitions" and "with global and matches all" in ROADMAP.md.
🧹 Nitpick comments (14)
docs/ROADMAP.md (1)
30-31: Consider reducing excessive blank lines.Two consecutive blank lines appear here. Typically, one blank line is sufficient for spacing in Markdown documents.
annot/src/test/java/com/predic8/membrane/annot/YAMLParsingReportTest.java (3)
157-157: RemoveSystem.out.printlndebug statements before merging.Multiple debug print statements scattered across tests (Lines 157, 171, 217, 233, 290, 369). Consider using a logger or removing them.
Also applies to: 171-171, 217-217, 233-233, 290-290, 369-369
293-293: PreferassertNulloverassertEquals(null, ...).- assertEquals(null, pc.getKey()); + assertNull(pc.getKey());
294-301: Consider removing commented-out code blocks.Lines 294–301 and 375–396 contain commented-out test code. If these represent future work, a TODO comment or a tracked issue would be more discoverable.
Also applies to: 375-396
annot/src/main/java/com/predic8/membrane/annot/yaml/YamlErrorRenderer.java (1)
30-46:ERROR_POINTERis evaluated once at class-load time — terminal color toggle changes won't be reflected.
ERROR_POINTERis astatic finalfield whose value is computed during class initialization. If themembrane.disable.term.colorsproperty is set after this class is loaded, the pointer string will still contain (or lack) ANSI codes from the initial evaluation. If this is intentional, a brief comment would help; otherwise, consider making it a method.Alternative: lazy evaluation via method
- public static final String ERROR_POINTER = RED() + "^^^ ERROR\n" + CYAN(); + public static String errorPointer() { + return RED() + "^^^ ERROR\n" + CYAN(); + }annot/src/main/java/com/predic8/membrane/annot/yaml/NodeValidationUtils.java (1)
21-44: Inconsistent context attachment across validation methods.Only
ensureSingleKeyreceives aParsingContextand attaches it to the exception. The other methods (ensureMappingStart,ensureTextual,ensureArray) throw a bareConfigurationParsingExceptionwithout context, which means the error report renderer won't be able to pinpoint the YAML location for those failures.If callers are expected to catch and enrich these exceptions, this is fine but fragile — consider passing
ParsingContextto all validators for consistency, or document the expectation.annot/src/main/java/com/predic8/membrane/annot/yaml/ParsingContext.java (3)
73-78: Remove commented-out code.Line 77 contains a commented-out
RuntimeExceptionthat was replaced. This should be cleaned up before merging.Proposed fix
if (clazz == null) { var e = new ConfigurationParsingException("Did not find java class for key '%s'.".formatted(key)); e.setParsingContext(this); throw e; -// throw new RuntimeException("Did not find java class for key '%s'.".formatted(key)); }
82-88:getNode()andgetToplevel()are identical — one should delegate or be removed.Both return
topLevel. Having two methods with different names for the same field is confusing. If the intent was forgetNode()to navigate to a sub-node viapath, consider implementing that; otherwise, drop one.
22-33: Javadoc claims "Immutable" butkeyis a mutable field.The class-level doc says "Immutable parsing state" (line 22), yet
key(line 33) is non-final and mutated via thekey()factory method on a new instance. The doc is misleading — either makekeyfinal (set via constructor) or update the Javadoc.annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (2)
184-184: RawParsingContexttype (missing<?>) in three method signatures.
parseNumericOrThrow(line 184),getObjectList(line 207), andresolveReference(line 225) use rawParsingContextwhile the rest of the file consistently usesParsingContext<?>. This generates unchecked warnings.Proposed fix
- private Object parseNumericOrThrow(ParsingContext pc, String key, Class<?> wanted, String value, JsonNode node) { + private Object parseNumericOrThrow(ParsingContext<?> pc, String key, Class<?> wanted, String value, JsonNode node) {- private `@Nullable` List<Object> getObjectList(ParsingContext ctx, JsonNode node, String key, Class<?> wanted) { + private `@Nullable` List<Object> getObjectList(ParsingContext<?> ctx, JsonNode node, String key, Class<?> wanted) {- private static `@NotNull` Object resolveReference(ParsingContext ctx, JsonNode node, String key, Class<?> wanted) { + private static `@NotNull` Object resolveReference(ParsingContext<?> ctx, JsonNode node, String key, Class<?> wanted) {Also applies to: 207-207, 225-225
229-232: Remove commented-out code in theConfigurationParsingExceptioncatch block.These leftover lines obscure the intent. If the context-enrichment logic is deferred, track it with a TODO or issue instead.
Proposed fix
} catch (ConfigurationParsingException e) { -// if (e.getParsingContext() != null) -// e.setParsingContext(e.getParsingContext().key(key)); throw e; }annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (3)
229-233: Remove commented-out context-enrichment code.Same pattern as in
MethodSetter— leftover commented-out logic in theConfigurationParsingExceptioncatch block. Clean up or track with an issue.Proposed fix
} catch (ConfigurationParsingException e) { -// if (e.getParsingContext() != null) -// e.setParsingContext(e.getParsingContext().key(key)); throw e; }
353-353: Remove debug comment// Check!.This looks like a review/debugging artifact that should be cleaned up.
Proposed fix
- var c = ctx.addPath("." + key); // Check! + var c = ctx.addPath("." + key);
80-81: Schema validation is disabled — track re-enablement.The comment says "Deactivated temporarily to get better error messages" but there's no tracking issue referenced. Consider adding a TODO with a ticket reference so this doesn't get forgotten.
annot/src/main/java/com/predic8/membrane/annot/yaml/ConfigurationParsingException.java
Show resolved
Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/ConfigurationParsingException.java
Show resolved
Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java
Outdated
Show resolved
Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/YamlErrorRenderer.java
Outdated
Show resolved
Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/YamlParsingUtils.java
Show resolved
Hide resolved
annot/src/test/java/com/predic8/membrane/annot/YAMLParsingErrorTest.java
Show resolved
Hide resolved
- Added missing Apache License headers to multiple classes. - Introduced `updateContext` in `ParsingContext` for enhanced YAML parsing context tracking. - Fixed formatting and improved error handling logic in `YamlErrorRenderer`. - Applied minor code cleanups in `MethodSetter` and `GenericYamlParser`.
…ated YAML parsing test blocks - Cleaned up unused debug statements in `YAMLParsingReportTest`. - Removed obsolete and commented-out test sections to simplify and improve readability. - Adjusted YAML error context validation to reflect updated parsing behavior.
…orRenderer` - Replaced `YamlErrorRenderer` with `LineYamlErrorRenderer` for improved line-based error rendering in YAML context. - Removed obsolete `YamlErrorRenderer` class and its dependencies. - Refactored `MethodSetter` and `ParsingContext` to enhance error handling and context tracking. - Updated test cases to align with the new error rendering approach and improved YAML parsing validation logic.
…remove unused constants - Replaced `assertEquals(null, ...)` with `assertNull(...)` for better readability. - Updated `ParsingContext#toString` with a modern `formatted` multi-line string. - Removed unused `BASE_INDENT` constant from `LineYamlErrorRenderer`.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java`:
- Around line 189-192: The error message uses a "%s" placeholder but it's never
substituted; in GenericYamlParser inside the isNoEnvelope check replace the
literal format string with a formatted one (e.g. String.format or
String#formatted) so the class name is inserted, e.g. call new
ConfigurationParsingException(String.format("Class %s is annotated with
`@MCElement`(noEnvelope=true), but the YAML/JSON structure does not contain a
list.", clazz.getName()), null, pc) (or use ".formatted(clazz.getName())"),
keeping the same null and pc arguments.
In `@annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java`:
- Around line 215-231: The ConfigurationParsingException instances thrown in
getObjectList and resolveReference need the ParsingContext attached so error
rendering works; instead of throwing directly, instantiate the
ConfigurationParsingException, call setParsingContext(ctx) on it, then throw it
(e.g., in getObjectList where you currently throw for elemType mismatches and in
resolveReference where reference errors are thrown). Locate the throws inside
methods getObjectList and resolveReference and change them to create the
exception variable, call exception.setParsingContext(ctx), then throw that
exception.
In `@annot/src/test/java/com/predic8/membrane/annot/YAMLParsingErrorTest.java`:
- Around line 170-175: In YAMLParsingErrorTest catch block, avoid calling
c.getParsingContext() twice: assign c.getParsingContext() to pc (as already
done) and change the assertEquals call to use pc.path() instead of
c.getParsingContext().path(); update the assertion that currently reads
assertEquals("$.demo", c.getParsingContext().path()) to assertEquals("$.demo",
pc.path()) so the test consistently uses the local pc variable returned by
getParsingContext().
🧹 Nitpick comments (14)
annot/src/main/java/com/predic8/membrane/annot/yaml/ParsingContext.java (2)
84-89: Remove commented-out code.Line 88 contains a commented-out
throw new RuntimeException(...)that was the predecessor of the newConfigurationParsingException. This is dead code that should be removed before merge.Proposed fix
if (clazz == null) { var e = new ConfigurationParsingException("Did not find java class for key '%s'.".formatted(key)); e.setParsingContext(this); throw e; -// throw new RuntimeException("Did not find java class for key '%s'.".formatted(key)); }
93-99:getToplevel()andgetNode()are identical — consider removing one.Both methods return the same
topLevelfield. Having two accessors with different names for the same data is confusing. Pick one canonical name and remove the other, or document the distinction if one is intended.annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java (1)
85-98:getSingleChildSetterusesConfigurationParsingExceptionbut helpers still throwRuntimeException.
getChildSetters(Line 106) andguardHasMCAttributeSetters(Line 118) still throw plainRuntimeException, which won't carry theParsingContextfor error rendering. If these are reachable throughgetSingleChildSetter, the context is lost for those error paths.Consider either wrapping those calls in a try-catch that attaches the context, or updating the helpers to accept and use
pc.annot/src/test/java/com/predic8/membrane/annot/YAMLParsingErrorTest.java (1)
277-291: Commented-out test block should be addressed.This disabled test for child list item parsing (
$.demo.children[2].c) suggests a known gap. Consider adding a@Disabled("reason")annotation with a tracking issue instead of leaving dead code.annot/src/test/java/com/predic8/membrane/annot/YAMLComponentsParsingTest.java (2)
202-258: Consider usingassertInstanceOfinstead of manualinstanceof+fail().The repeated pattern of
if (c instanceof ConfigurationParsingException cpe) { ... } else { fail(...); }(lines 209, 227, 249) can be simplified with JUnit 5'sassertInstanceOf, which also produces a clearer failure message:var cpe = assertInstanceOf(ConfigurationParsingException.class, getRootCause(ex)); var pc = cpe.getParsingContext(); // assertions on pc ...This reduces boilerplate across all three test methods.
508-530: Two overlapping root-cause helpers — localgetCausevs. importedgetRootCause.The class imports
ExceptionUtil.getRootCause(line 28, used at lines 208, 226, 248) and also defines a localgetCause(line 526) that does the same recursive cause-chain traversal. Using a single approach would be cleaner.annot/src/main/java/com/predic8/membrane/annot/yaml/error/LineYamlErrorRenderer.java (3)
67-177: Substantial duplication between thekey == nullandkey != nullbranches.The ObjectNode wrapping logic (field-order-preserving copy with
__ERROR_MARKER__) at lines 86–101 is nearly identical to lines 136–155. Similarly, the ArrayNode wrapping at lines 107–116 mirrors lines 157–172. Extracting shared helpers (e.g.,wrapObjectField(ObjectNode parent, String fieldName)andwrapArrayElement(ArrayNode parent, int index)) would cut ~40 lines of duplication.
74-84:ctx.read(parentPath)can returnnullwhenSUPPRESS_EXCEPTIONSis enabled — no null guard.With
Option.SUPPRESS_EXCEPTIONS,ctx.read(parentPath)on line 78 returnsnullfor missing paths instead of throwing. The result is used directly ininstanceofchecks (line 80), which safely evaluates tofalsefornull, but then falls through to line 119 and throws a confusingIllegalArgumentException("Parent is neither an object nor an array"). Adding an explicit null check with a clearer message would improve debuggability.
196-246: RESET color code is emitted only after the entire marked block, but RED is emitted per line.Each marked line emits
RED()(lines 219, 236), butRESET()is only emitted once after the loop (line 244). If the terminal or log viewer doesn't handle redundant color codes gracefully, intermediate lines between marked blocks could remain red. Consider emittingRESET()at the end of each line or after eachRED()block.annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (2)
21-21: Unused import:javax.lang.model.util.*.This package is for annotation-processing-time utilities (
Types,Elements, etc.) and is only referenced in a Javadoc@linkon line 251. It is not used in runtime code and can be removed.#!/bin/bash # Verify javax.lang.model.util is not referenced in actual code (only Javadoc) rg -n 'javax\.lang\.model\.util' annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java rg -n 'Types' annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java
192-192: Inconsistent use of rawParsingContextvs.ParsingContext<?>in internal methods.
parseNumericOrThrow(line 192),getObjectList(line 215), andresolveReference(line 233) accept rawParsingContextwhile the public-facing methods useParsingContext<?>. This produces unchecked warnings and is inconsistent within the same class.annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (3)
233-240: Re-throwingConfigurationParsingExceptionas-is is fine, but the barecatch (Throwable)on line 236 will swallowErrorsubtypes.Catching
Throwablewraps JVM errors (e.g.,OutOfMemoryError,StackOverflowError) into aConfigurationParsingException, masking them. Consider narrowing tocatch (Exception cause)or at least re-throwingErrorinstances directly.Proposed fix
- catch (Throwable cause) { - var e = new ConfigurationParsingException(cause); - e.setParsingContext(ctx.key(key)); - throw e; + catch (Exception cause) { + var e = new ConfigurationParsingException(cause); + e.setParsingContext(ctx.key(key)); + throw e; }
218-220: Samecatch (Throwable)concern — wraps JVMErrors into a runtime exception.This catch block (and the one on line 236) both catch
Throwable. Narrowing toExceptionwould let fatal JVM errors propagate normally.
410-410: TODO comment suggests the condition is always true — verify or simplify.The inline comment
// TODO always true indSetterForKey(...)hints that thefindSetterForKeynull-check may be unnecessary. If confirmed, the conditional can be simplified and the TODO removed.Would you like me to verify whether
findSetterForKeycan ever returnnullin this context?
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java
Show resolved
Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java
Outdated
Show resolved
Hide resolved
annot/src/test/java/com/predic8/membrane/annot/YAMLParsingErrorTest.java
Show resolved
Hide resolved
… exception messages - Added SLF4J logging to `MethodSetter` for detailed debug messages on missing methods/beans. - Simplified `ConfigurationParsingException` message initialization in `GenericYamlParser`. - Replaced redundant `System.out.println` call with direct usage of `renderErrorReport`. - Enhanced error messages in `McYamlIntrospector` for clarity on list vs. object mismatches. - Imported SLF4J and utility classes as part of code cleanup.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (1)
236-240:⚠️ Potential issue | 🟠 MajorOriginal exception cause is lost — only the message string is preserved.
new ConfigurationParsingException(cause.getMessage())discards the original stack trace. This makes debugging significantly harder. Pass the cause through.Suggested fix
catch (Throwable cause) { - var e = new ConfigurationParsingException(cause.getMessage()); + var e = new ConfigurationParsingException(cause); e.setParsingContext(ctx.key(key)); throw e; }
🤖 Fix all issues with AI agents
In `@annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java`:
- Around line 218-220: The catch block in GenericYamlParser currently throws a
new ConfigurationParsingException(cause) which discards the parsing context
(pc); change the handling so that if cause is already a
ConfigurationParsingException you rethrow it, otherwise wrap the cause in a new
ConfigurationParsingException that includes the parsing context (use the
constructor or setter that accepts pc, e.g. new
ConfigurationParsingException(pc, cause) or setParsingContext(pc) on the
exception) so getFormattedReport() retains useful context.
- Around line 236-240: The catch block in GenericYamlParser currently constructs
ConfigurationParsingException with only cause.getMessage(), losing the original
Throwable and stacktrace; change it to create the exception with the original
cause (e.g., new ConfigurationParsingException(cause.getMessage(), cause) or
create and call initCause(cause)) before calling
e.setParsingContext(ctx.key(key)) and throwing, so the original exception is
preserved (same approach as used in MethodSetter).
🧹 Nitpick comments (3)
annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (1)
219-219: Raw typeParsingContext— should beParsingContext<?>.For consistency with the rest of the file and to avoid unchecked warnings.
- private `@Nullable` List<Object> getObjectList(ParsingContext ctx, JsonNode node, String key, Class<?> wanted) { + private `@Nullable` List<Object> getObjectList(ParsingContext<?> ctx, JsonNode node, String key, Class<?> wanted) {annot/src/main/java/com/predic8/membrane/annot/yaml/NodeValidationUtils.java (1)
40-51: RawParsingContexttype — should beParsingContext<?>for consistency.
ensureSingleKey(line 27) usesParsingContext<?>, but bothensureArrayoverloads use rawParsingContext. This creates unchecked warnings and is inconsistent.- public static void ensureArray(ParsingContext pc, JsonNode node, String message) throws ConfigurationParsingException { + public static void ensureArray(ParsingContext<?> pc, JsonNode node, String message) throws ConfigurationParsingException {- public static void ensureArray(ParsingContext pc, JsonNode node) throws ConfigurationParsingException { + public static void ensureArray(ParsingContext<?> pc, JsonNode node) throws ConfigurationParsingException {annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
233-235: Redundant catch-and-rethrow forConfigurationParsingException.This block exists solely to prevent
ConfigurationParsingExceptionfrom being caught by theThrowablehandler below. This is a valid pattern, but a brief comment would clarify the intent.
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java
Show resolved
Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java
Show resolved
Hide resolved
…c handling - Corrected typos in method names (e.g., `validateAgaistSchema` to `validateAgainstSchema`). - Introduced additional debug logging in `GenericYamlParser` for enhanced error tracking. - Updated generic type handling in `ParsingContext` and `NodeValidationUtils`. - Improved exception message formatting for clarity.
…g' into yaml-configuration-error-handling
…thods - Reformatted YAML sources in `YAMLParsingCollapsedTest` and `YAMLParsingTest` for consistent indentation. - Replaced redundant inline structure definitions with properly aligned multi-line structures in test assertions. - Updated exception validation logic to use `assertNull` and `assertTrue` for better clarity.
…ng tests - Removed unused `rootCause` method from `YAMLParsingCollapsedTest`. - Updated exception handling to ignore caught exceptions for clarity. - Replaced `assertEquals(null, ...)` with `assertNull(...)` in `YAMLParsingTest`. - Fixed minor formatting issues in test cases.
|
/ok-to-test |
… usages - Updated `ParsingContext` to use consistent `get*` accessor methods. - Replaced direct field accesses with the new accessor methods across all relevant classes and test files. - Improved exception messages and updated formatting for enhanced readability and clarity.
…g' into yaml-configuration-error-handling
annot/src/main/java/com/predic8/membrane/annot/yaml/ParsingContext.java
Outdated
Show resolved
Hide resolved
annot/src/test/java/com/predic8/membrane/annot/YAMLComponentsParsingTest.java
Outdated
Show resolved
Hide resolved
annot/src/test/java/com/predic8/membrane/annot/YAMLComponentsParsingTest.java
Outdated
Show resolved
Hide resolved
annot/src/test/java/com/predic8/membrane/annot/YAMLParsingErrorTest.java
Outdated
Show resolved
Hide resolved
annot/src/test/java/com/predic8/membrane/annot/YAMLParsingErrorTest.java
Outdated
Show resolved
Hide resolved
…ling in YAML tests - Converted `ParsingContext` to a record for cleaner implementation. - Updated YAML parsing tests to use the new `getRootCause` helper method with type assertions. - Reintroduced a previously commented-out YAML parsing test with improved exception handling.
- Replaced `assertEquals(null, ...)` with `assertNull(...)` in `YAMLParsingErrorTest`. - Added `@NotNull` annotation to `ParsingContext#toString` for improved null safety. - Removed unused `assertSchemaErrorContains` method from `YAMLComponentsParsingTest`. - Simplified `getRootCause` logic by removing redundant null checks.
…g' into yaml-configuration-error-handling
2 participants
…ed contexts
ParsingExceptionwithYamlParsingExceptionfor improved error details.YamlErrorRendererfor visual error markers in YAML reports.YamlParsingException.NodeValidationUtilsandMethodSetter.GenericYamlParserto enhance schema validation handling.Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Tests
Documentation