Skip to content

Commit 2ccc534

Browse files
marko-bekhtamarko-bekhta
committed
HV-1831 A few cleanups
Signed-off-by: marko-bekhta <[email protected]>
1 parent c55ea33 commit 2ccc534

15 files changed

+302
-109
lines changed

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

Lines changed: 15 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
import java.lang.reflect.Type;
99
import java.lang.reflect.TypeVariable;
1010
import java.lang.reflect.WildcardType;
11-
import java.util.HashMap;
1211
import java.util.HashSet;
1312
import java.util.Map;
1413
import java.util.Set;
@@ -21,33 +20,17 @@
2120
import org.hibernate.validator.internal.metadata.aggregated.ReturnValueMetaData;
2221
import org.hibernate.validator.internal.metadata.aggregated.ValidatableParametersMetaData;
2322
import org.hibernate.validator.internal.metadata.facets.Cascadable;
24-
import org.hibernate.validator.internal.properties.Signature;
23+
import org.hibernate.validator.internal.metadata.facets.Validatable;
2524
import org.hibernate.validator.internal.util.CollectionHelper;
2625

2726
public class PredefinedScopeProcessedBeansTrackingStrategy implements ProcessedBeansTrackingStrategy {
2827

2928
private final Map<Class<?>, Boolean> trackingEnabledForBeans;
3029

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 ?
35-
private final Map<Signature, Boolean> trackingEnabledForReturnValues;
36-
private final Map<Signature, Boolean> trackingEnabledForParameters;
37-
3830
public PredefinedScopeProcessedBeansTrackingStrategy(Map<Class<?>, BeanMetaData<?>> rawBeanMetaDataMap) {
39-
// TODO: build the maps from the information inside the beanMetaDataManager
40-
// There is a good chance we will need a structure with the whole hierarchy of constraint classes.
41-
// That's something we could add to PredefinedScopeBeanMetaDataManager, as we are already doing similar things
42-
// there (see the ClassHierarchyHelper.getHierarchy call).
43-
// In the predefined scope case, we will have the whole hierarchy of constrained classes passed to
44-
// PredefinedScopeBeanMetaDataManager.
45-
4631
this.trackingEnabledForBeans = CollectionHelper.toImmutableMap(
4732
new TrackingEnabledStrategyBuilder( rawBeanMetaDataMap ).build()
4833
);
49-
this.trackingEnabledForReturnValues = CollectionHelper.toImmutableMap( new HashMap<>() );
50-
this.trackingEnabledForParameters = CollectionHelper.toImmutableMap( new HashMap<>() );
5134
}
5235

5336
private static class TrackingEnabledStrategyBuilder {
@@ -223,16 +206,13 @@ private static void processSingleCascadable(Cascadable cascadable, Set<Class<?>>
223206
final ContainerCascadingMetaData containerCascadingMetaData = cascadingMetaData.as( ContainerCascadingMetaData.class );
224207
processContainerCascadingMetaData( containerCascadingMetaData, directCascadedBeanClasses );
225208
}
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":
209+
else if ( cascadingMetaData instanceof PotentiallyContainerCascadingMetaData ) {
210+
// If it's a potentially container cascading one, we are "in trouble" as thing can be "almost anything".
211+
// Let's be much more cautious and just assume that it can be "anything":
232212
directCascadedBeanClasses.add( Object.class );
233213
}
234214
else {
235-
// TODO: For now, assume non-container Cascadables are always beans. Truee???
215+
// TODO: For now, assume non-container Cascadables are always beans. True???
236216
directCascadedBeanClasses.add( typeToClassToProcess( cascadable.getCascadableType() ) );
237217
}
238218
}
@@ -260,8 +240,8 @@ else if ( typeArgument instanceof WildcardType wildcard ) {
260240
}
261241
}
262242
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." );
243+
// In any unexpected case treat things as if they require tracking just to be on the safe side:
244+
directCascadedBeanClasses.add( Object.class );
265245
}
266246
}
267247
}
@@ -288,9 +268,7 @@ else if ( type instanceof ParameterizedType parameterizedType ) {
288268
return typeToClassToProcess( parameterizedType.getRawType() );
289269
}
290270
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." );
271+
return Object.class;
294272
}
295273
}
296274

