-
Notifications
You must be signed in to change notification settings - Fork 6
Fixes during and after testing RC9.1 #405
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Introduce `runActionAndPublishEvent` to centralize action execution and event publishing logic. Simplifies code by replacing duplicate logic with a reusable method, improving maintainability and reducing redundancy. Added `consumerMap` to support future event handling enhancements.
Replaces `findByImportId` with `findAllByImportId` for improved pagination handling in `ProcessRoleService`. Updates `ensureIndex` to `createIndex` in `AbstractMongoIndexesConfigurator` to align with updated MongoDB driver conventions.
WalkthroughAdds a new helper in PetriNetService to execute actions and publish a ProcessEvent in import flows; changes ProcessRoleService to detect duplicates via a paged query and filter nulls; switches Mongo index creation call from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java`:
- Around line 131-132: The protected field consumerMap in PetriNetService is
declared but never initialized or used; either remove the consumerMap
declaration entirely if not needed, or initialize it and/or annotate its
intended use (e.g., add a TODO comment) so it isn't a null-unused field — if
keeping it, initialize consumerMap = new ConcurrentHashMap<>() (or new
HashMap<>() as appropriate) and document its purpose for future event handling
within the PetriNetService class.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/configuration/AbstractMongoIndexesConfigurator.java
🧰 Additional context used
🧠 Learnings (7)
📓 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.
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.
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.
📚 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:
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/configuration/AbstractMongoIndexesConfigurator.java
📚 Learning: 2025-12-19T09:06:38.316Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 398
File: application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java:57-58
Timestamp: 2025-12-19T09:06:38.316Z
Learning: In the file `application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java`, the `definition` attribute of the `Action` class (from `com.netgrif.application.engine.objects.petrinet.domain.dataset.logic.action.Action`) is never null, so null checks for `action.getDefinition()` are not needed.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.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/service/ProcessRoleService.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java
📚 Learning: 2026-01-13T16:33:27.510Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 348
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java:4-4
Timestamp: 2026-01-13T16:33:27.510Z
Learning: In the netgrif/application-engine repository, wildcard imports are preferred for the com.netgrif.application.engine.objects.petrinet.domain package. Do not suggest replacing wildcard imports with explicit imports for this package.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.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/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java
📚 Learning: 2025-07-29T17:19:18.300Z
Learnt from: tuplle
Repo: netgrif/application-engine PR: 331
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java:45-46
Timestamp: 2025-07-29T17:19:18.300Z
Learning: In ElasticPetriNetService class, petriNetService is properly initialized using Lazy setter injection rather than constructor injection. This pattern with Lazy Autowired setter methods is commonly used in Spring to resolve circular dependencies and is a valid alternative to constructor injection.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (4)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (2)
215-224: LGTM! Clean consolidation of action execution and event publishing.The refactoring to use
runActionAndPublishEventcorrectly handles both PRE and POST phases:
- PRE phase (line 215):
saveProcessisnullsince the net hasn't been persisted yet- POST phase (line 223):
saveProcessis the actual savedPetriNetThis reduces code duplication and improves maintainability.
229-246: Well-documented helper method.The
runActionAndPublishEventmethod is well-documented with clear parameter descriptions. The implementation correctly:
- Executes actions via
eventService.runActions- Updates the outcome with results
- Publishes the event
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java (1)
209-216: LGTM! Consistent use of paginated query for duplicate detection.The refactoring to use
findAllByImportIdwith pagination is consistent with patterns used elsewhere in the codebase (e.g.,DefaultRoleRunner,AnonymousRoleRunner). The short-circuit evaluation ensuresfindAllByImportIdis only called for global roles, which consistently have non-nullimportIdvalues in this codebase. The null filter at line 215 correctly handles roles skipped due to duplicate detection.nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/configuration/AbstractMongoIndexesConfigurator.java (1)
54-54: The change fromensureIndextocreateIndexis correct.
ensureIndexwas deprecated in Spring Data MongoDB 4.5 with planned removal.createIndexis the recommended replacement method and this change properly aligns with current Spring Data MongoDB conventions.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
...on-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java
Outdated
Show resolved
Hide resolved
Remove the unused `consumerMap` field from `PetriNetService`, simplifying the class structure. This change enhances maintainability by eliminating unnecessary code. No functional impact as the field was not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java`:
- Line 64: Remove the unused import of java.util.function.Consumer from the
PetriNetService class; locate the import line referencing Consumer in the
imports of class PetriNetService and delete it so only used imports remain,
ensuring no other references to Consumer exist in methods or fields before
removing.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java
🧰 Additional context used
🧠 Learnings (5)
📓 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.
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.
📚 Learning: 2025-12-19T09:06:38.316Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 398
File: application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java:57-58
Timestamp: 2025-12-19T09:06:38.316Z
Learning: In the file `application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java`, the `definition` attribute of the `Action` class (from `com.netgrif.application.engine.objects.petrinet.domain.dataset.logic.action.Action`) is never null, so null checks for `action.getDefinition()` are not needed.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java
📚 Learning: 2026-01-13T16:33:27.510Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 348
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java:4-4
Timestamp: 2026-01-13T16:33:27.510Z
Learning: In the netgrif/application-engine repository, wildcard imports are preferred for the com.netgrif.application.engine.objects.petrinet.domain package. Do not suggest replacing wildcard imports with explicit imports for this package.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.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/service/PetriNetService.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/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (2)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (2)
213-222: LGTM! Refactoring to use the helper method is clean.The consolidation of action execution and event publishing into
runActionAndPublishEventimproves maintainability. The distinction between PRE phase (passingnullfor the net since it's not yet persisted) and POST phase (passing the saved net) is correctly preserved.
240-244: Well-structured helper method with clear documentation.The implementation correctly consolidates action execution and event publishing. The Javadoc thoroughly explains the method's purpose. The design is sound:
getPreUploadActions()andgetPostUploadActions()always return a non-nullList<Action>(possibly empty), andeventService.runActions()safely handles empty lists with an early return check.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Description
Fixes during testing.
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?
This was tested manually and with unit tests.
Test Configuration
Checklist:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.