Take RestMulti headers and status into account when using SSE resource method #44903

dastrobu · 2024-12-03T15:25:29Z

Fixes: Setting headers for SSE annotated Multi resources is ignoring headers #44902

geoand · 2024-12-03T15:37:32Z

Thanks!

Are you also interested in providing the actual fix?

We essentially need to update SseMultiSubscriber to use List<StreamingResponseCustomizer> in the same way as in StreamingMultiSubscriber

dastrobu · 2024-12-03T15:59:37Z

I am not sure I find the time right now. There is quite a bit of code to understand. The way you proposed does not seem to work out of the box, as there is some header handling going on in handleSse already:

quarkus/independent-projects/resteasy-reactive/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/handlers/PublisherResponseHandler.java

Line 346 in 7230239

    
           SseUtil.setHeaders(requestContext, requestContext.serverResponse(), streamingResponseCustomizers);

geoand · 2024-12-03T16:02:15Z

Some adjustments will probably be needed, but my guess is that it shouldn't require figuring out much of what the test of the code does

geoand · 2024-12-04T08:00:31Z

I have updated this PR to include a fix for the issue

...time/src/main/java/org/jboss/resteasy/reactive/server/handlers/PublisherResponseHandler.java

.../java/io/quarkus/resteasy/reactive/server/test/customproviders/MultiResponseHeadersTest.java

dastrobu · 2024-12-04T09:59:14Z

@geoand thanks, I did no look through the code in detail, but I think its not handling all cases correctly. I extended ResponseHeaderTest to cover the same cases as for non-SSE Multis. I can probably delete MultiResponseHeadersTest as it seems to be redundant.

geoand · 2024-12-04T10:00:19Z

If you have cases which don't work with the current implementation, please add tests for them

geoand · 2024-12-04T10:28:46Z

@dastrobu let me know when you are done so I can check the latest tests.

Thanks

dastrobu · 2024-12-04T10:39:39Z

I had to rebase and force push, as quarkus-bot was complaining about the merge:

quarkus-bot
/ Check Pull Request - Merge commits

Error - Pull request commit check
[sha=2f089a593200bd8c229aa53ca9e8671238d9744a ; message="Merge remote-tracking branch 'dastrobu/test/sse-response-header-test' into test/sse-response-header-test"]

I think I am done, but let's wait for CI to see the test cases.
Locally, there are two cases that are not passing:

testReturnRestSse3()
testReturnRestSse5()

We probably need to squash commits before merging in the end, but I don't want to do that right now as long as contributing from both ends.

geoand · 2024-12-04T10:46:50Z

Don't worry about the bot and squashing the commits, I'll take care of that.

I think I am done

Okay, thanks

geoand · 2024-12-04T12:24:20Z

This is now ready to go. Thanks a lot @dastrobu!

dastrobu · 2024-12-04T12:46:53Z

@geoand thanks for implementing the fix. It was a pleasure to collaborate on a PR that quickly. It's a rare experience in the open-source universe.

geoand · 2024-12-04T12:49:34Z

🙏🥰

…e method

quarkus-bot · 2024-12-04T16:26:54Z

Status for workflow `Quarkus CI`

This is the status report for running Quarkus CI on commit 949f45c.

Failing Jobs

Status	Name	Step	Failures	Logs	Raw logs	Build scan
✖	JVM Tests - JDK 17	`Build`	Failures	Logs	Raw logs	🔍
✖	JVM Tests - JDK 21	`Build`	Failures	Logs	Raw logs	🔍
✖	JVM Tests - JDK 17 Windows	`Build`	Failures	Logs	Raw logs	🔍

Full information is available in the Build summary check run.
You can consult the Develocity build scans.

Failures

⚙️ JVM Tests - JDK 17 #

- Failing: extensions/resteasy-reactive/rest-jaxb/deployment integration-tests/grpc-descriptor-sets/grpc-descriptor-set-alternate-output-dir 
! Skipped: integration-tests/hibernate-validator-resteasy-reactive

📦 extensions/resteasy-reactive/rest-jaxb/deployment

✖ io.quarkus.resteasy.reactive.jaxb.deployment.test.SseResourceTest.testMultiFromMulti line 104 - History - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

Expecting actual:
  []
to contain exactly (and in same order):
  ["hello"]
but could not find the following elements:
  ["hello"]

