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 2.0 support #4699

Merged
merged 16 commits into from
Feb 11, 2021
Merged

MicroProfile Rest Client 2.0 support #4699

merged 16 commits into from
Feb 11, 2021

Conversation

jGauravGupta
Copy link
Member

Fixes Github issue: #4654
Signed-off-by: Gaurav Gupta gaurav.gupta@payara.fish

MP Rest Client 2.0 - QueryParamStyle support:
JerseyClient, JerseyClientBuilder, and JerseyUriBuilder is extended by the MP Rest Client module to modify the query param based on the QueryParamStyle.

MP Rest Client 2.0 - CDI-managed provider:
Commit cherry-picked from #4695

@jansupol
Copy link
Contributor

For the PR we need to get the following CQs approved:

        <dependency>
            <groupId>io.projectreactor</groupId>
            <artifactId>reactor-core</artifactId>
            <version>3.4.0</version>
        </dependency>
        <dependency>
            <groupId>com.github.tomakehurst</groupId>
            <artifactId>wiremock</artifactId>
            <version>2.24.1</version>
            <scope>test</scope>
        </dependency>
        <dependency>
            <groupId>org.reactivestreams</groupId>
            <artifactId>reactive-streams</artifactId>
            <version>1.0.3</version>
        </dependency>

Do we need all of them?

Copy link
Member

@Verdent Verdent left a comment

Choose a reason for hiding this comment

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

In terms of QueryParamStyle, I might suggest to create some equivalent enum directly in Jersey and implement this query param formatting in Jersey itself but based on Jersey QueryParamStyle equivalent. I think @jansupol would be more then fine with taking this approach.

With that said, I also believe that it would be much more suitable to define some normal property and use that to propagate this new Jersey enum value.

@jGauravGupta
Copy link
Member Author

Hi @jansupol

The org.reactivestreams:reactive-streams dependency is required to provide Server-Sent Events support. Removed the wiremock upgrade and unused dependency.

@Verdent
Copy link
Member

Verdent commented Jan 28, 2021

Hi @jGauravGupta ,
what I actually meant by not using field for storing proxy string at all is to assign that proxy value directly to the properties and not doing it in build method. :-) Feel free to give me some feedback if you don't agree.

@jGauravGupta
Copy link
Member Author

Hi @Verdent,

If proxy address will be assigned directly to the properties then either host & port validation need to be duplicated in RestClientProducer or skipped as you suggested above to split the value.

I might suggest to split host and port in RestClientProducer and set it to the method above. By using this approach you will validate the host and port even from config.

@jansupol
Copy link
Contributor

Filed CQ 23011 for reactive streams 1.0.3

@Verdent
Copy link
Member

Verdent commented Jan 28, 2021

@jGauravGupta The splitting is OK to do, what I meant is that you do not have to save proxy parts to the fields. You can do that property setting directly in that proxy method after validation and not in the build method.

@jansupol
Copy link
Contributor

So the PR brings two main parts as I see it:

  • The QueryParamStyle
  • The SSE

Both parts are handled separately from the rest of Jersey so that Jersey without MP Rest Client may not benefit from the functionality and some code duplication takes place.

For the

  • QueryParamStyle: is it possible to make the new URIBuilder part of Jersey, so that JerseyWebTarget can have a new method setQueryParamStyle(JerseyQueryParamStyle style) for instance?
  • SSE: the SSE is implemented from scratch and does not use the SSE implementation from Jersey. Is there any problem to provide a bridge to the Jersey SSE impl?

@jGauravGupta
Copy link
Member Author

Hi @jansupol and @Verdent ,

I have updated the PR with the requested changes.

@jansupol
Copy link
Contributor

@jGauravGupta can you please rebase atop master so that the PR does contain just the relevant changes? Thank you

jGauravGupta and others added 16 commits January 30, 2021 18:45
Signed-off-by: Gaurav Gupta <gaurav.gupta@payara.fish>
Signed-off-by: Gaurav Gupta <gaurav.gupta@payara.fish>
Signed-off-by: Gaurav Gupta <gaurav.gupta@payara.fish>
Signed-off-by: Gaurav Gupta <gaurav.gupta@payara.fish>
Signed-off-by: Gaurav Gupta <gaurav.gupta@payara.fish>
Signed-off-by: Gaurav Gupta <gaurav.gupta@payara.fish>
… dependencies

Signed-off-by: jansupol <jan.supol@oracle.com>
Signed-off-by: jansupol <jan.supol@oracle.com>
Signed-off-by: Gaurav Gupta <gaurav.gupta@payara.fish>
Signed-off-by: Gaurav Gupta <gaurav.gupta@payara.fish>
Signed-off-by: Gaurav Gupta <gaurav.gupta@payara.fish>
Signed-off-by: Gaurav Gupta <gaurav.gupta@payara.fish>
Signed-off-by: Gaurav Gupta <gaurav.gupta@payara.fish>
Signed-off-by: Gaurav Gupta <gaurav.gupta@payara.fish>
Signed-off-by: Gaurav Gupta <gaurav.gupta@payara.fish>
Signed-off-by: Gaurav Gupta <gaurav.gupta@payara.fish>
@jansupol
Copy link
Contributor

The CQ has been approved.

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.

MicroProfile Rest Client 2.0 support
4 participants