Add Yaml include list with base path resolving (Discussion #2817)#2890
Closed
christiangoerdes wants to merge 40 commits intomasterfrom
Closed
Add Yaml include list with base path resolving (Discussion #2817)#2890christiangoerdes wants to merge 40 commits intomasterfrom
christiangoerdes wants to merge 40 commits intomasterfrom
Conversation
Collaborator
Author
|
/ok-to-test |
Contributor
|
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:
📝 WalkthroughWalkthroughAdds YAML include support with per-file source metadata and include-cycle detection; introduces BeanDefinition-aware context and IncludeList generator; propagates bean source base locations across many components; updates annotation processor, YAML parser, CLI, tests, and examples to support includes and bean-aware resource resolution. Changes
Sequence DiagramsequenceDiagram
participant CLI as RouterCLI
participant Parser as GenericYamlParser
participant Resolver as IncludeResolver
participant Context as BeanDefinitionContext
participant Container as BeanContainer
participant Component as BeanDefinitionAware Component
CLI->>Parser: parseMembraneResources(stream, grammar, rootSourceFile)
Parser->>Resolver: collect include entries (files & dirs)
Resolver->>Resolver: expand dirs (*.apis.yaml|yml), detect cycles
Resolver-->>Parser: return ordered include snippets
Parser->>Context: create SourceMetadata per file
Parser->>Container: define(beanDefinition with sourceMetadata)
Container->>Context: push(beanDefinition)
Container->>Container: instantiate bean
Container->>Component: if BeanDefinitionAware -> setBeanDefinition(def)
Container->>Context: pop()
Parser-->>CLI: return BeanDefinitions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
|
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: 2
🧹 Nitpick comments (3)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2server/LoginDialog2.java (2)
96-112: Consider adding error handling forindex.htmlresolution.Unlike
LoginDialog.javawhich wraps theindex.htmlcheck in try-catch with meaningful error messages, thisinit()method will throw a rawResourceRetrievalExceptionif the file doesn't exist. Consider adding similar error handling for consistency and better user experience:♻️ Suggested improvement
public void init(Router router) throws Exception { String effectiveBaseLocation = locationBaseLocation == null ? router.getConfiguration().getBaseLocation() : locationBaseLocation; String resolvedDialogBaseLocation = ResolverMap.combine(effectiveBaseLocation, dialogLocation); wsi.setDocBase(resolvedDialogBaseLocation); uriFactory = router.getConfiguration().getUriFactory(); wsi.init(router); - router.getResolverMap().resolve(ResolverMap.combine(asDirectory(resolvedDialogBaseLocation), "index.html")).close(); - + try { + router.getResolverMap().resolve(ResolverMap.combine(asDirectory(resolvedDialogBaseLocation), "index.html")).close(); + } catch (ResourceRetrievalException e) { + throw new ConfigurationException(""" + Cannot access index.html at: + Location base: %s + Doc base: %s + """.formatted(effectiveBaseLocation, dialogLocation), e); + } }🤖 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/oauth2server/LoginDialog2.java` around lines 96 - 112, The init method currently calls router.getResolverMap().resolve(ResolverMap.combine(asDirectory(resolvedDialogBaseLocation), "index.html")).close() without handling failures; wrap that resolve(...).close() call in a try-catch (catch ResourceRetrievalException or the specific exception thrown by resolve) inside init(), and on failure log a clear, user-friendly message and either rethrow a wrapped exception with context or handle gracefully (similar to LoginDialog.java). Target the init(Router) method and the resolve(...) call (using ResolverMap.combine and asDirectory) so the missing or unreadable index.html produces a controlled error message instead of a raw exception.
106-111:asDirectory()is duplicated fromLoginDialog.java.This helper method is identical to the one in
LoginDialog.java. Consider extracting it to a shared utility class (e.g.,PathUtilsor withinResolverMap) to avoid duplication.🤖 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/oauth2server/LoginDialog2.java` around lines 106 - 111, The asDirectory(String location) method in LoginDialog2.java is a duplicate of the one in LoginDialog.java; extract it to a shared utility (e.g., create PathUtils.asDirectory(String) or add it to ResolverMap) and replace both LoginDialog.asDirectory(...) and LoginDialog2.asDirectory(...) calls with the new shared method to eliminate duplication; ensure the new method preserves the exact behavior (null/empty passthrough and appending '/' when needed) and update imports/usages accordingly.core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserIncludeListTest.java (1)
146-149: Optionally harden exception-message assertions against null messages.You call
ex.getMessage()multiple times; a null message would throwNullPointerExceptionand obscure the test failure cause. Consider asserting non-null once and grouping checks.♻️ Optional refactor
- assertTrue(ex.getMessage().contains("Cyclic include detected"), ex.getMessage()); - assertTrue(ex.getMessage().contains("a.apis.yaml"), ex.getMessage()); - assertTrue(ex.getMessage().contains("b.apis.yaml"), ex.getMessage()); + String message = ex.getMessage(); + assertNotNull(message); + assertTrue(message.contains("Cyclic include detected"), message); + assertTrue(message.contains("a.apis.yaml"), message); + assertTrue(message.contains("b.apis.yaml"), message);- assertTrue(ex.getMessage().contains("Duplicate component id '#/components/auth'"), ex.getMessage()); + String message = ex.getMessage(); + assertNotNull(message); + assertTrue(message.contains("Duplicate component id '#/components/auth'"), message);Also applies to: 176-176
🤖 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/kubernetes/GenericYamlParserIncludeListTest.java` around lines 146 - 149, The exception-message assertions in GenericYamlParserIncludeListTest call ex.getMessage() multiple times which can NPE; first assert that ex.getMessage() is not null (e.g., capture it into a local String msg = ex.getMessage() or call assertNotNull(ex.getMessage()) once), then reuse that msg for subsequent contains checks (the existing contains checks for "Cyclic include detected", "a.apis.yaml", "b.apis.yaml"); apply the same pattern to the other occurrence around line 176 to harden the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java`:
- Around line 230-231: The call to Path.of(pathFromFileURI(location)) can throw
for non-file URIs (http/https); update the code around getConfigDefinition(...
parseYamlBeanDefinitions(..., grammar, Path.of(...))) to detect whether location
is a file-based URI before converting: use pathFromFileURI(location) only when
the resolved URI has a file scheme (or no scheme/local path) — e.g. inspect new
URI(location).getScheme() or use router.getResolverMap().resolve(location)
metadata — and when it's not file-based pass null (or omit the rootSourceFile
argument) to parseYamlBeanDefinitions so remote URLs are handled without
Path.of; adjust the call site (the expression passed into
parseYamlBeanDefinitions) accordingly and keep getConfigDefinition and
router.applyConfiguration usage unchanged.
In `@distribution/examples/configuration/includes/membrane.cmd`:
- Around line 1-24: The batch script uses LF-only endings which breaks Windows
parsing of labels and goto/call; save the file with CRLF line endings so labels
like :search_up and :found and invocations of
"%MEMBRANE_HOME%\scripts\run-membrane.cmd" work correctly—open the file in your
editor or run a conversion tool (or set core.autocrlf) and rewrite the file with
CRLF line endings, then verify the MEMBRANE_HOME/MEMBRANE_CALLER_DIR logic and
that call "%MEMBRANE_HOME%\scripts\run-membrane.cmd" %* executes as expected.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/oauth2server/LoginDialog2.java`:
- Around line 96-112: The init method currently calls
router.getResolverMap().resolve(ResolverMap.combine(asDirectory(resolvedDialogBaseLocation),
"index.html")).close() without handling failures; wrap that resolve(...).close()
call in a try-catch (catch ResourceRetrievalException or the specific exception
thrown by resolve) inside init(), and on failure log a clear, user-friendly
message and either rethrow a wrapped exception with context or handle gracefully
(similar to LoginDialog.java). Target the init(Router) method and the
resolve(...) call (using ResolverMap.combine and asDirectory) so the missing or
unreadable index.html produces a controlled error message instead of a raw
exception.
- Around line 106-111: The asDirectory(String location) method in
LoginDialog2.java is a duplicate of the one in LoginDialog.java; extract it to a
shared utility (e.g., create PathUtils.asDirectory(String) or add it to
ResolverMap) and replace both LoginDialog.asDirectory(...) and
LoginDialog2.asDirectory(...) calls with the new shared method to eliminate
duplication; ensure the new method preserves the exact behavior (null/empty
passthrough and appending '/' when needed) and update imports/usages
accordingly.
In
`@core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserIncludeListTest.java`:
- Around line 146-149: The exception-message assertions in
GenericYamlParserIncludeListTest call ex.getMessage() multiple times which can
NPE; first assert that ex.getMessage() is not null (e.g., capture it into a
local String msg = ex.getMessage() or call assertNotNull(ex.getMessage()) once),
then reuse that msg for subsequent contains checks (the existing contains checks
for "Cyclic include detected", "a.apis.yaml", "b.apis.yaml"); apply the same
pattern to the other occurrence around line 176 to harden the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0b045007-3f3d-423a-8138-02f6a9a2e93f
📒 Files selected for processing (59)
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/BeanDefinitionAware.javaannot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinitionContext.javaannot/src/main/java/com/predic8/membrane/annot/generator/IncludeListClassGenerator.javaannot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.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/test/java/com/predic8/membrane/core/config/spring/k8s/EnvelopeTest.javacore/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserIncludeListTest.javadistribution/examples/configuration/includes/README.mddistribution/examples/configuration/includes/apis.yamldistribution/examples/configuration/includes/includes/directory/10-first.apis.ymldistribution/examples/configuration/includes/includes/directory/20-second.apis.yamldistribution/examples/configuration/includes/includes/directory/ignored.yamldistribution/examples/configuration/includes/includes/file.apis.yamldistribution/examples/configuration/includes/includes/nested/nested.apis.yamldistribution/examples/configuration/includes/membrane.cmddistribution/examples/configuration/includes/membrane.shdistribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/ConfigurationIncludesExampleTest.java
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java
Outdated
Show resolved
Hide resolved
predic8
reviewed
Mar 25, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
distribution/examples/configuration/include/membrane.cmd (1)
1-24:⚠️ Potential issue | 🟠 MajorUse CRLF line endings for this
.cmdfile.The file appears to be LF-only. Batch scripts should be committed with CRLF to avoid Windows parsing issues around labels/
goto/call.#!/bin/bash python - <<'PY' from pathlib import Path p = Path("distribution/examples/configuration/include/membrane.cmd") b = p.read_bytes() crlf = b.count(b"\r\n") lf_total = b.count(b"\n") print("CRLF count:", crlf) print("LF-only count:", lf_total - crlf) PYExpected result:
LF-only count: 0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@distribution/examples/configuration/include/membrane.cmd` around lines 1 - 24, Convert this Windows batch file to use CRLF line endings throughout and recommit the file so the LF-only count becomes 0; ensure every line (including labels like :search_up, :found, :notfound and statements that set MEMBRANE_HOME / MEMBRANE_CALLER_DIR and the call to scripts\run-membrane.cmd) ends with CRLF, or add a .gitattributes entry to force CRLF for *.cmd and re-normalize if your repo is converting line endings automatically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@distribution/examples/configuration/include/membrane.cmd`:
- Around line 1-24: Convert this Windows batch file to use CRLF line endings
throughout and recommit the file so the LF-only count becomes 0; ensure every
line (including labels like :search_up, :found, :notfound and statements that
set MEMBRANE_HOME / MEMBRANE_CALLER_DIR and the call to
scripts\run-membrane.cmd) ends with CRLF, or add a .gitattributes entry to force
CRLF for *.cmd and re-normalize if your repo is converting line endings
automatically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 126a2657-13a9-4638-a628-bd4f54fa1271
📒 Files selected for processing (9)
distribution/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 skipped from review due to trivial changes (7)
- distribution/examples/configuration/include/customers/customers.apis.yaml
- distribution/examples/configuration/include/orders/orders.apis.yaml
- distribution/examples/configuration/include/orders/orders-response.json.template
- distribution/examples/configuration/include/apis.yaml
- distribution/examples/configuration/include/customers/customers-response.json.template
- distribution/examples/configuration/include/README.md
- distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/ConfigurationIncludesExampleTest.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java`:
- Around line 282-294: getRootSourceFile currently treats any input parsed by
new URI(location) with a non-file scheme as remote and returns null, which
misclassifies Windows absolute paths like "C:/..." as having scheme "C"; before
the existing scheme check, detect Windows absolute paths (e.g., pattern like
/^[a-zA-Z]:[\\/]/ or similar) and, if matched, skip the scheme-based remote
check so the code proceeds to call pathFromFileURI; update getRootSourceFile to
perform this Windows-path check prior to using uri.getScheme() while keeping the
existing try/catch behavior and fallback to pathFromFileURI/InvalidPathException
handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f516ae97-0b5d-425d-8b17-7c803950573a
📒 Files selected for processing (1)
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java
predic8
reviewed
Mar 25, 2026
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinition.java
Show resolved
Hide resolved
predic8
reviewed
Mar 25, 2026
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinition.java
Show resolved
Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/GenericYamlParser.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java
Outdated
Show resolved
Hide resolved
predic8
previously requested changes
Mar 25, 2026
# Conflicts: # core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java
Collaborator
Author
|
See #2898 |
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
2 participants
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
Bug Fixes
Tests