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

corrections and clarifications to Jakarta Validation section #231

Merged
merged 4 commits into from
Sep 4, 2023

Conversation

njr-11
Copy link
Contributor

@njr-11 njr-11 commented Aug 30, 2023

This updates sections in the specification on Jakarta Validation based on conversations that we had with the Jakarta Validation team on the validation mailing list about method validation and cascading validation, which resulted from what is proposed under #216 . This pull reflects what is currently possible with Jakarta Validation. In order to fully achieve what was wanted, to have @Valid on the repository parameterized type be applicable to usage of that type on the repository methods, we need an enhancement from the Jakarta Validation spec, which we have asked for in Jakarta EE 11. If we get that enhancement, this pull is written in a way so as to already be fully consistent with it, but would benefit from an additional code example showing the possibility of that approach.

@njr-11 njr-11 added bug Something isn't working design Improvements or additions to design labels Aug 30, 2023
@njr-11 njr-11 added this to the Jakarta Data 1.0 milestone Aug 30, 2023
@gunnarmorling
Copy link

Note that JPA also performs validation of constraints on entities upon persist, so there's a risk of redundant validation here, when mandating the same at the repository level.

@njr-11
Copy link
Contributor Author

njr-11 commented Aug 30, 2023

Note that JPA also performs validation of constraints on entities upon persist, so there's a risk of redundant validation here, when mandating the same at the repository level.

I believe the JPA validation is configurable. I added an update to include a statement that it should be turned off in order to avoid possible duplication.

@keilw keilw added the documentation Improvements or additions to documentation label Sep 1, 2023
Copy link
Member

@mswatosh mswatosh left a comment

Choose a reason for hiding this comment

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

Looks good, just a few suggestions

spec/src/main/asciidoc/jakarta-ee.adoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/jakarta-ee.adoc Outdated Show resolved Hide resolved
@njr-11
Copy link
Contributor Author

njr-11 commented Sep 1, 2023

fixes #237

@otaviojava
Copy link
Contributor

otaviojava commented Sep 4, 2023

Note that JPA also performs validation of constraints on entities upon persist, so there's a risk of redundant validation here, when mandating the same at the repository level.

I believe the JPA validation is configurable. I added an update to include a statement that it should be turned off in order to avoid possible duplication.

Hey Nathan, I got your point about disabling JPA and bringing it to the Jakarta Data level.
But why?
That point needs to be clarified for me because, in the end, the Provider will be a wrapper or decorator of either JPA or NoSQL implementation.

From the user perspective, the same result, a validation before a database interaction.

Sorry for drawing it; I hope to get myself cleaner this way.

In the first picture, we let the JPA or NoSQL provider follow the same flow:

flowchart TD
    Entity--save method-->JakartaDataProvider
   JakartaDataProvider --> JPAEngine
   JPAEngine --> Validation
   Validation--Is valid?--> Decision
   Decision--Yes--> Database
   Decision--No--> Exception
Loading

This one disables on JPA and moves it to the JakartaDataEngine, which will be an adapter or wrapper at the end.:

flowchart TD
   Entity--save method-->JakartaDataProvider
   JakartaDataProvider --> Validation
   Validation--Is valid?--> Decision
   Decision--Yes--> JPAEngine
   Decision--No--> Exception
   JPAEngine --> Database
 JPAEngine --Disable the trigger--> Validation
Loading

Somehow expressing that the engine will handle that, avoiding the duplication, or bringing this to the "adapter" of the Jakarta Data Provider sounds better, mainly because it will not change anything at the JPA.

As Werner said: #237 (comment)

Automatic validation using these constraints is achieved by specifying that Jakarta Persistence delegate
validation to the Bean Validation implementation upon the pre-persist, pre-update, and pre-remove
entity lifecycle events described in Section 3.5.3.

https://jakarta.ee/specifications/persistence/3.1/jakarta-persistence-spec-3.1.pdf

@njr-11
Copy link
Contributor Author

njr-11 commented Sep 4, 2023

Hey Nathan, I got your point about disabling JPA and bringing it to the Jakarta Data level.
But why?
That point needs to be clarified for me because, in the end, the Provider will be a wrapper or decorator of either JPA or NoSQL implementation.

Or it will have pure JDBC implementation, or some other implementation.

From the user perspective, the same result, a validation before a database interaction.

It is not always the same result. Your diagram is not correct. There are 2 aspects to validation to consider. One is the validation of the constraints on the entity, which is what JPA was doing, and defaulting a certain way. The other is method validation, as defined by the Jakarta Validation specification. This includes constraints that are placed on repository methods themselves, to the parameters and/or return value. JPA doesn't see these constraints. Only Jakarta Data sees them. They need to be validated according to the rules of Jakarta Validation, which defines its own way of controlling validation, covering both the method validation and the cascading validation to the entity constraints. To prevent duplication, on the first aspect, we turn off validation that JPA does. This allows us to fully honor the Jakarta Validation requirements that the Jakarta Validation spec has for an architecture like ours.

@otaviojava
Copy link
Contributor

In the query method, I got your point, but in the save method, I did not copy.

I mean, give an entity "Cat" with the validation annotations, and then at the CatRepository.

@Entity
public class Contact {
@Email
 public String email;
@NotBlank
public String name;
}

@Repository
CatRepository extends CrudRepository<UUID, Cat> {
}


CatRepository repository;
repository.save(cat);

What is going to happen?

@njr-11
Copy link
Contributor Author

njr-11 commented Sep 4, 2023

What is going to happen?

It's going to follow the rules of the Jakarta Validation specification which means validating constraints on parameters and the return value (none are defined) and not cascade to validating the constraints on the entity because you didn't specify @Valid. I understand in this case you cannot specify @Valid without declaring the save method yourself, and that's why we are requesting from Jakarta Validation the ability to put @Valid onto the type variable. I realize you don't like what we have in the interim, but we cannot define something in our Jakarta specification that involves using another Jakarta specification contrary to how that specification defines itself to be used. This is the best we can do until Jakarta Validation adds the function that we asked for, which hopefully will be Jakarta EE 11 lining up with our 1.0 version.

@otaviojava otaviojava merged commit c196b31 into jakartaee:main Sep 4, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working design Improvements or additions to design documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants