You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "Will-thom (via GitHub)" <gi...@apache.org> on 2026/05/23 23:20:48 UTC
[PR] Avoid final logger injection in EventSpyDispatcher [maven]
Will-thom opened a new pull request, #12145:
URL: https://github.com/apache/maven/pull/12145
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid final logger injection in EventSpyDispatcher [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #12145:
URL: https://github.com/apache/maven/pull/12145#issuecomment-4528631541
Also, this issue reminds me of https://github.com/apache/maven/commit/dea5f14a85e26291f654a4c4a29a58210293628b
There, we _thought_ we migrated the component to Sisu (from Plexus) but in fact, we did not. So, if something still defines `EventSpyDispatcher` as Plexus component (instead of Sisu JSR330 component), then yes, this could happen. Otherwise, I don't know what exactly happens here...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid final logger injection in EventSpyDispatcher [maven]
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on PR #12145:
URL: https://github.com/apache/maven/pull/12145#issuecomment-4528474022
In this case logger is slf4j ... but in some case sisu try to update such filed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid final logger injection in EventSpyDispatcher [maven]
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on PR #12145:
URL: https://github.com/apache/maven/pull/12145#issuecomment-4527857475
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid final logger injection in EventSpyDispatcher [maven]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo closed pull request #12145: Avoid final logger injection in EventSpyDispatcher
URL: https://github.com/apache/maven/pull/12145
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid final logger injection in EventSpyDispatcher [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #12145:
URL: https://github.com/apache/maven/pull/12145#issuecomment-4528528040
See
https://github.com/eclipse-sisu/sisu-project/blob/a2d1d99282f37d366e7fe417c8c0fea4897c5704/org.eclipse.sisu.plexus/src/main/java/org/eclipse/sisu/plexus/PlexusLifecycleManager.java#L105
But something is off then, as in Maven Core of 3.10.x we should not have Plexus Components anymore, need to look more...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid final logger injection in EventSpyDispatcher [maven]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on PR #12145:
URL: https://github.com/apache/maven/pull/12145#issuecomment-6248884305
Linked issue closed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid final logger injection in EventSpyDispatcher [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #12145:
URL: https://github.com/apache/maven/pull/12145#issuecomment-4528566313
@Will-thom can you provide a reproducer for refd issue?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid final logger injection in EventSpyDispatcher [maven]
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on PR #12145:
URL: https://github.com/apache/maven/pull/12145#issuecomment-4527866926
@Will-thom please always preserve last paragraph from PR template
> To make clear that you license your contribution under the Apache License Version 2.0, January 2004 you have to acknowledge this by using the following check-box.
...
Please also consider to sign - https://www.apache.org/licenses/icla.pdf
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid final logger injection in EventSpyDispatcher [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #12145:
URL: https://github.com/apache/maven/pull/12145#discussion_r3293748828
##########
maven-core/src/test/java/org/apache/maven/eventspy/internal/EventSpyDispatcherTest.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.eventspy.internal;
+
+import java.lang.reflect.Modifier;
+import java.util.Arrays;
+
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+
+class EventSpyDispatcherTest {
+
+ @Test
+ void doesNotExposeFinalInstanceLoggerFieldForInjection() {
+ boolean hasFinalInstanceLogger = Arrays.stream(EventSpyDispatcher.class.getDeclaredFields())
+ .filter(field -> Logger.class.equals(field.getType()))
+ .anyMatch(field -> !Modifier.isStatic(field.getModifiers()) && Modifier.isFinal(field.getModifiers()));
+
+ assertFalse(hasFinalInstanceLogger);
+ }
Review 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:
```suggestion
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");
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid final logger injection in EventSpyDispatcher [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #12145:
URL: https://github.com/apache/maven/pull/12145#issuecomment-4527959904
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid final logger injection in EventSpyDispatcher [maven]
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on code in PR #12145:
URL: https://github.com/apache/maven/pull/12145#discussion_r3294146522
##########
maven-core/src/test/java/org/apache/maven/eventspy/internal/EventSpyDispatcherTest.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.eventspy.internal;
+
+import java.lang.reflect.Modifier;
+import java.util.Arrays;
+
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+
+class EventSpyDispatcherTest {
+
+ @Test
+ void doesNotExposeFinalInstanceLoggerFieldForInjection() {
+ boolean hasFinalInstanceLogger = Arrays.stream(EventSpyDispatcher.class.getDeclaredFields())
+ .filter(field -> Logger.class.equals(field.getType()))
+ .anyMatch(field -> !Modifier.isStatic(field.getModifiers()) && Modifier.isFinal(field.getModifiers()));
+
+ assertFalse(hasFinalInstanceLogger);
+ }
Review Comment:
IMHO testing final presence is a little redundant ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid final logger injection in EventSpyDispatcher [maven]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #12145:
URL: https://github.com/apache/maven/pull/12145#discussion_r3427691771
##########
maven-core/src/test/java/org/apache/maven/eventspy/internal/EventSpyDispatcherTest.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.eventspy.internal;
+
+import java.lang.reflect.Modifier;
+import java.util.Arrays;
+
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+
+class EventSpyDispatcherTest {
+
+ @Test
+ void doesNotExposeFinalInstanceLoggerFieldForInjection() {
+ boolean hasFinalInstanceLogger = Arrays.stream(EventSpyDispatcher.class.getDeclaredFields())
+ .filter(field -> Logger.class.equals(field.getType()))
+ .anyMatch(field -> !Modifier.isStatic(field.getModifiers()) && Modifier.isFinal(field.getModifiers()));
+
+ assertFalse(hasFinalInstanceLogger);
+ }
Review Comment:
I don't feel strongly, but it does feel like this is testing too much of the internal implementation details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid final logger injection in EventSpyDispatcher [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #12145:
URL: https://github.com/apache/maven/pull/12145#issuecomment-5816007144
I agree with @gnodet this PR misses the point, as Plexus DI is not used anymore in Maven 3.10 (it uses Sisu DI/JSR330).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org