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

Various improvements #9

Merged
merged 4 commits into from Feb 15, 2018

Conversation

Projects
None yet
3 participants
@sdeleuze
Collaborator

sdeleuze commented Feb 15, 2018

Hey, I have made various improvements, could you please merge them?

The last commit switch back to Spring MVC because JPA is a blocking API and should not be used as it is with WebFlux, that would lead to big scalability issues and should not be shown especially in such "official" example. But using WebFlux in tests is fine and recommended.

@arey arey merged commit 4702c81 into spring-petclinic:master Feb 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arey

This comment has been minimized.

Contributor

arey commented Feb 15, 2018

Thanks a lot Sébastien for this PR and your recommandation to not use WebFlux with JPA. I've migrated to WebFlux in order to use the Kotlin routing DSL.
Maybe we could plan to migrate form JPA to a NoSQL database that supports non blocking API?

Do you have any idea to fix the 2 tests that are ignored into the OwnerControllerTest class?

@sdeleuze

This comment has been minimized.

Collaborator

sdeleuze commented Feb 15, 2018

Thanks for the quick merge. Any chance you could drop a note on http://javaetmoi.com/2017/12/migration-spring-web-mvc-vers-spring-webflux/ that in production, WebFlux server and JPA are not a good fit (client is perfectly ok)?

@arey arey referenced this pull request Feb 15, 2018

Closed

Use Kotlin routing DSL #6

@sdeleuze

This comment has been minimized.

Collaborator

sdeleuze commented Feb 15, 2018

Yeah sadly Kotlin routing DSL and JPA are not a good fit for now. We are experimenting on some Reactive/Async SQL support but nothing usable in production for now. I am not sure about migrating this sample to NoSQL since a JPA example is actually super useful. But why not creating a new fork maybe?

I think I forgot to change @WebFluxTest to @WebMvcTest, could you please change this?

Could you please detail what you are trying to achieve with the client ("How to submit data into a GET with the Webflux client"), I am not sure to understand?

@arey

This comment has been minimized.

Contributor

arey commented Feb 19, 2018

Hi @sdeleuze. I've added a post-scriptum to my blog entry and I've fixed the 2 unit tests.

I tried to change @WebFluxTest to @WebMvcTest but the build failed: https://travis-ci.org/spring-petclinic/spring-petclinic-kotlin/builds/341988016?utm_source=email&utm_medium=notification
The org.springframework.test.web.reactive.server.WebTestClient seems to not be injected. Any idea?

@sdeleuze

This comment has been minimized.

Collaborator

sdeleuze commented Mar 13, 2018

@arey Sorry back from long vacations. Did you manage to find the error?

@arey

This comment has been minimized.

Contributor

arey commented Mar 13, 2018

No, I don't find it.

@sdeleuze

This comment has been minimized.

Collaborator

sdeleuze commented Mar 13, 2018

I tried to use @AutoConfigureWebTestClient but it does not seem to work, I guess it is related to spring-projects/spring-boot#12318.

@snicoll

This comment has been minimized.

Contributor

snicoll commented Mar 14, 2018

No it's not. Mocking with WebTestClient doesn't work with WebMvc (if that's what you're trying to do)

@arey

This comment has been minimized.

Contributor

arey commented Mar 26, 2018

Yes, we would like to use WebTestClient with WebMvc. @snicoll Do you know if it is plan?
@sdeleuze maybe we should rollback WebFlux usage?

@sdeleuze

This comment has been minimized.

Collaborator

sdeleuze commented Apr 3, 2018

Since WebTestClient mocking is not available with WebMvc as said by @snicoll, we could migrate *Controller tests from @WebFluxTest to integration tests with @SpringBootTest(webEnvironment = WebEnvironment.RANDOM_PORT) + @AutoConfigureWebTestClient like in this test.

Other possibility: just using @WebMvcTest + MockMvc ... This is maybe the approach to prefer in order to keep mocked tests.

@arey

This comment has been minimized.

Contributor

arey commented Apr 5, 2018

When it's possible, I prefer unit tests than integration tests. Thus returning to @WebMvcTest + MockMvc has my preference, even it we don't use WebFlux anymore :-(

@snicoll

This comment has been minimized.

Contributor

snicoll commented Apr 5, 2018

I agree with @arey. IMO going to full integration tests route only to be able to use WebTestClient is not warranted.

@sdeleuze

This comment has been minimized.

Collaborator

sdeleuze commented Apr 5, 2018

Works for me.

@arey

This comment has been minimized.

Contributor

arey commented May 12, 2018

Thank you Sébastien and Stéphane for your analyse

@sdeleuze

This comment has been minimized.

Collaborator

sdeleuze commented May 12, 2018

You're welcome, see also https://spring.io/guides/tutorials/spring-boot-kotlin/ for an example of a Kotlin + JPA idiomatic application.

@arey

This comment has been minimized.

Contributor

arey commented May 12, 2018

It's a really complete guide. Thanks.
I've just applied the org.springframework.ui.set extension function.

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