Unable to use custom JsonFormat.Printer for ProtobufResponseConverterFunction

[resquivel-squareup]

Hello, we’re trying to use in a custom com.google.protobuf.util.JsonFormat.Printer for the ProtobufResponseConverterFunction , which we are using to convert the HttpResult from our endpoint into JSON

We’re doing this by passing in a custom instance of the converter function like so:

serverBuilder.annotatedService(myService, ProtobufResponseConverterFunction(JsonFormat.printer().includingDefaultValueFields()))) 

However, the custom Printer isn't being used. It looks like a new converter is being instantiated along the way, which results in the Printer passed in to be ignored

[minwoox]

I guess it's a bug. 😅 The specified ResponseConverterFunction should be applied before the ProtobufResponseConverterFunctionProvider but it's not: https://github.com/line/armeria/blob/87d2c926675c49d513855fa6b8a8c0844ee30f1f/core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedService.java#L206-L235

[minwoox]

Do you want to fix this by yourself? Of course, we can handle it if you don't have time for that. 😄

[resquivel-squareup]

Will give it a shot 👍

[minwoox]

Thanks, @resquivel-squareup 😄

[minwoox]

To be precise, I think ResponseConverterFunctions should be applied in the following order:

• Annotated ResponseConverterFunction
• Set via ServerBuilder.annotatedService() and equivalent
• Found via SPI
• Default ResponseConverterFunctions

[resquivel-squareup]

Hello, I've created a Draft PR (definitely not for merging yet).

FlowAnnotatedServiceTest test_EventStreaming(), appears to expect the annotated-response-converter-wrapped-by-the-SPI-converter to be prioritised over just the annotated-response-converter. This seems to be a special case, so I wanted to get your opinion on it.

[ikhoon]

Good point. We need more options for the orders to resolve the conflict. It seems that we can't prioritize an annotated ResponseConverterFunction always.

• If the class type of an instance returned by ResponseConverterFunctionProvider.createResponseConverterFunction() is the same or super class, We may prioritize the ResponseConverterFunction passed through @ResponseConverter or AnnotatedServiceBindingBuilder.responseConverters().
• If ResponseConverterFunctionProvider.createResponseConverterFunction() returns a new type using the given ResponseConverterFunction, we can assume that the ResponseConverterFunctionProvider either wraps or ignores the ResponseConverterFunction.

[minwoox]

Had a chat with the team and we realized:

• ResponseConverterFunctionProvider(found via SPI) uses the responseConverterFunctions set via annotation, service builder and default ones.
• So the relationship between ResponseConverterFunctionProvider and others is not about which one is used beforehand.
  • It's just that ResponseConverterFunctionProvider uses the configured responseConverterFunctions.

So here's what we need to do:

• Change the ResponseConverterFunctionProviders that Armeria provides to use the configured responseConverterFunctions or to be applied after the responseConverterFunctions
  • so that users are always able to use the ResponseConverterFunction they want via setter
  • ProtobufResponseConverterFunctionProvider and ScalaPbResponseConverterFunctionProvider should be fixed to abide by the rule.
• Add ResponseConverterFunction.orElse(responseConverterFunction) for easy chaining.
  • So we can just do return responseConverter.orElse(new ProtobufResponseConverterFunction()); here
• Update the Javadoc of ResponseConverterFunctionProvider.createResponseConverterFunction(...) to inform the ResponseConverterFunctions parameter.

[jrhee17]

Just for clarification:

Change the ResponseConverterFunctionProvider to use the configured responseConverterFunctions or to be applied after the responseConverterFunctions

By this, I think we mean that we will define the behavior of armeria's default ResponseConverterFunctionProvider implementation such that it returns a function:

1. Try to apply the passed responseConverterFunction
2. If the passed responseConverterFunction falls through, the ResponseConverterFunctionProvider will try to apply its own ResponseConverterFunction

so I guess the PR would look roughly like follows (the same would be applied to ScalaPbResponseConverterFunctionProvider):

