[NAE-2212] Cluster scope process function caches #363
Conversation
WalkthroughRefactors function caching from namespace-scoped to a global cache, adds paged bulk caching of PetriNets, maps deprecated "namespace" scope to GLOBAL, and shifts many PetriNet lookups from "newest" to "active" semantics; also adds versionActive flags, import/version activation behavior, and related tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IFieldActionsCacheService.java(1 hunks)
⏰ 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). (1)
- GitHub Check: Build
🔇 Additional comments (1)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (1)
42-45: Constructor change is safe — no manual instantiations foundrg output shows only the class declaration; no
new FieldActionsCacheService(...)or other constructor references. The class is annotated@Service, so Spring will injectMongoTemplate.
.../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
| void cacheAllPetriNetsFunctions(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define the contract (scope, version, and cache semantics) for the new bulk method.
Please document whether this:
- processes only newest PetriNet versions or all historical ones,
- clears stale entries for deleted PetriNets before rebuilding,
- is intended to be long‑running and should be invoked off the request thread.
Consider a tighter name for consistency with the singular: cacheAllPetriNetFunctions().
Apply this diff to add Javadoc:
- void cacheAllPetriNetsFunctions();
+ /**
+ * Pre-populates the namespace function cache for all PetriNets.
+ * Contract:
+ * - Only newest version per PetriNet identifier SHOULD be cached.
+ * - Stale cache entries for non-existing PetriNets SHOULD be cleared or overwritten.
+ * - This operation MAY be long-running; invoke off request threads.
+ */
+ void cacheAllPetriNetsFunctions();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void cacheAllPetriNetsFunctions(); | |
| /** | |
| * Pre-populates the namespace function cache for all PetriNets. | |
| * Contract: | |
| * - Only newest version per PetriNet identifier SHOULD be cached. | |
| * - Stale cache entries for non-existing PetriNets SHOULD be cleared or overwritten. | |
| * - This operation MAY be long-running; invoke off request threads. | |
| */ | |
| void cacheAllPetriNetsFunctions(); |
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IFieldActionsCacheService.java
around lines 30-31, add Javadoc for the bulk caching method that defines its
contract: clarify whether it processes only the newest PetriNet versions or all
historical versions, state whether it clears stale entries for deleted PetriNets
before rebuilding the cache, and indicate that it is long-running and should be
called off the request thread (or provide async variant). Also rename the method
to cacheAllPetriNetFunctions() for consistency with the singular form and
reflect the chosen semantics (scope, version policy, cache eviction strategy,
and threading expectations) in the Javadoc summary and
parameter/return/exception notes.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 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(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/cache/CacheMapKeys.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/cache/ReadOnlyCache.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/cache/WorkerConcurrentCacheManager.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java(2 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/cache/WorkerConcurrentCacheManager.java (1)
WorkerConcurrentCacheManager(6-23)
application-engine/src/main/java/com/netgrif/application/engine/configuration/cache/WorkerConcurrentCacheManager.java (1)
application-engine/src/main/java/com/netgrif/application/engine/configuration/cache/CacheMapKeys.java (1)
CacheMapKeys(3-5)
⏰ 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). (3)
- GitHub Check: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (3)
application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java (1)
44-48: Configurable name vs hard‑coded constant: ensure they stay in syncSince this name is configurable, hard-coding a different literal in WorkerConcurrentCacheManager/CacheMapKeys breaks the intended read‑only behavior. Either keep the constant aligned (see prior comment) or have the manager read this value.
application-engine/src/main/java/com/netgrif/application/engine/configuration/cache/ReadOnlyCache.java (1)
30-41: Enforce consistent immutability or update naming
ReadOnlyCache permits put(...) but throws on evict/clear, which conflicts with “read-only.” Decide whether to block put (and override Spring 6.x defaults like putIfAbsent and evictIfPresent) for true immutability, or rename to NoEvictCache if only invalidation should be disallowed. Confirm intended behavior.application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java (1)
27-27: Incorrect suggestion—missing constructor: WorkerConcurrentCacheManager only definesWorkerConcurrentCacheManager()andWorkerConcurrentCacheManager(String… ), so callingnew WorkerConcurrentCacheManager(Set<String>, String… )won’t compile.Likely an incorrect or invalid review comment.
| public class CacheMapKeys { | ||
| public static final String NAMESPACE_FUNCTIONS = "namespaceFunctionsCache"; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make constants holder non-instantiable and final
Prevents accidental instantiation and clarifies intent.
-public class CacheMapKeys {
+public final class CacheMapKeys {
+ private CacheMapKeys() {}
public static final String NAMESPACE_FUNCTIONS = "namespaceFunctions";
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/configuration/cache/CacheMapKeys.java
around lines 3 to 5, the constants holder class should be made non-instantiable
and final; change the class declaration to final and add a private no-argument
constructor (preferably throwing an AssertionError) to prevent instantiation and
clarify intent while leaving the existing public static constant unchanged.
...on-engine/src/main/java/com/netgrif/application/engine/configuration/cache/CacheMapKeys.java
Outdated
Show resolved
Hide resolved
| public Set<String> getAllCaches() { | ||
| Set<String> caches = new LinkedHashSet<>(Arrays.asList(petriNetById, petriNetByIdentifier, petriNetNewest, petriNetCache, loadedModules)); | ||
| Set<String> caches = new LinkedHashSet<>(Arrays.asList(petriNetById, petriNetByIdentifier, petriNetNewest, petriNetCache, loadedModules, namespaceFunctions)); | ||
| caches.addAll(additional); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Guard against future drift and blank values
If users override namespaceFunctions in config, the read‑only wrapping must follow that value. Recommend passing this property into the CacheManager instead of relying on a constant. Also consider ignoring blank entries to avoid empty‑named caches.
Possible follow-up (see proposed constructor in WorkerConcurrentCacheManager and bean wiring in CacheConfiguration).
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/cache/CacheMapKeys.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/cache/WorkerConcurrentCacheManager.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java (2)
application-engine/src/main/java/com/netgrif/application/engine/configuration/cache/CacheMapKeys.java (1)
CacheMapKeys(3-6)application-engine/src/main/java/com/netgrif/application/engine/configuration/cache/WorkerConcurrentCacheManager.java (1)
WorkerConcurrentCacheManager(9-35)
⏰ 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 (4)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (1)
103-120: Prevent caching stale PetriNet revisions
petriNetService.getAll(pageable)returns every stored revision. Visiting them withcachePetriNetFunctionsmeans the last revision encountered wins, so if an older version shows up later in the page order the namespace cache ends up serving obsolete code. We also never evict entries for PetriNets that were deleted, so their namespace functions linger indefinitely. Clear the cache before the rebuild and resolve each identifier to its newest revision before caching.Apply a fix along these lines:
- Pageable pageable = PageRequest.of(0, 500); - Page<PetriNet> page = petriNetService.getAll(pageable); + clearNamespaceFunctionCache(); + Pageable pageable = PageRequest.of(0, 500); + Page<PetriNet> page = petriNetService.getAll(pageable); @@ - cachePetriNetFunctions(petriNet); + reloadCachedFunctions(petriNet);application-engine/src/main/java/com/netgrif/application/engine/configuration/cache/CacheMapKeys.java (1)
3-6: LGTM: constants holder is final with a private ctor and aligned cache keyMatches naming in configuration and prevents instantiation. No further action.
application-engine/src/main/java/com/netgrif/application/engine/configuration/cache/WorkerConcurrentCacheManager.java (1)
27-34: LGTM: read-only wrapping at creation pointWrapping via ReadOnlyCache on cache creation is correct and sufficient.
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java (1)
30-31: Namespace functions cache exposed Lombok @DaTa generates getNamespaceFunctions(), and getAllCaches() includes namespaceFunctions. No code change needed; verify each environment’s netgrif.engine.cache.namespace-functions property is set correctly or relies on the default.
...in/java/com/netgrif/application/engine/configuration/cache/WorkerConcurrentCacheManager.java
Outdated
Show resolved
Hide resolved
...on-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java
Outdated
Show resolved
Hide resolved
machacjozef
changed the title
machacjozef
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
application-engine/src/main/java/com/netgrif/application/engine/configuration/cache/WorkerConcurrentCacheManager.java(1 hunks)nae-object-library/src/main/resources/petriflow_schema.xsd(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Pull Request
nae-object-library/src/main/resources/petriflow_schema.xsd
[error] 2-35: XJC error: SAXParseException: s4s-elt-character: Non-whitespace characters are not allowed in schema elements other than 'xs:appinfo' and 'xs:documentation'. Saw '301 Moved Permanently'.
[error] 4-36: XJC error: SAXParseException: s4s-elt-character: Non-whitespace characters are not allowed in schema elements other than 'xs:appinfo' and 'xs:documentation'. Saw '301 Moved Permanently'.
[error] 5-20: XJC error: SAXParseException: s4s-elt-character: Non-whitespace characters are not allowed in schema elements other than 'xs:appinfo' and 'xs:documentation'. Saw 'nginx'.
[error] 6-3: XJC error: The element type "hr" must be terminated by the matching end-tag "".
⏰ 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). (1)
- GitHub Check: task-list-completed
🔇 Additional comments (4)
application-engine/src/main/java/com/netgrif/application/engine/configuration/cache/WorkerConcurrentCacheManager.java (4)
6-8: Use immutable Set and null-safety imports (Objects, Set) instead of List/ArrayListSwitching to Set enables O(1) membership checks and, with defensive copying, removes thread-safety risks from external mutation.
-import java.util.ArrayList; -import java.util.List; +import java.util.Objects; +import java.util.Set;
9-15: Make readOnlyCacheNames final and non-null with an immutable defaultCurrent mutable List can be altered after construction, causing racy behavior. Make it final and default to Set.of().
-public class WorkerConcurrentCacheManager extends ConcurrentMapCacheManager { - private List<String> readOnlyCacheNames; +public class WorkerConcurrentCacheManager extends ConcurrentMapCacheManager { + private final Set<String> readOnlyCacheNames; @@ public WorkerConcurrentCacheManager() { super(); - this.readOnlyCacheNames = new ArrayList<>(); + this.readOnlyCacheNames = Set.of(); }
17-25: Defensively copy constructor args and accept Set; add null checksThe constructors leak internal state and allow null. Copy into an immutable Set and enforce non-null.
- public WorkerConcurrentCacheManager(List<String> readOnlyCacheNames) { + public WorkerConcurrentCacheManager(Set<String> readOnlyCacheNames) { super(); - this.readOnlyCacheNames = readOnlyCacheNames; + this.readOnlyCacheNames = Set.copyOf(Objects.requireNonNull(readOnlyCacheNames, "readOnlyCacheNames")); } @@ - public WorkerConcurrentCacheManager(List<String> readOnlyCacheNames, String... cacheNames) { + public WorkerConcurrentCacheManager(Set<String> readOnlyCacheNames, String... cacheNames) { super(cacheNames); - this.readOnlyCacheNames = readOnlyCacheNames; + this.readOnlyCacheNames = Set.copyOf(Objects.requireNonNull(readOnlyCacheNames, "readOnlyCacheNames")); }
27-34: Read-only cache names include namespace‐functions constant. WorkerConcurrentCacheManager is constructed with List.of(CacheMapKeys.NAMESPACE_FUNCTIONS), covering the Petri-net namespace-functions cache used by FieldActionsCacheService; no naming drift detected.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
application-engine/src/main/java/com/netgrif/application/engine/configuration/cache/WorkerConcurrentCacheManager.java (3)
34-34: Remove unnecessary null check on non-null final field.Since
readOnlyCacheNamesisfinaland always initialized to a non-nullSetin all constructors, the null check is redundant.Apply this diff:
- if (readOnlyCacheNames != null && readOnlyCacheNames.contains(name)) { + if (readOnlyCacheNames.contains(name)) {
34-34: Remove unnecessary null check on non-null final field.The
readOnlyCacheNamesfield isfinaland initialized to a non-null value in all constructors. The null check at line 34 is redundant and adds unnecessary complexity.Apply this diff:
@Override protected Cache createConcurrentMapCache(String name) { Cache base = super.createConcurrentMapCache(name); - if (readOnlyCacheNames != null && readOnlyCacheNames.contains(name)) { + if (readOnlyCacheNames.contains(name)) { return new ReadOnlyCache(base); } return base; }
31-38: Remove redundant null check onreadOnlyCacheNames.The field
readOnlyCacheNamesisfinaland guaranteed non-null by all constructors, so the null check on line 34 is redundant and can be simplified.Apply this diff to simplify the logic:
@Override protected Cache createConcurrentMapCache(String name) { Cache base = super.createConcurrentMapCache(name); - if (readOnlyCacheNames != null && readOnlyCacheNames.contains(name)) { + if (readOnlyCacheNames.contains(name)) { return new ReadOnlyCache(base); } return base; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/cache/WorkerConcurrentCacheManager.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java (1)
application-engine/src/main/java/com/netgrif/application/engine/configuration/cache/WorkerConcurrentCacheManager.java (1)
WorkerConcurrentCacheManager(9-39)
⏰ 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). (6)
- 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 (11)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java (3)
30-30: LGTM! Past review feedback has been addressed.The cache manager instantiation correctly uses the configured namespace functions cache name from properties instead of a hard-coded constant, and properly passes a Set as the first argument to WorkerConcurrentCacheManager.
30-30: No null guard required for namespaceFunctions
CacheConfigurationProperties definesprivate String namespaceFunctions = "namespaceFunctions";so
getNamespaceFunctions()always returns this non-null default.
30-30: No null check needed for namespaceFunctions
CacheConfigurationPropertiesinitializesnamespaceFunctionswith a non-null default value ("namespaceFunctions"), soSet.of(properties.getNamespaceFunctions())cannot receive null.application-engine/src/main/java/com/netgrif/application/engine/configuration/cache/WorkerConcurrentCacheManager.java (8)
10-29: LGTM! Constructors follow immutability and defensive copying best practices.The
readOnlyCacheNamesfield is properly declared final, constructors useSet.copyOf()for defensive immutability, and null checks with descriptive messages prevent NPEs. This addresses the past review feedback about making the field immutable and thread-safe.
10-10: Excellent: immutable, thread-safe field design.The
final Set<String>field with defensive copying in constructors correctly addresses the past refactor suggestion and ensures thread-safe, immutable state.
12-15: LGTM: Default constructor correctly initializes empty set.Using
Set.of()for the default case is clean and efficient.
17-22: LGTM: Proper defensive copying with null safety.The constructor correctly uses
Set.copyOf()withObjects.requireNonNull()to ensure immutability and prevent null values.
24-29: LGTM: Varargs constructor maintains consistency.The constructor properly delegates to the parent and applies the same defensive copying pattern for read-only cache names.
12-15: LGTM!The default constructor correctly initializes
readOnlyCacheNamesto an empty immutable set, ensuring the field is always non-null.
17-22: LGTM!The constructor correctly performs null validation and creates a defensive immutable copy of the provided set, ensuring thread-safety and immutability.
24-29: LGTM!The constructor correctly validates input, creates a defensive copy, and delegates cache name registration to the superclass. Immutability and null-safety are guaranteed.
| @Override | ||
| protected Cache createConcurrentMapCache(String name) { | ||
| Cache base = super.createConcurrentMapCache(name); | ||
| if (readOnlyCacheNames != null && readOnlyCacheNames.contains(name)) { | ||
| return new ReadOnlyCache(base); | ||
| } | ||
| return base; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify ReadOnlyCache correctly prevents all write operations.
The wrapper logic correctly delegates wrapping to ReadOnlyCache for caches in the read-only set. Ensure that ReadOnlyCache properly blocks all mutating operations (put, evict, clear, etc.) to maintain the read-only guarantee.
Run the following script to examine the ReadOnlyCache implementation:
🏁 Script executed:
#!/bin/bash
# Description: Verify ReadOnlyCache blocks all write operations.
# Search for ReadOnlyCache class and its method implementations
ast-grep --pattern $'class ReadOnlyCache {
$$$
}'
# Check for methods that throw UnsupportedOperationException
rg -nP -A3 'class ReadOnlyCache' --type=java
rg -nP 'UnsupportedOperationException' --type=java -g '**/ReadOnlyCache.java'Length of output: 913
🏁 Script executed:
#!/bin/bash
# Display the full ReadOnlyCache implementation to verify write-blocking methods
sed -n '1,200p' application-engine/src/main/java/com/netgrif/application/engine/configuration/cache/ReadOnlyCache.javaLength of output: 1198
🏁 Script executed:
#!/bin/bash
rg -n 'putIfAbsent' -g 'application-engine/src/main/java/com/netgrif/application/engine/configuration/cache/ReadOnlyCache.java'Length of output: 0
Enforce read-only behavior by blocking all mutating cache operations in ReadOnlyCache
ReadOnlyCache currently delegates put(Object,Object) and inherits default putIfAbsent(Object,Object), so write operations still succeed. Override both methods to throw UnsupportedOperationException to uphold the read-only guarantee.
File: application-engine/src/main/java/com/netgrif/application/engine/configuration/cache/ReadOnlyCache.java
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/configuration/cache/ReadOnlyCache.java
(around the class definition), the ReadOnlyCache currently delegates
put(Object,Object) and inherits putIfAbsent(Object,Object) which allows writes;
override both put(K,V) and putIfAbsent(K,V) to throw
UnsupportedOperationException to enforce true read-only behavior (ensure
signatures match the Cache interface used and include clear exception messages),
and run tests to confirm write attempts now fail.
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 (2)
.github/workflows/pr-build.yml(1 hunks)application-engine/docker-compose.yml(1 hunks)
⏰ 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 (1)
application-engine/docker-compose.yml (1)
46-46: Command addition looks good, but verify image consistency with CI.The addition of the MinIO startup command with the console address is valid and aligns with the exposed port 9001.
However, note an inconsistency: this docker-compose file uses the official
minio/minio:RELEASE.2022-12-12T19-27-27Zimage, while the CI workflow (.github/workflows/pr-build.ymlline 55) usesdocker.io/bawix/minio:2022. Consider standardizing on the same image across both environments to ensure consistent behavior and avoid configuration drift.Additionally, verify whether these MinIO changes are actually required for the stated PR objective (cluster-scoped function caching), as the connection is not immediately apparent.
machacjozef
changed the base branch from
release/7.0.0-rev8
to
release/7.0.0-rev9
- add attribute isVersionActive in PetriNet - introduce PetriNetService.getActiveVersionByIdentifier - rename PetriNetService.getNewestVersionByIdentifier to PetriNetService.getLatestVersionByIdentifier - switch usage of getNewestVersionByIdentifier to getActiveVersionByIdentifier - enhance process import validations
- remove todos - implement PetriNetServiceTest.testMakeVersionActive
- split PetriNetService.save into two methods
- rename version active attribute in PetriNet
- remove makeVersionActive API
- update PetriNetService.deletePetriNet to handle versionActive attribute - implement another test for process deletion
- mark methods import petriNet method transactional
- update javadoc
- add null checks
- add null check
- update javadoc
- add null check
- removed ReadOnlyCache.java, the use of this is not needed, because global scoped functions will be read from admin - removed WorkerConcurrentCacheManager because the ReadOnlyCache is not used - refactored name of namespace scoped functions to "global" - deprecated "namespace" function scope
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 (2)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IFieldActionsCacheService.java (1)
22-22: RenamegetNamespaceFunctionCache()for consistency with the global caching approach.The method still uses "Namespace" in its name while the underlying cache and related methods have been renamed to use "Global". This inconsistency may confuse API consumers.
- Map<String, List<CachedFunction>> getNamespaceFunctionCache(); + Map<String, List<CachedFunction>> getGlobalFunctionCache();nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/FunctionScope.java (1)
26-33: Handle backward compatibility for persisted "namespace" values.The
fromValue()method will throwIllegalArgumentExceptionif it encounters the old "namespace" value from existing persisted data. WhileFunctionFactoryhandles the deprecation during import, data already stored with"namespace"scope will fail to deserialize.Consider adding backward compatibility mapping:
public static FunctionScope fromValue(String v) { + if ("namespace".equals(v)) { + return GLOBAL; // backward compatibility for deprecated NAMESPACE scope + } for (FunctionScope c : FunctionScope.values()) { if (c.value.equals(v)) { return c; } } throw new IllegalArgumentException(v); }
♻️ Duplicate comments (3)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IFieldActionsCacheService.java (1)
28-30: LGTM on the method rename and addition.The rename from
clearNamespaceFunctionCache()toclearGlobalFunctionCache()aligns with the global caching strategy. The newcacheAllPetriNetsFunctions()method supports bulk caching across the cluster.As noted in a previous review, consider adding Javadoc to document the contract (version policy, threading expectations, cache eviction strategy) for
cacheAllPetriNetsFunctions().application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (2)
36-37: Consider thread-safety for the global functions cache in a clustered environment.
MaxSizeHashMap(likely extendingLinkedHashMap) is not thread-safe. In a cluster scenario with concurrent cache mutations from multiple threads, this could lead to data corruption orConcurrentModificationException.Consider using a thread-safe alternative:
-private Map<String, List<CachedFunction>> globalFunctionsCache; +private volatile Map<String, List<CachedFunction>> globalFunctionsCache;Or use
Collections.synchronizedMap()orConcurrentHashMapwith manual LRU eviction. Thevolatilekeyword at minimum ensures visibility of the reference swap inclearGlobalFunctionCache().
101-121: UsereloadCachedFunctions()instead ofcachePetriNetFunctions()incacheAllPetriNetsFunctions().The
petriNetService.getAll(pageable)method returns all PetriNet versions without filtering for the newest. Since the cache is keyed by identifier only, iterating through multiple versions of the same PetriNet results in the last version processed overwriting earlier ones—potentially leaving outdated function definitions cached. ThereloadCachedFunctions()method explicitly fetches the newest version viagetNewestVersionByIdentifier(), ensuring only the current definition is cached.for (PetriNet petriNet : page) { try { - cachePetriNetFunctions(petriNet); + reloadCachedFunctions(petriNet); } catch (Exception e) { log.warn("Failed to cache functions for PetriNet id={}", petriNet.getStringId(), e); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/RunnerConfigurationProperties.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/importer/service/FunctionFactory.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java(6 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IFieldActionsCacheService.java(1 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/TestHelper.groovy(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/FunctionScope.java(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-05T10:21:54.893Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 350
File: application-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovy:135-135
Timestamp: 2025-09-05T10:21:54.893Z
Learning: In ExportServiceTest.groovy, writing to src/test/resources is intentional to simulate production behavior where the working tree is mutated during file exports. This mirrors how the system works in production.
Applied to files:
application-engine/src/test/groovy/com/netgrif/application/engine/TestHelper.groovy
📚 Learning: 2025-12-12T12:37:58.852Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/test/groovy/com/netgrif/application/engine/auth/WorkflowAuthorizationServiceTest.groovy:4-4
Timestamp: 2025-12-12T12:37:58.852Z
Learning: In tests under application-engine/src/test, GroupService.getDefaultSystemGroup() lazily creates the default system group if it doesn't exist. Tests can rely on this behavior and omit explicit existence/setup checks for the default system group.
Applied to files:
application-engine/src/test/groovy/com/netgrif/application/engine/TestHelper.groovy
📚 Learning: 2025-12-12T12:40:25.863Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovy:18-20
Timestamp: 2025-12-12T12:40:25.863Z
Learning: In Groovy/Java, if a parameter or collection expects ActorFieldValue, you can pass UserFieldValue (a subclass) due to polymorphism. In tests, verify that ActorListFieldValue accepts ActorFieldValue instances and that passing UserFieldValue instances is valid without casts. This pattern applies to similar Groovy files across the repository.
Applied to files:
application-engine/src/test/groovy/com/netgrif/application/engine/TestHelper.groovy
📚 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/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
🪛 GitHub Actions: Pull Request
application-engine/src/main/java/com/netgrif/application/engine/importer/service/FunctionFactory.java
[error] 29-29: cannot find symbol: variable GLOBAL
location: class com.netgrif.application.engine.objects.importer.model.Scope
⏰ 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). (3)
- 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/CacheConfiguration.java (1)
13-13: LGTM!The blank line improves readability by providing visual separation between the imports and the class annotations.
application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/RunnerConfigurationProperties.java (1)
59-59: No property rename occurred—this is a newly created configuration file.This file is being added fresh as part of the new feature [NAE-2212]. The property
globalFunctionsCacheSizeis a new configuration property, not a renamed one. There is no previousnamespaceCacheSizeproperty in the codebase and no backward compatibility concerns.Likely an incorrect or invalid review comment.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/FunctionScope.java (1)
3-18: LGTM on the Javadoc additions and GLOBAL constant.The documentation clearly explains the enum's purpose and the scope constants.
application-engine/src/test/groovy/com/netgrif/application/engine/TestHelper.groovy (1)
96-96: LGTM!The method call update aligns with the renamed interface method
clearGlobalFunctionCache().application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (2)
55-71: LGTM on the scope migration to GLOBAL.The function caching logic correctly uses
FunctionScope.GLOBALand manages theglobalFunctionsCacheappropriately, including removal when no functions exist.
171-173: LGTM on the renamed clear method.The implementation correctly reinitializes the
globalFunctionsCachewith the configured size.
| /** | ||
| * The size of the cache used for maintaining field runner namespaces. | ||
| */ |
There was a problem hiding this comment.
Update the JavaDoc comment to reflect the global functions cache.
The comment still references "maintaining field runner namespaces" but the property has been renamed to globalFunctionsCacheSize and serves a different purpose (global functions caching).
Apply this diff to update the comment:
- /**
- * The size of the cache used for maintaining field runner namespaces.
- */
+ /**
+ * The size of the cache used for managing global Petri net namespace functions.
+ */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * The size of the cache used for maintaining field runner namespaces. | |
| */ | |
| /** | |
| * The size of the cache used for managing global Petri net namespace functions. | |
| */ |
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/RunnerConfigurationProperties.java
around lines 56 to 58, the JavaDoc still refers to "maintaining field runner
namespaces" but the property is now named globalFunctionsCacheSize and is used
for caching global functions; update the JavaDoc to reference
globalFunctionsCacheSize and describe that it controls the size of the cache for
global functions (e.g., "The size of the cache used for storing global
functions"), ensuring the comment matches the renamed property and its purpose.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| private void checkDeprecatedAttributes(com.netgrif.application.engine.objects.importer.model.Function function) { | ||
| if (function.getScope() != null && function.getScope().name().equals(NAMESPACE)) { | ||
| log.warn("Function scope [NAMESPACE] is deprecated. Replacing with [GLOBAL].]"); | ||
| function.setScope(Scope.GLOBAL); | ||
| } | ||
| } |
There was a problem hiding this comment.
Build failure: Scope.GLOBAL does not exist in the importer model.
The pipeline failure indicates that Scope.GLOBAL cannot be found in com.netgrif.application.engine.objects.importer.model.Scope. The GLOBAL constant was added to FunctionScope (domain enum), but the importer model's Scope enum was not updated.
You need to either:
- Add
GLOBALto the importer model'sScopeenum, or - Map directly to
FunctionScopeinstead of mutating the importer model's scope.
Option 1 - Add GLOBAL to the importer Scope enum (in the XSD/generated model):
// In the Scope enum definition (likely generated from XSD)
GLOBAL("global")Option 2 - Skip mutating the input and handle mapping at conversion time:
private void checkDeprecatedAttributes(com.netgrif.application.engine.objects.importer.model.Function function) {
if (function.getScope() != null && function.getScope().name().equals(NAMESPACE)) {
log.warn("Function scope [NAMESPACE] is deprecated. Replacing with [GLOBAL].]");
- function.setScope(Scope.GLOBAL);
+ // Handle mapping in getFunction() when setting FunctionScope
}
}Then adjust getFunction():
public Function getFunction(com.netgrif.application.engine.objects.importer.model.Function function) {
- checkDeprecatedAttributes(function);
Function function1 = new Function();
function1.setDefinition(function.getValue());
function1.setName(function.getName());
- function1.setScope(FunctionScope.valueOf(function.getScope().name()));
+ function1.setScope(mapScope(function.getScope()));
return function1;
}
+private FunctionScope mapScope(Scope scope) {
+ if (scope == null) {
+ return FunctionScope.PROCESS; // or appropriate default
+ }
+ if (NAMESPACE.equals(scope.name().toLowerCase())) {
+ log.warn("Function scope [NAMESPACE] is deprecated. Replacing with [GLOBAL].");
+ return FunctionScope.GLOBAL;
+ }
+ return FunctionScope.valueOf(scope.name());
+}🧰 Tools
🪛 GitHub Actions: Pull Request
[error] 29-29: cannot find symbol: variable GLOBAL
location: class com.netgrif.application.engine.objects.importer.model.Scope
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/importer/service/FunctionFactory.java
around lines 26-31, the code calls Scope.GLOBAL which does not exist in the
importer model; either add GLOBAL to the importer Scope enum (update the
XSD/generated model and regenerate so Scope has GLOBAL("global")) or avoid
mutating the importer model here and instead only log the deprecation and
perform the mapping when converting to the domain enum (map importer
Scope.NAMESPACE -> domain FunctionScope.GLOBAL in getFunction()/conversion
logic). Implement one of these two fixes and remove the invalid call to
setScope(Scope.GLOBAL) if you choose the second approach.
There was a problem hiding this comment.
the XSD is not updated yet. This will work when the XSD gets updated
There was a problem hiding this comment.
@Retoocs, understood! This PR has a dependency on the XSD update being merged first. Once the importer model's Scope enum includes the GLOBAL constant (after XSD regeneration), the build will succeed.
Just to confirm the merge order:
- XSD update (adds
GLOBALto the importer model'sScopeenum) - This PR (which references
Scope.GLOBAL)
This approach makes sense as it keeps the domain model (FunctionScope) and importer model (Scope) in sync. The build failure is expected until the XSD dependency is resolved.
| if (function.getScope() != null && function.getScope().name().equals(NAMESPACE)) { | ||
| log.warn("Function scope [NAMESPACE] is deprecated. Replacing with [GLOBAL].]"); |
There was a problem hiding this comment.
Minor: Log message has a typo with double closing bracket.
- log.warn("Function scope [NAMESPACE] is deprecated. Replacing with [GLOBAL].]");
+ log.warn("Function scope [NAMESPACE] is deprecated. Replacing with [GLOBAL].");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (function.getScope() != null && function.getScope().name().equals(NAMESPACE)) { | |
| log.warn("Function scope [NAMESPACE] is deprecated. Replacing with [GLOBAL].]"); | |
| if (function.getScope() != null && function.getScope().name().equals(NAMESPACE)) { | |
| log.warn("Function scope [NAMESPACE] is deprecated. Replacing with [GLOBAL]."); |
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/importer/service/FunctionFactory.java
around lines 27 to 28, the log message contains a typo with an extra closing
bracket at the end; remove the redundant bracket so the warning reads without
the double closing bracket and retains the same wording and severity.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| @Override | ||
| public Map<String, List<CachedFunction>> getNamespaceFunctionCache() { | ||
| return new HashMap<>(namespaceFunctionsCache); | ||
| return new HashMap<>(globalFunctionsCache); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Rename method to match the global caching convention.
The method name getNamespaceFunctionCache() is inconsistent with the renamed field globalFunctionsCache and other renamed methods. This should be updated for API consistency.
@Override
-public Map<String, List<CachedFunction>> getNamespaceFunctionCache() {
+public Map<String, List<CachedFunction>> getGlobalFunctionCache() {
return new HashMap<>(globalFunctionsCache);
}Note: This requires updating the interface as well (flagged separately).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
around lines 160 to 163, the method name getNamespaceFunctionCache() is
inconsistent with the renamed field globalFunctionsCache and should be renamed
to getGlobalFunctionCache (or getGlobalFunctionsCache to match pluralization) to
follow the global caching convention; update the method name, its return
behavior (still return new HashMap<>(globalFunctionsCache)), and update the
corresponding declaration in the interface so signatures match across
implementation and interface.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| /** | ||
| * Default cache name for caching namespace functions. | ||
| */ | ||
| private String namespaceFunctions = "namespaceFunctions"; |
There was a problem hiding this comment.
shouldnt this be "globalFunctions"?
| /** | ||
| * The size of the cache used for maintaining field runner namespaces. | ||
| */ |
| if (function.getScope() != null && function.getScope().name().equals(NAMESPACE)) { | ||
| log.warn("Function scope [NAMESPACE] is deprecated. Replacing with [GLOBAL].]"); |
| private static final String NAMESPACE = "namespace"; | ||
|
|
||
| public Function getFunction(com.netgrif.application.engine.objects.importer.model.Function function) { | ||
| checkDeprecatedAttributes(function); |
There was a problem hiding this comment.
For deprecated attributes we already use interface IModelValidator. Please implement this interface for functions and use it
| private void checkDeprecatedAttributes(com.netgrif.application.engine.objects.importer.model.Function function) { | ||
| if (function.getScope() != null && function.getScope().name().equals(NAMESPACE)) { | ||
| log.warn("Function scope [NAMESPACE] is deprecated. Replacing with [GLOBAL].]"); | ||
| function.setScope(Scope.GLOBAL); | ||
| } | ||
| } |
There was a problem hiding this comment.
the XSD is not updated yet. This will work when the XSD gets updated
| } | ||
|
|
||
| @Override | ||
| public void cacheAllPetriNetsFunctions() { |
| @Override | ||
| public Map<String, List<CachedFunction>> getNamespaceFunctionCache() { | ||
| return new HashMap<>(namespaceFunctionsCache); | ||
| return new HashMap<>(globalFunctionsCache); | ||
| } |
| /** | ||
| * Represents the global scope of a function. | ||
| */ | ||
| GLOBAL("global"); |
There was a problem hiding this comment.
Please update also documentation. The repository netgrif-platform-docs
…2212_w_2266 # Conflicts: # application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java # application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
- renamed function in IFieldActionsCacheService according to PR
coderabbitai
bot
added
improvement
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/PetriNet.java (1)
63-107: Remove commented code or track the decision in a proper issue.This change appears unrelated to the PR's core objective of implementing cluster-scoped function caching (NAE-2212). The TODO comment and the extensive commented-out clone method (44 lines) should either be:
- Deleted immediately if the clone method is confirmed as unnecessary, or
- Tracked in a dedicated issue if further evaluation is needed
Keeping commented-out code with inline TODOs adds maintenance burden without providing value. Consider moving this cleanup to a separate PR focused on code hygiene.
🔎 If the clone method is not needed, apply this diff to remove it entirely:
- // todo: delete clone method if not needed -// @Override -// public PetriNet clone() { -// PetriNet clone = new PetriNet(); -// clone.setIdentifier(this.getIdentifier()); -// clone.setUriNodeId(this.getUriNodeId()); -// clone.setInitials(this.getInitials()); -// clone.setTitle(this.getTitle().clone()); -// clone.setDefaultRoleEnabled(this.isDefaultRoleEnabled()); -// clone.setDefaultCaseName(this.getDefaultCaseName() == null ? null : this.getDefaultCaseName().clone()); -// clone.setDefaultCaseNameExpression(this.getDefaultCaseNameExpression() == null ? null : this.getDefaultCaseNameExpression().clone()); -// clone.setIcon(this.getIcon()); -// clone.setCreationDate(this.getCreationDate()); -// clone.setVersion(this.getVersion() == null ? null : this.getVersion().clone()); -// clone.setAuthor(this.getAuthor() == null ? null : this.getAuthor().clone()); -// clone.setTransitions(this.getTransitions() == null ? null : this.getTransitions().entrySet().stream().collect(Collectors.toMap(Map.Entry::getKey, e -> new com.netgrif.application.engine.adapter.spring.petrinet.domain.Transition(e.getValue()), (v1, v2) -> v1, LinkedHashMap::new))); -// clone.setRoles(this.roles == null ? null : this.roles.entrySet().stream().collect(Collectors.toMap(Map.Entry::getKey, e -> new com.netgrif.application.engine.adapter.spring.petrinet.domain.roles.ProcessRole(e.getValue()), (v1, v2) -> v1, LinkedHashMap::new))); -// clone.setTransactions(this.getTransactions() == null ? null : this.getTransactions().entrySet().stream().collect(Collectors.toMap(Map.Entry::getKey, e -> new com.netgrif.application.engine.adapter.spring.petrinet.domain.Transaction(e.getValue()), (v1, v2) -> v1, LinkedHashMap::new))); -// clone.setImportXmlPath(this.getImportXmlPath()); -// clone.setImportId(this.importId); -// clone.setObjectId(this._id); -// clone.setDataSet(this.getDataSet().entrySet() -// .stream() -// .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().clone(), (x, y)->y, LinkedHashMap::new)) -// ); -// clone.setPlaces(this.getPlaces().entrySet() -// .stream() -// .collect(Collectors.toMap(Map.Entry::getKey, e -> new Place(e.getValue()), (x, y)->y, LinkedHashMap::new)) -// ); -// clone.setArcs(this.getArcs().entrySet() -// .stream() -// .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().stream() -// .map(Arc::clone) -// .collect(Collectors.toList()), (x,y)->y, LinkedHashMap::new)) -// ); -// clone.initializeArcs(); -// clone.setCaseEvents(this.getCaseEvents() == null ? null : this.getCaseEvents().entrySet().stream().collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().clone()))); -// clone.setProcessEvents(this.getProcessEvents() == null ? null : this.getProcessEvents().entrySet().stream().collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().clone()))); -// clone.setPermissions(this.getPermissions() == null ? null : this.getPermissions().entrySet().stream().collect(Collectors.toMap(Map.Entry::getKey, e -> new HashMap<>(e.getValue())))); -// clone.setUserRefs(this.getUserRefs() == null ? null : this.getUserRefs().entrySet().stream().collect(Collectors.toMap(Map.Entry::getKey, e -> new HashMap<>(e.getValue())))); -// this.getNegativeViewRoles().forEach(clone::addNegativeViewRole); -// this.getFunctions().forEach(clone::addFunction); -// clone.setTags(new HashMap<>(this.getTags())); -// return clone; -// }application-engine/src/main/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.java (1)
263-268: Add null check before accessing stringId.Line 264 could throw a NullPointerException if the "preference_filter_item" process is not found or not active. Other similar changes in this PR include null checks (e.g., FilterImportExportService line 270-272, DashboardItemServiceImpl line 67-69).
🔎 Apply this diff to add a null check:
- Case menuItemCase = workflowService.createCase( - petriNetService.getActiveVersionByIdentifier("preference_filter_item").getStringId(), - item.getEntryName() + "_" + menuIdentifier, - "", - ActorTransformer.toLoggedUser(userService.getSystem()) - ).getCase(); + PetriNet preferenceNet = petriNetService.getActiveVersionByIdentifier("preference_filter_item"); + if (preferenceNet == null) { + resultMessage.append("\n- Preference filter item process not found or not active\n"); + netCheck.set(false); + return ""; + } + Case menuItemCase = workflowService.createCase( + preferenceNet.getStringId(), + item.getEntryName() + "_" + menuIdentifier, + "", + ActorTransformer.toLoggedUser(userService.getSystem()) + ).getCase();application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java (1)
364-370: Remove duplicate Javadoc block.The Javadoc for
findAllByRoleIdis duplicated at lines 357-363 and 364-370.🔎 Apply this diff to remove the duplicate:
/** * Retrieves a paginated list of {@link PetriNet} objects associated with a specific role ID. * * @param roleId the ID of the role to filter the PetriNets by * @param pageable the pagination information * @return a {@link Page} of {@link PetriNet} objects matching the role ID */ - /** - * Retrieves a paginated list of {@link PetriNet} objects associated with a specific role ID. - * - * @param roleId the ID of the role to filter the PetriNets by - * @param pageable the pagination information - * @return a {@link Page} of {@link PetriNet} objects matching the role ID - */ Page<PetriNet> findAllByRoleId(String roleId, Pageable pageable);
♻️ Duplicate comments (3)
application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java (1)
52-52: Rename cache key toglobalFunctionsfor consistency.The cache key
namespaceFunctionsis inconsistent with the broader shift to global-scoped function caching in this PR. Other components referenceglobalFunctionsCacheandGLOBALscope semantics.Based on past review comments, this naming inconsistency has been flagged before.
🔎 Apply this diff to align naming:
- private String namespaceFunctions = "namespaceFunctions"; + private String globalFunctions = "globalFunctions";Additionally, update the getter method name and all references in line 68 and throughout the codebase.
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IFieldActionsCacheService.java (1)
30-30: Document the contract for the bulk caching method.The new
cacheAllPetriNetFunctions()method lacks documentation about:
- Which PetriNet versions are cached (active only, or all versions?)
- Whether stale entries are cleared before rebuilding
- Threading expectations (is this long-running? should it be called off request threads?)
Based on past review comments, these contract details were requested previously.
🔎 Apply this diff to add Javadoc:
+ /** + * Pre-populates the global function cache for all PetriNets. + * <p> + * Contract: + * <ul> + * <li>Only active versions of PetriNets are cached.</li> + * <li>Stale cache entries for non-existing PetriNets are overwritten.</li> + * <li>This operation may be long-running; invoke off request threads.</li> + * </ul> + */ void cacheAllPetriNetFunctions();application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (1)
160-163: Method namegetNamespaceFunctionCache()is inconsistent withglobalFunctionsCachefield.Per the previous review feedback requesting this rename, the method should be updated to
getGlobalFunctionCache()(orgetGlobalFunctionsCache()) to match the field and maintain API consistency. This requires updating the interface as well.🔎 Suggested fix:
@Override -public Map<String, List<CachedFunction>> getNamespaceFunctionCache() { +public Map<String, List<CachedFunction>> getGlobalFunctionCache() { return new HashMap<>(globalFunctionsCache); }
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (35)
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy(1 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/delegate/RoleActionDelegate.groovy(2 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovy(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/domain/repositories/PetriNetRepository.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java(6 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/startup/runner/DefaultFiltersRunner.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/startup/runner/ImpersonationRunner.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java(6 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FilterImportExportService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IFieldActionsCacheService.java(1 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovy(1 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/ImporterTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/PetriNetTest.groovy(1 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/CachePetriNetServiceTest.groovy(1 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy(5 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/workflow/NewInitTest.groovy(1 hunks)application-engine/src/test/resources/change_caseref_value_action_test.xml(2 hunks)application-engine/src/test/resources/petriNets/NAE-1290_Export_actions.xml(1 hunks)application-engine/src/test/resources/petriNets/process_version_1_0_0.xml(1 hunks)application-engine/src/test/resources/petriNets/process_version_2_0_0.xml(1 hunks)application-engine/src/test/resources/petriNets/process_version_3_0_0.xml(1 hunks)application-engine/src/test/resources/petriNets/process_version_4_0_0.xml(1 hunks)application-engine/src/test/resources/petriNets/process_version_5_0_0.xml(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.java(3 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.java(6 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/version/Version.java(2 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/PetriNet.java(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-12-12T13:40:00.786Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java:98-104
Timestamp: 2025-12-12T13:40:00.786Z
Learning: In application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java, the permission constraint logic in buildQuery() intentionally applies negative actor constraints globally: ((roleConstraint AND NOT negRoleConstraint) OR actorConstraint) AND NOT negActorConstraint. This means users in negativeViewActors are excluded even if they have positive role permissions, which is the intended authorization behavior.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.javaapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.javaapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy
📚 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/CaseSearchService.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.javaapplication-engine/src/test/resources/petriNets/process_version_3_0_0.xmlapplication-engine/src/test/resources/petriNets/process_version_1_0_0.xmlapplication-engine/src/test/resources/change_caseref_value_action_test.xmlapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.javaapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.javaapplication-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovyapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.javaapplication-engine/src/test/resources/petriNets/NAE-1290_Export_actions.xmlapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/FilterImportExportService.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/CachePetriNetServiceTest.groovyapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/domain/repositories/PetriNetRepository.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/ImporterTest.groovyapplication-engine/src/test/resources/petriNets/process_version_4_0_0.xmlapplication-engine/src/main/java/com/netgrif/application/engine/startup/runner/DefaultFiltersRunner.javaapplication-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/PetriNetTest.groovyapplication-engine/src/main/java/com/netgrif/application/engine/startup/runner/ImpersonationRunner.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/delegate/RoleActionDelegate.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/workflow/NewInitTest.groovyapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/PetriNet.javaapplication-engine/src/test/resources/petriNets/process_version_5_0_0.xmlnae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.java
📚 Learning: 2025-12-12T12:36:06.651Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/main/resources/petriNets/engine-processes/org_group.xml:36-39
Timestamp: 2025-12-12T12:36:06.651Z
Learning: In the org_group.xml file (application-engine/src/main/resources/petriNets/engine-processes/org_group.xml), variable names in action blocks like userField and user_selection should be kept as-is even when the field type changes from "user" to "actor".
Applied to files:
application-engine/src/test/resources/petriNets/process_version_3_0_0.xmlapplication-engine/src/test/resources/petriNets/process_version_1_0_0.xmlapplication-engine/src/test/resources/change_caseref_value_action_test.xmlapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.javaapplication-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovyapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.javaapplication-engine/src/test/resources/petriNets/NAE-1290_Export_actions.xmlapplication-engine/src/test/resources/petriNets/process_version_2_0_0.xmlapplication-engine/src/test/resources/petriNets/process_version_4_0_0.xmlapplication-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/delegate/RoleActionDelegate.groovyapplication-engine/src/test/resources/petriNets/process_version_5_0_0.xml
📚 Learning: 2025-06-23T13:30:13.096Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field `attributeId` will be renamed to `value` to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.javaapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.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/WorkflowService.javaapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.javaapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.javaapplication-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovyapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java
📚 Learning: 2025-09-29T10:31:31.469Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 362
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java:43-45
Timestamp: 2025-09-29T10:31:31.469Z
Learning: In the Netgrif application engine, when Authentication objects reach controller endpoints, auth.getPrincipal() always returns a LoggedUser instance and is never null, so defensive casting checks are not necessary in controller methods.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.javaapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.java
📚 Learning: 2025-12-12T12:40:25.863Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovy:18-20
Timestamp: 2025-12-12T12:40:25.863Z
Learning: In Groovy/Java, if a parameter or collection expects ActorFieldValue, you can pass UserFieldValue (a subclass) due to polymorphism. In tests, verify that ActorListFieldValue accepts ActorFieldValue instances and that passing UserFieldValue instances is valid without casts. This pattern applies to similar Groovy files across the repository.
Applied to files:
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/CachePetriNetServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/ImporterTest.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/PetriNetTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/delegate/RoleActionDelegate.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/workflow/NewInitTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovy
📚 Learning: 2025-09-05T10:21:54.893Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 350
File: application-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovy:135-135
Timestamp: 2025-09-05T10:21:54.893Z
Learning: In ExportServiceTest.groovy, writing to src/test/resources is intentional to simulate production behavior where the working tree is mutated during file exports. This mirrors how the system works in production.
Applied to files:
application-engine/src/test/resources/petriNets/NAE-1290_Export_actions.xmlapplication-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/workflow/NewInitTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovy
📚 Learning: 2025-12-12T12:37:58.852Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/test/groovy/com/netgrif/application/engine/auth/WorkflowAuthorizationServiceTest.groovy:4-4
Timestamp: 2025-12-12T12:37:58.852Z
Learning: In tests under application-engine/src/test, GroupService.getDefaultSystemGroup() lazily creates the default system group if it doesn't exist. Tests can rely on this behavior and omit explicit existence/setup checks for the default system group.
Applied to files:
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/CachePetriNetServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/ImporterTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/PetriNetTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/workflow/NewInitTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovy
📚 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/workflow/service/FieldActionsCacheService.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovyapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.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
🧬 Code graph analysis (1)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/ActorTransformer.java (1)
ActorTransformer(10-131)
⏰ 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). (2)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (46)
application-engine/src/test/resources/petriNets/process_version_5_0_0.xml (1)
1-9: LGTM!This test resource file is well-formed and follows the expected structure for Petri net process versioning tests. The semantic versioning and metadata fields align with the PR's shift toward "active version" semantics across the test suite.
application-engine/src/test/resources/petriNets/process_version_3_0_0.xml (1)
1-9: Verify that this test resource is properly referenced in test code.The XML file is syntactically correct and fills the version gap (3.0.0) in the process_version test resources. However, to confirm it supports the PR's shift to "active version" semantics, verify that it is imported and used in test cases.
#!/bin/bash # Description: Verify that process_version_3_0_0.xml is imported and used in tests. # Search for references to this test resource file rg -n 'process_version_3_0_0' --type=java --type=xml # Alternatively, search for tests that load process_version files to ensure # this new 3.0.0 resource is part of the test coverage rg -n 'process_version.*\.xml' --type=java -B2 -A2 | head -50application-engine/src/test/resources/petriNets/process_version_4_0_0.xml (1)
1-9: Verify how this test resource is loaded and used in the test suite.Multiple version test resources exist (process_version_1_0_0.xml through process_version_5_0_0.xml), suggesting an intentional versioning test structure. However, grep searches found no explicit file references in visible test code. Confirm:
- How these version resources are discovered/loaded (dynamic pattern, resource scanning, etc.)
- Whether tests validate version-specific behavior with these fixtures
- If the version sequence is intentionally testing version progression/handling
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/PetriNetTest.groovy (1)
139-139: LGTM! More specific error message.The updated assertion now expects a more descriptive error message that includes the process name and version number, improving clarity for debugging version conflicts.
application-engine/src/test/resources/petriNets/process_version_2_0_0.xml (1)
1-9: LGTM! Valid test resource for version testing.This minimal PetriFlow document is properly structured for testing version-active semantics and version conflict scenarios.
application-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovy (2)
252-256: Verify consistent behavior with upsertNet.This method mirrors the logic in
upsertNet(line 131). Both now rely ongetActiveVersionByIdentifier. Ensure consistency: if no active version exists, both should proceed to import rather than returning an inactive or null version.
131-135: Verify behavior when no active version exists.The change to
getActiveVersionByIdentifieris appropriate for idempotent import logic. The method correctly queries only for active versions (versionActive = true) and returns null when no active version exists, allowing bothupsertNetandimportProcessOnceto fall through to the create/import logic. This prevents erroneously reusing inactive versions even when multiple versions exist.nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.java (2)
27-27: LGTM! Adds version-active tracking to Elastic index.The new
versionActivefield enables filtering and querying by active version status in Elasticsearch, aligning with the PR's active-version semantics.
43-43: LGTM! Consistent propagation of version-active state.The
versionActivefield is correctly initialized from the sourcePetriNetin the constructor and updated in theupdatemethod, ensuring Elastic documents stay synchronized with the database state.Also applies to: 52-52
nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/version/Version.java (2)
55-69: LGTM! Good documentation addition.The Javadoc clearly documents the
compareTocontract, improving code maintainability.
71-87: LGTM! Useful semantic helpers.The
isHigherThanandisLowerThanmethods improve readability by providing semantic comparisons rather than numeric comparison results. Both correctly delegate tocompareTo.nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.java (4)
167-167: LGTM! Safe default initialization.Initializing new
PetriNetinstances as inactive (makeInactive()) is a safe default that prevents unintended activation until explicitly set.
177-177: LGTM! Correct copy constructor propagation.The
versionActivestate is correctly propagated in the copy constructor, ensuring cloned nets inherit the active/inactive status.
441-447: LGTM! Clear semantic helpers for activation state.The
makeActive()andmakeInactive()methods provide clear, intention-revealing operations for managing version activation state.
76-79: LGTM! Properly indexed version-active flag for compound queries.The
versionActivefield is correctly annotated with@Indexedto support the compound query patternfindByIdentifierAndVersionActive(String, boolean)used in PetriNetService. Lombok generates theisVersionActive()getter andsetVersionActive(boolean)setter.application-engine/src/test/groovy/com/netgrif/application/engine/workflow/NewInitTest.groovy (1)
48-48: Change is correct and test setup ensures active version exists.The
importPetriNetmethod automatically marks the imported process as active viamakeActive()before saving. The test imports "new_init_test" withVersionType.MAJOR, ensuring the process is persisted as the active version and available for immediate retrieval viagetActiveVersionByIdentifier.application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/CachePetriNetServiceTest.groovy (1)
70-77: LGTM! Test correctly validates active-version caching.The updates from
getPetriNetNewest/getNewestVersionByIdentifiertogetPetriNetActive/getActiveVersionByIdentifieralign with the PR's shift to active-version semantics and correctly validate the new caching behavior.application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.java (1)
68-72: LGTM! Proper null handling for active version lookup.The change to
getActiveVersionByIdentifierwith a null check and clear exception message follows defensive programming best practices. This ensures the dashboard management process is both present and active before attempting to create a case.application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/ImporterTest.groovy (1)
133-133: LGTM! Test correctly uses active-version lookup.The updates from
getNewestVersionByIdentifiertogetActiveVersionByIdentifieralign with the PR's active-version semantics and correctly validate the imported PetriNet retrieval behavior.Also applies to: 207-207
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (1)
2447-2453: LGTM! Proper null handling with clear error message.The change to
getActiveVersionByIdentifierwith a null check and informative exception message follows best practices. The error message includes the process identifier, which will aid debugging when a role collection fails.application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java (1)
283-283: Add null filtering to prevent NPE from missing active versions.The method
getActiveVersionByIdentifiercan return null when no active version exists for a process identifier. In the stream at line 283, null values will be added to thepetriNetslist and cause an NPE during subsequent operations. Filter nulls or add an explicit null check before collecting:petriNets = processes.stream() .map(process -> petriNetService.getActiveVersionByIdentifier(process)) .filter(Objects::nonNull) .collect(Collectors.toList());This aligns with null-handling patterns used throughout the codebase in WorkflowService, MenuImportExportService, and other services.
⛔ Skipped due to learnings
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.application-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovy (1)
131-131: The test setup properly ensures an active version exists. Thebefore()method imports the PetriNet viaimportHelper.createNet("NAE-1290_Export_actions.xml", VersionType.MAJOR), which callspetriNetService.importPetriNet(). This method automatically marks the imported net withversionActive=true. Since the XML file contains the identifierexport_test, the subsequent calls togetActiveVersionByIdentifier("export_test")will return a non-null PetriNet, and.stringIdwill execute without error.Likely an incorrect or invalid review comment.
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FilterImportExportService.java (1)
269-272: LGTM! Active version retrieval with appropriate error handling.The switch to
getActiveVersionByIdentifieraligns with the PR's objective to use active PetriNet versions. The null check with a clear error message ensures robustness.application-engine/src/main/java/com/netgrif/application/engine/startup/runner/ImpersonationRunner.java (1)
42-42: LGTM! Consistent version resolution change.The switch to
getActiveVersionByIdentifieris appropriate for startup process detection. The existing null check at line 43 ensures proper handling when the active version is not found.application-engine/src/test/resources/petriNets/NAE-1290_Export_actions.xml (1)
220-220: LGTM! Test resource updated to match active-version semantics.The change ensures the test exercises the same active-version lookup pattern used in production code.
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java (1)
287-316: LGTM! Consistent active-version adoption across overloads.All three
createCaseByIdentifieroverloads now consistently retrieve the active PetriNet version. The existing error handling with clear exception messages ensures robustness when the active version is not found.application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/delegate/RoleActionDelegate.groovy (2)
53-59: LGTM! Role assignment properly validates active process.The null check ensures the process exists and is active before attempting role assignment, with a clear error message.
80-86: LGTM! Role removal properly validates active process.Consistent with the assignRole method, this ensures the process is active before role removal.
application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.java (1)
66-70: LGTM! Dashboard item creation properly validates active process.The null check with clear error message ensures robustness when creating dashboard items. Using the active version's stringId for case creation is appropriate.
application-engine/src/main/java/com/netgrif/application/engine/startup/runner/DefaultFiltersRunner.java (1)
427-430: LGTM! Appropriate version resolution for filter creation.The switch to
getActiveVersionByIdentifierensures default filters are created from the active filter process. The existing graceful handling (returningOptional.empty()when null) is appropriate for startup scenarios.application-engine/src/main/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.java (1)
240-243: LGTM! Role validation properly handles missing nets.The change to
getActiveVersionByIdentifieris appropriate, with existing null handling that logs the missing net and sets the validation flag.application-engine/src/test/resources/change_caseref_value_action_test.xml (2)
32-32: LGTM!The migration from
getNewestVersionByIdentifiertogetActiveVersionByIdentifieraligns with the broader semantic shift across the codebase. This ensures tests use the active version rather than the latest version.
46-46: LGTM!Consistent with the other change in this file.
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java (1)
156-169: LGTM!The distinction between
getActiveVersionByIdentifier(returns the version withversionActive=true) andgetLatestVersionByIdentifier(returns the highest version by version number) is well-documented and provides clear semantics.application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy (2)
214-241: LGTM!The test correctly validates that deleting the active version causes the latest remaining version to become active, and deleting an inactive version does not affect the active version.
303-305: LGTM!The helper method centralizes import logic and ensures consistent
VersionType.MAJORusage across tests.application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (6)
140-154: LGTM!Cache eviction properly updated for the new
petriNetActiveandpetriNetLatestcaches, replacing the previouspetriNetNewestcache.
254-271: LGTM!The refactoring to
doSaveInternalis clean. The method properly initializes arcs, evicts cache, saves to repository, and indexes to Elasticsearch.
287-289: LGTM!The null guard for
identifierandversionprevents aNullPointerExceptionand aligns with the@Cacheable(unless = "#result == null")annotation.
310-331: LGTM!The implementations correctly distinguish between:
getActiveVersionByIdentifier: Uses the repository method to find byversionActive=truegetLatestVersionByIdentifier: Sorts by version components descending and returns the firstThe caching with
unless = "#result == null"is appropriate.
192-252: Race condition on concurrent imports of same identifier.The @transactional annotation ensures atomicity for the saves (lines 243-246) with proper MongoTransactionManager configuration. However, there's a race condition between checking
existingLatestProcess(line 209) and the save operations. Two concurrent imports of the same identifier could both pass the version checks and attempt to import, potentially resulting in inconsistent state. Consider adding distributed locking or a unique database constraint on (identifier, version) to prevent concurrent duplicate imports.
596-604: Review comment is incorrect.The
petriNetLatestcache is already properly evicted by identifier in theevictCachemethod (line 151). WhendeletePetriNetcallsevictCache(petriNet)at line 594, it evicts the cache entry usingnet.getIdentifier()as the key. Subsequently, whengetLatestVersionByIdentifier(petriNet.getIdentifier())is called at line 597, the cache has already been evicted with the same identifier, so it queries the database and retrieves the correct latest version. No stale cache issue exists in this flow.Likely an incorrect or invalid review comment.
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (4)
37-47: LGTM!The field rename to
globalFunctionsCacheand initialization usingproperties.getGlobalFunctionsCacheSize()correctly aligns with the PR objective of implementing cluster-scoped global function caching.
56-71: LGTM!The shift from
FunctionScope.NAMESPACEtoFunctionScope.GLOBALaligns with the PR objective. The cache key strategy usingpetriNet.getIdentifier()is appropriate for global-scoped caching, and the cleanup logic when no functions exist prevents stale empty entries.
74-77: LGTM!The method correctly clears the cache entry before reloading using the active version. The semantic shift from "newest" to "active" version aligns with the broader PR changes mentioned in the summary.
170-173: LGTM!The method is properly renamed to
clearGlobalFunctionCache()and correctly reinitializes the cache using the configured size.
| /** | ||
| * Finds a {@link PetriNet} entity by its identifier and versionActive attribute | ||
| * | ||
| * @param identifier the unique identifier of the PetriNet. | ||
| * @param versionActive if true, the active version will be found, otherwise the inactive version | ||
| * @return the {@link PetriNet} entity matching the given identifier and versionActive attribute, or {@code null} if none found. | ||
| */ | ||
| PetriNet findByIdentifierAndVersionActive(String identifier, boolean versionActive); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for how findByIdentifierAndVersionActive is used
echo "=== Usage of findByIdentifierAndVersionActive ==="
rg -n "findByIdentifierAndVersionActive" --type java -B 2 -A 5
echo -e "\n=== PetriNet entity definition ==="
fd "PetriNet.java" --type f | head -1 | xargs cat | head -200
echo -e "\n=== Database constraints or schema annotations ==="
fd "PetriNet.java" --type f | head -1 | xargs rg -n "@.*Unique|@.*Constraint|@.*Index" | head -20
echo -e "\n=== How versionActive is defined ==="
fd "PetriNet.java" --type f | head -1 | xargs rg -n "versionActive" -B 2 -A 2 | head -50Repository: netgrif/application-engine
Length of output: 3009
🏁 Script executed:
#!/bin/bash
# Find the actual PetriNet entity (not SearchPetriNet)
echo "=== Finding PetriNet entity ==="
fd "PetriNet\.java" -x sh -c 'wc -l {} && echo "File: {}"' | sort -rn | head -20
echo -e "\n=== PetriNet entity with @Entity annotation ==="
fd "\.java" -x grep -l "class PetriNet" | grep -v Search | head -5
echo -e "\n=== Searching for PetriNet class definition ==="
rg "class PetriNet" --type java -A 100 | head -150
echo -e "\n=== Looking for unique constraints ==="
rg "UniqueConstraint|@Index.*unique|@Unique" -B 2 -A 2 --type java | grep -A 5 -B 5 PetriNet | head -50Repository: netgrif/application-engine
Length of output: 24594
🏁 Script executed:
#!/bin/bash
# Read the actual PetriNet entity
echo "=== PetriNet Entity Definition ==="
head -300 "./nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.java"
echo -e "\n=== Searching for constraints and versionActive ==="
grep -n "@\|versionActive\|UniqueConstraint\|@Index" "./nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.java" | head -60Repository: netgrif/application-engine
Length of output: 11098
Implement the planned compound index for identifier and version uniqueness.
The TODO comment on line 34 indicates this is a known limitation: a compound unique constraint on (identifier, version) should be added. Currently, only versionActive is indexed individually. While the method is presently only invoked with versionActive=true (ensuring single active version per identifier), the lack of a compound constraint creates a gap if inactive version queries are introduced later. Implement the @CompoundIndex annotation as noted in the TODO to enforce this constraint at the database level.
| @Override | ||
| public void cacheAllPetriNetFunctions() { | ||
| Pageable pageable = PageRequest.of(0, 500); | ||
| Page<PetriNet> page = petriNetService.getAll(pageable); | ||
|
|
||
| while (!page.isEmpty()) { | ||
| for (PetriNet petriNet : page) { | ||
| try { | ||
| cachePetriNetFunctions(petriNet); | ||
| } catch (Exception e) { | ||
| log.warn("Failed to cache functions for PetriNet id={}", petriNet.getStringId(), e); | ||
| } | ||
| } | ||
|
|
||
| if (!page.hasNext()) { | ||
| break; | ||
| } | ||
| pageable = pageable.next(); | ||
| page = petriNetService.getAll(pageable); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find PetriNetService and look for getAll method implementation
find . -name "PetriNetService.java" -type f 2>/dev/null | head -5Repository: netgrif/application-engine
Length of output: 173
🏁 Script executed:
#!/bin/bash
# Find all Java files that might contain PetriNetService
fd "PetriNetService" --type f 2>/dev/null || \
rg -l "class PetriNetService" --type java 2>/dev/null || \
find . -name "*PetriNet*Service*.java" -type f 2>/dev/null | head -10Repository: netgrif/application-engine
Length of output: 1506
🏁 Script executed:
#!/bin/bash
# Search for getAll method that takes Pageable parameter and returns Page<PetriNet>
rg -A 15 "Page<PetriNet>\s+getAll\(Pageable" --type java 2>/dev/null | head -50Repository: netgrif/application-engine
Length of output: 4926
🏁 Script executed:
#!/bin/bash
# Also search for any version filtering or active/latest filtering in PetriNet queries
rg -n "active|latest|version" application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ --type java 2>/dev/null | head -30Repository: netgrif/application-engine
Length of output: 6375
🏁 Script executed:
#!/bin/bash
# Find PetriNetRepository and check its implementation
rg -n "class.*PetriNetRepository" --type java | head -5Repository: netgrif/application-engine
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Search for repository interface/implementation of PetriNet
find . -name "*PetriNetRepository*.java" -type f 2>/dev/nullRepository: netgrif/application-engine
Length of output: 300
🏁 Script executed:
#!/bin/bash
# Look for the repository file and check what findAll does or doesn't do with versions
rg -A 30 "interface.*PetriNetRepository|class.*PetriNetRepository" --type java | head -80Repository: netgrif/application-engine
Length of output: 6432
🏁 Script executed:
#!/bin/bash
# Search for getActive and getLatest methods in PetriNetService
rg -A 10 "getActive|getLatest" application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java --type java | head -50Repository: netgrif/application-engine
Length of output: 2484
🏁 Script executed:
#!/bin/bash
# Also check the FieldActionsCacheService to see how it's using cachePetriNetFunctions
rg -B 5 -A 20 "cachePetriNetFunctions|reloadCachedFunctions" application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java --type java | head -100Repository: netgrif/application-engine
Length of output: 2937
Ensure cacheAllPetriNetFunctions() caches only the active version of each PetriNet.
Since petriNetService.getAll() returns all PetriNet versions including historical ones, and cachePetriNetFunctions() caches by identifier only, multiple versions with the same identifier will overwrite each other. If an older version is processed last, it may be cached instead of the active version. Replace the direct call to cachePetriNetFunctions(petriNet) with reloadCachedFunctions(petriNet), which explicitly retrieves and caches only the active version.
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
around lines 101 to 121, the loop currently calls
cachePetriNetFunctions(petriNet) for every PetriNet version; replace that call
with reloadCachedFunctions(petriNet) so the service explicitly retrieves and
caches only the active version for each identifier, keeping the try/catch and
logging as-is and leaving paging logic unchanged.
|
|
||
| def createConfigCase(AbstractUser user, String impersonator, List<String> roles = null, List<String> auths = null) { | ||
| def caze = helper.createCase("config", petriNetService.getNewestVersionByIdentifier(ImpersonationRunner.IMPERSONATION_CONFIG_PETRI_NET_IDENTIFIER)) | ||
| def caze = helper.createCase("config", petriNetService.getActiveVersionByIdentifier(ImpersonationRunner.IMPERSONATION_CONFIG_PETRI_NET_IDENTIFIER)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the test file and examine the specific line and surrounding context
find . -name "ImpersonationServiceTest.groovy" -type f | head -5Repository: netgrif/application-engine
Length of output: 183
🏁 Script executed:
#!/bin/bash
# Find and examine the test file around line 314
find . -name "ImpersonationServiceTest.groovy" -type f -exec wc -l {} \;Repository: netgrif/application-engine
Length of output: 187
🏁 Script executed:
#!/bin/bash
# Get the specific section around line 314
find . -name "ImpersonationServiceTest.groovy" -type f -exec sed -n '305,330p' {} \;Repository: netgrif/application-engine
Length of output: 1905
🏁 Script executed:
#!/bin/bash
# Search for getActiveVersionByIdentifier method implementation
rg -n "def getActiveVersionByIdentifier|getActiveVersionByIdentifier\(" -B2 -A8Repository: netgrif/application-engine
Length of output: 50382
🏁 Script executed:
#!/bin/bash
# Search for petriNetService references to understand its behavior
rg -n "getActiveVersionByIdentifier" --type groovy --type java | head -20Repository: netgrif/application-engine
Length of output: 4301
Ensure null handling when impersonation config net is not active.
The method getActiveVersionByIdentifier can return null if no active version exists. While the codebase standard is to check for null after this call (as seen in FilterImportExportService, WorkflowService, RoleActionDelegate, etc.), the test passes the result directly to helper.createCase() without null checks. When this @disabled test is re-enabled, add null validation or ensure the impersonation config net is properly set up as active in the test setup.
🤖 Prompt for AI Agents
In
application-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovy
around line 314, the call to petriNetService.getActiveVersionByIdentifier(...)
may return null and is passed directly to helper.createCase(...). Add a null
check for the returned PetriNet (or active version) before calling
helper.createCase: if null, either set up/activate the impersonation config net
in the test setup or fail the test with a clear assertion message so you don't
pass null into createCase.
| @Test | ||
| void testVersionsOnImport() { | ||
| ImportPetriNetEventOutcome outcome = importProcess(VERSION_PROCESS_FILE_FORMAT.formatted("2"), superCreator.loggedSuper) | ||
| PetriNet petriNetV2 = outcome.getNet() | ||
| assert petriNetV2 != null | ||
| assert petriNetV2.versionActive | ||
| Version version = new Version() | ||
| version.setMajor(2) | ||
| assert petriNetV2.getVersion() == version | ||
| Thread.sleep(5000) | ||
| Optional<ElasticPetriNet> elasticPetriNetV2Optional = elasticPetriNetRepository.findById(petriNetV2.stringId) | ||
| assert elasticPetriNetV2Optional.isPresent() | ||
| assert elasticPetriNetV2Optional.get().isVersionActive() | ||
|
|
||
| outcome = importProcess(VERSION_PROCESS_FILE_FORMAT.formatted("4"), superCreator.loggedSuper) | ||
| PetriNet petriNetV4 = outcome.getNet() | ||
| assert petriNetV4 != null | ||
| assert !petriNetService.get(petriNetV2.getObjectId()).isVersionActive() | ||
| assert petriNetV4.isVersionActive() | ||
| version = new Version() | ||
| version.setMajor(4) | ||
| assert petriNetV4.getVersion() == version | ||
| Thread.sleep(5000) | ||
| elasticPetriNetV2Optional = elasticPetriNetRepository.findById(petriNetV2.stringId) | ||
| assert !elasticPetriNetV2Optional.get().isVersionActive() | ||
| Optional<ElasticPetriNet> elasticPetriNetV4Optional = elasticPetriNetRepository.findById(petriNetV4.stringId) | ||
| assert elasticPetriNetV4Optional.isPresent() | ||
| assert elasticPetriNetV4Optional.get().isVersionActive() | ||
|
|
||
| assertThrows(IllegalArgumentException.class, { | ||
| // cannot import with lower version number than the highest | ||
| importProcess(VERSION_PROCESS_FILE_FORMAT.formatted("1"), superCreator.loggedSuper) | ||
| }) | ||
| assertThrows(IllegalArgumentException.class, { | ||
| // cannot import already existing version | ||
| importProcess(VERSION_PROCESS_FILE_FORMAT.formatted("2"), superCreator.loggedSuper) | ||
| }) | ||
|
|
||
| petriNetV2.makeActive() | ||
| petriNetV2 = petriNetService.save(petriNetV2).get() | ||
| assert petriNetV2.versionActive | ||
| petriNetV4.makeInactive() | ||
| petriNetV4 = petriNetService.save(petriNetV4).get() | ||
| assert !petriNetV4.versionActive | ||
|
|
||
| outcome = importProcess(VERSION_PROCESS_FILE_FORMAT.formatted("5"), superCreator.loggedSuper) | ||
| PetriNet petriNetV5 = outcome.getNet() | ||
| assert petriNetV5 != null | ||
| assert !petriNetService.get(petriNetV2.getObjectId()).versionActive | ||
| assert !petriNetService.get(petriNetV4.getObjectId()).versionActive | ||
| assert petriNetV5.versionActive | ||
| version = new Version() | ||
| version.setMajor(5) | ||
| assert petriNetV5.getVersion() == version | ||
| Thread.sleep(5000) | ||
| elasticPetriNetV2Optional = elasticPetriNetRepository.findById(petriNetV2.stringId) | ||
| assert !elasticPetriNetV2Optional.get().versionActive | ||
| elasticPetriNetV4Optional = elasticPetriNetRepository.findById(petriNetV4.stringId) | ||
| assert !elasticPetriNetV4Optional.get().versionActive | ||
| Optional<ElasticPetriNet> elasticPetriNetV5Optional = elasticPetriNetRepository.findById(petriNetV5.stringId) | ||
| assert elasticPetriNetV5Optional.isPresent() | ||
| assert elasticPetriNetV5Optional.get().versionActive | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good test coverage for version semantics.
The test comprehensively validates:
- Version activation on import
- Deactivation of previous active versions
- Prevention of importing lower or duplicate versions
- Manual activation/deactivation behavior
However, Thread.sleep(5000) is brittle and slows down the test suite. Consider using a polling/await utility (e.g., Awaitility) for the Elasticsearch indexing assertions.
🔎 Example using Awaitility:
// Instead of:
Thread.sleep(5000)
Optional<ElasticPetriNet> elasticPetriNetV2Optional = elasticPetriNetRepository.findById(petriNetV2.stringId)
assert elasticPetriNetV2Optional.isPresent()
// Consider:
await().atMost(10, TimeUnit.SECONDS).untilAsserted {
Optional<ElasticPetriNet> elasticPetriNetV2Optional = elasticPetriNetRepository.findById(petriNetV2.stringId)
assert elasticPetriNetV2Optional.isPresent()
assert elasticPetriNetV2Optional.get().isVersionActive()
}| <?xml version="1.0" encoding="UTF-8"?> | ||
| <document xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
| xsi:noNamespaceSchemaLocation="https://petriflow.com/petriflow.schema.xsd"> | ||
| <id>process_version_test</id> | ||
| <version>1.0.0</version> | ||
| <initials>PVT</initials> | ||
| <title>Process Version 1.0.0 test</title> | ||
| <transitionRole>false</transitionRole> | ||
| </document> |
There was a problem hiding this comment.
Add at least one transition element to comply with the Petriflow schema.
This test resource file lacks required transition elements. Petriflow schema mandates at least one transition element in a valid process definition. Either add minimal transition definitions or clarify if this file is intentionally invalid for test purposes.
🤖 Prompt for AI Agents
In application-engine/src/test/resources/petriNets/process_version_1_0_0.xml
around lines 1 to 9, the XML document lacks any <transition> elements required
by the Petriflow schema; add at least one minimal <transition> element (with
required attributes/child elements per schema) inside the <document> to make the
test resource valid, or if the file is intentionally invalid for a negative
test, update the test name/comment to state that explicitly; ensure the added
transition uses realistic IDs and any required references (places/conditions) so
the file validates against the schema.
Updated method and variable names from "namespace functions" to "global functions" across relevant classes for better clarity and consistency. This includes changes in `FieldActionsCacheService`, `IFieldActionsCacheService`, and cache configuration properties.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IFieldActionsCacheService.java (1)
30-30: Document the method contract and clarify version semantics.This method lacks documentation defining its behavior. Based on the implementation, it pages through all PetriNets and caches their functions, but the contract is unclear on:
- Whether it processes only active versions or all historical versions
- Whether stale entries are cleared before repopulation
- That it's a long-running operation and should be called off request threads
Based on past review comments, this concern has already been raised but remains unaddressed.
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (1)
101-121: Ensure only active PetriNet versions are cached.This issue was previously flagged:
petriNetService.getAll()returns all PetriNet versions (active and inactive). SincecachePetriNetFunctions()caches by identifier only (line 67), multiple versions with the same identifier will overwrite each other. If an inactive version is processed after the active one, the inactive version will remain in the cache.Replace the call to
cachePetriNetFunctions(petriNet)at line 109 withreloadCachedFunctions(petriNet), which explicitly retrieves and caches only the active version viagetActiveVersionByIdentifier()(line 76).🔎 Apply this diff to cache only active versions:
for (PetriNet petriNet : page) { try { - cachePetriNetFunctions(petriNet); + reloadCachedFunctions(petriNet); } catch (Exception e) { log.warn("Failed to cache functions for PetriNet id={}", petriNet.getStringId(), e); }
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/FieldActionsRunner.groovy(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java(6 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IFieldActionsCacheService.java(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-12T12:36:06.651Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/main/resources/petriNets/engine-processes/org_group.xml:36-39
Timestamp: 2025-12-12T12:36:06.651Z
Learning: In the org_group.xml file (application-engine/src/main/resources/petriNets/engine-processes/org_group.xml), variable names in action blocks like userField and user_selection should be kept as-is even when the field type changes from "user" to "actor".
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IFieldActionsCacheService.java
📚 Learning: 2025-12-12T12:40:25.863Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovy:18-20
Timestamp: 2025-12-12T12:40:25.863Z
Learning: In Groovy/Java, if a parameter or collection expects ActorFieldValue, you can pass UserFieldValue (a subclass) due to polymorphism. In tests, verify that ActorListFieldValue accepts ActorFieldValue instances and that passing UserFieldValue instances is valid without casts. This pattern applies to similar Groovy files across the repository.
Applied to files:
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/FieldActionsRunner.groovy
📚 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/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). (3)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (10)
application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java (1)
67-68: LGTM!The method correctly aggregates all cache names including the newly added
petriNetActive,petriNetLatest, andglobalFunctionscaches.application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IFieldActionsCacheService.java (2)
22-22: LGTM! Method rename aligns with global cache refactoring.The rename from
getNamespaceFunctionCache()togetGlobalFunctionsCache()correctly reflects the shift from namespace-scoped to global-scoped function caching.
28-28: LGTM! Method rename aligns with global cache refactoring.The rename from
clearNamespaceFunctionCache()toclearGlobalFunctionCache()correctly reflects the global cache semantics.application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/FieldActionsRunner.groovy (1)
80-86: LGTM! Method call updated consistently with interface rename.The change from
getNamespaceFunctionCache()togetGlobalFunctionsCache()correctly follows the interface refactoring. The logic for building and assigning the namespace map toactionDelegate.metaClassremains unchanged.application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (6)
18-20: LGTM: Pagination imports added.The Spring Data pagination imports are necessary for the new bulk caching method.
56-71: LGTM: Cache operations updated to global scope.The method correctly uses
FunctionScope.GLOBALand operates onglobalFunctionsCache. The logic for caching and removal is consistent with the namespace-to-global refactoring.
74-77: LGTM: Reload logic correctly fetches active version.The method properly clears the cache entry and reloads using
getActiveVersionByIdentifier, ensuring only the active version is cached.
161-162: LGTM: Method renamed to match global cache convention.The method name
getGlobalFunctionsCache()is now consistent with theglobalFunctionsCachefield and the global scope refactoring.
171-172: LGTM: Clear method renamed and updated.The method name
clearGlobalFunctionCache()is consistent with the global scope refactoring, and it correctly reinitializesglobalFunctionsCache.
37-37: Verify the configuration property exists.The field rename from
namespaceFunctionsCachetoglobalFunctionsCacheis consistent with the namespace-to-global refactoring.RunnerConfigurationProperties.FieldRunnerPropertiesincludes theglobalFunctionsCacheSizefield with auto-generated getter via@Dataannotation, sogetGlobalFunctionsCacheSize()is available.Also applies to: 45-45
| * Default cache name for caching the active versions of Petri nets. | ||
| */ | ||
| private String petriNetNewest = "petriNetNewest"; | ||
| private String petriNetActive = "petriNetActive"; | ||
|
|
||
| /** | ||
| * Default cache name for caching the latest versions of Petri nets. | ||
| */ | ||
| private String petriNetLatest = "petriNetLatest"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.javaRepository: netgrif/application-engine
Length of output: 2263
🏁 Script executed:
rg "petriNetNewest" --type javaRepository: netgrif/application-engine
Length of output: 178
🏁 Script executed:
rg -i "migration" application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/ --type javaRepository: netgrif/application-engine
Length of output: 52
🏁 Script executed:
find . -name "*MIGRAT*" -o -name "*CHANGELOG*" -o -name "*RELEASE*" 2>/dev/null | head -20Repository: netgrif/application-engine
Length of output: 234
🏁 Script executed:
rg "petriNetNewest|cache|breaking" ./application-engine/CHANGELOG.md -A 2 -B 2Repository: netgrif/application-engine
Length of output: 1241
🏁 Script executed:
rg "@Deprecated" application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.javaRepository: netgrif/application-engine
Length of output: 52
🏁 Script executed:
git log --oneline --all --grep="petriNetNewest\|cache\|CacheConfiguration" 2>/dev/null | head -20Repository: netgrif/application-engine
Length of output: 127
🏁 Script executed:
git show fc637ee7e --statRepository: netgrif/application-engine
Length of output: 50383
🏁 Script executed:
rg "petriNetNewest" application-engine/src/main --type java -B 2 -A 2Repository: netgrif/application-engine
Length of output: 581
🏁 Script executed:
find . -name "*.properties" -o -name "*.yml" -o -name "*.yaml" | xargs grep -l "petriNetNewest" 2>/dev/null | head -10Repository: netgrif/application-engine
Length of output: 140
Document the breaking change for users overriding petriNetNewest.
The removal of petriNetNewest (replaced by petriNetActive and petriNetLatest) is a breaking change for users who have customized this property in their configuration files. Ensure that migration documentation is provided to guide users in updating their configurations, or consider adding a deprecated petriNetNewest property temporarily with a warning that maps to one of the new properties.
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java
around lines 30 to 37, the removal of petriNetNewest is a breaking change for
users who set that property; add a deprecated compatibility property named
petriNetNewest (annotated @Deprecated and exposed for deserialization) whose
setter maps the incoming value to the new petriNetActive and/or petriNetLatest
fields (or to the most appropriate single field per your migration decision) and
emits a clear warning log message when used, and also update the project
configuration docs and release notes to document the breaking change and show
example replacements for existing configs.
| /** | ||
| * Default cache name for caching namespace functions. | ||
| */ | ||
| private String globalFunctions = "globalFunctions"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify the comment to reflect the global scope.
The comment describes "caching namespace functions" while the field is named globalFunctions. Consider updating the comment to clarify that these are functions from Petri net namespaces cached in a global (cluster-wide) scope, e.g., "Default cache name for globally caching Petri net namespace functions."
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java
around lines 49 to 52, the Javadoc incorrectly or ambiguously refers to "caching
namespace functions" while the field name is globalFunctions; update the Javadoc
to explicitly state this is the default cache name for globally (cluster-wide)
caching Petri net namespace functions so the comment matches the field intent
and scope.
Removed duplicate JUnit dependencies from the pom.xml to clean up the build configuration. Corrected the scope comparison logic in FunctionFactory to use the proper `value()` method instead of `name()`.
coderabbitai
bot
added
the
breaking change
Description
This PR introduces a function that caches all petriNets namespace functions in cache.
Implements NAE-2212
Blocking PRs
This PR is blocked by NAE-2266 and on PF-82
How Has Been This Tested?
The functionality was tested in local cluster with admin and 1 worker.
Checklist:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.