[MNG-8729] Use correct `outputStream` destination; `request` instead of `path` in `DefaultPluginXmlFactory#write` #2312

Pankraz76 · 2025-05-09T20:02:23Z

JIRA issue: MNG-8729

follow up of:

Fix unused stream in DefaultPluginXmlFactory#write #2303

ref:

https://docs.pmd-code.org/latest/pmd_rules_java_bestpractices.html#unusedassignment

finally 100

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java

Pankraz76 · 2025-05-10T19:51:11Z

waiting for enabler

Fix unused stream in DefaultPluginXmlFactory#write #2303

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelXmlFactory.java

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java

Pankraz76 · 2025-05-11T12:50:53Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java

-     * @throws XmlReaderException if an error occurs during the parsing
-     * @see #toXmlString(Object)
-     */
-    public static PluginDescriptor fromXml(@Nonnull String xml) throws XmlReaderException {


do we need this, as seems unused?

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java

gnodet

The point of this PR is to fix the call with the output stream. Please rename it accordingly.

Pankraz76 · 2025-05-13T11:19:36Z

rdy. lets hope windows-ci make is this time.

gnodet · 2025-05-13T12:07:28Z

The UTs are failing on windows

Pankraz76 · 2025-05-13T12:22:03Z

yes thx. items:

Error: DefaultPluginXmlFactoryReadWriteTest.readFromUrlParsesPluginDescriptorCorrectly � JUnit Failed to close extension context

Error: org.apache.maven.impl.DefaultPluginXmlFactoryReadWriteTest.readFromUrlParsesPluginDescriptorCorrectly -- Time elapsed: 0.016 s <<< ERROR!
org.junit.platform.commons.JUnitException: Failed to close extension context
at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: java.io.IOException: Failed to delete temp directory C:\Users\RUNNER~1\AppData\Local\Temp\junit-6477311588346591642. The following paths could not be deleted (see suppressed exceptions for details): , plugin.xml

Pankraz76 · 2025-05-13T12:30:03Z

yes thx. items:

Error: DefaultPluginXmlFactoryReadWriteTest.readFromUrlParsesPluginDescriptorCorrectly � JUnit Failed to close extension context

Error: org.apache.maven.impl.DefaultPluginXmlFactoryReadWriteTest.readFromUrlParsesPluginDescriptorCorrectly -- Time elapsed: 0.016 s <<< ERROR! org.junit.platform.commons.JUnitException: Failed to close extension context at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) Caused by: java.io.IOException: Failed to delete temp directory C:\Users\RUNNER~1\AppData\Local\Temp\junit-6477311588346591642. The following paths could not be deleted (see suppressed exceptions for details): , plugin.xml

@Disabled("maybe bug in Junit, as MS-DOS only https://github.com/apache/maven/pull/2312#issuecomment-2876291814")

… DefaultPluginXmlFactory#write

Pankraz76 · 2025-05-13T12:32:34Z

disabled failing test. found then other disabled and removed that one. Lets see.

michael-o · 2025-05-13T15:02:48Z

We need to fix

maven/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java

Lines 55 to 56 in 717b959

    
           if (path == null && url == null && reader == null && inputStream == null) { 
        
               throw new IllegalArgumentException("path, url, reader or inputStream must be non null");

as well. It has to be an NPE.

Pankraz76 · 2025-05-13T15:21:39Z

It has to be an NPE.

yes this next topic, as global smell not having SPOT (single point of truth).

Appreciate the feedback.

Pankraz76 · 2025-05-13T15:32:01Z

We need to fix

what to be done, except of giving dedication?

extract common functionality of DefaultPluginXmlFactory #2326

Pankraz76 · 2025-05-14T06:24:49Z

merci

…ath in DefaultPluginXmlFactory#write (apache#2312) Co-authored-by: Vincent Potucek <[email protected]>

The Spring codebase sometimes ignores exceptions in catch blocks on purpose. This is often called out by an inline comment. We should make this more obvious by renaming the exception argument in the catch block to declare whether the exception is "ignored" or "expected". See gh-35047 Signed-off-by: Vincent Potucek <[email protected]> [[email protected]: rework commit message] Signed-off-by: Brian Clozel <[email protected]>

jira-importer · 2025-07-07T09:08:12Z

Resolve #9348

jtnord · 2025-08-27T22:24:52Z

yes thx. items:
Error: DefaultPluginXmlFactoryReadWriteTest.readFromUrlParsesPluginDescriptorCorrectly � JUnit Failed to close extension context
Error: org.apache.maven.impl.DefaultPluginXmlFactoryReadWriteTest.readFromUrlParsesPluginDescriptorCorrectly -- Time elapsed: 0.016 s <<< ERROR! org.junit.platform.commons.JUnitException: Failed to close extension context at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) Caused by: java.io.IOException: Failed to delete temp directory C:\Users\RUNNER~1\AppData\Local\Temp\junit-6477311588346591642. The following paths could not be deleted (see suppressed exceptions for details): , plugin.xml

