[NAE-2182] Clearing actions cache #348
Conversation
…ache endpoints for actionsCache, functionsCache and namespaceFunctionsCache
WalkthroughReplaces previous cache construction with a SimpleCacheManager containing per-name ConcurrentMapCache instances and adds bounded GenericMapCache caches for actions and functions; introduces CacheMapKeys; refactors FieldActionsCacheService to use CacheManager and adds reload by PetriNet id; updates cache properties and actuator exposure. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant FieldActionsCacheService as FieldActionsCacheService
participant CacheManager
participant ActionsCache as Cache "actions"
note over FieldActionsCacheService,ActionsCache: getCompiledAction(field, actionRef)
Client->>FieldActionsCacheService: getCompiledAction(...)
FieldActionsCacheService->>CacheManager: getCache("actions")
CacheManager-->>FieldActionsCacheService: Cache
FieldActionsCacheService->>ActionsCache: get(key, loader)
alt cache miss
ActionsCache->>FieldActionsCacheService: invoke loader
FieldActionsCacheService->>FieldActionsCacheService: compile Closure via Groovy shell
FieldActionsCacheService-->>ActionsCache: put(key, Closure)
ActionsCache-->>FieldActionsCacheService: Closure (stored)
else cache hit
ActionsCache-->>FieldActionsCacheService: Closure
end
FieldActionsCacheService-->>Client: Closure
sequenceDiagram
autonumber
participant Caller
participant FieldActionsCacheService
participant CacheManager
participant NamespaceFunctions as Cache "namespaceFunctions"
participant PetriNetService
note over FieldActionsCacheService,NamespaceFunctions: reloadCachedFunctions(petriNetId)
Caller->>FieldActionsCacheService: reloadCachedFunctions(petriNetId)
FieldActionsCacheService->>CacheManager: getCache("namespaceFunctions")
CacheManager-->>FieldActionsCacheService: Cache
FieldActionsCacheService->>NamespaceFunctions: evict(petriNetId)
FieldActionsCacheService->>PetriNetService: getNewestVersion(petriNetId)
PetriNetService-->>FieldActionsCacheService: PetriNet
FieldActionsCacheService->>FieldActionsCacheService: evaluate functions for PetriNet
FieldActionsCacheService->>NamespaceFunctions: put(petriNetId, evaluatedFunctions)
FieldActionsCacheService-->>Caller: void
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-07-31T23:40:46.499ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dominikvozr
requested review from
Retoocs,
machacjozef,
renczesstefan and
tuplle
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheMapKeys.java (1)
3-7: Make utility holder non-instantiable and final.Prevents accidental instantiation/extension and clarifies intent.
-public class CacheMapKeys { +public final class CacheMapKeys { public static final String ACTIONS = "actionsCache"; public static final String FUNCTIONS = "functionsCache"; public static final String NAMESPACE_FUNCTIONS = "namespaceFunctionsCache"; + + private CacheMapKeys() { + throw new AssertionError("No instances"); + } }application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (1)
22-24: Leaking the mutable Map via getNativeCache couples callers to internals.FieldActionsCacheService manipulating the map directly bypasses cache invariants/locking.
Prefer cache APIs (
get/put/evict/clear). If direct access is required, document it and ensure all such usage is confined and synchronized.application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java (1)
43-65: Confirm thread-safety of MaxSizeHashMap or wrap it.If
MaxSizeHashMapis not concurrent, concurrent access through the cache will race.Options:
- Wrap:
() -> java.util.Collections.synchronizedMap(new MaxSizeHashMap<>(...))- Or rely on the synchronized GenericMapCache changes above.
- Add a brief Javadoc on
MaxSizeHashMapusage here clarifying concurrency expectations.application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (3)
142-143: Avoid raw cast in getNamespaceFunctionCache; ensure type-safety and null-safety.Keeps method contract explicit and prevents CCE if the impl changes.
- return new HashMap<>((Map) cacheManager.getCache(CacheMapKeys.NAMESPACE_FUNCTIONS).getNativeCache()); + Map<String, List<CachedFunction>> nativeMap = + (Map<String, List<CachedFunction>>) getRequiredCache(CacheMapKeys.NAMESPACE_FUNCTIONS).getNativeCache(); + return new HashMap<>(nativeMap);
147-148: Clear methods: use guarded cache retrieval.- cacheManager.getCache(CacheMapKeys.ACTIONS).clear(); + getRequiredCache(CacheMapKeys.ACTIONS).clear(); @@ - cacheManager.getCache(CacheMapKeys.NAMESPACE_FUNCTIONS).clear(); + getRequiredCache(CacheMapKeys.NAMESPACE_FUNCTIONS).clear(); @@ - cacheManager.getCache(CacheMapKeys.FUNCTIONS).clear(); + getRequiredCache(CacheMapKeys.FUNCTIONS).clear();Also applies to: 151-153, 156-158
74-84: Keep this service cache-implementation-agnostic; avoidgetNativeCache()andinstanceof Map.Relying on the native Map couples you to
GenericMapCacheand its thread-safety characteristics. UsingCacheAPIs (get(key, Callable),evict,clear) preserves portability and simplifies reasoning.Also applies to: 89-99, 142-143
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheMapKeys.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java(4 hunks)application-engine/src/main/resources/application.yaml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java (1)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheMapKeys.java (1)
CacheMapKeys(3-7)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (2)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheMapKeys.java (1)
CacheMapKeys(3-7)application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (1)
Slf4j(8-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
- GitHub Check: task-list-completed
🔇 Additional comments (3)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java (2)
67-70: CacheManager wiring LGTM.Unified
SimpleCacheManagerwith default and specialized caches is clean and extensible.
37-41: Exclude specialized cache names from default caches. Filter outCacheMapKeys.ACTIONS,CacheMapKeys.FUNCTIONSandCacheMapKeys.NAMESPACE_FUNCTIONSfrom theproperties.getAllCaches()stream before mapping toConcurrentMapCacheto avoid registering those caches twice.application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (1)
28-37: Verify FieldActionsCacheService bean registration
Confirm that FieldActionsCacheService is annotated (e.g. @service or @component) or declared via a @bean method so Spring will pick up the new constructor signature, and search your configuration for any @bean methods or manual instantiations of FieldActionsCacheService using the old two-arg constructor.
...ation-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java
Show resolved
Hide resolved
...ation-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java
Outdated
Show resolved
Hide resolved
...ation-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java
Outdated
Show resolved
Hide resolved
.../src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
Outdated
Show resolved
Hide resolved
.../src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
Outdated
Show resolved
Hide resolved
.../src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
Show resolved
Hide resolved
| Cache functionsCache = cacheManager.getCache(CacheMapKeys.FUNCTIONS); | ||
| Object nativeFunctionsCache = functionsCache.getNativeCache(); | ||
|
|
||
| if (nativeFunctionsCache instanceof Map<?, ?> map) { | ||
| functions.forEach(function -> { | ||
| if (!map.containsKey(function.getStringId())) { | ||
| functionsCache.put(function.getStringId(), CachedFunction.build(shell, function)); | ||
| } | ||
| cachedFunctions.add((CachedFunction) functionsCache.get(function.getStringId()).get()); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid native Map access in getCachedFunctions; compute-if-absent via Cache API.
Works with any Cache impl and removes silent empty-return if native cache isn’t a Map.
- Cache functionsCache = cacheManager.getCache(CacheMapKeys.FUNCTIONS);
- Object nativeFunctionsCache = functionsCache.getNativeCache();
-
- if (nativeFunctionsCache instanceof Map<?, ?> map) {
- functions.forEach(function -> {
- if (!map.containsKey(function.getStringId())) {
- functionsCache.put(function.getStringId(), CachedFunction.build(shell, function));
- }
- cachedFunctions.add((CachedFunction) functionsCache.get(function.getStringId()).get());
- });
- }
+ Cache functionsCache = getRequiredCache(CacheMapKeys.FUNCTIONS);
+ for (Function function : functions) {
+ CachedFunction cf = functionsCache.get(function.getStringId(),
+ () -> CachedFunction.build(shell, function));
+ cachedFunctions.add(cf);
+ }Committable suggestion skipped: line range outside the PR's diff.
| exclude: shutdown | ||
| include: "*" | ||
| endpoint: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Actuator exposure is fully open; combined with anonymous '/manage/' this is a security risk.**
include: "*" exposes sensitive endpoints (env, beans, heapdump, caches). Your config also lists /manage/** under anonymous exceptions (Lines 87–88), making these endpoints publicly accessible.
Apply both changes:
- anonymous-exceptions: /api/auth/signup,/api/auth/token/verify,/api/auth/reset,/api/auth/recover,/manage/**
+ anonymous-exceptions: /api/auth/signup,/api/auth/token/verify,/api/auth/reset,/api/auth/recover exposure:
exclude: shutdown
- include: "*"
+ include: health,info,metrics,prometheus,cachesOptional hardening:
- Require auth/role for /manage/** or move management to a separate port.
- Only include the endpoints you actually need in non-dev profiles.
🤖 Prompt for AI Agents
In application-engine/src/main/resources/application.yaml around lines 119–121
(and note anonymous /manage/** at lines 87–88), the actuator exposure is set to
include: "*" which together with anonymous access to /manage/** exposes
sensitive endpoints; change the actuator exposure to list only the required
endpoints (e.g., health, info, metrics) instead of "*" and remove or restrict
the anonymous exception for /manage/**—either require authentication/appropriate
role for /manage/** or move management endpoints to a separate management port;
make these config edits in the relevant profile(s) and/or add security
constraints so management endpoints are not publicly accessible.
coderabbitai
bot
added
improvement
There was a problem hiding this comment.
Actionable comments posted: 19
♻️ Duplicate comments (5)
application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (1)
15-15: Make map-backed cache thread-safe; compute-and-store must be atomic.Backed by a plain Map with unsynchronized mutations; races with put/evict/clear and get(loader). Clear also reassigns the map mid-flight. Synchronize mutations and use double-checked locking in get(loader).
public abstract class GenericMapCache<V> implements Cache { @@ - protected volatile Map<String, V> map; + protected volatile Map<String, V> map; + private final Object monitor = new Object(); @@ - @Override - public <T> T get(Object key, java.util.concurrent.Callable<T> loader) { + @Override + public <T> T get(Object key, java.util.concurrent.Callable<T> loader) { String stringKey = String.valueOf(key); - Object present = map.get(stringKey); + Object present = map.get(stringKey); if (present != null) { return (T) present; } - try { - T computed = loader.call(); - if (computed != null) { - map.put(stringKey, safeCast(computed)); - } - return computed; - } catch (Exception ex) { - throw new org.springframework.cache.Cache.ValueRetrievalException(stringKey, loader, ex); - } + synchronized (monitor) { + present = map.get(stringKey); + if (present != null) { + return (T) present; + } + try { + T computed = loader.call(); + if (computed != null) { + map.put(stringKey, safeCast(computed)); + } + return computed; + } catch (Exception ex) { + throw new org.springframework.cache.Cache.ValueRetrievalException(stringKey, loader, ex); + } + } } @@ - public void put(Object key, Object value) { - map.put(String.valueOf(key), safeCast(value)); + public void put(Object key, Object value) { + synchronized (monitor) { + map.put(String.valueOf(key), safeCast(value)); + } } @@ - public void evict(Object key) { - map.remove(String.valueOf(key)); + public void evict(Object key) { + synchronized (monitor) { + map.remove(String.valueOf(key)); + } } @@ - public void clear() { - this.map = mapFactory.get(); + public void clear() { + synchronized (monitor) { + this.map = mapFactory.get(); + } log.info("{} cache cleared", this.getName()); }Also applies to: 33-51, 53-67
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (4)
55-62: Fail fast on missing cache; avoid NPE.- Cache namespaceFunctionsCache = cacheManager.getCache(CacheMapKeys.NAMESPACE_FUNCTIONS); + Cache namespaceFunctionsCache = getRequiredCache(CacheMapKeys.NAMESPACE_FUNCTIONS); @@ - namespaceFunctionsCache.evictIfPresent(petriNet.getIdentifier()); + namespaceFunctionsCache.evictIfPresent(petriNet.getIdentifier());Support code (add once in this class):
private Cache getRequiredCache(String name) { Cache cache = cacheManager.getCache(name); if (cache == null) throw new IllegalStateException("Cache '" + name + "' not configured"); return cache; }
65-69: Guard cache retrieval and reload atomically.- public void reloadCachedFunctions(String petriNetId) { - cacheManager.getCache(CacheMapKeys.NAMESPACE_FUNCTIONS).evictIfPresent(petriNetId); - cachePetriNetFunctions(petriNetService.getNewestVersionByIdentifier(petriNetId)); - } + public void reloadCachedFunctions(String petriNetId) { + getRequiredCache(CacheMapKeys.NAMESPACE_FUNCTIONS).evictIfPresent(petriNetId); + cachePetriNetFunctions(petriNetService.getNewestVersionByIdentifier(petriNetId)); + }
79-89: Use Cache APIs; avoid peeking into native Map and eliminate TOCTOU.- Cache actionsCache = cacheManager.getCache(CacheMapKeys.ACTIONS); - Object nativeActionsCache = actionsCache.getNativeCache(); - - if (nativeActionsCache instanceof Map<?, ?> map) { - if (shouldRewriteCachedActions || !map.containsKey(stringId) ) { - Closure code = (Closure) shell.evaluate("{-> " + action.getDefinition() + "}"); - actionsCache.put(stringId, code); - } - } - return (Closure) actionsCache.get(stringId).get(); + Cache actionsCache = getRequiredCache(CacheMapKeys.ACTIONS); + if (shouldRewriteCachedActions) { + actionsCache.evict(stringId); + } + return actionsCache.get(stringId, + () -> (Closure) shell.evaluate("{-> " + action.getDefinition() + "}"));
93-105: Compute functions atomically and avoid native cache access.- Cache functionsCache = cacheManager.getCache(CacheMapKeys.FUNCTIONS); - Object nativeFunctionsCache = functionsCache.getNativeCache(); - - if (nativeFunctionsCache instanceof Map<?, ?> map) { - functions.forEach(function -> { - if (!map.containsKey(function.getStringId())) { - functionsCache.put(function.getStringId(), CachedFunction.build(shell, function)); - } - cachedFunctions.add((CachedFunction) functionsCache.get(function.getStringId()).get()); - }); - } + Cache functionsCache = getRequiredCache(CacheMapKeys.FUNCTIONS); + for (Function function : functions) { + CachedFunction cf = functionsCache.get(function.getStringId(), + () -> CachedFunction.build(shell, function)); + cachedFunctions.add(cf); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
application-engine/src/main/java/com/netgrif/application/engine/configuration/ActionsMapCache.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/FunctionsMapCache.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/FunctionsNamespaceMapCache.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java(4 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IFieldActionsCacheService.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-31T23:40:46.499Z
Learnt from: tuplle
PR: netgrif/application-engine#334
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java:204-214
Timestamp: 2025-07-31T23:40:46.499Z
Learning: In the PetriNetService.importPetriNet method, existingNet.getVersion() cannot be null because all existing nets in the system were deployed through processes that ensure every net always has a version assigned.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java
🧬 Code graph analysis (3)
application-engine/src/main/java/com/netgrif/application/engine/configuration/ActionsMapCache.java (2)
application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (1)
Slf4j(10-80)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (1)
Slf4j(66-581)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (4)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheMapKeys.java (1)
CacheMapKeys(3-7)application-engine/src/main/java/com/netgrif/application/engine/configuration/ActionsMapCache.java (1)
Slf4j(10-41)application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (1)
Slf4j(10-80)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (1)
Slf4j(66-581)
application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (1)
application-engine/src/main/java/com/netgrif/application/engine/configuration/ActionsMapCache.java (1)
Slf4j(10-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (6)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java (1)
4-4: No action on wildcard import.Acceptable here unless the project enforces “no wildcard imports”.
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (2)
8-8: No action on wildcard import.Safe refactor.
558-569: Add index on functions._id in petriNet collection
The query in PetriNetService.findByFunctionId filters on functions._id; without an index this will trigger a full collection scan. Verify (via @indexed on Function.objectId, a database migration, or manual index setup) that an index exists for functions._id.application-engine/src/main/java/com/netgrif/application/engine/configuration/FunctionsNamespaceMapCache.java (1)
15-17: Ignore namespace vs PetriNet ID key concernAll cache operations on
NAMESPACE_FUNCTIONSusepetriNet.getIdentifier()as the key (see FieldActionsCacheService.java:55 and 67), so the cache is intentionally keyed by PetriNet ID. No changes required.Likely an incorrect or invalid review comment.
application-engine/src/main/java/com/netgrif/application/engine/configuration/ActionsMapCache.java (1)
13-13: Confirm reload key semantics
reloadCachedFunctions(stringKey)presumesstringKeyis the Petri net identifier the service expects. Please confirm the key format aligns withIFieldActionsCacheService’s contract; otherwise, misses will never populate.I can generate a short repo scan to locate all
reloadCachedFunctions(...)overloads and usages to confirm the expected key shape. Do you want that?application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java (1)
46-50: Filtering specialized cache names is unnecessary CacheConfigurationProperties.getAllCaches() returns only the default caches (petriNetById,petriNetByIdentifier,petriNetNewest,petriNetCache,loadedModules) and any user-added names—it never includesCacheMapKeys.ACTIONS,CacheMapKeys.FUNCTIONS, orCacheMapKeys.NAMESPACE_FUNCTIONS, so no duplicate registrations can occur by default.Likely an incorrect or invalid review comment.
...ation-engine/src/main/java/com/netgrif/application/engine/configuration/ActionsMapCache.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public ValueWrapper get(Object key) { | ||
| String stringKey = String.valueOf(key); | ||
|
|
||
| Object valueObject = map.get(stringKey); | ||
| if (valueObject != null) { | ||
| return new org.springframework.cache.support.SimpleValueWrapper(valueObject); | ||
| } | ||
| fieldActionsCacheService.reloadCachedFunctions(stringKey); | ||
| return new org.springframework.cache.support.SimpleValueWrapper(map.get(stringKey)); | ||
| } |
There was a problem hiding this comment.
Cache.get(Object) should return null on miss; don’t wrap null
Returning SimpleValueWrapper(null) violates the Cache contract. Return null when the key is absent or the reload doesn’t populate the key.
@Override
public ValueWrapper get(Object key) {
String stringKey = String.valueOf(key);
Object valueObject = map.get(stringKey);
if (valueObject != null) {
return new org.springframework.cache.support.SimpleValueWrapper(valueObject);
}
fieldActionsCacheService.reloadCachedFunctions(stringKey);
- return new org.springframework.cache.support.SimpleValueWrapper(map.get(stringKey));
+ Object refreshed = map.get(stringKey);
+ return refreshed != null
+ ? new org.springframework.cache.support.SimpleValueWrapper(refreshed)
+ : null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public ValueWrapper get(Object key) { | |
| String stringKey = String.valueOf(key); | |
| Object valueObject = map.get(stringKey); | |
| if (valueObject != null) { | |
| return new org.springframework.cache.support.SimpleValueWrapper(valueObject); | |
| } | |
| fieldActionsCacheService.reloadCachedFunctions(stringKey); | |
| return new org.springframework.cache.support.SimpleValueWrapper(map.get(stringKey)); | |
| } | |
| @Override | |
| public ValueWrapper get(Object key) { | |
| String stringKey = String.valueOf(key); | |
| Object valueObject = map.get(stringKey); | |
| if (valueObject != null) { | |
| return new org.springframework.cache.support.SimpleValueWrapper(valueObject); | |
| } | |
| fieldActionsCacheService.reloadCachedFunctions(stringKey); | |
| Object refreshed = map.get(stringKey); | |
| return refreshed != null | |
| ? new org.springframework.cache.support.SimpleValueWrapper(refreshed) | |
| : null; | |
| } |
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/configuration/ActionsMapCache.java
around lines 17 to 27, the get(Object) implementation currently returns new
SimpleValueWrapper(null) on misses which violates the Cache contract; modify the
method to convert the key to string, try map.get(stringKey) and if non-null
return a SimpleValueWrapper, otherwise call
fieldActionsCacheService.reloadCachedFunctions(stringKey), then re-check
map.get(stringKey) and return a SimpleValueWrapper only if that second lookup is
non-null; if still null return null (do not wrap null).
| public <T> T get(Object key, Class<T> type) { | ||
| String stringKey = String.valueOf(key); | ||
| Object valueObject = map.get(stringKey); | ||
|
|
||
| if (valueObject != null) { | ||
| return type.cast(valueObject); | ||
| } | ||
|
|
||
| fieldActionsCacheService.reloadCachedFunctions(stringKey); | ||
| return type.cast(map.get(stringKey)); | ||
|
|
||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Override typed get; mirror null semantics after reload
Add @Override and return null when still missing.
- public <T> T get(Object key, Class<T> type) {
+ @Override
+ public <T> T get(Object key, Class<T> type) {
String stringKey = String.valueOf(key);
Object valueObject = map.get(stringKey);
if (valueObject != null) {
return type.cast(valueObject);
}
fieldActionsCacheService.reloadCachedFunctions(stringKey);
- return type.cast(map.get(stringKey));
+ Object refreshed = map.get(stringKey);
+ return refreshed != null ? type.cast(refreshed) : null;
}🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/configuration/ActionsMapCache.java
around lines 29 to 40, add the @Override annotation to the get method and change
the logic so that after calling
fieldActionsCacheService.reloadCachedFunctions(stringKey) you read the map again
into a local variable, return null if that lookup is still missing, and only
call type.cast(...) when the looked-up value is non-null to preserve the
original null semantics and avoid a ClassCastException.
...on-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java
Outdated
Show resolved
Hide resolved
| Supplier<Map<String, CachedFunction>> functionsFactory = () -> new MaxSizeHashMap<>(fieldRunnerProperties.getFunctionsCacheSize()); | ||
|
|
||
| caches.add(new FunctionsMapCache( | ||
| CacheMapKeys.FUNCTIONS, | ||
| functionsFactory, | ||
| fieldActionsCacheService, | ||
| petriNetService | ||
| )); | ||
|
|
There was a problem hiding this comment.
Align FUNCTIONS cache with stored type from FunctionsMapCache
With FunctionsMapCache storing Function, adjust the factory’s type accordingly.
- Supplier<Map<String, CachedFunction>> functionsFactory = () -> new MaxSizeHashMap<>(fieldRunnerProperties.getFunctionsCacheSize());
+ Supplier<Map<String, Function>> functionsFactory = () -> new MaxSizeHashMap<>(fieldRunnerProperties.getFunctionsCacheSize());
@@
- caches.add(new FunctionsMapCache(
+ caches.add(new FunctionsMapCache(
CacheMapKeys.FUNCTIONS,
functionsFactory,
fieldActionsCacheService,
petriNetService
));Ensure you import com.netgrif.application.engine.objects.petrinet.domain.Function.
@@
-import com.netgrif.application.engine.workflow.domain.CachedFunction;
+import com.netgrif.application.engine.objects.petrinet.domain.Function;Note: Keep the CachedFunction import for the namespace cache below.
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java
around lines 61 to 69, the Supplier factory is typed as Supplier<Map<String,
CachedFunction>> but FunctionsMapCache stores Function instances; change the
factory type to Supplier<Map<String,
com.netgrif.application.engine.objects.petrinet.domain.Function>> (and update
the MaxSizeHashMap generic accordingly) and add an import for
com.netgrif.application.engine.objects.petrinet.domain.Function; keep the
existing CachedFunction import for the namespace cache below.
| /** | ||
| * Finds and returns a single function subdocument from the {@code petriNet} collection | ||
| * by its nested {@code functions._id}. | ||
| * | ||
| * @param functionId the string form of the function's ObjectId (24-hex) | ||
| * @return the matching {@code Function} subdocument, or {@code null} if not found | ||
| * @throws IllegalArgumentException if {@code functionId} is not a valid ObjectId | ||
| */ | ||
| Function findByFunctionId(String functionId); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Return nullability + index requirement for fast lookups by nested functions._id.
- Mark the return as nullable to match the “null if not found” contract.
- Add an index on petriNet.functions._id; without it, lookups will scan the whole collection at scale.
Apply near the signature:
- Function findByFunctionId(String functionId);
+ @org.springframework.lang.Nullable
+ Function findByFunctionId(String functionId);Create the index during startup (e.g., in configuration):
// Example (place in a @Configuration initializer)
mongoTemplate.indexOps("petriNet")
.ensureIndex(new org.springframework.data.mongodb.core.index.Index()
.on("functions._id", org.springframework.data.domain.Sort.Direction.ASC));Would you like me to open a follow-up for the index migration?
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java
around lines 337 to 345, the method signature and docs promise "null if not
found" but the return isn't marked nullable and there is no note about indexing
for performance; update the method signature to indicate the return can be null
(use your codebase's Nullable annotation or JSR305/@Nullable) and update the
Javadoc accordingly, and add a startup configuration that ensures an ascending
index on petriNet.functions._id (create the index via your
MongoTemplate/IndexOps during application startup) so nested lookups are
efficient at scale.
| @Override | ||
| public Function findByFunctionId(String functionId) { | ||
| Query query = new Query(); | ||
|
|
||
| query.addCriteria(Criteria.where("functions._id").is(new ObjectId(functionId))); | ||
|
|
||
| PetriNet petriNet = mongoTemplate.findOne(query, PetriNet.class, "petriNet"); | ||
|
|
||
| Optional<Function> optionalFunction = petriNet.getFunctions().stream().filter(function -> function.getObjectId().toString().equals(functionId)).findFirst(); | ||
|
|
||
| return optionalFunction.orElse(null); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix NPE on miss and validate ObjectId; project only the matched function.
Current code NPEs when no PetriNet matches and accepts invalid ids. Also loads all functions; use elemMatch to project the matched one only.
Apply:
@@
@Override
public Function findByFunctionId(String functionId) {
- Query query = new Query();
-
- query.addCriteria(Criteria.where("functions._id").is(new ObjectId(functionId)));
-
- PetriNet petriNet = mongoTemplate.findOne(query, PetriNet.class, "petriNet");
-
- Optional<Function> optionalFunction = petriNet.getFunctions().stream().filter(function -> function.getObjectId().toString().equals(functionId)).findFirst();
-
- return optionalFunction.orElse(null);
+ if (functionId == null || functionId.isBlank() || !ObjectId.isValid(functionId)) {
+ throw new IllegalArgumentException("Invalid functionId: " + functionId);
+ }
+ ObjectId oid = new ObjectId(functionId);
+ Query query = new Query(Criteria.where("functions._id").is(oid));
+ // project only the matched array element to reduce payload
+ query.fields().elemMatch("functions", Criteria.where("_id").is(oid));
+
+ PetriNet petriNet = mongoTemplate.findOne(query, PetriNet.class, "petriNet");
+ if (petriNet == null || petriNet.getFunctions() == null || petriNet.getFunctions().isEmpty()) {
+ return null;
+ }
+ // after elemMatch the first item is the matched function
+ Function f = petriNet.getFunctions().getFirst();
+ return (f != null && oid.equals(f.getObjectId())) ? f : null;
}Committable suggestion skipped: line range outside the PR's diff.
| return new HashMap<>((Map) cacheManager.getCache(CacheMapKeys.NAMESPACE_FUNCTIONS).getNativeCache()); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Return a safe, typed copy of the namespace function cache.
- return new HashMap<>((Map) cacheManager.getCache(CacheMapKeys.NAMESPACE_FUNCTIONS).getNativeCache());
+ @SuppressWarnings("unchecked")
+ Map<String, List<CachedFunction>> nativeMap =
+ (Map<String, List<CachedFunction>>) getRequiredCache(CacheMapKeys.NAMESPACE_FUNCTIONS).getNativeCache();
+ return new HashMap<>(nativeMap);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return new HashMap<>((Map) cacheManager.getCache(CacheMapKeys.NAMESPACE_FUNCTIONS).getNativeCache()); | |
| } | |
| @SuppressWarnings("unchecked") | |
| Map<String, List<CachedFunction>> nativeMap = | |
| (Map<String, List<CachedFunction>>) getRequiredCache(CacheMapKeys.NAMESPACE_FUNCTIONS).getNativeCache(); | |
| return new HashMap<>(nativeMap); | |
| } |
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
around lines 147-148, the method currently returns an unchecked raw-cast of the
native cache which is unsafe and may return the underlying map directly; change
it to obtain the cache object, null-check it, cast it to the expected generic
type (e.g. Map<String, YourValueType>), and return a new typed HashMap copy (or
Collections.emptyMap() when null) to ensure type safety and avoid exposing the
native cache instance.
.../src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
Outdated
Show resolved
Hide resolved
| void reloadCachedFunctions(String petriNetId); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Document the new overload and define its contract (evict vs. rebuild).
Clarify whether this method evicts and then rebuilds, or only rebuilds. Also specify expected ID format (24-hex) and error behavior.
Apply Javadoc near the signature:
+ /**
+ * Reloads (evict + rebuild) cached functions for the given PetriNet ID.
+ * The ID must be a valid 24-hex ObjectId.
+ * @throws IllegalArgumentException if {@code petriNetId} is invalid or the PetriNet does not exist
+ */
void reloadCachedFunctions(String petriNetId);🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IFieldActionsCacheService.java
around lines 16 to 17, add Javadoc to the reloadCachedFunctions(String
petriNetId) overload that (1) states its contract clearly — whether it evicts
the existing cache entry and rebuilds it or only rebuilds in-place (choose and
document one behavior explicitly), (2) documents the expected petriNetId format
(24-character lowercase hex string) and validation behavior, (3) documents error
behavior (throw IllegalArgumentException for invalid id format,
NoSuchElementException if the referenced Petri net does not exist, and
Propagate/Log RuntimeException for rebuild failures), and (4) include any
thread-safety and performance notes (e.g., this method is blocking and may
acquire cache lock). Ensure the Javadoc is concise and placed immediately above
the method signature to define the contract unambiguously.
|
There are test errors |
| .collect(Collectors.toCollection(ArrayList::new)); | ||
|
|
||
|
|
||
| Supplier<Map<String, Closure>> actionsFactory = () -> new MaxSizeHashMap<>(fieldRunnerProperties.getActionCacheSize()); |
There was a problem hiding this comment.
MaxSizeHashMap is not a thread safe collection. Multiple threads can read and write to the cache at the same time
There was a problem hiding this comment.
Thanks for pointing this out, I’ll fix it.
…e mutation on get or put
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (9)
application-engine/src/main/java/com/netgrif/application/engine/configuration/FunctionsNamespaceMapCache.java (2)
21-32: Honor Cache.get contract; avoid NPE; remove unnecessary PetriNet fetch; handle contention.
- Return null on miss, not ValueWrapper(null).
- Use reload-by-id API; fetching PetriNet is unnecessary.
- Prevent thundering herd by delegating to GenericMapCache.get(key, loader).
Apply:
@Override - public ValueWrapper get(Object key) { - String stringKey = String.valueOf(key); - - Object valueObject = map().get(stringKey); - if (valueObject != null) { - return new SimpleValueWrapper(List.copyOf((List<CachedFunction>) valueObject)); - } - PetriNet petriNet = petriNetService.getPetriNet(stringKey); - fieldActionsCacheService.cachePetriNetFunctions(petriNet); - return new SimpleValueWrapper(List.copyOf((List<CachedFunction>) map().get(stringKey))); - } + public ValueWrapper get(Object key) { + if (key == null) throw new IllegalArgumentException("Cache key must not be null"); + final String stringKey = key.toString(); + List<CachedFunction> value = this.get(stringKey, () -> { + fieldActionsCacheService.reloadCachedFunctions(stringKey); + List<CachedFunction> v = (List<CachedFunction>) map().get(stringKey); + return v == null ? null : List.copyOf(v); + }); + return value == null ? null : new SimpleValueWrapper(value); + }
34-46: Override typed get; mirror null-miss semantics; avoid PetriNet fetch.Also add key validation and contention-safe loader.
Apply:
- public <T> T get(Object key, Class<T> type) { - String stringKey = String.valueOf(key); - Object valueObject = map().get(stringKey); - - if (valueObject != null) { - return type.cast(List.copyOf((List<CachedFunction>) valueObject)); - } - - PetriNet petriNet = petriNetService.getPetriNet(stringKey); - fieldActionsCacheService.cachePetriNetFunctions(petriNet); - return type.cast(List.copyOf((List<CachedFunction>) map().get(stringKey))); - - } + @Override + public <T> T get(Object key, Class<T> type) { + if (key == null) throw new IllegalArgumentException("Cache key must not be null"); + final String stringKey = key.toString(); + List<CachedFunction> value = this.get(stringKey, () -> { + fieldActionsCacheService.reloadCachedFunctions(stringKey); + List<CachedFunction> v = (List<CachedFunction>) map().get(stringKey); + return v == null ? null : List.copyOf(v); + }); + return value == null ? null : type.cast(value); + }application-engine/src/main/java/com/netgrif/application/engine/configuration/FunctionsMapCache.java (3)
15-22: Parameterize the cache; align with stored type; add @OverRide imports not needed.Removes raw type usage and keeps type-safety.
Apply:
-@Slf4j -public class FunctionsMapCache extends GenericMapCache { +@Slf4j +public class FunctionsMapCache extends GenericMapCache<CachedFunction> { @@ - public FunctionsMapCache(String name, java.util.function.Supplier<Map<String, CachedFunction>> mapFactory, IFieldActionsCacheService fieldActionsCacheService, IPetriNetService petriNetService, IGroovyShellFactory shellFactory) { - super(name, CachedFunction.class, mapFactory, fieldActionsCacheService, petriNetService); + public FunctionsMapCache(String name, + java.util.function.Supplier<Map<String, CachedFunction>> mapFactory, + IFieldActionsCacheService fieldActionsCacheService, + IPetriNetService petriNetService, + IGroovyShellFactory shellFactory) { + super(name, CachedFunction.class, mapFactory, fieldActionsCacheService, petriNetService); this.shell = shellFactory.getGroovyShell(); }
25-40: Fix miss semantics; avoid double build; use super.put; validate key.
- Return null when absent.
- Build once on miss; reuse built instance for storage and response (or consistently return a copy).
Apply:
@Override public synchronized ValueWrapper get(Object key) { - String stringKey = String.valueOf(key); + if (key == null) throw new IllegalArgumentException("Cache key must not be null"); + String stringKey = key.toString(); Object valueObject = map().get(stringKey); if (valueObject != null) { - return new SimpleValueWrapper(CachedFunction.copyOf(shell, (CachedFunction) valueObject)); + return new SimpleValueWrapper(CachedFunction.copyOf(shell, (CachedFunction) valueObject)); } Function function = petriNetService.findByFunctionId(stringKey); if (function != null) { - map().put(stringKey, CachedFunction.build(shell, function)); - return new SimpleValueWrapper(CachedFunction.build(shell, function)); + CachedFunction built = CachedFunction.build(shell, function); + super.put(stringKey, built); + return new SimpleValueWrapper(CachedFunction.copyOf(shell, built)); } - return new SimpleValueWrapper(null); + return null; }
42-57: Override typed get; unify semantics with untyped; return null on miss.Also avoid double build and write via super.put.
Apply:
- public synchronized <T> T get(Object key, Class<T> type) { - String stringKey = String.valueOf(key); + @Override + public synchronized <T> T get(Object key, Class<T> type) { + if (key == null) throw new IllegalArgumentException("Cache key must not be null"); + String stringKey = key.toString(); Object valueObject = map().get(stringKey); if (valueObject != null) { - return type.cast(valueObject); + return type.cast(CachedFunction.copyOf(shell, (CachedFunction) valueObject)); } Function function = petriNetService.findByFunctionId(stringKey); if (function != null) { - map().put(stringKey, CachedFunction.build(shell, function)); - return type.cast(CachedFunction.build(shell, function)); + CachedFunction built = CachedFunction.build(shell, function); + super.put(stringKey, built); + return type.cast(CachedFunction.copyOf(shell, built)); } - return type.cast(null); + return null; }application-engine/src/main/java/com/netgrif/application/engine/configuration/ActionsMapCache.java (3)
12-12: Parameterize the cache with Closure to eliminate raw typesExtend
GenericMapCache<Closure>for compile-time safety.- public class ActionsMapCache extends GenericMapCache { + public class ActionsMapCache extends GenericMapCache<Closure> {
22-28: Honor Cache contract: return null on miss, don’t wrap nullWrapping
nullinSimpleValueWrapperviolates Spring’sCachecontract.Object valueObject = map().get(stringKey); if (valueObject != null) { return new SimpleValueWrapper(valueObject); } fieldActionsCacheService.reloadCachedFunctions(stringKey); - return new SimpleValueWrapper(map().get(stringKey)); + Object refreshed = map().get(stringKey); + return refreshed != null ? new SimpleValueWrapper(refreshed) : null;
30-41: Add @OverRide and mirror null semantics for typed getAlso avoid
type.cast(null)and returnnulldirectly.- public synchronized <T> T get(Object key, Class<T> type) { + @Override + public synchronized <T> T get(Object key, Class<T> type) { String stringKey = String.valueOf(key); Object valueObject = map().get(stringKey); if (valueObject != null) { return type.cast(valueObject); } fieldActionsCacheService.reloadCachedFunctions(stringKey); - return type.cast(map().get(stringKey)); + Object refreshed = map().get(stringKey); + return refreshed != null ? type.cast(refreshed) : null; }Run to locate similar violations across caches:
#!/bin/bash rg -nP --type=java 'SimpleValueWrapper\\(null\\)|return\\s+type\\.cast\\(null\\)'application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (1)
17-18: Tighten generics: use Class and make it finalReduces unchecked casts and encodes the value type at the compiler level.
- protected Class<?> valueType; + protected final Class<V> valueType; @@ - public GenericMapCache(String name, Class<?> valueType, java.util.function.Supplier<Map<String, V>> mapFactory, IFieldActionsCacheService fieldActionsCacheService, IPetriNetService petriNetService) { + public GenericMapCache(String name, Class<V> valueType, java.util.function.Supplier<Map<String, V>> mapFactory, IFieldActionsCacheService fieldActionsCacheService, IPetriNetService petriNetService) {Also applies to: 26-26
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
application-engine/src/main/java/com/netgrif/application/engine/configuration/ActionsMapCache.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/FunctionsMapCache.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/FunctionsNamespaceMapCache.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/CachedFunction.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
application-engine/src/main/java/com/netgrif/application/engine/configuration/FunctionsMapCache.java (4)
application-engine/src/main/java/com/netgrif/application/engine/configuration/ActionsMapCache.java (1)
Slf4j(11-42)application-engine/src/main/java/com/netgrif/application/engine/configuration/FunctionsNamespaceMapCache.java (1)
Slf4j(14-53)application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (1)
Slf4j(14-104)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (1)
Slf4j(66-581)
application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (4)
application-engine/src/main/java/com/netgrif/application/engine/configuration/ActionsMapCache.java (1)
Slf4j(11-42)application-engine/src/main/java/com/netgrif/application/engine/configuration/FunctionsMapCache.java (1)
Slf4j(14-58)application-engine/src/main/java/com/netgrif/application/engine/configuration/FunctionsNamespaceMapCache.java (1)
Slf4j(14-53)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (1)
Slf4j(66-581)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java (1)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheMapKeys.java (1)
CacheMapKeys(3-7)
application-engine/src/main/java/com/netgrif/application/engine/configuration/ActionsMapCache.java (4)
application-engine/src/main/java/com/netgrif/application/engine/configuration/FunctionsMapCache.java (1)
Slf4j(14-58)application-engine/src/main/java/com/netgrif/application/engine/configuration/FunctionsNamespaceMapCache.java (1)
Slf4j(14-53)application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (1)
Slf4j(14-104)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (1)
Slf4j(66-581)
application-engine/src/main/java/com/netgrif/application/engine/configuration/FunctionsNamespaceMapCache.java (4)
application-engine/src/main/java/com/netgrif/application/engine/configuration/ActionsMapCache.java (1)
Slf4j(11-42)application-engine/src/main/java/com/netgrif/application/engine/configuration/FunctionsMapCache.java (1)
Slf4j(14-58)application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (1)
Slf4j(14-104)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (1)
Slf4j(66-581)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (1)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java (1)
53-82: Good: bounded maps wrapped with synchronizationWrapping
MaxSizeHashMapwithCollections.synchronizedMapaddresses immediate thread-safety concerns for shared caches.
| @Override | ||
| public synchronized ValueWrapper get(Object key) { |
There was a problem hiding this comment.
Fix return type reference: unresolved ValueWrapper
ValueWrapper is an inner type of org.springframework.cache.Cache; reference it properly to fix compilation.
- public synchronized ValueWrapper get(Object key) {
+ public synchronized org.springframework.cache.Cache.ValueWrapper get(Object key) {Add this import outside the shown range (either one of the two):
- import org.springframework.cache.Cache;
- or import org.springframework.cache.Cache.ValueWrapper;
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public synchronized ValueWrapper get(Object key) { | |
| @Override | |
| public synchronized org.springframework.cache.Cache.ValueWrapper get(Object key) { |
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/configuration/ActionsMapCache.java
around lines 18-19, the method signature uses ValueWrapper but the type is
unresolved because ValueWrapper is an inner type of
org.springframework.cache.Cache; fix by adding an import for the inner type
(import org.springframework.cache.Cache.ValueWrapper;) or import the outer type
(import org.springframework.cache.Cache;) and reference the type explicitly as
Cache.ValueWrapper in the signature; add the chosen import at the top of the
file (outside the shown range) so the method compiles.
...on-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java
Show resolved
Hide resolved
| Supplier<Map<String, Closure>> actionsFactory = | ||
| () -> Collections.synchronizedMap(new MaxSizeHashMap<>(fieldRunnerProperties.getActionCacheSize())); | ||
|
|
||
| caches.add(new ActionsMapCache( | ||
| CacheMapKeys.ACTIONS, | ||
| actionsFactory, | ||
| fieldActionsCacheService, | ||
| petriNetService | ||
| )); | ||
|
|
||
| Supplier<Map<String, CachedFunction>> functionsFactory = | ||
| () -> Collections.synchronizedMap(new MaxSizeHashMap<>(fieldRunnerProperties.getFunctionsCacheSize())); | ||
|
|
||
| caches.add(new FunctionsMapCache( | ||
| CacheMapKeys.FUNCTIONS, | ||
| functionsFactory, | ||
| fieldActionsCacheService, | ||
| petriNetService, | ||
| groovyShellFactory | ||
| )); | ||
|
|
||
| Supplier<Map<String, List<CachedFunction>>> nsFactory = | ||
| () -> Collections.synchronizedMap(new MaxSizeHashMap<>(fieldRunnerProperties.getNamespaceCacheSize())); | ||
|
|
||
| caches.add(new FunctionsNamespaceMapCache( | ||
| CacheMapKeys.NAMESPACE_FUNCTIONS, | ||
| nsFactory, | ||
| fieldActionsCacheService, | ||
| petriNetService | ||
| )); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider Caffeine for production-grade bounded caches
Caffeine gives O(1) concurrency, admission/eviction policies, and metrics; wire CaffeineCache instances into SimpleCacheManager as a follow-up.
| Object valueObject = map().get(stringKey); | ||
| if (valueObject != null) { | ||
| return new SimpleValueWrapper(CachedFunction.copyOf(shell, (CachedFunction) valueObject)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider eliminating per-call copyOf for performance.
Re-evaluating Groovy on every hit undermines caching. If thread-safety allows, return the stored CachedFunction directly, or cache compiled classes and rehydrate closures cheaply. If you must copy, consider Closure#rehydrate over full re-eval.
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/configuration/FunctionsMapCache.java
around lines 28-31, the code currently calls CachedFunction.copyOf(...) on every
cache hit which forces Groovy re-evaluation and kills performance; instead, if
CachedFunction is immutable or can be made thread-safe, return the stored
CachedFunction directly from the cache; otherwise change the cache to store a
compiled/rehydratable Closure or class and create lightweight per-call wrappers
via Closure.rehydrate (or a cached compiled form) rather than full re-eval, or
implement copy-on-write so copies are made only on mutating calls — pick one
approach, make CachedFunction immutable or mark it thread-safe, update the cache
return to return the stored instance when safe, or update the cache value to a
compiled/rehydratable closure and create lightweight closures on read.
| public class FunctionsNamespaceMapCache extends GenericMapCache { | ||
|
|
||
| public FunctionsNamespaceMapCache(String name, java.util.function.Supplier<Map<String, List<CachedFunction>>> mapFactory, IFieldActionsCacheService fieldActionsCacheService, IPetriNetService petriNetService) { | ||
| super(name, List.class, mapFactory, fieldActionsCacheService, petriNetService); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parameterize the cache type; avoid raw GenericMapCache.
Prevents unsafe casts and lets GenericMapCache.safeCast help.
Apply:
-@Slf4j
-public class FunctionsNamespaceMapCache extends GenericMapCache {
+@Slf4j
+public class FunctionsNamespaceMapCache extends GenericMapCache<List<CachedFunction>> {
@@
- public FunctionsNamespaceMapCache(String name, java.util.function.Supplier<Map<String, List<CachedFunction>>> mapFactory, IFieldActionsCacheService fieldActionsCacheService, IPetriNetService petriNetService) {
- super(name, List.class, mapFactory, fieldActionsCacheService, petriNetService);
- }
+ public FunctionsNamespaceMapCache(String name,
+ java.util.function.Supplier<Map<String, List<CachedFunction>>> mapFactory,
+ IFieldActionsCacheService fieldActionsCacheService,
+ IPetriNetService petriNetService) {
+ super(name, List.class, mapFactory, fieldActionsCacheService, petriNetService);
+ }🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/configuration/FunctionsNamespaceMapCache.java
around lines 15–19, the class currently extends the raw GenericMapCache which
causes unsafe casts; change the class to extend GenericMapCache<Map<String,
List<CachedFunction>>> and update the constructor signature to accept
Supplier<Map<String, List<CachedFunction>>>; in the super(...) call replace the
raw List.class with a properly typed Class for Map (use
GenericMapCache.safeCast(Map.class) or an equivalent safeCast to produce a
Class<Map<String, List<CachedFunction>>>), ensuring all generics line up so no
raw types remain and unsafe casting is avoided.
| @Override | ||
| public void put(Object key, Object value) { | ||
| String k = String.valueOf(key); | ||
| map().put(k, List.copyOf((List<CachedFunction>) value)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use super.put and enforce immutability once.
Avoid bypassing safeCast; still store an unmodifiable List.
Apply:
@Override
public void put(Object key, Object value) {
- String k = String.valueOf(key);
- map().put(k, List.copyOf((List<CachedFunction>) value));
+ if (key == null) throw new IllegalArgumentException("Cache key must not be null");
+ super.put(key, List.copyOf((List<CachedFunction>) value));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public void put(Object key, Object value) { | |
| String k = String.valueOf(key); | |
| map().put(k, List.copyOf((List<CachedFunction>) value)); | |
| } | |
| @Override | |
| public void put(Object key, Object value) { | |
| if (key == null) throw new IllegalArgumentException("Cache key must not be null"); | |
| super.put(key, List.copyOf((List<CachedFunction>) value)); | |
| } |
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/configuration/FunctionsNamespaceMapCache.java
around lines 48-52, the current implementation bypasses safeCast and writes
directly to map(); change it to call super.put(key, value) and ensure
immutability exactly once: use the class's safeCast (or equivalent) to cast the
incoming value to List<CachedFunction>, wrap that result with List.copyOf(...)
to make it unmodifiable, and pass that wrapped list into super.put(key,
wrappedList) instead of map().put(...).
...ation-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java
Outdated
Show resolved
Hide resolved
...ation-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java
Show resolved
Hide resolved
| public static CachedFunction copyOf(GroovyShell shell, CachedFunction cachedFunction) { | ||
| if (cachedFunction == null) { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Guard against null input early (returning null here may mask bugs).
Prefer failing fast on null input or document the nullable contract. I recommend throwing IllegalArgumentException for null cachedFunction to avoid silent nulls propagating.
Apply:
- public static CachedFunction copyOf(GroovyShell shell, CachedFunction cachedFunction) {
- if (cachedFunction == null) {
- return null;
- }
+ public static CachedFunction copyOf(GroovyShell shell, CachedFunction cachedFunction) {
+ if (cachedFunction == null) {
+ throw new IllegalArgumentException("cachedFunction must not be null");
+ }🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/CachedFunction.java
around lines 27 to 31, the copyOf method currently returns null when
cachedFunction is null which can hide errors; change the method to fail fast by
throwing an IllegalArgumentException when cachedFunction is null (with a clear
message like "cachedFunction must not be null") instead of returning null so
callers immediately see the contract violation.
| Closure code = (Closure) shell.evaluate(cachedFunction.getFunction().getDefinition()); | ||
| if (code == null) { | ||
| throw new IllegalArgumentException("Non compilable function"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Harden evaluation: null/blank definition, non-Closure result, and exceptions.
Current code may NPE on null definition and ClassCastException on non-Closure. Wrap compilation errors consistently.
Apply:
- Closure code = (Closure) shell.evaluate(cachedFunction.getFunction().getDefinition());
- if (code == null) {
- throw new IllegalArgumentException("Non compilable function");
- }
+ String def = cachedFunction.getFunction() != null ? cachedFunction.getFunction().getDefinition() : null;
+ if (def == null || def.isBlank()) {
+ throw new IllegalArgumentException("Function definition must not be null/blank");
+ }
+ final Object evaluated;
+ try {
+ evaluated = shell.evaluate(def);
+ } catch (Throwable t) {
+ throw new IllegalArgumentException("Non compilable function", t);
+ }
+ if (!(evaluated instanceof Closure)) {
+ throw new IllegalArgumentException("Function definition did not evaluate to a Closure");
+ }
+ Closure code = (Closure) evaluated;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Closure code = (Closure) shell.evaluate(cachedFunction.getFunction().getDefinition()); | |
| if (code == null) { | |
| throw new IllegalArgumentException("Non compilable function"); | |
| } | |
| String def = cachedFunction.getFunction() != null | |
| ? cachedFunction.getFunction().getDefinition() | |
| : null; | |
| if (def == null || def.isBlank()) { | |
| throw new IllegalArgumentException("Function definition must not be null/blank"); | |
| } | |
| final Object evaluated; | |
| try { | |
| evaluated = shell.evaluate(def); | |
| } catch (Throwable t) { | |
| throw new IllegalArgumentException("Non compilable function", t); | |
| } | |
| if (!(evaluated instanceof Closure)) { | |
| throw new IllegalArgumentException("Function definition did not evaluate to a Closure"); | |
| } | |
| Closure code = (Closure) evaluated; |
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/CachedFunction.java
around lines 32 to 36, harden the evaluation: first validate
cachedFunction.getFunction().getDefinition() for null or blank and throw
IllegalArgumentException with a clear message if invalid; then call
shell.evaluate(...) inside a try/catch to catch and wrap any compilation/runtime
exceptions into an IllegalArgumentException that includes the original
exception; after evaluation verify the returned object is a Closure (not null or
another type) and if not throw IllegalArgumentException indicating non-Closure
result (including actual result type when available).
…namespaceFunctionsCache from local Map to Redis
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (8)
application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (1)
59-61: Fix compile error: qualify ValueRetrievalExceptionIt’s an inner type of Cache.
- } catch (Exception ex) { - throw new ValueRetrievalException(stringKey, loader, ex); + } catch (Exception ex) { + throw new Cache.ValueRetrievalException(stringKey, loader, ex); }application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (6)
58-65: Fail fast on missing namespace functions cacheUse a required-cache helper to avoid NPEs and produce clear errors.
- Cache namespaceFunctionsCache = cacheManager.getCache(properties.getNamespaceFunctions()); + Cache namespaceFunctionsCache = getRequiredCache(properties.getNamespaceFunctions());
149-151: Return a typed copy of the namespace cache; avoid raw castsSafer and future‑proof.
- return new HashMap<>((Map) cacheManager.getCache(properties.getNamespaceFunctions()).getNativeCache()); + @SuppressWarnings("unchecked") + Map<String, List<CachedFunction>> nativeMap = + (Map<String, List<CachedFunction>>) getRequiredCache(properties.getNamespaceFunctions()).getNativeCache(); + return new HashMap<>(nativeMap);
154-166: Clear caches via required-cache helperPrevents NPEs if a cache is missing.
- cacheManager.getCache(CacheMapKeys.ACTIONS).clear(); + getRequiredCache(CacheMapKeys.ACTIONS).clear(); @@ - cacheManager.getCache(properties.getNamespaceFunctions()).clear(); + getRequiredCache(properties.getNamespaceFunctions()).clear(); @@ - cacheManager.getCache(CacheMapKeys.FUNCTIONS).clear(); + getRequiredCache(CacheMapKeys.FUNCTIONS).clear();
69-72: Guard reload path with required-cache helperPrevents NPE when cache is misconfigured.
- cacheManager.getCache(properties.getNamespaceFunctions()).evictIfPresent(petriNetId); + getRequiredCache(properties.getNamespaceFunctions()).evictIfPresent(petriNetId); cachePetriNetFunctions(petriNetService.getNewestVersionByIdentifier(petriNetId));
80-92: Use atomic Cache#get(key, loader) and evict on rewrite; stop peeking into native mapThis fixes potential NPE and races, and honors rewrite semantics.
- Cache actionsCache = cacheManager.getCache(CacheMapKeys.ACTIONS); - Object nativeActionsCache = actionsCache.getNativeCache(); - - if (nativeActionsCache instanceof Map<?, ?> map) { - if (shouldRewriteCachedActions || !map.containsKey(stringId) ) { - Closure code = (Closure) shell.evaluate("{-> " + action.getDefinition() + "}"); - actionsCache.put(stringId, code); - } - } - return (Closure) actionsCache.get(stringId).get(); + Cache actionsCache = getRequiredCache(CacheMapKeys.ACTIONS); + if (shouldRewriteCachedActions) { + actionsCache.evict(stringId); + } + return actionsCache.get(stringId, () -> (Closure) shell.evaluate("{-> " + action.getDefinition() + "}"));
95-107: Compute functions via Cache API, no native map castingCleaner, thread-safe, and backend-agnostic.
- Cache functionsCache = cacheManager.getCache(CacheMapKeys.FUNCTIONS); - Object nativeFunctionsCache = functionsCache.getNativeCache(); - - if (nativeFunctionsCache instanceof Map<?, ?> map) { - functions.forEach(function -> { - if (!map.containsKey(function.getStringId())) { - functionsCache.put(function.getStringId(), CachedFunction.build(shell, function)); - } - cachedFunctions.add((CachedFunction) functionsCache.get(function.getStringId()).get()); - }); - } + Cache functionsCache = getRequiredCache(CacheMapKeys.FUNCTIONS); + for (Function function : functions) { + CachedFunction cf = functionsCache.get(function.getStringId(), + () -> CachedFunction.build(shell, function)); + cachedFunctions.add(cf); + }application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java (1)
38-42: Avoid duplicate registrations for specialized cachesFilter out ACTIONS and FUNCTIONS from the generic cache list before adding specialized implementations.
- Set<String> cacheNames = properties.getAllCaches(); - List<Cache> caches = cacheNames.stream() + Set<String> cacheNames = new LinkedHashSet<>(properties.getAllCaches()); + cacheNames.removeAll(Set.of(CacheMapKeys.ACTIONS, CacheMapKeys.FUNCTIONS)); + List<Cache> caches = cacheNames.stream() .map(ConcurrentMapCache::new) .collect(Collectors.toCollection(ArrayList::new));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheMapKeys.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java (1)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheMapKeys.java (1)
CacheMapKeys(3-6)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (3)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheMapKeys.java (1)
CacheMapKeys(3-6)application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (1)
Slf4j(12-122)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (1)
Slf4j(66-581)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: task-list-completed
- GitHub Check: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (1)
application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java (1)
44-48: LGTM on adding namespaceFunctions property and wiring it into getAllCaches()Also applies to: 62-63
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheMapKeys.java
Outdated
Show resolved
Hide resolved
...ation-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java
Outdated
Show resolved
Hide resolved
...ation-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java
Show resolved
Hide resolved
.../src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
Show resolved
Hide resolved
…e to changes in cache structure
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheMapKeys.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-31T23:40:46.499Z
Learnt from: tuplle
PR: netgrif/application-engine#334
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java:204-214
Timestamp: 2025-07-31T23:40:46.499Z
Learning: In the PetriNetService.importPetriNet method, existingNet.getVersion() cannot be null because all existing nets in the system were deployed through processes that ensure every net always has a version assigned.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
🔇 Additional comments (4)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (2)
567-567: No functional change here.Whitespace-only change; nothing to address.
8-8: Cache names match — no action required.
@Cacheable values in PetriNetService ("petriNetCache" — line 158; "petriNetById" — 250; "petriNetByIdentifier" — 261; "petriNetNewest" — 284) exactly match defaults in CacheConfigurationProperties (lines 22, 27, 32, 37).application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheMapKeys.java (1)
3-7: LGTM — constants holder is correctly non-instantiable.
Matches prior guidance: final class + private ctor; fields are public static final. No issues.application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (1)
33-39: No action required — factories already return synchronized maps. CacheConfiguration creates GenericMapCache instances with Suppliers that wrap MaxSizeHashMap via Collections.synchronizedMap (application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java:45–58), so the current unsynchronized reads are safe.
| @Override public Object getNativeCache() { return Map.copyOf(map()); } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick
Preserve native cache identity; clear in place.
Returning Map.copyOf breaks “native” semantics and reassigning the map on clear invalidates existing nativeCache holders. Return the backing map and clear it in place.
Apply this diff:
- @Override public Object getNativeCache() { return Map.copyOf(map()); }
+ @Override public Object getNativeCache() { return map(); }
@@
- public synchronized void clear() {
- this.atomicMapRef.set(mapFactory.get());
- }
+ public synchronized void clear() {
+ map().clear();
+ log.debug("{} cache cleared", name);
+ }Also applies to: 95-98
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java
around lines 30-31 (and likewise adjust lines 95-98), the getNativeCache method
currently returns Map.copyOf(map()) which breaks native cache identity; change
getNativeCache to return the backing map directly (return map()) so callers keep
the same instance. Also modify clear logic (lines ~95-98) to clear the existing
backing map in place (call map().clear()) instead of reassigning a new map
instance, preserving nativeCache holders and expected semantics.
| @Override | ||
| public synchronized ValueWrapper get(Object key) { | ||
| String stringKey = String.valueOf(key); | ||
| Object valueObject = map().get(stringKey); | ||
| return valueObject != null ? new SimpleValueWrapper(valueObject) : null; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick
Reduce contention after ensuring thread-safe map.
Once the backing map is guaranteed thread-safe (ConcurrentMap or synchronizedMap), method-level synchronized on reads/writes becomes redundant and harms throughput. Consider removing synchronized from get(key), get(key, Class), put, evict.
Also applies to: 72-83, 85-93
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java
around lines 65-71 (and similarly for 72-83 and 85-93), the methods are declared
synchronized which is redundant if the backing map is a thread-safe
ConcurrentMap or synchronizedMap; remove the synchronized modifier from
get(Object), get(Object, Class), put(...) and evict(...) methods, ensure the
map() method returns a thread-safe map implementation (e.g., ConcurrentHashMap
or Collections.synchronizedMap(...)) and add a short comment/javadoc asserting
the map is thread-safe so callers know method-level synchronization was
intentionally removed.
...ation-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java
Show resolved
Hide resolved
| import com.netgrif.application.engine.objects.petrinet.domain.PetriNetSearch; | ||
| import com.netgrif.application.engine.objects.petrinet.domain.Transition; | ||
| import com.netgrif.application.engine.objects.petrinet.domain.VersionType; | ||
| import com.netgrif.application.engine.objects.petrinet.domain.*; |
There was a problem hiding this comment.
🧹 Nitpick
Avoid wildcard imports; restore explicit imports.
Wildcard imports are against common style guides (and often blocked by Checkstyle), can hide accidental type collisions, and make IDE organize‑imports unstable. Please revert to explicit imports for the actually used types (PetriNet, PetriNetSearch, Transition, VersionType).
Apply:
-import com.netgrif.application.engine.objects.petrinet.domain.*;
+import com.netgrif.application.engine.objects.petrinet.domain.PetriNet;
+import com.netgrif.application.engine.objects.petrinet.domain.PetriNetSearch;
+import com.netgrif.application.engine.objects.petrinet.domain.Transition;
+import com.netgrif.application.engine.objects.petrinet.domain.VersionType;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import com.netgrif.application.engine.objects.petrinet.domain.*; | |
| import com.netgrif.application.engine.objects.petrinet.domain.PetriNet; | |
| import com.netgrif.application.engine.objects.petrinet.domain.PetriNetSearch; | |
| import com.netgrif.application.engine.objects.petrinet.domain.Transition; | |
| import com.netgrif.application.engine.objects.petrinet.domain.VersionType; |
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java
around line 4, replace the wildcard import
"com.netgrif.application.engine.objects.petrinet.domain.*" with explicit imports
for the actual types used (PetriNet, PetriNetSearch, Transition, VersionType) so
the file imports only those classes; update the import statements accordingly
and run organizer/IDE fix imports to ensure no other unused imports remain.
...on-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java
Outdated
Show resolved
Hide resolved
renczesstefan
changed the base branch from
release/7.0.0-rev7
to
release/7.0.0-rev8
renczesstefan
left a comment
renczesstefan
left a comment
There was a problem hiding this comment.
Please review coderabbitAI comments.
| private final CacheConfigurationProperties properties; | ||
|
|
||
| public CacheConfiguration(CacheConfigurationProperties properties) { | ||
| public CacheConfiguration(RunnerConfigurationProperties.FieldRunnerProperties fieldRunnerProperties, CacheConfigurationProperties properties) { |
There was a problem hiding this comment.
Its only cosmetic, but instead of this constructor you can use @RequiredArgsConstructor over the class.
There was a problem hiding this comment.
Thank you. I will fix that.
| public void reloadCachedFunctions(PetriNet petriNet) { | ||
| namespaceFunctionsCache.remove(petriNet.getIdentifier()); | ||
| cachePetriNetFunctions(petriNetService.getNewestVersionByIdentifier(petriNet.getIdentifier())); | ||
| this.reloadCachedFunctions(petriNet.getIdentifier()); |
There was a problem hiding this comment.
Will this be always valid? Is it possible that there will be petri net with multiple valid versions? I am thinking about the situation, where there is no migration executed for newest version, and the old cases are using old petri nets.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-31T23:40:46.499Z
Learnt from: tuplle
PR: netgrif/application-engine#334
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java:204-214
Timestamp: 2025-07-31T23:40:46.499Z
Learning: In the PetriNetService.importPetriNet method, existingNet.getVersion() cannot be null because all existing nets in the system were deployed through processes that ensure every net always has a version assigned.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
🧬 Code graph analysis (2)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (3)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheMapKeys.java (1)
CacheMapKeys(3-7)application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (1)
Slf4j(12-110)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (1)
Slf4j(68-570)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java (1)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheMapKeys.java (1)
CacheMapKeys(3-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
- GitHub Check: task-list-completed
🔇 Additional comments (16)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (1)
8-11: Explicit imports look good—thanks for aligning with the project style.The explicit imports remove the wildcard and keep the dependency surface clear.
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (6)
58-65: LGTM: fail‑fast cache retrieval and eviction on empty setUsing getRequiredCache() avoids NPEs; evicting when empty keeps the cache tidy.
155-166: LGTM: clearing caches via required cache helperSafe and consistent with the new cache infra.
168-174: LGTM: centralized required-cache helperGood error message and avoids scattered null checks.
150-151: Return a typed copy of namespace function cacheRemove raw cast; return a typed map copy for safety.
- return new HashMap<>((Map) getRequiredCache(properties.getNamespaceFunctions()).getNativeCache()); + @SuppressWarnings("unchecked") + Map<String, List<CachedFunction>> nativeMap = + (Map<String, List<CachedFunction>>) getRequiredCache(properties.getNamespaceFunctions()).getNativeCache(); + return new HashMap<>(nativeMap);
95-108: Major: Cache logic bypassed when native isn’t Map; also potential NPE via ValueWrapper.get()Avoid peeking into native map and rely on Cache#get(key, loader) to compute‑if‑absent.
- Cache functionsCache = getRequiredCache(CacheMapKeys.FUNCTIONS); - Object nativeFunctionsCache = functionsCache.getNativeCache(); - - if (nativeFunctionsCache instanceof Map<?, ?> map) { - functions.forEach(function -> { - if (!map.containsKey(function.getStringId())) { - functionsCache.put(function.getStringId(), CachedFunction.build(shell, function)); - } - cachedFunctions.add((CachedFunction) functionsCache.get(function.getStringId()).get()); - }); - } + Cache functionsCache = getRequiredCache(CacheMapKeys.FUNCTIONS); + for (Function function : functions) { + CachedFunction cf = functionsCache.get(function.getStringId(), + () -> CachedFunction.build(shell, function)); + cachedFunctions.add(cf); + }
82-91: Critical: NPE risk and brittle native-cache introspection; use Cache#get(key, loader)If the native cache isn’t a Map (e.g., Caffeine), the compute path is skipped and actionsCache.get(...).get() can NPE. Use compute‑if‑absent via Cache API and evict on rewrite.
- Cache actionsCache = getRequiredCache(CacheMapKeys.ACTIONS); - Object nativeActionsCache = actionsCache.getNativeCache(); - - if (nativeActionsCache instanceof Map<?, ?> map) { - if (shouldRewriteCachedActions || !map.containsKey(stringId) ) { - Closure code = (Closure) shell.evaluate("{-> " + action.getDefinition() + "}"); - actionsCache.put(stringId, code); - } - } - return (Closure) actionsCache.get(stringId).get(); + Cache actionsCache = getRequiredCache(CacheMapKeys.ACTIONS); + if (shouldRewriteCachedActions) { + actionsCache.evict(stringId); + } + return actionsCache.get(stringId, () -> (Closure) shell.evaluate("{-> " + action.getDefinition() + "}"));application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java (4)
41-48: LGTM: bounded ACTIONS cache with thread‑safe backingSynchronized MaxSizeHashMap + GenericMapCache is acceptable for now.
50-57: Confirm FUNCTIONS value type matches usageStoring CachedFunction aligns with FieldActionsCacheService.getCachedFunctions().
59-62: LGTM: cache manager assemblySimpleCacheManager wiring is correct; Actuator should pick these up.
35-39: Filter out ACTIONS and FUNCTIONS from generic caches
Exclude the reserved CACHE_MAP keys before creating the genericConcurrentMapCacheinstances:- Set<String> cacheNames = properties.getAllCaches(); + Set<String> cacheNames = new LinkedHashSet<>(properties.getAllCaches()); + cacheNames.removeAll(Set.of(CacheMapKeys.ACTIONS, CacheMapKeys.FUNCTIONS)); List<Cache> caches = cacheNames.stream() .map(ConcurrentMapCache::new) .collect(Collectors.toCollection(ArrayList::new));application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (5)
32-63: LGTM: per‑key locking for atomic computeCorrect double‑checked compute with per‑key locks and proper exception wrapping.
98-109: LGTM: runtime type safety via valueTypesafeCast guards value type mismatches appropriately.
30-31: Expose true native cache to align with Spring semanticsReturning Map.copyOf breaks native identity and snapshots values; callers expect the live backing store. Return the backing map.
- @Override public Object getNativeCache() { return Map.copyOf(map()); } + @Override public Object getNativeCache() { return map(); }
72-87: Reduce contention: remove method‑level synchronized on simple opsBacked by a thread‑safe map plus per‑key locks for loader; method‑level synchronized on get(type), put, evict is unnecessary and harms throughput.
- public synchronized <T> T get(Object key, Class<T> type) { + public <T> T get(Object key, Class<T> type) { String stringKey = String.valueOf(key); Object valueObject = map().get(stringKey); return valueObject != null ? type.cast(valueObject) : null; } @@ - public synchronized void put(Object key, Object value) { + public void put(Object key, Object value) { map().put(String.valueOf(key), safeCast(value)); } @@ - public synchronized void evict(Object key) { + public void evict(Object key) { map().remove(String.valueOf(key)); }
89-92: Preserve native identity on clear()Reassigning the map breaks existing native references; clear in place.
- public synchronized void clear() { - this.atomicMapRef.set(mapFactory.get()); - } + public synchronized void clear() { + map().clear(); + log.debug("{} cache cleared", name); + }
| @Override | ||
| public void reloadCachedFunctions(String petriNetId) { | ||
| getRequiredCache(properties.getNamespaceFunctions()).evictIfPresent(petriNetId); | ||
| cachePetriNetFunctions(petriNetService.getNewestVersionByIdentifier(petriNetId)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Guard reload when no newer Petri net exists (log for observability)
If getNewestVersionByIdentifier returns null (fully deleted), this silently no‑ops. Consider logging at debug/info when no net is found to make actuator-driven cache ops traceable.
public void reloadCachedFunctions(String petriNetId) {
getRequiredCache(properties.getNamespaceFunctions()).evictIfPresent(petriNetId);
- cachePetriNetFunctions(petriNetService.getNewestVersionByIdentifier(petriNetId));
+ PetriNet newest = petriNetService.getNewestVersionByIdentifier(petriNetId);
+ if (newest == null) {
+ log.debug("No Petri net found for id={}, namespace functions cache remains evicted", petriNetId);
+ return;
+ }
+ cachePetriNetFunctions(newest);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public void reloadCachedFunctions(String petriNetId) { | |
| getRequiredCache(properties.getNamespaceFunctions()).evictIfPresent(petriNetId); | |
| cachePetriNetFunctions(petriNetService.getNewestVersionByIdentifier(petriNetId)); | |
| } | |
| @Override | |
| public void reloadCachedFunctions(String petriNetId) { | |
| getRequiredCache(properties.getNamespaceFunctions()).evictIfPresent(petriNetId); | |
| PetriNet newest = petriNetService.getNewestVersionByIdentifier(petriNetId); | |
| if (newest == null) { | |
| log.debug("No Petri net found for id={}, namespace functions cache remains evicted", petriNetId); | |
| return; | |
| } | |
| cachePetriNetFunctions(newest); | |
| } |
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
around lines 68 to 72, the call to
petriNetService.getNewestVersionByIdentifier(petriNetId) can return null for
fully deleted nets and you should avoid passing null into
cachePetriNetFunctions; change the method to fetch the newest version into a
local variable, if it is null log a debug/info message including the petriNetId
(and optionally why) for observability, and return early instead of calling
cachePetriNetFunctions; keep the existing evictIfPresent call as-is.
machacjozef
changed the base branch from
release/7.0.0-rev8
to
release/7.0.0-rev9
Description
This PR introduces a GenericMapCache implementation of Spring’s Cache interface, backed by in-memory Maps. The solution replaces ad-hoc local maps with proper cache wrappers that integrate with Spring Boot Actuator’s /caches endpoints.
This makes all cache operations consistent and manageable via Actuator.
Implements NAE-2182
How Has Been This Tested?
The functionality was tested using Postman to verify cache operations through the Actuator endpoints.
Checklist:
Summary by CodeRabbit
New Features
Refactor