✖ io.quarkus.resteasy.reactive.jaxb.deployment.test.SseResourceTest.testSseFromMulti line 68 - History - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

Expecting actual:
  []
to contain exactly (and in same order):
  ["hello"]
but could not find the following elements:
  ["hello"]

📦 integration-tests/grpc-descriptor-sets/grpc-descriptor-set-alternate-output-dir

✖ io.quarkus.grpc.examples.hello.DescriptorSetExistsTest.descriptorSetExists line 16 - History - More details - Source on GitHub

java.lang.AssertionError: 

Expecting path:
  /home/runner/work/quarkus/quarkus/integration-tests/grpc-descriptor-sets/grpc-descriptor-set-alternate-output-dir/target/proto
to exist (symbolic links were followed).
	at io.quarkus.grpc.examples.hello.DescriptorSetExistsTest.descriptorSetExists(DescriptorSetExistsTest.java:16)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

⚙️ JVM Tests - JDK 21 #

- Failing: extensions/resteasy-reactive/rest-jaxb/deployment integration-tests/grpc-descriptor-sets/grpc-descriptor-set-alternate-output-dir 
! Skipped: integration-tests/hibernate-validator-resteasy-reactive

📦 extensions/resteasy-reactive/rest-jaxb/deployment

✖ io.quarkus.resteasy.reactive.jaxb.deployment.test.SseResourceTest.testMultiFromMulti line 104 - History - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

Expecting actual:
  []
to contain exactly (and in same order):
  ["hello"]
but could not find the following elements:
  ["hello"]

✖ io.quarkus.resteasy.reactive.jaxb.deployment.test.SseResourceTest.testSseFromMulti line 68 - History - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

Expecting actual:
  []
to contain exactly (and in same order):
  ["hello"]
but could not find the following elements:
  ["hello"]

📦 integration-tests/grpc-descriptor-sets/grpc-descriptor-set-alternate-output-dir

✖ io.quarkus.grpc.examples.hello.DescriptorSetExistsTest.descriptorSetExists line 16 - History - More details - Source on GitHub

java.lang.AssertionError: 

Expecting path:
  /home/runner/work/quarkus/quarkus/integration-tests/grpc-descriptor-sets/grpc-descriptor-set-alternate-output-dir/target/proto
to exist (symbolic links were followed).
	at io.quarkus.grpc.examples.hello.DescriptorSetExistsTest.descriptorSetExists(DescriptorSetExistsTest.java:16)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)

⚙️ JVM Tests - JDK 17 Windows #

- Failing: extensions/resteasy-reactive/rest-jaxb/deployment integration-tests/grpc-descriptor-sets/grpc-descriptor-set-alternate-output-dir 
! Skipped: integration-tests/hibernate-validator-resteasy-reactive

📦 extensions/resteasy-reactive/rest-jaxb/deployment

✖ io.quarkus.resteasy.reactive.jaxb.deployment.test.SseResourceTest.testMultiFromMulti line 104 - History - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

Expecting actual:
  []
to contain exactly (and in same order):
  ["hello"]
but could not find the following elements:
  ["hello"]

✖ io.quarkus.resteasy.reactive.jaxb.deployment.test.SseResourceTest.testSseFromMulti line 68 - History - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

Expecting actual:
  []
to contain exactly (and in same order):
  ["hello"]
but could not find the following elements:
  ["hello"]

📦 integration-tests/grpc-descriptor-sets/grpc-descriptor-set-alternate-output-dir

✖ io.quarkus.grpc.examples.hello.DescriptorSetExistsTest.descriptorSetExists line 16 - History - More details - Source on GitHub

java.lang.AssertionError: 

Expecting path:
  D:\a\quarkus\quarkus\integration-tests\grpc-descriptor-sets\grpc-descriptor-set-alternate-output-dir\target\proto
to exist (symbolic links were followed).
	at io.quarkus.grpc.examples.hello.DescriptorSetExistsTest.descriptorSetExists(DescriptorSetExistsTest.java:16)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

geoand · 2024-12-04T16:39:42Z

The test failure is related, I'll have a look tomorrow

This comment was marked as resolved.

Sign in to view

quarkus-bot bot added the area/rest label Dec 3, 2024

geoand force-pushed the test/sse-response-header-test branch from 6cfe757 to 5653246 Compare December 4, 2024 07:59

