Skip to content

Commit 28efd75

Browse files
marko-bekhtamarko-bekhta
committed
HV-1831 Handle processed bean tracking for executables
Signed-off-by: marko-bekhta <[email protected]>
1 parent 997c626 commit 28efd75

File tree

6 files changed

+310
-83
lines changed

6 files changed

+310
-83
lines changed

‎engine/src/main/java/org/hibernate/validator/internal/engine/tracking/PredefinedScopeProcessedBeansTrackingStrategy.java‎

Lines changed: 114 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@
1818
import org.hibernate.validator.internal.metadata.aggregated.CascadingMetaData;
1919
import org.hibernate.validator.internal.metadata.aggregated.ContainerCascadingMetaData;
2020
import org.hibernate.validator.internal.metadata.aggregated.PotentiallyContainerCascadingMetaData;
21+
import org.hibernate.validator.internal.metadata.aggregated.ReturnValueMetaData;
22+
import org.hibernate.validator.internal.metadata.aggregated.ValidatableParametersMetaData;
2123
import org.hibernate.validator.internal.metadata.facets.Cascadable;
2224
import org.hibernate.validator.internal.properties.Signature;
2325
import org.hibernate.validator.internal.util.CollectionHelper;
@@ -26,8 +28,11 @@ public class PredefinedScopeProcessedBeansTrackingStrategy implements ProcessedB
2628

2729
private final Map<Class<?>, Boolean> trackingEnabledForBeans;
2830

31+
// TODO: signature is just name and parameters so that can clash between different beans.
32+
// with that.. do we even need to track it per signature or since
33+
// we already built the `trackingEnabledForBeans` we can just "inspect" the cascadable as we go
34+
// and check against this `trackingEnabledForBeans` to see if tracking is required ?
2935
private final Map<Signature, Boolean> trackingEnabledForReturnValues;
30-
3136
private final Map<Signature, Boolean> trackingEnabledForParameters;
3237

3338
public PredefinedScopeProcessedBeansTrackingStrategy(Map<Class<?>, BeanMetaData<?>> rawBeanMetaDataMap) {
@@ -198,92 +203,94 @@ private <T> Set<Class<?>> getDirectCascadedBeanClasses(Class<T> beanClass) {
198203
}
199204

200205
for ( Cascadable cascadable : beanMetaData.getCascadables() ) {
201-
final CascadingMetaData cascadingMetaData = cascadable.getCascadingMetaData();
202-
if ( cascadingMetaData.isContainer() ) {
203-
final ContainerCascadingMetaData containerCascadingMetaData = cascadingMetaData.as( ContainerCascadingMetaData.class );
204-
processContainerCascadingMetaData( containerCascadingMetaData, directCascadedBeanClasses );
205-
}
206-
else if ( cascadingMetaData instanceof PotentiallyContainerCascadingMetaData potentiallyContainerCascadingMetaData ) {
207-
// if it's a potentially container cascading one, we are "in trouble" as thing can be "almost anything".
208-
// TODO: would it be enough to just take the type as defined ?
209-
// directCascadedBeanClasses.add( (Class<?>) cascadable.getCascadableType() );
210-
//
211-
// TODO: or be much more cautious and just assume that it can be "anything":
212-
directCascadedBeanClasses.add( Object.class );
213-
}
214-
else {
215-
// TODO: For now, assume non-container Cascadables are always beans. Truee???
216-
directCascadedBeanClasses.add( typeToClassToProcess( cascadable.getCascadableType() ) );
217-
}
206+
processSingleCascadable( cascadable, directCascadedBeanClasses );
218207
}
219208
}
220209
return directCascadedBeanClasses;
221210
}
222211

