Skip to content

Commit

Permalink
HHH-16594 Preserve consistent query parameter processing order
Browse files Browse the repository at this point in the history
  • Loading branch information
mbladel committed May 29, 2023
1 parent 87867b2 commit 89c1937
Show file tree
Hide file tree
Showing 6 changed files with 113 additions and 35 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
* Hibernate, Relational Persistence for Idiomatic Java
*
* License: GNU Lesser General Public License (LGPL), version 2.1 or later
* See the lgpl.txt file in the root directory or http://www.gnu.org/licenses/lgpl-2.1.html
*/
package org.hibernate.internal.util.collections;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.IdentityHashMap;
import java.util.LinkedHashSet;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;

/**
* Utility {@link IdentityHashMap} implementation that maintains predictable iteration order,
* with special care for keys iteration efficiency, and uses reference equality
* (i.e. {@code ==}) in place of object-equality when comparing keys.
* <p>
* Note that the {@link #keySet()}, {@link #values()} and {@link #entrySet()} methods for this class
* return unmodifiable collections and are only meant for iteration.
*
* @author Marco Belladelli
*/
public class LinkedIdentityHashMap<K, V> extends IdentityHashMap<K, V> {
private final ArraySet<K> orderedKeys = new ArraySet<>();

@Override
public V put(K key, V value) {
orderedKeys.add( key );
return super.put( key, value );
}

@Override
public V remove(Object key) {
orderedKeys.remove( key );
return super.remove( key );
}

@Override
public boolean remove(Object key, Object value) {
final boolean removed = super.remove( key, value );
if ( removed ) {
orderedKeys.remove( key );
}
return removed;
}

@Override
public void clear() {
orderedKeys.clear();
super.clear();
}

@Override
public Set<K> keySet() {
return Collections.unmodifiableSet( orderedKeys );
}

@Override
public Collection<V> values() {
return orderedKeys.stream().map( this::get ).collect( Collectors.toUnmodifiableList() );
}

@Override
public Set<Entry<K, V>> entrySet() {
return Collections.unmodifiableSet(
(Set<? extends Entry<K, V>>) orderedKeys.stream()
.map( key -> Map.entry( key, get( key ) ) )
.collect( Collectors.toCollection( LinkedHashSet::new ) )
);
}

@Override
public Object clone() {
throw new UnsupportedOperationException();
}

private static final class ArraySet<E> extends ArrayList<E> implements Set<E> {
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,17 @@
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.IdentityHashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.function.Consumer;
import java.util.function.Predicate;
import jakarta.persistence.Parameter;

import org.hibernate.QueryException;
import org.hibernate.internal.util.StringHelper;
import org.hibernate.internal.util.collections.CollectionHelper;
import org.hibernate.internal.util.collections.LinkedIdentityHashMap;
import org.hibernate.internal.util.compare.ComparableComparator;
import org.hibernate.query.BindableType;
import org.hibernate.query.QueryParameter;
Expand All @@ -29,6 +28,8 @@
import org.hibernate.query.spi.QueryParameterImplementor;
import org.hibernate.query.sqm.tree.expression.SqmParameter;

import jakarta.persistence.Parameter;

/**
* Encapsulates metadata about parameters encountered within a query.
*
Expand Down Expand Up @@ -98,7 +99,7 @@ public ParameterMetadataImpl(
this.labels = Collections.emptySet();
}
else {
this.queryParameters = new IdentityHashMap<>();
this.queryParameters = new LinkedIdentityHashMap<>();
if ( positionalQueryParameters != null ) {
for ( QueryParameterImplementor<?> value : positionalQueryParameters.values() ) {
this.queryParameters.put( value, Collections.emptyList() );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.function.BiConsumer;

Expand Down Expand Up @@ -167,29 +166,30 @@ public void visitBindings(@SuppressWarnings("rawtypes") BiConsumer action) {
}

@Override
public QueryKey.ParameterBindingsMemento generateQueryKeyMemento(SharedSessionContractImplementor persistenceContext) {
final MutableCacheKeyImpl mutableCacheKey = new MutableCacheKeyImpl(parameterBindingMap.size());
public QueryKey.ParameterBindingsMemento generateQueryKeyMemento(SharedSessionContractImplementor session) {
final MutableCacheKeyImpl mutableCacheKey = new MutableCacheKeyImpl( parameterBindingMap.size() );
// We know that parameters are consumed in processing order, this ensures consistency of generated cache keys
parameterMetadata.visitParameters( queryParameter -> {
final QueryParameterBinding<?> binding = parameterBindingMap.get( queryParameter );
assert binding != null : "Found unbound query parameter while generating cache key";

for ( Map.Entry<QueryParameter<?>, QueryParameterBinding<?>> entry : parameterBindingMap.entrySet() ) {
final QueryParameterBinding<?> binding = entry.getValue();
final MappingModelExpressible<?> mappingType = determineMappingType(
binding,
entry.getKey(),
persistenceContext
queryParameter,
session
);

if ( binding.isMultiValued() ) {
for ( Object bindValue : binding.getBindValues() ) {
assert bindValue != null;
mappingType.addToCacheKey( mutableCacheKey, bindValue, persistenceContext );
mappingType.addToCacheKey( mutableCacheKey, bindValue, session );
}
}
else {
final Object bindValue = binding.getBindValue();
mappingType.addToCacheKey( mutableCacheKey, bindValue, persistenceContext );
mappingType.addToCacheKey( mutableCacheKey, bindValue, session );
}
}

} );
// Finally, build the overall cache key
return mutableCacheKey.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import java.util.Map;
import java.util.TreeMap;

import org.hibernate.internal.util.collections.LinkedIdentityHashMap;
import org.hibernate.query.internal.QueryParameterNamedImpl;
import org.hibernate.query.internal.QueryParameterPositionalImpl;
import org.hibernate.query.spi.QueryParameterImplementor;
Expand Down Expand Up @@ -45,7 +46,7 @@ public static DomainParameterXref from(SqmStatement<?> sqmStatement) {
return empty();
}

final Map<QueryParameterImplementor<?>, List<SqmParameter<?>>> sqmParamsByQueryParam = new IdentityHashMap<>();
final Map<QueryParameterImplementor<?>, List<SqmParameter<?>>> sqmParamsByQueryParam = new LinkedIdentityHashMap<>();

final int sqmParamCount = parameterResolutions.getSqmParameters().size();
final Map<SqmParameter<?>, QueryParameterImplementor<?>> queryParamBySqmParam = new IdentityHashMap<>( sqmParamCount );
Expand Down Expand Up @@ -166,14 +167,6 @@ public int getNumberOfSqmParameters(QueryParameterImplementor<?> queryParameter)
return sqmParameters.size();
}

/**
* Get the mapping of all QueryParameters to the List of its corresponding
* SqmParameters
*/
public Map<QueryParameterImplementor<?>, List<SqmParameter<?>>> getSqmParamByQueryParam() {
return sqmParamsByQueryParam;
}

public SqmStatement.ParameterResolutions getParameterResolutions() {
return parameterResolutions;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.IdentityHashMap;
import java.util.Iterator;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
Expand All @@ -31,12 +31,8 @@
import org.hibernate.metamodel.mapping.JdbcMapping;
import org.hibernate.metamodel.mapping.MappingModelExpressible;
import org.hibernate.metamodel.mapping.PluralAttributeMapping;
import org.hibernate.sql.exec.spi.JdbcParametersList;
import org.hibernate.type.JavaObjectType;
import org.hibernate.type.descriptor.converter.spi.BasicValueConverter;
import org.hibernate.query.IllegalQueryOperationException;
import org.hibernate.query.IllegalSelectQueryException;
import org.hibernate.spi.NavigablePath;
import org.hibernate.query.spi.QueryParameterBinding;
import org.hibernate.query.spi.QueryParameterBindings;
import org.hibernate.query.spi.QueryParameterImplementor;
Expand All @@ -50,12 +46,16 @@
import org.hibernate.query.sqm.tree.expression.SqmParameter;
import org.hibernate.query.sqm.tree.jpa.ParameterCollector;
import org.hibernate.query.sqm.tree.select.SqmSelectStatement;
import org.hibernate.spi.NavigablePath;
import org.hibernate.sql.ast.SqlTreeCreationException;
import org.hibernate.sql.ast.tree.expression.JdbcParameter;
import org.hibernate.sql.ast.tree.from.TableGroup;
import org.hibernate.sql.exec.internal.JdbcParameterBindingImpl;
import org.hibernate.sql.exec.internal.JdbcParameterBindingsImpl;
import org.hibernate.sql.exec.spi.JdbcParameterBindings;
import org.hibernate.sql.exec.spi.JdbcParametersList;
import org.hibernate.type.JavaObjectType;
import org.hibernate.type.descriptor.converter.spi.BasicValueConverter;
import org.hibernate.type.spi.TypeConfiguration;

/**
Expand Down Expand Up @@ -118,7 +118,7 @@ public static Map<QueryParameterImplementor<?>, Map<SqmParameter<?>, List<JdbcPa
final int queryParameterCount = domainParameterXref.getQueryParameterCount();
final Map<QueryParameterImplementor<?>, Map<SqmParameter<?>, List<JdbcParametersList>>> result = new IdentityHashMap<>( queryParameterCount );

for ( Map.Entry<QueryParameterImplementor<?>, List<SqmParameter<?>>> entry : domainParameterXref.getSqmParamByQueryParam().entrySet() ) {
for ( Map.Entry<QueryParameterImplementor<?>, List<SqmParameter<?>>> entry : domainParameterXref.getQueryParameters().entrySet() ) {
final QueryParameterImplementor<?> queryParam = entry.getKey();
final List<SqmParameter<?>> sqmParams = entry.getValue();

Expand Down Expand Up @@ -211,7 +211,7 @@ public static JdbcParameterBindings createJdbcParameterBindings(
);

for ( Map.Entry<QueryParameterImplementor<?>, List<SqmParameter<?>>> entry :
domainParameterXref.getSqmParamByQueryParam().entrySet() ) {
domainParameterXref.getQueryParameters().entrySet() ) {
final QueryParameterImplementor<?> queryParam = entry.getKey();
final List<SqmParameter<?>> sqmParameters = entry.getValue();

Expand Down Expand Up @@ -480,7 +480,7 @@ private static class CriteriaParameterCollector {

public void process(SqmParameter<?> parameter) {
if ( sqmParameters == null ) {
sqmParameters = new HashSet<>();
sqmParameters = new LinkedHashSet<>();
}

if ( parameter instanceof SqmJpaCriteriaParameterWrapper<?> ) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
package org.hibernate.query.sqm.tree.select;

import java.util.Collections;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand Down Expand Up @@ -113,7 +113,7 @@ public SqmSelectStatement<T> copy(SqmCopyContext context) {
parameters = null;
}
else {
parameters = new HashSet<>( this.parameters.size() );
parameters = new LinkedHashSet<>( this.parameters.size() );
for ( SqmParameter<?> parameter : this.parameters ) {
parameters.add( parameter.copy( context ) );
}
Expand Down Expand Up @@ -225,7 +225,7 @@ public <X> X accept(SemanticQueryWalker<X> walker) {
@Override
public void addParameter(SqmParameter<?> parameter) {
if ( parameters == null ) {
parameters = new HashSet<>();
parameters = new LinkedHashSet<>();
}

parameters.add( parameter );
Expand Down

0 comments on commit 89c1937

Please sign in to comment.