geoand changed the title ~~add unit test for adding a custom response header to a resource returning a sse multi.~~ Take RestMulti headers and status into account when using SSE resource method Dec 4, 2024

geoand marked this pull request as ready for review December 4, 2024 08:00

geoand requested a review from cescoffier December 4, 2024 08:00

geoand mentioned this pull request Dec 4, 2024

Setting headers for SSE annotated Multi resources is ignoring headers #44902

Open

dastrobu commented Dec 4, 2024

View reviewed changes

...time/src/main/java/org/jboss/resteasy/reactive/server/handlers/PublisherResponseHandler.java Outdated Show resolved Hide resolved

dastrobu commented Dec 4, 2024

View reviewed changes

.../java/io/quarkus/resteasy/reactive/server/test/customproviders/MultiResponseHeadersTest.java Outdated Show resolved Hide resolved

dastrobu force-pushed the test/sse-response-header-test branch from 4a09268 to 1a8aeda Compare December 4, 2024 10:36

Polish PublisherResponseHandler

8190ffb

This comment has been minimized.

Sign in to view

geoand force-pushed the test/sse-response-header-test branch from 1a8aeda to 6a8af7c Compare December 4, 2024 12:24

geoand and others added 2 commits December 4, 2024 15:26

Take RestMulti headers and status into account when using SSE resourc…

87e33ed

…e method

Add test for adding a custom response header for SSE resource method

949f45c

geoand force-pushed the test/sse-response-header-test branch from 6a8af7c to 949f45c Compare December 4, 2024 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Take RestMulti headers and status into account when using SSE resource method #44903

Take RestMulti headers and status into account when using SSE resource method #44903

dastrobu commented Dec 3, 2024 •

edited by geoand

Loading

This comment was marked as resolved.

geoand commented Dec 3, 2024

dastrobu commented Dec 3, 2024 •

edited

Loading

geoand commented Dec 3, 2024

geoand commented Dec 4, 2024

dastrobu commented Dec 4, 2024

geoand commented Dec 4, 2024

geoand commented Dec 4, 2024

dastrobu commented Dec 4, 2024

geoand commented Dec 4, 2024

This comment has been minimized.

geoand commented Dec 4, 2024

dastrobu commented Dec 4, 2024

geoand commented Dec 4, 2024

quarkus-bot bot commented Dec 4, 2024 •

edited by github-actions bot

Loading

geoand commented Dec 4, 2024

Take RestMulti headers and status into account when using SSE resource method #44903

Are you sure you want to change the base?

Take RestMulti headers and status into account when using SSE resource method #44903

Conversation

dastrobu commented Dec 3, 2024 • edited by geoand Loading

This comment was marked as resolved.

geoand commented Dec 3, 2024

dastrobu commented Dec 3, 2024 • edited Loading

geoand commented Dec 3, 2024

geoand commented Dec 4, 2024

dastrobu commented Dec 4, 2024

geoand commented Dec 4, 2024

geoand commented Dec 4, 2024

dastrobu commented Dec 4, 2024

geoand commented Dec 4, 2024

This comment has been minimized.

geoand commented Dec 4, 2024

dastrobu commented Dec 4, 2024

geoand commented Dec 4, 2024

quarkus-bot bot commented Dec 4, 2024 • edited by github-actions bot Loading

Status for workflow Quarkus CI

Failing Jobs

Failures

⚙️ JVM Tests - JDK 17 #

📦 extensions/resteasy-reactive/rest-jaxb/deployment

📦 integration-tests/grpc-descriptor-sets/grpc-descriptor-set-alternate-output-dir

⚙️ JVM Tests - JDK 21 #

📦 extensions/resteasy-reactive/rest-jaxb/deployment

📦 integration-tests/grpc-descriptor-sets/grpc-descriptor-set-alternate-output-dir

⚙️ JVM Tests - JDK 17 Windows #

📦 extensions/resteasy-reactive/rest-jaxb/deployment

📦 integration-tests/grpc-descriptor-sets/grpc-descriptor-set-alternate-output-dir

geoand commented Dec 4, 2024

dastrobu commented Dec 3, 2024 •

edited by geoand

Loading

dastrobu commented Dec 3, 2024 •

edited

Loading

quarkus-bot bot commented Dec 4, 2024 •

edited by github-actions bot

Loading

Status for workflow `Quarkus CI`