@Disabled("maybe bug in Junit, as MS-DOS only https://github.com/apache/maven/pull/2312#issuecomment-2876291814")

The test has an InputStream that is not closed for this file (plugin.xml). The input stream in the test needs closing in the test (assuming the method it's calling is not supposed to be closing the stream handed to it)

jtnord · 2025-08-27T22:27:44Z

impl/maven-impl/src/test/java/org/apache/maven/impl/DefaultPluginXmlFactoryTest.java

+        Path xmlFile = tempDir.resolve("plugin.xml");
+        Files.write(xmlFile, SAMPLE_PLUGIN_XML.getBytes());
+        PluginDescriptor descriptor = defaultPluginXmlFactory.read(XmlReaderRequest.builder()
+                .inputStream(xmlFile.toUri().toURL().openStream())


This input stream for plugin.xml is not closed, leading to a failure to clean up the test context. (On windows and certain Linux file systems (eg NFS)).
Windows (generally) will not let you delete a file that has open handles even if it is the same process or thread that has the open handle)

Nice catch, raised #11069

jtnord · 2025-08-27T22:30:46Z

impl/maven-impl/src/test/java/org/apache/maven/impl/DefaultPluginXmlFactoryTest.java

+
+    @Test
+    @Disabled("maybe bug in JUnit, as MS-DOS only https://github.com/apache/maven/pull/2312#issuecomment-2876291814")
+    void readFromUrlParsesPluginDescriptorCorrectly() throws Exception {


Am I missing the bit reading from a URL?
The XmlReaderRequest is built using an InputStream not a URL

Pankraz76 · 2025-08-28T06:43:16Z

thanks for feedback.

Pankraz76 mentioned this pull request May 9, 2025

Fix unused stream in DefaultPluginXmlFactory#write #2303

Closed

Pankraz76 force-pushed the fix-write-test branch from c5fa2c3 to 0b37d3b Compare May 10, 2025 19:02

Pankraz76 mentioned this pull request May 10, 2025

Fix redundant code in DefaultPluginXmlFactory:write #2306

Closed

Pankraz76 force-pushed the fix-write-test branch from 0b37d3b to e68ec7d Compare May 10, 2025 19:29

Pankraz76 commented May 10, 2025

View reviewed changes

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java Outdated Show resolved Hide resolved

Pankraz76 commented May 10, 2025

View reviewed changes

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java Outdated Show resolved Hide resolved

Pankraz76 commented May 10, 2025

View reviewed changes

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java Outdated Show resolved Hide resolved

Pankraz76 commented May 10, 2025

View reviewed changes

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java Outdated Show resolved Hide resolved

Pankraz76 force-pushed the fix-write-test branch from 48a2706 to 00c0d7d Compare May 10, 2025 19:50

Pankraz76 marked this pull request as ready for review May 10, 2025 19:50

Pankraz76 commented May 10, 2025

View reviewed changes

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelXmlFactory.java Outdated Show resolved Hide resolved

Pankraz76 mentioned this pull request May 10, 2025

fix spelling leveraging Oxford comma #2316

Closed

gnodet requested changes May 11, 2025

View reviewed changes

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java Outdated Show resolved Hide resolved

Pankraz76 force-pushed the fix-write-test branch from 8c0fd3c to f7e0024 Compare May 11, 2025 12:20

Pankraz76 commented May 11, 2025

View reviewed changes

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java Outdated Show resolved Hide resolved

Pankraz76 commented May 11, 2025

View reviewed changes

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java Outdated Show resolved Hide resolved

Pankraz76 requested a review from gnodet May 11, 2025 12:49

Pankraz76 commented May 11, 2025

View reviewed changes

Pankraz76 force-pushed the fix-write-test branch from bbc549c to c99650c Compare May 11, 2025 13:03

Pankraz76 mentioned this pull request May 11, 2025

[POC] rewrite-maven-plugin: Introduce OpenRewrite by Moderne #2322

Closed

Pankraz76 commented May 12, 2025

View reviewed changes

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java Outdated Show resolved Hide resolved

Pankraz76 commented May 12, 2025

View reviewed changes

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java Show resolved Hide resolved

Pankraz76 mentioned this pull request May 12, 2025

extract common functionality of DefaultPluginXmlFactory #2326

Closed

Pankraz76 force-pushed the fix-write-test branch from c99650c to 0022e03 Compare May 12, 2025 11:50

Pankraz76 mentioned this pull request May 12, 2025

Remove unused DefaultPluginXmlFactory#fromXml/toXml #2327

Closed

gnodet approved these changes May 12, 2025

View reviewed changes

Pankraz76 changed the title ~~Test unused stream in DefaultPluginXmlFactory#write~~ Fix unused variable os use correct outputStream destination; request instead of path in DefaultPluginXmlFactory#write May 12, 2025

Pankraz76 changed the title ~~Fix unused variable os use correct outputStream destination; request instead of path in DefaultPluginXmlFactory#write~~ Fix: Use correct outputStream destination; request instead of path in DefaultPluginXmlFactory#write May 12, 2025

Pankraz76 force-pushed the fix-write-test branch from 0022e03 to b7bf6f2 Compare May 12, 2025 12:18

Pankraz76 force-pushed the fix-write-test branch from 87c33a2 to 6a98647 Compare May 13, 2025 11:17

gnodet changed the title ~~Fix: Use correct outputStream destination; request instead of path in DefaultPluginXmlFactory#write~~ [MNG-8729] Fix: Use correct outputStream destination; request instead of path in DefaultPluginXmlFactory#write May 13, 2025

gnodet changed the title ~~[MNG-8729] Fix: Use correct outputStream destination; request instead of path in DefaultPluginXmlFactory#write~~ [MNG-8729] Use correct outputStream destination; request instead of path in DefaultPluginXmlFactory#write May 13, 2025

gnodet self-assigned this May 13, 2025

gnodet added the bug Something isn't working label May 13, 2025

gnodet added this to the 4.0.0-rc-4 milestone May 13, 2025

Pankraz76 force-pushed the fix-write-test branch from 6a98647 to 75faa01 Compare May 13, 2025 12:29

Fix: Use correct outputStream destination; request instead of path in…

717b959

… DefaultPluginXmlFactory#write

Pankraz76 force-pushed the fix-write-test branch from 75faa01 to 717b959 Compare May 13, 2025 12:30

Pankraz76 mentioned this pull request May 13, 2025

[POC-FIX-PMD] maven-pmd-plugin: reactivate PMD - Best practices #2331

Closed

gnodet merged commit 3e9c164 into apache:master May 14, 2025
13 checks passed

Pankraz76 added a commit to Pankraz76/maven that referenced this pull request May 14, 2025

[MNG-8729] Use correct outputStream destination; request instead of p…

d159dfe

…ath in DefaultPluginXmlFactory#write (apache#2312) Co-authored-by: Vincent Potucek <[email protected]>

Pankraz76 mentioned this pull request May 20, 2025

Revert "[MNG-8729] Use correct outputStream destination; request instead of path in DefaultPluginXmlFactory#write" #2357

Closed

Pankraz76 mentioned this pull request Aug 12, 2025

Apply UnnecessaryParentheses google/guava#7930

Closed

jtnord reviewed Aug 27, 2025

View reviewed changes

jtnord mentioned this pull request Aug 28, 2025

Re enable test on windows #11070

Closed

8 tasks

[MNG-8729] Use correct outputStream destination; request instead of path in DefaultPluginXmlFactory#write #2312

[MNG-8729] Use correct outputStream destination; request instead of path in DefaultPluginXmlFactory#write #2312

Uh oh!

Conversation

Pankraz76 commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Pankraz76 commented May 10, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Pankraz76 May 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

Pankraz76 commented May 13, 2025

Uh oh!

gnodet commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pankraz76 commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pankraz76 commented May 13, 2025

Uh oh!

Pankraz76 commented May 13, 2025

Uh oh!

michael-o commented May 13, 2025

Uh oh!

Pankraz76 commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pankraz76 commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Pankraz76 commented May 14, 2025

Uh oh!

jira-importer commented Jul 7, 2025

Uh oh!

jtnord commented Aug 27, 2025

Uh oh!

jtnord Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnodet Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

jtnord Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

Pankraz76 commented Aug 28, 2025

Uh oh!

Uh oh!

[MNG-8729] Use correct `outputStream` destination; `request` instead of `path` in `DefaultPluginXmlFactory#write` #2312

[MNG-8729] Use correct `outputStream` destination; `request` instead of `path` in `DefaultPluginXmlFactory#write` #2312

Pankraz76 commented May 9, 2025 •

edited

Loading

gnodet commented May 13, 2025 •

edited

Loading

Pankraz76 commented May 13, 2025 •

edited

Loading

Pankraz76 commented May 13, 2025 •

edited

Loading

Pankraz76 commented May 13, 2025 •

edited

Loading

jtnord Aug 27, 2025 •

edited

Loading