4.x: Web Client Discovery integration

[ljnelson]

Web Client Discovery

Summary

This PR integrates Discovery with Helidon Web Client by introducing a suitable WebClientService implementation (as required for such integrations).

Pull Request

The heart of the pull request is the WebClientDiscovery interface (an extension of the WebClientService interface) and the DefaultWebClientDiscovery class that implements it. All other .java files are boilerplate or scaffolding related to the required usage of Helidon Builder.

The full specification of this integration can be found in the documentation of the WebClientDiscovery#handle(io.helidon.webclient.spi.WebClientService.Chain, io.helidon.webclient.api.WebClientServiceRequest) method.

Documentation

Documentation included in this pull request consists of:

• Full javadoc (class and method specifications)
• Style- and length-compatible additions to webclient.adoc
• A new section in discovery.adoc

Thanks in particular to @tjquinno, @romain-grecourt and @barchetta for their collective design and documentation help.

Requirements

• Discovery is opt-in, not out-out, so Web Client Discovery must be opt-in as well.

• Discovery itself can be disabled, and WebClientServiceProvider implementations can be disabled. When either is disabled, the implementation must continue by using any supplied URIs "as is".

• Discovery must not prevent requests from completing, even in the face of errors, so Web Client Discovery must not either.

• It must be easy to see (in configuration, etc.) which Web Client requests are subject to discovery, and which are not.

• Given a WebClientServiceRequest, the implementation must be able to retrieve or infer a discovery name and a default URI from its contents.

• Given a UriInfo, the implementation must have a deterministic algorithm for combining path information that is part of the discovered URI returned by discovery, path information that is part of the default URI submitted as part of discovery, and remaining path information that is part of the UriInfo that is not part of either.

Invariants and Constraints

• Invariant: The implementation fundamentally must be a WebClientService created by a WebClientServiceProvider implementation and therefore must adhere to its requirements as described in its javadocs.

• Invariant: Helidon Builder API must be used.

• Constraint: ClientRequestBase supports only URIs whose schemes are http or https. (See also issue 10752.)

• Constraint: ClientUri does not support opaque URIs.

• Constraint: ClientUri paths are never empty, even when explicitly set to be empty.

• Constraint: ClientUri#create() returns a ClientUri with a scheme component of http, a host component of localhost, a port component of 80 and a path of /.

Sample Javadoc

io.helidon.webclient.discovery.WebClientDiscovery

Javadoc HTML pages are not capturable by full-screen screenshot tools. Screenshots are accordingly paged below.

Page 1

Page 2

Page 3

Page 4

Page 5

[tjquinno]

A couple possible small changes.

[tomas-langer]

The way the webclient discovery is configured seems very complicated to me. I think this deserves more attention. I think we should dedicate some time for it on our weekly meeting

[tomas-langer]

Can you explain why you are testing URI type that is part of Java?

[ljnelson]

This was another exploratory test. It led me, among other useful things, to find JDK-6523089. Shall I remove it?

[tomas-langer]

yes, please remove tests related to JDK classes.

[tomas-langer]

This test does not seem to belong here at all - this should be tested as part of io.helidon.webclient.api module.
If this test is missing, please add it. If it exists, please remove this one.

[ljnelson]

This was another exploratory test I wrote while exploring different strategies for making discovery "visible" in URIs that helped me (and, I hoped, other readers) to understand that URI decoding as performed by the UriEncoding class surprisingly turns pluses (+) into spaces ( ), which is a feature of application/x-www-form-urlencoded parsing, not percent encoding/decoding. Shall I (a) move it or (b) remove it?

[tomas-langer]

Move it to webclient API tests please.

[tomas-langer]

Again not sure what you are testing here related to discovery - just the fact that webclient does not support otherscheme?

[ljnelson]

In general, I left in tests I used to explore contracts that were not immediately apparent to me. This is, as you say, to "prove" that any strategy using, for example, a discovery: scheme or anything like that wouldn't work. Should I remove such exploratory tests?

[tomas-langer]

It seems to me that the tests are misleading when you try to understand what the module does.
Maybe this would be good to keep in some other repository, where we could keep such "reference" tests when somebody else needs to understand the behavior of URI and webclient.
So I think these should be removed from here.

[ljnelson]

Shall I move this test in particular to the webclient module? It seems important to test somewhere for an (error?) condition that is not explicitly documented

[tomas-langer]

If there is something that is not tested, of course add it to the webclient module (either directly to it, or to the tests submodule depending on what it does)

[tomas-langer]

The reason we have webclient service is to be able to configure them with the webclient in a manner that makes it clear to what the configuration belongs. This approach you use is not doing that.

Please use the same approach as with other services (both in webserver and webclient)
I would expect the configuration to be similar to:

webclient-config-key:
  base-uri: http://prod.example.com/foo
  services:
    discovery:
      # some structure here specific to discovery, i.e. 
      "TEST": 
          prefix-uri: "http://test.example.com"
          name: "TEST"

This configuration should then be processed by the service, and used when processing a request.

[ljnelson]

So don't use client properties, but ensure this information is part of the configuration of the service itself? Just want to be clear. (The client properties approach allows a user to supply or alter them in a one-off fashion without having to use the programmatic discovery API. That may or may not be important.)

[tomas-langer]

That seems the right way here - we are configuring the discovery service (or maybe I do not understand what you are doing here).

[ljnelson]

OK. I will redo the PR and documentation and specification to include this change, and will follow the suggested YAML above (or something similar to it).

Would a structure like this be OK?

webclient-config-key:
  services:
    discovery:
      names:
        "https://prod.example.com:443": "prod"

Or the inverse:

webclient-config-key:
  services:
    discovery:
      prefix-uris:
        prod: "https://prod.example.com:443/"

[ljnelson]

One last question: my original approach permits one-off discovery usage triggered by client properties. I'm not saying it's right, just that it's possible, which is why I went this route. For example:

var c = webclientbuilder.build(); // with the discovery service on/enabled/present
// For this particular request, set up discovery a particular way:
c.property("helidon-discovery-TEST-prefix-uri", "http://test.example.com/test")
  .get(someUriSomeonePassedInThatMayOrMayNotMatch);

(Typed without proofing but you get the idea.)

To be very clear: we want to disable this use case, correct?

[tomas-langer]

I am not against augmenting the configuration via properties, but it should not be the main means of configuration.
I would start without it, with the option to add it in the future, once we know how people intend to use this feature.

[tomas-langer]

Regarding the first question - I do not understand the configuration use case enough to know the correct answer to this. One thing I would avoid is to have URI as a key anywhere, as that makes it really hard to override using system properties/environment variables.

I.e. client.services.discovery.prod.uri=https://prod.example.com.433 is OK, but client.services.discovery.names.https://prod.example.com:443=prod is not OK