Skip to content

add bean validation implementation dependency #608

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

Merged
merged 1 commit into from
Apr 27, 2020

Conversation

haijian-vaadin
Copy link

@haijian-vaadin haijian-vaadin commented Apr 25, 2020

Fixes #606


This change is Reviewable

@haijian-vaadin haijian-vaadin added the fusion Changes required for fusion label Apr 26, 2020
Copy link

@denis-anisimov denis-anisimov left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @denis-anisimov)


vaadin-spring/pom.xml, line 88 at r1 (raw file):

hibernate-validator

Do you really need this validation impl ?
hibernate-validator is just one impl of Validation API.
Use may decide to use another impl.
This dep fixes the impl.

As I understand the issue is inside our code which unconditionally tries to get the validation impl.
I'm not familiar with the code but if you need this validation conditionally (only if it's in the classpath) you may use similar code that we have in Binder:

https://github.com/vaadin/flow/blob/master/flow-data/src/main/java/com/vaadin/flow/data/binder/BeanValidationBinder.java#L80

https://github.com/vaadin/flow/blob/master/flow-server/src/main/java/com/vaadin/flow/internal/BeanUtil.java#L160

Copy link
Author

@haijian-vaadin haijian-vaadin left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @denis-anisimov)


vaadin-spring/pom.xml, line 88 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…
hibernate-validator

Do you really need this validation impl ?
hibernate-validator is just one impl of Validation API.
Use may decide to use another impl.
This dep fixes the impl.

As I understand the issue is inside our code which unconditionally tries to get the validation impl.
I'm not familiar with the code but if you need this validation conditionally (only if it's in the classpath) you may use similar code that we have in Binder:

https://github.com/vaadin/flow/blob/master/flow-data/src/main/java/com/vaadin/flow/data/binder/BeanValidationBinder.java#L80

https://github.com/vaadin/flow/blob/master/flow-server/src/main/java/com/vaadin/flow/internal/BeanUtil.java#L160

It's actually mandatory for CCDM, we use bean validators to validate the endpoint methods. Basically it's these 2 requirement tickets: vaadin/vaadin-connect#215, vaadin/vaadin-connect#217.

It would be a bit inconvenient for people who try to use endpoints, which is the core feature of CCDM that they cannot start the server until they add a bean validator dependency to the pom file.

Copy link

@denis-anisimov denis-anisimov left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


vaadin-spring/pom.xml, line 88 at r1 (raw file):

Previously, haijian-vaadin (Haijian Wang) wrote…

It's actually mandatory for CCDM, we use bean validators to validate the endpoint methods. Basically it's these 2 requirement tickets: vaadin/vaadin-connect#215, vaadin/vaadin-connect#217.

It would be a bit inconvenient for people who try to use endpoints, which is the core feature of CCDM that they cannot start the server until they add a bean validator dependency to the pom file.

OK then.

Copy link

@denis-anisimov denis-anisimov left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@denis-anisimov denis-anisimov merged commit 43d1e13 into master Apr 27, 2020
@denis-anisimov denis-anisimov deleted the haijian/add-bean-validation-dependency branch April 27, 2020 09:33
@Artur-
Copy link
Member

Artur- commented Apr 27, 2020

I think you should add spring-boot-starter-validation instead so that the version comes from Spring Boot

@snicoll
Copy link
Contributor

snicoll commented Apr 27, 2020

I think you should add spring-boot-starter-validation instead so that the version comes from Spring Boot

It is unusual for a library to bring a starter in compile scope and the starter is not a requirement at all to get dependency management. If the version in the current dependency is removed and this project is configured to use Spring Boot's dependency management, the version of hibernate-validator will be indeed driven by the version of Spring Boot the project compiles against.

@haijian-vaadin
Copy link
Author

Hi @Artur-, I think probably using hibernate is better. Since spring-boot-starter-validation includes some other dependencies that we don't really need:

  1. jakarta.validation-api, we already have the javax.validation.validation-api in Flow
  2. tomcat-embed-el, which we don't really use/need currently.

Also using spring-boot-starter-validation would force the user to use Spring boot, even though (theoretically) we should support non-boot Spring applications.

What do you think?

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

Successfully merging this pull request may close these issues.

vaadin-spring-boot-starter 15.0.5 projects do not start with Spring Boot 2.3
4 participants