|
Hi Emmanuel,
Micronaut is using Javac to decide on hidden/overridden methods. I rechecked by using the debugger and Javac's
javax.lang.model.util.Elements. For both "overrides" and "hides" it returns false. So that's one of those cases when the first method definition is the chosen one.
The reflection behaves the same. The "getMethod" returns the first method and only its annotations.
Denis
From: Emmanuel Bernard <ebernard@xxxxxxxxxx>
Sent: Monday, September 4, 2023 12:31 PM
To: Denis Stepanov <denis.stepanov@xxxxxxxxx>
Cc: Guillaume Smet <gsmet@xxxxxxxxxx>; Gunnar Morling <gunnar@xxxxxxxxxxxxx>; Nathan Rauh <nathan.rauh@xxxxxxxxxx>; bean-validation developer discussions <bean-validation-dev@xxxxxxxxxxx>; Denis Stepanov <denis.s.stepanov@xxxxxxxxxx>; denis.stepanov+github@xxxxxxxxx
<denis.stepanov+github@xxxxxxxxx>
Subject: [External] : Re: [bean-validation-dev] Proposed usage of @Valid by Jakarta Data specification
Hi Denis,
Thank you for the test you wrote. From what I see, the test validated the behavior of Mivronaut wrt type parameter inheritance, it is not validating some JLS / JVM behavior, I how is understanding ?
If correct than this micronaut behavior is a choice you’ve made. Is it deterministic? Do you have rules you could write down?
Emmanuel
Hi all,
Regarding 1.
Denis
Hey all,
I think there's two discussions hiding in here.
1. Automatic propagation of constraints/@Valid from type parameters to all the usages of that type.
I vaguely remember that we did consider this option while working on Bean Validation 2.0 but decided against it in the end. I don't remember the reasons (didn't find anything on the mailing list either, but it may warrant a more extensive search by someone).
I don't think anything speaks against it fundamentally, we may just have shied away from it due to time constraints. When adding this feature, one important aspect to consider is handling of parallel types in inheritance hierarchies and dealing with any potential
conflicts:
public interface BaseRepository<K, V> {
V findById(K key);
}
public interface Finder<K, V> {
V findById(K key);
}
public class MyImpl implements BaseRepository<Long, @NotNull Customer>, Finder<Long, @Null Customer> { // are all the constraints
applied? How about conflicting group conversion rules for @Valid, just rule them out? }
An (arguably even more attractive) variant of that feature is the definition of constraints/@Valid on the type parameters of the base type itself, thus making those more semantically rich types:
public interface BaseRepository<@NotNull @Valid K, @NotNull @Valid V> { // apply to every usage of K, V in the entire hierarchy
V findById(K key);
}
2. When should validation be executed by default
Best,
--Gunnar
> 4. My reading of your experience it looks like CDI is implementing method validation for you (your test on @Valid and @ValidateOnExecution) and
you did not implement it explicitly in the Data repo code, am I correct?
I tried to keep the experiment that I did very straightforward in order to be sure exactly which methods are being invoked, so I had some mock Data repository code directly invoke
ExecutableValidator.validateParameters for the experiment. It showed the Valid annotation is being used to opt-in to cascading validation if the Valid annotation is specified on the method argument type. It showed that
specifying the Valid annotation on the interface parameterized type did not add the cascading validation to usage of that type by methods of the interface. Would it be possible to add the latter to the Jakarta EE 11 version of Jakarta Bean Validation? I
think then we would have a fairly complete solution here.
Reading your sentence, I am thinking of @Inherited https: //docs. oracle. com/en/java/javase/11/docs/api/java. base/java/lang/annotation/Inherited. html but I think I am confused. I don't think
@Inherited has anything to do here. You are arguing
|
This Message Is From an Untrusted Sender
|
|
You have not previously corresponded with this sender.
|
|
|
I don't think @Inherited has anything to do here.
You are arguing for enabling TYPE_PARAMETER on @Valid and on all constraint annotations.
My point is, in this case:
interface MyRepository extends BaseRepository<@MyTypeUseAnnotation1 MyEntity, @MyTypeUseAnnotation2 MyId> {..} and BaseRepository<E, ID>
Every TYPE_USE annotation put on the generic parameters type in "BaseRepository" should be present for every use of a generic type of E/ID in the superclass. I believe that's the official Java annotation behavior.
I would expect that if you use the reflection to read the return annotations of "MyRepository # E findById(ID id)" you should get "@MyTypeUseAnnotation1 for E and @MyTypeUseAnnotation2 for ID".
And in Micronaut, the example:
"interface MyRepository extends BaseRepository<@MyTypeUseAnnotation1 MyEntity, @MyTypeUseAnnotation2 MyId>"
It is equivalent to repeating the annotations on the entity and the id generic type arguments.
interface MyRepository extends BaseRepository<MyEntity, MyId> {
@MyTypeUseAnnotation1 MyEntity findById(@MyTypeUseAnnotation2 MyId);
If the annotations were used with TYPE_PARAMETER instead, this propagation should not occur as the annotation would be bound only to the type parameter.
I hope I managed to explain it.
My point here is that nothing needs to be changed in the specification, and the only trigger for the repository to be validated could be any presence of @Valid or @Constraint.
Regarding "we did not extend the semantic of @Valid to class parameters and allow propagation", the @Valid is defined as TYPE_USE, so if I'm correct, this means that the annotation should appear on every type
that is being used in the superclass repository.
TYPE_USE was added in Bean Validation to support array and collections (that we call containers in the spec).
Thinks like `public void processMap(Map<@Email String, @Valid User>) { ... }` can be expressed.
It is applied where the type is... used :) Not where it is defined.
Your use case would be if the annotation would have TYPE_PARAMETER. That's how it works in Micronaut.
My idea of using `@Valid` or any other constraints of the repository arguments is that if a user already requests the validation by putting it on the type parameter, I don't think we should require something
extra like @ValidateOnExecution to put on the repository.
Yes understood. A couple of clarification and rephrasing.
You are arguing for enabling TYPE_PARAMETER on @Valid and on all constraint annotations. with the idea that these constraints would be semantically copied on any usage site of the parameter in question. In
Nathan's example, any usage of the parameter where the value is set to `Student` (e.g. on `save` etc.).
Yes in this case @ValidateEnExecute is not technically needed (like some CDI engine do no require it on the type). What remains is the conrner cases and semantic of this type parameter inheritance of constraints.
This can be wide and need to be thought of carefully.
Bean Validation will not interpret @ValidateOnExecution nor trigger the method validation. It is the interception / AOP technology responsibility, in your case the data repository technology. Each integration
layer implements the rule.
And you are correct that to validate a class constraint, the parameter (or return value) should be annotated with `@Valid` , at least that the Bean Validation current spec model.
And you are correct that today, we did not extend the semantic of @Valid to class parameters and allow propagation. We could but I don't remember why we decided not to do it today (this would enable the Micronaut
case assuming @ValidateOnExecution is also present somewhere).
Rereading chapter 11, `@ValidateOnExecution` is not mandatory but some implementations of CDI require it to detect methods marked with validation annotations. This is not the case of all solutions and by default
any method marked with constraint should be validated by the interceptor technology (like CDI or the Data repo if it's not a CDI bean already).
I have one question before trying to guide the rest, you say the committee prefers to have validation disabled by default but is that an all in from there? Meaning will it validate for `save()`, what about
other convenient methods? Do you want granularity in your validation or rather an all validated vs none validated switch? What about custom methods like `findByStudent(Student)`, what is the expected behavior?
Here is what runs through my head at the moment,
1. method validation is on by default in Bean Validation so your default would clash
2. @Valid is the right way to ask for a class to be validated
3. so maybe class parameters annotated @Valid is an ok pattern (we would need to rediscuss the sections discussing the Liskov substitution principles to be sure we want that)
4. My reading of your experience it looks like CDI is implementing method validation for you (your test on @Valid and @ValidateOnExecution) and you did not implement it explicitly in the Data repo code, am
I correct?
Emmanuel,
Thanks for your response. It is very helpful.
Reading more about ValidateOnExecution, it looks like this is how the user requests validation of repository methods per constraints that are specified on the method parameters and return value. For example,
this would cause the parameters to the findStudentById method below to be validated via ExecutableValidator.validateParameters, and likewise the return value via ExecutableValidator.validateReturnValue (although the return value in my example doesn’t have
any constraints),
@Repository
@ValidateOnExecution
public interface School extends DataRepository<Student, String> {
Student findByStudentId(@Min(1) @Max(99999) int studentIdNum);
Student save(Student s);
}
This seems useful for method parameter constraints and return value constraints. However, what ExecutableValidator.validateParameters does not do (when I just tried it) is automatically validate constraints
that are on the Student class itself, as in the case of the save method above. But if I add the @Valid annotation to the Student parameter of the save method, such that the validation cascades to it, then ExecutableValidator.validateParameters does automatically
validate the constraints from the Student class,
@Repository
@ValidateOnExecution
public interface School extends DataRepository<Student, String> {
Student findByStudentId(@Min(1) @Max(99999) int studentIdNum);
Student save(@Valid Student s);
}
Similarly, with ValidateOnExecution on the method,
@Repository
public interface School extends DataRepository<Student, String> {
@ValidateOnExecution
Student findByStudentId(@Min(1) @Max(99999) int studentIdNum);
@ValidateOnExecution
Student save(@Valid Student s);
}
So it looks like we need them both in combination.
Mostly, this seems fine. The problem we run into is that the user typically doesn’t provide the save method because they inherit it from a built-in convenience interface, such as PageableRepository or CrudRepository,
which defines save, remove, and some other common operations for them,
@Repository
@ValidateOnExecution
public interface School extends CrudRepository<Student, String> {
Student findByStudentId(@Min(1) @Max(99999) int studentIdNum);
// common operations like save, remove, ... are inherited
}
In the above case, the user loses the place where they can put @Valid onto the save parameter. I can see why Micronaut wanted to put it on the parameterized type to the repository interface -
CrudRepository<@Valid Student, String> - in an attempt to get it to cascade to methods such as save that accept the entity as a parameter. I tried invoking ExecutableValidator.validateParameters
on save in this case, and it doesn’t achieve getting the validation to cascade to the constraints of the Student class that is the parameter to the save method.
I suppose one option would be to force the user to override the save operation in order to add @Valid to the parameter,
@Repository
@ValidateOnExecution
public interface School extends CrudRepository<Student, String> {
Student findByStudentId(@Min(1) @Max(99999) int studentIdNum);
Student save(@Valid Student s); // duplicated by user just to add @Valid
}
The above would work, but is unfortunate because it requires duplication and extra work from the end user that defeats the purpose of why they used CrudRepository with built-in methods in the first place.
The other questions that I have are around the ValidateOnExecution annotation. Jakarta Data is looking for a pattern where automatic validation is disabled for Jakarta Data repositories unless the application
specifically opts into it, which now looks like it would be done via the ValidateOnExecution annotation. We would document that mechanism in our spec. Who is responsible for interpreting the presence of ValidateOnExecution? Is it expected that the Jakarta
Data provider would directly look for it (on repository classes/methods)? Or is there a way that the Jakarta Data provider should be asking Bean Validation whether a method is designed as having the ValidateOnExecution behavior? And if so, what is that mechanism?
Either seems fine, but the latter would allow for Bean Validation to have other ways of designating the behavior beyond just the annotation and allow those to be taken into account.
Hey there, I did not spend a lot of time looking into it but scanning through the Bean Validation spec to refresh my memory, here is what comes up for me. I think the semantic is closer to a @ValidateOnExecution
annotation than @Valid which
|
This Message Is From an External Sender
|
|
This message came from outside your organization.
|
|
|
I did not spend a lot of time looking into it but scanning through the Bean Validation spec to refresh my memory, here is what comes up for me.
I think the semantic is closer to a @ValidateOnExecution annotation than @Valid which has specific cascading semantics irrespective of *when* the validation happens. I don't remember the exact reasons but
we did not reuse @Valid for the usages of @ValidateOnExecution.
It might even be reusing @ValidateOnExecution for that matter (would need to add the extra TARGET)
If @Valid is used (which I think would be the wrong choice, here is a cursory list of what should be looked after to make sure it does not break things
- TraversableResolver and Node
- Concept of class parameterized providing inheritance to the usage site (e.g. T getFoo() in heriting Bar<@Valid T>
- This implied the traversal rules that are quite non trivial
- The data repo interface Would have a ValueExtractor implementation?
- ExecutableValidator like interface for these containers?
- figure out how make Group and its annotations work the same if we go that way
So far these concepts are under chapter 11 of the bean validation spec as it describe integration standard semantics with other technologies (CDI, JPA, JSF, Jakarta EE etc)
Hi, I'd like to check with the Jakarta Bean Validation specification group on a proposed usage of the jakarta.validation.Valid annotation for Jakarta Data. As discussed in our
issue 216,
we are looking for a way to allow users to opt in to automatic validation of entities when saved via Jakarta Data repositories. The most straightforward way for users to ask for this is to standardize the approach that existing vendors (such as Micronaut)
are already using, which is that users opt in by placing the Valid annotation on the entity parameter of the repository, as follows,
@Repository
public interface School extends PageableRepository<@Valid Student, String> {
}
Because this differs somewhat from the intended purpose of the Valid annotation, I’d like to check with the Jakarta Bean Validation specification group on whether you would find this usage/overloading acceptable
before defining this pattern in the Jakarta Data specification. There are other ways we could define the ability to opt in in Jakarta Data, but this one is preferable if you find it acceptable because it matches what implementations are already doing. Let
me know what your thoughts are.
_______________________________________________
bean-validation-dev mailing list
bean-validation-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/bean-validation-dev
|
