Skip to content

Commit 495428d

Browse files
marko-bekhtamarko-bekhta
committed
HV-2151 Change the caching keys in the traversable resolvers
sice the nodes are mutable now... Signed-off-by: marko-bekhta <[email protected]>
1 parent e032943 commit 495428d

File tree

6 files changed

+262
-65
lines changed

6 files changed

+262
-65
lines changed

‎engine/src/main/java/org/hibernate/validator/internal/engine/path/MutableNode.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -258,6 +258,7 @@ public void reset() {
258258
typeArgumentIndex = null;
259259
containerClass = null;
260260
value = null;
261+
asString = null;
261262
}
262263

263264
@Override

‎engine/src/main/java/org/hibernate/validator/internal/engine/resolver/AbstractTraversableHolder.java‎

Lines changed: 0 additions & 54 deletions
This file was deleted.

‎engine/src/main/java/org/hibernate/validator/internal/engine/resolver/CachingJPATraversableResolverForSingleValidation.java‎

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ public boolean isReachable(Object traversableObject, Path.Node traversableProper
3838
return true;
3939
}
4040

41-
return traversables.computeIfAbsent( new TraversableHolder( traversableObject, traversableProperty ), th -> delegate.isReachable(
41+
return traversables.computeIfAbsent( new TraversableHolder( traversableObject, traversableProperty, elementType ), th -> delegate.isReachable(
4242
traversableObject,
4343
traversableProperty,
4444
rootBeanType,
@@ -54,10 +54,50 @@ public boolean isCascadable(Object traversableObject, Path.Node traversablePrope
5454
return true;
5555
}
5656

57-
private static class TraversableHolder extends AbstractTraversableHolder {
57+
private static class TraversableHolder {
5858

59-
private TraversableHolder(Object traversableObject, Path.Node traversableProperty) {
60-
super( traversableObject, traversableProperty );
59+
private final Object traversableObject;
60+
private final String traversableProperty;
61+
private final ElementType elementType;
62+
private final int hashCode;
63+
64+
private TraversableHolder(Object traversableObject, Path.Node traversableProperty, ElementType elementType) {
65+
this.traversableObject = traversableObject;
66+
// nodes and paths are mutable, do not use them for caching:
67+
this.traversableProperty = traversableProperty.getName();
68+
this.elementType = elementType;
69+
this.hashCode = buildHashCode();
70+
}
71+
72+
@Override
73+
public boolean equals(Object o) {
74+
if ( this == o ) {
75+
return true;
76+
}
77+
if ( o == null || !( o instanceof TraversableHolder that ) ) {
78+
return false;
79+
}
80+
if ( traversableObject != null ? ( traversableObject != that.traversableObject ) : that.traversableObject != null ) {
81+
return false;
82+
}
83+
if ( !traversableProperty.equals( that.traversableProperty ) ) {
84+
return false;
85+
}
86+
return elementType == that.elementType;
87+
}
88+
89+
@Override
90+
public int hashCode() {
91+
return hashCode;
92+
}
93+
94+
private int buildHashCode() {
95+
// HV-1013 Using identity hash code in order to avoid calling hashCode() of objects which may
96+
// be handling null properties not correctly
97+
int result = traversableObject != null ? System.identityHashCode( traversableObject ) : 0;
98+
result = 31 * result + traversableProperty.hashCode();
99+
result = 31 * result + elementType.hashCode();
100+
return result;
61101
}
62102
}
63103
}

‎engine/src/main/java/org/hibernate/validator/internal/engine/resolver/CachingTraversableResolverForSingleValidation.java‎

Lines changed: 45 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ public CachingTraversableResolverForSingleValidation(TraversableResolver delegat
3030

3131
@Override
3232
public boolean isReachable(Object traversableObject, Path.Node traversableProperty, Class<?> rootBeanType, Path pathToTraversableObject, ElementType elementType) {
33-
TraversableHolder currentLH = new TraversableHolder( traversableObject, traversableProperty );
33+
TraversableHolder currentLH = new TraversableHolder( traversableObject, traversableProperty, elementType );
3434
TraversableHolder cachedLH = traversables.get( currentLH );
3535
if ( cachedLH == null ) {
3636
currentLH.isReachable = delegate.isReachable(
@@ -57,7 +57,7 @@ else if ( cachedLH.isReachable == null ) {
5757

5858
@Override
5959
public boolean isCascadable(Object traversableObject, Path.Node traversableProperty, Class<?> rootBeanType, Path pathToTraversableObject, ElementType elementType) {
60-
TraversableHolder currentLH = new TraversableHolder( traversableObject, traversableProperty );
60+
TraversableHolder currentLH = new TraversableHolder( traversableObject, traversableProperty, elementType );
6161

6262
TraversableHolder cachedLH = traversables.get( currentLH );
6363
if ( cachedLH == null ) {
@@ -83,12 +83,52 @@ else if ( cachedLH.isCascadable == null ) {
8383
return cachedLH.isCascadable;
8484
}
8585

86-
private static final class TraversableHolder extends AbstractTraversableHolder {
86+
private static final class TraversableHolder {
87+
private final Object traversableObject;
88+
private final String traversableProperty;
89+
private final ElementType elementType;
90+
private final int hashCode;
91+
8792
private Boolean isReachable;
8893
private Boolean isCascadable;
8994

90-
private TraversableHolder(Object traversableObject, Path.Node traversableProperty) {
91-
super( traversableObject, traversableProperty );
95+
private TraversableHolder(Object traversableObject, Path.Node traversableProperty, ElementType elementType) {
96+
this.traversableObject = traversableObject;
97+
// nodes and paths are mutable, do not use them for caching:
98+
this.traversableProperty = traversableProperty.getName();
99+
this.elementType = elementType;
100+
this.hashCode = buildHashCode();
101+
}
102+
103+
@Override
104+
public boolean equals(Object o) {
105+
if ( this == o ) {
106+
return true;
107+
}
108+
if ( o == null || !( o instanceof TraversableHolder that ) ) {
109+
return false;
110+
}
111+
if ( traversableObject != null ? ( traversableObject != that.traversableObject ) : that.traversableObject != null ) {
112+
return false;
113+
}
114+
if ( !traversableProperty.equals( that.traversableProperty ) ) {
115+
return false;
116+
}
117+
return elementType == that.elementType;
118+
}
119+
120+
@Override
121+
public int hashCode() {
122+
return hashCode;
123+
}
124+
125+
private int buildHashCode() {
126+
// HV-1013 Using identity hash code in order to avoid calling hashCode() of objects which may
127+
// be handling null properties not correctly
128+
int result = traversableObject != null ? System.identityHashCode( traversableObject ) : 0;
129+
result = 31 * result + traversableProperty.hashCode();
130+
result = 31 * result + elementType.hashCode();
131+
return result;
92132
}
93133
}
94134
}

‎engine/src/test/java/org/hibernate/validator/test/internal/engine/traversableresolver/CachedTraversableResolverCacheHitsTest.java‎

Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
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.traversableresolver;
6+
7+
import static org.hibernate.validator.testutil.ConstraintViolationAssert.assertThat;
8+
import static org.hibernate.validator.testutil.ConstraintViolationAssert.pathWith;
9+
import static org.hibernate.validator.testutil.ConstraintViolationAssert.violationOf;
10+
import static org.testng.Assert.fail;
11+
12+
import java.lang.annotation.ElementType;
13+
import java.util.HashSet;
14+
import java.util.Set;
15+
16+
import jakarta.validation.Configuration;
17+
import jakarta.validation.Path;
18+
import jakarta.validation.TraversableResolver;
19+
import jakarta.validation.Valid;
20+
import jakarta.validation.Validation;
21+
import jakarta.validation.ValidationException;
22+
import jakarta.validation.Validator;
23+
import jakarta.validation.ValidatorFactory;
24+
import jakarta.validation.constraints.NotNull;
25+
26+
import org.hibernate.validator.testutil.TestForIssue;
27+
28+
import org.testng.annotations.Test;
29+
30+
@TestForIssue(jiraKey = "HV-2151")
31+
public class CachedTraversableResolverCacheHitsTest {
32+
33+
@Test
34+
public void testHits() {
35+
TraversableResolver resolver = new TestResolver( Set.of( "string1", "string2", "bean1" ), Set.of( "bean1" ) );
36+
Configuration<?> config = Validation.byDefaultProvider()
37+
.configure()
38+
.traversableResolver( resolver );
39+
ValidatorFactory factory = config.buildValidatorFactory();
40+
41+
MyBean bean = new MyBean();
42+
bean.bean1 = new MyInnerBean();
43+
bean.bean2 = new MyInnerBean();
44+
45+
Validator v = factory.getValidator();
46+
try {
47+
assertThat( v.validate( bean ) ).containsOnlyViolations(
48+
violationOf( NotNull.class )
49+
.withPropertyPath( pathWith().property( "string1" ) ),
50+
violationOf( NotNull.class )
51+
.withPropertyPath( pathWith().property( "string2" ) ),
52+
violationOf( NotNull.class )
53+
.withPropertyPath( pathWith().property( "bean1" )
54+
.property( "string1" ) ),
55+
violationOf( NotNull.class )
56+
.withPropertyPath( pathWith().property( "bean1" )
57+
.property( "string2" ) )
58+
);
59+
}
60+
catch (ValidationException e) {
61+
fail( "TraversableResolver called several times for a given object", e );
62+
}
63+
}
64+
65+
private static class MyBean {
66+
@NotNull
67+
String string1;
68+
@NotNull
69+
String string2;
70+
@NotNull
71+
String string3;
72+
@NotNull
73+
String string4;
74+
75+
@Valid
76+
MyInnerBean bean1;
77+
@Valid
78+
MyInnerBean bean2;
79+
}
80+
81+
private static class MyInnerBean {
82+
@NotNull
83+
String string1;
84+
@NotNull
85+
String string2;
86+
}
87+
88+
private static class TestResolver implements TraversableResolver {
89+
private final Set<String> reachableProperties;
90+
private final Set<String> cascadableProperties;
91+
private Set<Holder> askedReach = new HashSet<Holder>();
92+
private Set<Holder> askedCascade = new HashSet<Holder>();
93+
94+
private TestResolver(Set<String> reachableProperties, Set<String> cascadableProperties) {
95+
this.reachableProperties = reachableProperties;
96+
this.cascadableProperties = cascadableProperties;
97+
}
98+
99+
private boolean isTraversable(Set<Holder> asked, Object traversableObject, Path.Node traversableProperty) {
100+
Holder h = new Holder( traversableObject, traversableProperty );
101+
if ( asked.contains( h ) ) {
102+
throw new IllegalStateException( "Called twice" );
103+
}
104+
asked.add( h );
105+
return true;
106+
}
107+
108+
@Override
109+
public boolean isReachable(
110+
Object traversableObject,
111+
Path.Node traversableProperty,
112+
Class<?> rootBeanType,
113+
Path pathToTraversableObject,
114+
ElementType elementType
115+
) {
116+
if ( reachableProperties.contains( traversableProperty.getName() ) ) {
117+
return isTraversable(
118+
askedReach,
119+
traversableObject,
120+
traversableProperty
121+
);
122+
}
123+
return false;
124+
}
125+
126+
@Override
127+
public boolean isCascadable(
128+
Object traversableObject,
129+
Path.Node traversableProperty,
130+
Class<?> rootBeanType,
131+
Path pathToTraversableObject,
132+
ElementType elementType
133+
) {
134+
if ( cascadableProperties.contains( traversableProperty.getName() ) ) {
135+
return isTraversable(
136+
askedCascade,
137+
traversableObject,
138+
traversableProperty
139+
);
140+
}
141+
return false;
142+
}
143+
144+
public static class Holder {
145+
Object NULL = new Object();
146+
Object to;
147+
String tp;
148+
149+
public Holder(Object traversableObject, Path.Node traversableProperty) {
150+
to = traversableObject == null ? NULL : traversableObject;
151+
tp = traversableProperty.getName();
152+
}
153+
154+
@Override
155+
public int hashCode() {
156+
return to.hashCode() + tp.hashCode();
157+
}
158+
159+
@Override
160+
public boolean equals(Object obj) {
161+
if ( !( obj instanceof Holder ) ) {
162+
return false;
163+
}
164+
Holder that = (Holder) obj;
165+
166+
return to != NULL && to == that.to && tp.equals( that.tp );
167+
}
168+
}
169+
}
170+
}

‎engine/src/test/java/org/hibernate/validator/test/internal/engine/traversableresolver/CachedTraversableResolverTest.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -215,11 +215,11 @@ public boolean isCascadable(Object traversableObject,
215215
public static class Holder {
216216
Object NULL = new Object();
217217
Object to;
218-
Path.Node tp;
218+
String tp;
219219

220220
public Holder(Object traversableObject, Path.Node traversableProperty) {
221221
to = traversableObject == null ? NULL : traversableObject;
222-
tp = traversableProperty;
222+
tp = traversableProperty.getName();
223223
}
224224

225225
@Override

0 commit comments

Comments
 (0)