@@ -303,55 +281,27 @@ public boolean isEnabledForBean(Class<?> rootBeanClass, boolean hasCascadables)
303281
return trackingEnabledForBeans.get( rootBeanClass );
304282
}
305283

306-
@Override
307-
public boolean isEnabledForReturnValue(Signature signature, boolean hasCascadables) {
308-
if ( !hasCascadables ) {
309-
return false;
310-
}
311-
312-
return trackingEnabledForReturnValues.getOrDefault( signature, true );
313-
}
314-
315284
@Override
316285
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;
286+
return isEnabledForExecutableValidatable( returnValueMetaData );
332287
}
333288

334289
@Override
335-
public boolean isEnabledForParameters(Signature signature, boolean hasCascadables) {
336-
if ( !hasCascadables ) {
337-
return false;
338-
}
339-
340-
return trackingEnabledForParameters.getOrDefault( signature, true );
290+
public boolean isEnabledForParameters(ValidatableParametersMetaData parametersMetaData) {
291+
return isEnabledForExecutableValidatable( parametersMetaData );
341292
}
342293

343-
@Override
344-
public boolean isEnabledForParameters(ValidatableParametersMetaData parametersMetaData) {
345-
if ( !parametersMetaData.hasCascadables() ) {
294+
private boolean isEnabledForExecutableValidatable(Validatable validatable) {
295+
if ( !validatable.hasCascadables() ) {
346296
return false;
347297
}
348298

349299
Set<Class<?>> directCascadedBeanClasses = new HashSet<>();
350-
for ( Cascadable cascadable : parametersMetaData.getCascadables() ) {
300+
for ( Cascadable cascadable : validatable.getCascadables() ) {
351301
processSingleCascadable( cascadable, directCascadedBeanClasses );
352302
}
353303
for ( Class<?> directCascadedBeanClass : directCascadedBeanClasses ) {
354-
if ( trackingEnabledForBeans.get( directCascadedBeanClass ) ) {
304+
if ( Boolean.TRUE.equals( trackingEnabledForBeans.get( directCascadedBeanClass ) ) ) {
355305
return true;
356306
}
357307
}
@@ -362,7 +312,5 @@ public boolean isEnabledForParameters(ValidatableParametersMetaData parametersMe
362312
@Override
363313
public void clear() {
364314
trackingEnabledForBeans.clear();
365-
trackingEnabledForReturnValues.clear();
366-
trackingEnabledForParameters.clear();
367315
}
368316
}

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

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,18 +6,13 @@
66

77
import org.hibernate.validator.internal.metadata.aggregated.ReturnValueMetaData;
88
import org.hibernate.validator.internal.metadata.aggregated.ValidatableParametersMetaData;
9-
import org.hibernate.validator.internal.properties.Signature;
109

1110
public interface ProcessedBeansTrackingStrategy {
1211

1312
boolean isEnabledForBean(Class<?> beanClass, boolean hasCascadables);
1413

15-
boolean isEnabledForReturnValue(Signature signature, boolean hasCascadables);
16-
1714
boolean isEnabledForReturnValue(ReturnValueMetaData returnValueMetaData);
1815

19-
boolean isEnabledForParameters(Signature signature, boolean hasCascadables);
20-
2116
boolean isEnabledForParameters(ValidatableParametersMetaData parametersMetaData);
2217

2318
void clear();

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,7 @@ private static <T> BeanMetaData<T> injectTrackingInformation(
234234
ProcessedBeansTrackingStrategy processedBeansTrackingStrategy,
235235
ProcessedBeansTrackingVoter processedBeansTrackingVoter
236236
) {
237-
return new BeanMetaDataImpl<T>( (BeanMetaDataImpl<T>) rawBeanMetaData, processedBeansTrackingStrategy, processedBeansTrackingVoter );
237+
return new BeanMetaDataImpl<>( (BeanMetaDataImpl<T>) rawBeanMetaData, processedBeansTrackingStrategy, processedBeansTrackingVoter );
238238
}
239239

240240
private static class UninitializedBeanMetaData<T> implements BeanMetaData<T> {

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,24 +30,24 @@
3030
public class ProcessedBeansTrackingCycles1Test {
3131

3232
@Test
33-
public void testValidNull() throws Exception {
33+
public void testValidNull() {
3434
final Parent parent = new Parent( "parent property" );
3535
Set<ConstraintViolation<Parent>> violations = getValidator().validate( parent );
3636
assertTrue( violations.isEmpty() );
3737
}
3838

3939
@Test
40-
public void testValidNotNull() throws Exception {
40+
public void testValidNotNull() {
4141
final Parent parent = new Parent( "parent property" );
4242
parent.child = new Child( "child property" );
4343

4444
Set<ConstraintViolation<Parent>> violations = getValidator().validate( parent );
45-
//Set<ConstraintViolation<Parent>> violations = ValidatorUtil.getValidator().validate( parent );
45+
4646
assertTrue( violations.isEmpty() );
4747
}
4848

4949
@Test
50-
public void testValidNotNullNonCyclic() throws Exception {
50+
public void testValidNotNullNonCyclic() {
5151
final Parent parent = new Parent( "parent property" );
5252
parent.child = new Child( "child property" );
5353
parent.child.parent = new Parent( "other parent property" );
@@ -57,7 +57,7 @@ public void testValidNotNullNonCyclic() throws Exception {
5757
}
5858

5959
@Test
60-
public void testValidNotNullCyclic() throws Exception {
60+
public void testValidNotNullCyclic() {
6161
final Parent parent = new Parent( "parent property" );
6262
parent.child = new Child( "child property" );
6363
parent.child.parent = parent;

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

Lines changed: 48 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import java.lang.reflect.Method;
88
import java.util.List;
99
import java.util.Optional;
10+
import java.util.OptionalInt;
1011
import java.util.Set;
1112

1213
import jakarta.validation.Valid;
@@ -33,7 +34,7 @@ public Object[][] createValidators() {
3334
}
3435

3536
@Test(dataProvider = "validators")
36-
public void testCycle(Validator validator) throws Exception {
37+
public void testCycle1(Validator validator) throws Exception {
3738
Parent parent = new Parent();
3839
Method doSomething = Parent.class.getMethod( "doSomething", int.class, List.class );
3940
validator.forExecutables()
@@ -43,11 +44,49 @@ public void testCycle(Validator validator) throws Exception {
4344
Optional.of( parent )
4445
);
4546

47+
}
48+
49+
@Test(dataProvider = "validators")
50+
public void testCycle2(Validator validator) throws Exception {
51+
Parent parent = new Parent();
52+
Method doSomething = Parent.class.getMethod( "doSomething", int.class, List.class );
53+
4654
validator.forExecutables().validateParameters(
4755
parent,
4856
doSomething,
4957
new Object[] { 10, List.of( new ChildWithNoCycles(), new Child( parent ) ) }
5058
);
59+
60+
Method doSomethingPotentiallyCascadable = Parent.class.getMethod( "doSomethingPotentiallyCascadable" );
61+
validator.forExecutables().validateParameters(
62+
parent,
63+
doSomethingPotentiallyCascadable,
64+
new Object[0]
65+
);
66+
}
67+
68+
@Test(dataProvider = "validators")
69+
public void testCycle3(Validator validator) throws Exception {
70+
Parent parent = new Parent();
71+
72+
Method doSomethingPotentiallyCascadable = Parent.class.getMethod( "doSomethingPotentiallyCascadable" );
73+
validator.forExecutables().validateParameters(
74+
parent,
75+
doSomethingPotentiallyCascadable,
76+
new Object[0]
77+
);
78+
}
79+
80+
@Test(dataProvider = "validators")
81+
public void testCycle4(Validator validator) throws Exception {
82+
Parent parent = new Parent();
83+
84+
Method doSomethingNoTypeIndex = Parent.class.getMethod( "doSomethingNoTypeIndex" );
85+
validator.forExecutables().validateParameters(
86+
parent,
87+
doSomethingNoTypeIndex,
88+
new Object[0]
89+
);
5190
}
5291

5392
private static class Parent {
@@ -66,6 +105,14 @@ public Parent() {
66105
this.property = null;
67106
return Optional.of( this );
68107
}
108+
109+
public @Valid Object doSomethingPotentiallyCascadable() {
110+
return null;
111+
}
112+
113+
public @Valid OptionalInt doSomethingNoTypeIndex() {
114+
return OptionalInt.of( 5 );
115+
}
69116
}
70117

71118
private static class ChildWithNoCycles {

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

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

7+
import static org.hibernate.validator.testutil.ConstraintViolationAssert.assertThat;
78
import static org.testng.Assert.assertFalse;
89
import static org.testng.Assert.assertTrue;
910

1011
import java.util.ArrayList;
1112
import java.util.Arrays;
1213
import java.util.HashSet;
1314
import java.util.List;
14-
import java.util.Set;
1515

16-
import jakarta.validation.ConstraintViolation;
1716
import jakarta.validation.Valid;
1817
import jakarta.validation.Validation;
1918
import jakarta.validation.Validator;
@@ -121,13 +120,13 @@ public void testValidate() {
121120
f.gValues.add( g );
122121

123122
final Validator validator = getValidator();
124-
final Set<ConstraintViolation<A>> violationsA = validator.validate( a );
125-
final Set<ConstraintViolation<B>> violationsB = validator.validate( b );
126-
final Set<ConstraintViolation<C>> violationsC = validator.validate( c );
127-
final Set<ConstraintViolation<D>> violationsD = validator.validate( d );
128-
final Set<ConstraintViolation<E>> violationsE = validator.validate( e );
129-
final Set<ConstraintViolation<F>> violationsF = validator.validate( f );
130-
final Set<ConstraintViolation<G>> violationsG = validator.validate( g );
123+
assertThat( validator.validate( a ) ).isEmpty();
124+
assertThat( validator.validate( b ) ).isEmpty();
125+
assertThat( validator.validate( c ) ).isEmpty();
126+
assertThat( validator.validate( d ) ).isEmpty();
127+
assertThat( validator.validate( e ) ).isEmpty();
128+
assertThat( validator.validate( f ) ).isEmpty();
129+
assertThat( validator.validate( g ) ).isEmpty();
131130
}
132131

133132
private Validator getValidator() {

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

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

7+
import static org.hibernate.validator.testutil.ConstraintViolationAssert.assertThat;
78
import static org.testng.Assert.assertFalse;
89
import static org.testng.Assert.assertTrue;
910

1011
import java.util.Arrays;
1112
import java.util.HashSet;
12-
import java.util.Set;
1313

14-
import jakarta.validation.ConstraintViolation;
1514
import jakarta.validation.Valid;
1615
import jakarta.validation.Validation;
1716
import jakarta.validation.Validator;
@@ -109,13 +108,13 @@ public void testValidate() {
109108
f.g = g;
110109

111110
final Validator validator = getValidator();
112-
final Set<ConstraintViolation<A>> violationsA = validator.validate( a );
113-
final Set<ConstraintViolation<B>> violationsB = validator.validate( b );
114-
final Set<ConstraintViolation<C>> violationsC = validator.validate( c );
115-
final Set<ConstraintViolation<D>> violationsD = validator.validate( d );
116-
final Set<ConstraintViolation<E>> violationsE = validator.validate( e );
117-
final Set<ConstraintViolation<F>> violationsF = validator.validate( f );
118-
final Set<ConstraintViolation<G>> violationsG = validator.validate( g );
111+
assertThat( validator.validate( a ) ).isEmpty();
112+
assertThat( validator.validate( b ) ).isEmpty();
113+
assertThat( validator.validate( c ) ).isEmpty();
114+
assertThat( validator.validate( d ) ).isEmpty();
115+
assertThat( validator.validate( e ) ).isEmpty();
116+
assertThat( validator.validate( f ) ).isEmpty();
117+
assertThat( validator.validate( g ) ).isEmpty();
119118
}
120119

121120
private Validator getValidator() {

‎performance/src/main/jakarta-predefined-scope/org/hibernate/validator/performance/cascaded/PredefinedScopeCascadedValidation.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,8 @@ public static class Person {
8484
@NotNull
8585
String name;
8686

87-
@Valid
88-
Set<Person> friends = new HashSet<>();
87+
88+
Set<@Valid Person> friends = new HashSet<>();
8989

9090
public Person(String name) {
9191
this.name = name;

0 commit comments

Comments
 (0)