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.

Sign up for GitHub

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

Jump to bottom

Bug: TenanAwareKey vs not tenant aware keys in cache #14

Closed
wants to merge 1 commit into from
Closed

Bug: TenanAwareKey vs not tenant aware keys in cache #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 4 additions & 12 deletions src/main/java/io/ebean/hazelcast/HzCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@

import io.ebean.cache.ServerCache;
import io.ebean.cache.ServerCacheStatistics;
import io.ebean.cache.TenantAwareKey;
import io.ebean.config.CurrentTenantProvider;

import java.util.Map;
import java.util.Set;
Expand All @@ -15,16 +13,10 @@
*/
final class HzCache implements ServerCache {

private final TenantAwareKey tenantAwareKey;
private final IMap<Object, Object> map;

HzCache(IMap<Object, Object> map, CurrentTenantProvider tenantProvider) {
HzCache(IMap<Object, Object> map) {
this.map = map;
this.tenantAwareKey = new TenantAwareKey(tenantProvider);
}

private Object key(Object key) {
return tenantAwareKey.key(key);
}

@Override
Expand All @@ -39,17 +31,17 @@ public void putAll(Map<Object, Object> keyValues) {

@Override
public Object get(Object id) {
return map.get(key(id));
return map.get(id);
}

@Override
public void put(Object id, Object value) {
map.put(key(id), value);
map.put(id, value);
}

@Override
public void remove(Object id) {
map.delete(key(id));
map.delete(id);
}

@Override
Expand Down
8 changes: 7 additions & 1 deletion src/main/java/io/ebean/hazelcast/HzCacheFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import io.ebean.cache.ServerCacheFactory;
import io.ebean.cache.ServerCacheNotification;
import io.ebean.cache.ServerCacheNotify;
import io.ebean.cache.TenantAwareKey;
import io.ebean.config.DatabaseConfig;
import io.ebeaninternal.server.cache.DefaultServerCacheConfig;
import io.ebeaninternal.server.cache.DefaultServerQueryCache;
Expand Down Expand Up @@ -118,7 +119,12 @@ private ServerCache createNormalCache(ServerCacheConfig config) {
String fullName = config.getType().name() + "-" + config.getCacheKey();
logger.debug("get cache [{}]", fullName);
IMap<Object, Object> map = instance.getMap(fullName);
return new HzCache(map, config.getTenantProvider());
ServerCache hzCache = new HzCache(map);
if (config.getTenantProvider() != null) {
return new TenantAwareCache(hzCache, new TenantAwareKey(config.getTenantProvider()));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rbygrave I would suggest to move that code to Ebean and remove cache handling form the plugin completely. It acts as a proxy and converts all keys to be tenant-aware.

Unfortunately, we have to do some more or less expensive copy operations on getAll/putAll/removeAll methods.

Note: this kind of bug is also present in other cache plugin(s), e.g. here: https:/ebean-orm/ebean-ignite/blob/master/src/main/java/io/ebean/ignite/IgCache.java#L49

} else {
return hzCache;
}
}

private ServerCache createQueryCache(ServerCacheConfig config) {
Expand Down
82 changes: 82 additions & 0 deletions src/main/java/io/ebean/hazelcast/TenantAwareCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
package io.ebean.hazelcast;

import java.util.HashMap;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;

import io.ebean.cache.ServerCache;
import io.ebean.cache.ServerCacheStatistics;
import io.ebean.cache.TenantAwareKey;

public class TenantAwareCache implements ServerCache {

private final ServerCache delegate;
private final TenantAwareKey tenantAwareKey;

public TenantAwareCache(ServerCache delegate, TenantAwareKey tenantAwareKey) {
this.delegate = delegate;
this.tenantAwareKey = tenantAwareKey;
}

private Object key(Object key) {
return tenantAwareKey.key(key);
}
@Override
public Object get(Object id) {
return delegate.get(key(id));
}

@Override
public void put(Object id, Object value) {
delegate.put(key(id), value);
}

@Override
public void remove(Object id) {
delegate.remove(key(id));
}

@Override
public void clear() {
delegate.clear();
}

@Override
public int size() {
return delegate.size();
}

@Override
public int getHitRatio() {
return delegate.getHitRatio();
}

@Override
public ServerCacheStatistics getStatistics(boolean reset) {
return delegate.getStatistics(reset);
}

@Override
public Map<Object, Object> getAll(Set<Object> keys) {
Map<Object, Object> keyMapping = new HashMap<>();
keys.forEach(k -> keyMapping.put(key(k),k));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to convert the keyset (e.g. 1234, 5678) to a tenantaware key (1234:1, 5678:1)

and we have to do remove the tenant-info after we get the result form the cache.

this could be probably done by tmp.forEach((k,v)-> ret.put(((CacheKey) k).key, v)) as it should be always a cacheKey

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ret.put(((CacheKey) k).key ...

Yes, that looks better to me.

Map<Object, Object> tmp = delegate.getAll(keyMapping.keySet());
Map<Object, Object> ret = new HashMap<Object, Object>();
tmp.forEach((k,v)-> ret.put(keyMapping.get(k), v)); // remove tenant info here
return ret;
}

@Override
public void putAll(Map<Object, Object> keyValues) {
Map<Object, Object> tmp = new HashMap<Object, Object>();
keyValues.forEach((k,v)-> tmp.put(key(k), v));
delegate.putAll(tmp);
}

@Override
public void removeAll(Set<Object> keys) {
delegate.removeAll(keys.stream().map(this::key).collect(Collectors.toSet()));
}

}