Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a PathBuilder #850

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
f970b29
HV-1480 Add a hv-6.0 entry to be able to compare the latest stable with
gsmet Sep 5, 2017
27f566a
HV-1480 Add a benchmark validating a bean containing a lot of beans to
gsmet Sep 5, 2017
d3672d3
HV-1480 Avoid removing and adding element to the node list
gsmet Sep 5, 2017
e3b444e
HV-1480 Copy the hashCode in the copy constructor
gsmet Sep 5, 2017
5398159
HV-1480 Avoid initializing lists and maps in the common case where we
gsmet Sep 5, 2017
780af71
HV-1480 Avoid creating a list in the common case when we only have the
gsmet Sep 5, 2017
e76f701
HV-1480 Avoid computing the hashCode if not necessary
gsmet Sep 5, 2017
c11fdeb
HV-1480 Don't take into account the parent in hashCode and equals
gsmet Sep 5, 2017
ed8f3b5
HV-1480 Implement a copy on write strategy for the node list
gsmet Sep 5, 2017
7c00b44
HV-1480 Centralize the processed works in one single set
gsmet Sep 5, 2017
f39e179
HV-1480 Setting the key or the index already makes the node iterable
gsmet Sep 5, 2017
2e66bec
HV-1480 We expect at least one node in the path so let's initialize the
gsmet Sep 5, 2017
16c19e5
HV-1480 Even if not strictly necessary, the leaf node should be
gsmet Sep 5, 2017
1acf669
HV-1480 Reduce the number of iterations in the other benchmarks
gsmet Sep 5, 2017
324b56e
HV-1480 Add the new benchmark to the default benchmarks
gsmet Sep 5, 2017
b82169c
HV-1480 Some more optimizations suggested by Sanne
gsmet Sep 6, 2017
d2ea4b8
HV-1480 Add another benchmark
gsmet Sep 7, 2017
0e0d825
Mark a test as candidate for the TCK
gsmet Sep 7, 2017
3b79cdd
HV-1480 Only set the property value if required
gsmet Sep 8, 2017
bf7b5c0
HV-1480 Getting reference to parent node in a simpler way
gunnarmorling Sep 18, 2017
da77077
WIP
gunnarmorling Sep 18, 2017
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
*/
package org.hibernate.validator.internal.engine;

import static org.hibernate.validator.internal.util.CollectionHelper.newHashMap;
import static org.hibernate.validator.internal.util.CollectionHelper.newHashSet;

import java.lang.reflect.Executable;
Expand All @@ -31,12 +30,12 @@
import org.hibernate.validator.internal.engine.constraintvalidation.ConstraintValidatorContextImpl;
import org.hibernate.validator.internal.engine.constraintvalidation.ConstraintValidatorManager;
import org.hibernate.validator.internal.engine.constraintvalidation.ConstraintViolationCreationContext;
import org.hibernate.validator.internal.engine.path.PathBuilder;
import org.hibernate.validator.internal.engine.path.PathImpl;
import org.hibernate.validator.internal.metadata.BeanMetaDataManager;
import org.hibernate.validator.internal.metadata.aggregated.BeanMetaData;
import org.hibernate.validator.internal.metadata.core.MetaConstraint;
import org.hibernate.validator.internal.util.ExecutableParameterNameProvider;
import org.hibernate.validator.internal.util.IdentitySet;
import org.hibernate.validator.internal.util.logging.Log;
import org.hibernate.validator.internal.util.logging.LoggerFactory;