public final class ProtobufResponseConverterFunctionProvider implements ResponseConverterFunctionProvider {
...
    public ResponseConverterFunction createResponseConverterFunction(
            Type returnType,
            ResponseConverterFunction responseConverter) {
        if (isSupportedType(returnType)) {
            return new responseConverter.orElse(ProtobufResponseConverterFunction);
        }
...

I wanted to make sure I also understood @minwoox 's comments correctly 😅

[resquivel-squareup]

Thanks for the responses, I’ve linked this new Draft PR so we can discuss the new implementation. Let me know if this is or is not what you intended.

As far as I can tell, this implementation doesn’t do this:

If the passed responseConverterFunction falls through, the ResponseConverterFunctionProvider will try to apply its own ResponseConverterFunction

The provider code is called when selecting a response converter to use, which is far before the responses are actually converted. There is just a check in the provider to see if the class-to-be-converted is appropriate. Therefore the fallthrough mechanism never gets a chance to be used with this implementation.

Also, without actually performing the conversion, we cannot validate at this stage whether the passed-in converter is indeed able to perform the conversion.

—--------

We may be running into this because of the differing behaviour between the ResponseConverterFunctions that have providers. Both FlowResponseConverterFunction and ObservableResponseConverterFunction take in responseConverters. When the time comes to convert, they prioritise using the passed-in response converters.

On the other hand ProtobufResponseConverterFunction does not take in a response converter at all. Therefore when the time comes to convert, it can't prioritise the passed-in converter.

This bit is relying on the fact that the response converter function will prioritise the passed-in converter.

[resquivel-squareup]

Thanks for your response here!

---

I also wanted to clarify something. As far as I can tell, a side effect of this implementation is that for ProtobufResponseConverterFunctionProvider (and maybe ScalaPbResponseConverterFunctionProvider?), the order of using the converters during response conversion would be as follows:

• Annotated ResponseConverterFunction, if it fails, then fall through
• Set via ServerBuilder.annotatedService() and equivalent, if it fails, then fall through
• Default ResponseConverterFunctions, if it fails, then fall through
• The ProtobufResponseConverterFunction converter we found via SPI

Whereas for some other converters found via SPI (e.g. FlowResponseConverterFunction), the behaviour would be: Using the converter found via SPI, decorate with some additional behaviour, and then use the converters as follows:

• Annotated ResponseConverterFunction, if it fails, then fall through
• Set via ServerBuilder.annotatedService() and equivalent, if it fails, then fall through
• Default ResponseConverterFunctions

Is this the behaviour that we're trying to achieve?

I realise this concern is mostly semantic and has no effect on the actual behaviour at the moment. I felt it was worthwhile to clarify this difference for documentation as well as posterity.

If we're happy with this, I'm happy to continue with the implementation.

[minwoox]

Is this the behaviour that we're trying to achieve?

That's exactly what we need to achieve through this PR! 👍 👍 👍

I realise this concern is mostly semantic and has no effect on the actual behaviour at the moment. I felt it was worthwhile to clarify this difference for documentation as well as posterity.

I can't agree more than this. Yeah, we definitely need documents for this. We need to update the Javadocs and also add descriptions to the official docs. 😄 https://armeria.dev/docs/server-annotated-service#converting-a-java-object-to-an-http-response Thanks a lot! 😄

[resquivel-squareup]

@minwoox Just a heads up, the existing ProtobufResponseAnnotatedServiceTest appears to be failing (I didn't notice it originally since other tests were failing too) due to the new behaviour:

Annotated ResponseConverterFunction, if it fails, then fall through Set via ServerBuilder.annotatedService() and equivalent, if it fails, then fall through Default ResponseConverterFunctions, if it fails, then fall through The ProtobufResponseConverterFunction converter we found via SPI

Because we moved from always using a fresh new ProtobufResponseConverterFunction to using the Default converters first, we're exercising bunch of new code paths that weren't exercised before 😅

Would love to get your opinion on how you folks would like to handle this. 🙏

[minwoox]

Oops, I didn't notice that the test's failing. Let me run the CI to see what's going on. 😄

[minwoox]

Yeah, it's always that the devil is in the details. 😅

JacksonResponseConverterFunction is applied before the protobuf converter so it converts everything. 😓 Had a discussion about this with the team, and we have come to the conclusion that we need two different SPIs.

• SPI that provides a sole instance of a response converter
  • This converter applied after annotation and builder converter
  • 1. annotation converter 2. builder converter 3. SPI converter 4. default converter
• SPI that provides a converter that uses the above 4 converters as the current SPI does:

I didn't expect that this is that hard. 😅 I'm really sorry for that. 🙏 If you want, you can continue to work on the PR. We will help you get it done. Or we can take it from now because it's going to be a big change and it isn't probably a good issue who is new to this framework. 🙇‍♂️

[resquivel-squareup]

Thanks for the information, and no worries! I might have some time next week to have a look. Could you please provide some more info regarding

we need two different SPIs ?

In which scenarios would each SPI be used? What would the converter selection logic be? Which classes would need to be changed?

[ikhoon]

Let's say we have the following annotated service:

class MyService {
  @ProduceJson
  fun flowProtobufJson(...): Flow<MyProtobuf> {
      ...
  }

}

Server.buider()
      .annotatedService()
      .responseConverters(MyProtobufResponseConverter())
      .build(MyService())

We need two 2 response converters to completely convert Flow<MyProtobuf> into a JSON response. First, MyProtobufResponseConverter will be used to convert each MyProtobuf into a JSON object. Next, FlowResponseConverterFunction should be chosen to collect the JSON objects and convert them into a JSON array.

Currently, we provide two APIs for a custom response converter.

• ResponseConverterFunction defines how to convert an object.
• ResponseConverterFunctionProvider defines how to create a ResponseConverterFunction via SPI.

The downside of the APIs is that they don't tell a ResponseConverterFunction is a primitive converter or a delegating ResponseConverterFunction. It is difficult to prioritize between them. So we'd like to change our API to show their explicit behavior. Note: As the API is @UnstableApi, we can still make a breaking change. 😅

Proposal:

• Split the original ResponseConverterFunctionProvider to two providers. DelegatingResponseConverterFunctionProvider requires a delegate to complete to convert a response like FlowResponseConverterFunction and the new ResponseConverterFunctionProvider does not take any parameters.
• Add ResponseConverterFunctionProvider that create a primitive ResponseConverterFunction which converters an object to an HttpResponse without depending on other ResponseConverterFunctions.
• A response converter created from DelegatingResponseConverterFunctionProvider has higher priority than primitive converters. Because it needs primitive converters to create a delegating response converter.

public interface ResponseConverterFunctionProvider {
    @Nullable
    ResponseConverterFunction newResponseConverterFunction();
}

public interface DelegatingResponseConverterFunctionProvider {

    @Nullable
    ResponseConverterFunction createResponseConverterFunction(
         Type responseType,
         ResponseConverterFunction responseConverter);
}

[resquivel-squareup]

Thanks for the response. I have a work-in-progress draft PR which implements most of the above.

It doesn't address the case for Flow<MyProtobuf>, but it does fix the initial bug and the tests (mostly seem to) pass.

Please let me know what you think so far and if you'd like me to change anything major. Thanks!

[ikhoon]

It doesn't address the case for Flow<MyProtobuf>

Could you elaborate on this? I didn't catch what could be addsssed. 😅

[resquivel-squareup]

Ah, I just meant I didn't write a test case for the exact scenario you mentioned for Flow<MyProtobuf> so I can't confirm that it's actually working