Pull #2292: Modernize codebase with Java improvements - SOC `DefaultModelProcessor#read` #2292

Pankraz76 · 2025-05-02T20:00:00Z

Pull #2292: Modernize codebase with Java improvements - functionalize DefaultModelProcessor#read

Modernize codebase with Java improvements #2277

enable for:

Pull #2291: Modernize codebase with Java improvements - Unnecessary throws #2291

depends on:

test cover DefaultModelProcessor#read #2304

add missing Exception requirement to prevent:

finally 100

Pankraz76 · 2025-05-02T20:08:31Z

whats the mystery about this?

[ERROR] Errors: 
[ERROR]   EmbeddedMavenExecutorTest.defaultFs3xCaptureOutput(Path) » Timeout defaultFs3xCaptureOutput(java.nio.file.Path) timed out after 15 seconds
[ERROR]   ForkedMavenExecutorTest.dump4(Path, Path) » Timeout dump4(java.nio.file.Path, java.nio.file.Path) timed out after 15 seconds
[ERROR]   ToolboxToolTest.metadataPath3(Mode)[1] » Timeout metadataPath3(org.apache.maven.cling.executor.ExecutorHelper$Mode) timed out after 15 seconds

…nalize DefaultModelProcessor#read

gnodet

This totally breaks the exception handling.

Pankraz76 · 2025-05-03T09:45:30Z

This totally breaks the exception handling.

yes, then we need test, thanks.

Pankraz76 · 2025-05-03T09:46:36Z

impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java

+    private Model readOnSelfOrParent(Path pomFile, XmlReaderRequest request) throws IOException {
+        return pomFile == null
+                ? doRead(request)
+                : modelParsers.stream()


extract concern into readParent() to hide impl. detail and give dedicated concern

elharo

Please don't. The original code is clearer. Lambdas do not improve this.

elharo

Test cases might be missing here, but it looks like this changes functionality in a negative way by not grouping exceptions

Pankraz76 · 2025-05-05T11:35:35Z

Yes it’s a test blind spot I want to cover.

…nalize DefaultModelProcessor#read

Pankraz76

add tc; challenge API

Pankraz76 · 2025-05-07T10:35:20Z

impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java

        }
    }

