MP-HEALTH, Revisit API terms #43

[sst...@redhat.com]

Here are the analogous components in Config vs Health in order to discuss the #43 issue.

Config                             Response
spi.ConfigBuilder                  ResponseBuilder
spi.ConfigProviderResolver         spi.SPIFactory

There seem to be two main concerns:

1. SPIFactory should be called ResponseBuilderResolver to be consistent with the config conventions.

2. There are 3 methods on ResponseBuilder(#up(), #down(), #state(boolean)) that return a Response instance rather than a single build() method.

Some other structural differences are:

3a. One accesses a ConfigProviderResolver via an ConfigProviderResolver#instance() static method while an SPIFactory is implicitly used from within the Response#named(String) static method to create a ResponseBuilder.

3b. The ConfigBuilder is part of the spi package while ResponseBuilder is in the root of the api package.

Concern 2. could be addressed by either:

public abstract class ResponseBuilder {

    public abstract ResponseBuilder name(String name);
    public abstract ResponseBuilder withAttribute(String key, String value);
    public abstract ResponseBuilder withAttribute(String key, long value);
    public abstract ResponseBuilder withAttribute(String key, boolean value);
    public abstract ResponseBuilder up();
    public abstract ResponseBuilder down();
    public abstract Response build();
}

or more simply:

public abstract class ResponseBuilder {

    public abstract ResponseBuilder name(String name);
    public abstract ResponseBuilder withAttribute(String key, String value);
    public abstract ResponseBuilder withAttribute(String key, long value);
    public abstract ResponseBuilder withAttribute(String key, boolean value);
    public abstract Response build(boolean up);
}

To address concerns 1. and 3. the following would have to change:

1. SPIFactory would be renamed ResponseBuilderProvider, and be made an abstract class with an instance() static method that either used the explicitly provided ResponseBuilder as previously set via a setInstance(ResponseBuilder) static method that exists to accommodate OSGi environments, or it would use the java.util.ServiceLoader pattern to locate a ResponseBuilderProvider.

2. ResponseBuilder would be moved into the spi package.

3. The Response#named(String) method and associated service loader code would be removed as it has been replaced by the ResponseBuilderProvider.

so creation of a response as it is currently:

Response response = Response.named("somename")
        .withAttribute("uptime", 60)
        .up();

would become:

Response response = ResponseBuilderProvider.instance()
        .name("somename")
        .withAttribute("uptime", 60)
        .build(true);

Discussion?

[Heiko Braun]

I am fine with the naming changes, but would love to see that 

 public abstract ResponseBuilder up();
    public abstract ResponseBuilder down();
    public abstract Response build();

stick around. Explicit up() and down() calls read better from my point if view.

In summary my preferred solution would be:

Response response = ResponseBuilderProvider.instance()
        .name("somename")
        .withAttribute("uptime", 60)

        .up()
        .build();

[Heiko Braun]

I think when we touch this area, we should also fix Match API terms to protocol terms

The issue here is about alignment of terms used in the companion doc (in particular the wireformat) and how that's reflected in the Java API. A good example example of the current mismatch is the usage "name" (a Response property) in the Java API in contrast to attribute "id" in the  JSON response format.

[Kenji Kazumura]

 >  public abstract ResponseBuilder up();

 >  public abstract ResponseBuilder down();

 >  public abstract Response build();

I also prefer these methods.

You don't need to bother the order of calling methods.

That is, you can call either way:

  Response response = ResponseBuilderProvider.instance()

        .name("somename")

        .withAttribute("uptime", 60)

        .up()

        .build();

  Response response = ResponseBuilderProvider.instance()

        .up()

        .name("somename")

        .withAttribute("uptime", 60)

        .build();

I think it is similar to javax.ws.rs.core.Response and Response.ResponseBuilder.

[Heiko Braun]

I think it is similar to javax.ws.rs.core.Response and Response.ResponseBuilder.

Yes, it borrows the design approach from JAX-RS. 

[Ladislav Thon]

Completely out of context, ResponseBuilderProvider.instance() is exactly why people hate Java. Even if it all were interfaces, there's no reason why it can't be Response.builder(). More complex scenarios will have more complex code, obviously.

LT

2017-08-18 14:33 GMT+02:00 'Heiko Braun' via Eclipse MicroProfile <microp...@googlegroups.com>:

I think it is similar to javax.ws.rs.core.Response and Response.ResponseBuilder.

Yes, it borrows the design approach from JAX-RS. 

--
You received this message because you are subscribed to the Google Groups "Eclipse MicroProfile" group.
To unsubscribe from this group and stop receiving emails from it, send an email to microprofile+unsubscribe@googlegroups.com.
To post to this group, send email to microp...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/microprofile/3c610e35-0126-42c0-b27c-14e43e1a4b52%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Heiko Braun]

On Friday, 18 August 2017 14:59:23 UTC+2, Ladislav Thon wrote:

Completely out of context, ResponseBuilderProvider.instance() is exactly why people hate Java. Even if it all were interfaces, there's no reason why it can't be Response.builder(). More complex scenarios will have more complex code, obviously.

+1

In the current API we have Response.named(), that takes you straight to the builder. But Response.builder() is equivalent and would be my preference over using  ResponseBuilderProvider.instance(). The fact that the builder is involved doesn't need to be exposed directly, similar to JAX-RS where you access a builder through Response.ok() (an any other static method) as well. For the termination an explicit build() call seems to be fine though.

 

[Emily Jiang]

+1 on Scott's suggestions! ResponseBuilder can have an extra method setStatus() to set up or down instead of having two methods up() and down().

>>Completely out of context, ResponseBuilderProvider.instance() is exactly why people hate Java. Even if it all were interfaces, there's no reason why it can't be

Response.builder()

First of all, I am unaware of this. I would rather to choose this over Response.builder() as it completely destroys the Response object as a pure object case. It should be called builder, which is why the .build should belong to ResponseBuilder.



Emily

[John D. Ament]

Named is clearer in what it means.  The problem with the builder method name is that typically you take in the minimum required arguments in the builder method and then everything optional goes after.  I would prefer to just see a newBuilder method on HealthCheckResponseBuilder instead of having it sourced from HealthCheckResponse, makes it a lot clearer (and then I have no issues with it being called something else).