Avoid final logger injection in EventSpyDispatcher#12145
Conversation
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
The fix itself is correct and follows the established convention in the codebase (private static final Logger LOGGER). This avoids Sisu mutating the final instance field on JDK 26+.
One thought on the test -- see inline comment.
Claude Code on behalf of Guillaume Nodet
| .anyMatch(field -> !Modifier.isStatic(field.getModifiers()) && Modifier.isFinal(field.getModifiers())); | ||
|
|
||
| assertFalse(hasFinalInstanceLogger); | ||
| } |
There was a problem hiding this comment.
This test guards against reintroducing a final instance Logger field, but it would not catch someone reintroducing a non-final instance Logger field (which would still be subject to Sisu injection and equally problematic). A simpler and broader guard would be to assert that no instance Logger field exists at all:
| } | |
| boolean hasInstanceLogger = Arrays.stream(EventSpyDispatcher.class.getDeclaredFields()) | |
| .filter(field -> Logger.class.equals(field.getType())) | |
| .anyMatch(field -> !Modifier.isStatic(field.getModifiers())); | |
| assertFalse(hasInstanceLogger, "Logger field should be static to avoid Sisu reflective mutation"); | |
| } |
There was a problem hiding this comment.
IMHO testing final presence is a little redundant ...
|
I'm not sure about we finally resolve a problem when Sisu mutating the final instance field ... we have many other places in code with final without static. |
|
@Will-thom please always preserve last paragraph from PR template
Please also consider to sign - https://www.apache.org/licenses/icla.pdf |
|
I would rather stop sisu/plexus (and i think this is rather plexus) doing this, AFAIR there was plan to inject Logger instances that was never finalized as Plexus Loggers are still in play (goal was to fully switch to slf4j) |
|
In this case logger is slf4j ... but in some case sisu try to update such filed |
|
But something is off then, as in Maven Core of 3.10.x we should not have Plexus Components anymore, need to look more... |
|
@Will-thom can you provide a reproducer for refd issue? |
|
Also, this issue reminds me of dea5f14 There, we thought we migrated the component to Sisu (from Plexus) but in fact, we did not. So, if something still defines |
4 participants
Fixes #12115.\n\nThis changes EventSpyDispatcher to use a static logger so Sisu does not mutate a final instance logger field reflectively on newer JDKs. It also adds a small regression test that guards against reintroducing a final instance Logger field.\n\nValidation:\n- mvn -B -pl maven-core -Dtest=EventSpyDispatcherTest test\n- mvn -B -pl maven-core test