-    private Model doRead(XmlReaderRequest request) throws IOException {


this changes the whole code as this "can not happen"; or if needed then we should change API, if we can, to make code more concise.

ATM this code is based in impl. detail, which should be communicated; therefor enforced.

Pankraz76 · 2025-05-07T10:35:24Z

impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java

-            try {
-                return doRead(request);
-            } catch (IOException e) {
-                exceptions.forEach(e::addSuppressed);


Then this is the last branch not covered.

…nalize DefaultModelProcessor#read

Pankraz76 · 2025-05-07T11:41:22Z

impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java

        if (Files.isDirectory(project)) {
            Path pom = project.resolve("pom.xml");
            return Files.isRegularFile(pom) ? pom : null;
-        } else if (Files.isRegularFile(project)) {


IMHO, it's either a directory (the higher-tier entity), as there is no file without a directory containing it. Which one came first is another topic, but I think it's that simple—either/or. So, we don't need this check, as the null branch is hard to test.

Pankraz76 · 2025-05-07T11:41:57Z

impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java

 * This is because the ModelProcessor interface extends ModelLocator and ModelReader. If we
 * made this component available under all its interfaces then it could end up being injected
 * into itself leading to a stack overflow.
- *


consider migration to https://checkstyle.org/checks/javadoc/javadocparagraph.html.

JavadocParagraph openrewrite/rewrite-static-analysis#545

Fix JavadocParagraph #2302

…nalize DefaultModelProcessor#read

Pankraz76 · 2025-05-07T19:51:01Z

Adding checked exceptions to the API which relies on unchecked exceptions everywhere is problematic

Thanks for the feedback. But just to clarify — this reflects what the current implementation actually does, right?
I'm doing this to enforce the current API and approach, so I’d suggest we state things as they are and move forward from there.

Otherwise, I’d consider removing the IOException check — it might actually be causing issues. That’s why I want to enforce and challenge this behavior.
The tests and interface should make the actual behavior and requirements clear.

I'm not fully aware of all the requirements, but ideally, we can follow TDD and refactor aggressively to reveal the final version of the code and surface the core business logic.

gnodet · 2025-05-07T20:18:51Z

Adding checked exceptions to the API which relies on unchecked exceptions everywhere is problematic

Thanks for the feedback. But just to clarify — this reflects what the current implementation actually does, right? I'm doing this to enforce the current API and approach, so I’d suggest we state things as they are and move forward from there.

Currently, the API is designed to throw XmlReaderException. Check the implemented interface, there's no IOException thrown here.

Otherwise, I’d consider removing the IOException check — it might actually be causing issues. That’s why I want to enforce and challenge this behavior. The tests and interface should make the actual behavior and requirements clear.

I'm not fully aware of all the requirements, but ideally, we can follow TDD and refactor aggressively to reveal the final version of the code and surface the core business logic.

Not sure what you mean exactly, but refactoring is not supposed to change the API in any way.

Pankraz76 · 2025-05-07T20:58:46Z

Not sure what you mean exactly, but refactoring is not supposed to change the API in any way.

yes of course will adapt then. sorry

…nalize DefaultModelProcessor#read

Pankraz76 · 2025-05-08T06:33:57Z

This totally breaks the exception handling.

I'm not sure—when asking AI and trying to mock it, both seem to say the same thing: "Checked exceptions must be declared!"
That's how I remember Java's behavior ever since.

We shouldn't surprise the consumer by forcing them to handle a checked exception that hasn't been declared. A better experience would be to adapt the API to declare the exception properly, or remove the catch block if the exception can't occur by contract—especially since such cases aren't testable.

Currently, the API is designed to throw XmlReaderException. Check the implemented interface, there's no IOException thrown here.

If no exception is thrown, then there's no need for a catch block—especially if it's not testable.

org.mockito.exceptions.base.MockitoException: 
Checked exception is invalid for this method!
Invalid: java.io.IOException

Pankraz76 · 2025-05-08T06:37:52Z

thats my headache Exception 'java.io.IOException' is never thrown in the corresponding try block

Dedicated PR:

test cover DefaultModelProcessor#read #2304

…nalize DefaultModelProcessor#read

Pankraz76 · 2025-05-08T08:01:30Z

Currently, the API is designed to throw XmlReaderException. Check the implemented interface, there's no IOException thrown here.

please notice dedicated thread: https://github.com/apache/maven/pull/2304/files#r2079127763

gnodet · 2025-05-08T12:29:58Z

It's not thrown, that's right. So do not add a throw clause.

gnodet

I' not completely sure you actually understand what you remove.
So let's keep focused PRs and let us know if you ran the tests successfully first.

elharo · 2025-05-08T18:50:53Z

impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java


    @Override
    public Model read(XmlReaderRequest request) throws IOException {
        Objects.requireNonNull(request, "source cannot be null");


source --> request

Pankraz76 · 2025-05-13T18:22:10Z

wip reopen

Pankraz76 force-pushed the functional-DefaultModelProcessor branch 2 times, most recently from 4e0114f to 4ba53a1 Compare May 2, 2025 20:01

Pankraz76 mentioned this pull request May 2, 2025

Pull #2291: Modernize codebase with Java improvements - Unnecessary throws #2291

Closed

Pankraz76 marked this pull request as ready for review May 2, 2025 20:01

Pankraz76 force-pushed the functional-DefaultModelProcessor branch 4 times, most recently from d897766 to 4a35897 Compare May 2, 2025 20:07

Pankraz76 changed the title ~~Pull #2287: Modernize codebase with Java improvements - functional DefaultModelProcessor#read~~ Pull #2287: Modernize codebase with Java improvements - functionalize DefaultModelProcessor#read May 2, 2025

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 2, 2025

Pull apache#2292: Modernize codebase with Java improvements - functio…

594f1bc

…nalize DefaultModelProcessor#read

Pankraz76 force-pushed the functional-DefaultModelProcessor branch from 4a35897 to 594f1bc Compare May 2, 2025 20:13

Pankraz76 changed the title ~~Pull #2287: Modernize codebase with Java improvements - functionalize DefaultModelProcessor#read~~ Pull #2292: Modernize codebase with Java improvements - functionalize DefaultModelProcessor#read May 2, 2025

Pankraz76 mentioned this pull request May 2, 2025

Pull #2292: Modernize codebase with Java improvements - remove dead code #2293

Closed

gnodet requested changes May 3, 2025

View reviewed changes

Pankraz76 commented May 3, 2025

View reviewed changes

elharo requested changes May 5, 2025

View reviewed changes

Pankraz76 force-pushed the functional-DefaultModelProcessor branch from 594f1bc to 0cd0f4e Compare May 7, 2025 10:32

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 7, 2025

Pull apache#2292: Modernize codebase with Java improvements - functio…

0cd0f4e

…nalize DefaultModelProcessor#read

Pankraz76 commented May 7, 2025

View reviewed changes

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 7, 2025

Pull apache#2292: Modernize codebase with Java improvements - functio…

70ee199

…nalize DefaultModelProcessor#read

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 7, 2025

Pull apache#2292: Modernize codebase with Java improvements - functio…

b52e0d0

…nalize DefaultModelProcessor#read

Pankraz76 force-pushed the functional-DefaultModelProcessor branch from 0cd0f4e to b52e0d0 Compare May 7, 2025 11:28

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 7, 2025

Pull apache#2292: Modernize codebase with Java improvements - functio…

e1d0f96

…nalize DefaultModelProcessor#read

Pankraz76 force-pushed the functional-DefaultModelProcessor branch from b52e0d0 to e1d0f96 Compare May 7, 2025 11:38

Pankraz76 commented May 7, 2025

View reviewed changes

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 7, 2025

Pull apache#2292: Modernize codebase with Java improvements - functio…

13ff316

…nalize DefaultModelProcessor#read

Pankraz76 force-pushed the functional-DefaultModelProcessor branch from fb93e7d to 13ff316 Compare May 7, 2025 19:42

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 7, 2025

Pull apache#2292: Modernize codebase with Java improvements - functio…

739cc50

…nalize DefaultModelProcessor#read

Pankraz76 force-pushed the functional-DefaultModelProcessor branch from 13ff316 to 739cc50 Compare May 7, 2025 19:44

Pankraz76 requested a review from elharo May 7, 2025 19:52

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 7, 2025

Pull apache#2292: Modernize codebase with Java improvements - functio…

323035c

…nalize DefaultModelProcessor#read

Pankraz76 force-pushed the functional-DefaultModelProcessor branch from 739cc50 to 323035c Compare May 7, 2025 22:49

Pull apache#2292: Modernize codebase with Java improvements - functio…

2279d78

…nalize DefaultModelProcessor#read

Pankraz76 force-pushed the functional-DefaultModelProcessor branch from 323035c to 2279d78 Compare May 7, 2025 23:03

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 8, 2025

Pull apache#2292: Modernize codebase with Java improvements - functio…

1d2b00a

…nalize DefaultModelProcessor#read

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 8, 2025

Pull apache#2292: Modernize codebase with Java improvements - functio…

962e9a2

…nalize DefaultModelProcessor#read

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 8, 2025

Pull apache#2292: Modernize codebase with Java improvements - functio…

57e584a

…nalize DefaultModelProcessor#read

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 8, 2025

Pull apache#2292: Modernize codebase with Java improvements - functio…

3994d80

…nalize DefaultModelProcessor#read

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 8, 2025

Pull apache#2292: Modernize codebase with Java improvements - functio…

ebe1e37

…nalize DefaultModelProcessor#read

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 8, 2025

Pull apache#2292: Modernize codebase with Java improvements - functio…

0f571b3

…nalize DefaultModelProcessor#read

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 8, 2025

Pull apache#2292: test DefaultModelProcessor#read

25dbfc7

Pankraz76 mentioned this pull request May 8, 2025

test cover DefaultModelProcessor#read #2304

Closed

Pankraz76 changed the title ~~Pull #2292: Modernize codebase with Java improvements - functionalize DefaultModelProcessor#read~~ Pull #2292: Modernize codebase with Java improvements - SOC DefaultModelProcessor#read May 8, 2025

gnodet requested changes May 8, 2025

View reviewed changes

elharo reviewed May 8, 2025

View reviewed changes

Pankraz76 marked this pull request as draft May 9, 2025 11:23

Pankraz76 closed this May 13, 2025

Uh oh!

Pull #2292: Modernize codebase with Java improvements - SOC DefaultModelProcessor#read #2292

Pull #2292: Modernize codebase with Java improvements - SOC DefaultModelProcessor#read #2292

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Pankraz76 commented May 2, 2025

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

Pankraz76 commented May 3, 2025

Uh oh!

Pankraz76 May 3, 2025

Choose a reason for hiding this comment

Uh oh!

elharo left a comment

Choose a reason for hiding this comment

Uh oh!

elharo left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Pankraz76 left a comment

Choose a reason for hiding this comment

Uh oh!

Pankraz76 May 7, 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!

Pankraz76 May 7, 2025

Choose a reason for hiding this comment

Uh oh!

Pankraz76 May 7, 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!

Pankraz76 May 9, 2025

Choose a reason for hiding this comment

Uh oh!

Pankraz76 May 7, 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!

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

Uh oh!

Uh oh!

gnodet commented May 7, 2025

Uh oh!

Pankraz76 commented May 7, 2025

Uh oh!

Pankraz76 commented May 8, 2025

Uh oh!

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

Uh oh!

Uh oh!

Pankraz76 commented May 8, 2025

Uh oh!

gnodet commented May 8, 2025

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

elharo May 8, 2025

Choose a reason for hiding this comment

Uh oh!

Pankraz76 commented May 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Pull #2292: Modernize codebase with Java improvements - SOC `DefaultModelProcessor#read` #2292

Pull #2292: Modernize codebase with Java improvements - SOC `DefaultModelProcessor#read` #2292

Pankraz76 commented May 2, 2025 •

edited

Loading

Pankraz76 commented May 5, 2025 via email •

edited

Loading

Pankraz76 May 7, 2025 •

edited

Loading

Pankraz76 May 7, 2025 •

edited

Loading

Pankraz76 May 7, 2025 •

edited

Loading

Pankraz76 commented May 7, 2025 •

edited

Loading

Pankraz76 commented May 8, 2025 •

edited

Loading