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

Implement support for NDJSON streaming in vertex-web #18182

Merged
merged 3 commits into from
Jun 30, 2021

Conversation

ntrp
Copy link
Contributor

@ntrp ntrp commented Jun 27, 2021

I noticed Vertx reactive routes support SSE but not NDJSON (usually is the one that is used by spring webflux). Spring Webflus was returning application/stream+json before but they are now switching to application/x-ndjson, this is why I am using that content type.

spring-projects/spring-framework#21283

If this integration is wanted I will polish and write test, otherwise I'll not waste more time on it, please let me know.

p.s. I did not touch the other supports but the the SSE one has a minor bug imho, the initialize function always applies the header since the headers are always empty. It is necessary to check the rc.getAcceptableContentType() method to verify if anybody set a custom produces value for the route. In case I can fix that too on the MultiSSESupport.

@cescoffier
Copy link
Member

cescoffier commented Jun 27, 2021

Interesting.

Is there any document explaining NDJSON? => http://ndjson.org/

@geoand FYI.

Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some tests?

@geoand
Copy link
Contributor

geoand commented Jun 27, 2021

@FroMage do you remember this one from your research around JSON streaming?

@FroMage
Copy link
Member

FroMage commented Jun 28, 2021

I don't remember. But the issue was #13663

@ntrp
Copy link
Contributor Author

ntrp commented Jun 28, 2021

I could not run the tests for some reason locally, not in Idea and also not with maven..
In Idea somehow all modules have been imported to java 8 instead of 11 even if I set the Porject jdk to 11. Diamond operators in other classes break because of that.

In Maven instead I get errors like this:

[ERROR] Failures:
[ERROR]   MultipleThrowableParamsFailureHandlerTest Build failed with wrong exception, expected class java.la
ng.IllegalStateException but got io.quarkus.builder.ChainBuildException: No producers for required item class
 io.quarkus.arc.deployment.TransformedAnnotationsBuildItem ==> expected: <true> but was: <false>

when I run mvn test -pl :quarkus-vertx-web-deployment

Any hints? In the meantime I wrote the tests and pushed hoping they are fine..

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 28, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building a50cb93

Status Name Step Test failures Logs Raw logs
Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs

@geoand
Copy link
Contributor

geoand commented Jun 29, 2021

when I run mvn test -pl :quarkus-vertx-web-deployment

Any hints? In the meantime I wrote the tests and pushed hoping they are fine..

Try building the entire project with: mvn -T C1 -Dquickly and the running the tests in the module with mvn install -f extensions/vertx-web/

@ntrp
Copy link
Contributor Author

ntrp commented Jun 29, 2021

Thanks, that did the trick, now I can run the test via mvn. Still cannot run via Intellij due to the fact that the modules are set to Java 8 but better then nothing.

Do you have an idea on why the modules got imported as java 8 when in the pom I've seen it's set maven.compiler.target to 11?

@ntrp ntrp requested a review from cescoffier June 29, 2021 06:39
@geoand
Copy link
Contributor

geoand commented Jun 29, 2021

@ntrp
Copy link
Contributor Author

ntrp commented Jun 29, 2021

See https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/IDEA.20modules.20and.20java.208

That worked, should we add it to the CONTRIBUTION.md? Either in the Q&A section or in the Intellij IDE setup one.

@geoand
Copy link
Contributor

geoand commented Jun 29, 2021

I added it this morning 🙂

@geoand
Copy link
Contributor

geoand commented Jun 30, 2021

@cescoffier is ready for merge?

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

Successfully merging this pull request may close these issues.

4 participants