HHH-2003 - Collections which are fetched AND restricted should not be written to second-level cache

hibernate-core/src/main/java/org/hibernate/cache/spi/support/AbstractCachedDomainDataAccess.java

@@ -6,6 +6,7 @@
  */
 package org.hibernate.cache.spi.support;
 
+import org.hibernate.Internal;
 import org.hibernate.cache.spi.DomainDataRegion;
 import org.hibernate.cache.spi.access.CachedDomainDataAccess;
 import org.hibernate.cache.spi.access.SoftLock;
@@ -34,7 +35,8 @@ public abstract class AbstractCachedDomainDataAccess implements CachedDomainData
 		return region;
 	}
 
-	protected DomainDataStorageAccess getStorageAccess() {
+	@Internal
+	public DomainDataStorageAccess getStorageAccess() {
 		return storageAccess;
 	}
 

hibernate-core/src/test/java/org/hibernate/orm/test/querycache/QueryRestrictedCollectionCachingTests.java

@@ -0,0 +1,176 @@
+/*
+ * 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.orm.test.querycache;
+
+import java.io.Serializable;
+
+import org.hibernate.CacheMode;
+import org.hibernate.cache.internal.BasicCacheKeyImplementation;
+import org.hibernate.cache.spi.CacheImplementor;
+import org.hibernate.cache.spi.access.CollectionDataAccess;
+import org.hibernate.cache.spi.entry.CollectionCacheEntry;
+import org.hibernate.cache.spi.support.AbstractReadWriteAccess;
+import org.hibernate.cache.spi.support.CollectionReadWriteAccess;
+import org.hibernate.cache.spi.support.DomainDataStorageAccess;
+import org.hibernate.cfg.AvailableSettings;
+import org.hibernate.metamodel.model.domain.NavigableRole;
+
+import org.hibernate.testing.cache.MapStorageAccessImpl;
+import org.hibernate.testing.orm.domain.StandardDomainModel;
+import org.hibernate.testing.orm.domain.library.Book;
+import org.hibernate.testing.orm.domain.library.Person;
+import org.hibernate.testing.orm.junit.DomainModel;
+import org.hibernate.testing.orm.junit.FailureExpected;
+import org.hibernate.testing.orm.junit.Jira;
+import org.hibernate.testing.orm.junit.ServiceRegistry;
+import org.hibernate.testing.orm.junit.SessionFactory;
+import org.hibernate.testing.orm.junit.SessionFactoryScope;
+import org.hibernate.testing.orm.junit.Setting;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import jakarta.persistence.CacheStoreMode;
+import jakarta.persistence.SharedCacheMode;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.fail;
+
+/**
+ * Assertions that collections which are join fetched and restricted in a query do not get put into the
+ * second level cache with the filtered state
+ *
+ * @author Steve Ebersole
+ */
+@SuppressWarnings("JUnitMalformedDeclaration")
+@Jira( "https://hibernate.atlassian.net/browse/HHH-2003" )
+@ServiceRegistry(settings = {
+		@Setting( name= AvailableSettings.USE_SECOND_LEVEL_CACHE, value = "true"),
+		@Setting( name= AvailableSettings.USE_QUERY_CACHE, value = "true"),
+})
+@DomainModel(standardModels = StandardDomainModel.LIBRARY, sharedCacheMode = SharedCacheMode.ALL)
+@SessionFactory
+public class QueryRestrictedCollectionCachingTests {
+	public static final String AUTHORS_ROLE = Book.class.getName() + ".authors";
+
+	@Test
+	void testSimpleFetch(SessionFactoryScope sessions) {
+		final CacheImplementor cache = sessions.getSessionFactory().getCache();
+		cache.evictAllRegions();
+
+		sessions.inTransaction( (session) -> {
+			final Book book = session.createSelectionQuery( "from Book b left join fetch b.authors a", Book.class ).getSingleResult();
+			assertThat( book ).isNotNull();
+			assertThat( book.getAuthors() ).hasSize( 2 );
+		} );
+
+		assertThat( cache.containsCollection( AUTHORS_ROLE, 1 ) ).isTrue();
+		assertThat( extractCachedCollectionKeys( cache, AUTHORS_ROLE, 1 ) ).hasSize( 2 );
+	}
+
+	@Test
+	void testSimpleFetch2(SessionFactoryScope sessions) {
+		final CacheImplementor cache = sessions.getSessionFactory().getCache();
+		cache.evictAllRegions();
+
+		sessions.inTransaction( (session) -> {
+			final Book book = session.createSelectionQuery(
+					"from Book b left join fetch b.authors a where b.id = 1",
+					Book.class
+			).getSingleResult();
+			assertThat( book ).isNotNull();
+			assertThat( book.getAuthors() ).hasSize( 2 );
+		} );
+
+		assertThat( cache.containsCollection( AUTHORS_ROLE, 1 ) ).isTrue();
+		assertThat( extractCachedCollectionKeys( cache, AUTHORS_ROLE, 1 ) ).hasSize( 2 );
+	}
+
+	@Test
+	void testRestrictedFetchWithCacheIgnored(SessionFactoryScope sessions) {
+		final CacheImplementor cache = sessions.getSessionFactory().getCache();
+		cache.evictAllRegions();
+
+		sessions.inTransaction( (session) -> {
+			final Book book = session
+					.createSelectionQuery( "from Book b left join fetch b.authors a where a.id = 1", Book.class )
+					.setCacheMode( CacheMode.IGNORE )
+					.getSingleResult();
+			assertThat( book ).isNotNull();
+			assertThat( book.getAuthors() ).hasSize( 1 );
+		} );
+
+		// we ignored the cache explicitly
+		assertThat( cache.containsCollection( AUTHORS_ROLE, 1 ) ).isFalse();
+	}
+
+	@Test
+	@FailureExpected
+	void testRestrictedFetch(SessionFactoryScope sessions) {
+		final CacheImplementor cache = sessions.getSessionFactory().getCache();
+		cache.evictAllRegions();
+
+		sessions.inTransaction( (session) -> {
+			final Book book = session
+					.createSelectionQuery( "from Book b left join fetch b.authors a where a.id = 1", Book.class )
+					.getSingleResult();
+			assertThat( book ).isNotNull();
+			assertThat( book.getAuthors() ).hasSize( 1 );
+		} );
+
+		// This is the crux of HHH-2003.
+		// At the moment we put the filtered collection into the cache
+		assertThat( cache.containsCollection( AUTHORS_ROLE, 1 ) ).isTrue();
+		// this is just some deeper checks to show that the data is "corrupt"
+		assertThat( extractCachedCollectionKeys( cache, AUTHORS_ROLE, 1 ) ).hasSize( 1 );
+
+		fail( "Really, HHH-2003 the collection to not be cached here" );
+	}
+
+	private static Serializable[] extractCachedCollectionKeys(CacheImplementor cache, String role, Integer ownerKey) {
+		final NavigableRole navigableRole = new NavigableRole( role );
+		final CollectionReadWriteAccess authorsRegionAccess = (CollectionReadWriteAccess) cache.getCollectionRegionAccess( navigableRole );
+
+		final MapStorageAccessImpl storageAccess = (MapStorageAccessImpl) authorsRegionAccess.getStorageAccess();
+		final BasicCacheKeyImplementation cacheKey = new BasicCacheKeyImplementation( ownerKey, role, ownerKey );
+		final AbstractReadWriteAccess.Item cacheItem = (AbstractReadWriteAccess.Item) storageAccess.getFromData( cacheKey );
+		assertThat( cacheItem ).isNotNull();
+
+		final CollectionCacheEntry cacheEntry = (CollectionCacheEntry) cacheItem.getValue();
+		return cacheEntry.getState();
+	}
+
+	@BeforeEach
+	void createTestData(SessionFactoryScope sessions) {
+		sessions.inTransaction( (session) -> {
+			final Person poe = new Person( 1, "John Poe" );
+			session.persist( poe );
+
+			final Person schmidt = new Person( 2, "Jacob Schmidt" );
+			session.persist( schmidt );
+
+			final Person king = new Person( 3, "David King" );
+			session.persist( king );
+
+			final Book nightsEdge = new Book( 1, "A Night's Edge" );
+			nightsEdge.addAuthor( poe );
+			nightsEdge.addAuthor( king );
+			nightsEdge.addEditor( schmidt );
+			session.persist( nightsEdge );
+		} );
+	}
+
+	@AfterEach
+	void dropTestData(SessionFactoryScope sessions) {
+		sessions.inTransaction( (session) -> {
+//			session.createNativeMutationQuery( "delete book_authors" ).executeUpdate();
+//			session.createNativeMutationQuery( "delete book_editors" ).executeUpdate();
+			session.createMutationQuery( "delete Book" ).executeUpdate();
+			session.createMutationQuery( "delete Person" ).executeUpdate();
+		} );
+	}
+}

hibernate-testing/src/main/java/org/hibernate/testing/cache/MapStorageAccessImpl.java

@@ -9,6 +9,7 @@ package org.hibernate.testing.cache;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 
+import org.hibernate.Internal;
 import org.hibernate.cache.spi.support.DomainDataStorageAccess;
 import org.hibernate.engine.spi.SharedSessionContractImplementor;
 
@@ -20,6 +21,14 @@ import org.hibernate.engine.spi.SharedSessionContractImplementor;
 public class MapStorageAccessImpl implements DomainDataStorageAccess {
 	private ConcurrentMap data;
 
+	@Internal
+	public Object getFromData(Object key) {
+		if ( data == null ) {
+			return null;
+		}
+		return data.get( key );
+	}
+
 	@Override
 	public boolean contains(Object key) {
 		return data != null && data.containsKey( key );
@@ -27,10 +36,7 @@ public class MapStorageAccessImpl implements DomainDataStorageAccess {
 
 	@Override
 	public Object getFromCache(Object key, SharedSessionContractImplementor session) {
-		if ( data == null ) {
-			return null;
-		}
-		return data.get( key );
+		return getFromData( key );
 	}
 
 	@Override

hibernate-testing/src/main/java/org/hibernate/testing/orm/domain/StandardDomainModel.java

@@ -10,6 +10,7 @@ import org.hibernate.testing.orm.domain.animal.AnimalDomainModel;
 import org.hibernate.testing.orm.domain.contacts.ContactsDomainModel;
 import org.hibernate.testing.orm.domain.gambit.GambitDomainModel;
 import org.hibernate.testing.orm.domain.helpdesk.HelpDeskDomainModel;
+import org.hibernate.testing.orm.domain.library.LibraryDomainModel;
 import org.hibernate.testing.orm.domain.retail.RetailDomainModel;
 import org.hibernate.testing.orm.domain.userguide.UserguideDomainModel;
 
@@ -22,7 +23,8 @@ public enum StandardDomainModel {
 	GAMBIT( GambitDomainModel.INSTANCE ),
 	HELPDESK( HelpDeskDomainModel.INSTANCE ),
 	RETAIL( RetailDomainModel.INSTANCE ),
-	USERGUIDE( UserguideDomainModel.INSTANCE );
+	USERGUIDE( UserguideDomainModel.INSTANCE ),
+	LIBRARY( LibraryDomainModel.INSTANCE );
 
 	private final DomainModelDescriptor domainModel;
 

hibernate-testing/src/main/java/org/hibernate/testing/orm/domain/library/Book.java

@@ -0,0 +1,107 @@
+/*
+ * 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.testing.orm.domain.library;
+
+import java.util.HashSet;
+import java.util.Set;
+
+import org.hibernate.annotations.Cache;
+import org.hibernate.annotations.CacheConcurrencyStrategy;
+
+import jakarta.persistence.CollectionTable;
+import jakarta.persistence.Entity;
+import jakarta.persistence.Id;
+import jakarta.persistence.Basic;
+import jakarta.persistence.JoinColumn;
+import jakarta.persistence.JoinTable;
+import jakarta.persistence.ManyToMany;
+
+/**
+ * @author Steve Ebersole
+ */
+@Entity
+public class Book {
+	@Id
+	private Integer id;
+	private String name;
+	private String isbn;
+
+	@ManyToMany
+	@JoinTable( name = "book_authors",
+			joinColumns = @JoinColumn(name = "book_fk"),
+			inverseJoinColumns = @JoinColumn(name = "author_fk")
+	)
+	@Cache( usage = CacheConcurrencyStrategy.READ_WRITE)
+	Set<Person> authors;
+
+	@ManyToMany
+	@JoinTable( name = "book_editors",
+			joinColumns = @JoinColumn(name = "book_fk"),
+			inverseJoinColumns = @JoinColumn(name = "editor_fk")
+	)
+	@Cache( usage = CacheConcurrencyStrategy.READ_WRITE)
+	Set<Person> editors;
+
+	protected Book() {
+		// for Hibernate use
+	}
+
+	public Book(Integer id, String name) {
+		this.id = id;
+		this.name = name;
+	}
+
+	public Integer getId() {
+		return id;
+	}
+
+	public String getName() {
+		return name;
+	}
+
+	public void setName(String name) {
+		this.name = name;
+	}
+
+	public String getIsbn() {
+		return isbn;
+	}
+
+	public void setIsbn(String isbn) {
+		this.isbn = isbn;
+	}
+
+	public Set<Person> getAuthors() {
+		return authors;
+	}
+
+	public void setAuthors(Set<Person> authors) {
+		this.authors = authors;
+	}
+
+	public void addAuthor(Person author) {
+		if ( authors == null ) {
+			authors = new HashSet<>();
+		}
+		authors.add( author );
+	}
+
+	public Set<Person> getEditors() {
+		return editors;
+	}
+
+	public void setEditors(Set<Person> editors) {
+		this.editors = editors;
+	}
+
+	public void addEditor(Person editor) {
+		if ( editors == null ) {
+			editors = new HashSet<>();
+		}
+		editors.add( editor );
+	}
+}

hibernate-testing/src/main/java/org/hibernate/testing/orm/domain/library/LibraryDomainModel.java

@@ -0,0 +1,20 @@
+/*
+ * 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.testing.orm.domain.library;
+
+import org.hibernate.testing.orm.domain.AbstractDomainModelDescriptor;
+
+/**
+ * @author Steve Ebersole
+ */
+public class LibraryDomainModel extends AbstractDomainModelDescriptor {
+	public static final LibraryDomainModel INSTANCE = new LibraryDomainModel();
+
+	public LibraryDomainModel() {
+		super( Book.class, Person.class );
+	}
+}

hibernate-testing/src/main/java/org/hibernate/testing/orm/domain/library/Person.java

@@ -0,0 +1,43 @@
+/*
+ * 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.testing.orm.domain.library;
+
+import jakarta.persistence.Entity;
+import jakarta.persistence.Id;
+import jakarta.persistence.Basic;
+
+/**
+ * @author Steve Ebersole
+ */
+@Entity
+public class Person {
+	@Id
+	private Integer id;
+	@Basic
+	private String name;
+
+	protected Person() {
+		// for Hibernate use
+	}
+
+	public Person(Integer id, String name) {
+		this.id = id;
+		this.name = name;
+	}
+
+	public Integer getId() {
+		return id;
+	}
+
+	public String getName() {
+		return name;
+	}
+
+	public void setName(String name) {
+		this.name = name;
+	}
+}