fix: replace Janino EvaluatorFilter expression with a native evaluator - #81
Conversation
default-logback.xml's EvaluatorFilter used a Janino-compiled expression (reflectionsEmpty.matches(message)) to suppress Reflections' noisy "given scan urls are empty" warning. JaninoEventEvaluator was removed in Logback 1.5.13, and Terasology is bumping to 1.6.0 - under either version this expression silently falls back to a no-op StubEventEvaluator (confirmed empirically: both the matching and non-matching log lines got through, i.e. the DENY never fired), so the filter would have quietly stopped working instead of failing loudly. Replaced with ReflectionsEmptyEvaluator, a small native EventEvaluator<ILoggingEvent> implementation doing the same regex check in plain Java. Verified in isolation against the real logback-classic 1.6.0 jar that the DENY correctly fires for a matching message and lets a non-matching one through, with no evaluator warnings at startup (the old config printed a "stub for JaninoEventEvaluator" warning on every run). The runtimeOnly Janino dependency this filter needed (terasology-module.gradle.kts, MovingBlocks/Terasology) is no longer required and is being removed there as part of the same change.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a custom Logback evaluator for detecting empty reflections scan warnings and replaces the inline evaluator configuration with the new class while preserving the existing filter outcomes. ChangesReflections warning filtering
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/org/terasology/moduletestingenvironment/logging/ReflectionsEmptyEvaluator.java`:
- Around line 26-27: Update ReflectionsEmptyEvaluator.evaluate to match
REFLECTIONS_EMPTY against event.getMessage() instead of
event.getFormattedMessage(), preserving the evaluator’s behavior for
parameterized logging messages.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: af43ebed-12aa-46a2-b9dd-b47fa91986a8
📒 Files selected for processing (2)
src/main/java/org/terasology/moduletestingenvironment/logging/ReflectionsEmptyEvaluator.javasrc/main/resources/org/terasology/moduletestingenvironment/default-logback.xml
|
Tested locally alongside MovingBlocks/Terasology#5343 on an Omega workspace. Looks good — the evaluator implements A/B on
One caveat on the unchecked test-plan item. I could not verify the Also worth rewording that item: " Merge order: after #5343 — the evaluator needs the Logback 1.6.0 that |
There was a problem hiding this comment.
Pull request overview
This PR updates ModuleTestingEnvironment’s default Logback configuration to keep suppressing Reflections’ “given scan urls are empty” warning after Logback removed Janino-based evaluators, by replacing the removed Janino expression with a native Java EventEvaluator.
Changes:
- Replaced the Janino
EvaluatorFilterexpression indefault-logback.xmlwith a custom evaluator class reference. - Added
ReflectionsEmptyEvaluator, a JavaEventEvaluator<ILoggingEvent>that matches the Reflections “scan urls are empty” warning text.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/main/resources/org/terasology/moduletestingenvironment/default-logback.xml | Switches the EvaluatorFilter to use a native evaluator class instead of a Janino expression. |
| src/main/java/org/terasology/moduletestingenvironment/logging/ReflectionsEmptyEvaluator.java | Implements the custom Logback evaluator used by the XML configuration to deny matching Reflections warnings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Override | ||
| public boolean evaluate(ILoggingEvent event) throws EvaluationException { | ||
| return REFLECTIONS_EMPTY.matcher(event.getFormattedMessage()).find(); | ||
| } |
| public class ReflectionsEmptyEvaluator extends ContextAwareBase implements EventEvaluator<ILoggingEvent>, LifeCycle { | ||
|
|
||
| private static final Pattern REFLECTIONS_EMPTY = Pattern.compile("given scan urls are empty"); | ||
|
|
||
| private String name; | ||
| private boolean started; | ||
|
|
||
| @Override | ||
| public boolean evaluate(ILoggingEvent event) throws EvaluationException { | ||
| return REFLECTIONS_EMPTY.matcher(event.getFormattedMessage()).find(); | ||
| } |
Matches the `message` binding the Janino expression used, and avoids denying events whose arguments happen to contain the phrase. Null-guarded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cervator
left a comment
Cervator
left a comment
There was a problem hiding this comment.
Sent through usual counter-agent review process and added a little tweak. Merging after the engine PR finished (gotta run, not waiting the 10 minutes for Jenkins to say it is still all-ok)
Summary
default-logback.xml'sEvaluatorFilterused a Janino-compiled expression (reflectionsEmpty.matches(message)) to suppress Reflections' noisy "given scan urls are empty" warning -JaninoEventEvaluatorwas actually removed back in Logback 1.5.13, well before this jump.StubEventEvaluatorinstead of failing loudly - both a matching and a non-matching log line got through in a test run, i.e. theDENYnever fired. The filter would have quietly stopped working rather than crashing, so this was easy to miss.ReflectionsEmptyEvaluator, a small nativeEventEvaluator<ILoggingEvent>implementation doing the same regex check in plain Java (Logback's suggested migration path for removed Janino expressions).logback-classic:1.6.0jar that theDENYcorrectly fires for a matching message and lets a non-matching one through, with no evaluator warnings at startup (the old config printed a "stub for JaninoEventEvaluator" warning on every run).runtimeOnlyJanino dependency this filter needed (terasology-module.gradle.kts, in the main Terasology repo) is no longer required and is being removed there as part of the companion PR.Test plan
logback-classic:1.6.0, correctly denies a message matching "given scan urls are empty" and allows a non-matching one through, no startup warnings.ModuleTestingEnvironmentand confirm the Reflections scan-empty warning is still suppressed in test output.Related
Summary by CodeRabbit