Skip to content

Commit 0797bf1

Browse files
jjennnnbeikov
jjennnn
authored and
beikov
committed
HHH-20515 Fix findMultiple overwriting session state with cache values
Signed-off-by: jjennnn <jennifer.joby@ucdconnect.ie>
1 parent 3a760c4 commit 0797bf1

3 files changed

Lines changed: 301 additions & 4 deletions

File tree

‎hibernate-core/src/main/java/org/hibernate/loader/ast/internal/AbstractMultiIdEntityLoader.java‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -246,10 +246,21 @@ private boolean isLoadFromCaches(
246246
// look for it in the second-level cache
247247
final Object entity =
248248
loadFromSecondLevelCache( entityKey, lockOptions, session );
249+
final var persistenceContext = session.getPersistenceContextInternal();
249250
if ( entity != null ) {
250-
results.add( i, entity );
251+
results.add( i, persistenceContext.proxyFor( getLoadable().getEntityPersister(), entityKey, entity ) );
251252
return true;
252253
}
254+
else {
255+
// check if the PC contains a deleted entry, if so return true
256+
final var holder = persistenceContext.getEntityHolder( entityKey );
257+
final var entry = holder == null ? null : holder.getEntityEntry();
258+
if ( entry != null && entry.getStatus().isDeletedOrGone() ) {
259+
assert loadOptions.getRemovalsMode() != FindMultipleOption.RemovalsMode.INCLUDE;
260+
results.add( i, null );
261+
return true;
262+
}
263+
}
253264
}
254265

255266
return false;