Expand Down Expand Up @@ -91,21 +90,15 @@ public class ValidationContext<T> {
private final Object executableReturnValue;

/**
* Maps a group to an identity set to keep track of already validated objects. We have to make sure
* that each object gets only validated once per group and property path.
* The set of already processed unit of works. See {@link ProcessedUnit}.
*/
private final Map<Class<?>, IdentitySet> processedBeansPerGroup;
private final Set<Object> processedUnits;

/**
* Maps an object to a list of paths in which it has been validated. The objects are the bean instances.
*/
private final Map<Object, Set<PathImpl>> processedPathsPerBean;

/**
* Maps processed constraints to the bean and path for which they have been processed.
*/
private final Map<BeanAndPath, IdentitySet> processedMetaConstraints;

/**
* Contains all failing constraints so far.
*/
Expand Down Expand Up @@ -174,9 +167,8 @@ private ValidationContext(ConstraintValidatorManager constraintValidatorManager,
this.executableParameters = executableParameters;
this.executableReturnValue = executableReturnValue;

this.processedBeansPerGroup = newHashMap();
this.processedUnits = new HashSet<>();
this.processedPathsPerBean = new IdentityHashMap<>();
this.processedMetaConstraints = newHashMap();
this.failingConstraintViolations = newHashSet();
}

Expand Down Expand Up @@ -294,7 +286,7 @@ public ConstraintViolation<T> createConstraintViolation(ValueContext<?, ?> local
constraintViolationCreationContext.getExpressionVariables()
);
// at this point we make a copy of the path to avoid side effects
Path path = PathImpl.createCopy( constraintViolationCreationContext.getPath() );
Path path = constraintViolationCreationContext.getPath().build();
Object dynamicPayload = constraintViolationCreationContext.getDynamicPayload();
if ( executableParameters != null ) {
return ConstraintViolationImpl.forParameterValidation(
Expand Down Expand Up @@ -349,22 +341,11 @@ else if ( executableReturnValue != null ) {
}

public boolean hasMetaConstraintBeenProcessed(Object bean, Path path, MetaConstraint<?> metaConstraint) {
// TODO switch to proper multi key map (HF)
IdentitySet processedConstraints = processedMetaConstraints.get( new BeanAndPath( bean, path ) );
return processedConstraints != null && processedConstraints.contains( metaConstraint );
return processedUnits.contains( new BeanPathMetaConstraintProcessedUnit( bean, path, metaConstraint ) );
}

public void markConstraintProcessed(Object bean, Path path, MetaConstraint<?> metaConstraint) {
// TODO switch to proper multi key map (HF)
BeanAndPath beanAndPath = new BeanAndPath( bean, path );
if ( processedMetaConstraints.containsKey( beanAndPath ) ) {
processedMetaConstraints.get( beanAndPath ).add( metaConstraint );
}
else {
IdentitySet set = new IdentitySet();
set.add( metaConstraint );
processedMetaConstraints.put( beanAndPath, set );
}
processedUnits.add( new BeanPathMetaConstraintProcessedUnit( bean, path, metaConstraint ) );
}

public String getValidatedProperty() {
Expand Down Expand Up @@ -443,33 +424,17 @@ private boolean isSubPathOf(Path p1, Path p2) {
}

private boolean isAlreadyValidatedForCurrentGroup(Object value, Class<?> group) {
IdentitySet objectsProcessedInCurrentGroups = processedBeansPerGroup.get( group );
return objectsProcessedInCurrentGroups != null && objectsProcessedInCurrentGroups.contains( value );
return processedUnits.contains( new BeanGroupProcessedUnit( value, group ) );
}

private void markCurrentBeanAsProcessedForCurrentPath(Object value, PathImpl path) {
private void markCurrentBeanAsProcessedForCurrentPath(Object bean, PathBuilder path) {
// HV-1031 The path object is mutated as we traverse the object tree, hence copy it before saving it
path = PathImpl.createCopy( path );

if ( processedPathsPerBean.containsKey( value ) ) {
processedPathsPerBean.get( value ).add( path );
}
else {
Set<PathImpl> set = new HashSet<>();
set.add( path );
processedPathsPerBean.put( value, set );
}
processedPathsPerBean.computeIfAbsent( bean, b -> new HashSet<>() )
.add( path.build() );
}

private void markCurrentBeanAsProcessedForCurrentGroup(Object value, Class<?> group) {
if ( processedBeansPerGroup.containsKey( group ) ) {
processedBeansPerGroup.get( group ).add( value );
}
else {
IdentitySet set = new IdentitySet();
set.add( value );
processedBeansPerGroup.put( group, set );
}
private void markCurrentBeanAsProcessedForCurrentGroup(Object bean, Class<?> group) {
processedUnits.add( new BeanGroupProcessedUnit( bean, group ) );
}

/**
Expand Down Expand Up @@ -609,15 +574,64 @@ public <T> ValidationContext<T> forValidateReturnValue(
}
}

private static final class BeanAndPath {
private final Object bean;
private final Path path;
private final int hashCode;
private static final class BeanGroupProcessedUnit {

private BeanAndPath(Object bean, Path path) {
// these fields are final but we don't mark them as final as an optimization
private Object bean;
private Class<?> group;
private int hashCode;

private BeanGroupProcessedUnit(Object bean, Class<?> group) {
this.bean = bean;
this.group = group;
this.hashCode = createHashCode();
}

@Override
public boolean equals(Object o) {
if ( this == o ) {
return true;
}
if ( o == null || getClass() != BeanGroupProcessedUnit.class ) {
return false;
}

BeanGroupProcessedUnit that = (BeanGroupProcessedUnit) o;

if ( bean != that.bean ) { // instance equality
return false;
}
if ( !group.equals( that.group ) ) {
return false;
}

return true;
}

@Override
public int hashCode() {
return hashCode;
}

private int createHashCode() {
int result = System.identityHashCode( bean );
result = 31 * result + group.hashCode();
return result;
}
}

private static final class BeanPathMetaConstraintProcessedUnit {

// these fields are final but we don't mark them as final as an optimization
private Object bean;
private Path path;
private MetaConstraint<?> metaConstraint;
private int hashCode;

private BeanPathMetaConstraintProcessedUnit(Object bean, Path path, MetaConstraint<?> metaConstraint) {
this.bean = bean;
this.path = path;
// pre-calculate hash code, the class is immutable and hashCode is needed often
this.metaConstraint = metaConstraint;
this.hashCode = createHashCode();
}

Expand All @@ -626,18 +640,21 @@ public boolean equals(Object o) {
if ( this == o ) {
return true;
}
if ( o == null || getClass() != o.getClass() ) {
if ( o == null || getClass() != BeanPathMetaConstraintProcessedUnit.class ) {
return false;
}

BeanAndPath that = (BeanAndPath) o;
BeanPathMetaConstraintProcessedUnit that = (BeanPathMetaConstraintProcessedUnit) o;

if ( bean != that.bean ) { // instance equality
return false;
}
if ( !path.equals( that.path ) ) {
return false;
}
if ( metaConstraint != that.metaConstraint ) {
return false;
}

return true;
}
Expand All @@ -650,6 +667,7 @@ public int hashCode() {
private int createHashCode() {
int result = System.identityHashCode( bean );
result = 31 * result + path.hashCode();
result = 31 * result + System.identityHashCode( metaConstraint );
return result;
}
}
Expand Down
Loading