Register any additional necessary providers for server sent event @Produce(s)

[jaikiran]

The commit here introduces a way where any additional necessary providers are registered for any resource that @Produces MediaType.SERVER_SENT_EVENTS. This should allow for fixing the issue reported in #2176 such that the PriceResouce in that example can now use:

@GET
    @Path("/stream")
    @Produces("text/event-stream;element-type=text/plain")
    public Publisher<Double> stream() {
        return prices;
    }

and will no longer require a (dummy) resource endpoint to register the basic text/plain type providers.

Fixes #2176

[gsmet]

This is more correct but I doubt people will get it right.

Another option would be to include the text plain media type as soon as we have a MediaType.SERVER_SENT_EVENTS. Maybe we could have a generic method contributing additional media types based on a media type.

Typically, we would call contributeAdditionalMediaTypes() at these places:

• https://github.com/quarkusio/quarkus/blob/master/extensions/resteasy-common/deployment/src/main/java/io/quarkus/resteasy/common/deployment/ResteasyCommonProcessor.java#L188
• https://github.com/quarkusio/quarkus/blob/master/extensions/resteasy-common/deployment/src/main/java/io/quarkus/resteasy/common/deployment/ResteasyCommonProcessor.java#L198
• https://github.com/quarkusio/quarkus/blob/master/extensions/resteasy-common/deployment/src/main/java/io/quarkus/resteasy/common/deployment/ResteasyCommonProcessor.java#L208

and it would return an augmented list of elements in some cases (for now, only the MediaType.SERVER_SENT_EVENTS case).

Does it make sense to you?

[jaikiran]

This is more correct but I doubt people will get it right.

@gsmet, I understand what you mean.

Another option would be to include the text plain media type as soon as we have a MediaType.SERVER_SENT_EVENTS. Maybe we could have a generic method contributing additional media types based on a media type.

Okay, so instead of expecting users to use an element-type MediaType parameter, we by default add the text/plain provider if and when we see the use of MediaType.SERVER_SENT_EVENTS in the application. That sounds fine to me.

Typically, we would call contributeAdditionalMediaTypes() at these places:

I have looked at the code that you pointed to (the 3 places) and do understand what you mean. However, those 3 places won't be the right place to do this logic, since those 3 places are processing the providers that have been found in the classpath, rather than the user's application resources which use those providers. That latter part happens later and I have updated this PR to use this common logic in that central place where it happens for MessageWriter, MessageReader and ContextResolver.

The commit in this PR now adds the text/plain provider if it finds the usage of server sent events, plus also supports any additional element-type parameter if at all the user application has used such a construct.

Furthermore, I have included a test case to test the server sent events. Do note that this test case is ineffective in reproducing the original reported issue because of the way we run our integration tests, where multiple other JaxRS endpoints are scanned and because of their usage of text/plain, the providers will anyway be registered and this issue won't be triggered. However, this test at least adds some amount of testing around server sent events.

[asoldano]

I think this proposal from Jaikiran is fine; see section 5 at https://docs.jboss.org/resteasy/docs/4.3.0.Final/userguide/html/Reactive.html#d4e2322

[geoand]

If @asoldano agrees, then it I think we should merge. @gsmet WDYT?

[gsmet]

Please wait.

AFAICS there is also @SseElementType which seems to be used in this case - see example 1 here: https://docs.jboss.org/resteasy/docs/4.3.0.Final/userguide/html/Reactive.html#d4e2483

I think I would add TEXT_PLAIN_TYPE only if the element type is not defined one way or another.

What I'm unclear about is the precedence between the ;element-type parameter and the @SseElementType annotation.

@asoldano could you shed some light on this?

[geoand]

@gsmet up to you :)

[asoldano]

@gsmet uh, thanks, I would need to run some tests, but you might have made me notice a "bug". So @SeeElementType is the RESTEasy annotation that controls the ;element-type parameter, however it looks like a @Produce("text/event-stream; element-type=foo/bar") would result in a response with no parameter if the @SseElementType annotation is not there too. Anyway, for the Quarkus case, it's probably safe to check if the element-type parameter is defined in any way (directly or with the dedicated annotation), with the @SseElementType overriding the value from @Produces.

[gsmet]

@asoldano OK, cool, thanks!

@jaikiran could you implement the above? (and defaults to text/plain if we don't have any meaningful information). I think it would greatly improve things in Quarkus.

[jaikiran]

Thank you @asoldano for all these details and the review and thank you @gsmet for the patience with this review. I have now pushed a change to this PR which takes into account what we have discussed here. The comments in the code should summarize how we deal with it now (essentially the same as what we decided here).

Additionally, my manual testing of that Kafka guide, without that workaround, for various permutations/combinations of the @SseElementType, ;element-type and the absence of those have all gone fine and they work as expected, without the need for the workaround in that guide.

[geoand]

There are some formatting issues.

You can do mvn process-resources to fix them and then update the PR

[jaikiran]

Interesting - I was under the impression, mvn clean install or even mvn clean test, locally, would have done the necessary formatting. Looks like I was wrong. I ran mvn process-resources explicitly and have pushed the changes to this PR.

[geoand]

Clean install should do the trick. It has so often however that we commit before doing it 😅

[gsmet]

@jaikiran I added a very minor comment but the main thing is that it needs a rebase (I would have merged as is if not).

Could you take care of the rebase and fix my minor remark along the way?

Other than that, it's perfect, thanks a lot for this.

Thanks!

[jaikiran]

Hi @gsmet, I've updated this PR after rebasing with latest and also incorporated the (valid) change that you mentioned in the latest comment.

[gsmet]

Merged, very nice improvement, thanks!