[NAE-2400] Prevent Caching of Null Results in Process Cache#427
Merged
machacjozef merged 10 commits intorelease/7.0.0-rev10from Apr 10, 2026
Merged
[NAE-2400] Prevent Caching of Null Results in Process Cache#427machacjozef merged 10 commits intorelease/7.0.0-rev10from
machacjozef merged 10 commits intorelease/7.0.0-rev10from
Conversation
- Updated `@Cacheable` annotations in `PetriNetService` to avoid caching null results.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a targeted cache-eviction method Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Service as PetriNetService
participant CacheMgr as CacheManager
participant Caches as PetriNetCaches
Caller->>Service: evictCache(id, identifier, version)
Service->>CacheMgr: compute target keys (ObjectId(id), id, identifier, identifier+version, version)
CacheMgr->>Caches: evict("petriNetById", new ObjectId(id))
CacheMgr->>Caches: evict("petriNetById", id)
CacheMgr->>Caches: evict("petriNetByIdentifier", identifier)
CacheMgr->>Caches: evict("petriNetByIdentifier", identifier+version)
CacheMgr->>Caches: evict("petriNetCache", version)
CacheMgr->>Caches: evict("petriNetDefault", identifier)
CacheMgr->>Caches: evict("petriNetLatest", identifier)
Caches-->>CacheMgr: confirmations
CacheMgr-->>Service: eviction complete
Service-->>Caller: return
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
renczesstefan
previously approved these changes
Apr 2, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java`:
- Line 153: The `@Cacheable` unless="#result == null" is ineffective because the
two PetriNetService methods currently throw when a net is missing instead of
returning null; update the two cached methods in PetriNetService (the two
methods currently annotated with `@Cacheable`) to catch the "not found" exception
they throw (e.g., ResourceNotFoundException/NoSuchElementException used in this
service) and return null instead so the unless condition can take effect and
missing nets are not cached; ensure you only convert the missing-resource path
to return null and preserve throwing for other error cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e06acaa3-ca9a-4358-86d7-c60b722d3203
📒 Files selected for processing (1)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java
| */ | ||
| @Override | ||
| @Cacheable(value = "petriNetCache") | ||
| @Cacheable(value = "petriNetCache", unless = "#result == null") |
There was a problem hiding this comment.
unless = "#result == null" is a no-op here and does not implement the PR objective
At Line 153 and Line 304, both methods throw on not-found instead of returning null, so the unless condition is never hit and caching behavior is unchanged. This likely leaves NAE-2400 unresolved in this class.
Based on learnings: In this repository, petriNetService.get() does not return null when a net is missing; it fails via exception.
Also applies to: 304-304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java`
at line 153, The `@Cacheable` unless="#result == null" is ineffective because the
two PetriNetService methods currently throw when a net is missing instead of
returning null; update the two cached methods in PetriNetService (the two
methods currently annotated with `@Cacheable`) to catch the "not found" exception
they throw (e.g., ResourceNotFoundException/NoSuchElementException used in this
service) and return null instead so the unless condition can take effect and
missing nets are not cached; ensure you only convert the missing-resource path
to return null and preserve throwing for other error cases.
tuplle
previously approved these changes
Apr 8, 2026
Kovy95
previously approved these changes
Apr 9, 2026
- added new function for evicting petriNet cache by several string properties
renczesstefan
dismissed stale reviews from Kovy95, tuplle, and themself
via
3653f03
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (1)
162-167:⚠️ Potential issue | 🟠 Major
unless = "#result == null"is ineffective here and does not change caching behavior.Both methods throw on not-found instead of returning
null, so theunlesspredicate is never reached in the miss path. This is the same unresolved issue noted earlier.#!/bin/bash # Verify that the two methods with `unless = "#result == null"` throw on miss # and do not return null in the not-found path. rg -n -C3 \ 'public PetriNet get\(ObjectId petriNetId\)|public PetriNet getPetriNet\(String id\)|@Cacheable\(value = "petriNetCache", unless = "#result == null"\)|@Cacheable\(value = "petriNetById", unless = "#result == null"\)|throw new IllegalArgumentException|return null' \ application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javaBased on learnings: In this repository,
petriNetService.get()does not return null for missing nets and fails via exception.Also applies to: 313-318
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java` around lines 162 - 167, The `@Cacheable` annotations on PetriNetService.get(ObjectId petriNetId) and getPetriNet(String id) include unless = "#result == null" which is ineffective because both methods throw IllegalArgumentException on not-found instead of returning null; fix by removing the unless attribute from the `@Cacheable` declarations (i.e., change `@Cacheable`(value = "petriNetCache", unless = "#result == null") and `@Cacheable`(value = "petriNetById", unless = "#result == null") to simply `@Cacheable`(value = "petriNetCache") / `@Cacheable`(value = "petriNetById")), or alternatively alter the methods (get/getPetriNet) to return null on not-found if you intend to keep the unless check—choose one consistent approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java`:
- Around line 141-148: In PetriNetService.evctCache (method evictCache) the
eviction for cacheProperties.getPetriNetByIdentifier() uses the wrong key (id +
version); change it to evict(identifier + version) so it matches the lookup key
used when populating petriNetByIdentifier and prevents stale entries; update the
call
requireNonNull(cacheManager.getCache(cacheProperties.getPetriNetByIdentifier()),
cacheProperties.getPetriNetByIdentifier()).evict(...) accordingly.
---
Duplicate comments:
In
`@application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java`:
- Around line 162-167: The `@Cacheable` annotations on
PetriNetService.get(ObjectId petriNetId) and getPetriNet(String id) include
unless = "#result == null" which is ineffective because both methods throw
IllegalArgumentException on not-found instead of returning null; fix by removing
the unless attribute from the `@Cacheable` declarations (i.e., change
`@Cacheable`(value = "petriNetCache", unless = "#result == null") and
`@Cacheable`(value = "petriNetById", unless = "#result == null") to simply
`@Cacheable`(value = "petriNetCache") / `@Cacheable`(value = "petriNetById")), or
alternatively alter the methods (get/getPetriNet) to return null on not-found if
you intend to keep the unless check—choose one consistent approach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ac3074a9-e375-434e-a10a-938476961958
📒 Files selected for processing (2)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java
Replaced the incorrect use of 'id' with 'identifier' when evicting caches associated with PetriNet objects. This ensures consistency and proper functioning of the cache eviction process.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (1)
162-163:⚠️ Potential issue | 🟠 Major
unless = "#result == null"is ineffective on these two methods.At Line 162 and Line 313, the methods throw on not-found (Line 166 and Line 317) instead of returning
null, so theunlesscondition is never hit. This means caching behavior is unchanged by these annotations and the intended null-result handling is not implemented here.Based on learnings: In the netgrif/application-engine project, the
petriNetService.get()method (from IPetriNetService) never returns null and handles missing nets via exception paths.Also applies to: 313-314
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java` around lines 162 - 163, The `@Cacheable` annotation's unless = "#result == null" is ineffective because PetriNetService.get(...) throws when a net is missing rather than returning null; remove the unless attribute from the `@Cacheable` on the get(ObjectId petriNetId) method (and the other get(...) overload flagged) so caching behavior matches reality, or alternatively change the method to return null instead of throwing if you truly want the unless guard to apply — update the annotations on the get methods (e.g., get(ObjectId petriNetId) and the other get(...) overload) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java`:
- Around line 162-163: The `@Cacheable` annotation's unless = "#result == null" is
ineffective because PetriNetService.get(...) throws when a net is missing rather
than returning null; remove the unless attribute from the `@Cacheable` on the
get(ObjectId petriNetId) method (and the other get(...) overload flagged) so
caching behavior matches reality, or alternatively change the method to return
null instead of throwing if you truly want the unless guard to apply — update
the annotations on the get methods (e.g., get(ObjectId petriNetId) and the other
get(...) overload) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c2820688-14a6-4a8a-a9de-3d771408bdb0
📒 Files selected for processing (1)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java
- Update version to 7.0.0-RC10.2 in all POM files
coderabbitai
Bot
added
the
breaking change
Kovy95
previously approved these changes
Apr 9, 2026
Retoocs
previously approved these changes
Apr 9, 2026
tuplle
previously approved these changes
Apr 9, 2026
Included log statements to indicate the start and completion of caching functions for a given PetriNet. This improves traceability and aids in debugging by providing clear insights into the caching process.
renczesstefan
dismissed stale reviews from tuplle, Retoocs, and Kovy95
via
dfc012f
Retoocs
approved these changes
Apr 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
@Cacheableannotations inPetriNetServiceto avoid caching null results.Fixes NAE-2400
Dependencies
Third party dependencies
Blocking Pull requests
<Depends on #(PR id)>/
How Has Been This Tested?
Test Configuration
<Please describe configuration for tests to run if applicable, like program parameters, host OS, VM configuration etc.>
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Chores