Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRESOLVER-600] - RFC9457 Implementation #576

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

doddi
Copy link

@doddi doddi commented Sep 1, 2024

Added an RFC9457Reporter for each of the 3 Transporters.

At the moment I tried to keep the original exception messages if the error response is not an RFC9457 type. This did create a little more complexity in the implementation than is perhaps necessary.
If the exceptions to be thrown can be consisted throughout the transporters then this PR could also be simplified considerably. At the moment I have taken the stance not to make any potential breaking changes in messaging.

Fixes: https://issues.apache.org/jira/browse/MRESOLVER-600

Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, it is RFC 9457. With a space and uppercase. I see no processing of the response body at all, JSON is just serialzation. An appropriate exception should extract the fields and populate.

@doddi doddi marked this pull request as ready for review September 5, 2024 08:35
@doddi doddi requested a review from michael-o September 5, 2024 08:35
@hboutemy
Copy link
Member

@doddi is this addition to maven-resolver automatically used by Maven when dependency is upgraded, to fix https://issues.apache.org/jira/browse/MNG-6795 or does Maven require an update to benefit?

@doddi
Copy link
Author

doddi commented Sep 10, 2024

@doddi is this addition to maven-resolver automatically used by Maven when dependency is upgraded, to fix https://issues.apache.org/jira/browse/MNG-6795 or does Maven require an update to benefit?

Maven does not need an update to use it but It will require servers to implement the RFC at their end. This PR will throw the RFC9457 exception when an application/problem+json is received as that is what indicates it is RFC 9457 payload. However, for those that implement RFC 9457 it is a suitable replacement and will overcome the issue with reasonPhrase and Http2

@hboutemy
Copy link
Member

hboutemy commented Sep 10, 2024

ok, IIUC, the exception gets translated into a Maven CLI message without changing Maven core code: nice
I don't know if we have an IT to check that... Looking at https://maven.apache.org/core-its/core-it-suite/surefire-report.html, I don't find anything that looks relevant.
We probably don't have any, or https://issues.apache.org/jira/browse/MNG-6584 would not have happened: this is the type of IT where we need to create a mock repository server, not so easy to write

@doddi
Copy link
Author

doddi commented Sep 10, 2024

ok, IIUC, the exception gets translated into a Maven CLI message without changing Maven core code: nice I don't know if we have an IT to check that...

I added https://github.com/apache/maven-resolver/pull/576/files#diff-2a1236811d71180ff302ffca463a0449c3c6c13d0df3ec362c78765b58025d47R503 but not aware of any cli ITs. I will look to do a build of maven with this PR and ensure there is a suitable response observed

pom.xml Outdated Show resolved Hide resolved
package org.eclipse.aether.spi.connector.transport.http.RFC9457;

@FunctionalInterface
public interface BiConsumerChecked<T, U, E extends Exception> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class looks awkward to me in SPI (not a -1 but...)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I kind of agree but I was trying to ensure that the original exceptions (which are different for each transporter) are not lost and thought the wrapped solution is more convenient (albeit a little ugly) as it is a single call rather than the if/else that would have to replace it.

Copy link
Author

@doddi doddi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add that to get maven-resolver to build with maven will require an update to https://github.com/apache/maven/blob/64e944792613e241b3faddcb4c793e147f6faa5e/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/MavenRepositorySystemSupplier.java#L672 as it will now require passing in an instance of ApacheRFC9457Reporter

@doddi
Copy link
Author

doddi commented Sep 12, 2024

I would add that to get maven-resolver to build with maven will require an update to https://github.com/apache/maven/blob/64e944792613e241b3faddcb4c793e147f6faa5e/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/MavenRepositorySystemSupplier.java#L672 as it will now require passing in an instance of ApacheRFC9457Reporter

Opted to move to static initialised as suggested. This has the benefit of now no longer changing the transporter constructors so the maven project no longer needs to be changed to make use of these changes. dad87aa

Using a static instance also has the benefit of keeping the changes internal. Therefore, no changes to the maven project are required to move to this version.
@doddi doddi force-pushed the MRESOLVER-600-RFC9457-implementation branch from dad87aa to 9f3391e Compare September 12, 2024 06:40
@michael-o michael-o self-requested a review September 13, 2024 06:49
Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK for me, but needs another pair of eyes at least.

@cstamas cstamas added this to the 2.0.2 milestone Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants