Skip to content

Commit e859ea7

Browse files
gbadnergbadner
committed
HV-1831 : Experiment detecting cycles in bean classes
1 parent f4d7656 commit e859ea7

File tree

4 files changed

+434
-6
lines changed

4 files changed

+434
-6
lines changed

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

Lines changed: 153 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,16 @@
77
package org.hibernate.validator.internal.engine.tracking;
88

99
import java.lang.reflect.Executable;
10+
import java.util.Collections;
1011
import java.util.HashMap;
12+
import java.util.HashSet;
1113
import java.util.Map;
14+
import java.util.Set;
1215

1316
import org.hibernate.validator.internal.metadata.PredefinedScopeBeanMetaDataManager;
17+
import org.hibernate.validator.internal.metadata.aggregated.BeanMetaData;
18+
import org.hibernate.validator.internal.metadata.aggregated.CascadingMetaData;
19+
import org.hibernate.validator.internal.metadata.facets.Cascadable;
1420
import org.hibernate.validator.internal.util.CollectionHelper;
1521

1622
public class PredefinedScopeProcessedBeansTrackingStrategy implements ProcessedBeansTrackingStrategy {
@@ -29,18 +35,163 @@ public PredefinedScopeProcessedBeansTrackingStrategy(PredefinedScopeBeanMetaData
2935
// In the predefined scope case, we will have the whole hierarchy of constrained classes passed to
3036
// PredefinedScopeBeanMetaDataManager.
3137

32-
this.trackingEnabledForBeans = CollectionHelper.toImmutableMap( new HashMap<>() );
38+
this.trackingEnabledForBeans = CollectionHelper.toImmutableMap(
39+
new TrackingEnabledStrategyBuilder( beanMetaDataManager ).build()
40+
);
3341
this.trackingEnabledForReturnValues = CollectionHelper.toImmutableMap( new HashMap<>() );
3442
this.trackingEnabledForParameters = CollectionHelper.toImmutableMap( new HashMap<>() );
3543
}
3644

45+
private static class TrackingEnabledStrategyBuilder {
46+
private final PredefinedScopeBeanMetaDataManager beanMetaDataManager;
47+
private final Map<Class<?>, Boolean> classToBeanTrackingEnabled;
48+
49+
TrackingEnabledStrategyBuilder(PredefinedScopeBeanMetaDataManager beanMetaDataManager) {
50+
this.beanMetaDataManager = beanMetaDataManager;
51+
this.classToBeanTrackingEnabled = new HashMap<>( beanMetaDataManager.getBeanMetaData().size() );
52+
}
53+
54+
public Map<Class<?>, Boolean> build() {
55+
final Set<Class<?>> beanClassesInPath = new HashSet<>();
56+
for (BeanMetaData<?> beanMetadata : beanMetaDataManager.getBeanMetaData() ) {
57+
determineTrackingRequired( beanMetadata.getBeanClass(), beanClassesInPath );
58+
if ( !beanClassesInPath.isEmpty() ) {
59+
throw new IllegalStateException( "beanClassesInPath not empty" );
60+
}
61+
}
62+
return classToBeanTrackingEnabled;
63+
}
64+
65+
// Do a depth-first search for cycles along paths of cascaded bean classes.
66+
// The algorithm stops due to one of the following:
67+
// 1) The bean class was previously put in classToBeanTrackingEnabled
68+
// (i.e., the bean class was already determined to either have a cycle,
69+
// or not have a cycle).
70+
// 2) A cycle is found. In this case, all bean classes in the particular path,
71+
// starting from beanClass up to first bean class that causes a cycle, will
72+
// be registered in classToBeanTrackingEnabled with a value of true.
73+
// Once a cycle is found, no further bean classes are examined. Those bean
74+
// classes that were examined in the process that are found to not have a
75+
// cycle are registered in classToBeanTrackingEnabled with a value of false.
76+
// 3) No cycle is found. In this case, all bean classes in the tree will be
77+
// registered in classToBeanTrackingEnabled with a value of false.
78+
//
79+
// Examples: An arrow, ->, indicates a cascading constraint from a bean class.
80+
//
81+
// 1) A -> B
82+
// | ^
83+
// | |
84+
// ----
85+
// A, B have no cycles. A has 2 paths to B, but there are no cycles, because there is no path from B to A.
86+
//
87+
// 2) A <-
88+
// | |
89+
// ---
90+
// A has a cycle to itself.
91+
//
92+
// 3) A -> B -> C -> D
93+
// ^ |
94+
// | |
95+
// -----
96+
// A, B, C have cycles; D does not have a cycle.
97+
//
98+
private boolean determineTrackingRequired(Class<?> beanClass, Set<Class<?>> beanClassesInPath) {
99+
100+
final Boolean isBeanTrackingEnabled = classToBeanTrackingEnabled.get( beanClass );
101+
if ( isBeanTrackingEnabled != null ) {
102+
// It was already determined for beanClass.
103+
return isBeanTrackingEnabled;
104+
}
105+
106+
// Add beanClass to the path.
107+
// We don't care about the order of the bean classes in
108+
// beanClassesInPath. We only care about detecting a duplicate,
109+
// which indicates a cycle. If no cycle is found in beanClass,
110+
// it will be removed below.
111+
if ( !beanClassesInPath.add( beanClass ) ) {
112+
// The bean was already present in the path being examined.
113+
// That means that there is cycle involving beanClass.
114+
// Enable tracking for all elements in beanClassesInPath
115+
for ( Class<?> dependency : beanClassesInPath ) {
116+
register( dependency, true );
117+
}
118+
beanClassesInPath.clear();
119+
return true;
120+
}
121+
122+
// Now check the cascaded bean classes.
123+
for ( Class<?> directCascadedBeanClass : getDirectCascadedBeanClasses( beanClass ) ) {
124+
// Check to see if tracking has already been determined for directCascadedBeanClass
125+
Boolean isSubBeanTrackingEnabled = classToBeanTrackingEnabled.get( directCascadedBeanClass );
126+
if ( isSubBeanTrackingEnabled != null ) {
127+
if ( isSubBeanTrackingEnabled ) {
128+
// We already know that directCascadedBeanClass has a cycle.
129+
// That means that all elements in beanClassesInPath
130+
// will have a cycle.
131+
for ( Class<?> dependency : beanClassesInPath ) {
132+
register( dependency, true );
133+
}
134+
// No point in checking any others in this loop.
135+
beanClassesInPath.clear();
136+
return true;
137+
}
138+
else {
139+
// We already know that directCascadedBeanClass is not involved in
140+
// any cycles, so move on to the next iteration.
141+
continue;
142+
}
143+
}
144+
if ( determineTrackingRequired( directCascadedBeanClass, beanClassesInPath ) ) {
145+
// A cycle was found. No point in checking any others in this loop.
146+
// beanClassesInPath should have already been cleared.
147+
assert beanClassesInPath.isEmpty();
148+
return true;
149+
}
150+
// directCascadedBeanClass does not have a cycle.
151+
// directCascadedBeanClass would have already been removed by the
152+
// call to #determineTrackingRequired above
153+
}
154+
beanClassesInPath.remove( beanClass );
155+
return register( beanClass, false );
156+
}
157+
158+
// TODO: is there a more concise way to do this?
159+
private <T> Set<Class<?>> getDirectCascadedBeanClasses(Class<T> beanClass ) {
160+
final BeanMetaData<T> beanMetaData = beanMetaDataManager.getBeanMetaData( beanClass );
161+
if ( beanMetaData.hasCascadables() ) {
162+
final Set<Class<?>> directCascadedBeanClasses = new HashSet<>();
163+
for ( Cascadable cascadable : beanMetaData.getCascadables() ) {
164+
final CascadingMetaData cascadingMetaData = cascadable.getCascadingMetaData();
165+
if ( cascadingMetaData.isContainer() ) {
166+
throw new UnsupportedOperationException( "Containers are not supported yet." );
167+
}
168+
else {
169+
// TODO: For now, assume non-container Cascadables are always beans. Truee???
170+
directCascadedBeanClasses.add( (Class<?>) cascadable.getCascadableType() );
171+
}
172+
}
173+
return directCascadedBeanClasses;
174+
}
175+
else {
176+
return Collections.emptySet();
177+
}
178+
}
179+
180+
private boolean register(Class<?> beanClass, boolean isBeanTrackingEnabled) {
181+
if ( classToBeanTrackingEnabled.put( beanClass, isBeanTrackingEnabled ) != null ) {
182+
throw new IllegalStateException( beanClass.getName() + " registered more than once." );
183+
}
184+
return isBeanTrackingEnabled;
185+
}
186+
}
187+
37188
@Override
38189
public boolean isEnabledForBean(Class<?> rootBeanClass, boolean hasCascadables) {
39190
if ( !hasCascadables ) {
40191
return false;
41192
}
42193

43-
return trackingEnabledForBeans.getOrDefault( rootBeanClass, true );
194+
return trackingEnabledForBeans.get( rootBeanClass );
44195
}
45196

46197
@Override

‎engine/src/main/java/org/hibernate/validator/internal/engine/validationcontext/ParameterExecutableValidationContext.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ private static boolean buildProcessedBeansTrackingEnabled(ProcessedBeansTracking
9595
return false;
9696
}
9797

98-
return processedBeansTrackingStrategy.isEnabledForReturnValue( executable,
98+
return processedBeansTrackingStrategy.isEnabledForParameters( executable,
9999
executableMetaData.get().getValidatableParametersMetaData().hasCascadables() );
100100
}
101101

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

Lines changed: 73 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,25 @@
66
*/
77
package org.hibernate.validator.test.internal.engine.tracking;
88

9+
import java.util.Arrays;
10+
import java.util.Collections;
11+
import java.util.HashSet;
12+
import java.util.Set;
13+
import javax.validation.ConstraintViolation;
914
import javax.validation.Valid;
15+
import javax.validation.Validation;
1016
import javax.validation.Validator;
17+
import javax.validation.ValidatorFactory;
18+
import javax.validation.constraints.Email;
1119
import javax.validation.constraints.NotNull;
1220

21+
import org.hibernate.validator.PredefinedScopeHibernateValidator;
22+
import org.hibernate.validator.test.predefinedscope.LocaleResolverTest;
1323
import org.hibernate.validator.testutils.ValidatorUtil;
1424
import org.testng.annotations.Test;
1525

26+
import static org.testng.Assert.assertTrue;
27+
1628
/**
1729
* This is not a real test, just an illustration.
1830
* <p>
@@ -23,14 +35,56 @@
2335
public class ProcessedBeansTrackingCycles1Test {
2436

2537
@Test
26-
public void testSerializeHibernateEmail() throws Exception {
27-
Validator validator = ValidatorUtil.getValidator();
38+
public void testValidNull() throws Exception {
39+
final Parent parent = new Parent( "parent property" );
40+
Set<ConstraintViolation<Parent>> violations = getValidator().validate( parent );
41+
assertTrue( violations.isEmpty() );
42+
}
43+
44+
@Test
45+
public void testValidNotNull() throws Exception {
46+
final Parent parent = new Parent( "parent property" );
47+
parent.child = new Child( "child property" );
48+
49+
Set<ConstraintViolation<Parent>> violations = getValidator().validate( parent );
50+
//Set<ConstraintViolation<Parent>> violations = ValidatorUtil.getValidator().validate( parent );
51+
assertTrue( violations.isEmpty() );
52+
}
53+
54+
@Test
55+
public void testValidNotNullNonCyclic() throws Exception {
56+
final Parent parent = new Parent( "parent property" );
57+
parent.child = new Child( "child property" );
58+
parent.child.parent = new Parent( "other parent property" );
59+
60+
Set<ConstraintViolation<Parent>> violations = getValidator().validate( parent );
61+
assertTrue( violations.isEmpty() );
62+
}
63+
64+
@Test
65+
public void testValidNotNullCyclic() throws Exception {
66+
final Parent parent = new Parent( "parent property" );
67+
parent.child = new Child( "child property" );
68+
parent.child.parent = parent;
2869

29-
validator.validate( new Parent() );
70+
Set<ConstraintViolation<Parent>> violations = getValidator().validate( parent );
71+
assertTrue( violations.isEmpty() );
3072
}
3173

74+
private Validator getValidator() {
75+
return Validation.byProvider( PredefinedScopeHibernateValidator.class )
76+
.configure()
77+
.builtinConstraints( new HashSet<>( Arrays.asList( NotNull.class.getName() ) ) )
78+
.initializeBeanMetaData( new HashSet<>( Arrays.asList( Parent.class, Child.class, Other.class) ) )
79+
.buildValidatorFactory()
80+
.getValidator();
81+
}
3282
private static class Parent {
3383

84+
Parent(String property) {
85+
this.property = property;
86+
}
87+
3488
@NotNull
3589
private String property;
3690

@@ -40,10 +94,26 @@ private static class Parent {
4094

4195
private static class Child {
4296

97+
Child(String property) {
98+
this.property = property;
99+
}
100+
43101
@NotNull
44102
private String property;
45103

46104
@Valid
47105
private Parent parent;
106+
107+
@Valid
108+
private Other other;
109+
}
110+
111+
private static class Other {
112+
Other(String property) {
113+
this.property = property;
114+
}
115+
116+
@NotNull
117+
private String property;
48118
}
49119
}

0 commit comments

Comments
 (0)