rmannibucau commented on code in PR #191:
URL: https://github.com/apache/bval/pull/191#discussion_r2096416804
##########
bval-jsr/src/main/java/org/apache/bval/cdi/BValExtension.java:
##########
@@ -132,28 +134,31 @@ public <A> void processAnnotatedType(final @Observes
ProcessAnnotatedType<A> pat
final int modifiers = javaClass.getModifiers();
if (!javaClass.isInterface() && !javaClass.isAnonymousClass() &&
!Modifier.isFinal(modifiers) && !Modifier.isAbstract(modifiers)) {
try {
- Queue<Class<?>> toProcess = new LinkedList<>();
- toProcess.add(annotatedType.getJavaClass());
+ Queue<AnnotatedType<?>> toProcess = new LinkedList<>();
+ toProcess.add(annotatedType);
while (!toProcess.isEmpty()) {
- Class<?> now = toProcess.poll();
- Executable[] methods = now.getMethods();
- Executable[] constructors = now.getConstructors();
+ AnnotatedType<?> now = toProcess.poll();
+ Set<? extends AnnotatedMethod<?>> methods =
now.getMethods();
+ Set<? extends AnnotatedConstructor<?>> constructors =
now.getConstructors();
if (hasValidation(now)
- || hasValidation(methods) ||
hasValidation(constructors)
- || hasParamsWithValidation(methods) ||
hasParamsWithValidation(constructors)) {
+ || methods.stream().anyMatch(this::hasValidation)
+ ||
constructors.stream().anyMatch(this::hasValidation)
+ ||
methods.stream().anyMatch(this::hasParamsWithValidation)
+ ||
constructors.stream().anyMatch(this::hasParamsWithValidation)) {
pat.setAnnotatedType(new
BValAnnotatedType<>(annotatedType));
-
break;
}
- // Nothing found, collect superclass/interface and repeat
(See BVAL-222)
- if (now.getSuperclass() != Object.class &&
now.getSuperclass() != null) {
- toProcess.add(now.getSuperclass());
+ Class<?> superclass = now.getJavaClass().getSuperclass();
Review Comment:
> one could argue it weakens preconditions but there is also no way for it
to "re enforce" the precondition from the parent
this is allowed and this is why we must not check the parents IMHO, what you
can't relax is the returned type contract, if the result is not null then you
cant return null but if you accept a not null param it is ok to accept a null
param since you do not break any parent use cases, you enable more.
> I think it is better to have false positives than false negatives
well there are multiple things there:
1. we speak about hundreds (thousands) vs dozens so it is not a small thing
- the bval filter is there for that, also note it applies to applications not
using bval at all or for a few beans
2. it is not only about "false" detection but also about not possible use
cases (relaxing a param must be possible and is enabled by cdi api so bval-cdi
must respect it)
while using pure reflection it is ok-ish since everything is kind of
immutable, using cdi where all the model is mutable this is very limiting to do
it so I wouldn't even try
maybe worse case we use a system prop disabled by default and enabled in tck
module but I would really make the extension fast, detection is already not
crazy fast due to the spec rules which doesn't enforce an interceptor binding
IIRC (if done it wouldn't be a real issue)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
