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

Retain default methods on interfaces #287

Open
pdolezal opened this issue Aug 20, 2020 · 13 comments
Open

Retain default methods on interfaces #287

pdolezal opened this issue Aug 20, 2020 · 13 comments
Assignees
Labels
Milestone

Comments

@pdolezal
Copy link

Let's start with the simpler use case that I considered: I have an interface with some convenience methods which have default implementations and some abstract methods which were supposed to do the actual work and which were also leveraged by the methods with default implementations. I annotated the abstract methods with JAX-RS annotations and I expected that the REST client would supply the implementation for these abstract methods only. Unfortunately, it didn't work like that.

Originally, I considered even a more complex use case: I wanted to have a pure interface for some functionality (pure means no annotations and no framework anticipation). The interface may or may not contain methods with default implementation. An implementation of the interface, which would talk to a remote REST service, could then be realized simply by inheriting from the interface; the inherited interface could supply some default method implementations and redeclare remaining abstract methods with JAX-RS annotations. The REST client was then supposed to take care of the remaining abstract methods, thus do the hard work. This way, I could keep the root interface pure, with no unnecessary dependencies (like JAX-RS annotations).

Could the client implement only abstract methods, so that the use cases mentioned above worked?

@andymc12
Copy link
Contributor

Hi @pdolezal - many apologies for the inexcusable delay in responding to this issue.

A few questions:

  • what MP version are you using?
  • what MP Rest Client implementation are you using? (CXF, Jersey, RESTEasy, Open Liberty, Quarkus, Wildfly, Tomitribe, etc.)
  • can you supply a test case that produces results you are seeing?

I've worked on the CXF implementation which is used in Open Liberty, and I don't think that that is the behavior we see. We use default interface methods for computing HTTP headers (see the first code sample under https://download.eclipse.org/microprofile/microprofile-rest-client-1.4.1/microprofile-rest-client-1.4.1.html#_specifying_additional_client_headers ).

I agree that how default interface methods are handled is not explicitly stated in the spec, and that is something we could change for the next release. Thanks for raising this issue.

@andymc12 andymc12 self-assigned this Jan 13, 2021
@andymc12 andymc12 added this to the 2.1 milestone Jan 13, 2021
@pdolezal
Copy link
Author

I'll have to find the code in question and attempt to reconstruct the state which I tried then. I hope I'll succeed. So, I have no idea at this moment which MP version I used. The only thing I'm sure about is that it was with the latest Quarkus release available then (I recall that I tried the latest release in the hope that it might work when it didn't work with a bit older release that I used initially).

@andymc12
Copy link
Contributor

andymc12 commented Jun 4, 2021

@pdolezal any update on this? Can we close?

@pdolezal
Copy link
Author

pdolezal commented Jun 4, 2021

Hello, apologies on my side this time. I could not find the original piece of code and then I probably had little time & forgot to attempt to reconstruct it.

Alright, better late than never, I hope. I tried to do reproduce the idea with Quarkus, which we started to use heavily since that time. I'm not quite sure if the original attempt was with Quarkus or with Jersey, but it does not still work with latest Quarkus (powered by RESTEasy). Sample code here.

@andymc12
Copy link
Contributor

andymc12 commented Jun 9, 2021

Hi @pdolezal - I ran your sample, and I agree it is broken in Quarkus/RESTEasy. I was able to get it to work in Open Liberty (which uses a fork of CXF) however - sample here - instructions on how to run it are in the readme file.

So I would suggest opening an issue with RESTEasy and/or Quarkus. That said, the spec doesn't specifically say this pattern should work - invoking the rest client method from a default interface method - and there are not any TCK tests for this, so it would be reasonable for RESTEasy/Quarkus to respond with a "not supported" answer. It might be something that we could add to the next version of the spec though. Invoking rest client methods from default interface methods certainly seems like a reasonable thing to do.

Thanks for the reproducer - if we do add it to the spec, would you be ok if we used your sample app as the basis for the TCK test?

@pdolezal
Copy link
Author

Thanks for the reproducer - if we do add it to the spec, would you be ok if we used your sample app as the basis for the TCK test?

Definitely no problem, feel free to use it, improve it or extend it as you will. Thank you for the feedback, I'll contact Quarkus and RESTEasy developers.

