[NAE-2235] Improve Efficiency of Database Reads and Saves #383
Conversation
- update loggers in elastic package
- update loggers for MinIoStorageService and mail package
- update loggers in PetriNetService - fix cacheable self-invocation in PetriNetService - remove code that is commented out in PetriNetService - fix nullptr exception in PetriNetService.findByImportId - resolve some warnings in PetriNetService
- optimize ProcessRoleService.delete - add todos - remove redundant method overload in ProcessRoleService
- implement create case performance test
- update method calls after parameters change
- add performance test
- optimize createCase event
- optimize createCase event
- update WorkflowPerformanceTest
- add nets for WorkflowPerformanceTest
- update WorkflowService.deleteCase - optimize WorkflowService.deleteSubtreeRootedAt - optimize logging in TaskService
- change task service method parameters
- optimize TaskService.assignTask
- more optimization for TaskService.assignTask
- optimize TaskService.cancelTask and TaskService.cancelTasksWithoutReload
- optimize TaskService.finishTask
- optimize TaskService.delegateTask
- update TaskService.fillMissingAttributes for delegateTask - update WorkflowPerformanceTest
- small tweaks in DataService
- light optimization of DataService.getData
- light optimization of DataService.setData
- light optimization of DataService.getDataGroups
- light optimizations in workflow package - optimize authorization services
- begin implementation of MVC performance tests
- fix nullptr in TaskAuthorizationService - update WorkflowMvcPerformanceTest
- resolve some small warnings
- introduce optimizations and fixes in ActionDelegate
- small update in ValidationService
- resolve some warning
WalkthroughConsolidates many service overloads into builder-style parameter objects (ImportPetriNetParams, DeletePetriNetParams, CreateCaseParams, TaskParams, DelegateTaskParams, DeleteCaseParams), updates controllers/tests to use them, tightens logging to parameterized SLF4J, and applies assorted null-safety and minor API/visibility refinements across the engine. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Possibly related PRs
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: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
application-engine/src/main/groovy/com/netgrif/application/engine/validation/service/ValidationService.groovy (2)
39-64: Consider removing or documenting the commented code.This large block of commented-out code for additional field types reduces readability. If these are planned features, track them in issues and remove the comments. If they're not needed, delete them entirely.
28-28: Add null check to prevent potential NPE.There's no null check on
validation.getValidationRule()before callingtrim().split(). IfgetValidationRule()returns null, this will throw aNullPointerException.Apply this diff to add a null check:
- List<String> rules = validation.getValidationRule().trim().split(" ") + List<String> rules = validation.getValidationRule()?.trim()?.split(" ") + if (rules == null || rules.size() < 1) { + return + } - if (rules.size() >= 1) { AbstractFieldValidation instance = new AbstractFieldValidation() // ... rest of the code - }application-engine/src/main/java/com/netgrif/application/engine/mail/MailService.java (1)
102-109: Address the TODO: Remove commented-out code.The commented-out
testConnectionmethod and its TODO should be cleaned up to improve code maintainability.Do you want me to open an issue to track the removal of this dead code, or should it be removed in this PR?
application-engine/src/main/java/com/netgrif/application/engine/startup/runner/DefaultFiltersRunner.java (1)
432-498: Reuse the retrieved logged userYou already fetched the acting user just above. Reusing that instance (and the converted
LoggedUser) here removes two extra service lookups and keeps the executor identity consistent across create/assign/finish. Suggested change:- AbstractUser loggedUser = this.userService.getLoggedOrSystem(); + AbstractUser loggedUser = this.userService.getLoggedOrSystem(); + LoggedUser actingUser = ActorTransformer.toLoggedUser(loggedUser); ... - Case filterCase = this.workflowService.createCase(CreateCaseParams.with() + Case filterCase = this.workflowService.createCase(CreateCaseParams.with() .process(filterNet) .title(title) .color(null) - .author(ActorTransformer.toLoggedUser(loggedUser)) + .author(actingUser) .build()).getCase(); ... - this.taskService.assignTask(TaskParams.with() - .task(newFilterTask) - .user(this.userService.getLoggedOrSystem()) - .build()); + this.taskService.assignTask(TaskParams.with() + .task(newFilterTask) + .user(actingUser) + .build()); ... - this.taskService.finishTask(TaskParams.with() - .task(newFilterTask) - .user(this.userService.getLoggedOrSystem()) - .build()); + this.taskService.finishTask(TaskParams.with() + .task(newFilterTask) + .user(actingUser) + .build());application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskAuthorizationService.java (1)
54-117: Restore impersonation-aware permission checksBy replacing every
getSelfOrImpersonated()usage with baregetStringId()/isAdmin()we regress impersonation flows in two ways:
- When an admin impersonates another account, all permission maps remain keyed by the impersonated identity. Looking up
task.getUsers().get(user.getStringId())(and comparingtask.getUserId()touser.getStringId()) now misses that entry, so impersonated users lose their explicit permissions and can no longer finish/assign/cancel their own tasks.- The early admin shortcuts now consult
loggedUser.isAdmin(), which still reflects the real operator. An admin impersonating a non-admin suddenly bypasses all permission checks, meaning the impersonation sandbox is no longer enforced.Both are functional/security regressions in production impersonation scenarios. Please reinstate the impersonation-aware identity before proceeding—e.g.:
- if (!task.getUsers().containsKey(user.getStringId())) { + String effectiveUserId = user.getSelfOrImpersonated().getStringId(); + if (!task.getUsers().containsKey(effectiveUserId)) { return null; } - Map<String, Boolean> userPermissions = task.getUsers().get(user.getStringId()); + Map<String, Boolean> userPermissions = task.getUsers().get(effectiveUserId);and similarly
- if (loggedUser.isAdmin()) { + if (loggedUser.getSelfOrImpersonated().isAdmin()) { return true; }and
- return task.getUserId().equals(user.getStringId()) + String effectiveUserId = user.getSelfOrImpersonated().getStringId(); + return task.getUserId().equals(effectiveUserId)across the affected methods.
Also applies to: 168-209
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (107)
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(15 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovy(5 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/validation/service/ValidationService.groovy(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/auth/service/LoginAttemptService.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/auth/service/RegistrationService.java(5 hunks)application-engine/src/main/java/com/netgrif/application/engine/auth/web/UserControllerAdvice.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/security/ImpersonationRequestFilter.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java(6 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ReindexingTask.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/executors/Executor.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/files/minio/MinIoStorageService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/mail/MailAttemptService.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/mail/MailService.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java(4 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/params/DeletePetriNetParams.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/params/ImportPetriNetParams.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java(13 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java(9 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java(4 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/PetriNetController.java(4 hunks)application-engine/src/main/java/com/netgrif/application/engine/startup/ApplicationRunnerOrderResolver.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/startup/runner/AnonymousRoleRunner.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/startup/runner/DefaultDashboardRunner.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/startup/runner/DefaultFiltersRunner.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/startup/runner/DefaultRoleRunner.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/outcomes/ReloadTaskOutcome.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/params/CreateCaseParams.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/params/DelegateTaskParams.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/params/DeleteCaseParams.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/params/TaskParams.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/AbstractAuthorizationService.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/ConfigurableMenuService.java(0 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java(21 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/EventService.java(7 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FilterImportExportService.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/MongoSearchService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskAuthorizationService.java(5 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskSearchService.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java(20 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowAuthorizationService.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java(11 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IEventService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/ITaskService.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IWorkflowService.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java(4 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/web/PublicWorkflowController.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/web/RestResponseExceptionHandler.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.java(4 hunks)application-engine/src/main/resources/petriNets/engine-processes/import_filters.xml(2 hunks)application-engine/src/main/resources/petriNets/engine-processes/menu/menu_item.xml(3 hunks)application-engine/src/main/resources/petriNets/engine-processes/menu/tabbed_case_view_configuration.xml(2 hunks)application-engine/src/main/resources/petriNets/engine-processes/menu/tabbed_ticket_view_configuration.xml(2 hunks)application-engine/src/main/resources/petriNets/engine-processes/preference_filter_item.xml(1 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/action/AssignActionTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovy(3 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/auth/SecurityContextTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/auth/TaskAuthorizationServiceTest.groovy(9 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/auth/WorkflowAuthorizationServiceTest.groovy(6 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/elastic/DataSearchRequestTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ReindexTest.groovy(3 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovy(4 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/importer/PredefinedRolesPermissionsTest.groovy(3 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/importer/UserListTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/insurance/EncryptionTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/ipc/AssignCancelFinishWithCaseTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/ipc/CaseApiTest.groovy(3 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/ipc/TaskApiTest.groovy(7 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/pdf/service/PdfGeneratorTest.groovy(15 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/permissions/ElasticSearchViewPermissionTest.groovy(8 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/permissions/QueryDSLViewPermissionTest.groovy(10 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/ArcOrderTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/FunctionsTest.groovy(8 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/ImporterTest.groovy(7 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/PetriNetTest.groovy(5 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/CaseFieldTest.groovy(4 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/ChangeBehaviorTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/ChangeCasePropertyTest.groovy(4 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/ChangeFieldValueInitTest.groovy(3 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/ChoiceFieldTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/DynamicCaseNameTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/DynamicChoicesTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/DynamicDefaultValueTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/DynamicEnumerationTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/DynamicValidationPerformanceTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FileFieldTest.groovy(3 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FileListFieldTest.groovy(3 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/MapFieldTest.groovy(5 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/CachePetriNetServiceTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/web/PetriNetControllerTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/workflow/DataServiceTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/workflow/NewInitTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/workflow/SetDataOnButtonTest.groovy(3 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/workflow/TaskRefInitTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/workflow/TaskRefPropagationTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/workflow/UserRefsTest.groovy(2 hunks)
⛔ Files not processed due to max files limit (19)
- application-engine/src/test/groovy/com/netgrif/application/engine/workflow/WorkflowServiceTest.groovy
- application-engine/src/test/java/com/netgrif/application/engine/importer/ConstructorAndDestructorTest.java
- application-engine/src/test/java/com/netgrif/application/engine/importer/ImporterTest.java
- application-engine/src/test/java/com/netgrif/application/engine/petrinet/service/ProcessRoleServiceTest.java
- application-engine/src/test/java/com/netgrif/application/engine/workflow/WorkflowMvcPerformanceTest.java
- application-engine/src/test/java/com/netgrif/application/engine/workflow/WorkflowPerformanceTest.java
- application-engine/src/test/java/com/netgrif/application/engine/workflow/service/TaskServiceTest.java
- application-engine/src/test/java/com/netgrif/application/engine/workflow/web/VariableArcsTest.java
- application-engine/src/test/resources/change_caseref_value_action_test.xml
- application-engine/src/test/resources/petriNets/test_create_case_performance.xml
- application-engine/src/test/resources/petriNets/test_create_case_performance_with_action.xml
- application-engine/src/test/resources/petriNets/test_task_event.xml
- application-engine/src/test/resources/petriNets/test_task_event_with_action.xml
- nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/Case.java
- nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/ProcessResourceId.java
- nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/Task.java
- nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/eventoutcomes/caseoutcomes/ChangeCasePropertyOutcome.java
- nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/service/ProcessRoleService.java
- nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
💤 Files with no reviewable changes (1)
- application-engine/src/main/java/com/netgrif/application/engine/workflow/service/ConfigurableMenuService.java
🧰 Additional context used
🧠 Learnings (16)
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ReindexingTask.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovyapplication-engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java
📚 Learning: 2025-08-20T07:27:02.660Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:27:02.660Z
Learning: When reviewing ElasticTaskQueueManager changes, task.getTask().getId() returns the document identifier while task.getTaskId() returns the business task identifier. The queue operations should use consistent identifiers throughout the lifecycle (scheduling, processing, cleanup).
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/ITaskService.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovyapplication-engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java
📚 Learning: 2025-11-07T13:11:20.622Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java:145-149
Timestamp: 2025-11-07T13:11:20.622Z
Learning: In the netgrif/application-engine project, failed Elasticsearch bulk operations in ElasticQueueManager should not be retried or re-queued within the flush() method. The system relies on scheduled reindexing and manual reindex endpoints as fallback mechanisms for failed operations, making in-method retries unnecessary and potentially problematic (could cause deadlocks, capacity issues, or infinite loops with bad data).
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ReindexingTask.java
📚 Learning: 2025-07-31T23:40:46.499Z
Learnt from: tuplle
Repo: netgrif/application-engine PR: 334
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java:204-214
Timestamp: 2025-07-31T23:40:46.499Z
Learning: In the PetriNetService.importPetriNet method, existingNet.getVersion() cannot be null because all existing nets in the system were deployed through processes that ensure every net always has a version assigned.
Applied to files:
application-engine/src/test/groovy/com/netgrif/application/engine/ipc/TaskApiTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/auth/WorkflowAuthorizationServiceTest.groovyapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/params/ImportPetriNetParams.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/ChoiceFieldTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/ipc/CaseApiTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/MapFieldTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/workflow/SetDataOnButtonTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/CaseFieldTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/elastic/DataSearchRequestTest.groovyapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/DynamicEnumerationTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/importer/UserListTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/insurance/EncryptionTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/ArcOrderTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/FunctionsTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/DynamicDefaultValueTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/PetriNetTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/CachePetriNetServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/workflow/UserRefsTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/ImporterTest.groovyapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.javaapplication-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/auth/TaskAuthorizationServiceTest.groovyapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/web/PetriNetController.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/DynamicValidationPerformanceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/ChangeFieldValueInitTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/action/AssignActionTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FileListFieldTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/permissions/ElasticSearchViewPermissionTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/elastic/ReindexTest.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/migration/ActionMigration.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/workflow/DataServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/importer/PredefinedRolesPermissionsTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FileFieldTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/web/PetriNetControllerTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/pdf/service/PdfGeneratorTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/workflow/TaskRefInitTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/ChangeCasePropertyTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/workflow/NewInitTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/auth/SecurityContextTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/ipc/AssignCancelFinishWithCaseTest.groovyapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/DynamicChoicesTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/DynamicCaseNameTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/ChangeBehaviorTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/workflow/TaskRefPropagationTest.groovyapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java
📚 Learning: 2025-07-29T17:19:18.300Z
Learnt from: tuplle
Repo: netgrif/application-engine PR: 331
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java:45-46
Timestamp: 2025-07-29T17:19:18.300Z
Learning: In ElasticPetriNetService class, petriNetService is properly initialized using Lazy setter injection rather than constructor injection. This pattern with Lazy Autowired setter methods is commonly used in Spring to resolve circular dependencies and is a valid alternative to constructor injection.
Applied to files:
application-engine/src/test/groovy/com/netgrif/application/engine/ipc/TaskApiTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/auth/WorkflowAuthorizationServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/workflow/SetDataOnButtonTest.groovyapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowAuthorizationService.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/auth/TaskAuthorizationServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FileListFieldTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/permissions/ElasticSearchViewPermissionTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FileFieldTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/workflow/TaskRefInitTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/ChangeCasePropertyTest.groovyapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/ChangeBehaviorTest.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/groovy/com/netgrif/application/engine/auth/WorkflowAuthorizationServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/MapFieldTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/workflow/SetDataOnButtonTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/FunctionsTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/PetriNetTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/auth/TaskAuthorizationServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/elastic/ReindexTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/workflow/DataServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FileFieldTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/pdf/service/PdfGeneratorTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/workflow/NewInitTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/auth/SecurityContextTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/workflow/TaskRefPropagationTest.groovy
📚 Learning: 2025-09-05T10:17:28.577Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 350
File: application-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovy:82-82
Timestamp: 2025-09-05T10:17:28.577Z
Learning: In the application-engine test suite, testHelper.truncateDbs() follows a two-phase approach: first it clears the databases, then it executes superCreator to create the admin user. This means Optional.get() is safe when retrieving the admin user after truncateDbs() because the admin user is guaranteed to exist.
Applied to files:
application-engine/src/test/groovy/com/netgrif/application/engine/auth/WorkflowAuthorizationServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/elastic/DataSearchRequestTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/workflow/UserRefsTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/auth/TaskAuthorizationServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/permissions/ElasticSearchViewPermissionTest.groovy
📚 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/menu/services/DashboardItemServiceImpl.javaapplication-engine/src/main/resources/petriNets/engine-processes/menu/menu_item.xmlapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.javaapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovy
📚 Learning: 2025-10-20T11:46:37.958Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 373
File: application-engine/src/main/java/com/netgrif/application/engine/actions/ActionApiImpl.java:105-107
Timestamp: 2025-10-20T11:46:37.958Z
Learning: In ActionApiImpl, the processIdentifier parameter in searchCases() is intentionally unused because it's required by the ActionApi interface for plugin implementations. This implementation returns cases from all processes without filtering by processIdentifier.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.javaapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.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/auth/web/UserControllerAdvice.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/security/ImpersonationRequestFilter.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowAuthorizationService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/ITaskService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskAuthorizationService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java
📚 Learning: 2025-08-19T20:07:15.621Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/startup/runner/DefaultFiltersRunner.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ReindexingTask.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovyapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/ChangeCasePropertyTest.groovy
📚 Learning: 2025-07-31T08:05:21.587Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 329
File: nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/RealmServiceImpl.java:39-39
Timestamp: 2025-07-31T08:05:21.587Z
Learning: In the Netgrif Application Engine, the domain Realm class (com.netgrif.application.engine.objects.auth.domain.Realm) is abstract and cannot be instantiated. The concrete implementation is in the adapter package (com.netgrif.application.engine.adapter.spring.auth.domain.Realm) which extends the abstract domain class. Services must instantiate the concrete adapter class and cast it when saving to repositories that accept the abstract domain type.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowAuthorizationService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskAuthorizationService.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/action/AssignActionTest.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/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FileListFieldTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FileFieldTest.groovyapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java
📚 Learning: 2025-09-04T11:09:31.264Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In SuperCreatorRunner.java, when creating a user with PasswordCredential via setCredential("password", passwordCredential), there's no need to also call setPassword() as it would be redundant - the PasswordCredential already contains the password information needed by UserServiceImpl.createUser().
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/auth/service/RegistrationService.java
📚 Learning: 2025-08-19T20:13:40.087Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:13:40.087Z
Learning: In CaseField.java, fulltextValue is mapped as a keyword field type in Elasticsearch (for exact matches, filtering, aggregations), while the separate caseValue field serves different Elasticsearch query requirements, allowing the system to support multiple query patterns on the same data through different field mappings.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java
📚 Learning: 2025-08-19T20:07:43.748Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:07:43.748Z
Learning: In CaseField.java, the separate caseValue field (List<String>) is intentionally maintained alongside fulltextValue for specific Elasticsearch query requirements, rather than being derived on-the-fly from fulltextValue.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java
🧬 Code graph analysis (13)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FilterImportExportService.java (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/ActorTransformer.java (1)
ActorTransformer(10-131)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/params/ImportPetriNetParams.java (3)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/params/DeletePetriNetParams.java (1)
Data(12-25)application-engine/src/main/java/com/netgrif/application/engine/workflow/params/DeleteCaseParams.java (1)
Data(15-55)application-engine/src/main/java/com/netgrif/application/engine/workflow/params/TaskParams.java (1)
Data(17-70)
application-engine/src/main/java/com/netgrif/application/engine/workflow/params/CreateCaseParams.java (4)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/params/DeletePetriNetParams.java (1)
Data(12-25)application-engine/src/main/java/com/netgrif/application/engine/petrinet/params/ImportPetriNetParams.java (1)
Data(17-51)application-engine/src/main/java/com/netgrif/application/engine/workflow/params/DelegateTaskParams.java (1)
Data(17-45)application-engine/src/main/java/com/netgrif/application/engine/workflow/params/DeleteCaseParams.java (1)
Data(15-55)
application-engine/src/main/java/com/netgrif/application/engine/workflow/params/DeleteCaseParams.java (3)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/params/DeletePetriNetParams.java (1)
Data(12-25)application-engine/src/main/java/com/netgrif/application/engine/workflow/params/DelegateTaskParams.java (1)
Data(17-45)application-engine/src/main/java/com/netgrif/application/engine/workflow/params/TaskParams.java (1)
Data(17-70)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.java (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/ActorTransformer.java (1)
ActorTransformer(10-131)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/params/DeletePetriNetParams.java (1)
application-engine/src/main/java/com/netgrif/application/engine/workflow/params/DeleteCaseParams.java (1)
Data(15-55)
application-engine/src/main/java/com/netgrif/application/engine/startup/runner/DefaultFiltersRunner.java (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/ActorTransformer.java (1)
ActorTransformer(10-131)
application-engine/src/main/java/com/netgrif/application/engine/workflow/params/TaskParams.java (3)
application-engine/src/main/java/com/netgrif/application/engine/workflow/params/CreateCaseParams.java (1)
Data(19-70)application-engine/src/main/java/com/netgrif/application/engine/workflow/params/DelegateTaskParams.java (1)
Data(17-45)application-engine/src/main/java/com/netgrif/application/engine/workflow/params/DeleteCaseParams.java (1)
Data(15-55)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/PetriNetController.java (1)
application-engine/src/main/java/com/netgrif/application/engine/workflow/web/responsebodies/MessageResource.java (1)
MessageResource(10-31)
application-engine/src/main/java/com/netgrif/application/engine/workflow/params/DelegateTaskParams.java (2)
application-engine/src/main/java/com/netgrif/application/engine/workflow/params/DeleteCaseParams.java (1)
Data(15-55)application-engine/src/main/java/com/netgrif/application/engine/workflow/params/TaskParams.java (1)
Data(17-70)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/ActorTransformer.java (1)
ActorTransformer(10-131)
application-engine/src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.java (1)
application-engine/src/main/java/com/netgrif/application/engine/workflow/web/responsebodies/MessageResource.java (1)
MessageResource(10-31)
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). (6)
- 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
...ation-engine/src/main/groovy/com/netgrif/application/engine/migration/ActionMigration.groovy
Outdated
Show resolved
Hide resolved
...e/src/main/groovy/com/netgrif/application/engine/validation/service/ValidationService.groovy
Show resolved
Hide resolved
...on-engine/src/main/java/com/netgrif/application/engine/auth/service/RegistrationService.java
Outdated
Show resolved
Hide resolved
...on-engine/src/main/java/com/netgrif/application/engine/auth/service/RegistrationService.java
Outdated
Show resolved
Hide resolved
...n/java/com/netgrif/application/engine/configuration/security/ImpersonationRequestFilter.java
Show resolved
Hide resolved
...cation-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java
Show resolved
Hide resolved
...ion-engine/src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.java
Show resolved
Hide resolved
...n-engine/src/test/groovy/com/netgrif/application/engine/elastic/DataSearchRequestTest.groovy
Outdated
Show resolved
Hide resolved
...test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/DynamicChoicesTest.groovy
Outdated
Show resolved
Hide resolved
...on-engine/src/test/groovy/com/netgrif/application/engine/workflow/SetDataOnButtonTest.groovy
Outdated
Show resolved
Hide resolved
- fix possible leak with try-with-resources
- fix incorrect counter in a log message
- change logging level info
- add stack trace in an error message
- remove token from log messages to reduce security risk
- override builder methods in DelegateTaskParams
- fix stale useCase problem
- improve consistency in WorkflowController
- fix possible input stream leak in test files
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
application-engine/src/main/java/com/netgrif/application/engine/auth/service/RegistrationService.java (1)
101-101: Token exposure in logs remains a concern.The full token value is still logged, which poses a security risk if logs are accessed by unauthorized parties. While this is now at DEBUG level (typically disabled in production), consider logging only a partial hash or omitting the token entirely.
Note: This concern was previously raised and only the logging syntax was modernized in this change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
application-engine/src/main/groovy/com/netgrif/application/engine/migration/ActionMigration.groovy(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/auth/service/RegistrationService.java(6 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/mail/MailService.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/params/DelegateTaskParams.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java(20 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.java(4 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/elastic/DataSearchRequestTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/DynamicChoicesTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/workflow/SetDataOnButtonTest.groovy(3 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
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.624Z
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.
📚 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/workflow/SetDataOnButtonTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/DynamicChoicesTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/elastic/DataSearchRequestTest.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/groovy/com/netgrif/application/engine/workflow/SetDataOnButtonTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/elastic/DataSearchRequestTest.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/test/groovy/com/netgrif/application/engine/workflow/SetDataOnButtonTest.groovy
📚 Learning: 2025-08-20T07:27:02.660Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:27:02.660Z
Learning: When reviewing ElasticTaskQueueManager changes, task.getTask().getId() returns the document identifier while task.getTaskId() returns the business task identifier. The queue operations should use consistent identifiers throughout the lifecycle (scheduling, processing, cleanup).
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/params/DelegateTaskParams.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java
📚 Learning: 2025-11-07T13:11:20.622Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java:145-149
Timestamp: 2025-11-07T13:11:20.622Z
Learning: In the netgrif/application-engine project, failed Elasticsearch bulk operations in ElasticQueueManager should not be retried or re-queued within the flush() method. The system relies on scheduled reindexing and manual reindex endpoints as fallback mechanisms for failed operations, making in-method retries unnecessary and potentially problematic (could cause deadlocks, capacity issues, or infinite loops with bad data).
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java
📚 Learning: 2025-11-14T09:31:48.833Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 383
File: application-engine/src/main/java/com/netgrif/application/engine/configuration/security/ImpersonationRequestFilter.java:87-87
Timestamp: 2025-11-14T09:31:48.833Z
Learning: In the ImpersonationRequestFilter.java file within the security configuration, logging the full principal object (including its toString() representation) in the warn statement when a non-LoggedUser principal is encountered is acceptable, even though it may include PII. The team prefers the additional debugging context over restricting the log to just the class name.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/auth/service/RegistrationService.java
📚 Learning: 2025-09-04T11:09:31.264Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In SuperCreatorRunner.java, when creating a user with PasswordCredential via setCredential("password", passwordCredential), there's no need to also call setPassword() as it would be redundant - the PasswordCredential already contains the password information needed by UserServiceImpl.createUser().
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/auth/service/RegistrationService.java
📚 Learning: 2025-09-05T10:17:28.577Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 350
File: application-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovy:82-82
Timestamp: 2025-09-05T10:17:28.577Z
Learning: In the application-engine test suite, testHelper.truncateDbs() follows a two-phase approach: first it clears the databases, then it executes superCreator to create the admin user. This means Optional.get() is safe when retrieving the admin user after truncateDbs() because the admin user is guaranteed to exist.
Applied to files:
application-engine/src/test/groovy/com/netgrif/application/engine/elastic/DataSearchRequestTest.groovy
📚 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/workflow/service/TaskService.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/TaskService.java
🧬 Code graph analysis (2)
application-engine/src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.java (1)
application-engine/src/main/java/com/netgrif/application/engine/workflow/web/responsebodies/MessageResource.java (1)
MessageResource(10-31)
application-engine/src/main/java/com/netgrif/application/engine/workflow/params/DelegateTaskParams.java (2)
application-engine/src/main/java/com/netgrif/application/engine/workflow/params/TaskParams.java (1)
Data(17-70)application-engine/src/main/java/com/netgrif/application/engine/workflow/params/DeleteCaseParams.java (1)
Data(15-55)
⏰ 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). (13)
- GitHub Check: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (23)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java (1)
59-59: Previous feedback addressed - logging improvement looks good.The parameterized logging with the exception object as the third parameter is now correctly implemented. This ensures both the error message and full stack trace are captured, which improves debuggability while maintaining efficient logging performance.
application-engine/src/test/groovy/com/netgrif/application/engine/workflow/SetDataOnButtonTest.groovy (2)
72-78: Excellent fix for the resource leak!The use of
.withCloseable(Groovy's try-with-resources equivalent) ensures theFileInputStreamis properly closed after use, directly addressing the resource leak concern from the previous review. TheImportPetriNetParamsbuilder is correctly configured with all required parameters.
94-95: LGTM: Consistent with PR's parameter-object refactoring.The migration to
TaskParamsforassignTaskandfinishTaskoperations aligns with the PR's objective to consolidate method overloads into builder-style parameter objects. The usage is straightforward and correct.application-engine/src/test/groovy/com/netgrif/application/engine/elastic/DataSearchRequestTest.groovy (2)
5-5: LGTM: Import updates support the API refactoring.The import changes appropriately add the necessary types (Case, Task, ImportPetriNetParams) and consolidate dataset imports with a wildcard, which is acceptable in test files.
Also applies to: 12-15
96-103: Excellent: Resource leak fixed and builder pattern adopted correctly.The refactoring properly addresses the previous resource leak concern by wrapping the FileInputStream in Groovy's
withCloseable, ensuring the stream is always closed. The migration toImportPetriNetParamsbuilder pattern is implemented correctly and aligns with the project-wide API standardization described in the PR objectives.application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/DynamicChoicesTest.groovy (2)
5-5: Import ofImportPetriNetParamsmatches the new parameter-object API.The new import is consistent with the refactor towards builder-style params and keeps this test aligned with the updated
IPetriNetServiceAPI.
54-63: Resource handling and import parameterization look correct.Using
FileInputStream(...).withCloseable {}ensures the stream is closed, and theImportPetriNetParamsbuilder correctly mirrors the previous(InputStream, VersionType, User)call. The explicitassert optNet.getNet() != nullbefore usingoptNetis also sound.application-engine/src/main/java/com/netgrif/application/engine/auth/service/RegistrationService.java (2)
69-69: LGTM! Correct fix for total count tracking.The
totalResetcounter properly accumulates the count across all pagination iterations, and the final log message now reports the accurate total number of expired tokens reset.Also applies to: 82-82, 87-87
95-95: LGTM! Parameterized logging improves code quality.The migration to SLF4J parameterized logging is a best practice that improves performance by avoiding unnecessary string concatenation when the log level is disabled.
Also applies to: 133-133, 138-138, 164-164, 183-183, 187-187, 200-200
application-engine/src/main/groovy/com/netgrif/application/engine/migration/ActionMigration.groovy (2)
7-7: LGTM! Resource leak fixed and parameter builder adopted.The try-with-resources pattern ensures the InputStream is properly closed, addressing the resource leak concern from the previous review. The migration to
ImportPetriNetParamsaligns with the PR's goal of standardizing service calls with parameter-builder objects.Also applies to: 28-33
60-60: LGTM! Appropriate use of static modifiers.Converting
migrateDataSetActionsandmigrateDataRefActionsto static methods is appropriate since they operate only on their parameters without requiring instance state. This improves clarity and follows best practices.Also applies to: 68-68
application-engine/src/main/java/com/netgrif/application/engine/workflow/params/DelegateTaskParams.java (1)
46-73: LGTM! Custom builder methods properly synchronize entity and ID fields.The custom builder methods correctly mirror the pattern established in TaskParams and DeleteCaseParams, ensuring that setting an entity (task, newAssignee, delegator) automatically populates the corresponding ID field. The null-safety checks prevent NPEs when calling getStringId().
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java (7)
132-159: LGTM! TaskParams pattern cleanly consolidates method parameters.The refactor to TaskParams provides a cleaner API and the automatic attribute population via fillAndValidateAttributes (line 133) reduces boilerplate at call sites. The consistent extraction pattern and proper event/action flow are well-structured.
344-388: LGTM! fillAndValidateAttributes helpers intelligently populate missing attributes.Both overloads follow a sound sequence: resolve task first, then useCase (which depends on task), then users. The DelegateTaskParams version properly validates that newAssignee can be resolved (lines 379-386) and throws informative exceptions when requirements aren't met.
492-551: LGTM! reloadTasks refactor adds valuable lazy-save optimization.The new ReloadTaskOutcome return type provides callers with visibility into whether tasks were executed and whether the case was saved. The lazyCaseSave flag (line 492) allows callers to defer expensive save operations until necessary, and the logic correctly ensures the case is saved before executing auto-triggered tasks (lines 543-546).
627-655: LGTM! validateData improvements enhance error messaging and clarity.Separating field and dataField retrieval (lines 629-630) improves readability, and the enhanced error messages (lines 642, 644, 649, 651) that include field names when available will significantly aid debugging validation failures.
814-816: LGTM! Early returns avoid unnecessary repository operations.These guard clauses prevent saveAll/deleteAll calls on empty lists, eliminating unnecessary round-trips to MongoDB and Elasticsearch. Simple but effective optimization aligned with the PR's efficiency goals.
Also applies to: 950-952
833-853: LGTM! AtomicBoolean tracking minimizes unnecessary task saves.The isTaskModified flag (line 833) ensures the task is persisted only when user references actually change (lines 840, 843, 847), avoiding wasteful save operations when resolveUserRef doesn't modify state. Good optimization.
390-409: LGTM! returnTokens now returns Case for better composability.Changing the return type from void to Case (line 390) enables method chaining and functional composition, and the implementation correctly returns the saved case (line 408).
application-engine/src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.java (4)
89-95: LGTM! CreateCaseParams builder consolidates case creation parameters.The refactor to CreateCaseParams.with() provides a cleaner, more maintainable API and aligns with the parameter-object pattern adopted across the workflow services.
190-190: LGTM! URL decoding now uses the modern Charset overload consistently.Both calls now use
URLDecoder.decode(caseId, StandardCharsets.UTF_8)instead of the older.name()variant, eliminating the need to handle UnsupportedEncodingException and making the code cleaner.Also applies to: 220-220
225-227: LGTM! DeleteCaseParams builder consolidates deletion parameters.Consistent with the CreateCaseParams pattern and the broader parameter-object refactor across the codebase.
196-196: LGTM! Logging now uses SLF4J parameterized messages.The placeholder-based formatting (
log.error("... [{}] failed:", caseId, e)) is more efficient than string concatenation and follows SLF4J best practices.Also applies to: 232-232
...ation-engine/src/main/groovy/com/netgrif/application/engine/migration/ActionMigration.groovy
Outdated
Show resolved
Hide resolved
application-engine/src/main/java/com/netgrif/application/engine/mail/MailService.java
Show resolved
Hide resolved
application-engine/src/main/java/com/netgrif/application/engine/mail/MailService.java
Show resolved
Hide resolved
- fix ActionMigration.migrateActions to fail on every successful import
# Conflicts: # application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java # application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.java (1)
188-197: AligndeleteIndexlogging with the actual delete outcomeThe switch to parameterized logging is fine, but the message
"Index: {} has been deleted!"is logged before the delete call and regardless of its boolean result. Ifdelete()returnsfalseor fails silently, logs will still state that the index has been deleted, which is misleading operationally.Consider logging after the delete call and basing the log level/message on the returned value, e.g.:
- if (this.indexExists(indexName)) { - log.warn("Index: {} has been deleted!", indexName); - return elasticsearchTemplate.indexOps(IndexCoordinates.of(indexName)).delete(); - } else { - log.warn("Index: {} not found!", indexName); - } + if (this.indexExists(indexName)) { + boolean deleted = elasticsearchTemplate.indexOps(IndexCoordinates.of(indexName)).delete(); + if (deleted) { + log.info("Index: {} deleted", indexName); + } else { + log.warn("Index: {} could not be deleted", indexName); + } + return deleted; + } else { + log.warn("Index: {} not found", indexName); + }This keeps the new parameterized style while making logs accurately reflect the operation result.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy(15 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java(7 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.java(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
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.
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java
📚 Learning: 2025-11-04T11:47:03.897Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java:16-16
Timestamp: 2025-11-04T11:47:03.897Z
Learning: In the netgrif/application-engine project, ElasticsearchRunner is responsible for creating Elasticsearch indexes automatically based on configuration. Spring Data's automatic index creation via the Document annotation's createIndex attribute should be disabled (set to false) to prevent conflicts with this centralized index management approach.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.java
📚 Learning: 2025-08-19T20:07:15.621Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java
📚 Learning: 2025-08-19T20:13:40.087Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:13:40.087Z
Learning: In CaseField.java, fulltextValue is mapped as a keyword field type in Elasticsearch (for exact matches, filtering, aggregations), while the separate caseValue field serves different Elasticsearch query requirements, allowing the system to support multiple query patterns on the same data through different field mappings.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java
📚 Learning: 2025-08-19T20:07:43.748Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:07:43.748Z
Learning: In CaseField.java, the separate caseValue field (List<String>) is intentionally maintained alongside fulltextValue for specific Elasticsearch query requirements, rather than being derived on-the-fly from fulltextValue.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
- GitHub Check: task-list-completed
🔇 Additional comments (9)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java (1)
166-166: LGTM! Logging improvements follow SLF4J best practices.The conversion from string concatenation to parameterized logging is correctly implemented across all six statements. This change improves performance by deferring string construction until the log level is confirmed active, and enhances readability through the standard placeholder pattern.
Also applies to: 183-183, 197-197, 262-262, 276-276, 319-319
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (8)
52-52: LGTM! New imports support parameter-object refactoring.The new imports (ChangeCasePropertyOutcome, FinishTaskEventOutcome, MenuProcessRunner, and parameter builder classes) are consistent with the PR's goal of consolidating method overloads into builder-pattern APIs.
Also applies to: 57-57, 75-75, 77-79
635-642: LGTM! Task operation chain correctly uses builder pattern.The refactoring properly chains task outcomes (assign → setData → finish) using the new TaskParams builder, and each outcome uses
.getTask()from the previous step to maintain state continuity.
881-882: LGTM! Adds missing outcome tracking for case property changes.The addition of
ChangeCasePropertyOutcomeensures that case property modifications are properly tracked in the outcome chain, enabling frontend notifications of these changes.
987-1002: LGTM! Task operations consistently refactored with builder pattern.All task operations (assign, cancel, finish) are consistently refactored to use
TaskParamsbuilder, with proper outcome tracking via theaddTaskOutcomeAndReturnTaskhelper method.Also applies to: 1010-1023, 1036-1049
1182-1184: LGTM! Consistent routing through getData(Task) overload.The method now delegates to the
getData(Task, params)overload, ensuring consistent data retrieval logic and reducing code duplication.
1383-1387: LGTM! Improved null safety with Optional handling.The refactoring to use
Optional<AbstractUser>improves null safety across user lookup methods. The explicitisEmpty()checks with clear error messages enhance debugging and prevent NPEs.Also applies to: 1430-1433, 1451-1457
1776-1776: LGTM! Consistent use of DeleteCaseParams.Both
deleteFilteranddeleteMenuItemcorrectly use the newDeleteCaseParamsparameter object pattern, consistent with the PR's refactoring goals.Also applies to: 2014-2014
2803-2803: Fix incorrect syntax on line 2808:MenuItemConstants.FIELD_NODE_NAME.valuereferences a String constant.
FIELD_NODE_NAMEin MenuItemConstants is defined as a String constant ("nodeName"), not an enum member. Calling.valueon a String constant will fail. Remove the.valueaccessor to use the constant directly asMenuItemConstants.FIELD_NODE_NAME.The reference to
MenuProcessRunner.MENU_NET_IDENTIFIERon line 2803 is correct (defined as"menu_item"in MenuProcessRunner.java:27).⛔ Skipped due to learnings
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.
...vy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy
Show resolved
Hide resolved
...vy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy
Show resolved
Hide resolved
- fix ActionDelegate.createCase outcomes initialization
- fix ActionDelegate.deleteCase outcomes initialization
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (1)
2823-2832:PATH_SEPARATOR.valueingetCorrectedUriis likely incorrect and may return the wrong type.Everywhere else in this class
MenuItemConstants.PATH_SEPARATORis used directly as the path separator (e.g., comparisons andlastIndexOf(MenuItemConstants.PATH_SEPARATOR)), implying it is already aString. Here only,String rootPath = MenuItemConstants.PATH_SEPARATOR.valueaccesses a.valueproperty, which for aStringwould expose its internalchar[]. That meansrootPath/getCorrectedUricould now return achar[]instead of the intended root-pathString, leading to subtle type/behavior issues.Unless
PATH_SEPARATORhas been redefined to a custom object with aString value(which would make the other usages inconsistent), this line should use the constant directly.Concrete fix:
- String getCorrectedUri(String uncheckedPath) { - String rootPath = MenuItemConstants.PATH_SEPARATOR.value + String getCorrectedUri(String uncheckedPath) { + String rootPath = MenuItemConstants.PATH_SEPARATOR if (uncheckedPath == "") { return rootPath } int lastIdx = uncheckedPath.lastIndexOf(MenuItemConstants.PATH_SEPARATOR) if (lastIdx == -1) { return rootPath } return uncheckedPath }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy(15 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
- GitHub Check: task-list-completed
🔇 Additional comments (8)
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (8)
52-52: Imports for new outcomes and param objects are consistent.The added imports for
ChangeCasePropertyOutcome,DeleteCaseEventOutcome,FinishTaskEventOutcome,MenuProcessRunner, and theCreateCaseParams/DeleteCaseParams/TaskParamsclasses are all used below and align with the refactored APIs. No issues here.Also applies to: 54-54, 58-58, 76-76, 78-80
636-642: Outcome chaining inaddTaskOutcomeslooks correct.Assign → setData → finish is executed in order, and each of
AssignTaskEventOutcome,SetDataEventOutcome, andFinishTaskEventOutcomeis added tothis.outcomes, which matches the event-stream pattern used elsewhere in this class.
868-884:ChangeCasePropertyOutcomeintegration is appropriate.Emitting
new ChangeCasePropertyOutcome(useCase)after mutating case (and task) properties ensures these changes participate in the outcome chain without altering existing functional behavior.
952-990:createCase/deleteCaserefactor to param objects and outcomes is sound.Using
CreateCaseParams.with()/DeleteCaseParamscentralizes arguments, and bothcreateCaseoverloads plus the twodeleteCaseoverloads now capture and push the respectiveCreateCaseEventOutcome/DeleteCaseEventOutcomeintothis.outcomesbefore returning theCase. This aligns with the rest of the delegate’s outcome handling and should improve consistency for consumers.
992-1009: Task operations now consistently useTaskParamsbuilders.
assignTask,cancelTask, andfinishTaskuseTaskParams.with()for both task-id and task-based overloads, feeding everything throughaddTaskOutcomeAndReturnTaskso outcomes are always collected. This unifies behavior and keeps the DSL surface clean. Please just double-check that the newTaskParams-based service methods preserve the same defaults (e.g., effective user, params) as the previous overloads they replace.Also applies to: 1015-1030, 1041-1055
1147-1163: TODO about deprecatedsetDataWithPropagationnoted.No functional change here; the TODO is a reasonable reminder to clean up deprecated helpers in a later pass. Nothing to block this PR.
1188-1191: DelegatinggetData(String taskId, ...)through theTaskoverload is an improvement.Resolving the task once and reusing
getData(Task, ...)removes duplication and keeps all data-loading logic in one place.
1437-1440: Null guard indeleteUser(String email)avoids follow-up failures.Early-returning when
userService.findByEmailreturnsnullprevents passing a null user into downstream logic and replaces an implicit NPE with a clear error log.
...vy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy
Outdated
Show resolved
Hide resolved
...vy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy
Show resolved
Hide resolved
...vy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy
Show resolved
Hide resolved
...vy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy
Show resolved
Hide resolved
machacjozef
changed the base branch from
release/7.0.0-rev9
to
release/7.0.0-rev10
machacjozef
dismissed
coderabbitai[bot]’s stale review
The base branch was changed.
# Conflicts: # application-engine/src/main/groovy/com/netgrif/application/engine/migration/ActionMigration.groovy # application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy # application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java # application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java # application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java # application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/PetriNetController.java # application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowAuthorizationService.java
- post merge fixes
- post merge fixes
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 (1)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java (1)
206-213: Good simplification of stream creation.Using
entities.stream()is more idiomatic thanStreamSupport.stream(entities.spliterator(), false)sinceCollectionalready provides thestream()method.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
application-engine/src/main/groovy/com/netgrif/application/engine/migration/ActionMigration.groovy(2 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy(14 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovy(5 hunks)application-engine/src/main/java/com/netgrif/application/engine/actions/ActionApiImpl.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/files/minio/MinIoStorageService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/params/DeletePetriNetParams.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java(13 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java(9 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java(4 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/PetriNetController.java(4 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java(21 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/web/RestResponseExceptionHandler.java(1 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/PetriNetTest.groovy(5 hunks)nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java(2 hunks)
🧰 Additional context used
🧠 Learnings (20)
📓 Common learnings
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.
📚 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:
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
📚 Learning: 2025-09-04T11:09:31.264Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In SuperCreatorRunner.java, when creating a user with PasswordCredential via setCredential("password", passwordCredential), there's no need to also call setPassword() as it would be redundant - the PasswordCredential already contains the password information needed by UserServiceImpl.createUser(), and both setPassword() and setCredential() ultimately reference the same credential system.
Applied to files:
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
📚 Learning: 2025-09-04T11:09:31.264Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In SuperCreatorRunner.java, when creating a user with PasswordCredential via setCredential("password", passwordCredential), there's no need to also call setPassword() as it would be redundant - the PasswordCredential already contains the password information needed by UserServiceImpl.createUser().
Applied to files:
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
📚 Learning: 2025-07-31T23:40:46.499Z
Learnt from: tuplle
Repo: netgrif/application-engine PR: 334
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java:204-214
Timestamp: 2025-07-31T23:40:46.499Z
Learning: In the PetriNetService.importPetriNet method, existingNet.getVersion() cannot be null because all existing nets in the system were deployed through processes that ensure every net always has a version assigned.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/PetriNetController.javaapplication-engine/src/main/groovy/com/netgrif/application/engine/migration/ActionMigration.groovyapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/params/DeletePetriNetParams.javaapplication-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-12-19T09:06:32.102Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 398
File: application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java:57-58
Timestamp: 2025-12-19T09:06:32.102Z
Learning: In the file `application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java`, the `definition` attribute of the `Action` class (from `com.netgrif.application.engine.objects.petrinet.domain.dataset.logic.action.Action`) is never null, so null checks for `action.getDefinition()` are not needed.
Applied to files:
application-engine/src/main/groovy/com/netgrif/application/engine/migration/ActionMigration.groovyapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/params/DeletePetriNetParams.javaapplication-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovyapplication-engine/src/main/java/com/netgrif/application/engine/actions/ActionApiImpl.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/main/groovy/com/netgrif/application/engine/migration/ActionMigration.groovyapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javaapplication-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.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/main/groovy/com/netgrif/application/engine/migration/ActionMigration.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.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-07-29T17:19:18.300Z
Learnt from: tuplle
Repo: netgrif/application-engine PR: 331
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java:45-46
Timestamp: 2025-07-29T17:19:18.300Z
Learning: In ElasticPetriNetService class, petriNetService is properly initialized using Lazy setter injection rather than constructor injection. This pattern with Lazy Autowired setter methods is commonly used in Spring to resolve circular dependencies and is a valid alternative to constructor injection.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java
📚 Learning: 2025-09-29T10:31:57.325Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 362
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java:513-529
Timestamp: 2025-09-29T10:31:57.325Z
Learning: PetriNet.getStringId() returns a simple ObjectId string representation (_id.toString()), not a composite Netgrif ID format, so new ObjectId(petriNetId) works correctly when petriNetId comes from PetriNet.getStringId().
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java
📚 Learning: 2025-12-19T07:44:36.826Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 400
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java:336-343
Timestamp: 2025-12-19T07:44:36.826Z
Learning: In the netgrif/application-engine repository, when a Case is created, the `Case.populateDataSetBehaviorAndComponents()` method automatically copies all transition-level behavior and components to the case-level data fields, keyed by transition ID. This means methods like `isDataFieldEditable` only need to check case-level behavior (dataField.getBehavior().get(transId)), without fallback to transition.getDataSet(), since the behavior has already been copied during case initialization.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javaapplication-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovyapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.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/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.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/groovy/com/netgrif/application/engine/petrinet/domain/PetriNetTest.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/domain/PetriNetTest.groovy
📚 Learning: 2025-08-19T20:07:15.621Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java
📚 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/DataService.java
📚 Learning: 2025-08-13T09:38:25.819Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 327
File: src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.java:362-362
Timestamp: 2025-08-13T09:38:25.819Z
Learning: In ElasticIndexService pagination calculations, the formula `((count / batchSize) + 1)` is intentionally used instead of the mathematically precise `(count + batchSize - 1) / batchSize` to ensure at least 1 page is always returned, even when count=0. This design choice prioritizes simplicity and guaranteeing minimum processing over exact page counts.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java
📚 Learning: 2025-10-20T11:46:37.958Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 373
File: application-engine/src/main/java/com/netgrif/application/engine/actions/ActionApiImpl.java:105-107
Timestamp: 2025-10-20T11:46:37.958Z
Learning: In ActionApiImpl, the processIdentifier parameter in searchCases() is intentionally unused because it's required by the ActionApi interface for plugin implementations. This implementation returns cases from all processes without filtering by processIdentifier.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/actions/ActionApiImpl.java
📚 Learning: 2025-12-02T14:35:05.305Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 373
File: nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/actions/ActionApi.java:98-98
Timestamp: 2025-12-02T14:35:05.305Z
Learning: In ActionApi, the deleteCase method intentionally does not include an AuthPrincipalDto parameter because it does not require author information, unlike createCase which needs the author via authPrincipalDto.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/actions/ActionApiImpl.java
📚 Learning: 2025-12-04T08:21:16.144Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 373
File: nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/actions/ActionApi.java:113-121
Timestamp: 2025-12-04T08:21:16.144Z
Learning: In ActionApiImpl, the processIdentifier parameter in searchTasks() is intentionally unused because it's required by the ActionApi interface for plugin implementations. This implementation returns tasks from all processes without filtering by processIdentifier, matching the behavior of searchCases().
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/actions/ActionApiImpl.java
🧬 Code graph analysis (1)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/params/DeletePetriNetParams.java (1)
application-engine/src/main/java/com/netgrif/application/engine/workflow/params/DeleteCaseParams.java (1)
Data(15-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
🔇 Additional comments (34)
application-engine/src/main/java/com/netgrif/application/engine/files/minio/MinIoStorageService.java (1)
86-86: LGTM! Logging optimization applied correctly.The change to parameterized logging is a best practice for SLF4J that avoids unnecessary string concatenation and improves performance. The exception is properly included for stack trace logging.
application-engine/src/main/java/com/netgrif/application/engine/workflow/web/RestResponseExceptionHandler.java (1)
32-32: LGTM – Logging optimization correctly applied.The conversion to SLF4J parameterized logging improves performance by deferring string concatenation until the logging level is confirmed active. The placeholders and arguments are correctly matched.
application-engine/src/main/java/com/netgrif/application/engine/actions/ActionApiImpl.java (4)
27-29: LGTM: New parameter class imports.The addition of CreateCaseParams, DeleteCaseParams, and TaskParams imports supports the refactoring to builder-based parameter objects, aligning with the PR's objective to standardize parameter passing across service layers.
126-133: LGTM: Clean refactoring to CreateCaseParams builder.The migration from direct parameter passing to the CreateCaseParams builder improves maintainability and aligns with the PR's standardization objectives. All parameters are correctly mapped, and the public API remains unchanged.
138-141: LGTM: Consistent refactoring to DeleteCaseParams builder.The refactoring follows the same pattern as other methods in this file, maintaining consistency and improving parameter management.
162-189: LGTM: Consistent refactoring of task operations to TaskParams builder.All three task operations (assignTask, cancelTask, finishTask) have been consistently refactored to use the TaskParams builder pattern. The changes:
- Maintain existing method signatures and exception declarations
- Preserve the pre-resolution of task and user entities
- Follow a uniform implementation pattern across all task operations
This refactoring improves code maintainability and aligns well with the PR's objectives.
application-engine/src/main/groovy/com/netgrif/application/engine/migration/ActionMigration.groovy (1)
7-7: LGTM! All refactoring changes correctly implemented.The migration code has been successfully updated to:
- Use the
ImportPetriNetParamsbuilder pattern (lines 30-34)- Properly manage resources with try-with-resources (line 29)
- Correctly validate the import outcome with fixed null check logic (line 37)
- Appropriately refactor helper methods to
private static(lines 61, 69)Both previously identified issues (resource leak and inverted null check) have been addressed in the commits.
Also applies to: 29-57, 61-61, 69-69
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java (7)
102-108: Good defensive programming with the null check.The early return when
compositeIdis null prevents potential NPEs. The parameter rename fromroleIdtocompositeIdalso better reflects the actual semantics of the parameter.
123-126: LGTM: Clean delegation pattern.The method properly delegates role assignment logic to
assignRolesToActor, maintaining consistency with similar methods likeassignRolesToGroup.
410-416: Excellent logging optimization.The migration to parameterized SLF4J logging with placeholders is more efficient than string concatenation and prevents unnecessary string construction when the logging level is disabled. The multi-line formatting also improves readability.
431-432: LGTM: Parameterized logging improves performance.Using SLF4J placeholders instead of string concatenation is the recommended approach and aligns with the PR's optimization objectives.
452-453: LGTM: Consistent logging optimization.Properly using parameterized logging throughout the deletion flow.
478-479: LGTM: Parameterized logging applied correctly.The logging improvement maintains consistency with the broader optimization effort across the service layer.
504-506: LGTM: Logging optimization complete.Parameterized logging correctly applied with all necessary context fields.
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (7)
636-643: LGTM - Correct outcome chaining pattern.The refactoring properly chains task outcomes by using
setDataOutcome.getTask()for the finish call, ensuring the most up-to-date task state is used. All outcomes are correctly tracked.
882-884: LGTM - Outcome tracking added for case property changes.Adding
ChangeCasePropertyOutcomeensures frontend notifications work consistently with other operations.
952-978: LGTM - createCase methods properly refactored with builder pattern and outcome tracking.Both overloads correctly use
CreateCaseParamsbuilder, add outcomes tothis.outcomes, and return the created case. The previous concern about missing outcome tracking in the first overload has been addressed.
980-991: LGTM - deleteCase methods now properly track outcomes.Both overloads correctly capture
DeleteCaseEventOutcomeand add it tothis.outcomes, resolving the previously flagged inconsistency withcreateCase.
992-1056: LGTM - Task operation methods consistently refactored.All
assignTask,cancelTask, andfinishTaskoverloads properly useTaskParamsbuilder pattern with appropriate outcome tracking viaaddTaskOutcomeAndReturnTask.
1188-1191: LGTM - getData simplified correctly.The simplified call
getData(task, params)internally handles fetching the case, avoiding redundant code.
1459-1467: LGTM - findUserByEmail properly uses Optional API.The refactoring to use
OptionalwithisEmpty()check follows modern Java idioms and maintains the same error logging behavior.nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java (1)
150-150: LGTM! Clean refactoring and logging improvements.The changes appropriately remove an unnecessary cast and improve logging clarity by explicitly mentioning the authentication method in third-party user creation flows, aligning with the broader parameterized logging improvements across the PR.
Also applies to: 217-217
application-engine/src/main/java/com/netgrif/application/engine/petrinet/params/DeletePetriNetParams.java (1)
1-22: LGTM! Appropriate parameter object design.The simpler design (compared to DeleteCaseParams) is well-justified since PetriNet deletions don't trigger events that would require an extensible params map. The builder pattern with required parameters (petriNetId, loggedUser) provides a clean API for deletion operations.
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/PetriNetTest.groovy (1)
71-75: LGTM! Consistent test migration to parameter objects.All test methods have been correctly updated to use the new ImportPetriNetParams builder pattern, maintaining consistent parameter passing (xmlFile, releaseType, author) across the test suite. The refactoring improves test readability and aligns with the project-wide parameter object adoption.
Also applies to: 96-100, 103-107, 110-114, 125-129, 133-137, 141-145, 148-152, 155-159, 169-173, 178-182
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java (1)
70-83: LGTM! Well-documented API consolidation.The interface correctly consolidates import and delete operations to use parameter objects (ImportPetriNetParams, DeletePetriNetParams), with clear Javadoc specifying required parameters. This improves API clarity and extensibility while maintaining backward compatibility through the builder pattern.
Also applies to: 279-298
application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/PetriNetController.java (1)
114-118: LGTM! Controller properly adopts parameter objects.The controller correctly constructs ImportPetriNetParams and DeletePetriNetParams using the builder pattern, providing all required parameters before delegating to the service layer. The logging improvements use parameterized SLF4J format, aligning with best practices.
Also applies to: 193-193, 235-235, 241-249
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (3)
170-214: LGTM! Clean parameter object integration in import flow.The importPetriNet method correctly extracts all parameters from ImportPetriNetParams, maintains version handling logic, and properly propagates the params map to pre/post upload actions. The use of
self.getNewestVersionByIdentifierensures @Cacheable annotations are respected.
518-549: LGTM! Consistent delete flow with parameter objects.The deletion methods appropriately use DeletePetriNetParams, validate inputs via fillAndValidateAttributes, and maintain the existing force-delete logic. The logging clearly tracks the deletion lifecycle with user context.
575-599: LGTM! Good defensive programming with validation helpers.The validation methods provide clear null-safety checks with informative error messages. The auto-fill of loggedUser with the system user (lines 596-598) is a sensible default for background operations where no user context exists, preventing unnecessary failures while maintaining audit trails.
application-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovy (4)
9-9: LGTM! Builder parameter imports support refactoring.The new imports for
ImportPetriNetParams,CreateCaseParams, andTaskParamsare necessary for the builder-based parameter passing pattern introduced in this PR.Also applies to: 13-14
123-135: Excellent null-safety improvement!The refactoring to use
ImportPetriNetParamsbuilder pattern improves readability, and the explicit null check (lines 129-132) properly returnsOptional.empty()when import produces no PetriNet. This prevents potential NPEs downstream compared to the previous behavior.
195-201: LGTM! Consistent builder pattern adoption.The refactoring to use
CreateCaseParamsbuilder is clean and consistent with the broader PR refactoring strategy. All parameters are properly mapped.
218-221: LGTM! Task operations consistently refactored.All three task operation methods (
assignTask,finishTask,cancelTask) have been consistently refactored to use theTaskParamsbuilder pattern. The parameter mapping is correct and the pattern is uniform across all operations.Also applies to: 229-232, 240-243
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java (1)
123-123: Excellent logging and null-safety improvements.The migration to parameterized SLF4J logging, use of
.formatted()for string formatting, and added null checks (e.g., lines 233-235, 380-382, 403) align well with the PR objectives to improve code quality and reduce potential bugs. These changes enhance both maintainability and runtime safety.Also applies to: 224-224, 321-321, 332-332, 380-382, 403-403, 445-445, 496-497, 511-511, 624-627, 659-661, 698-700, 736-737, 785-786, 834-834, 840-841, 1149-1150
|
|
||
|
|
||
| ProcessRole getProcessRoleByImportId(PetriNet net, String roleId) { | ||
| static ProcessRole getProcessRoleByImportId(PetriNet net, String roleId) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all callers of getProcessRoleByImportId and getProcessRolesByImportId
# to verify static access patterns
echo "=== Searching for getProcessRoleByImportId calls ==="
rg -n "getProcessRoleByImportId" --type groovy --type java -C2
echo ""
echo "=== Searching for getProcessRolesByImportId calls ==="
rg -n "getProcessRolesByImportId" --type groovy --type java -C2
echo ""
echo "=== Checking ImportHelper.groovy for method definitions ==="
cat -n application-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovy | sed -n '160,175p'Repository: netgrif/application-engine
Length of output: 3901
External caller uses instance-style access for static method.
The external caller in InsuranceTest.groovy (line 142) calls getProcessRolesByImportId using instance-style access (importHelper.getProcessRolesByImportId(...)). Update to use static access pattern: ImportHelper.getProcessRolesByImportId(...).
🤖 Prompt for AI Agents
In
application-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovy
around line 163, the static method getProcessRoleByImportId is being called from
tests/other code using an instance
(importHelper.getProcessRolesByImportId(...)); change those call sites (e.g.,
InsuranceTest.groovy line 142) to use static access
ImportHelper.getProcessRolesByImportId(...) instead of using an ImportHelper
instance, ensuring any setup that created an instance is removed or replaced and
imports/reference adjusted accordingly.
| if (isForbidden(fieldId, transition, useCase.getDataField(fieldId))) | ||
| return; | ||
| Field field = useCase.getPetriNet().getField(fieldId).get(); | ||
| Field<?> field = useCase.getPetriNet().getField(fieldId).get(); |
There was a problem hiding this comment.
Unsafe Optional.get() call risks NoSuchElementException.
Calling .get() on the Optional returned by useCase.getPetriNet().getField(fieldId) without first checking isPresent() will throw NoSuchElementException if the field doesn't exist. While the fieldId comes from transition.getDataSet().keySet(), data integrity issues could still cause a crash.
🔎 Safer alternative using orElseThrow
- Field<?> field = useCase.getPetriNet().getField(fieldId).get();
+ Field<?> field = useCase.getPetriNet().getField(fieldId)
+ .orElseThrow(() -> new IllegalStateException(
+ "Field %s not found in Petri net %s".formatted(fieldId, useCase.getPetriNet().getStringId())));🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java
around line 136, the code calls useCase.getPetriNet().getField(fieldId).get()
which can throw NoSuchElementException; replace the unsafe Optional.get() with a
safe alternative — either call .orElseThrow(...) to throw a clear, specific
exception (including fieldId and context) when the field is missing, or first
check .isPresent() and handle the missing-field case gracefully (log and skip,
or return/throw a domain-specific exception). Ensure the new behavior provides
meaningful error details and avoids an unhandled NoSuchElementException.
| return; | ||
| } | ||
|
|
||
| Field<?> field = useCase.getPetriNet().getField(fieldId).get(); |
There was a problem hiding this comment.
Unsafe Optional.get() call risks NoSuchElementException.
Similar to line 136, calling .get() without checking the Optional could throw NoSuchElementException if the field doesn't exist in the Petri net. Even though line 233 checks if dataField exists, this doesn't guarantee the Petri net field reference exists.
🔎 Safer alternative using orElseThrow
- Field<?> field = useCase.getPetriNet().getField(fieldId).get();
+ Field<?> field = useCase.getPetriNet().getField(fieldId)
+ .orElseThrow(() -> new IllegalStateException(
+ "Field %s not found in Petri net %s".formatted(fieldId, useCase.getPetriNet().getStringId())));📝 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.
| Field<?> field = useCase.getPetriNet().getField(fieldId).get(); | |
| Field<?> field = useCase.getPetriNet().getField(fieldId) | |
| .orElseThrow(() -> new IllegalStateException( | |
| "Field %s not found in Petri net %s".formatted(fieldId, useCase.getPetriNet().getStringId()))); |
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java
around line 237, the code calls useCase.getPetriNet().getField(fieldId).get()
which unsafely invokes Optional.get() and can throw NoSuchElementException if
the Petri net does not contain that field; replace this with a safe check or
orElseThrow with a clear exception: either obtain the Optional first and handle
absence (e.g., throw a meaningful IllegalStateException/NoSuchElementException
with context) or call .orElseThrow(() -> new <appropriate exception>("...")) so
the absence is handled explicitly and logged. Ensure the new exception message
includes useCase id and fieldId for debugging.
| Field<?> field = net.getDataSet().get(dataFieldId); | ||
| if (field.getType() == FieldType.TASK_REF | ||
| && shouldResolveTaskRefData(field, transition.getDataSet().get(field.getStringId()))) { |
There was a problem hiding this comment.
Potential NullPointerException when accessing field.
Line 348 retrieves field from net.getDataSet().get(dataFieldId), which can return null if the field doesn't exist in the dataset. Line 349 then calls field.getType() without checking for null, which would cause a NullPointerException.
🔎 Add null check before accessing field
resources.add(resource);
Field<?> field = net.getDataSet().get(dataFieldId);
- if (field.getType() == FieldType.TASK_REF
+ if (field != null && field.getType() == FieldType.TASK_REF
&& shouldResolveTaskRefData(field, transition.getDataSet().get(field.getStringId()))) {
resultDataGroups.addAll(collectTaskRefDataGroups((TaskField) resource, locale, collectedTaskIds, level));
}📝 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.
| Field<?> field = net.getDataSet().get(dataFieldId); | |
| if (field.getType() == FieldType.TASK_REF | |
| && shouldResolveTaskRefData(field, transition.getDataSet().get(field.getStringId()))) { | |
| Field<?> field = net.getDataSet().get(dataFieldId); | |
| if (field != null && field.getType() == FieldType.TASK_REF | |
| && shouldResolveTaskRefData(field, transition.getDataSet().get(field.getStringId()))) { |
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java
around lines 348 to 350, the code retrieves Field<?> field =
net.getDataSet().get(dataFieldId) and immediately calls field.getType(), which
can throw a NullPointerException if the dataset does not contain that id; add a
null check (e.g., if (field == null) continue/skip or return as appropriate)
before accessing field.getType(), and optionally log or trace the missing field
for diagnostics so the subsequent TASK_REF/type logic only runs when field is
non-null.
| EventPhase.PRE, useCase, referencingTask, params)); | ||
| outcomes.addAll(resolveDataEvents(useCase.getPetriNet().getField(fieldId).get(), DataEventType.SET, | ||
| EventPhase.POST, useCase, referencingTask, params)); | ||
| Field<?> field = useCase.getPetriNet().getField(fieldId).get(); |
There was a problem hiding this comment.
Unsafe Optional.get() call risks NoSuchElementException.
Calling .get() on the Optional without checking presence could throw NoSuchElementException if the field doesn't exist in the Petri net.
🔎 Safer alternative using orElseThrow
- Field<?> field = useCase.getPetriNet().getField(fieldId).get();
+ Field<?> field = useCase.getPetriNet().getField(fieldId)
+ .orElseThrow(() -> new IllegalStateException(
+ "Field %s not found in Petri net".formatted(fieldId)));📝 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.
| Field<?> field = useCase.getPetriNet().getField(fieldId).get(); | |
| Field<?> field = useCase.getPetriNet().getField(fieldId) | |
| .orElseThrow(() -> new IllegalStateException( | |
| "Field %s not found in Petri net".formatted(fieldId))); |
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java
around line 670, the code calls Optional.get() on getField(fieldId) which can
throw NoSuchElementException; replace the direct .get() with a safe retrieval —
either use .orElseThrow(() -> new IllegalArgumentException("Field not found: " +
fieldId)) or check isPresent() and handle the missing field (log and throw a
domain-specific exception or return a sensible default) so the absence of the
field is handled explicitly and with a clear error message.
Description
Implements NAE-2235
Dependencies
No new dependencies were introduced
Third party dependencies
No new dependencies were introduced
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
By unit tests and manually
Test Configuration
Checklist:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.