Skip to content

Commit e9f9bc5

Browse files
jxblumjxblum
committed
Fix bug in SimpleGemfireRepository.deleteAll running in a client/server topology with a server-side PARTITION Region.
Resolves spring-projectsgh-512.
1 parent daefa9b commit e9f9bc5

File tree

5 files changed

+353
-33
lines changed

5 files changed

+353
-33
lines changed

‎spring-data-geode/src/main/java/org/springframework/data/gemfire/repository/support/SimpleGemfireRepository.java

+57-9
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,9 @@
2626
import java.util.stream.Collectors;
2727
import java.util.stream.StreamSupport;
2828

29-
import org.apache.geode.cache.Cache;
3029
import org.apache.geode.cache.CacheTransactionManager;
3130
import org.apache.geode.cache.DataPolicy;
31+
import org.apache.geode.cache.GemFireCache;
3232
import org.apache.geode.cache.Region;
3333
import org.apache.geode.cache.query.SelectResults;
3434

@@ -42,6 +42,7 @@
4242
import org.springframework.data.gemfire.repository.query.QueryString;
4343
import org.springframework.data.gemfire.repository.query.support.PagingUtils;
4444
import org.springframework.data.gemfire.util.CollectionUtils;
45+
import org.springframework.data.gemfire.util.RegionUtils;
4546
import org.springframework.data.gemfire.util.SpringUtils;
4647
import org.springframework.data.repository.CrudRepository;
4748
import org.springframework.data.repository.PagingAndSortingRepository;
@@ -144,6 +145,9 @@ public SimpleGemfireRepository(@NonNull GemfireTemplate template,
144145
return this.template;
145146
}
146147