@pdolezal
Copy link
Author

pdolezal commented Jun 11, 2021

By the way, one thing occurred to me: I wonder what should be the appropriate behavior (according to the specification) if a default method is provided with JAX-RS annotations. I think that in such a case, the REST Client should supply an own implementation that employs the annotations. At least, I could not find any case when the other way would make more sense.

@andymc12
Copy link
Contributor

By the way, one thing occurred to me: I wonder what should be the appropriate behavior (according to the specification) if a default method is provided with JAX-RS annotations. I think that in such a case, the REST Client should supply an own implementation that employs the annotations. At least, I could not find any case when the other way would make more sense.

Good question. I agree with your thinking - it essentially means that the rest client implementation is overriding the default interface method. And the http method annotation is what tells the implementation to override it.

@ahofmeister
Copy link

Hi guys!

I think, I have the same use case, but I am not sure.

I got following code (within Quarkus!):

@RegisterRestClient(configKey = "backend")
@RegisterClientHeaders(Authenticator.class)
public interface DietsEndpoint {

    @Path("diets")
    @GET
    Response get();

    default List<String> getDiets() {
        Response response = get();
        assertThat(response.getStatus()).isEqualTo(200);
        return response.readEntity(new GenericType<>() {
        });
    }

}

and following exception:

: java.lang.RuntimeException: RESTEASY004530: Could not find a method for: public default java.util.List org.acme.diets.DietsEndpoint.getDiets()

Is that the same or a different one? Did you open an Issue at Quarkus?

@ahofmeister
Copy link

ahofmeister commented Aug 18, 2021

However...this works...

@RegisterRestClient(configKey = "backend")
@RegisterClientHeaders(Authenticator.class)
public interface DietsEndpoint {

    Response get();

    @GET
    @Path("diets")
    default List<String> getDiets() {
        Response response = get();
        assertThat(response.getStatus()).isEqualTo(200);
        return response.readEntity(new GenericType<>() {
        });
    }

}

Looks a bit weird. :D

@pdolezal
Copy link
Author

Is that the same or a different one? Did you open an Issue at Quarkus?

Not exactly same, but close enough to qualify. According to my understanding, this is rather an issue of RESTEasy, which Quarkus uses as the underlying JAX-RS & MicroProfile REST Client implementation, so I filed a ticket to RESTEasy besides noting the problem here.

I hope that this corner should be clarified in the next JAX-RS specification release, as hinted by @andymc12 above, and corrected in all compliant implementations then. RESTEasy got notified in advance thanks to the fact that it bothered a humble Quarkus user 😄

@andymc12
Copy link
Contributor

In these cases, I think RESTEasy is behaving correctly. Please clarify if I misunderstand, but in the working case (where the JAX-RS annotations are applied to the default method), the Rest Client implementation invokes the remote service and returns a response - effectively overriding the default implementation in the interface - as discussed in previous comments (1 and 2.

In the failing case, I wonder if the problem is that the default method is invoking a method on a CDI bean without using CDI semantics. Something similar to:

@ApplicationScoped
public class SomeBean {

    @Logged // qualifier intended to invoke a logging interceptor
    public void doSomething() { ... }
}

@ApplicationScoped
public class SomeOtherBean {

    @Inject
    SomeBean injectedBean;

    @PostConstruct
    public void init() {
        injectedBean.doSomething(); // will be logged
        new SomeBean().doSomething(); // will not be logged - not going through CDI
    }
}

To test this idea, you could modify your default method to lookup the rest client interface bean and then invoke the get method to see if it works.

Thanks!

@andymc12 andymc12 modified the milestones: 3.0, Future Aug 27, 2021
@WhiteCat22
Copy link
Contributor

We triaged this issue in today's MP Rest Client meeting. Re-posting Andy's summary for visibility:

"That said, the spec doesn't specifically say this pattern should work - invoking the rest client method from a default interface method - and there are not any TCK tests for this, so it would be reasonable for RESTEasy/Quarkus to respond with a "not supported" answer. It might be something that we could add to the next version of the spec though. Invoking rest client methods from default interface methods certainly seems like a reasonable thing to do."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants