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

Produce valid JSON for Multi<String> return type in RESTEasy Reactive #18344

Closed
wants to merge 2 commits into from
Closed

Conversation

oliver-brm
Copy link

@oliver-brm oliver-brm commented Jul 2, 2021

This is primarily meant to provide a fix for #18043. However, there were no integration-tests/resteasy-reactive yet, so I added them. I have a few questions with you to discuss:

Regarding my fix for the issue in StreamingUtil.java...

  • is it safe for integration, regarding the UTF-8 charset and regarding the MediaType condition?
  • the fix is a bit hack-ish; I think, if possible, it would be better to change those Jackson/JSONB writers, for example FullyFeaturedServerJacksonMessageBodyWriter; however, I wasn't sure how to test that for side-effects; probably running the RESTeasy TCK?

Regarding the integration tests for RESTeasy reactive
I added the project right under integration-tests and copied (YUK!) some classes from the resteasy-mutiny and resteasy-jackson integration tests. Would be better to share the classes, using a parent or shared project. However, I was not sure how much restructuring I would be allowed to do in this scope. You might like to have this done under a separate GitHub issue?

  - also add integration-tests/resteasy-reactive
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 2, 2021

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

This message is automatically generated by a bot.

@oliver-brm oliver-brm changed the title closes #18043 produce valid JSON on Multi<String> Fix - produce valid JSON on Multi<String> #18043 Jul 2, 2021
@oliver-brm oliver-brm changed the title Fix - produce valid JSON on Multi<String> #18043 Fix - produce valid JSON on Multi<String> Jul 2, 2021
@geoand
Copy link
Contributor

geoand commented Jul 2, 2021

Thanks!

Ping me when you are ready for a review

@geoand geoand changed the title Fix - produce valid JSON on Multi<String> Produce valid JSON for Multi<String> return type in RESTEasy Reactive Jul 2, 2021
@oliver-brm
Copy link
Author

oliver-brm commented Jul 2, 2021

Thanks!

Ping me when you are ready for a review

Ping! :-)

@geoand
Copy link
Contributor

geoand commented Jul 4, 2021

Thanks for this!

I would prefer if the PR didn't introduce a whole new project just for testing.
The PR could likely easily just add a new test in one of the json modules (and it doesn't need to be a integration test, it can be one a @QuarkusUnitTest)

@oliver-brm
Copy link
Author

Hmm, I tried to move some of the tests to the quarkus-resteasy-reactive-jackson. However, I cannot get the test to run:

Caused by: io.quarkus.bootstrap.BootstrapException: Failed to create the application model for io.quarkus:quarkus-resteasy-reactive-jackson::jar:999-SNAPSHOT
	at io.quarkus.bootstrap.BootstrapAppModelFactory.resolveAppModel(BootstrapAppModelFactory.java:325)
	at io.quarkus.bootstrap.app.QuarkusBootstrap.bootstrap(QuarkusBootstrap.java:169)
	at io.quarkus.test.junit.QuarkusTestExtension.doJavaStart(QuarkusTestExtension.java:314)
	at io.quarkus.test.junit.QuarkusTestExtension.ensureStarted(QuarkusTestExtension.java:635)
	at io.quarkus.test.junit.QuarkusTestExtension.beforeAll(QuarkusTestExtension.java:682)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$invokeBeforeAllCallbacks$8(ClassBasedTestDescriptor.java:368)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.invokeBeforeAllCallbacks(ClassBasedTestDescriptor.java:368)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.before(ClassBasedTestDescriptor.java:192)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.before(ClassBasedTestDescriptor.java:78)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$5(NodeTestTask.java:136)
	... 34 more
Caused by: io.quarkus.bootstrap.resolver.maven.BootstrapMavenException: Failed to resolve artifacts
	at io.quarkus.bootstrap.resolver.maven.MavenArtifactResolver.resolve(MavenArtifactResolver.java:172)
	at io.quarkus.bootstrap.resolver.BootstrapAppModelResolver.doResolveModel(BootstrapAppModelResolver.java:233)
	at io.quarkus.bootstrap.resolver.BootstrapAppModelResolver.resolveManagedModel(BootstrapAppModelResolver.java:147)
	at io.quarkus.bootstrap.BootstrapAppModelFactory.resolveAppModel(BootstrapAppModelFactory.java:311)
	... 44 more
Caused by: org.eclipse.aether.resolution.ArtifactResolutionException: Could not find artifact io.quarkus:quarkus-resteasy-reactive-server-spi-deployment:jar:999-SNAPSHOT
	at org.eclipse.aether.internal.impl.DefaultArtifactResolver.resolve(DefaultArtifactResolver.java:424)
	at org.eclipse.aether.internal.impl.DefaultArtifactResolver.resolveArtifacts(DefaultArtifactResolver.java:229)
	at org.eclipse.aether.internal.impl.DefaultRepositorySystem.resolveArtifacts(DefaultRepositorySystem.java:270)
	at io.quarkus.bootstrap.resolver.maven.MavenArtifactResolver.resolve(MavenArtifactResolver.java:170)
	... 47 more

@geoand
Copy link
Contributor

geoand commented Jul 5, 2021

I would suggest the following:

mvn -Dquickly to build all changes and then mvn test -f extensions/resteasy-reactive/quarkus-resteasy-reactive-jackson/deployment/ to run the tests in the jackson module.

see https://github.com/quarkusio/quarkus/blob/main/CONTRIBUTING.md#workflow-tips for more details

@oliver-brm
Copy link
Author

Sorry, I messed up my fork. Had to re-create one and open a new PR. Please see #18401.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage/invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants