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

Create new async client based on Reactor Netty #123

Merged
merged 38 commits into from
May 15, 2018

Conversation

acogoluegnes
Copy link
Contributor

Fixes #122

This commit covers the typical use cases (basic authentication, serialization,
deserialization, GET/PUT/DELETE methods). Some refactoring will be
needed to re-use and clean up code (e.g. URI creation, authentication).
TLS support will also be needed.

[#157001487]

References #122
import io.netty.handler.codec.http.HttpHeaderValues;
import io.netty.handler.codec.json.JsonObjectDecoder;
import org.reactivestreams.Publisher;
import org.springframework.web.util.UriComponentsBuilder;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the story just to make WebFlux optional?
Howe about this spring-web?
Why should we depend on Spring in this library at all?

Thanks

Copy link
Member

Choose a reason for hiding this comment

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

spring-web is only used because it has correct utility classes the JDK and Apache HTTP Commons lack (or at least used to lack).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This client depends on spring-web just for URL encoding, I added a FIXME in the code to experiment with other solutions, something JDK-native being the prefered one.

return doGetFlux(builder -> builder.pathSegment("policies"), PolicyInfo.class);
}

public Mono<HttpClientResponse> deletePolicy(String vhost, String name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does sounds like HttpClientResponse is a high-level API for REST.
Since we talk here only about DELETE wouldn't it be just enough to return a status instead of the whole low-level HttpClientResponse?

Thanks

Copy link
Member

Choose a reason for hiding this comment

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

Either that or a boolean (whether the delete succeeded).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. This applies to empty responses (POST, PUT, and DELETE). Different options:

  • stick to HttpClientResponse: simplest, complete, but leaky
  • create our own HTTP response wrapper: more involving but the most flexible
  • return the bare minimal (boolean or status code): simple, but limiting

I'd go with the wrapper, with only the status code and headers, we could make it more elaborate on demand.

@acogoluegnes
Copy link
Contributor Author

@artembilan Thanks for the follow-up BTW!

This provide an abstraction and a reactive solution to create the token.
Default is to compute the basic value on demand and cache it.

[#157001487]

References #122
private Mono<HttpClientRequest> addAuthorization(Mono<HttpClientRequest> request) {
return Mono
.zip(request, token)
.map(tuple -> tuple.getT1().addHeader(HttpHeaderNames.AUTHORIZATION, tuple.getT2()));
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the token is that class property, therefore this. would help.
And if it is that, it looks like

.doOnNext(r -> r.addHeader(HttpHeaderNames.AUTHORIZATION, this.token))

would enough and much efficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! I see what you mean with zip(). That's because token is a Mono.
Well, I think .block() especially for cached in-memory value isn't evil here at all.
I just mean that I don't see reason in extra zip just for resolving credential on demand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

token updated to this.token.
token is a Mono<String>, so the point here is to get the token only when needed and potentially a value that can change between calls. The solution you suggest would work only if the token doesn't change once the instance has been created. This is actually the case by default (the Authorization header uses username / password digest). Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, but you use .cache() there. I think this will eliminate any your attempts to change credentials at runtime.

Am I missing anything else ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right, the current default implementation is static and may be the one used 99% of the time. I plan to add a way to provide a custom Mono<String> token when creating the client, to address any case (e.g. dynamic token of some kind). This is why the addAuthorization is implemented this way. Hope this is clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good. It is now. Thank you!

import io.netty.handler.codec.http.HttpHeaderValues;
import io.netty.handler.codec.json.JsonObjectDecoder;
import org.reactivestreams.Publisher;
import org.springframework.util.StringUtils;
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, sorry for coming me back again:, but my thought was that you are going to get rid of any Spring dependencies at all. No? Am I to nit-picking?
Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, we still want to get rid of Spring dependencies for this client, I'll use something there. Thanks!

URI path encoding implementation is taken from HTTP components, this
avoids depending on Spring Web. The HTTP response wrapper contains the
status code, reason phrase, and headers, this avoids leaking the Reactor
Netty API.

[#157001487]

References #122
Namely channels, virtual hosts, users, permissions.

[#157001487]

References #122
Bump dependencies for 2.0.x as well.

Fixes #126

Conflicts:
	build.gradle
Copy link
Contributor

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

LGTM

Exchanges, bindings, shovel, queues.

[#157001487]

References #122
Exchanges & bindings.

[#157001487]

References #122
[#157001487]

References #122
@michaelklishin michaelklishin changed the title DO NOT MERGE YET Create Reactor Netty based client Switch async client to Reactor Netty May 6, 2018
@michaelklishin
Copy link
Member

LGTM, I found no functional changes and no API changes we haven't agreed on earlier.

spring-builds and others added 18 commits May 14, 2018 13:07
Conflicts:
	gradle.properties
This commit covers the typical use cases (basic authentication, serialization,
deserialization, GET/PUT/DELETE methods). Some refactoring will be
needed to re-use and clean up code (e.g. URI creation, authentication).
TLS support will also be needed.

[#157001487]

References #122
This provide an abstraction and a reactive solution to create the token.
Default is to compute the basic value on demand and cache it.

[#157001487]

References #122
URI path encoding implementation is taken from HTTP components, this
avoids depending on Spring Web. The HTTP response wrapper contains the
status code, reason phrase, and headers, this avoids leaking the Reactor
Netty API.

[#157001487]

References #122
Namely channels, virtual hosts, users, permissions.

[#157001487]

References #122
Exchanges, bindings, shovel, queues.

[#157001487]

References #122
Exchanges & bindings.

[#157001487]

References #122
[#157001487]

References #122
@acogoluegnes acogoluegnes merged commit d319e85 into master May 15, 2018
@acogoluegnes acogoluegnes changed the title Switch async client to Reactor Netty Create new async client based on Reactor Netty May 15, 2018
@acogoluegnes acogoluegnes deleted the reactor-netty-client branch March 21, 2019 08:49
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