‎hibernate-core/src/main/java/org/hibernate/loader/internal/CacheLoadHelper.java‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,9 @@ private static Object processCachedEntry(
198198
instanceToLoad,
199199
entityKey
200200
);
201+
if ( entity == null ) {
202+
return null;
203+
}
201204
if ( !persister.isInstance( entity ) ) {
202205
// Clean up the inconsistent return class entity from the persistence context
203206
final var persistenceContext = source.getPersistenceContext();
@@ -266,10 +269,18 @@ private static Object convertCacheEntryToEntity(
266269
if ( instanceToLoad != null ) {
267270
entity = instanceToLoad;
268271
}
272+
else if ( oldHolder != null && oldHolder.getEntity() != null ) {
273+
if ( oldHolder.isInitialized() ) {
274+
return oldHolder.getEntityEntry() != null && oldHolder.getEntityEntry().getStatus().isDeletedOrGone()
275+
? null
276+
: oldHolder.getEntity();
277+
}
278+
else {
279+
entity = oldHolder.getEntity();
280+
}
281+
}
269282
else {
270-
entity = oldHolder != null && oldHolder.getEntity() != null
271-
? oldHolder.getEntity()
272-
: source.instantiate( subclassPersister, entityId );
283+
entity = subclassPersister.instantiate( entityId, source );
273284
}
274285

275286
if ( isPersistentAttributeInterceptable( entity ) ) {

‎hibernate-core/src/test/java/org/hibernate/orm/test/cache/FindMultipleCacheTest.java‎

Lines changed: 275 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,275 @@
1+
/*
2+
* SPDX-License-Identifier: Apache-2.0
3+
* Copyright Red Hat Inc. and Hibernate Authors
4+
*/
5+
package org.hibernate.orm.test.cache;
6+
7+
import java.util.List;
8+
9+
import org.hibernate.FindMultipleOption;
10+
import org.hibernate.annotations.Cache;
11+
import org.hibernate.annotations.CacheConcurrencyStrategy;
12+
import org.hibernate.cfg.AvailableSettings;
13+
14+
import org.hibernate.testing.orm.junit.DomainModel;
15+
import org.hibernate.testing.orm.junit.JiraKey;
16+
import org.hibernate.testing.orm.junit.ServiceRegistry;
17+
import org.hibernate.testing.orm.junit.SessionFactory;
18+
import org.hibernate.testing.orm.junit.SessionFactoryScope;
19+
import org.hibernate.testing.orm.junit.Setting;
20+
import org.junit.jupiter.api.AfterEach;
21+
import org.junit.jupiter.api.Test;
22+
23+
import jakarta.persistence.Basic;
24+
import jakarta.persistence.Entity;
25+
import jakarta.persistence.Id;
26+
27+
import static org.assertj.core.api.Assertions.assertThat;
28+
29+
@DomainModel(
30+
annotatedClasses = {
31+
FindMultipleCacheTest.CachedEntity.class,
32+
FindMultipleCacheTest.NotCachedEntity.class
33+
}
34+
)
35+
@ServiceRegistry(
36+
settings = {
37+
@Setting(name = AvailableSettings.USE_SECOND_LEVEL_CACHE, value = "true"),
38+
@Setting(name = AvailableSettings.USE_QUERY_CACHE, value = "true"),
39+
@Setting(name = AvailableSettings.SHOW_SQL, value = "true"),
40+
@Setting(name = AvailableSettings.FORMAT_SQL, value = "true"),
41+
}
42+
)
43+
@SessionFactory(generateStatistics = true)
44+
@JiraKey("HHH-20515")
45+
public class FindMultipleCacheTest {
46+
47+
@AfterEach
48+
public void tearDown(SessionFactoryScope scope) {
49+
scope.getSessionFactory().getSchemaManager().truncate();
50+
scope.getSessionFactory().getCache().evictAllRegions();
51+
}
52+
53+
@Test
54+
public void testFindMultipleWithDisabledPreservesModifiedStateWith2LCPresent(SessionFactoryScope scope) {
55+
scope.inTransaction( session -> {
56+
session.persist( new CachedEntity( 1L, "originalName" ) );
57+
} );
58+
59+
scope.inTransaction( session -> {
60+
session.find( CachedEntity.class, 1L );
61+
} );
62+
63+
scope.inTransaction( session -> {
64+
CachedEntity entity = session.find( CachedEntity.class, 1L );
65+
entity.setName( "modifiedName" );
66+
67+
List<CachedEntity> results = session.findMultiple(
68+
CachedEntity.class,
69+
List.of( 1L ),
70+
FindMultipleOption.SessionCheckMode.DISABLED
71+
);
72+
73+
assertThat( results ).hasSize( 1 );
74+
assertThat( results.get( 0 ) ).isSameAs( entity );
75+
assertThat( results.get( 0 ).getName() ).isEqualTo( "modifiedName" );
76+
} );
77+
}
78+
79+
@Test
80+
public void testFindMultipleWithDisabledAndUninitializedProxyInPersistenceContext(
81+
SessionFactoryScope scope) {
82+
scope.inTransaction( session -> {
83+
session.persist( new CachedEntity( 1L, "name1" ) );
84+
} );
85+
86+
scope.inTransaction( session -> {
87+
session.find( CachedEntity.class, 1L );
88+
} );
89+
90+
scope.inTransaction( session -> {
91+
CachedEntity proxy =
92+
session.getReference( CachedEntity.class, 1L );
93+
94+
List<CachedEntity> results = session.findMultiple(
95+
CachedEntity.class,
96+
List.of( 1L ),
97+
FindMultipleOption.SessionCheckMode.DISABLED
98+
99+
);
100+
101+
assertThat( results ).hasSize( 1 );
102+
assertThat( results.get( 0 ) ).isSameAs( proxy );
103+
} );
104+
}
105+
106+
@Test
107+
public void testFindMultipleWithEnabledPreservesModifiedStateWith2LCPresent(SessionFactoryScope scope) {
108+
scope.inTransaction( session -> {
109+
session.persist( new CachedEntity( 1L, "originalName" ) );
110+
} );
111+
112+
scope.inTransaction( session -> {
113+
session.find( CachedEntity.class, 1L );
114+
} );
115+
116+
scope.inTransaction( session -> {
117+
CachedEntity entity = session.find( CachedEntity.class, 1L );
118+
entity.setName( "modifiedName" );
119+
120+
List<CachedEntity> results = session.findMultiple(
121+
CachedEntity.class,
122+
List.of( 1L ),
123+
FindMultipleOption.SessionCheckMode.ENABLED
124+
);
125+
126+
assertThat( results ).hasSize( 1 );
127+
assertThat( results.get( 0 ) ).isSameAs( entity );
128+
assertThat( results.get( 0 ).getName() ).isEqualTo( "modifiedName" );
129+
} );
130+
}
131+
132+
@Test
133+
public void testFindMultiplePreservesModifiedStateForNonCachedEntity(SessionFactoryScope scope) {
134+
scope.inTransaction( session -> {
135+
session.persist( new NotCachedEntity( 1L, "originalName" ) );
136+
} );
137+
138+
scope.inTransaction( session -> {
139+
NotCachedEntity entity = session.findMultiple( NotCachedEntity.class, List.of( 1L ) ).get( 0 );
140+
assertThat( entity.getName() ).isEqualTo( "originalName" );
141+
entity.setName( "modifiedName" );
142+
143+
NotCachedEntity entity2 = session.findMultiple( NotCachedEntity.class, List.of( 1L ) ).get( 0 );
144+
assertThat( entity2 ).isSameAs( entity );
145+
assertThat( entity2.getName() ).isEqualTo( "modifiedName" );
146+
} );
147+
}
148+
149+
@Test
150+
public void testFindMultipleRetrievesPersistedChangesFromCache(SessionFactoryScope scope) {
151+
scope.inTransaction( session -> {
152+
session.persist( new CachedEntity( 1L, "originalName" ) );
153+
} );
154+
155+
scope.inTransaction( session -> {
156+
CachedEntity entity = session.findMultiple( CachedEntity.class, List.of( 1L ) ).get( 0 );
157+
entity.setName( "persistedName" );
158+
} );
159+
160+
scope.inTransaction( session -> {
161+
CachedEntity entity = session.findMultiple( CachedEntity.class, List.of( 1L ) ).get( 0 );
162+
assertThat( entity.getName() ).isEqualTo( "persistedName" );
163+
} );
164+
}
165+
166+
@Test
167+
public void testFindMultipleWithDisabledPreservesModifiedStateFromDb(SessionFactoryScope scope) {
168+
scope.inTransaction( session -> {
169+
session.persist( new CachedEntity( 1L, "originalName" ) );
170+
} );
171+
172+
scope.inTransaction( session -> {
173+
CachedEntity entity = session.find( CachedEntity.class, 1L );
174+
175+
entity.setName( "modifiedName" );
176+
177+
List<CachedEntity> results = session.findMultiple(
178+
CachedEntity.class,
179+
List.of( 1L ),
180+
FindMultipleOption.SessionCheckMode.DISABLED
181+
);
182+
183+
assertThat( results ).hasSize( 1 );
184+
assertThat( results.get( 0 ) ).isSameAs( entity );
185+
assertThat( results.get( 0 ).getName() ).isEqualTo( "modifiedName" );
186+
} );
187+
}
188+
189+
@Test
190+
public void testFindMultipleWithDisabledReturnsDeletedEntity(SessionFactoryScope scope) {
191+
scope.inTransaction( session -> {
192+
session.persist( new CachedEntity( 1L, "name1" ) );
193+
session.persist( new CachedEntity( 2L, "name2" ) );
194+
} );
195+
196+
scope.inTransaction( session -> {
197+
CachedEntity deletedEntity =
198+
session.find( CachedEntity.class, 1L );
199+
200+
session.remove( deletedEntity );
201+
202+
List<CachedEntity> results = session.findMultiple(
203+
CachedEntity.class,
204+
List.of( 1L, 2L ),
205+
FindMultipleOption.SessionCheckMode.DISABLED
206+
);
207+
208+
CachedEntity returnedEntity = results.get( 0 );
209+
assertThat( results ).hasSize( 2 );
210+
211+
assertThat( results.get( 0 ) ).isNull();
212+
assertThat( results.get( 1 ) ).isNotNull();
213+
assertThat( results.get( 1 ).getName() ).isEqualTo( "name2" );
214+
} );
215+
}
216+
217+
@Entity(name = "CachedEntity")
218+
@Cache(usage = CacheConcurrencyStrategy.READ_WRITE)
219+
public static class CachedEntity {
220+
@Id
221+
private Long id;
222+
223+
@Basic
224+
private String name;
225+
226+
public CachedEntity() {
227+
}
228+
229+
public CachedEntity(Long id, String name) {
230+
this.id = id;
231+
this.name = name;
232+
}
233+
234+
public Long getId() {
235+
return id;
236+
}
237+
238+
public String getName() {
239+
return name;
240+
}
241+
242+
public void setName(String name) {
243+
this.name = name;
244+
}
245+
}
246+
247+
@Entity(name = "NotCachedEntity")
248+
public static class NotCachedEntity {
249+
@Id
250+
private Long id;
251+
252+
@Basic
253+
private String name;
254+
255+
public NotCachedEntity() {
256+
}
257+
258+
public NotCachedEntity(Long id, String name) {
259+
this.id = id;
260+
this.name = name;
261+
}
262+
263+
public Long getId() {
264+
return id;
265+
}
266+
267+
public String getName() {
268+
return name;
269+
}
270+
271+
public void setName(String name) {
272+
this.name = name;
273+
}
274+
}
275+
}

0 commit comments

Comments
 (0)