Builtin Functions for Groovy - Unify Builtin Functions#2322
Conversation
|
Warning Rate limit exceeded@christiangoerdes has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughIntroduces a centralized public utility class Changes
Sequence Diagram(s)sequenceDiagram
participant Template as Groovy/SpEL template
participant Scripting as ScriptingUtils
participant GroovyFn as GroovyBuiltInFunctions
participant SpELFn as SpELBuiltInFunctions
participant Common as CommonBuiltInFunctions
participant Exchange as Exchange
participant Flow as Flow
participant Message as Message
rect rgb(230,240,255)
Note over Template,Scripting: Parameter binding creation
Template->>Scripting: requestBindings(exchange, flow)
Scripting-->>Template: provide "fn" -> GroovyBuiltInFunctions(exchange, flow)
end
rect rgb(240,255,230)
Note over Template,GroovyFn: Groovy calls built-ins (instance)
Template->>GroovyFn: fn.jsonPath(path) / fn.user() / fn.hasScope(...)
GroovyFn->>Common: delegate(exchange, flow, args)
Common->>Exchange: read message / inspect security schemes
Exchange-->>Common: data
Common-->>GroovyFn: result
GroovyFn-->>Template: result
end
rect rgb(255,245,230)
Note over Template,SpELFn: SpEL calls built-ins (static)
Template->>SpELFn: SpELBuiltInFunctions.jsonPath(path, ctx) / hasScope(ctx)
SpELFn->>Common: delegate(ctx.getExchange(), ctx.getFlow(), args)
Common->>Message: parse/inspect as needed
Common-->>SpELFn: result
SpELFn-->>Template: result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 1
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/lang/ScriptingUtils.java (1)
107-109: Acknowledge the TODO for test coverage.The implementation correctly exposes Groovy built-in functions via the
fnbinding. However, the TODO indicates that tests are missing for both the Groovy interceptor and Template interceptor.Do you want me to help generate test cases for the Groovy built-in functions, or open an issue to track this task?
core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java (1)
7-10: Acknowledge the TODO for additional function wrappers.The TODO indicates that other SpEL built-in functions (like
jsonPath,weight,isLoggedIn,scopes,hasScope, etc. fromBuiltInFunctions.java) should also be wrapped here to achieve full parity between SpEL and Groovy contexts.Do you want me to help identify which functions from
BuiltInFunctions.javashould be wrapped next, or open an issue to track this work?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/ScriptingUtils.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctions.java(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-22T19:49:29.473Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2156
File: pom.xml:0-0
Timestamp: 2025-09-22T19:49:29.473Z
Learning: In the Membrane API Gateway project, jackson-annotations is pinned to version "2.20" instead of using ${jackson.version} (2.20.0) because version 2.20.0 is not available in Maven repository yet for jackson-annotations, while it is available for jackson-core and jackson-databind. This version inconsistency is intentional and necessary due to different release schedules between Jackson modules.
Applied to files:
core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctions.java
🧬 Code graph analysis (3)
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (1)
core/src/main/java/com/predic8/membrane/core/exchange/Exchange.java (1)
Exchange(31-220)
core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctions.java (1)
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (1)
CommonBuiltInFunctions(16-27)
core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java (1)
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (1)
CommonBuiltInFunctions(16-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (7)
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (1)
10-15: Acknowledge the TODO for function migration.The TODO notes the intent to migrate more function implementations from SpEL's BuiltInFunctions to this shared location, promoting code reuse between SpEL and Groovy contexts.
core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctions.java (4)
19-19: LGTM: Import added for CommonBuiltInFunctions.The import is necessary to access the shared
CommonBuiltInFunctions.user()method.
110-112: LGTM: User function delegates to shared implementation.The delegation to
CommonBuiltInFunctions.user()promotes code reuse between SpEL and Groovy contexts, achieving the PR's goal of unifying built-in functions.
128-130: LGTM: Helper method extracts property retrieval logic.The
getSecuritySchemeshelper cleanly encapsulates the retrieval ofSECURITY_SCHEMES, improving maintainability and enabling consistent null handling viaOptional.ofNullableat the call site.
118-130: Backward compatibility verified—no changes needed.The refactored
getSchemeScopes()correctly flattens scopes from all matching security schemes into a singleList<String>. The public API methods (scopes(),hasScope()variants) work correctly with the flattened result: they check for scope presence, not structure. Overlapping scopes appearing multiple times is expected behavior per the test at line 91 of BuiltInFunctionsTest.java, which explicitly validates the flattened list. No breaking changes detected.core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java (2)
11-17: LGTM: Clean delegation pattern for Groovy integration.The class correctly extends
GroovyObjectSupportfor Groovy runtime integration and stores theExchangeas a final field for immutability.
19-21: LGTM: User function delegates to shared implementation.The delegation to
CommonBuiltInFunctions.user()maintains consistency with the SpEL implementation and achieves the PR's goal of unified built-in functions.
…e from Groovy/SpEL Introduce CommonBuiltInFunctions (jsonPath, scopes/hasScope, isXML/isJSON, base64Encode and helpers), update GroovyBuiltInFunctions to accept Flow and delegate to CommonBuiltInFunctions, and modify SpEL BuiltInFunctions to call the shared implementations. Pass Flow when constructing GroovyBuiltInFunctions in ScriptingUtils.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
core/src/main/java/com/predic8/membrane/core/lang/ScriptingUtils.java (1)
107-109: Reminder: Address the TODO comment.The TODO indicates that tests are missing for built-in functions in Groovy and Template interceptors. Please ensure that the new
fnbinding and its delegated functions are thoroughly tested.Do you want me to generate test cases or open a new issue to track this task?
core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java (2)
6-6: Remove unused import.The import
com.predic8.membrane.core.lang.spel.SpELExchangeEvaluationContextis not used in this file and should be removed.Apply this diff:
-import com.predic8.membrane.core.lang.spel.SpELExchangeEvaluationContext;
11-14: Track remaining function migrations.The TODO indicates that additional SpEL built-in functions (e.g.,
weight(),isLoggedIn(),getDefaultSessionLifetime(),isBearerAuthorization()) should be wrapped here for Groovy. Consider tracking this in a separate issue to ensure feature parity between SpEL and Groovy.Do you want me to open an issue to track the remaining function migrations?
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (2)
63-66: Consider performance implications ofcontainsAll.The
@SuppressWarnings("SlowListContainsAll")indicates awareness of a potential performance issue. Ifscopescan be large, consider whether the scope list should be converted to aSetfor faster lookups. However, if scope lists are typically small (which is common), the current implementation is acceptable.
19-24: Track remaining function migrations.The TODO indicates that additional functions from
BuiltInFunctions.javashould be migrated here. Consider creating a tracking issue for functions likeweight(),isLoggedIn(),getDefaultSessionLifetime(), andisBearerAuthorization()to ensure complete migration.Do you want me to open an issue to track the remaining function migrations from SpEL's BuiltInFunctions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/ScriptingUtils.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctions.java(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-22T19:49:29.473Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2156
File: pom.xml:0-0
Timestamp: 2025-09-22T19:49:29.473Z
Learning: In the Membrane API Gateway project, jackson-annotations is pinned to version "2.20" instead of using ${jackson.version} (2.20.0) because version 2.20.0 is not available in Maven repository yet for jackson-annotations, while it is available for jackson-core and jackson-databind. This version inconsistency is intentional and necessary due to different release schedules between Jackson modules.
Applied to files:
core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctions.java
📚 Learning: 2025-11-15T18:38:23.719Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2322
File: core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java:18-26
Timestamp: 2025-11-15T18:38:23.719Z
Learning: In the membrane/api-gateway repository, the maintainer predic8 prefers fail-fast behavior where NullPointerExceptions are allowed to crash rather than being caught or having defensive null checks that silently return null, as it makes debugging easier by showing where problems originate.
Applied to files:
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java
🧬 Code graph analysis (3)
core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctions.java (1)
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (1)
CommonBuiltInFunctions(25-106)
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (2)
core/src/main/java/com/predic8/membrane/core/lang/spel/SpELExchangeEvaluationContext.java (1)
SpELExchangeEvaluationContext(35-216)core/src/main/java/com/predic8/membrane/core/exchange/Exchange.java (1)
Exchange(31-220)
core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java (3)
core/src/main/java/com/predic8/membrane/core/exchange/Exchange.java (1)
Exchange(31-220)core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (1)
CommonBuiltInFunctions(25-106)core/src/main/java/com/predic8/membrane/core/lang/spel/SpELExchangeEvaluationContext.java (1)
SpELExchangeEvaluationContext(35-216)
🔇 Additional comments (3)
core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctions.java (1)
84-118: LGTM: Clean delegation to CommonBuiltInFunctions.All methods correctly delegate to
CommonBuiltInFunctions, maintaining the SpEL signature requirements while centralizing the implementation. The naming of the unusedctxparameter asignoredinbase64Encodeis a good practice.core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java (1)
26-64: LGTM: Correct delegations to CommonBuiltInFunctions.All wrapper methods correctly delegate to
CommonBuiltInFunctionswith the appropriate parameters (Exchange,Flow,Message) extracted from the instance fields.core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (1)
29-78: LGTM: Well-structured centralized built-in functions.The public API methods are clean and correctly implement the shared logic for SpEL and Groovy. The use of
Optional.ofNullablein private helpers properly handles null cases while allowing fail-fast NPE behavior inuser()as per project conventions.
|
This pull request needs "/ok-to-test" from an authorized committer. |
…egate SpEL/Groovy Move implementations for auth/session/weight/json helpers into CommonBuiltInFunctions, add isBearerAuthorization, weight, isLoggedIn and getDefaultSessionLifetime, and wire SpEL and Groovy built-in resolvers to delegate to the common implementations. Add logging and necessary imports; handle bean resolution failures gracefully.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java (1)
10-13: Clarify or remove the TODO comment.The TODO suggests wrapping "other functions" for parity with SpEL, but comparing with
BuiltInFunctions.java, this class appears to already provide all the corresponding methods. If the work is complete, consider removing this TODO or clarifying what specific functions are still missing.Do you want me to verify completeness by comparing method signatures between
GroovyBuiltInFunctionsandBuiltInFunctions, or would you like help opening an issue to track any remaining work?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctions.java(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-15T18:38:23.728Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2322
File: core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java:18-26
Timestamp: 2025-11-15T18:38:23.728Z
Learning: In the membrane/api-gateway repository, the maintainer predic8 prefers fail-fast behavior where NullPointerExceptions are allowed to crash rather than being caught or having defensive null checks that silently return null, as it makes debugging easier by showing where problems originate.
Applied to files:
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java
🧬 Code graph analysis (3)
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (3)
core/src/main/java/com/predic8/membrane/core/exchange/Exchange.java (1)
Exchange(31-220)core/src/main/java/com/predic8/membrane/core/interceptor/AbstractInterceptorWithSession.java (1)
AbstractInterceptorWithSession(27-105)core/src/main/java/com/predic8/membrane/core/security/BasicHttpSecurityScheme.java (1)
BasicHttpSecurityScheme(16-45)
core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java (2)
core/src/main/java/com/predic8/membrane/core/exchange/Exchange.java (1)
Exchange(31-220)core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (1)
CommonBuiltInFunctions(30-142)
core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctions.java (2)
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (1)
CommonBuiltInFunctions(30-142)core/src/main/java/com/predic8/membrane/core/lang/spel/SpELExchangeEvaluationContext.java (1)
SpELExchangeEvaluationContext(35-216)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (3)
core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctions.java (1)
16-85: LGTM! Clean delegation refactoring.The refactoring successfully centralizes built-in function logic in
CommonBuiltInFunctionswhile maintaining the SpEL convention of passingSpELExchangeEvaluationContextas the last parameter. All delegations correctly extract the necessary context (Exchange, Message, Flow) before calling the common implementation.core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java (1)
14-80: LGTM! Groovy wrapper delegates consistently.The class provides a clean instance-method wrapper for Groovy contexts, properly leveraging the stored
exchangeandflowfields to delegate toCommonBuiltInFunctions. All method signatures and delegations are correct.core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (1)
30-142: LGTM! Well-designed centralized implementation.The
CommonBuiltInFunctionsclass successfully consolidates built-in function logic for both SpEL and Groovy contexts. The implementation demonstrates good practices:
- Consistent delegation patterns with appropriate error handling
- Proper use of
Optional.ofNullablein scope-related methods for null safety- Clean functional programming style in private helpers
- Appropriate silent failures (e.g.,
jsonPathreturning null) and logged failures (e.g.,isLoggedIncatching exceptions) for templating contextsThe fail-fast behavior in methods like
user()aligns with the project's debugging philosophy.Based on learnings.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java (1)
21-24: Consider adding null parameter validation.The constructor doesn't validate that
exchangeandfloware non-null. While these are likely provided by the framework, adding explicit null checks would make failures clearer and prevent NPEs in downstream method calls.Apply this diff to add defensive checks:
public GroovyBuiltInFunctions(Exchange exchange, Flow flow) { + if (exchange == null) { + throw new IllegalArgumentException("exchange must not be null"); + } + if (flow == null) { + throw new IllegalArgumentException("flow must not be null"); + } this.exchange = exchange; this.flow = flow; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java (2)
core/src/main/java/com/predic8/membrane/core/exchange/Exchange.java (1)
Exchange(31-220)core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (1)
CommonBuiltInFunctions(30-142)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (2)
core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java (2)
26-80: Excellent delegation pattern implementation.All methods correctly delegate to
CommonBuiltInFunctionswith appropriate parameter mapping. The instance-levelexchangeandflowfields are properly threaded through to the static utility methods, and the method signatures align well with the common implementation.
15-82: Clean design that successfully unifies built-in functions.The delegation pattern with immutable fields provides a clean wrapper around
CommonBuiltInFunctions, achieving the PR objective of unifying built-in functions across Groovy and SpEL. ExtendingGroovyObjectSupportappropriately enables Groovy runtime integration.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
annot/src/test/java/com/predic8/membrane/annot/util/InMemoryURLStreamHandler.java (1)
55-65: Guard against missingdataentries to avoid NPE ingetInputStreamIf
dataisnullordata.contentdoes not contain theuri,data.content.get(uri)returnsnull, which will cause aNullPointerExceptionwhen passed toByteArrayInputStream. The overlay branch already throws aFileNotFoundExceptionfor missing entries; mirroring that behavior for the basedatabranch would make failures clearer and more consistent.Consider something along these lines:
- return new ByteArrayInputStream(data.content.get(uri)); + if (data == null) { + throw new FileNotFoundException("In-memory file system not activated for " + uri); + } + byte[] buffer = data.content.get(uri); + if (buffer == null) { + throw new FileNotFoundException("No in-memory resource for " + uri); + } + return new ByteArrayInputStream(buffer);This keeps the behavior explicit and avoids a surprising NPE in tests.
🧹 Nitpick comments (2)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/SyncB2CResourceIntegrationTest.java (1)
20-20: Minor formatting: Missing space before opening brace.There's a missing space between the class name and the opening brace:
OAuth2ResourceB2CIntegrationTest{should beOAuth2ResourceB2CIntegrationTest {.Apply this diff:
-public class SyncB2CResourceIntegrationTest extends OAuth2ResourceB2CIntegrationTest{ +public class SyncB2CResourceIntegrationTest extends OAuth2ResourceB2CIntegrationTest {core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (1)
136-154: Don’t swallow beanFactory misconfiguration in session helpersBecause
requireNonNull(exc.getHandler().getTransport().getRouter().getBeanFactory())sits inside thetry, a null beanFactory (misconfigured router) is caught, logged as “Failed to resolve bean…”, and then treated as “not logged in” / lifetime-1. That hides a configuration error that should likely fail fast.You can keep the graceful fallback for “bean not found” while letting real misconfigurations surface by moving the
requireNonNullcall outside thetryin both methods:- public static boolean isLoggedIn(String beanName, Exchange exc) { - try { - return ((AbstractInterceptorWithSession) requireNonNull(exc.getHandler().getTransport().getRouter().getBeanFactory()).getBean(beanName)) - .getSessionManager().getSession(exc).isVerified(); - } catch (Exception e) { - log.info("Failed to resolve bean with name '{}'", beanName); - return false; - } - } + public static boolean isLoggedIn(String beanName, Exchange exc) { + requireNonNull(exc.getHandler().getTransport().getRouter().getBeanFactory()); + try { + return ((AbstractInterceptorWithSession) exc.getHandler().getTransport().getRouter().getBeanFactory().getBean(beanName)) + .getSessionManager().getSession(exc).isVerified(); + } catch (Exception e) { + log.info("Failed to resolve bean with name '{}'", beanName); + return false; + } + } @@ - public static long getDefaultSessionLifetime(String beanName, Exchange exc) { - try { - return ((AbstractInterceptorWithSession) requireNonNull(exc.getHandler().getTransport().getRouter().getBeanFactory()).getBean(beanName)) - .getSessionManager().getExpiresAfterSeconds(); - } catch (Exception e) { - log.info("Failed to resolve bean with name '{}'", beanName); - return -1; - } - } + public static long getDefaultSessionLifetime(String beanName, Exchange exc) { + requireNonNull(exc.getHandler().getTransport().getRouter().getBeanFactory()); + try { + return ((AbstractInterceptorWithSession) exc.getHandler().getTransport().getRouter().getBeanFactory().getBean(beanName)) + .getSessionManager().getExpiresAfterSeconds(); + } catch (Exception e) { + log.info("Failed to resolve bean with name '{}'", beanName); + return -1; + } + }This preserves the “bean missing → false / -1” behavior while surfacing genuine router/beanFactory misconfiguration via an NPE, in line with the fail-fast preference. Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/AbstractSchema.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/AnyOf.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/BasicSchema.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaArray.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaFactory.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaRef.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaString.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/util/SchemaGeneratorUtil.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/ParsingTest.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaObjectTest.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaTest.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/CompositeClassLoader.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/ConcatenatingEnumeration.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/InMemoryClassLoader.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/InMemoryData.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/InMemoryFileObject.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/InMemoryJavaFileObject.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/InMemoryURLStreamHandler.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/InnerFileObject.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/LoggingFileObject.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/LoggingJavaFileObject.java(1 hunks)core/src/main/java/com/predic8/membrane/core/cli/util/JwkGenerator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/cli/util/YamlLoader.java(1 hunks)core/src/main/java/com/predic8/membrane/core/config/xml/XmlConfig.java(1 hunks)core/src/main/java/com/predic8/membrane/core/http/ReadingBodyException.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/log/access/package-info.java(1 hunks)core/src/main/java/com/predic8/membrane/core/kubernetes/WrongEnumConstantException.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpression.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctionException.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctionResolver.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/YamlUtil.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/log/AccessLogCallTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/OAuth2ResourceB2CUnitTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/util/YamlUtilTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/IntegrationTests.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/InMemB2CResourceIntegrationTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/JwtB2CResourceIntegrationTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2ResourceB2CIntegrationTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/SyncB2CResourceIntegrationTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (27)
- annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/AnyOf.java
- annot/src/test/java/com/predic8/membrane/annot/util/InMemoryData.java
- core/src/main/java/com/predic8/membrane/core/cli/util/YamlLoader.java
- annot/src/test/java/com/predic8/membrane/annot/util/InMemoryFileObject.java
- core/src/main/java/com/predic8/membrane/core/cli/util/JwkGenerator.java
- core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/JwtB2CResourceIntegrationTest.java
- annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaRef.java
- annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaString.java
- annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/AbstractSchema.java
- annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaArray.java
- annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaFactory.java
- core/src/main/java/com/predic8/membrane/core/interceptor/log/access/package-info.java
- annot/src/test/java/com/predic8/membrane/annot/util/ConcatenatingEnumeration.java
- core/src/main/java/com/predic8/membrane/core/util/YamlUtil.java
- core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/OAuth2ResourceB2CUnitTest.java
- annot/src/test/java/com/predic8/membrane/annot/util/InMemoryJavaFileObject.java
- annot/src/test/java/com/predic8/membrane/annot/util/LoggingJavaFileObject.java
- annot/src/test/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaTest.java
- annot/src/test/java/com/predic8/membrane/annot/ParsingTest.java
- annot/src/test/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaObjectTest.java
- core/src/test/java/com/predic8/membrane/integration/IntegrationTests.java
- annot/src/main/java/com/predic8/membrane/annot/generator/util/SchemaGeneratorUtil.java
- core/src/main/java/com/predic8/membrane/core/config/xml/XmlConfig.java
- core/src/main/java/com/predic8/membrane/core/kubernetes/WrongEnumConstantException.java
- annot/src/test/java/com/predic8/membrane/annot/util/InnerFileObject.java
- annot/src/test/java/com/predic8/membrane/annot/util/InMemoryClassLoader.java
- annot/src/test/java/com/predic8/membrane/annot/util/LoggingFileObject.java
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-28T16:28:17.849Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 1977
File: core/src/main/java/com/predic8/membrane/core/interceptor/opentelemetry/OpenTelemetryInterceptor.java:107-109
Timestamp: 2025-07-28T16:28:17.849Z
Learning: In Java exception handling for OpenTelemetry body logging, use log.debug() level with ExceptionUtil.concatMessageAndCauseMessages(e) to log body reading failures. This prevents cluttering info logs with potentially common stream reading issues while still providing comprehensive error information when debug logging is enabled.
Applied to files:
core/src/main/java/com/predic8/membrane/core/http/ReadingBodyException.java
📚 Learning: 2025-11-15T18:38:23.728Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2322
File: core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java:18-26
Timestamp: 2025-11-15T18:38:23.728Z
Learning: In the membrane/api-gateway repository, the maintainer predic8 prefers fail-fast behavior where NullPointerExceptions are allowed to crash rather than being caught or having defensive null checks that silently return null, as it makes debugging easier by showing where problems originate.
Applied to files:
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java
🧬 Code graph analysis (1)
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (3)
core/src/main/java/com/predic8/membrane/core/exchange/Exchange.java (1)
Exchange(31-220)core/src/main/java/com/predic8/membrane/core/interceptor/AbstractInterceptorWithSession.java (1)
AbstractInterceptorWithSession(27-105)core/src/main/java/com/predic8/membrane/core/security/BasicHttpSecurityScheme.java (1)
BasicHttpSecurityScheme(16-45)
🔇 Additional comments (14)
core/src/main/java/com/predic8/membrane/core/http/ReadingBodyException.java (1)
1-13: LGTM! License header properly added.The Apache 2.0 license header is correctly formatted and appropriate for the file.
annot/src/test/java/com/predic8/membrane/annot/util/CompositeClassLoader.java (1)
1-14: LGTM! License header correctly added.The Apache 2.0 license header follows the standard format with appropriate copyright year and holder information.
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2ResourceB2CIntegrationTest.java (1)
1-14: LGTM! Standard license header addition.The Apache 2.0 license header is correctly formatted and properly credits predic8 GmbH.
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/InMemB2CResourceIntegrationTest.java (2)
1-14: LGTM! Standard license header addition.The Apache 2.0 license header is correctly formatted.
20-24: LGTM! Appropriate test session manager override.The override correctly provides an
InMemorySessionManagerfor this test implementation, which is suitable for in-memory session testing scenarios.core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/SyncB2CResourceIntegrationTest.java (2)
1-14: LGTM! Standard license header addition.The Apache 2.0 license header is correctly formatted.
21-24: LGTM! Appropriate test session manager override.The override correctly provides a
FakeSyncSessionStoreManagerfor synchronous session store testing.annot/src/test/java/com/predic8/membrane/annot/util/InMemoryURLStreamHandler.java (1)
1-13: Apache 2.0 license header addition looks correctThe header text and formatting are standard, match Apache 2.0, and do not affect behavior. No further changes needed here.
core/src/test/java/com/predic8/membrane/core/interceptor/log/AccessLogCallTest.java (1)
1-13: License header addition looks goodApache 2.0 header is correctly formatted and placed before the package declaration; no impact on the ArchUnit test behavior.
core/src/test/java/com/predic8/membrane/core/util/YamlUtilTest.java (1)
1-13: License header added correctlyApache 2.0 header is consistent with the rest of the project and does not affect the YamlUtil tests.
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/BasicSchema.java (1)
1-25: Constructors cleanly expose superclass initializationAdding both the named and no-arg constructors that delegate directly to
super(...)broadens usage without changing behavior; looks good.core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctionResolver.java (1)
37-45: Exception rename is consistent with new BuiltInFunctionExceptionWrapping invocation failures in
BuiltInFunctionExceptioninstead of the old typoed type keeps behavior the same while aligning with the renamed exception class in this package.core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctionException.java (1)
16-23: LGTM! Typo correction improves naming consistency.The class rename from
BuildInFunctionExceptiontoBuiltInFunctionExceptioncorrectly fixes the spelling and aligns with standard naming conventions for "built-in" functionality.core/src/main/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpression.java (1)
99-99: Typo fix verified as complete. The search for the old misspelled name "BuildInFunctionException" returns no results, and the corrected name "BuiltInFunctionException" is properly used across:
- Exception catch site (line 99, SpELExchangeExpression.java)
- Exception throw site (line 44, BuiltInFunctionResolver.java)
- Class definition (line 16, BuiltInFunctionException.java)
All references have been consistently updated.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/lang/spel/functions/SpELBuiltInFunctions.java (2)
21-28: Consider adding method-level JavaDoc.The class-level documentation clearly explains the architectural constraints. However, individual methods lack JavaDoc describing their purpose, parameters, and return values. Adding method-level documentation would improve discoverability and maintenance, especially since these methods are automatically registered in the SpEL context.
35-37: Consider consistent parameter naming for unused context.The context parameter is named
ignoredinweight()andbase64Encode(), while other methods usectx. While naming itignoredclearly signals the parameter isn't used, usingctxconsistently across all methods would improve uniformity.Also applies to: 83-85
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctionResolver.java(2 hunks)core/src/main/java/com/predic8/membrane/core/lang/spel/functions/SpELBuiltInFunctions.java(1 hunks)core/src/test/java/com/predic8/membrane/core/lang/spel/functions/SpELBuiltInFunctionsTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctionResolver.java
🧰 Additional context used
🧬 Code graph analysis (2)
core/src/main/java/com/predic8/membrane/core/lang/spel/functions/SpELBuiltInFunctions.java (2)
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (1)
CommonBuiltInFunctions(44-156)core/src/main/java/com/predic8/membrane/core/lang/spel/SpELExchangeEvaluationContext.java (1)
SpELExchangeEvaluationContext(35-216)
core/src/test/java/com/predic8/membrane/core/lang/spel/functions/SpELBuiltInFunctionsTest.java (1)
core/src/main/java/com/predic8/membrane/core/lang/spel/functions/SpELBuiltInFunctions.java (1)
SpELBuiltInFunctions(29-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (2)
core/src/test/java/com/predic8/membrane/core/lang/spel/functions/SpELBuiltInFunctionsTest.java (1)
27-134: LGTM! Clean refactoring to use SpELBuiltInFunctions.The test file has been correctly updated to reference the new
SpELBuiltInFunctionsclass. All static imports, class name, and method invocations have been consistently renamed. The test logic and assertions remain unchanged, preserving existing test coverage.core/src/main/java/com/predic8/membrane/core/lang/spel/functions/SpELBuiltInFunctions.java (1)
31-85: LGTM! Clean delegation pattern to CommonBuiltInFunctions.All methods correctly follow the documented constraints:
- Static methods for SpEL registration
- SpELExchangeEvaluationContext as the last parameter
- Proper delegation to CommonBuiltInFunctions with appropriate context extraction
The method overloads for
scopes()andhasScope()are well-structured and cover different use cases.
…me from BasicHttpSecurityScheme
…me from BasicHttpSecurityScheme
…to builtin-functions-for-groovy
…to builtin-functions-for-groovy
…to builtin-functions-for-groovy
…and add CommonBuiltInFunctionsTest using Exchange-based API
This reverts commit 41f8714.
…s username from BasicHttpSecurityScheme" This reverts commit a4d9adf
|
/ok-to-test |
3 participants
Summary by CodeRabbit
New Features
Improvements