[NAE-2239] Method calls not updated after Pageable was added #384
Conversation
- added pageable to method calls in ActionDelegate - unused imports removed
SamuelPalaj
requested review from
Retoocs,
machacjozef,
renczesstefan and
tuplle
WalkthroughAdds pageable support to action migration and per-net role operations, changes tests and service call return types to use Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
⏰ 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)
🔇 Additional comments (3)
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: 1
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/groovy/com/netgrif/application/engine/startup/ImportHelper.groovy (1)
169-175: Handle missing persisted roles (and avoid repeated linear searches) ingetProcessRolesUsing
processRoleService.findAllByNetStringId(net.stringId)is consistent with looking roles up by net string ID, but the current implementation will:
- Perform an O(n²) search (
roles.find) over the same list for eachnetRole.- Silently store
nullvalues in the map if a matchingProcessRoleis not found in the DB.Consider indexing the loaded roles by
stringIdonce and logging or skipping missing matches to surface inconsistencies:- Map<String, ProcessRole> getProcessRoles(PetriNet net) { - List<ProcessRole> roles = processRoleService.findAllByNetStringId(net.stringId) - Map<String, ProcessRole> map = [:] - net.roles.values().each { netRole -> - map[netRole.name.getDefaultValue()] = roles.find { it.stringId == netRole.stringId } - } - return map - } + Map<String, ProcessRole> getProcessRoles(PetriNet net) { + List<ProcessRole> roles = processRoleService.findAllByNetStringId(net.stringId) + Map<String, ProcessRole> rolesByStringId = roles.collectEntries { [it.stringId, it] } + Map<String, ProcessRole> map = [:] + net.roles.values().each { netRole -> + ProcessRole persisted = rolesByStringId[netRole.stringId] + if (persisted != null) { + map[netRole.name.getDefaultValue()] = persisted + } else { + log.warn("No persisted ProcessRole found for net [${net.stringId}] role [${netRole.stringId}]") + } + } + return map + }This keeps the method behaviorally similar while making it more robust and slightly more efficient.
application-engine/src/main/groovy/com/netgrif/application/engine/migration/ActionMigration.groovy (1)
32-40: Critical logic errors: inverted null check and missing.getContent()call.Two critical issues:
Line 33: The condition is inverted. It checks
if(newPetriNet.getNet() != null)but then throws an error saying the net "was not imported". This should be== null.Line 37:
getByIdentifiernow returnsPage<PetriNet>, so calling.stream()directly on it will fail. You need to call.getContent().stream()or just use.getContent()and then apply the stream operations.Apply this diff:
- if(newPetriNet.getNet() != null) { + if(newPetriNet.getNet() == null) { String message = "Petri net from file [" + petriNetPath + "] was not imported" log.error(message) throw new IllegalArgumentException(message) } else { - oldPetriNets = petriNetService.getByIdentifier(newPetriNet.getNet().importId, pageable) - .stream().filter({ net -> (net.version != newPetriNet.getNet().version)}) + oldPetriNets = petriNetService.getByIdentifier(newPetriNet.getNet().importId, pageable) + .getContent().stream().filter({ net -> (net.version != newPetriNet.getNet().version)}) .collect(Collectors.toList()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
application-engine/src/main/groovy/com/netgrif/application/engine/integration/modules/ModuleServiceInjector.groovy(0 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/migration/ActionMigration.groovy(3 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy(3 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovy(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowAuthorizationService.java(0 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/web/RestResponseExceptionHandler.java(0 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.java(0 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/PetriNetTest.groovy(1 hunks)
💤 Files with no reviewable changes (4)
- application-engine/src/main/groovy/com/netgrif/application/engine/integration/modules/ModuleServiceInjector.groovy
- application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowAuthorizationService.java
- application-engine/src/main/java/com/netgrif/application/engine/workflow/web/RestResponseExceptionHandler.java
- application-engine/src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.java
🧰 Additional context used
🧠 Learnings (2)
📚 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/groovy/com/netgrif/application/engine/migration/ActionMigration.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/PetriNetTest.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovy
📚 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/groovy/com/netgrif/application/engine/migration/ActionMigration.groovy
⏰ 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 (6)
application-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovy (1)
114-116: String-basedcreateNetoverload now delegates cleanly to theVersionTypeoverloadThe delegation to
createNet(fileName, VersionType, author)keeps all import logic in one place, and normalizingreleaseviatrim().toUpperCase()beforeVersionType.valueOfis reasonable given the existing API. No functional concerns from this change alone.application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/PetriNetTest.groovy (1)
125-127: LGTM! Test correctly updated to reflect API change.The test has been properly updated to use
Page<PetriNet>instead ofPage<PetriNetReference>, aligning with the changed return type ofIPetriNetService.getByIdentifier.application-engine/src/main/groovy/com/netgrif/application/engine/migration/ActionMigration.groovy (2)
12-12: LGTM! Import added for pagination support.
27-27: LGTM! Method signature extended with pagination support.The default value of
Pageable.unpaged()ensures backward compatibility for existing callers.application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (2)
1027-1031: LGTM! Pagination support added correctly.The method signature has been extended with a
Pageableparameter (defaulting toPageable.unpaged()), and.contentcorrectly extracts the list from thePage<PetriNet>result.
1048-1052: LGTM! Pagination support added correctly.The method signature has been extended with a
Pageableparameter (defaulting toPageable.unpaged()), and.contentcorrectly extracts the list from thePage<PetriNet>result.
...vy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy
Show resolved
Hide resolved
- empty optional check added
Description
Fixes JIRA-ISSUE
Test Configuration
<Please describe configuration for tests to run if applicable, like program parameters, host OS, VM configuration etc.>
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Chores