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

MicroProfile REST Client v1.1 support #3844

Closed
wants to merge 18 commits into from
Closed

MicroProfile REST Client v1.1 support #3844

wants to merge 18 commits into from

Conversation

jGauravGupta
Copy link
Member

@jGauravGupta jGauravGupta commented Jun 1, 2018

@jGauravGupta
Copy link
Member Author

e2e test are also failing for fresh fork.

@jansupol
Copy link
Contributor

Right, the tests are a bummer. We have fixed some tests and there always comes a new test that intermittently fails. Travis seems to have a special capability of exposing test issues. We are trying to solve it, but it is not easy to simulate conditions that make the test fail, usually the tests pass in our local environments.

@jansupol
Copy link
Contributor

This is really a large set of changes. I need to further review, but we should split the commit at least into:

  • refactoring part, where the classes would be moved from server to other (core) module
  • and the functional part of the commit

Definitely, there are missing examples/tests.

bom/pom.xml Outdated Show resolved Hide resolved
@@ -0,0 +1,79 @@
/*
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not use CDDL/GPLwCPE license


package org.glassfish.jersey.server;
package org.glassfish.jersey;
Copy link
Contributor

@jansupol jansupol Jul 10, 2018

Choose a reason for hiding this comment

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

Uri would make sense to put to package org.glassfish.jersey.uri

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @jansupol ,

As MP REST Client requires core-server#org.glassfish.jersey.server.Uri api, so previously I moved this api to core-common#org.glassfish.jersey.Uri, and now to core-common#org.glassfish.jersey.uri.Uri (after feedback).

But this api is publicly available so it will not be compatibly, if application depends on jersey (instead of JAX-RS api) .

So perhaps, either I should :
1- Move this api to core-common#org.glassfish.jersey.server.Uri
or
2. Create another api in core-client#org.glassfish.jersey.client.Uri (without refactoring the core-server#org.glassfish.jersey.server.Uri)

Please suggest.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, did not think of that. I would move it core-common as you did and retain the one server (just inherit from the common) and mark it as deprecated, remove it in 3.0. But, I cannot find where it is actually used in the PR for which it was needed to be moved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, I forgot Uri is an annotation...I am thinking to create a Parameter in common and a new one in a server, inherit from the common, include the server @uri and use the server Parameter on a server, keep Uri at a server, it is not used by the mp client

Copy link
Contributor

Choose a reason for hiding this comment

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

There is really not a nice solution to this. I tried to come up with something, but I am not particularly happy about the solution. I mentioned some alternatives, not sure which would be right.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jGauravGupta Let's split the PR into more parts. I have separated the inserters classes on your behalf into a separate PR #4098.

@jGauravGupta
Copy link
Member Author

Thanks @jansupol for review, I will add the MP Rest Client tck (https://github.com/eclipse/microprofile-rest-client/tree/master/tck).

@jansupol jansupol added the 2.29 label Mar 27, 2019
@jansupol
Copy link
Contributor

The main issue here is that Jersey core packages/classes MUST NOT depend on microprofile packages. Jersey is integrated into several application servers and those servers will not accept an additional microprofile dependency, it does not belong to traditional Java EE/Jakarta EE world by default. It is fine to have the dependency on microprofile as long as it is in a separate extension module only. It is also fine to create an SPI that would provide the required functionality, being implemented from the mp module.

We would like to add the rest client support to 2.29. There is another PR #4086 that deals with MP rest client as well. While this one builds on top of Jersey, using its internals, the other one is rather Jersey inner structures independent. I'd love to put the best from both PRs together to create a fine extension module.

@jGauravGupta
Copy link
Member Author

Hi @jansupol ,

I agree with the concern that Jersey core packages/classes MUST NOT depend on microprofile packages. I will move those implementation to microprofile-rest-client module using SPI.

I am also looking on the PR #4086 but it depends on vendor (helidon) dependencies.

This was referenced Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants