micronaut-projects / micronaut-core

Micronaut Application Framework
http://micronaut.io
Apache License 2.0
6.09k stars 1.07k forks source link

Add member on @Client annotation to allow inversing produces/consumes semantics defined by an API #3142

Open l0co opened 4 years ago

l0co commented 4 years ago

In 2.0.0.M3.

I have following interface declared, which is then implemented by the controller:

public interface IPdfApi {

    @Post(value = "/generate", consumes = MediaType.TEXT_HTML, produces = "application/pdf")
    byte[] generatePdf(@NotNull @Body String html);

}

@Controller("/pdf")
public class PdfController implements IPdfApi {
  // ...
}

As I understand from the docs this method should get String html as text/html and return some bytes as application/pdf. In other words the request should be done with Content-Type: text/html and Accept: application/pdf headers.

And it works this way when I test the controller from the external client.

Now, consider this declarative client to this interface:

@Client(id="SOME_SERVICE_ID", path = "/pdf")
interface Client extends IPdfApi {}

When this client is created by HttpClientIntroductionAdvice the meaning of produces and consumes is exactly opposite. The request using this client is done with Accept: text/html and Content-Type: application/pdf. And this is revoked by the client with Unsupported mimetype exception. See the related code here.

My question: is this OK or this is a bug?

graemerocher commented 4 years ago

This is correct behaviour yes, you can resolve it by using @Produces / @Consumes on the implementation method in the controller and by overriding the method in the child interface.

A separate feature request my be an option of @Client to reverse the definition, but that is a separate unrelated thing.

l0co commented 4 years ago

To be 100% sure: currently if I have some interfaces, let's say 20 interfaces making up the API with 20 methods each, I cannot use the same interfaces to introduce client advice, but to build a client to this API I need to introduce separate 20 interfaces with exactly the same methods (400 in total) just to reverse @Produces and @Consumes annotations? And this is by design?

Honestly, I'd rather patch HttpClientIntroductionAdvice.

jameskleeh commented 4 years ago

I cannot use the same interfaces to introduce client advice, but to build a client to this API I need to introduce separate 20 interfaces with exactly the same methods (400 in total) just to reverse @Produces and @Consumes annotations? And this is by design?

It would be incorrect to work any other way for @Consumes and @Produces. That being said, there could be different annotations that would apply their behavior differently in the context of a client and server. Perhaps something like @Input, @Output where the meaning of the annotation is the same across both. Open to suggestions

l0co commented 4 years ago

There are two main problems with this approach.

First one is described above: with the current approach one cannot use the same same interfaces for client and server, and this is the most natural thing you want to do with this technology. In microservice-based systems you can extract some common lib with interfaces, and the include them in both server (to be implemented in the controller) and client (to use as a declarative client). The necessity of changing the meaning of produces and consumes for client and server looks like a quirk.

To solve this problem it would be enough to introduce some additional property on @Client advice, as @graemerocher said above.

The second one is than in my opinion "produces" and "consumes" concepts should be consistent across the framework and currently they aren't. Regardless server implementation, do a poll amongs developers what do they think about this code:

@Client
public interface IPdfApi {

    @Post(value = "/generate", consumes = MediaType.TEXT_HTML, produces = "application/pdf")
    byte[] generatePdf(@NotNull @Body String html);

}

I bet, the majority will say that this method consumes (gets) text/html in html argument and produces (returns) application/pdf as a result. This is somehow natural about what produces and consumes means. Because this method indeed consumes text/html and produces application/pdf.

Now look at this:

@Client
public interface IPdfApi {

    @Post(value = "/generate", consumes = "application/pdf", produces = MediaType.TEXT_HTML)
    byte[] generatePdf(@NotNull @Body String html);

}

This looks really odd. This method looks to get application/pdf in html argument and return text/html as a result.

This problem is worse to solve, because changing this behavior would mean breaking backward compatibility. If you think it's reasonable, maybe 1->2 upgrade is a good moment to rethink this concept. I, for example, would go with consistent meaning for "produces" and "consumes" and break the backward compatibilty, with an option to restore old behavior with some property in application.yml, for those who migrate from 1 to 2 with large codebase (a painful process BTW :D).

jameskleeh commented 4 years ago

this is the most natural thing you want to do with this technology. In microservice-based systems you can extract some common lib with interfaces ...

I agree that is a desirable use case. My suggestion above would allow you to achieve that without reversing the meaning of consumes and produces.

The second one is than in my opinion "produces" and "consumes" concepts should be consistent across the framework and currently they aren't

They are applied consistently. They are dependent on the context (request/response).

Because this method indeed consumes text/html and produces application/pdf

It doesn't though. It consumes (from the response), a PDF. It produces (sends in a request), HTML.

I think the issue here is that you aren't thinking of this from the perspective of HTTP. You are only looking at the method and applying the concept of consumes/produces to the method instead of the resulting request/response.

l0co commented 4 years ago

This is why I think it's worhwhile a poll. I don't believe this will will be undestandable this way by anyone. And yes, I don't think about this from the perspective of HTTP, but from the perspective of the code, and in the code this method consumes html and produces pdf.

I generally rarely think about applications I write from the perspective of HTTP, TCP/IP or how the higher level code is translated to machine and executed. Of course from time to time you need to go a few levels down and focus on this, but generally I think about the code, model and business logic of the app. And I believe this is the most efficient way to write a software. And what's the declarative client - isn't a way to move things to a higher level? To stop thinking about the HTTP and starting to thing about java interface to use instead?

Anyway, fixing this without changing the meaning of produces and consumes is good enough for me. Every framework has its own quirks and for Micronaut it will be reversed meaning of produces and consumes on declarative client interface, I can live with that ;)