223-
private static void processContainerCascadingMetaData(ContainerCascadingMetaData metaData, Set<Class<?>> directCascadedBeanClasses) {
224-
if ( metaData.isCascading() ) {
225-
if ( metaData.getDeclaredTypeParameterIndex() != null ) {
226-
if ( metaData.getEnclosingType() instanceof ParameterizedType parameterizedType ) {
227-
Type typeArgument = parameterizedType.getActualTypeArguments()[metaData.getDeclaredTypeParameterIndex()];
228-
if ( typeArgument instanceof Class<?> typeArgumentClass ) {
229-
directCascadedBeanClasses.add( typeArgumentClass );
230-
}
231-
else if ( typeArgument instanceof TypeVariable<?> typeVariable ) {
232-
for ( Type bound : typeVariable.getBounds() ) {
233-
directCascadedBeanClasses.add( typeToClassToProcess( bound ) );
234-
}
212+
private boolean register(Class<?> beanClass, boolean isBeanTrackingEnabled) {
213+
if ( classToBeanTrackingEnabled.put( beanClass, isBeanTrackingEnabled ) != null ) {
214+
throw new IllegalStateException( beanClass.getName() + " registered more than once." );
215+
}
216+
return isBeanTrackingEnabled;
217+
}
218+
}
219+
220+
private static void processSingleCascadable(Cascadable cascadable, Set<Class<?>> directCascadedBeanClasses) {
221+
CascadingMetaData cascadingMetaData = cascadable.getCascadingMetaData();
222+
if ( cascadingMetaData.isContainer() ) {
223+
final ContainerCascadingMetaData containerCascadingMetaData = cascadingMetaData.as( ContainerCascadingMetaData.class );
224+
processContainerCascadingMetaData( containerCascadingMetaData, directCascadedBeanClasses );
225+
}
226+
else if ( cascadingMetaData instanceof PotentiallyContainerCascadingMetaData potentiallyContainerCascadingMetaData ) {
227+
// if it's a potentially container cascading one, we are "in trouble" as thing can be "almost anything".
228+
// TODO: would it be enough to just take the type as defined ?
229+
// directCascadedBeanClasses.add( (Class<?>) cascadable.getCascadableType() );
230+
//
231+
// TODO: or be much more cautious and just assume that it can be "anything":
232+
directCascadedBeanClasses.add( Object.class );
233+
}
234+
else {
235+
// TODO: For now, assume non-container Cascadables are always beans. Truee???
236+
directCascadedBeanClasses.add( typeToClassToProcess( cascadable.getCascadableType() ) );
237+
}
238+
}
239+
240+
private static void processContainerCascadingMetaData(ContainerCascadingMetaData metaData, Set<Class<?>> directCascadedBeanClasses) {
241+
if ( metaData.isCascading() ) {
242+
if ( metaData.getDeclaredTypeParameterIndex() != null ) {
243+
if ( metaData.getEnclosingType() instanceof ParameterizedType parameterizedType ) {
244+
Type typeArgument = parameterizedType.getActualTypeArguments()[metaData.getDeclaredTypeParameterIndex()];
245+
if ( typeArgument instanceof Class<?> typeArgumentClass ) {
246+
directCascadedBeanClasses.add( typeArgumentClass );
247+
}
248+
else if ( typeArgument instanceof TypeVariable<?> typeVariable ) {
249+
for ( Type bound : typeVariable.getBounds() ) {
250+
directCascadedBeanClasses.add( typeToClassToProcess( bound ) );
235251
}
236-
else if ( typeArgument instanceof WildcardType wildcard ) {
237-
for ( Type bound : wildcard.getUpperBounds() ) {
238-
directCascadedBeanClasses.add( typeToClassToProcess( bound ) );
239-
}
240-
if ( wildcard.getLowerBounds().length != 0 ) {
241-
// if it's a lower bound ? super smth ... it doesn't matter anymore since it can contain anything so go with object ?
242-
directCascadedBeanClasses.add( Object.class );
243-
}
252+
}
253+
else if ( typeArgument instanceof WildcardType wildcard ) {
254+
for ( Type bound : wildcard.getUpperBounds() ) {
255+
directCascadedBeanClasses.add( typeToClassToProcess( bound ) );
244256
}
245-
else {
246-
// TODO: instead of failing, add an Object.class and assume it can be anything ?
247-
throw new UnsupportedOperationException( typeArgument.getClass().getSimpleName() + " type argument values are not supported." );
257+
if ( wildcard.getLowerBounds().length != 0 ) {
258+
// if it's a lower bound ? super smth ... it doesn't matter anymore since it can contain anything so go with object ?
259+
directCascadedBeanClasses.add( Object.class );
248260
}
249261
}
250-
}
251-
else {
252-
// If we do not have the type arguments then we can go though the value extractors,
253-
// as they are required to define the `@ExtractedValue(type = ???)` ...
254-
// this way we should get the type we want:
255-
for ( ValueExtractorDescriptor valueExtractorCandidate : metaData.getValueExtractorCandidates() ) {
256-
valueExtractorCandidate.getExtractedType().ifPresent( directCascadedBeanClasses::add );
262+
else {
263+
// TODO: instead of failing, add an Object.class and assume it can be anything ?
264+
throw new UnsupportedOperationException( typeArgument.getClass().getSimpleName() + " type argument values are not supported." );
257265
}
258266
}
259267
}
260-
261-
if ( metaData.getEnclosingType() instanceof ParameterizedType parameterizedType ) {
262-
for ( ContainerCascadingMetaData sub : metaData.getContainerElementTypesCascadingMetaData() ) {
263-
processContainerCascadingMetaData( sub, directCascadedBeanClasses );
268+
else {
269+
// If we do not have the type arguments then we can go though the value extractors,
270+
// as they are required to define the `@ExtractedValue(type = ???)` ...
271+
// this way we should get the type we want:
272+
for ( ValueExtractorDescriptor valueExtractorCandidate : metaData.getValueExtractorCandidates() ) {
273+
valueExtractorCandidate.getExtractedType().ifPresent( directCascadedBeanClasses::add );
264274
}
265275
}
266276
}
267277

268-
private static Class<?> typeToClassToProcess(Type type) {
269-
if ( type instanceof Class<?> cascadableClass ) {
270-
return cascadableClass;
271-
}
272-
else if ( type instanceof ParameterizedType parameterizedType ) {
273-
return typeToClassToProcess( parameterizedType.getRawType() );
274-
}
275-
else {
276-
// TODO: instead of failing, add an Object.class and assume it can be anything ?
277-
// return Object.class;
278-
throw new UnsupportedOperationException( type.getClass().getSimpleName() + " type values are not supported." );
279-
}
278+
for ( ContainerCascadingMetaData sub : metaData.getContainerElementTypesCascadingMetaData() ) {
279+
processContainerCascadingMetaData( sub, directCascadedBeanClasses );
280280
}
281+
}
281282

282-
private boolean register(Class<?> beanClass, boolean isBeanTrackingEnabled) {
283-
if ( classToBeanTrackingEnabled.put( beanClass, isBeanTrackingEnabled ) != null ) {
284-
throw new IllegalStateException( beanClass.getName() + " registered more than once." );
285-
}
286-
return isBeanTrackingEnabled;
283+
private static Class<?> typeToClassToProcess(Type type) {
284+
if ( type instanceof Class<?> cascadableClass ) {
285+
return cascadableClass;
286+
}
287+
else if ( type instanceof ParameterizedType parameterizedType ) {
288+
return typeToClassToProcess( parameterizedType.getRawType() );
289+
}
290+
else {
291+
// TODO: instead of failing, add an Object.class and assume it can be anything ?
292+
// return Object.class;
293+
throw new UnsupportedOperationException( type.getClass().getSimpleName() + " type values are not supported." );
287294
}
288295
}
289296

@@ -305,6 +312,25 @@ public boolean isEnabledForReturnValue(Signature signature, boolean hasCascadabl
305312
return trackingEnabledForReturnValues.getOrDefault( signature, true );
306313
}
307314

315+
@Override
316+
public boolean isEnabledForReturnValue(ReturnValueMetaData returnValueMetaData) {
317+
if ( !returnValueMetaData.isCascading() ) {
318+
return false;
319+
}
320+
321+
Set<Class<?>> directCascadedBeanClasses = new HashSet<>();
322+
for ( Cascadable cascadable : returnValueMetaData.getCascadables() ) {
323+
processSingleCascadable( cascadable, directCascadedBeanClasses );
324+
}
325+
for ( Class<?> directCascadedBeanClass : directCascadedBeanClasses ) {
326+
if ( trackingEnabledForBeans.get( directCascadedBeanClass ) ) {
327+
return true;
328+
}
329+
}
330+
331+
return false;
332+
}
333+
308334
@Override
309335
public boolean isEnabledForParameters(Signature signature, boolean hasCascadables) {
310336
if ( !hasCascadables ) {
@@ -314,6 +340,25 @@ public boolean isEnabledForParameters(Signature signature, boolean hasCascadable
314340
return trackingEnabledForParameters.getOrDefault( signature, true );
315341
}
316342

343+
@Override
344+
public boolean isEnabledForParameters(ValidatableParametersMetaData parametersMetaData) {
345+
if ( !parametersMetaData.hasCascadables() ) {
346+
return false;
347+
}
348+
349+
Set<Class<?>> directCascadedBeanClasses = new HashSet<>();
350+
for ( Cascadable cascadable : parametersMetaData.getCascadables() ) {
351+
processSingleCascadable( cascadable, directCascadedBeanClasses );
352+
}
353+
for ( Class<?> directCascadedBeanClass : directCascadedBeanClasses ) {
354+
if ( trackingEnabledForBeans.get( directCascadedBeanClass ) ) {
355+
return true;
356+
}
357+
}
358+
359+
return false;
360+
}
361+
317362
@Override
318363
public void clear() {
319364
trackingEnabledForBeans.clear();

‎engine/src/main/java/org/hibernate/validator/internal/engine/tracking/ProcessedBeansTrackingStrategy.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44
*/
55
package org.hibernate.validator.internal.engine.tracking;
66

7+
import org.hibernate.validator.internal.metadata.aggregated.ReturnValueMetaData;
8+
import org.hibernate.validator.internal.metadata.aggregated.ValidatableParametersMetaData;
79
import org.hibernate.validator.internal.properties.Signature;
810

911
public interface ProcessedBeansTrackingStrategy {
@@ -12,7 +14,11 @@ public interface ProcessedBeansTrackingStrategy {
1214

1315
boolean isEnabledForReturnValue(Signature signature, boolean hasCascadables);
1416

17+
boolean isEnabledForReturnValue(ReturnValueMetaData returnValueMetaData);
18+
1519
boolean isEnabledForParameters(Signature signature, boolean hasCascadables);
1620

21+
boolean isEnabledForParameters(ValidatableParametersMetaData parametersMetaData);
22+
1723
void clear();
1824
}

‎engine/src/main/java/org/hibernate/validator/internal/metadata/aggregated/ExecutableMetaData.java‎

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -159,21 +159,11 @@ public ExecutableMetaData(ExecutableMetaData originalExecutableMetaData,
159159
}
160160

161161
private boolean trackingEnabledForParameters(ProcessedBeansTrackingStrategy processedBeansTrackingStrategy) {
162-
boolean trackingEnabledForParameters = false;
163-
for ( Signature signature : getSignatures() ) {
164-
trackingEnabledForParameters = trackingEnabledForParameters
165-
|| processedBeansTrackingStrategy.isEnabledForParameters( signature, getValidatableParametersMetaData().hasCascadables() );
166-
}
167-
return trackingEnabledForParameters;
162+
return processedBeansTrackingStrategy.isEnabledForParameters( getValidatableParametersMetaData() );
168163
}
169164

170165
private boolean trackingEnabledForReturnValue(ProcessedBeansTrackingStrategy processedBeansTrackingStrategy) {
171-
boolean trackingEnabledForReturnValue = false;
172-
for ( Signature signature : getSignatures() ) {
173-
trackingEnabledForReturnValue = trackingEnabledForReturnValue
174-
|| processedBeansTrackingStrategy.isEnabledForReturnValue( signature, getReturnValueMetaData().hasCascadables() );
175-
}
176-
return trackingEnabledForReturnValue;
166+
return processedBeansTrackingStrategy.isEnabledForReturnValue( getReturnValueMetaData() );
177167
}
178168

179169
/**

‎engine/src/test/java/org/hibernate/validator/test/internal/engine/tracking/ProcessedBeansTrackingCycles7Test.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,12 @@
99
import java.util.List;
1010
import java.util.Set;
1111

12-
import org.hibernate.validator.testutils.ValidatorUtil;
13-
1412
import jakarta.validation.Valid;
1513
import jakarta.validation.Validator;
1614
import jakarta.validation.constraints.NotNull;
15+
16+
import org.hibernate.validator.testutils.ValidatorUtil;
17+
1718
import org.testng.annotations.DataProvider;
1819
import org.testng.annotations.Test;
1920

‎engine/src/test/java/org/hibernate/validator/test/internal/engine/tracking/ProcessedBeansTrackingCyclesExecutable1Test.java‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
/*
2+
* SPDX-License-Identifier: Apache-2.0
3+
* Copyright Red Hat Inc. and Hibernate Authors
4+
*/
5+
package org.hibernate.validator.test.internal.engine.tracking;
6+
7+
import java.lang.reflect.Method;
8+
import java.util.List;
9+
import java.util.Optional;
10+
import java.util.Set;
11+
12+
import jakarta.validation.Valid;
13+
import jakarta.validation.Validator;
14+
import jakarta.validation.constraints.NotNull;
15+
import jakarta.validation.constraints.Positive;
16+
17+
import org.hibernate.validator.testutils.ValidatorUtil;
18+
19+
import org.testng.annotations.DataProvider;
20+
import org.testng.annotations.Test;
21+
22+
/**
23+
* This scenario is a simple return value and parameter cascading validation.
24+
*/
25+
public class ProcessedBeansTrackingCyclesExecutable1Test {
26+
27+
@DataProvider(name = "validators")
28+
public Object[][] createValidators() {
29+
return new Object[][] {
30+
{ ValidatorUtil.getValidator() },
31+
{ ValidatorUtil.getPredefinedValidator( Set.of( Parent.class, ChildWithNoCycles.class, Child.class ) ) }
32+
};
33+
}
34+
35+
@Test(dataProvider = "validators")
36+
public void testCycle(Validator validator) throws Exception {
37+
Parent parent = new Parent();
38+
Method doSomething = Parent.class.getMethod( "doSomething", int.class, List.class );
39+
validator.forExecutables()
40+
.validateReturnValue(
41+
parent,
42+
doSomething,
43+
Optional.of( parent )
44+
);
45+
46+
validator.forExecutables().validateParameters(
47+
parent,
48+
doSomething,
49+
new Object[] { 10, List.of( new ChildWithNoCycles(), new Child( parent ) ) }
50+
);
51+
}
52+
53+
private static class Parent {
54+
55+
@NotNull
56+
private String property;
57+
58+
private List<@Valid ChildWithNoCycles> children;
59+
60+
public Parent() {
61+
this.property = null;
62+
this.children = List.of( new ChildWithNoCycles(), new Child( this ) );
63+
}
64+
65+
public Optional<@Valid Parent> doSomething(@Positive int number, List<@Valid ChildWithNoCycles> children) {
66+
this.property = null;
67+
return Optional.of( this );
68+
}
69+
}
70+
71+
private static class ChildWithNoCycles {
72+
73+
@NotNull
74+
private String property;
75+
}
76+
77+
private static class Child extends ChildWithNoCycles {
78+
79+
@Valid
80+
private Parent parent;
81+
82+
public Child(Parent parent) {
83+
this.parent = parent;
84+
}
85+
}
86+
}

0 commit comments

Comments
 (0)