[NAE-2182] Clearing actions cache #348
Conversation
…ache endpoints for actionsCache, functionsCache and namespaceFunctionsCache
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughReworks caching infrastructure: adds typed GenericMapCache and CacheMapKeys; CacheConfiguration now implements CachingConfigurer and registers configured caches (including ACTIONS, FUNCTIONS, GLOBAL_FUNCTIONS) into a SimpleCacheManager; FieldActionsCacheService now uses CacheManager-backed caches and adds global-function reload/precache APIs; cache sizing properties added; PetriNet deletion promotes latest version when removing the default. Changes
Sequence Diagram(s)sequenceDiagram
actor Startup
participant CacheConfiguration
participant SimpleCacheManager
participant FieldActionsCacheService
participant PetriNetService
Startup->>CacheConfiguration: initialize cacheManager bean
CacheConfiguration->>SimpleCacheManager: register configured caches + add ACTIONS/FUNCTIONS/GLOBAL_FUNCTIONS (GenericMapCache)
Startup->>FieldActionsCacheService: cacheAllPetriNetFunctions()
FieldActionsCacheService->>PetriNetService: fetch paged PetriNets
PetriNetService-->>FieldActionsCacheService: return PetriNet pages
loop per PetriNet
FieldActionsCacheService->>SimpleCacheManager: getCache("globalFunctions")
SimpleCacheManager-->>FieldActionsCacheService: Cache instance
FieldActionsCacheService->>FieldActionsCacheService: compute CachedFunction/Closure
FieldActionsCacheService->>SimpleCacheManager: put(petriNetId, CachedFunction)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
Outdated
Show resolved
Hide resolved
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
...ation-engine/src/main/java/com/netgrif/application/engine/configuration/ActionsMapCache.java
Outdated
Show resolved
Hide resolved
...ation-engine/src/main/java/com/netgrif/application/engine/configuration/ActionsMapCache.java
Outdated
Show resolved
Hide resolved
...on-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java
Outdated
Show resolved
Hide resolved
...on-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java
Outdated
Show resolved
Hide resolved
...c/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java
Outdated
Show resolved
Hide resolved
...on-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.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
...va/com/netgrif/application/engine/workflow/service/interfaces/IFieldActionsCacheService.java
Outdated
Show resolved
Hide resolved
|
There are test errors |
...on-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java
Outdated
Show resolved
Hide resolved
…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.
...ation-engine/src/main/java/com/netgrif/application/engine/configuration/ActionsMapCache.java
Outdated
Show resolved
Hide resolved
...on-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java
Show resolved
Hide resolved
...on-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java
Outdated
Show resolved
Hide resolved
...ion-engine/src/main/java/com/netgrif/application/engine/configuration/FunctionsMapCache.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/com/netgrif/application/engine/configuration/FunctionsNamespaceMapCache.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/com/netgrif/application/engine/configuration/FunctionsNamespaceMapCache.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
Outdated
Show resolved
Hide resolved
...tion-engine/src/main/java/com/netgrif/application/engine/workflow/domain/CachedFunction.java
Outdated
Show resolved
Hide resolved
...tion-engine/src/main/java/com/netgrif/application/engine/workflow/domain/CachedFunction.java
Outdated
Show resolved
Hide resolved
…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.
...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
...ation-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java
Show resolved
Hide resolved
...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.
...on-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java
Outdated
Show resolved
Hide resolved
.../src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
Outdated
Show resolved
Hide resolved
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); + }
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
@application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java:
- Around line 29-48: The get method in GenericMapCache uses map.computeIfAbsent
on a Collections.synchronizedMap which is not atomic and can cause the loader to
run multiple times; fix by synchronizing around the compound operation: in
GenericMapCache.get(Object key, Callable<T> loader) acquire a lock on the
underlying map (synchronized(map) { ... }) and perform the compute-if-absent
logic (or check-then-put) inside that synchronized block so the loader.call() is
only invoked once per key; alternatively, replace the synchronizedMap with a
thread-safe ConcurrentHashMap-backed implementation and use its computeIfAbsent
to get atomic semantics—update references to map, computeIfAbsent, and the get
method accordingly.
- Line 27: The getNativeCache implementation uses Map.copyOf(map), which throws
NPE if the stored map contains null values; change
GenericMapCache.getNativeCache to create a new HashMap copy via the HashMap(Map)
constructor instead, and perform the copy while synchronizing on the underlying
map (synchronize on the map field) to avoid concurrent-modification during
iteration; return the new copy (or an unmodifiable view of that new HashMap if
immutability is desired) so null values are preserved and thread-safety is
maintained.
In
@application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java:
- Around line 71-78: In reloadCachedGlobalFunctions, ensure stale cache entries
are evicted even when
petriNetService.getDefaultVersionByIdentifier(processIdentifier) returns null:
call
getRequiredCache(CacheMapKeys.GLOBAL_FUNCTIONS).evictIfPresent(processIdentifier)
before returning (and only call cachePetriNetFunctions(petriNet) when petriNet
!= null); alternatively, if retaining cache for missing nets is intentional, add
a clear comment in reloadCachedGlobalFunctions explaining that design decision.
- Around line 99-116: Replace the manual cache-aside logic in getCachedFunctions
with Cache.get(key, Callable) on the functionsCache (obtained via
getRequiredCache(CacheMapKeys.FUNCTIONS)) so that for each Function you call
functionsCache.get(function.getStringId(), () -> CachedFunction.build(shell,
function)); this removes the explicit ValueWrapper null check and put, still
uses CachedFunction.build(...) as the loader, and collects the returned
CachedFunction instances into the cachedFunctions list.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 363
File: application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java:66-69
Timestamp: 2026-01-08T08:37:58.317Z
Learning: In the CacheConfigurationProperties class, the cache property for namespace/global functions is named `globalFunctions`, not `namespaceFunctions`. This property was renamed as part of the refactoring from namespace-scoped to global-scoped function caching.
📚 Learning: 2026-01-08T08:37:58.317Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 363
File: application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java:66-69
Timestamp: 2026-01-08T08:37:58.317Z
Learning: In the CacheConfigurationProperties class, the cache property for namespace/global functions is named `globalFunctions`, not `namespaceFunctions`. This property was renamed as part of the refactoring from namespace-scoped to global-scoped function caching.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
📚 Learning: 2025-07-31T23:40:46.499Z
Learnt from: tuplle
Repo: netgrif/application-engine PR: 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/workflow/service/FieldActionsCacheService.java
📚 Learning: 2025-12-19T09:06:38.316Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 398
File: application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java:57-58
Timestamp: 2025-12-19T09:06:38.316Z
Learning: In the file `application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java`, the `definition` attribute of the `Action` class (from `com.netgrif.application.engine.objects.petrinet.domain.dataset.logic.action.Action`) is never null, so null checks for `action.getDefinition()` are not needed.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
📚 Learning: 2025-09-29T10:31:57.325Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 362
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java:513-529
Timestamp: 2025-09-29T10:31:57.325Z
Learning: PetriNet.getStringId() returns a simple ObjectId string representation (_id.toString()), not a composite Netgrif ID format, so new ObjectId(petriNetId) works correctly when petriNetId comes from PetriNet.getStringId().
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
📚 Learning: 2025-11-14T10:22:01.634Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 383
File: application-engine/src/main/java/com/netgrif/application/engine/startup/ApplicationRunnerOrderResolver.java:43-43
Timestamp: 2025-11-14T10:22:01.634Z
Learning: For the netgrif/application-engine repository, avoid flagging trivial or nitpick-level issues such as redundant null checks, minor code style improvements, or obvious simplifications that don't affect functionality or introduce bugs. Focus review comments on substantive issues like logic errors, security concerns, performance problems, or breaking changes.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.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). (21)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- 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
- 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: task-list-completed
- 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/configuration/GenericMapCache.java (2)
50-77: LGTM!The
get,put,evict, andclearmethods correctly delegate to the synchronized map with proper type handling viasafeCast. TheValueWrapperpattern is correctly implemented.
79-90: LGTM!The
safeCasthelper properly validates types at runtime and provides clear error messages on type mismatch.application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (4)
85-97: LGTM!The cache lookup pattern is correctly implemented. The
shouldRewriteCachedActionsflag is properly checked before cache lookup, and the closure evaluation/caching logic is sound.
177-186: LGTM!Good defensive implementation: uses
instanceofpattern matching, returns a defensive copy to prevent external modification of cache state, and handles the non-Map case gracefully.
188-209: LGTM!The clear methods correctly delegate to the underlying cache, and
getRequiredCacheprovides a fail-fast mechanism for catching configuration errors early.
32-43: LGTM!Constructor correctly initializes dependencies via injection. The
@Lazyannotation onsetPetriNetServiceappropriately handles the circular dependency.
...ation-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java
Show resolved
Hide resolved
|
@machacjozef: I'll rescan the entire PR and conduct a fresh comprehensive review of all the changes. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java (1)
287-316: Inconsistent error messages acrosscreateCaseByIdentifieroverloads.The error message was updated on Line 290 to "does not have default version", but the other overloads (Lines 304 and 313) still use "does not exist". Since all three methods call
getDefaultVersionByIdentifierand check the same condition, the messages should be consistent.Proposed fix
@@ -301,7 +301,7 @@ public class WorkflowService implements IWorkflowService { public CreateCaseEventOutcome createCaseByIdentifier(String identifier, String title, String color, LoggedUser user, Map<String, String> params) { PetriNet net = petriNetService.getDefaultVersionByIdentifier(identifier); if (net == null) { - throw new IllegalArgumentException("Petri net with identifier [" + identifier + "] does not exist."); + throw new IllegalArgumentException("Petri net with identifier [" + identifier + "] does not have default version."); } return this.createCase(net.getStringId(), title, color, user, params); } @@ -310,7 +310,7 @@ public class WorkflowService implements IWorkflowService { public CreateCaseEventOutcome createCaseByIdentifier(String identifier, String title, String color, LoggedUser user) { PetriNet net = petriNetService.getDefaultVersionByIdentifier(identifier); if (net == null) { - throw new IllegalArgumentException("Petri net with identifier [" + identifier + "] does not exist."); + throw new IllegalArgumentException("Petri net with identifier [" + identifier + "] does not have default version."); } return this.createCase(net.getStringId(), title, color, user); }
🤖 Fix all issues with AI agents
In
@application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java:
- Around line 71-78: The method reloadCachedGlobalFunctions currently skips
evicting cache when
petriNetService.getDefaultVersionByIdentifier(processIdentifier) returns null;
update reloadCachedGlobalFunctions so that it always calls
getRequiredCache(CacheMapKeys.GLOBAL_FUNCTIONS).evictIfPresent(processIdentifier)
before returning, and only calls cachePetriNetFunctions(petriNet) when petriNet
is non-null, ensuring stale entries are removed; reference
reloadCachedGlobalFunctions, petriNetService.getDefaultVersionByIdentifier,
getRequiredCache(CacheMapKeys.GLOBAL_FUNCTIONS).evictIfPresent(processIdentifier),
and cachePetriNetFunctions.
- Around line 80-83: The one-arg overload reloadCachedGlobalFunctions(PetriNet
petriNet) currently dereferences petriNet (petriNet.getIdentifier()) and can
throw NPE if petriNet is null; change it to mirror cachePetriNetFunctions by
adding a null check and delegating appropriately (e.g., if petriNet is null call
the String overload with null or otherwise return) so
reloadCachedGlobalFunctions handles null inputs safely.
- Around line 177-186: getGlobalFunctionsCache currently returns an empty map
silently if getRequiredCache(CacheMapKeys.GLOBAL_FUNCTIONS).getNativeCache() is
not a Map, which can hide misconfigurations; modify getGlobalFunctionsCache to
detect non-Map nativeCache and log a warning (including
CacheMapKeys.GLOBAL_FUNCTIONS and nativeCache.getClass() or the null value)
before returning Collections.emptyMap, while keeping the defensive copy logic
for the Map branch; use the class logger (e.g., the existing logger field) to
emit the warning so callers and operators can diagnose unexpected cache
implementations.
In
@nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/version/Version.java:
- Line 59: In Version.java update the Javadoc return description to remove the
malformed closing tag `</0>` and escape comparison operators using {@code}, e.g.
change the @return line in the Version class (compareTo method Javadoc) to
something like: "@return {@code 0} if the versions equal, {@code < 0} if this is
lower than other, {@code > 0} if this is higher than other" so the Javadoc
parses correctly and shows the operators as code.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/CacheMapKeys.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/properties/RunnerConfigurationProperties.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IFieldActionsCacheService.javaapplication-engine/src/main/resources/application.yamldocs/process/functions.mdnae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/version/Version.java
💤 Files with no reviewable changes (1)
- application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/RunnerConfigurationProperties.java
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 363
File: application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java:66-69
Timestamp: 2026-01-08T08:37:58.317Z
Learning: In the CacheConfigurationProperties class, the cache property for namespace/global functions is named `globalFunctions`, not `namespaceFunctions`. This property was renamed as part of the refactoring from namespace-scoped to global-scoped function caching.
📚 Learning: 2025-11-14T10:22:01.634Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 383
File: application-engine/src/main/java/com/netgrif/application/engine/startup/ApplicationRunnerOrderResolver.java:43-43
Timestamp: 2025-11-14T10:22:01.634Z
Learning: For the netgrif/application-engine repository, avoid flagging trivial or nitpick-level issues such as redundant null checks, minor code style improvements, or obvious simplifications that don't affect functionality or introduce bugs. Focus review comments on substantive issues like logic errors, security concerns, performance problems, or breaking changes.
Applied to files:
application-engine/src/main/resources/application.yamlapplication-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.javadocs/process/functions.mdapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java
📚 Learning: 2026-01-13T16:33:15.098Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 348
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java:4-4
Timestamp: 2026-01-13T16:33:15.098Z
Learning: In the com.netgrif.application.engine.objects.petrinet.domain package, prefer using wildcard imports and do not suggest replacing them with explicit imports during code reviews. This guideline applies to all Java files under that package (and subpackages) to reflect project-specific import conventions and reduce noise in imports.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/version/Version.java
📚 Learning: 2026-01-08T08:37:58.317Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 363
File: application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java:66-69
Timestamp: 2026-01-08T08:37:58.317Z
Learning: In the CacheConfigurationProperties class, the cache property for namespace/global functions is named `globalFunctions`, not `namespaceFunctions`. This property was renamed as part of the refactoring from namespace-scoped to global-scoped function caching.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/CacheMapKeys.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
📚 Learning: 2025-12-19T09:06:38.316Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 398
File: application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java:57-58
Timestamp: 2025-12-19T09:06:38.316Z
Learning: In the file `application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java`, the `definition` attribute of the `Action` class (from `com.netgrif.application.engine.objects.petrinet.domain.dataset.logic.action.Action`) is never null, so null checks for `action.getDefinition()` are not needed.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java
📚 Learning: 2025-07-31T23:40:46.499Z
Learnt from: tuplle
Repo: netgrif/application-engine PR: 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/workflow/service/WorkflowService.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java
📚 Learning: 2026-01-13T16:33:15.098Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 348
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java:4-4
Timestamp: 2026-01-13T16:33:15.098Z
Learning: In the netgrif/application-engine repository, wildcard imports are preferred for the com.netgrif.application.engine.objects.petrinet.domain package. Do not suggest replacing wildcard imports with explicit imports for this package.
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
📚 Learning: 2025-07-29T17:19:18.300Z
Learnt from: tuplle
Repo: netgrif/application-engine PR: 331
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java:45-46
Timestamp: 2025-07-29T17:19:18.300Z
Learning: In ElasticPetriNetService class, petriNetService is properly initialized using Lazy setter injection rather than constructor injection. This pattern with Lazy Autowired setter methods is commonly used in Spring to resolve circular dependencies and is a valid alternative to constructor injection.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java
📚 Learning: 2025-09-29T10:31:57.325Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 362
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java:513-529
Timestamp: 2025-09-29T10:31:57.325Z
Learning: PetriNet.getStringId() returns a simple ObjectId string representation (_id.toString()), not a composite Netgrif ID format, so new ObjectId(petriNetId) works correctly when petriNetId comes from PetriNet.getStringId().
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.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java
📚 Learning: 2025-12-19T07:55:13.427Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 400
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java:239-247
Timestamp: 2025-12-19T07:55:13.427Z
Learning: In the netgrif/application-engine project, the `taskService.findOne()` method (from ITaskService) never returns null. When a task is not found, it handles the error through other means (such as throwing an exception) rather than returning null, so null checks after `findOne()` calls are unnecessary.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java
🧬 Code graph analysis (1)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (1)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheMapKeys.java (1)
CacheMapKeys(3-8)
⏰ 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). (42)
- 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: 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: 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: 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
- 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: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (21)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheMapKeys.java (1)
1-8: LGTM!Clean utility class following standard patterns: final class with private constructor and public static constants. The constant names align with the cache configuration used elsewhere in this PR. Based on learnings,
globalFunctionsis the correct naming after the refactoring from namespace-scoped to global-scoped function caching.docs/process/functions.md (1)
7-7: No issues.Trailing whitespace removal; no semantic impact.
application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (1)
14-23: LGTM!The cache implementation correctly uses
Collections.synchronizedMapfor thread safety and enforces type constraints viasafeCast. The design appropriately wraps loader exceptions inCache.ValueRetrievalExceptionper Spring Cache contract, andcomputeIfAbsentis atomic on synchronized maps in Java 8+.application-engine/src/main/resources/application.yaml (1)
119-121: Clarify the intent for exposing all management endpoints in/manage/**.While exposing all Actuator endpoints can pose a security risk, the configuration in
application.yamlhas/manage/**explicitly listed in bothserver-patterns(line 88) andanonymous-exceptions(line 89), indicating this access is intentional by design. This same configuration already existed in the legacy property files and has no production override.Before restricting endpoints to
health,info,prometheus,cachesas suggested, verify with the team whether:
- This intentional anonymous access is required for operational monitoring or cache management systems
- The isolated
/manage/**path provides sufficient security isolation- Any production environments override this configuration (which would need to be added separately)
If unrestricted anonymous access to these endpoints is truly unintended, the suggested restriction is valid. If this is intentional operational design, the current configuration should be preserved.
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java (1)
4-4: LGTM!The wildcard import for
com.netgrif.application.engine.objects.petrinet.domain.*aligns with the repository's preferred import style for this package. Based on learnings, this is the expected convention.application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (3)
8-11: LGTM!Explicit imports for domain types are appropriate here.
636-636: LGTM!The method call correctly aligns with the updated
IFieldActionsCacheServiceinterface, which now providesreloadCachedGlobalFunctionsinstead ofreloadCachedFunctions.
637-645: Good handling of default version promotion.The logic correctly promotes the latest version to default when the current default is deleted. The cache eviction at line 635 ensures
getLatestVersionByIdentifier()returns fresh data from the database, excluding the just-deleted net.application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java (2)
24-25: Good migration from deprecated class.Implementing
CachingConfigurerdirectly instead of extending the deprecatedCachingConfigurerSupportis the correct approach for Spring Framework 5.3+.
31-55: LGTM!The cache manager implementation cleanly separates standard
ConcurrentMapCacheinstances for Petri net caching from the specializedGenericMapCacheinstances for actions and functions. The size-limited caches for ACTIONS, FUNCTIONS, and GLOBAL_FUNCTIONS align with the PR's goal of making cache operations manageable via Actuator.application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java (2)
56-80: LGTM!The new cache size properties are well-documented with sensible defaults (500) and proper validation (
@Min(1)). The naming follows the established pattern, withglobalFunctionsCacheSizecorrectly using theglobalFunctionsnaming convention per learnings.
88-93: LGTM!The
getAllCaches()method correctly returns only the cache names for standardConcurrentMapCacheinstances. The specialized caches (ACTIONS, FUNCTIONS, GLOBAL_FUNCTIONS) are handled separately viaGenericMapCacheinCacheConfiguration, so excluding them here prevents duplicate cache creation.application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IFieldActionsCacheService.java (1)
16-18: Breaking API change verified: method renamed and fully migrated.The interface provides two overloads for
reloadCachedGlobalFunctions(by ID and by PetriNet object), replacing the previousreloadCachedFunctionsmethod. All usages have been migrated and both overloads are implemented correctly, with the PetriNet overload properly delegating to the String-based implementation.application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (8)
32-43: Clean dependency injection pattern.The constructor properly injects
CacheConfigurationPropertiesandCacheManager, enabling integration with Spring's cache infrastructure. The lazy injection ofpetriNetService(lines 45-49) correctly avoids potential circular dependencies.
51-69: LGTM - Proper cache management with cleanup.The method correctly handles the null case, uses
evictIfPresentto clean up entries when no global functions exist, and properly integrates with the Spring Cache API.
85-97: LGTM - Correct cache retrieval pattern.The method properly uses
Cache.ValueWrapperto differentiate between cache miss (null wrapper) and cached value, with appropriate handling for theshouldRewriteCachedActionsflag.
99-116: LGTM - Proper cache-aside pattern implementation.The method correctly implements a cache-aside pattern, computing and storing
CachedFunctionon cache miss. Pre-allocating the ArrayList with the expected size is a good optimization.
118-138: LGTM - Robust batch caching with proper error isolation.The pagination pattern is correct, and the per-net exception handling ensures a single failure doesn't prevent caching other PetriNets. The warning log includes the net ID for debugging.
140-153: Correct duplicate function signature detection.The evaluation logic properly validates that no duplicate method signatures exist within the cached functions, throwing a clear exception message on violation.
188-201: LGTM - Clean cache clearing implementation.All three clear methods correctly delegate to the respective cache's
clear()method viagetRequiredCache, ensuring fail-fast behavior if a cache is misconfigured.
203-209: Well-designed fail-fast helper.The
getRequiredCachehelper provides consistent fail-fast behavior across all cache operations. The clear error message aids debugging when a cache is misconfigured.
.../src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
Show resolved
Hide resolved
.../src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
Show resolved
Hide resolved
...ry/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/version/Version.java
Outdated
Show resolved
Hide resolved
Replaced synchronized map with ConcurrentHashMap in GenericMapCache for thread-safety and ensured proper caching logic by reordering conditionals in FieldActionsCacheService. These changes enhance performance and maintainability of the caching system.
Add a null check to prevent potential NullPointerException when calling reloadCachedGlobalFunctions. This ensures better stability and avoids unnecessary errors during execution.
|
@coderabbitai please rescan this PR |
|
@renczesstefan: I'll rescan the entire PR and conduct a fresh comprehensive review of all the changes. ✅ Actions performedFull review triggered. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In
@application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java:
- Around line 12-16: The class GenericMapCache<V> is annotated with @Slf4j but
never uses the generated logger; remove the unused @Slf4j annotation from the
class declaration to eliminate the unused-field warning and dead import; update
any imports to remove lombok.extern.slf4j.Slf4j if present and ensure the class
compiles unchanged (retain GenericMapCache, name, valueType, and map fields and
the implemented Cache interface).
- Around line 63-66: The put(Object key, Object value) method calls
map.put(String.valueOf(key), safeCast(value)) which will throw
NullPointerException when safeCast(value) is null because ConcurrentHashMap
disallows null values; either (A) explicitly reject nulls by validating value in
put and throw IllegalArgumentException (update put to check value == null and
log/throw) or (B) change the backing map to store a wrapper (e.g., Optional) and
adapt safeCast and get/put/remove methods to wrap on insert and unwrap on
retrieval so nulls can be cached safely; locate and update the put method,
safeCast helper, and any get/remove logic to keep behavior consistent.
- Around line 28-47: The get method currently fails to cache null loader results
(because ConcurrentHashMap.computeIfAbsent won't store null) and performs an
unchecked cast; change the caching to store a sentinel or Optional<V> wrapper so
null results are cached (e.g., wrap loader.call() into Optional.ofNullable(...)
or a private NULL_SENTINEL and store that in map inside the lambda used by
map.computeIfAbsent in get), update the retrieval logic to unwrap the stored
wrapper before returning, and add a @SuppressWarnings("unchecked") annotation
(on the get method or immediately before the unchecked cast) to silence the
unchecked cast warning related to the (T) value return; refer to the get method,
map.computeIfAbsent lambda, safeCast and Cache.ValueRetrievalException to locate
the changes.
In
@application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java:
- Around line 101-118: Replace the manual get-then-put race in
getCachedFunctions by using the cache's atomic compute-if-absent form: obtain
the cache via getRequiredCache(CacheMapKeys.FUNCTIONS) and for each Function
call functionsCache.get(function.getStringId(), () ->
CachedFunction.build(shell, function)) to retrieve or atomically create the
CachedFunction instead of using functionsCache.get(...) followed by
functionsCache.put(...); add the result to cachedFunctions (casting to
CachedFunction if necessary).
- Around line 87-99: The getCompiledAction method in FieldActionsCacheService
uses a check-then-act pattern (actionsCache.get then put) which can cause
redundant concurrent compilation; change it to use Cache.get(stringId, Callable)
to perform an atomic get-or-compute: build the closure with shell.evaluate("{->
" + action.getDefinition() + "}") inside the Callable and return that result,
keeping references to actionsCache, stringId, CacheMapKeys.ACTIONS and the
shell.evaluate call to locate where to replace the existing logic.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 363
File: application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java:66-69
Timestamp: 2026-01-08T08:37:58.317Z
Learning: In the CacheConfigurationProperties class, the cache property for namespace/global functions is named `globalFunctions`, not `namespaceFunctions`. This property was renamed as part of the refactoring from namespace-scoped to global-scoped function caching.
📚 Learning: 2026-01-08T08:37:58.317Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 363
File: application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java:66-69
Timestamp: 2026-01-08T08:37:58.317Z
Learning: In the CacheConfigurationProperties class, the cache property for namespace/global functions is named `globalFunctions`, not `namespaceFunctions`. This property was renamed as part of the refactoring from namespace-scoped to global-scoped function caching.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
📚 Learning: 2025-07-31T23:40:46.499Z
Learnt from: tuplle
Repo: netgrif/application-engine PR: 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/workflow/service/FieldActionsCacheService.java
📚 Learning: 2025-12-19T09:06:38.316Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 398
File: application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java:57-58
Timestamp: 2025-12-19T09:06:38.316Z
Learning: In the file `application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java`, the `definition` attribute of the `Action` class (from `com.netgrif.application.engine.objects.petrinet.domain.dataset.logic.action.Action`) is never null, so null checks for `action.getDefinition()` are not needed.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
📚 Learning: 2025-09-29T10:31:57.325Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 362
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java:513-529
Timestamp: 2025-09-29T10:31:57.325Z
Learning: PetriNet.getStringId() returns a simple ObjectId string representation (_id.toString()), not a composite Netgrif ID format, so new ObjectId(petriNetId) works correctly when petriNetId comes from PetriNet.getStringId().
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
📚 Learning: 2025-11-14T10:22:01.634Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 383
File: application-engine/src/main/java/com/netgrif/application/engine/startup/ApplicationRunnerOrderResolver.java:43-43
Timestamp: 2025-11-14T10:22:01.634Z
Learning: For the netgrif/application-engine repository, avoid flagging trivial or nitpick-level issues such as redundant null checks, minor code style improvements, or obvious simplifications that don't affect functionality or introduce bugs. Focus review comments on substantive issues like logic errors, security concerns, performance problems, or breaking changes.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.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). (11)
- 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: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (8)
application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (2)
49-61: LGTM!Both
getoverloads correctly implement the Spring Cache contract with proper null handling and type casting.
68-89: LGTM!The
evict,clear, andsafeCastmethods are correctly implemented with proper type checking.application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (6)
32-43: LGTM!Clean transition from custom map-based caching to Spring Cache abstractions. The constructor properly injects
CacheConfigurationPropertiesandCacheManager, aligning with the PR's goal of integrating with Spring Boot Actuator's cache management endpoints.
51-69: LGTM!The caching logic correctly handles the two cases: storing functions when present, and evicting the cache entry when the PetriNet has no global functions. The null check on line 53-55 provides appropriate early exit.
71-85: LGTM!The null check on line 75 correctly prevents NPE when
getDefaultVersionByIdentifierreturns null. The evict-before-cache ordering on lines 74-77 ensures no window where stale cache data persists after a reload request, which aligns with the commit message about ensuring correct caching behavior.
120-140: LGTM!Good resilience pattern with per-PetriNet exception handling that logs failures but continues processing the batch. The pagination approach correctly avoids loading all entities into memory.
205-211: LGTM!Good defensive programming pattern. Throwing
IllegalStateExceptionfor missing cache configuration provides clear, fast-fail behavior that will surface misconfiguration issues early rather than causing obscure NPEs downstream.
179-188: The native cache implementation guarantees a Map is always returned; the type check will never fail.
GenericMapCache.getNativeCache()returnsMap.copyOf(map)(line 26 of GenericMapCache.java), which always succeeds and returns aMap. Theinstanceofcheck at line 182 will always pass, so the else branch returningCollections.emptyMap()is unreachable and the silent failure scenario cannot occur. No defensive logging is needed.Likely an incorrect or invalid review comment.
...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
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
.../src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
Show resolved
Hide resolved
Updated the constructor parameter and internal map initialization to use 'initialCapacity' instead of the ambiguous 'cacheSize'. This improves code clarity and better
|
2026-01-13T18:22:37.9729165Z [INFO] Results: |
Added a null check in the GenericMapCache.put method to evict entries with null values instead of storing them. Updated JavaDoc in Version.java to fix a minor formatting issue.
Previously, the cache retrieval logic did not properly handle cases where cached objects were lists. This change ensures that the type check applies to the first element of a list, preventing potential `ClassCastException` issues.
|
Please review PR, and resolve trivial comments. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java`:
- Around line 83-94: The cache is mis-typed and safeCast incorrectly handles
lists: change the GLOBAL_FUNCTIONS cache to
GenericMapCache<List<CachedFunction>> and update FieldActionsCacheService usages
to store and retrieve List<CachedFunction> (so types align), and fix
GenericMapCache.safeCast(Object) to avoid calling getFirst() on an empty list
(use isEmpty()/get(0) or check size>0 before inspecting element) and only
perform element-type checks when the cache value type is the element type
(otherwise allow casting the entire List to V).
♻️ Duplicate comments (2)
application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (2)
13-17: Remove unused@Slf4jannotation.The
@Slf4jannotation generates a logger field, but no logging is performed anywhere in this class (e.g., inclear()or exception handling).
29-48: Add@SuppressWarnings("unchecked")for the unchecked cast.Line 43 performs an unchecked cast from
VtoT. WhilesafeCastensures the value conforms toV, there's no compile-time guarantee thatTis assignable fromV. Add the suppression annotation to acknowledge this intentional cast.🧹 Suggested fix
`@Override` +@SuppressWarnings("unchecked") public <T> T get(Object key, Callable<T> loader) {
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 363
File: application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java:66-69
Timestamp: 2026-01-08T08:38:06.491Z
Learning: In the CacheConfigurationProperties class, the cache property for namespace/global functions is named `globalFunctions`, not `namespaceFunctions`. This property was renamed as part of the refactoring from namespace-scoped to global-scoped function caching.
⏰ 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: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (3)
application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (3)
25-27: LGTM!The
getNativeCache()implementation is now safe. Sinceput()evicts on null values (lines 66-68) andConcurrentHashMaprejects null values,Map.copyOf()will not encounter null values.
50-62: LGTM!Both
getoverloads correctly implement the Spring Cache contract: returningnullon cache miss and properly wrapping/casting values on hit.
64-81: LGTM!The
put()method correctly handles null values by evicting the key, preventingConcurrentHashMapNPE. Theclear()method now clears in place, preserving native cache identity as recommended in past reviews.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
...ation-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java
Show resolved
Hide resolved
Introduce an additional generic type parameter in `GenericMapCache` to ensure proper type handling for list elements. Update `safeCast` logic to validate element types in lists and modify related cache configurations accordingly. Remove redundant cache eviction from `FieldActionsCacheService`.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java`:
- Line 19: The import java.lang.annotation.ElementType in CacheConfiguration is
unused; remove that import line from the CacheConfiguration class so only
necessary imports remain (keep other imports such as
org.springframework.cache.annotation.EnableCaching and
org.springframework.context.annotation.Configuration intact).
In
`@application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java`:
- Around line 71-77: In reloadCachedGlobalFunctions, always evict the cache
entry for the given processIdentifier before calling
petriNetService.getDefaultVersionByIdentifier(processIdentifier) so stale
functions aren't left when the PetriNet is deleted; after eviction, fetch the
PetriNet and only call cachePetriNetFunctions(petriNet) if the fetched petriNet
is non-null. Ensure you use the class's existing cache-eviction method (invoke
it at the start of reloadCachedGlobalFunctions) then proceed with the current
null-check and repopulation logic.
♻️ Duplicate comments (2)
application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (2)
13-14: Remove unused@Slf4jannotation.The
@Slf4jannotation generates a logger field, but no logging is performed in this class.🧹 Suggested fix
-@Slf4j public class GenericMapCache<V, E> implements Cache {
31-50: Add@SuppressWarnings("unchecked")for the unavoidable cast.Line 45 performs an unchecked cast
(T) value. While this is inherent to the Spring Cache contract where the caller asserts the type, adding the annotation documents the intentional cast.🧹 Suggested fix
`@Override` +@SuppressWarnings("unchecked") public <T> T get(Object key, Callable<T> loader) {
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 363
File: application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java:66-69
Timestamp: 2026-01-08T08:38:06.491Z
Learning: In the CacheConfigurationProperties class, the cache property for namespace/global functions is named `globalFunctions`, not `namespaceFunctions`. This property was renamed as part of the refactoring from namespace-scoped to global-scoped function caching.
📚 Learning: 2026-01-08T08:38:06.491Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 363
File: application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java:66-69
Timestamp: 2026-01-08T08:38:06.491Z
Learning: In the CacheConfigurationProperties class, the cache property for namespace/global functions is named `globalFunctions`, not `namespaceFunctions`. This property was renamed as part of the refactoring from namespace-scoped to global-scoped function caching.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
📚 Learning: 2025-12-19T09:06:38.316Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 398
File: application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java:57-58
Timestamp: 2025-12-19T09:06:38.316Z
Learning: In the file `application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java`, the `definition` attribute of the `Action` class (from `com.netgrif.application.engine.objects.petrinet.domain.dataset.logic.action.Action`) is never null, so null checks for `action.getDefinition()` are not needed.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
📚 Learning: 2025-11-14T10:22:01.634Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 383
File: application-engine/src/main/java/com/netgrif/application/engine/startup/ApplicationRunnerOrderResolver.java:43-43
Timestamp: 2025-11-14T10:22:01.634Z
Learning: For the netgrif/application-engine repository, avoid flagging trivial or nitpick-level issues such as redundant null checks, minor code style improvements, or obvious simplifications that don't affect functionality or introduce bugs. Focus review comments on substantive issues like logic errors, security concerns, performance problems, or breaking changes.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
📚 Learning: 2025-07-31T23:40:46.499Z
Learnt from: tuplle
Repo: netgrif/application-engine PR: 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/workflow/service/FieldActionsCacheService.java
📚 Learning: 2025-09-29T10:31:57.325Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 362
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java:513-529
Timestamp: 2025-09-29T10:31:57.325Z
Learning: PetriNet.getStringId() returns a simple ObjectId string representation (_id.toString()), not a composite Netgrif ID format, so new ObjectId(petriNetId) works correctly when petriNetId comes from PetriNet.getStringId().
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
🧬 Code graph analysis (1)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (1)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheMapKeys.java (1)
CacheMapKeys(3-8)
⏰ 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: task-list-completed
- GitHub Check: Build
🔇 Additional comments (9)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java (1)
38-59: LGTM!The cache configuration correctly defines type parameters for each specialized cache:
ACTIONScache storesClosurevaluesFUNCTIONScache storesCachedFunctionvaluesGLOBAL_FUNCTIONScache storesList<CachedFunction>with proper element type validationThe use of
SimpleCacheManagerto aggregate both genericConcurrentMapCacheinstances and specializedGenericMapCacheinstances is a clean approach.application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (3)
27-29: LGTM!The
getNativeCache()implementation usingMap.copyOf(map)is safe because:
ConcurrentHashMapdoesn't allow null values- The
put()method evicts the key instead of storing null (lines 68-70)This ensures the backing map never contains null values, preventing
NullPointerExceptionfromMap.copyOf().
52-83: LGTM!The Cache interface methods are correctly implemented:
get(Object key)returnsnullon cache miss per Spring Cache contractput()defensively evicts on null values, preventingConcurrentHashMapNPEclear()clears in-place, preserving native cache references
85-123: LGTM!The
safeCastmethod correctly handles:
- Direct type matching via
isInstancecheck- List-typed caches with element type validation
- Empty lists without throwing
NoSuchElementException(addressed from past review)The element-type validation against only the first element is a reasonable performance tradeoff for homogeneous lists.
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (5)
39-43: LGTM!The constructor properly initializes dependencies, and the lazy setter for
petriNetService(lines 45-49) appropriately breaks potential circular dependencies.
51-69: LGTM!The method correctly:
- Guards against null input
- Caches functions when present
- Evicts stale entries when no global functions exist (preventing stale cache data)
86-98: LGTM!The method correctly caches compiled closures and properly reuses the
wrappervariable instead of performing a redundant cache lookup (addressed from past review).
100-117: LGTM!The method correctly handles cache lookups with proper null checks on
ValueWrapperbefore calling.get().
204-210: LGTM!The
getRequiredCachehelper provides fail-fast behavior with a clear error message when a cache is misconfigured. This centralizes null-checking and improves error diagnostics.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
...on-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java
Outdated
Show resolved
Hide resolved
.../src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
Show resolved
Hide resolved
Ensure the global functions cache is cleared for the given process identifier before reloading. This prevents stale data from persisting and ensures updated functions are cached correctly.
The import for `ElementType` was unnecessary and has been removed to clean up the code. This change improves maintainability by keeping the imports list concise and relevant.
Reordered method logic to ensure cache eviction precedes PetriNet retrieval. This avoids potential redundant operations and ensures consistent caching behavior. No functional changes were introduced.
|
@coderabbitai I made the change that you wanted, can you please review your last request? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 255969
To help me review your changes effectively, could you please clarify:
Alternatively, if you'd like me to conduct a fresh review of the latest changes, I can scan the most recent commits to see what was updated. Would you prefer that I:
🧠 Learnings used |
|
@coderabbitai can you please review once more and accept if everything is OK? |
🧩 Analysis chain🏁 Script executed: Length of output: 3971 🏁 Script executed: Length of output: 131 🏁 Script executed: Length of output: 2304 🏁 Script executed: Length of output: 641 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 614 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 7835 🏁 Script executed: Length of output: 8198 🏁 Script executed: Length of output: 67 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 112 🏁 Script executed: Length of output: 266189 🏁 Script executed: Length of output: 9432 🏁 Script executed: Length of output: 1383
What I verified
Non-blocking suggestion
Everything else looks good to me. Approving now. 🧠 Learnings used✅ Actions performedComments resolved and changes approved. |
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
Blocking PRs
This PR depends on NAE-2212/PR #363
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
Bug Fixes
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.