Add Yaml include list and refactor Yaml parsing (Discussion #2817)#2898
Open
christiangoerdes wants to merge 56 commits intomasterfrom
Open
Add Yaml include list and refactor Yaml parsing (Discussion #2817)#2898christiangoerdes wants to merge 56 commits intomasterfrom
christiangoerdes wants to merge 56 commits intomasterfrom
Conversation
# Conflicts: # core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors YAML parsing into a modular parsing/binding pipeline with include resolution and source metadata tracking; adds bean-definition tracking/context and an IncludeList code generator; and threads bean-derived base-location resolution across many core components. Changes
Sequence Diagram(s)sequenceDiagram
participant Reader as YamlDocumentReader
participant Resolver as IncludeResolver
participant Session as ParseSession
participant Extractor as BeanDefinitionExtractor
participant Registry as BeanRegistry
Reader->>Resolver: readDocuments(sourceMetadata, yaml)
Resolver->>Session: use includeStack(), loadedIncludeFiles()
Resolver->>Resolver: resolve each include (file or dir), detect cycles
Resolver->>Reader: readDocuments(sourceMetadata, includedYaml)
Reader->>Extractor: produce ResolvedDocument list
Extractor->>Session: assign bean names (nextBeanName)
Extractor->>Registry: emit BeanDefinition(kind,name,uuid,node,SourceMetadata)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Collaborator
Author
|
/ok-to-test |
|
This pull request needs "/ok-to-test" from an authorized committer. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (10)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistry.java (1)
50-55: Clarify behavior when noBeanDefinitionis associated.At Line 55, the contract does not define what happens for unknown objects (null/exception). Please make this explicit in the interface docs (or return
Optional<BeanDefinition>).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistry.java` around lines 50 - 55, Update the contract for BeanRegistry.getBeanDefinition to explicitly express absence by changing its return type to Optional<BeanDefinition> and update the Javadoc to state it returns Optional.empty() when no BeanDefinition is associated; then update all implementations of BeanRegistry.getBeanDefinition to return Optional.of(def) or Optional.empty() as appropriate and update callers to handle the Optional instead of assuming a non-null BeanDefinition.core/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.java (1)
208-208: Resolve and reuse a single base-resolved resource variable.Lines 208 and 231 re-apply
combine(getBeanBaseLocation(), ...)in two places. Resolving once and reusing reduces ambiguity and avoids accidental double-prefix behavior.♻️ Proposed refactor
- exc.setResponse(webServerInterceptor.createResponse(router.getResolverMap(), resource = combine(getBeanBaseLocation(), wsdl))); + String resolvedWsdl = combine(getBeanBaseLocation(), wsdl); + exc.setResponse(webServerInterceptor.createResponse(router.getResolverMap(), resource = resolvedWsdl)); exc.getResponse().getHeader().setContentType(TEXT_XML); ... - wi.setPathRewriter(new RelativePathRewriter(exc, combine(getBeanBaseLocation(), resource))); + wi.setPathRewriter(new RelativePathRewriter(exc, resource));Also applies to: 231-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.java` at line 208, Resolve the resource once using combine(getBeanBaseLocation(), ...) and reuse that variable for both response creation sites to avoid double-prefixing; specifically, compute a single Resource (e.g., resource) from combine(getBeanBaseLocation(), wsdl) before calling webServerInterceptor.createResponse(...) and reuse that same resource for the later call (the other occurrence around line with the schema/other WSDL reference), instead of calling combine(getBeanBaseLocation(), ...) twice in exc.setResponse(...) and the second place.annot/src/main/java/com/predic8/membrane/annot/yaml/ParsingContext.java (1)
41-43: Consider using parameterized return type.The
childmethod returns rawParsingContext<?>but creates an unparameterizedParsingContext. For consistency with the class design, consider preserving the type parameter:Suggested fix
- public ParsingContext<?> child(String childContext, String pathSegment) { - return new ParsingContext(childContext, registry, grammar, topLevel, path + pathSegment, null); + public ParsingContext<T> child(String childContext, String pathSegment) { + return new ParsingContext<>(childContext, registry, grammar, topLevel, path + pathSegment, null); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/yaml/ParsingContext.java` around lines 41 - 43, The child method currently returns ParsingContext<?> while instantiating a raw ParsingContext; change it to preserve the generic type parameter by using the class's type parameter (e.g., change the method signature to return ParsingContext<T> and instantiate new ParsingContext<T>(...)) so the type parameter is propagated; update the method declaration and the constructor invocation in child(String childContext, String pathSegment) to use T instead of a raw type and ensure any callers expecting ParsingContext<?> still compile or are updated.core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/LoginDialog.java (1)
111-117: Consider unifyingasDirectorywithBeanDefinitionBasePathUtil.ensureDirectorySemantics.This helper duplicates the logic in
BeanDefinitionBasePathUtil.ensureDirectorySemantics. Consider extracting a shared utility or reusing the existing one to avoid divergence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/LoginDialog.java` around lines 111 - 117, The asDirectory method in LoginDialog duplicates logic already in BeanDefinitionBasePathUtil.ensureDirectorySemantics; remove or replace asDirectory by calling BeanDefinitionBasePathUtil.ensureDirectorySemantics from LoginDialog (or extract the shared logic to a common utility and update both LoginDialog.asDirectory usage and any other callers to use that new utility) so there is a single implementation to maintain and avoid divergence.annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/CollectionBinder.java (1)
50-55: Missing parsing context in exception for null list items.The
ConfigurationParsingExceptionat Line 52 doesn't include the parsing contextctx, which would help users identify which list item is null.Proposed fix
private static Object parseListItem(ParsingContext<?> ctx, JsonNode item, Class<?> elemType) { if (item == null || item.isNull()) - throw new ConfigurationParsingException("List items must not be null."); + throw new ConfigurationParsingException("List items must not be null.", null, ctx);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/CollectionBinder.java` around lines 50 - 55, The null-check in parseListItem currently throws new ConfigurationParsingException("List items must not be null.") without the parsing context; modify the throw to include the ParsingContext<?> ctx so the exception is constructed with context (e.g., pass ctx into the ConfigurationParsingException constructor or call the constructor overload that accepts context) so errors from parseListItem include ctx information for better diagnostics; update the throw site inside parseListItem to use the context-aware constructor.annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/ScalarValueConverter.java (2)
37-41: DuplicateSpelEvaluatorinstances - static and instance fields.Both
STATIC_SPEL_EVALUATOR(static) andspelEvaluator(instance) are defined. IfSpelEvaluatoris stateless, consider using only the static instance to avoid confusion and reduce object allocation.Proposed consolidation
public final class ScalarValueConverter { private static final ObjectMapper SCALAR_MAPPER = new ObjectMapper(); private static final SpelEvaluator STATIC_SPEL_EVALUATOR = new SpelEvaluator(); - - private final SpelEvaluator spelEvaluator = new SpelEvaluator(); private final ReferenceResolver referenceResolver = new ReferenceResolver();Then replace
spelEvaluator.resolve(...)calls withSTATIC_SPEL_EVALUATOR.resolve(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/ScalarValueConverter.java` around lines 37 - 41, There's a duplicate SpelEvaluator: STATIC_SPEL_EVALUATOR (static) and the instance field spelEvaluator in ScalarValueConverter; remove the instance field spelEvaluator and replace all usages of spelEvaluator.resolve(...) (and any other spelEvaluator.* calls) with STATIC_SPEL_EVALUATOR.resolve(...) to consolidate to the single static instance, ensuring all references in methods of ScalarValueConverter now use STATIC_SPEL_EVALUATOR.
138-146: Inconsistent enum parsing: only uppercase fallback, unlikeCollectionBinder.
CollectionBinder.coerceScalarListItem()tries exact match first, then uppercase. This method only tries uppercase, which could reject valid enum constants with mixed case.Proposed fix for consistency
`@SuppressWarnings`("unchecked") private static <E extends Enum<E>> E parseEnum(Class<?> enumClass, JsonNode node) throws WrongEnumConstantException { - String value = node.asText().toUpperCase(ROOT); + String value = node.asText(); try { return Enum.valueOf((Class<E>) enumClass, value); } catch (IllegalArgumentException e) { - throw new WrongEnumConstantException(enumClass, value); + try { + return Enum.valueOf((Class<E>) enumClass, value.toUpperCase(ROOT)); + } catch (IllegalArgumentException e2) { + throw new WrongEnumConstantException(enumClass, value); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/ScalarValueConverter.java` around lines 138 - 146, parseEnum currently uppercases the input before calling Enum.valueOf and so rejects valid mixed-case enum constants; change parseEnum (in ScalarValueConverter) to first attempt Enum.valueOf with the node text as-is, and only if that throws IllegalArgumentException try again with node.asText().toUpperCase(ROOT) before throwing WrongEnumConstantException; keep the cast to (Class<E>) enumClass and preserve throwing WrongEnumConstantException(enumClass, value) on final failure to match CollectionBinder.coerceScalarListItem behavior.annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/ReferenceResolver.java (1)
43-45: CatchingThrowableis overly broad and may maskErrorconditions.Catching
Throwablewill catchErrorsubclasses (likeOutOfMemoryError) that should typically propagate. Consider narrowing toReflectiveOperationExceptionwhich coversIllegalAccessException,InvocationTargetException, etc.Proposed fix
- } catch (Throwable t) { - throw new ConfigurationParsingException(t); + } catch (ReflectiveOperationException e) { + throw new ConfigurationParsingException(e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/ReferenceResolver.java` around lines 43 - 45, The catch block in ReferenceResolver that currently catches Throwable is too broad; replace it with a narrower catch for reflective failures (e.g., catch ReflectiveOperationException) so Errors (like OutOfMemoryError) still propagate; wrap the caught ReflectiveOperationException (or other specific reflection exceptions if used) in the existing ConfigurationParsingException to preserve the original behavior and stack trace.annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/ObjectBinder.java (1)
97-102: CatchingThrowableis overly broad.Similar to
ReferenceResolver, catchingThrowablewill catchErrorsubclasses that should typically propagate. Consider usingExceptionor more specific exception types.Proposed fix
- } catch (Throwable cause) { + } catch (Exception cause) { log.debug("", cause); var cpe = new ConfigurationParsingException(cause); applyCurrentSourceFileIfMissing(cpe); throw cpe; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/ObjectBinder.java` around lines 97 - 102, The catch block in ObjectBinder currently catches Throwable (in the try/catch that logs and wraps into ConfigurationParsingException), which is too broad; change it to catch Exception (or specific checked/runtime exceptions you expect) instead of Throwable, and let Errors propagate — i.e., replace "catch (Throwable cause)" with "catch (Exception cause)" (or multiple catches if you need to handle specific exception types), keep the existing log.debug("", cause), create the ConfigurationParsingException(cause), call applyCurrentSourceFileIfMissing(cpe) and rethrow the ConfigurationParsingException as before so only non-Error exceptions are wrapped and returned.annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/GenericYamlParser.java (1)
84-86: Avoid exposing internal mutable parser state
getBeanDefinitions()returns the backingArrayList, allowing callers to mutate internal state after parsing.Suggested fix
public List<BeanDefinition> getBeanDefinitions() { - return beanDefs; + return List.copyOf(beanDefs); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/GenericYamlParser.java` around lines 84 - 86, getBeanDefinitions() currently returns the internal mutable field beanDefs from GenericYamlParser, exposing parser state to callers; change it to return an immutable or defensive copy instead (e.g., return an unmodifiable view or a new List copy) so callers cannot mutate the internal ArrayList of BeanDefinition objects; update the method to return an immutable List<BeanDefinition> (using List.copyOf(beanDefs) or Collections.unmodifiableList(new ArrayList<>(beanDefs))) while keeping the field name beanDefs and the return type BeanDefinition list unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java`:
- Around line 203-209: The method findSingleSetterOrNullForAnnotation currently
returns the first annotated setter and thus can be non-deterministic; change it
to collect all methods that satisfy isSetter and have the annotation, then: if
the collection is empty return null, if it contains exactly one return that
Method, and if it contains more than one throw an IllegalStateException (or
similar) describing the conflicting methods so callers get a deterministic
failure; reference McYamlIntrospector::findSingleSetterOrNullForAnnotation and
the isSetter predicate when locating the matching methods.
In
`@annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/SetterResolver.java`:
- Around line 76-86: Remove the dead code path that checks for MCChildElement in
getConfigElementName: delete the clazz.getAnnotation(MCChildElement.class)
branch (the MCChildElement check and its return) since MCChildElement is
METHOD-targeted and always null on a Class; instead only check for MCElement via
clazz.getAnnotation(MCElement.class) and fall back to clazz.getSimpleName() in
getConfigElementName.
In
`@annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/GenericYamlParser.java`:
- Around line 58-60: The current code creates two separate ParseSession
instances when calling includeResolver.resolve(...) and
definitionExtractor.extract(...), so state from include resolution isn't shared
with extraction; fix by instantiating a single ParseSession (e.g., ParseSession
session = new ParseSession()) and pass that same session to
includeResolver.resolve(session, rootSourceMetadata, yaml) and to
definitionExtractor.extract(session, resolvedResult) so beanDefs.addAll(...)
receives extraction results produced with the same ParseSession; update calls in
GenericYamlParser to reuse that session variable for both resolve and extract.
In
`@annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/MethodSetter.java`:
- Around line 64-66: The setter currently always supplies a List<Object> (via
getObjectList/resolveSetterValue) which will cause IllegalArgumentException when
the reflected setter parameter is a non-List collection type; update setSetter
(and/or resolveSetterValue) to adapt the returned collection to the setter
parameter type: inspect setter.getParameterTypes()[0], if it is a Collection
subtype then convert the List into an instance of the expected type (choose
default implementations for interfaces: Collection->ArrayList,
Set->LinkedHashSet, SortedSet->TreeSet, Queue/Deque->LinkedList; if it's a
concrete class try to instantiate with a no-arg constructor and addAll the
list), then pass that converted collection to setter.invoke(instance,...);
reference the setSetter method and setter.invoke usage so the correct parameter
conversion is applied before invoking the setter.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java`:
- Around line 81-84: The init method assigns log and then calls
getBeanBaseLocation() before setting this.router, but getBeanBaseLocation()
reads the router field; update init so this.router is assigned before any calls
to getBeanBaseLocation() (and any other methods that access router) — e.g.,
assign this.router = router immediately at start of init (and apply the same
reorder in the second init/initialization block around lines 391-393) so
getBeanBaseLocation() sees the correct router instance.
In `@core/src/main/java/com/predic8/membrane/core/util/OSUtil.java`:
- Around line 50-57: The isWindowsAbsolutePath method currently only recognizes
drive-letter forms; update it to also treat UNC paths as absolute by returning
true when the input starts with double backslashes or double slashes (e.g.,
"\\\\server\\share" or "//server/share"); keep the existing null/length checks
and the drive-letter logic in place and ensure you reference the
isWindowsAbsolutePath(String location) method when making the change.
In `@distribution/examples/configuration/include/membrane.cmd`:
- Around line 1-24: The batch file uses Unix (LF-only) line endings which can
break Windows batch parsing around labels like :search_up, :found, and :notfound
and commands such as GOTO and CALL; convert membrane.cmd to CRLF line endings
(e.g., set git core.autocrlf true or add a .gitattributes entry like *.cmd text
eol=crlf, or run unix2dos on membrane.cmd) so the labels and control flow (CALL
"%MEMBRANE_HOME%\scripts\run-membrane.cmd" %*, GOTO found/notfound) parse
correctly.
In `@distribution/examples/configuration/include/README.md`:
- Around line 13-15: Update the startup instructions in the README to use the
correct example directory name: replace the mistaken path string
"examples/configuration/includes" with "examples/configuration/include" so the
two-line snippet that cds into the example and runs "./membrane.sh" points to
the actual folder (look for the exact string "examples/configuration/includes"
in the README and change it).
---
Nitpick comments:
In
`@annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistry.java`:
- Around line 50-55: Update the contract for BeanRegistry.getBeanDefinition to
explicitly express absence by changing its return type to
Optional<BeanDefinition> and update the Javadoc to state it returns
Optional.empty() when no BeanDefinition is associated; then update all
implementations of BeanRegistry.getBeanDefinition to return Optional.of(def) or
Optional.empty() as appropriate and update callers to handle the Optional
instead of assuming a non-null BeanDefinition.
In
`@annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/CollectionBinder.java`:
- Around line 50-55: The null-check in parseListItem currently throws new
ConfigurationParsingException("List items must not be null.") without the
parsing context; modify the throw to include the ParsingContext<?> ctx so the
exception is constructed with context (e.g., pass ctx into the
ConfigurationParsingException constructor or call the constructor overload that
accepts context) so errors from parseListItem include ctx information for better
diagnostics; update the throw site inside parseListItem to use the context-aware
constructor.
In
`@annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/ObjectBinder.java`:
- Around line 97-102: The catch block in ObjectBinder currently catches
Throwable (in the try/catch that logs and wraps into
ConfigurationParsingException), which is too broad; change it to catch Exception
(or specific checked/runtime exceptions you expect) instead of Throwable, and
let Errors propagate — i.e., replace "catch (Throwable cause)" with "catch
(Exception cause)" (or multiple catches if you need to handle specific exception
types), keep the existing log.debug("", cause), create the
ConfigurationParsingException(cause), call applyCurrentSourceFileIfMissing(cpe)
and rethrow the ConfigurationParsingException as before so only non-Error
exceptions are wrapped and returned.
In
`@annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/ReferenceResolver.java`:
- Around line 43-45: The catch block in ReferenceResolver that currently catches
Throwable is too broad; replace it with a narrower catch for reflective failures
(e.g., catch ReflectiveOperationException) so Errors (like OutOfMemoryError)
still propagate; wrap the caught ReflectiveOperationException (or other specific
reflection exceptions if used) in the existing ConfigurationParsingException to
preserve the original behavior and stack trace.
In
`@annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/ScalarValueConverter.java`:
- Around line 37-41: There's a duplicate SpelEvaluator: STATIC_SPEL_EVALUATOR
(static) and the instance field spelEvaluator in ScalarValueConverter; remove
the instance field spelEvaluator and replace all usages of
spelEvaluator.resolve(...) (and any other spelEvaluator.* calls) with
STATIC_SPEL_EVALUATOR.resolve(...) to consolidate to the single static instance,
ensuring all references in methods of ScalarValueConverter now use
STATIC_SPEL_EVALUATOR.
- Around line 138-146: parseEnum currently uppercases the input before calling
Enum.valueOf and so rejects valid mixed-case enum constants; change parseEnum
(in ScalarValueConverter) to first attempt Enum.valueOf with the node text
as-is, and only if that throws IllegalArgumentException try again with
node.asText().toUpperCase(ROOT) before throwing WrongEnumConstantException; keep
the cast to (Class<E>) enumClass and preserve throwing
WrongEnumConstantException(enumClass, value) on final failure to match
CollectionBinder.coerceScalarListItem behavior.
In
`@annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/GenericYamlParser.java`:
- Around line 84-86: getBeanDefinitions() currently returns the internal mutable
field beanDefs from GenericYamlParser, exposing parser state to callers; change
it to return an immutable or defensive copy instead (e.g., return an
unmodifiable view or a new List copy) so callers cannot mutate the internal
ArrayList of BeanDefinition objects; update the method to return an immutable
List<BeanDefinition> (using List.copyOf(beanDefs) or
Collections.unmodifiableList(new ArrayList<>(beanDefs))) while keeping the field
name beanDefs and the return type BeanDefinition list unchanged.
In `@annot/src/main/java/com/predic8/membrane/annot/yaml/ParsingContext.java`:
- Around line 41-43: The child method currently returns ParsingContext<?> while
instantiating a raw ParsingContext; change it to preserve the generic type
parameter by using the class's type parameter (e.g., change the method signature
to return ParsingContext<T> and instantiate new ParsingContext<T>(...)) so the
type parameter is propagated; update the method declaration and the constructor
invocation in child(String childContext, String pathSegment) to use T instead of
a raw type and ensure any callers expecting ParsingContext<?> still compile or
are updated.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/LoginDialog.java`:
- Around line 111-117: The asDirectory method in LoginDialog duplicates logic
already in BeanDefinitionBasePathUtil.ensureDirectorySemantics; remove or
replace asDirectory by calling
BeanDefinitionBasePathUtil.ensureDirectorySemantics from LoginDialog (or extract
the shared logic to a common utility and update both LoginDialog.asDirectory
usage and any other callers to use that new utility) so there is a single
implementation to maintain and avoid divergence.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.java`:
- Line 208: Resolve the resource once using combine(getBeanBaseLocation(), ...)
and reuse that variable for both response creation sites to avoid
double-prefixing; specifically, compute a single Resource (e.g., resource) from
combine(getBeanBaseLocation(), wsdl) before calling
webServerInterceptor.createResponse(...) and reuse that same resource for the
later call (the other occurrence around line with the schema/other WSDL
reference), instead of calling combine(getBeanBaseLocation(), ...) twice in
exc.setResponse(...) and the second place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f1d9cf9b-dbdf-4139-835b-c49fbf02f269
📒 Files selected for processing (87)
annot/src/main/java/com/predic8/membrane/annot/SpringConfigurationXSDGeneratingAnnotationProcessor.javaannot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanCollector.javaannot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanContainer.javaannot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinition.javaannot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinitionContext.javaannot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinitions.javaannot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistry.javaannot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.javaannot/src/main/java/com/predic8/membrane/annot/beanregistry/SpringContextAdapter.javaannot/src/main/java/com/predic8/membrane/annot/generator/IncludeListClassGenerator.javaannot/src/main/java/com/predic8/membrane/annot/yaml/ConfigurationParsingException.javaannot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.javaannot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.javaannot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.javaannot/src/main/java/com/predic8/membrane/annot/yaml/ParsingContext.javaannot/src/main/java/com/predic8/membrane/annot/yaml/YamlParsingUtils.javaannot/src/main/java/com/predic8/membrane/annot/yaml/error/LineYamlErrorRenderer.javaannot/src/main/java/com/predic8/membrane/annot/yaml/parsing/GenericYamlParser.javaannot/src/main/java/com/predic8/membrane/annot/yaml/parsing/MethodSetter.javaannot/src/main/java/com/predic8/membrane/annot/yaml/parsing/ParseSession.javaannot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/CollectionBinder.javaannot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/ObjectBinder.javaannot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/PropertyBinder.javaannot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/ReferenceResolver.javaannot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/ScalarValueConverter.javaannot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/SetterResolver.javaannot/src/main/java/com/predic8/membrane/annot/yaml/parsing/definition/BeanDefinitionExtractor.javaannot/src/main/java/com/predic8/membrane/annot/yaml/parsing/definition/ComponentDefinitionExtractor.javaannot/src/main/java/com/predic8/membrane/annot/yaml/parsing/source/IncludeResolver.javaannot/src/main/java/com/predic8/membrane/annot/yaml/parsing/source/SourceMetadata.javaannot/src/main/java/com/predic8/membrane/annot/yaml/parsing/source/YamlDocumentReader.javaannot/src/main/java/com/predic8/membrane/annot/yaml/parsing/support/BeanLifecycleInvoker.javaannot/src/main/java/com/predic8/membrane/annot/yaml/parsing/support/SpelEvaluator.javaannot/src/test/java/com/predic8/membrane/annot/yaml/parsing/MethodSetterTest.javacore/src/main/java/com/predic8/membrane/core/cli/RouterCLI.javacore/src/main/java/com/predic8/membrane/core/interceptor/AbstractInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/apikey/stores/ApiKeyFileStore.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/LDAPUserDataProvider.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/LoginDialog.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/LoginInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/xen/XenAuthenticationInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/cache/CacheInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/headerfilter/HeaderFilterInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.javacore/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtSignInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/ConsentPageFile.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/OAuth2AuthorizationServerInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/DynamicRegistration.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/MembraneAuthorizationService.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/MicrosoftEntraIDAuthorizationService.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/processors/LoginDialogEndpointProcessor.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/tokengenerators/BearerJwtTokenGenerator.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2server/LoginDialog2.javacore/src/main/java/com/predic8/membrane/core/interceptor/rest/SOAPRESTHelper.javacore/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/server/WebServerInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/session/JwtSessionManager.javacore/src/main/java/com/predic8/membrane/core/interceptor/stomp/STOMPClient.javacore/src/main/java/com/predic8/membrane/core/interceptor/templating/AbstractTemplateInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/xslt/XSLTInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/xslt/XSLTTransformer.javacore/src/main/java/com/predic8/membrane/core/lang/AbstractScriptInterceptor.javacore/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxy.javacore/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactory.javacore/src/main/java/com/predic8/membrane/core/proxies/AbstractProxy.javacore/src/main/java/com/predic8/membrane/core/proxies/AbstractServiceProxy.javacore/src/main/java/com/predic8/membrane/core/proxies/SSLProxy.javacore/src/main/java/com/predic8/membrane/core/proxies/SSLableProxy.javacore/src/main/java/com/predic8/membrane/core/sslinterceptor/RouterIpResolverInterceptor.javacore/src/main/java/com/predic8/membrane/core/transport/http/UnableToTunnelException.javacore/src/main/java/com/predic8/membrane/core/util/BeanDefinitionBasePathUtil.javacore/src/main/java/com/predic8/membrane/core/util/OSUtil.javacore/src/test/java/com/predic8/membrane/core/config/spring/k8s/EnvelopeTest.javacore/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserIncludeListTest.javacore/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.javacore/src/test/java/com/predic8/membrane/test/TestAppender.javadistribution/examples/configuration/include/README.mddistribution/examples/configuration/include/apis.yamldistribution/examples/configuration/include/customers/customers-response.json.templatedistribution/examples/configuration/include/customers/customers.apis.yamldistribution/examples/configuration/include/membrane.cmddistribution/examples/configuration/include/membrane.shdistribution/examples/configuration/include/orders/orders-response.json.templatedistribution/examples/configuration/include/orders/orders.apis.yamldistribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/ConfigurationIncludesExampleTest.java
💤 Files with no reviewable changes (3)
- annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java
- annot/src/main/java/com/predic8/membrane/annot/yaml/YamlParsingUtils.java
- annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java
annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java
Show resolved
Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/SetterResolver.java
Show resolved
Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/GenericYamlParser.java
Outdated
Show resolved
Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/MethodSetter.java
Show resolved
Hide resolved
.../com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
core/src/test/java/com/predic8/membrane/core/util/OSUtilTest.java (1)
31-44: Good test coverage for the new utility method.The test covers the key scenarios: null/empty inputs, drive-letter paths with both slash styles, UNC paths with both slash styles, and correctly rejects Unix absolute and relative paths.
For even more robust coverage, consider adding edge cases for lowercase drive letters and invalid first characters:
💡 Optional additional test cases
assertTrue(isWindowsAbsolutePath("C:\\temp")); assertTrue(isWindowsAbsolutePath("C:/temp")); + assertTrue(isWindowsAbsolutePath("c:\\temp")); // lowercase drive letter + assertTrue(isWindowsAbsolutePath("D:/folder")); assertTrue(isWindowsAbsolutePath("\\\\server\\share")); assertTrue(isWindowsAbsolutePath("//server/share")); assertFalse(isWindowsAbsolutePath("/tmp")); assertFalse(isWindowsAbsolutePath("relative\\path")); + assertFalse(isWindowsAbsolutePath("1:\\temp")); // digit, not letter🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/util/OSUtilTest.java` around lines 31 - 44, Add tests to OSUtilTest::windowsAbsolutePath to cover lowercase drive letters and inputs with invalid first characters: call isWindowsAbsolutePath with strings like "c:\\temp" and "c:/temp" and assertTrue, and add negative cases such as "?:\\path", "1:\\path" or strings starting with an invalid drive-char to assertFalse; ensure null/empty/UNC/unix/relative assertions remain unchanged and only the additional edge-case assertions are appended to the existing test method.annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/GenericYamlParser.java (3)
75-77: Error message may be misleading for other YAML parse errors.The catch block assumes all
JsonParseExceptions are caused by missing---separators, but this exception can occur for various syntax errors (invalid tokens, malformed structures, etc.). Consider providing a more generic message or inspecting the exception to provide context-specific guidance.Suggested fix
} catch (JsonParseException e) { - throw new IOException("Invalid YAML: multiple configurations must be separated by '---' (at line %d, column %d).".formatted(e.getLocation().getLineNr(), e.getLocation().getColumnNr()), e); + throw new IOException("Invalid YAML at line %d, column %d: %s".formatted(e.getLocation().getLineNr(), e.getLocation().getColumnNr(), e.getOriginalMessage()), e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/GenericYamlParser.java` around lines 75 - 77, The catch in GenericYamlParser currently converts any JsonParseException into a specific "must be separated by '---'" message; update the catch for JsonParseException in the parse logic to produce a more generic, informative IOException that includes the original exception message and location (e.getMessage() and e.getLocation().getLineNr()/getColumnNr()) and optionally mention the '---' separator as one possible cause rather than the definitive cause so callers see the true parsing error context.
101-109: Raw type usage ofParsingContext.Lines 102 and 105 use raw
ParsingContextwithout type parameters. SinceParsingContextappears to be generic (as shown byParsingContext<?>at line 118), consider using the wildcard form for consistency and type safety.Suggested fix
public static Class<?> decideClazz(String kind, Grammar grammar, JsonNode node) { - ensureSingleKey(new ParsingContext("", null, grammar, node, "$", null), node); + ensureSingleKey(new ParsingContext<>("", null, grammar, node, "$", null), node); Class<?> clazz = grammar.getElement(kind); if (clazz == null) { - var pc = new ParsingContext("", null, grammar, node, "$", null).key(kind); + var pc = new ParsingContext<>("", null, grammar, node, "$", null).key(kind); throw new ConfigurationParsingException("Did not find java class for kind '%s'.".formatted(kind), null, pc); } return clazz; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/GenericYamlParser.java` around lines 101 - 109, The method decideClazz uses raw ParsingContext when calling ensureSingleKey and constructing pc; replace raw usages with the generic wildcard form (e.g. new ParsingContext<?> or new ParsingContext<>(...) depending on the constructor signature) so both calls use ParsingContext<?> rather than raw ParsingContext, updating the expressions in ensureSingleKey(new ParsingContext(...)) and var pc = new ParsingContext(...).key(kind) to preserve type safety and consistency with ParsingContext<?> used elsewhere (refer to decideClazz and ensureSingleKey/ParsingContext usages).
58-58: Consider usingSourceMetadata.empty()for clarity whenrootSourceFileis null.The parameter
rootSourceFileis documented as optional, and while the current code handles null correctly (sincenormalizePathreturns null for null input, andSourceMetadata.empty()is alreadynew SourceMetadata(null, null)), usingempty()explicitly makes the intent clearer:Suggested improvement
- SourceMetadata rootSourceMetadata = root(rootSourceFile); + SourceMetadata rootSourceMetadata = rootSourceFile != null ? root(rootSourceFile) : SourceMetadata.empty();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/GenericYamlParser.java` at line 58, The code calls root(rootSourceFile) and constructs a SourceMetadata implicitly when rootSourceFile is null; change the call to use SourceMetadata.empty() for clarity by passing SourceMetadata.empty() (or using it where root() is invoked) instead of relying on normalizePath/root to produce null—update GenericYamlParser.java to detect null rootSourceFile and call root(SourceMetadata.empty()) or otherwise use SourceMetadata.empty() so intent is explicit (referencing rootSourceFile, normalizePath, root(), and SourceMetadata.empty()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/GenericYamlParser.java`:
- Around line 75-77: The catch in GenericYamlParser currently converts any
JsonParseException into a specific "must be separated by '---'" message; update
the catch for JsonParseException in the parse logic to produce a more generic,
informative IOException that includes the original exception message and
location (e.getMessage() and e.getLocation().getLineNr()/getColumnNr()) and
optionally mention the '---' separator as one possible cause rather than the
definitive cause so callers see the true parsing error context.
- Around line 101-109: The method decideClazz uses raw ParsingContext when
calling ensureSingleKey and constructing pc; replace raw usages with the generic
wildcard form (e.g. new ParsingContext<?> or new ParsingContext<>(...) depending
on the constructor signature) so both calls use ParsingContext<?> rather than
raw ParsingContext, updating the expressions in ensureSingleKey(new
ParsingContext(...)) and var pc = new ParsingContext(...).key(kind) to preserve
type safety and consistency with ParsingContext<?> used elsewhere (refer to
decideClazz and ensureSingleKey/ParsingContext usages).
- Line 58: The code calls root(rootSourceFile) and constructs a SourceMetadata
implicitly when rootSourceFile is null; change the call to use
SourceMetadata.empty() for clarity by passing SourceMetadata.empty() (or using
it where root() is invoked) instead of relying on normalizePath/root to produce
null—update GenericYamlParser.java to detect null rootSourceFile and call
root(SourceMetadata.empty()) or otherwise use SourceMetadata.empty() so intent
is explicit (referencing rootSourceFile, normalizePath, root(), and
SourceMetadata.empty()).
In `@core/src/test/java/com/predic8/membrane/core/util/OSUtilTest.java`:
- Around line 31-44: Add tests to OSUtilTest::windowsAbsolutePath to cover
lowercase drive letters and inputs with invalid first characters: call
isWindowsAbsolutePath with strings like "c:\\temp" and "c:/temp" and assertTrue,
and add negative cases such as "?:\\path", "1:\\path" or strings starting with
an invalid drive-char to assertFalse; ensure null/empty/UNC/unix/relative
assertions remain unchanged and only the additional edge-case assertions are
appended to the existing test method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f7d72c49-bbe9-4b96-b9b9-27b6547fb7a6
📒 Files selected for processing (7)
annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.javaannot/src/main/java/com/predic8/membrane/annot/yaml/parsing/GenericYamlParser.javaannot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/SetterResolver.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.javacore/src/main/java/com/predic8/membrane/core/util/OSUtil.javacore/src/test/java/com/predic8/membrane/core/util/OSUtilTest.javadistribution/examples/configuration/include/README.md
✅ Files skipped from review due to trivial changes (1)
- distribution/examples/configuration/include/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java
- annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/SetterResolver.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests / Docs