148+
/**
149+
* @inheritDoc
150+
*/
147151
@Override
148152
public <U extends T> U save(@NonNull U entity) {
149153

@@ -159,6 +163,9 @@ public <U extends T> U save(@NonNull U entity) {
159163
return entity;
160164
}
161165

166+
/**
167+
* @inheritDoc
168+
*/
162169
@Override
163170
public T save(@NonNull Wrapper<T, ID> wrapper) {
164171

@@ -174,6 +181,9 @@ public T save(@NonNull Wrapper<T, ID> wrapper) {
174181
return entity;
175182
}
176183

184+
/**
185+
* @inheritDoc
186+
*/
177187
@Override
178188
public <U extends T> Iterable<U> saveAll(@NonNull Iterable<U> entities) {
179189

@@ -227,6 +237,9 @@ public boolean existsById(ID id) {
227237
return findById(id).isPresent();
228238
}
229239

240+
/**
241+
* @inheritDoc
242+
*/
230243
@Override
231244
public @NonNull Iterable<T> findAll() {
232245

@@ -238,6 +251,9 @@ public boolean existsById(ID id) {
238251
return toList(selectResults);
239252
}
240253

254+
/**
255+
* @inheritDoc
256+
*/
241257
@Override
242258
public Page<T> findAll(@NonNull Pageable pageable) {
243259

@@ -248,6 +264,9 @@ public Page<T> findAll(@NonNull Pageable pageable) {
248264
return toPage(results, pageable);
249265
}
250266

267+
/**
268+
* @inheritDoc
269+
*/
251270
@Override
252271
public @NonNull Iterable<T> findAll(@NonNull Sort sort) {
253272

@@ -260,6 +279,9 @@ public Page<T> findAll(@NonNull Pageable pageable) {
260279
return toList(selectResults);
261280
}
262281

282+
/**
283+
* @inheritDoc
284+
*/
263285
@Override
264286
public @NonNull Iterable<T> findAllById(@NonNull Iterable<ID> ids) {
265287

@@ -278,6 +300,9 @@ public Page<T> findAll(@NonNull Pageable pageable) {
278300
return values;
279301
}
280302

303+
/**
304+
* @inheritDoc
305+
*/
281306
@Override
282307
public Optional<T> findById(@NonNull ID id) {
283308

@@ -288,11 +313,17 @@ public Optional<T> findById(@NonNull ID id) {
288313
return Optional.ofNullable(value);
289314
}
290315

316+
/**
317+
* @inheritDoc
318+
*/
291319
@Override
292320
public void delete(@NonNull T entity) {
293321
deleteById(getEntityInformation().getRequiredId(entity));
294322
}
295323

324+
/**
325+
* @inheritDoc
326+
*/
296327
@Override
297328
public void deleteAll() {
298329

@@ -309,11 +340,17 @@ public void deleteAll() {
309340
});
310341
}
311342

343+
/**
344+
* @inheritDoc
345+
*/
312346
@Override
313347
public void deleteAll(@NonNull Iterable<? extends T> entities) {
314348
CollectionUtils.nullSafeIterable(entities).forEach(this::delete);
315349
}
316350

351+
/**
352+
* @inheritDoc
353+
*/
317354
@Override
318355
public void deleteAllById(@NonNull Iterable<? extends ID> ids) {
319356

@@ -327,34 +364,45 @@ public void deleteAllById(@NonNull Iterable<? extends ID> ids) {
327364
}
328365
}
329366

367+
/**
368+
* @inheritDoc
369+
*/
330370
@Override
331371
public void deleteById(@NonNull ID id) {
332372
getTemplate().remove(id);
333373
}
334374

335-
boolean isPartitioned(Region<?, ?> region) {
375+
boolean isPartitioned(@Nullable Region<?, ?> region) {
336376

337377
return region != null
338378
&& region.getAttributes() != null
339379
&& isPartitioned(region.getAttributes().getDataPolicy());
340380
}
341381

342-
boolean isPartitioned(DataPolicy dataPolicy) {
382+
boolean isPartitioned(@Nullable DataPolicy dataPolicy) {
343383
return dataPolicy != null && dataPolicy.withPartitioning();
344384
}
345385

346-
boolean isTransactionPresent(Region<?, ?> region) {
386+
boolean isTransactionPresent(@Nullable Region<?, ?> region) {
347387

348-
return region.getRegionService() instanceof Cache
349-
&& isTransactionPresent(((Cache) region.getRegionService()).getCacheTransactionManager());
388+
return region != null
389+
&& region.getRegionService() instanceof GemFireCache
390+
&& isTransactionPresent(((GemFireCache) region.getRegionService()).getCacheTransactionManager());
350391
}
351392

352-
boolean isTransactionPresent(CacheTransactionManager cacheTransactionManager) {
393+
boolean isTransactionPresent(@Nullable CacheTransactionManager cacheTransactionManager) {
353394
return cacheTransactionManager != null && cacheTransactionManager.exists();
354395
}
355396

356-
<K> void doRegionClear(Region<K, ?> region) {
357-
region.removeAll(region.keySet());
397+
<K> void doRegionClear(@NonNull Region<K, ?> region) {
398+
region.removeAll(resolveRegionKeys(region));
399+
}
400+
401+
@NonNull <K> Set<K> resolveRegionKeys(@NonNull Region<K, ?> region) {
402+
403+
return RegionUtils.isClient(region) ? region.keySetOnServer()
404+
: RegionUtils.isServer(region) ? region.keySet()
405+
: Collections.emptySet();
358406
}
359407

360408
@NonNull List<T> toList(@Nullable Iterable<T> iterable) {

‎spring-data-geode/src/main/java/org/springframework/data/gemfire/util/CacheUtils.java

+19-15
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@
2828
import org.apache.geode.distributed.DistributedSystem;
2929
import org.apache.geode.internal.cache.GemFireCacheImpl;
3030

31+
import org.springframework.lang.NonNull;
32+
import org.springframework.lang.Nullable;
3133
import org.springframework.util.StringUtils;
3234

3335
/**
@@ -51,10 +53,9 @@ public abstract class CacheUtils extends DistributedSystemUtils {
5153

5254
public static final String DEFAULT_POOL_NAME = "DEFAULT";
5355

54-
@SuppressWarnings("all")
55-
public static boolean isClient(GemFireCache cache) {
56+
public static boolean isClient(@Nullable GemFireCache cache) {
5657

57-
boolean client = (cache instanceof ClientCache);
58+
boolean client = cache instanceof ClientCache;
5859

5960
if (cache instanceof GemFireCacheImpl) {
6061
client &= ((GemFireCacheImpl) cache).isClient();
@@ -63,23 +64,27 @@ public static boolean isClient(GemFireCache cache) {
6364
return client;
6465
}
6566

66-
public static boolean isDefaultPool(Pool pool) {
67-
return Optional.ofNullable(pool).map(Pool::getName).filter(CacheUtils::isDefaultPool).isPresent();
67+
public static boolean isDefaultPool(@Nullable Pool pool) {
68+
69+
return Optional.ofNullable(pool)
70+
.map(Pool::getName)
71+
.filter(CacheUtils::isDefaultPool)
72+
.isPresent();
6873
}
6974

70-
public static boolean isNotDefaultPool(Pool pool) {
75+
public static boolean isNotDefaultPool(@Nullable Pool pool) {
7176
return !isDefaultPool(pool);
7277
}
7378

74-
public static boolean isDefaultPool(String poolName) {
79+
public static boolean isDefaultPool(@Nullable String poolName) {
7580
return DEFAULT_POOL_NAME.equals(poolName);
7681
}
7782

78-
public static boolean isNotDefaultPool(String poolName) {
83+
public static boolean isNotDefaultPool(@Nullable String poolName) {
7984
return !isDefaultPool(poolName);
8085
}
8186

82-
public static boolean isDurable(ClientCache clientCache) {
87+
public static boolean isDurable(@Nullable ClientCache clientCache) {
8388

8489
// NOTE: Technically, the following code snippet would be more useful/valuable but is not "testable"!
8590
//((InternalDistributedSystem) distributedSystem).getConfig().getDurableClientId();
@@ -93,10 +98,9 @@ public static boolean isDurable(ClientCache clientCache) {
9398
.isPresent();
9499
}
95100

96-
@SuppressWarnings("all")
97-
public static boolean isPeer(GemFireCache cache) {
101+
public static boolean isPeer(@Nullable GemFireCache cache) {
98102

99-
boolean peer = (cache instanceof Cache);
103+
boolean peer = cache instanceof Cache;
100104

101105
if (cache instanceof GemFireCacheImpl) {
102106
peer &= !((GemFireCacheImpl) cache).isClient();
@@ -109,17 +113,17 @@ public static boolean close() {
109113
return close(resolveGemFireCache());
110114
}
111115

112-
public static boolean close(GemFireCache gemfireCache) {
116+
public static boolean close(@NonNull GemFireCache gemfireCache) {
113117
return close(gemfireCache, () -> {});
114118
}
115119

116-
public static boolean close(GemFireCache gemfireCache, Runnable shutdownHook) {
120+
public static boolean close(@NonNull GemFireCache gemfireCache, @Nullable Runnable shutdownHook) {
117121

118122
try {
119123
gemfireCache.close();
120124
return true;
121125
}
122-
catch (Exception ignore) {
126+
catch (Throwable ignore) {
123127
return false;
124128
}
125129
finally {

‎spring-data-geode/src/main/java/org/springframework/data/gemfire/util/RegionUtils.java

+12-1
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ public static boolean close(Region<?, ?> region) {
113113
* @return a boolean indicating whether the target {@link Region} is a {@literal client} {@link Region}.
114114
* @see org.apache.geode.cache.Region
115115
*/
116-
public static boolean isClient(Region<?, ?> region) {
116+
public static boolean isClient(@Nullable Region<?, ?> region) {
117117

118118
return Optional.ofNullable(region)
119119
.map(Region::getAttributes)
@@ -175,4 +175,15 @@ public static String toRegionPath(@Nullable Region<?, ?> region) {
175175
public static String toRegionPath(String regionName) {
176176
return String.format("%1$s%2$s", Region.SEPARATOR, regionName);
177177
}
178+
179+
/**
180+
* Determines whether the target {@link Region} is a {@literal server-side} {@link Region}.
181+
*
182+
* @param region {@link Region} to evaluate.
183+
* @return a boolean indicating whether the target {@link Region} is a {@literal server-side} {@link Region}.
184+
* @see org.apache.geode.cache.Region
185+
*/
186+
public static boolean isServer(@Nullable Region<?, ?> region) {
187+
return region != null && !isClient(region);
188+
}
178189
}

0 commit comments

Comments
 (0)