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

Consider not requiring for repositories to trigger bean validation by default #216

Closed
dstepanov opened this issue Aug 22, 2023 · 9 comments
Labels
design Improvements or additions to design

Comments

@dstepanov
Copy link

dstepanov commented Aug 22, 2023

When you invoke the save or saveAll method on repositories, the Jakarta Data provider ensures that the validation rules defined in the entity are automatically executed prior to updating the database if a Jakarta Validation provider is present. For instance, when persisting a Student object, the name field will be validated to ensure it is not blank.

This kind of validation might be unwanted for performance reasons, or validation might have happened before and is unnecessary.
The latest version of Micronaut Data requires the repository type arguments to be annotated for the entity or ID to be validated:

@Repository
public interface School extends PageableRepository<@Valid Student, String> {
}
@otaviojava
Copy link
Contributor

@dstepanov Hey, how are you?
Yeap, the goal is to make it optional as any integration with Jakarta EE specs.

https://github.com/jakartaee/data/blob/main/spec/src/main/asciidoc/jakarta-ee.adoc

@dstepanov
Copy link
Author

Sorry, my knowledge of the Jakarta EE is limited. I just saw that note and assumed the repository should always check for validation. We actually removed the implicit validation in Micronaut 4.

@otaviojava
Copy link
Contributor

There is no sorry; we are here to learn together!

What you said makes sense; it should not make this validation.

I will create a PR explicating the integration which is not mandatory. It is possible and period.

@njr-11
Copy link
Contributor

njr-11 commented Aug 22, 2023

This kind of validation might be unwanted for performance reasons, or validation might have happened before and is unnecessary. The latest version of Micronaut Data requires the repository type arguments to be annotated for the entity or ID to be validated:

@Repository
public interface School extends PageableRepository<@Valid Student, String> {
}

This is an excellent point from @dstepanov and I think we should look into doing the same in Jakarta Data. @Valid is a spec-defined annotation under Jakarta [Bean] Validation and it certainly seems useful and intuitive if it could be used to opt in to validation.

Regarding @Valid, I could also see the possibility of someone using it on a more granular basis, like this, if using DataRepository where they define their own save/saveAll,

@Repository
public interface School extends DataRepository<Student, String> {
    void save(@Valid Student);
    ...
}

so we should look into whether @Valid can be used in this way.

@KyleAure KyleAure added the design Improvements or additions to design label Aug 22, 2023
@njr-11
Copy link
Contributor

njr-11 commented Aug 24, 2023

I sent a question to the Jakarta Bean Validation mailing list to check on our proposed usage of the Valid annotation, referencing this issue.

njr-11 added a commit to njr-11/data that referenced this issue Aug 30, 2023
@njr-11
Copy link
Contributor

njr-11 commented Aug 30, 2023

Pull @231 corrects the validation sections in the spec based on current behavior of Jakarta Validation, which allows the Valid annotation in some other places but not yet on the parameterized types of the interface from which the repository inherits.
jakartaee/validation#186 is opened against Jakarta Validation to request adding support for the above.

@dstepanov
Copy link
Author

To repeat my points from the email.

In this scenario:

class MyEntity {
}

interface MyRepository extends CrudRepository<@Valid MyEntity, @Min(0) Long> {
}

interface CrudRepository<E, ID> {

    E findById(ID id);

}

Annotations @Valid and @Min are defined as TYPE_USE, in my understanding, they should be bound to the specified type (MyEntity/Long) and should be available any time the type is used in the repository.
So if some framework reads the return type annotations of the findById, it should see @Valid.

The annotations that should be bound only to the type argument are defined as TYPE_PARAMETER.

@njr-11
Copy link
Contributor

njr-11 commented Sep 1, 2023

interface MyRepository extends CrudRepository<@Valid MyEntity, @Min(0) Long> {

I just tried out adding a constraint to the ID type variable as in your example. This doesn't seem to work either, suggesting that maybe the issue with Jakarta Validation is bigger than just @Valid. I made a note of this in the issue against Jakarta Validation.

@otaviojava
Copy link
Contributor

@njr-11 @dstepanov could we close this issue?

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

No branches or pull requests

4 participants