[NAE-2266] Introduce active version of process#389
[NAE-2266] Introduce active version of process#389machacjozef merged 35 commits intorelease/7.0.0-rev9from
Conversation
- add attribute isVersionActive in PetriNet - introduce PetriNetService.getActiveVersionByIdentifier - rename PetriNetService.getNewestVersionByIdentifier to PetriNetService.getLatestVersionByIdentifier - switch usage of getNewestVersionByIdentifier to getActiveVersionByIdentifier - enhance process import validations
- fix repository query - fix null pointer exception - fix unwanted java reference
- fix version initialization
- implement tests to test the version
- implement API to activate the process
- fix logging
- fix log message
- remove unused PetriNetService.evaluateRules method - remove the explicit rollback when saving process changes -> resolved in EE as transactions
- remove todos - implement PetriNetServiceTest.testMakeVersionActive
- split PetriNetService.save into two methods
- rename version active attribute in PetriNet
- remove makeVersionActive API
- update PetriNetService.deletePetriNet to handle versionActive attribute - implement another test for process deletion
- mark methods import petriNet method transactional
- update javadoc
Retoocs
requested review from
machacjozef,
mazarijuraj,
renczesstefan and
tuplle
Retoocs
added
bugfix
WalkthroughThis PR refactors PetriNet version management by replacing Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-07-31T23:40:46.499ZApplied to files:
🧬 Code graph analysis (1)nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.java (2)
⏰ 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)
🔇 Additional comments (5)
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/PetriNet.java (1)
63-107: Consider fully removing or restoring the commented clone method.The extensive commented-out
clone()method (lines 64–107) with the TODO comment suggests deferred maintenance. If the method is genuinely not needed, delete it to reduce clutter and maintenance burden; if it may be useful in the future, restore and test it instead of leaving it commented out.application-engine/src/main/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.java (1)
236-261: Role/net resolution now depends on active versions; consider aligning messaging and null‑safety
- Using
petriNetService.getActiveVersionByIdentifier(netImportId)(Line 240) andgetActiveVersionByIdentifier("preference_filter_item")(Line 264) correctly aligns this flow with the new “active version” semantics.- The log text on Line 255 still says “latest present version of net”, which is slightly misleading now that you explicitly use the active version. Consider updating it to “active version of net” for clarity.
getActiveVersionByIdentifier("preference_filter_item")is dereferenced without a null check. This was structurally identical before with the “newest” call, but with the stricter “active” constraint it might now be easier to end up with no active version configured. If that’s a realistic state, adding an explicit check (and a clear error/log) would make this more robust.Also applies to: 262-268
application-engine/src/main/java/com/netgrif/application/engine/startup/runner/ImpersonationRunner.java (1)
41-54: Config net existence now depends on an active versionSwitching to
petriNetService.getActiveVersionByIdentifier(netIdentifier)means that if a config net exists only in inactive versions,importProcesswill reimport it instead of treating it as already present. For system configuration processes this may be exactly what you want, but it is a behavioral change from the previous “any newest version” check. Please confirm that reimporting when no active version exists is the intended behavior.application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java (1)
267-285: Potential NPE in full‑text search when no active PetriNet version existsHere:
petriNets = processes.stream() .map(process -> petriNetService.getActiveVersionByIdentifier(process)) .collect(Collectors.toList());you later do
petriNets.forEach(net -> net.getImmediateFields().forEach(...))without checking for nulls. If anyprocesshas no active version (identifier exists but all versions inactive, or data not yet migrated),getActiveVersionByIdentifiercan returnnulland you’ll get an NPE at runtime.Consider defensively filtering out nulls and treating “no active nets resolved” as “no full‑text predicates”:
- petriNets = processes.stream() - .map(process -> petriNetService.getActiveVersionByIdentifier(process)) - .collect(Collectors.toList()); + petriNets = processes.stream() + .map(petriNetService::getActiveVersionByIdentifier) + .filter(Objects::nonNull) + .collect(Collectors.toList());and keep the existing
if (petriNets.isEmpty()) return null;behavior.This avoids NPEs while still restricting the search to active nets.
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/PetriNetTest.groovy (1)
131-140:testVersioningConflictsdoes not fail if no exception is thrown; message assertion is fragileRight now, if
importPetriNetstops throwing for this scenario, the test will still pass because all assertions are inside thecatchblock. Also, asserting the full exception message string is brittle to minor wording changes.Consider tightening the test like this:
- try { - petriNetService.importPetriNet(netResource3.inputStream, VersionType.MAJOR, superCreator.loggedSuper) - } catch (Exception e) { - assert e.getMessage() == "A process [test] with such version [0.0.1] already exists" - } + try { + petriNetService.importPetriNet(netResource3.inputStream, VersionType.MAJOR, superCreator.loggedSuper) + assert false: "Expected importPetriNet to fail for duplicate process version" + } catch (Exception e) { + assert e.getMessage() == "A process [test] with such version [0.0.1] already exists" + }Optionally, you could relax the message check a bit (e.g. check that it contains the identifier and version, rather than matching the whole string) to reduce brittleness while still verifying the key dynamic parts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (34)
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy(1 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/delegate/RoleActionDelegate.groovy(2 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovy(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/domain/repositories/PetriNetRepository.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java(6 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/startup/runner/DefaultFiltersRunner.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/startup/runner/ImpersonationRunner.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FilterImportExportService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java(3 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovy(1 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/ImporterTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/PetriNetTest.groovy(1 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/CachePetriNetServiceTest.groovy(1 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy(5 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/workflow/NewInitTest.groovy(1 hunks)application-engine/src/test/resources/change_caseref_value_action_test.xml(2 hunks)application-engine/src/test/resources/petriNets/NAE-1290_Export_actions.xml(1 hunks)application-engine/src/test/resources/petriNets/process_version_1_0_0.xml(1 hunks)application-engine/src/test/resources/petriNets/process_version_2_0_0.xml(1 hunks)application-engine/src/test/resources/petriNets/process_version_3_0_0.xml(1 hunks)application-engine/src/test/resources/petriNets/process_version_4_0_0.xml(1 hunks)application-engine/src/test/resources/petriNets/process_version_5_0_0.xml(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.java(3 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.java(6 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/version/Version.java(2 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/PetriNet.java(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: tuplle
Repo: netgrif/application-engine PR: 334
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java:204-214
Timestamp: 2025-07-31T23:40:46.499Z
Learning: In the PetriNetService.importPetriNet method, existingNet.getVersion() cannot be null because all existing nets in the system were deployed through processes that ensure every net always has a version assigned.
📚 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/petrinet/domain/PetriNetTest.groovyapplication-engine/src/test/resources/petriNets/process_version_2_0_0.xmlapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/FilterImportExportService.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.javaapplication-engine/src/test/resources/change_caseref_value_action_test.xmlapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/domain/repositories/PetriNetRepository.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovynae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovyapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/workflow/NewInitTest.groovyapplication-engine/src/main/java/com/netgrif/application/engine/startup/runner/DefaultFiltersRunner.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/PetriNet.javaapplication-engine/src/test/resources/petriNets/process_version_3_0_0.xmlnae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/ImporterTest.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovyapplication-engine/src/main/java/com/netgrif/application/engine/startup/runner/ImpersonationRunner.javaapplication-engine/src/test/resources/petriNets/process_version_1_0_0.xmlapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javaapplication-engine/src/test/resources/petriNets/NAE-1290_Export_actions.xmlapplication-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/delegate/RoleActionDelegate.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/CachePetriNetServiceTest.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.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/workflow/NewInitTest.groovyapplication-engine/src/test/resources/petriNets/NAE-1290_Export_actions.xml
📚 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/workflow/service/CaseSearchService.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/MenuImportExportService.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:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovynae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/PetriNet.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.java
📚 Learning: 2025-09-29T10:31:57.325Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 362
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java:513-529
Timestamp: 2025-09-29T10:31:57.325Z
Learning: PetriNet.getStringId() returns a simple ObjectId string representation (_id.toString()), not a composite Netgrif ID format, so new ObjectId(petriNetId) works correctly when petriNetId comes from PetriNet.getStringId().
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.javaapplication-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovyapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java
🧬 Code graph analysis (1)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/ActorTransformer.java (1)
ActorTransformer(10-131)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- 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 (36)
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/PetriNet.java (1)
18-62: Verify whether this entity requires active version field mapping.Based on the PR objective (introducing active version semantics across the codebase), this Spring Data MongoDB entity should be reviewed to ensure it includes necessary field mappings for active version support. The AI summary notes that
ElasticPetriNetandVersionclasses were extended to supportversionActive. Please confirm whether thisPetriNetclass should also have an active version field mapped (e.g.,@org.springframework.data.mongodb.core.mapping.Field("versionActive")).application-engine/src/test/resources/petriNets/NAE-1290_Export_actions.xml (1)
220-220: Verify thatgetActiveVersionByIdentifier()contract guarantees non-null and that test setup establishes active versions.The change aligns well with the PR's semantic shift to active versions. However, the direct chain
.stringIdassumes non-null return fromgetActiveVersionByIdentifier().Per the retrieved learnings, existing nets always have versions assigned; confirm that:
getActiveVersionByIdentifier()has a non-null contract (or clarify fallback behavior if no active version exists)- The test setup for NAE-1290_Export_actions properly marks the PetriNet as "active" before this action executes
If the active version may be absent in edge cases, consider adding a null check or handling the scenario explicitly.
application-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovy (2)
131-135: Aligning task search with active PetriNet version looks correctSwitching to
petriNetService.getActiveVersionByIdentifier("export_test").stringIdkeeps the test in sync with the new “active version” semantics and remains safe given the DB is truncated and the net is freshly imported inbefore(), so this call should always resolve to the same net that produced the tasks.
149-156: Consistent use of active version in header test is appropriateUsing
getActiveVersionByIdentifier("export_test")here mirrors the previous change and ensures the repository query targets the active net version whose tasks are being exported; this is consistent with the intended behavior and test setup.application-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovy (1)
314-314: LGTM! Change aligns with PR objectives.The replacement of
getNewestVersionByIdentifier()withgetActiveVersionByIdentifier()is consistent with the PR's goal of introducing semantic distinction between active and latest versions. The test correctly retrieves the active version of the impersonation config Petri net.Since the entire test class is disabled (line 60), verify that this change works as expected when the impersonation tests are re-enabled and ensure an active version of the config Petri net exists during test execution.
application-engine/src/test/groovy/com/netgrif/application/engine/workflow/NewInitTest.groovy (1)
48-48: Using the active PetriNet version in test setup is consistent with new semanticsSwitching from
getNewestVersionByIdentifier("new_init_test")togetActiveVersionByIdentifier("new_init_test")matches the new “active vs newest” behavior. Given the DB is truncated inbefore()and the net is imported right before case creation, resolving the active version here is unambiguous and correctly exercises the active-version path.Based on learnings, the import flow guarantees nets have a proper version assigned, so relying on an active version here is safe.
application-engine/src/test/resources/petriNets/process_version_1_0_0.xml (1)
1-9: LGTM! Test resource is well-formed.The XML structure is valid and properly defines a PetriNet process version test resource with appropriate schema references.
application-engine/src/test/resources/petriNets/process_version_2_0_0.xml (1)
1-9: LGTM! Test resource is well-formed.The XML structure is valid and properly defines version 2.0.0 of the process version test resource.
application-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovy (2)
131-131: LGTM! Active version retrieval is appropriate here.The change correctly uses the active version to determine if a PetriNet already exists before creating a new one. The null check on line 132 properly handles the case where no active version exists.
252-252: LGTM! Active version check is correct.The change appropriately uses the active version to determine if a process has already been imported. The null check on line 253 properly handles cases where no active version exists.
application-engine/src/test/resources/petriNets/process_version_5_0_0.xml (1)
1-9: LGTM! Test resource is well-formed.The XML structure is valid and properly defines version 5.0.0 of the process version test resource.
application-engine/src/test/resources/petriNets/process_version_3_0_0.xml (1)
1-9: LGTM! Test resource is well-formed.The XML structure is valid and properly defines version 3.0.0 of the process version test resource.
application-engine/src/test/resources/petriNets/process_version_4_0_0.xml (1)
1-9: LGTM! Test resource is well-formed.The XML structure is valid and properly defines version 4.0.0 of the process version test resource.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/version/Version.java (1)
55-87: LGTM! Helpful convenience methods added.The new
isHigherThanandisLowerThanmethods provide clear, readable alternatives to directcompareTocalls. The implementation correctly delegates to the existingcompareTomethod, and the Javadoc is well-written.application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (1)
72-75: LGTM! Null handling is present downstream.The change to use
getActiveVersionByIdentifieris correct. ThecachePetriNetFunctionsmethod (line 55) already has a null check, so the flow is safe even if the active version is not found.application-engine/src/main/java/com/netgrif/application/engine/startup/runner/DefaultFiltersRunner.java (1)
427-430: LGTM! Proper null handling in place.The change to
getActiveVersionByIdentifierincludes an appropriate null check with a graceful failure path (returningOptional.empty()). This ensures the filter creation logic is resilient to missing or inactive process definitions.application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java (1)
287-293: LGTM! Null handling is consistent and appropriate.All three
createCaseByIdentifiermethods properly check for null after callinggetActiveVersionByIdentifierand throw descriptiveIllegalArgumentExceptions. This ensures clear error messages when attempting to create cases from inactive or non-existent process definitions.application-engine/src/test/resources/change_caseref_value_action_test.xml (1)
32-37: Switch to active PetriNet versions in test actions is consistentUsing
petriNetService.getActiveVersionByIdentifier(...)here aligns the test with the new active-version semantics and keeps behavior equivalent under the assumption that the referenced nets are active. No additional changes needed.Also applies to: 46-52
nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.java (1)
3-3: versionActive flag and initialization look consistent with active‑version model
- Adding
versionActivewith an index and accessor methods, plusmakeActive()/makeInactive(), cleanly supports the new active‑version concept.- The no‑arg constructor now fully initializes collections and calls
makeInactive(), which is a safe default for newly constructed nets.- The copy constructor copies
versionActivefrom the source net. That’s reasonable for cloning semantics, but any code that uses this copy constructor to create a new version should explicitly reset the flag (e.g., viamakeInactive()) before persisting, if a cloned net must start inactive.Overall the changes are internally consistent; just keep the copy‑semantics in mind at call sites where a “new version” is derived from an existing one.
Also applies to: 72-80, 143-167, 169-204, 441-447
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/ImporterTest.groovy (1)
132-170: Test now correctly validates active vs historical versionsSwitching to
getActiveVersionByIdentifier("new_model")for both the first and second import assertions matches the new semantics: the active net should move from v1 to v2, whilegetPetriNet(net.stringId)later still validates that v1 remains intact as a historical version. The control flow and assertions remain coherent.Also applies to: 206-232
application-engine/src/main/java/com/netgrif/application/engine/petrinet/domain/repositories/PetriNetRepository.java (1)
36-44: Repository method for active/inactive selection is fine; relies on uniqueness invariant
findByIdentifierAndVersionActive(String identifier, boolean versionActive)is consistent with your existingfindByIdentifierAndVersionmethod and matches the new field onPetriNet. It does rely on there being at most one net per(identifier, versionActive)combination; if multiple active versions could ever exist for a given identifier, callers would get an arbitrary one. As long as the service layer enforces “exactly one active version per identifier”, this is fine.nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.java (1)
27-28: Elastic projection correctly mirrors versionActive stateIntroducing
versionActiveonElasticPetriNetand populating it both in the constructor (fromPetriNet) and inupdate()ensures the search index reflects the active/inactive status of each net version. The change is straightforward and consistent with the domain model.Also applies to: 39-44, 50-53
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/CachePetriNetServiceTest.groovy (1)
70-77: Test correctly validates active version cache behavior.The test properly verifies the cache lifecycle:
- Cache is empty before retrieval (line 70, 75)
- After calling
getActiveVersionByIdentifier, the cache is populated (line 77)Minor observation: Line 70 checks for
"processDeleteTest"as a hardcoded string, while line 75 usestestNet.getIdentifier(). This is intentional as the first check occurs before import, but ensure the identifier in the XML (process_delete_test.xml) matches"processDeleteTest".application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy (3)
46-46: New constant for version test files.The format string pattern is clear and follows the existing naming convention.
214-241: Test validates active version reassignment on deletion.This test correctly verifies that:
- Deleting an inactive version doesn't change the active version
- Deleting the active version promotes the latest remaining version to active
- All versions can be deleted sequentially
The test complements
testVersionsOnImportby covering the deletion path of the version lifecycle.
303-305: Good extraction of import helper method.The helper method reduces duplication across tests and centralizes the import call pattern.
application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java (2)
30-37: Clear semantic separation of cache identifiers.The distinction between
petriNetActive(the designated active version) andpetriNetLatest(the highest version number) is well-defined. The documentation comments accurately describe each cache's purpose.
61-66: Both new caches properly included in the aggregate set.The
getAllCaches()method correctly includes bothpetriNetActiveandpetriNetLatestin the returned set, ensuring they are managed alongside other caches.application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (8)
140-146: Cache eviction updated for new cache structure.Both
petriNetActiveandpetriNetLatestcaches are properly cleared, ensuring cache consistency when evicting all caches.
181-184: Transactional annotation added to import methods.Adding
@Transactionalensures atomicity of the import operation. If an exception occurs during import, all database changes (including the deactivation of previous versions) will be rolled back.
243-251: Correct ordering: save inactivated process before new process.Saving the inactivated process first (line 244) before the new process (line 246) ensures that if the new process save fails, the transaction rollback will restore the previous active state. This is the correct order of operations within a transaction.
254-271: Refactored save with internal delegation.The split into public
save()andprotected final doSaveInternal()provides a clear extension point while ensuring consistent persistence behavior. Thefinalmodifier ondoSaveInternalprevents subclasses from bypassing the core save logic.Minor: Line 267 log message uses placeholder correctly. Good error handling that logs but doesn't fail the save operation due to indexing issues.
287-296: Null guard prevents cache key issues.The null check on
identifierandversion(lines 287-289) preventsNullPointerExceptionin the cache key computation (#identifier+#version.toString()in the@Cacheableannotation). Returningnullfor invalid inputs is appropriate since the cache is configured withunless = "#result == null".
310-331: New version retrieval methods with proper caching.Both methods correctly:
- Include null guards to prevent NPE and cache key issues
- Use appropriate cache names (
petriNetActive,petriNetLatest)- Configure
unless = "#result == null"to avoid caching null values
getLatestVersionByIdentifier(lines 321-331) uses descending sort on version components to find the highest version, which is correct.
594-602: Active version reassignment on deletion.When deleting the currently active version, the service correctly promotes the latest remaining version to active. This ensures there's always an active version (if any versions remain) after deletion.
The
self.getLatestVersionByIdentifier()call (line 595) uses the proxy to ensure cache semantics are respected, though note thatevictCache(petriNet)was called just before (line 592), which evicts the latest cache entry. This should still work correctly as the repository query will be executed.
461-464: getReference defaults to active version when version is null.This is the correct semantic change - when no specific version is requested, the active version (not just the newest) should be returned. This aligns with the PR's objective of distinguishing between "active" and "latest" versions.
- add null checks
- rename keyword active to default
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/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.java (1)
263-268: Add null check before dereferencing PetriNet.Line 264 retrieves the default version of "preference_filter_item" and immediately calls
.getStringId()without null validation. This could cause aNullPointerExceptionif the default version doesn't exist.🛡️ Proposed fix
//Creating new Case of preference_filter_item net and setting its data... + PetriNet preferenceItemNet = petriNetService.getDefaultVersionByIdentifier("preference_filter_item"); + if (preferenceItemNet == null) { + resultMessage.append("- Preference filter item process not found or not active\n"); + netCheck.set(false); + return ""; + } Case menuItemCase = workflowService.createCase( - petriNetService.getDefaultVersionByIdentifier("preference_filter_item").getStringId(), + preferenceItemNet.getStringId(), item.getEntryName() + "_" + menuIdentifier, "", ActorTransformer.toLoggedUser(userService.getSystem()) ).getCase();
🤖 Fix all issues with AI agents
In
@application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.java:
- Around line 67-69: Replace the IllegalStateException thrown when petriNet is
null in DashboardItemServiceImpl (the check using the petriNet variable) with
the project-wide ResourceNotFoundException: import
com.netgrif.application.engine.objects.common.ResourceNotFoundException and
com.netgrif.application.engine.objects.common.ResourceNotFoundExceptionCode,
then throw new
ResourceNotFoundException(ResourceNotFoundExceptionCode.DEFAULT_PROCESS_NOT_FOUND)
at the same spot to match FilterImportExportService behavior and codebase
conventions.
In
@application-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovy:
- Line 131: The test calls
petriNetService.getDefaultVersionByIdentifier("export_test").stringId without
checking for null; add an assertion or explicit null check on the result of
getDefaultVersionByIdentifier before accessing .stringId so failures report a
clear message—e.g., assert that the returned object from
petriNetService.getDefaultVersionByIdentifier("export_test") is not null (with a
descriptive message) and only then assign processId from .stringId.
- Line 149: The test currently calls
petriNetService.getDefaultVersionByIdentifier("export_test").stringId without
checking for null; update ExportServiceTest to first assert that
petriNetService.getDefaultVersionByIdentifier("export_test") is not null (or add
a null-check) before accessing .stringId so failures report a clear assertion
error instead of a NullPointerException; reference the call to
petriNetService.getDefaultVersionByIdentifier and the variable processId when
adding the assertion.
In
@application-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovy:
- Line 314: Add a defensive null check for the PetriNet returned by
petriNetService.getDefaultVersionByIdentifier(ImpersonationRunner.IMPERSONATION_CONFIG_PETRI_NET_IDENTIFIER)
before calling helper.createCase; retrieve the result into a local variable,
assert it is not null (or throw an exception) and then pass that variable to
helper.createCase("config", ...), so the test mirrors production patterns (see
getDefaultVersionByIdentifier,
ImpersonationRunner.IMPERSONATION_CONFIG_PETRI_NET_IDENTIFIER, and
helper.createCase).
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (25)
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/delegate/RoleActionDelegate.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovyapplication-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.javaapplication-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/petrinet/domain/repositories/PetriNetRepository.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.javaapplication-engine/src/main/java/com/netgrif/application/engine/startup/runner/DefaultFiltersRunner.javaapplication-engine/src/main/java/com/netgrif/application/engine/startup/runner/ImpersonationRunner.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/FilterImportExportService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/ImporterTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/CachePetriNetServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/workflow/NewInitTest.groovynae-object-library/src/main/java/com/netgrif/application/engine/objects/common/ResourceNotFoundExceptionCode.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.java
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
Learnt from: tuplle
Repo: netgrif/application-engine PR: 334
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java:204-214
Timestamp: 2025-07-31T23:40:46.499Z
Learning: In the PetriNetService.importPetriNet method, existingNet.getVersion() cannot be null because all existing nets in the system were deployed through processes that ensure every net always has a version assigned.
📚 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/petrinet/domain/ImporterTest.groovyapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/FilterImportExportService.javaapplication-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/delegate/RoleActionDelegate.groovyapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/CachePetriNetServiceTest.groovyapplication-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/WorkflowService.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/MenuImportExportService.javaapplication-engine/src/main/java/com/netgrif/application/engine/startup/runner/ImpersonationRunner.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovyapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/workflow/NewInitTest.groovyapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovyapplication-engine/src/main/java/com/netgrif/application/engine/startup/runner/DefaultFiltersRunner.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/test/groovy/com/netgrif/application/engine/petrinet/domain/ImporterTest.groovyapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/FilterImportExportService.javaapplication-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/delegate/RoleActionDelegate.groovyapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.javaapplication-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/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovyapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.javaapplication-engine/src/main/java/com/netgrif/application/engine/startup/runner/DefaultFiltersRunner.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java
📚 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/ImporterTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/CachePetriNetServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/workflow/NewInitTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy
📚 Learning: 2025-12-12T12:40:25.863Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovy:18-20
Timestamp: 2025-12-12T12:40:25.863Z
Learning: In Groovy/Java, if a parameter or collection expects ActorFieldValue, you can pass UserFieldValue (a subclass) due to polymorphism. In tests, verify that ActorListFieldValue accepts ActorFieldValue instances and that passing UserFieldValue instances is valid without casts. This pattern applies to similar Groovy files across the repository.
Applied to files:
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/ImporterTest.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/delegate/RoleActionDelegate.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/CachePetriNetServiceTest.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/workflow/NewInitTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy
📚 Learning: 2025-12-19T07:55:13.427Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 400
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java:239-247
Timestamp: 2025-12-19T07:55:13.427Z
Learning: In the netgrif/application-engine project, the `taskService.findOne()` method (from ITaskService) never returns null. When a task is not found, it handles the error through other means (such as throwing an exception) rather than returning null, so null checks after `findOne()` calls are unnecessary.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FilterImportExportService.javaapplication-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/delegate/RoleActionDelegate.groovyapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.javaapplication-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/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy
📚 Learning: 2025-11-14T10:22:01.634Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 383
File: application-engine/src/main/java/com/netgrif/application/engine/startup/ApplicationRunnerOrderResolver.java:43-43
Timestamp: 2025-11-14T10:22:01.634Z
Learning: For the netgrif/application-engine repository, avoid flagging trivial or nitpick-level issues such as redundant null checks, minor code style improvements, or obvious simplifications that don't affect functionality or introduce bugs. Focus review comments on substantive issues like logic errors, security concerns, performance problems, or breaking changes.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FilterImportExportService.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.javaapplication-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.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/menu/services/DashboardItemServiceImpl.javaapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.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/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/FieldActionsCacheService.java
📚 Learning: 2025-12-12T13:40:00.786Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java:98-104
Timestamp: 2025-12-12T13:40:00.786Z
Learning: In application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java, the permission constraint logic in buildQuery() intentionally applies negative actor constraints globally: ((roleConstraint AND NOT negRoleConstraint) OR actorConstraint) AND NOT negActorConstraint. This means users in negativeViewActors are excluded even if they have positive role permissions, which is the intended authorization behavior.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/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.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy
📚 Learning: 2025-09-29T10:31:57.325Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 362
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java:513-529
Timestamp: 2025-09-29T10:31:57.325Z
Learning: PetriNet.getStringId() returns a simple ObjectId string representation (_id.toString()), not a composite Netgrif ID format, so new ObjectId(petriNetId) works correctly when petriNetId comes from PetriNet.getStringId().
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.javaapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.javaapplication-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovyapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java
📚 Learning: 2025-12-19T07:44:48.019Z
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:48.019Z
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/menu/services/DashboardItemServiceImpl.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/MenuImportExportService.java
📚 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/export/service/ExportServiceTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.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/workflow/service/CaseSearchService.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.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy
🧬 Code graph analysis (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.java (1)
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/PetriNet.java (1)
Getter(15-108)
⏰ 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 (39)
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (1)
2447-2457: LGTM! Null check properly implemented.The null check after retrieving the default version by identifier correctly handles the case where no default version exists, throwing a descriptive
IllegalArgumentException. This addresses the previous review concern and aligns with the PR's shift to default version semantics.nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.java (1)
76-79: LGTM! Default version field properly implemented.The
defaultVersionfield is correctly:
- Annotated with
@Indexedfor efficient queries- Initialized to
falsein the constructor (explicit opt-in required)- Copied in the copy constructor
- Managed via clean helper methods
makeDefault()andmakeNonDefault()This implementation properly supports the PR's default version resolution semantics.
Also applies to: 166-167, 177-177, 441-447
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.java (1)
27-28: LGTM! ElasticPetriNet correctly mirrors PetriNet changes.The
defaultVersionfield is properly:
- Added as a field
- Initialized from the source
PetriNetin the constructor- Updated in the
update()methodThis ensures consistency between MongoDB and Elasticsearch representations of PetriNet default version semantics.
Also applies to: 43-43, 52-52
application-engine/src/test/groovy/com/netgrif/application/engine/workflow/NewInitTest.groovy (1)
48-48: LGTM! Test updated for default version API.The test correctly uses
getDefaultVersionByIdentifierinstead ofgetNewestVersionByIdentifier, aligning with the PR's shift to default version semantics for case creation.application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (1)
74-74: LGTM! Cache reload updated for default version API.The cache reload correctly uses
getDefaultVersionByIdentifierto ensure cached functions are reloaded from the default version of the PetriNet, aligning with the PR's version resolution semantics.application-engine/src/main/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.java (1)
240-243: LGTM - Proper null handling for PetriNet lookup.The code correctly retrieves the default version and validates it exists before using it. The error message is logged appropriately when the net is not found.
application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.java (1)
68-72: LGTM - Proper null handling and error messaging.The code correctly retrieves the default version, validates it exists, and throws a clear exception when not found. This follows the pattern established across the codebase for default version lookups.
application-engine/src/main/java/com/netgrif/application/engine/startup/runner/DefaultFiltersRunner.java (1)
425-428: LGTM - Proper null handling with Optional pattern.The code correctly retrieves the default version and returns
Optional.empty()when the filter process doesn't exist, which is the appropriate error handling for this context.application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/delegate/RoleActionDelegate.groovy (2)
53-59: LGTM - Proper null handling with clear error message.The code correctly retrieves the default version and validates it exists before use. The error message clearly indicates which process identifier was not found.
80-86: LGTM - Consistent null handling pattern.The implementation mirrors the
assignRolemethod with proper null validation and clear error messaging, ensuring consistent behavior across role operations.application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/ImporterTest.groovy (1)
133-133: LGTM! Test assertions correctly updated to use default version retrieval.The test logic aligns with the PR's objective to switch from newest-version to default-version semantics. After importing each version, the test correctly verifies that
getDefaultVersionByIdentifierreturns the expected active version.Also applies to: 207-207
application-engine/src/main/java/com/netgrif/application/engine/startup/runner/ImpersonationRunner.java (1)
42-46: LGTM! Proper null handling for default version retrieval.The code correctly checks if the default version exists before proceeding to import, with appropriate null handling at line 43.
application-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovy (2)
131-135: LGTM! Proper null handling in upsertNet.The method correctly uses
getDefaultVersionByIdentifierand handles the null case (when no default version exists) with Groovy's truthy check at line 132.
252-256: LGTM! Proper null handling in importProcessOnce.The method correctly uses
getDefaultVersionByIdentifierand handles the null case with an explicit null check at line 253.application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java (1)
283-283: Filter out null results to prevent potential NullPointerException.If
getDefaultVersionByIdentifier(process)returnsnullfor any process identifier (e.g., when no default version exists), the null will be collected into thepetriNetslist. TheisEmpty()check at line 285 won't catch null elements, and theforEachloop at line 302 will throw aNullPointerExceptionwhen attempting to access methods on a nullnet.🛡️ Proposed fix to filter nulls
- petriNets = processes.stream().map(process -> petriNetService.getDefaultVersionByIdentifier(process)).collect(Collectors.toList()); + petriNets = processes.stream() + .map(process -> petriNetService.getDefaultVersionByIdentifier(process)) + .filter(Objects::nonNull) + .collect(Collectors.toList());You'll also need to add this import at the top of the file:
import java.util.Objects;⛔ Skipped due to learnings
Learnt from: tuplle Repo: netgrif/application-engine PR: 334 File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java:204-214 Timestamp: 2025-07-31T23:40:46.499Z Learning: In the PetriNetService.importPetriNet method, existingNet.getVersion() cannot be null because all existing nets in the system were deployed through processes that ensure every net always has a version assigned.Learnt from: Retoocs Repo: netgrif/application-engine PR: 400 File: application-engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java:239-247 Timestamp: 2025-12-19T07:55:13.427Z Learning: In the netgrif/application-engine project, the `taskService.findOne()` method (from ITaskService) never returns null. When a task is not found, it handles the error through other means (such as throwing an exception) rather than returning null, so null checks after `findOne()` calls are unnecessary.application-engine/src/main/java/com/netgrif/application/engine/petrinet/domain/repositories/PetriNetRepository.java (1)
36-43: Method only supportsdefaultVersion=truein current codebase; document or restrict accordingly.The method is only called with
defaultVersion=true(inPetriNetService.getDefaultVersionByIdentifier), which is correct since a single active version per identifier is guaranteed. However, the method signature allowsdefaultVersion=false, which would causeIncorrectResultSizeDataAccessExceptionif called when multiple inactive versions exist for the same identifier, since the return type expects a single entity.Either document in the Javadoc that this method should only be called with
defaultVersion=true, or remove thedefaultVersionparameter and create a separate method for the inactive version case if future support is needed.nae-object-library/src/main/java/com/netgrif/application/engine/objects/common/ResourceNotFoundExceptionCode.java (1)
9-10: LGTM!The new enum constant follows the existing pattern and provides a clear, specific error code for missing default processes.
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java (1)
156-169: LGTM!The method rename from
getNewestVersionByIdentifiertogetDefaultVersionByIdentifierand the addition ofgetLatestVersionByIdentifieralign well with the PR objectives. The Javadoc clearly documents that both methods can return null, which helps callers handle missing processes appropriately.application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FilterImportExportService.java (1)
271-274: LGTM!The change to
getDefaultVersionByIdentifierwith proper null checking andResourceNotFoundExceptionfollows the correct pattern for handling missing resources. The error message is clear and informative.application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java (3)
287-293: LGTM! Correct transition to default version retrieval.The change from
getNewestVersionByIdentifiertogetDefaultVersionByIdentifieraligns with the PR's introduction of the default version concept. The null-check and exception handling remain appropriate.
301-307: LGTM! Consistent with other overloads.This overload correctly follows the same pattern as the other
createCaseByIdentifiermethods, using the default version for case creation.
310-316: LGTM! Third overload correctly updated.All three
createCaseByIdentifieroverloads now consistently use the default version, maintaining proper error handling throughout.application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/CachePetriNetServiceTest.groovy (1)
70-77: LGTM! Test correctly validates default cache behavior.The test has been properly updated to validate the new default cache semantics:
- References
petriNetDefaultcache instead of the removedpetriNetNewest- Uses
getDefaultVersionByIdentifierfor retrieval- Validates caching behavior for the default version
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy (5)
45-45: Good addition of version test file format constant.The
VERSION_PROCESS_FILE_FORMATconstant provides a clear template for version test files, improving test maintainability.
309-311: LGTM! Helper method reduces test duplication.The
importProcesshelper centralizes import invocation with consistent parameters (stream and VersionType.MAJOR), improving test code maintainability.
151-217: Comprehensive test validates version activation semantics.This test thoroughly covers the version activation logic:
- ✅ Importing a new version marks it as default
- ✅ Importing a higher version deactivates the previous default
- ✅ Importing a lower version doesn't affect default status
- ✅ Duplicate version imports are rejected
- ✅ Manual default manipulation is respected
- ✅ Elasticsearch synchronization is validated
The test validates both in-memory PetriNet state and Elasticsearch indexed state, ensuring consistency across storage layers.
219-246: Excellent test coverage for default version on deletion.This test validates the critical behavior when deleting PetriNet versions:
- ✅ Deleting a non-default version preserves the current default
- ✅ Deleting the default version activates the latest remaining version
- ✅ Deleting the last version removes it without errors
This aligns with the implementation in
PetriNetService.deletePetriNet(lines 624-632).
107-107: Consistent refactoring to use helper method.The existing
processImportAndDeletetest has been refactored to use the newimportProcesshelper, improving consistency across the test suite.application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/CacheConfigurationProperties.java (2)
29-37: LGTM! Cache configuration correctly updated for new semantics.The replacement of
petriNetNewestwithpetriNetDefaultandpetriNetLatestprovides clearer separation of concerns:
petriNetDefaultcaches the active/default versionpetriNetLatestcaches the highest version numberThis distinction supports the new versioning semantics introduced in the PR.
61-66: LGTM! Cache set construction updated correctly.The
getAllCaches()method correctly includes both new cache names (petriNetDefaultandpetriNetLatest) in place of the removedpetriNetNewest.application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (9)
140-154: LGTM! Cache management correctly updated for new cache structure.Both
evictAllCachesandevictCachehave been properly updated:
- Clear/evict from
petriNetDefaultandpetriNetLatestcaches- Removed references to the old
petriNetNewest- Maintained null-safety with
requireNonNull
181-194: LGTM! @transactional annotations ensure import atomicity.Adding
@Transactionalto allimportPetriNetoverloads ensures that version checking, default version updates, and persistence operations are atomic. This reduces (though doesn't fully eliminate without database constraints) race condition risks during concurrent imports.
194-224: LGTM! Import flow refactored for clarity.The import flow has been improved:
- Version handling logic extracted to
checkAndHandleProcessVersion- Clear separation between validation, preparation, and persistence phases
- Properly saves both the new process and any inactivated previous default
- Uses new
doSaveInternalfor consistent persistence/indexing
226-280: Comprehensive version handling logic correctly implemented.The
checkAndHandleProcessVersionmethod implements the version activation requirements:✅ Duplicate detection (lines 249-252): Throws exception if version already exists
✅ Version initialization (lines 255-263):
- New identifier → initialize to 1.0.0
- Existing identifier with no version → increment from latest
- Explicit version → use as-is and determine if highest
✅ Default version management (lines 264-273):
- If new version is highest, deactivate current default (either latest or explicitly default)
- If new version is lower, don't change existing default
✅ Activation (lines 275-277): Only highest version becomes default
The logic correctly handles the requirement stated in past review: "uploading also lower versions must be allowed (patching older version); uploaded process becomes active only if the version is highest."
282-299: LGTM! Centralized persistence logic with improved return type.The refactoring introduces:
doSaveInternal: Protected final method that centralizes persistence and indexing logic- Updated
savesignature: Now returnsOptional<PetriNet>(breaking change, but appropriate given potential failures)- Consistent usage: Both public
saveand internal import flow usedoSaveInternalThe
finalmodifier ondoSaveInternalensures consistent persistence behavior across the service.
314-324: Good defensive null guard added.Lines 315-317 add a null check that prevents
NullPointerExceptionif eitheridentifierorversionis null. This is consistent with the method's contract of returningnullwhen the PetriNet cannot be found.
339-359: LGTM! New version retrieval methods correctly implemented.Both methods provide clear semantics:
getDefaultVersionByIdentifier(lines 339-345):
- Retrieves the process marked as default (via
defaultVersionflag)- Cached in
petriNetDefault- Requires new repository method
findByIdentifierAndDefaultVersion
getLatestVersionByIdentifier(lines 347-359):
- Retrieves the highest version by sorting (DESC on major, minor, patch)
- Cached in
petriNetLatest- Efficiently uses
PageRequest.of(0, 1, ...)to fetch only the top resultBoth include null guards and appropriate caching strategies.
607-634: Excellent default version management on deletion.Lines 624-632 implement critical logic:
- When the default version is deleted, the system automatically promotes the latest remaining version to default
- Prevents "orphaned" processes where no default exists despite having versions
- Proper logging helps with debugging and auditing
This behavior is validated by the
testVersionActiveOnDeletetest and ensures consistent system state.
489-492: LGTM! Reference retrieval correctly uses default version.Line 490 ensures that when no specific version is requested (
version == null), the method returns a reference to the default version rather than an arbitrary version. This aligns with user expectations.
- use ResourceNotFoundException - add paged result in PetriNetRepository.findByIdentifierAndDefaultVersion
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
@application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java:
- Around line 339-365: getDefaultVersionByIdentifier silently returns the first
match if multiple entities have defaultVersion=true; update the method (around
repository.findByIdentifierAndDefaultVersion and the getFirst() return path) to
check if the returned List has size > 1 and log a warning (e.g., logger.warn)
including the identifier and the number of defaults found to surface
data-integrity issues; ensure the logging call uses the class logger (LOGGER or
existing logger) and keep the original return behaviour (still return
result.getFirst()).
- Around line 198-224: The call to doSaveInternal(newProcess) currently discards
the returned persisted instance, then outcome.setNet(...) uses the pre-save
importedProcess; instead capture the return Optional from doSaveInternal (e.g.,
Optional<PetriNet> savedProcess = doSaveInternal(newProcess)) and then set
outcome.setNet(savedProcess.orElseThrow()) so the outcome contains the
DB-persisted PetriNet with assigned IDs/timestamps; keep running post-upload
actions as-is but replace the direct use of importedProcess/newProcess for
outcome.setNet with the savedProcess result.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
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/petrinet/domain/repositories/PetriNetRepository.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javaapplication-engine/src/test/resources/change_caseref_value_action_test.xmlapplication-engine/src/test/resources/petriNets/NAE-1290_Export_actions.xml
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: tuplle
Repo: netgrif/application-engine PR: 334
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java:204-214
Timestamp: 2025-07-31T23:40:46.499Z
Learning: In the PetriNetService.importPetriNet method, existingNet.getVersion() cannot be null because all existing nets in the system were deployed through processes that ensure every net always has a version assigned.
📚 Learning: 2025-12-12T12:36:06.651Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/main/resources/petriNets/engine-processes/org_group.xml:36-39
Timestamp: 2025-12-12T12:36:06.651Z
Learning: In the org_group.xml file (application-engine/src/main/resources/petriNets/engine-processes/org_group.xml), variable names in action blocks like userField and user_selection should be kept as-is even when the field type changes from "user" to "actor".
Applied to files:
application-engine/src/test/resources/change_caseref_value_action_test.xmlapplication-engine/src/test/resources/petriNets/NAE-1290_Export_actions.xmlapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.javaapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.java
📚 Learning: 2025-12-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/test/resources/change_caseref_value_action_test.xmlapplication-engine/src/test/resources/petriNets/NAE-1290_Export_actions.xmlapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.javaapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.java
📚 Learning: 2025-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/resources/change_caseref_value_action_test.xmlapplication-engine/src/test/resources/petriNets/NAE-1290_Export_actions.xmlapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javaapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.javaapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.java
📚 Learning: 2025-09-05T10:21:54.893Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 350
File: application-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovy:135-135
Timestamp: 2025-09-05T10:21:54.893Z
Learning: In ExportServiceTest.groovy, writing to src/test/resources is intentional to simulate production behavior where the working tree is mutated during file exports. This mirrors how the system works in production.
Applied to files:
application-engine/src/test/resources/petriNets/NAE-1290_Export_actions.xml
📚 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
📚 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:55:13.427Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 400
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java:239-247
Timestamp: 2025-12-19T07:55:13.427Z
Learning: In the netgrif/application-engine project, the `taskService.findOne()` method (from ITaskService) never returns null. When a task is not found, it handles the error through other means (such as throwing an exception) rather than returning null, so null checks after `findOne()` calls are unnecessary.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.javaapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.java
📚 Learning: 2025-09-29T10:31:31.469Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 362
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java:43-45
Timestamp: 2025-09-29T10:31:31.469Z
Learning: In the Netgrif application engine, when Authentication objects reach controller endpoints, auth.getPrincipal() always returns a LoggedUser instance and is never null, so defensive casting checks are not necessary in controller methods.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.javaapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.java
📚 Learning: 2025-12-12T13:40:00.786Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java:98-104
Timestamp: 2025-12-12T13:40:00.786Z
Learning: In application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java, the permission constraint logic in buildQuery() intentionally applies negative actor constraints globally: ((roleConstraint AND NOT negRoleConstraint) OR actorConstraint) AND NOT negActorConstraint. This means users in negativeViewActors are excluded even if they have positive role permissions, which is the intended authorization behavior.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.javaapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.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/menu/services/DashboardItemServiceImpl.java
📚 Learning: 2025-11-14T10:22:01.634Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 383
File: application-engine/src/main/java/com/netgrif/application/engine/startup/ApplicationRunnerOrderResolver.java:43-43
Timestamp: 2025-11-14T10:22:01.634Z
Learning: For the netgrif/application-engine repository, avoid flagging trivial or nitpick-level issues such as redundant null checks, minor code style improvements, or obvious simplifications that don't affect functionality or introduce bugs. Focus review comments on substantive issues like logic errors, security concerns, performance problems, or breaking changes.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.java
🧬 Code graph analysis (1)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/ActorTransformer.java (1)
ActorTransformer(10-131)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (11)
application-engine/src/test/resources/change_caseref_value_action_test.xml (1)
32-32: LGTM! API migration aligns with PR objectives.The replacement of
getNewestVersionByIdentifierwithgetDefaultVersionByIdentifieris consistent with this PR's introduction of the active/default version concept. Both changes correctly update the test resource to use the new API while preserving the test's intent.Also applies to: 46-46
application-engine/src/test/resources/petriNets/NAE-1290_Export_actions.xml (1)
219-224: Change correctly implements active-version semantics and is safe.The update from
getNewestVersionByIdentifiertogetDefaultVersionByIdentifierproperly aligns with the PR's shift to default-version support. The test setup inExportServiceTest.groovy(@beforeeach, line 69) successfully imports and establishes "export_test" as the default version before action_2 executes, so the direct call to.stringIdis safe.application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.java (1)
70-74: LGTM! Consistent migration to default version semantics.The change from
getNewestVersionByIdentifiertogetDefaultVersionByIdentifierwith explicit null-checking andResourceNotFoundExceptionaligns with the PR's objective to use an active/default version instead of newest. The error message is clear and the exception code is appropriate.application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.java (1)
68-72: LGTM! Consistent migration to default version semantics.The change from
getNewestVersionByIdentifiertogetDefaultVersionByIdentifierwith explicit null-checking andResourceNotFoundExceptionaligns with the PR's objective. The pattern is consistent with other services in this PR.application-engine/src/main/java/com/netgrif/application/engine/petrinet/domain/repositories/PetriNetRepository.java (1)
36-44: LGTM! Well-documented repository method.The new
findByIdentifierAndDefaultVersionmethod follows Spring Data naming conventions and includes comprehensive Javadoc. The paginated return type is appropriate for potentially large result sets.application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (6)
142-143: LGTM! Cache eviction updated consistently.The cache eviction logic has been correctly updated to use
petriNetDefaultandpetriNetLatestinstead of the deprecatedpetriNetNewest. BothevictAllCachesandevictCachemethods are updated consistently.Also applies to: 150-151
181-181: LGTM! Critical transactional boundary.Adding
@Transactionalto allimportPetriNetoverloads is essential for maintaining data consistency. The import flow now involves updating the previous default version (making it non-default) and saving the new default version, which must happen atomically to prevent race conditions where multiple processes could be marked as default.Also applies to: 187-187, 193-193
315-317: LGTM! Prevents cache key generation errors.Adding null checks for
identifierandversionis essential because the method is annotated with@Cacheableusing a key expression#identifier+#version.toString(). Without these checks, null inputs would cause NPE during cache key generation.
496-496: LGTM! Consistent with default version semantics.The change from
getNewestVersionByIdentifiertogetDefaultVersionByIdentifierwhen version is null aligns with the PR's objective to use explicit default versions instead of newest.
283-299: No breaking change detected.The
save()method already returnsOptional<PetriNet>as defined in theIPetriNetServiceinterface; this is not a change fromvoid. The refactoring simply delegates the publicsave()method to a protected finaldoSaveInternal()method, separating the public interface from the internal implementation—a reasonable design pattern.The
protected finalmodifier ondoSaveInternal()appropriately prevents subclasses from overriding critical persistence logic.Likely an incorrect or invalid review comment.
226-280: The version management logic incheckAndHandleProcessVersionis correctly implemented. The caching and transaction isolation are handled appropriately:
- The
@Cacheablemethods (getLatestVersionByIdentifierandgetDefaultVersionByIdentifier) are called within the@Transactionalboundary and provide the initial state- In-memory modifications are made to objects (via
makeDefault()/makeNonDefault())- Cache eviction occurs in
doSaveInternalbeforerepository.save(), which is the correct pattern- The
@Transactionaldecorator ensures database-level isolation for the entire import operation- Subsequent cache accesses will fetch fresh data after persistence
No transaction isolation or cache consistency issues are present. The implementation follows standard Spring caching best practices.
| @Cacheable(value = "petriNetDefault", unless = "#result == null") | ||
| public PetriNet getDefaultVersionByIdentifier(String identifier) { | ||
| if (identifier == null) { | ||
| return null; | ||
| } | ||
| return nets.getFirst(); | ||
|
|
||
| List<PetriNet> result = repository.findByIdentifierAndDefaultVersion(identifier, true, PageRequest.of(0, 1)).getContent(); | ||
| if (!result.isEmpty()) { | ||
| return result.getFirst(); | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| @Cacheable(value = "petriNetLatest", unless = "#result == null") | ||
| public PetriNet getLatestVersionByIdentifier(String identifier) { | ||
| if (identifier == null) { | ||
| return null; | ||
| } | ||
| List<PetriNet> processes = repository.findByIdentifier(identifier, PageRequest.of(0, 1, | ||
| Sort.Direction.DESC, "version.major", "version.minor", "version.patch")).getContent(); | ||
| if (processes.isEmpty()) { | ||
| return null; | ||
| } | ||
| return processes.getFirst(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider logging warning if multiple default versions found.
Both methods implement correct version lookup logic:
getDefaultVersionByIdentifier: Queries bydefaultVersion=trueflaggetLatestVersionByIdentifier: Sorts by version numbers descending
However, getDefaultVersionByIdentifier uses PageRequest.of(0, 1) and getFirst() (lines 345-347). If a data integrity issue causes multiple processes with the same identifier to have defaultVersion=true, this silently returns one of them. Consider logging a warning if the query returns more than one result to detect such integrity violations.
🔍 Proposed logging enhancement
- List<PetriNet> result = repository.findByIdentifierAndDefaultVersion(identifier, true, PageRequest.of(0, 1)).getContent();
+ Page<PetriNet> page = repository.findByIdentifierAndDefaultVersion(identifier, true, PageRequest.of(0, 2));
+ List<PetriNet> result = page.getContent();
+ if (result.size() > 1) {
+ log.warn("Data integrity issue: Multiple default versions found for identifier [{}]", identifier);
+ }
if (!result.isEmpty()) {
return result.getFirst();
}🤖 Prompt for AI Agents
In
@application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java
around lines 339 - 365, getDefaultVersionByIdentifier silently returns the first
match if multiple entities have defaultVersion=true; update the method (around
repository.findByIdentifierAndDefaultVersion and the getFirst() return path) to
check if the returned List has size > 1 and log a warning (e.g., logger.warn)
including the identifier and the number of defaults found to surface
data-integrity issues; ensure the logging call uses the class logger (LOGGER or
existing logger) and keep the original return behaviour (still return
result.getFirst()).
| if (petriNet.isDefaultVersion()) { | ||
| PetriNet processToMakeDefault = self.getLatestVersionByIdentifier(petriNet.getIdentifier()); | ||
| if (processToMakeDefault != null) { | ||
| log.debug("The default version was removed. Making the latest version of the process [{}] with id [{}] as default...", | ||
| processToMakeDefault.getIdentifier(), processToMakeDefault.getStringId()); | ||
| processToMakeDefault.makeDefault(); | ||
| save(processToMakeDefault); | ||
| } | ||
| } |
There was a problem hiding this comment.
Correct default version succession logic; minor race condition.
The logic correctly ensures that when the default version is deleted, the latest remaining version becomes the new default. The cache eviction on line 628 happens before the lookup on line 631, which is the correct order.
However, there's a minor race condition: between the cache eviction (line 628) and the lookup (line 631), another thread could populate the cache with the just-deleted version. While unlikely and low-impact (would self-correct on next cache eviction), consider using a more aggressive eviction strategy for this critical path.
🔒 Optional: Reduce race condition window
repository.deleteBy_id(petriNet.getObjectId());
evictCache(petriNet);
+ // Aggressively clear all version caches for this identifier to minimize stale data
+ evictAllCaches();
functionCacheService.reloadCachedFunctions(petriNet);Note: This trades performance for correctness. The race condition is minor, so this may not be necessary.
Committable suggestion skipped: line range outside the PR's diff.
- fix outcome consistency in PetriNetService
- fix javadoc
There was a problem hiding this comment.
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 226-280: The method checkAndHandleProcessVersion does an
unnecessary fetch of existingActiveProcess via getDefaultVersionByIdentifier
when existingLatestProcess is already the default; avoid the redundant DB call
by reusing existingLatestProcess: when makeNonDefaultCurrentVersion is true and
existingLatestProcess != null, check existingLatestProcess.isDefaultVersion()
first and handle makeNonDefault on it and set processToMakeNonDefault to
existingLatestProcess, otherwise only then call getDefaultVersionByIdentifier to
fetch and make that one non-default; update control flow around
existingLatestProcess and existingActiveProcess to prevent the extra fetch.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/domain/repositories/PetriNetRepository.javaapplication-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: tuplle
Repo: netgrif/application-engine PR: 334
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java:204-214
Timestamp: 2025-07-31T23:40:46.499Z
Learning: In the PetriNetService.importPetriNet method, existingNet.getVersion() cannot be null because all existing nets in the system were deployed through processes that ensure every net always has a version assigned.
📚 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-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
📚 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
⏰ 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 (8)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/domain/repositories/PetriNetRepository.java (1)
36-44: LGTM! Well-designed repository method.The new
findByIdentifierAndDefaultVersionmethod follows Spring Data naming conventions correctly and provides the necessary pagination support for retrieving processes by their default version status. The Javadoc is comprehensive and clearly explains the method's purpose.application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (7)
142-143: Cache management correctly updated for default/latest semantics.The cache eviction logic properly reflects the shift from "newest" to separate "default" and "latest" caches, ensuring cache consistency across the new version resolution strategy.
Also applies to: 150-151
181-181: Appropriate use of @transactional for data consistency.Adding
@Transactionalto all import overloads ensures atomicity when updating multiple PetriNet entities (e.g., making one non-default while activating another), which is critical for maintaining default version integrity.Also applies to: 187-187, 193-193
315-317: Good defensive programming with null guards.Adding null checks for both
identifierandversionparameters prevents potential NullPointerExceptions and provides clear, predictable behavior when invalid arguments are passed.
339-351: Well-implemented default version retrieval with proper caching.The method correctly uses the new repository method with efficient pagination (fetching only 1 result) and proper null handling. The cache key strategy (identifier only) is appropriate for default version lookups.
630-638: Critical logic correctly maintains default version invariant.The deletion logic properly handles the case where the default version is deleted by promoting the latest remaining version to default. This ensures the system invariant (exactly one default version per identifier) is maintained. The null check safely handles the case where no other versions exist.
353-365: The sorting by"version.major","version.minor","version.patch"in descending order works correctly. These fields are declared aslongprimitive types in the Version class, so MongoDB stores them as numeric types and sorts them numerically, not lexicographically. The concern about string sorting issues does not apply here.Likely an incorrect or invalid review comment.
283-299: The refactoring is safe. There are no subclasses of PetriNetService in the codebase, and no other implementations of IPetriNetService. Thesave()method itself is not final and can still be overridden if needed. Theprotected finalmodifier ondoSaveInternalis appropriate—it's an internal method designed to prevent external override while maintaining encapsulation.Likely an incorrect or invalid review comment.
| /** | ||
| * Validates the version of an importing process. This method can update 'newProces': initialize version, | ||
| * initialize default version attribute. This method can also update the current default version of the process if needed. | ||
| * | ||
| * <h4>Version initialization logic</h4> | ||
| * <ul> | ||
| * <li>Uploaded process becomes default only if its version is the highest</li> | ||
| * <li>Only one process can be default. If the importing process is about to get default, the currently default | ||
| * process becomes non-default</li> | ||
| * <li>If no version is provided, it's initialized to 1.0.0 or by the existing highest version incremented | ||
| * by 'releaseType' input parameter</li> | ||
| * </ul> | ||
| * | ||
| * @param newProcess A process to be checked and updated | ||
| * @param releaseType requested release type level. It's used for version initialization | ||
| * | ||
| * @return The process, which has been made non-default or null if no process updated | ||
| * | ||
| * @throws IllegalArgumentException if the version already exists | ||
| * */ | ||
| private PetriNet checkAndHandleProcessVersion(PetriNet newProcess, VersionType releaseType) { | ||
| PetriNet processToMakeNonDefault = null; | ||
|
|
||
| if (newProcess.getVersion() != null && self.getPetriNet(newProcess.getIdentifier(), newProcess.getVersion()) != null) { | ||
| throw new IllegalArgumentException("A process [%s] with such version [%s] already exists" | ||
| .formatted(newProcess.getIdentifier(), newProcess.getVersion())); | ||
| } | ||
| PetriNet existingLatestProcess = self.getLatestVersionByIdentifier(newProcess.getIdentifier()); | ||
| boolean makeNonDefaultCurrentVersion = true; | ||
| if (existingLatestProcess == null && newProcess.getVersion() == null) { | ||
| newProcess.setVersion(new Version()); | ||
| } else { | ||
| if (newProcess.getVersion() == null) { | ||
| newProcess.setVersion(existingLatestProcess.getVersion().clone()); | ||
| newProcess.incrementVersion(releaseType); | ||
| } else if (existingLatestProcess != null && newProcess.getVersion().isLowerThan(existingLatestProcess.getVersion())) { | ||
| makeNonDefaultCurrentVersion = false; | ||
| } | ||
| if (makeNonDefaultCurrentVersion && existingLatestProcess != null && existingLatestProcess.isDefaultVersion()) { | ||
| existingLatestProcess.makeNonDefault(); | ||
| processToMakeNonDefault = existingLatestProcess; | ||
| } else if (makeNonDefaultCurrentVersion) { | ||
| PetriNet existingActiveProcess = self.getDefaultVersionByIdentifier(newProcess.getIdentifier()); | ||
| if (existingActiveProcess != null) { | ||
| existingActiveProcess.makeNonDefault(); | ||
| processToMakeNonDefault = existingActiveProcess; | ||
| } | ||
| } | ||
| } | ||
| if (makeNonDefaultCurrentVersion) { | ||
| newProcess.makeDefault(); | ||
| } | ||
|
|
||
| return processToMakeNonDefault; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider optimizing the default version lookup to avoid redundant fetches.
The logic correctly ensures only one default version exists, but there's a potential inefficiency: when existingLatestProcess is also the default version (lines 264-266), the code later fetches existingActiveProcess (line 268) which would be the same entity already in memory.
♻️ Optimization to avoid redundant database fetch
- if (makeNonDefaultCurrentVersion && existingLatestProcess != null && existingLatestProcess.isDefaultVersion()) {
- existingLatestProcess.makeNonDefault();
- processToMakeNonDefault = existingLatestProcess;
- } else if (makeNonDefaultCurrentVersion) {
- PetriNet existingActiveProcess = self.getDefaultVersionByIdentifier(newProcess.getIdentifier());
- if (existingActiveProcess != null) {
- existingActiveProcess.makeNonDefault();
- processToMakeNonDefault = existingActiveProcess;
- }
- }
+ if (makeNonDefaultCurrentVersion) {
+ if (existingLatestProcess != null && existingLatestProcess.isDefaultVersion()) {
+ existingLatestProcess.makeNonDefault();
+ processToMakeNonDefault = existingLatestProcess;
+ } else {
+ PetriNet existingActiveProcess = self.getDefaultVersionByIdentifier(newProcess.getIdentifier());
+ if (existingActiveProcess != null) {
+ existingActiveProcess.makeNonDefault();
+ processToMakeNonDefault = existingActiveProcess;
+ }
+ }
+ }This avoids fetching existingActiveProcess when existingLatestProcess is already the default version.
🤖 Prompt for AI Agents
In
@application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java
around lines 226 - 280, The method checkAndHandleProcessVersion does an
unnecessary fetch of existingActiveProcess via getDefaultVersionByIdentifier
when existingLatestProcess is already the default; avoid the redundant DB call
by reusing existingLatestProcess: when makeNonDefaultCurrentVersion is true and
existingLatestProcess != null, check existingLatestProcess.isDefaultVersion()
first and handle makeNonDefault on it and set processToMakeNonDefault to
existingLatestProcess, otherwise only then call getDefaultVersionByIdentifier to
fetch and make that one non-default; update control flow around
existingLatestProcess and existingActiveProcess to prevent the extra fetch.
| void testVersionsOnImport() { | ||
| ImportPetriNetEventOutcome outcome = importProcess(VERSION_PROCESS_FILE_FORMAT.formatted("2"), superCreator.loggedSuper) | ||
| PetriNet petriNetV2 = outcome.getNet() | ||
| assert petriNetV2 != null |
There was a problem hiding this comment.
For better readability of assertion error and test readability, it is better to use JUnit assertions, like assertNotNull, assertTrue, etc. (https://docs.junit.org/5.14.2/writing-tests/assertions.html)
It is also explained in this article: https://learn-it-university.com/the-ultimate-guide-to-choosing-between-assert-and-junit-assertions/
There was a problem hiding this comment.
thanks, I updated the tests
- update test to use assert methods
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
@application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy:
- Line 161: Replace identity comparisons using '==' between Version instances
with value equality checks: change assertions like
assertTrue(petriNetV2.getVersion() == version) to use assertEquals(version,
petriNetV2.getVersion()) or assertTrue(version.equals(petriNetV2.getVersion())).
Apply the same change for the other assertions that compare Version instances
(the ones using petriNetV2.getVersion() and other Version vars) so all version
comparisons use .equals() or assertEquals rather than '=='.
- Line 184: Replace the inconsistent assertion assertTrue(petriNetV1 != null)
with assertNotNull(petriNetV1) to match the other checks (lines using
assertNotNull) and improve readability; update the test that references
petriNetV1 by changing the assertion method from assertTrue to assertNotNull.
- Around line 162-163: Replace the brittle Thread.sleep(5000) calls with a
polling helper that repeatedly calls
elasticPetriNetRepository.findById(petriNetV2.stringId) until present or a
timeout elapses; implement a private waitForElasticSync(String id, long
timeoutMs) that polls (e.g. 100ms intervals) and throws an AssertionError if
timeout is reached, then replace the sleep usages (including the other
occurrences around lines 175-176 and 212-213) with calls to
waitForElasticSync(petriNetV2.stringId).
- Line 103: The tests processImportAndDelete and processSearch are annotated
@Disabled without explanation; either update the test implementations in
PetriNetServiceTest to align with the new default-version semantics and remove
@Disabled from the methods processImportAndDelete and processSearch so they run,
or keep them disabled but add an inline comment and a TODO referencing a
tracking issue/PR that explains why they are incompatible with the new behavior
and what must change to re-enable them; ensure you reference the exact test
method names (processImportAndDelete, processSearch) when adding the comment or
issue link so future reviewers can find and restore them.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: tuplle
Repo: netgrif/application-engine PR: 334
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java:204-214
Timestamp: 2025-07-31T23:40:46.499Z
Learning: In the PetriNetService.importPetriNet method, existingNet.getVersion() cannot be null because all existing nets in the system were deployed through processes that ensure every net always has a version assigned.
📚 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/petrinet/service/PetriNetServiceTest.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/service/PetriNetServiceTest.groovy
📚 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/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy
📚 Learning: 2025-12-12T13:40:00.786Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java:98-104
Timestamp: 2025-12-12T13:40:00.786Z
Learning: In application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java, the permission constraint logic in buildQuery() intentionally applies negative actor constraints globally: ((roleConstraint AND NOT negRoleConstraint) OR actorConstraint) AND NOT negActorConstraint. This means users in negativeViewActors are excluded even if they have positive role permissions, which is the intended authorization behavior.
Applied to files:
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.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/petrinet/service/PetriNetServiceTest.groovy
📚 Learning: 2025-12-12T12:37:58.852Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/test/groovy/com/netgrif/application/engine/auth/WorkflowAuthorizationServiceTest.groovy:4-4
Timestamp: 2025-12-12T12:37:58.852Z
Learning: In tests under application-engine/src/test, GroupService.getDefaultSystemGroup() lazily creates the default system group if it doesn't exist. Tests can rely on this behavior and omit explicit existence/setup checks for the default system group.
Applied to files:
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy
📚 Learning: 2025-12-12T12:40:25.863Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovy:18-20
Timestamp: 2025-12-12T12:40:25.863Z
Learning: In Groovy/Java, if a parameter or collection expects ActorFieldValue, you can pass UserFieldValue (a subclass) due to polymorphism. In tests, verify that ActorListFieldValue accepts ActorFieldValue instances and that passing UserFieldValue instances is valid without casts. This pattern applies to similar Groovy files across the repository.
Applied to files:
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- 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
🔇 Additional comments (5)
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy (5)
4-4: LGTM: Import reorganization supports new versioning tests.The added imports (wildcard auth.domain, ElasticPetriNet, repositories, and static assertions) are appropriate for the new version management test cases.
Also applies to: 8-10, 16-18, 37-40
48-48: LGTM: Well-named constant for versioned test files.The format string clearly indicates its purpose and is used consistently in the new version tests.
223-249: LGTM: Solid test coverage for default version cascade on deletion.The test correctly validates that when the default version is deleted, the system automatically promotes another version to default. The logic flow (delete non-default, delete default, verify cascade) is well-structured.
258-259: LGTM: Consistent use of importProcess helper.Refactoring to use the new
importProcesshelper improves test maintainability.
312-314: LGTM: Helper method reduces duplication.The
importProcesshelper centralizes the import logic withVersionType.MAJOR, making tests more maintainable and consistent.
| assertTrue(petriNetV2.defaultVersion) | ||
| Version version = new Version() | ||
| version.setMajor(2) | ||
| assertTrue(petriNetV2.getVersion() == version) |
There was a problem hiding this comment.
Fix Version equality comparisons.
Using == to compare Version objects checks reference equality, not value equality. In Java/Groovy, this may not work as intended if the objects are different instances with the same version numbers.
🐛 Use assertEquals or .equals() for value comparison
Replace:
-assertTrue(petriNetV2.getVersion() == version)
+assertEquals(version, petriNetV2.getVersion())Apply similar changes at lines 174, 189, and 211.
Also applies to: 174-174, 189-189, 211-211
🤖 Prompt for AI Agents
In
@application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy
at line 161, Replace identity comparisons using '==' between Version instances
with value equality checks: change assertions like
assertTrue(petriNetV2.getVersion() == version) to use assertEquals(version,
petriNetV2.getVersion()) or assertTrue(version.equals(petriNetV2.getVersion())).
Apply the same change for the other assertions that compare Version instances
(the ones using petriNetV2.getVersion() and other Version vars) so all version
comparisons use .equals() or assertEquals rather than '=='.
- update test
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
@application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy:
- Line 252: The test method processSearch in PetriNetServiceTest is annotated
with @Disabled; either remove the @Disabled annotation to enable the test now
(so its 9 search scenarios run) or add a clear comment/Javadoc on the test
method explaining the specific blocker (e.g., awaiting NAE-2266 "Introduce
active version of process") and reference the issue id, and if temporarily
disabled wrap it with a conditional/assumption that documents the expected
state; update the test method name processSearch and its annotation accordingly
so the intent is explicit.
- Line 103: The disabled test processImportAndDelete is no longer aligned with
the new version-focused tests; either remove the test entirely or update it to
reflect version semantics: if keeping it, rewrite processImportAndDelete to
import a process using the current version-aware importer (use the same import
helper used by testVersionsOnImport), then assert expected behavior under
versioning (verify created process version, createCase against the imported
process, verify process role creation and user role assignment using the
versioned process identifier, and assert delete behavior using the version-aware
delete flow similar to testVersionActiveOnDelete); if removing, delete the
processImportAndDelete test method and its @Disabled annotation and adjust any
shared fixtures referenced only by that test.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: tuplle
Repo: netgrif/application-engine PR: 334
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java:204-214
Timestamp: 2025-07-31T23:40:46.499Z
Learning: In the PetriNetService.importPetriNet method, existingNet.getVersion() cannot be null because all existing nets in the system were deployed through processes that ensure every net always has a version assigned.
📚 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/petrinet/service/PetriNetServiceTest.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/service/PetriNetServiceTest.groovy
📚 Learning: 2025-12-12T12:36:06.651Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/main/resources/petriNets/engine-processes/org_group.xml:36-39
Timestamp: 2025-12-12T12:36:06.651Z
Learning: In the org_group.xml file (application-engine/src/main/resources/petriNets/engine-processes/org_group.xml), variable names in action blocks like userField and user_selection should be kept as-is even when the field type changes from "user" to "actor".
Applied to files:
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy
📚 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/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy
📚 Learning: 2025-12-12T13:40:00.786Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java:98-104
Timestamp: 2025-12-12T13:40:00.786Z
Learning: In application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java, the permission constraint logic in buildQuery() intentionally applies negative actor constraints globally: ((roleConstraint AND NOT negRoleConstraint) OR actorConstraint) AND NOT negActorConstraint. This means users in negativeViewActors are excluded even if they have positive role permissions, which is the intended authorization behavior.
Applied to files:
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.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/petrinet/service/PetriNetServiceTest.groovy
📚 Learning: 2025-12-12T12:37:58.852Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/test/groovy/com/netgrif/application/engine/auth/WorkflowAuthorizationServiceTest.groovy:4-4
Timestamp: 2025-12-12T12:37:58.852Z
Learning: In tests under application-engine/src/test, GroupService.getDefaultSystemGroup() lazily creates the default system group if it doesn't exist. Tests can rely on this behavior and omit explicit existence/setup checks for the default system group.
Applied to files:
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy
📚 Learning: 2025-12-12T12:40:25.863Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovy:18-20
Timestamp: 2025-12-12T12:40:25.863Z
Learning: In Groovy/Java, if a parameter or collection expects ActorFieldValue, you can pass UserFieldValue (a subclass) due to polymorphism. In tests, verify that ActorListFieldValue accepts ActorFieldValue instances and that passing UserFieldValue instances is valid without casts. This pattern applies to similar Groovy files across the repository.
Applied to files:
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (2)
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy (2)
312-314: LGTM! Good refactoring to reduce duplication.The helper method simplifies test code and provides a consistent way to import processes with sensible defaults.
223-249: Verify auto-promotion logic when default version is deleted.The test assumes that when the default version (v3) is deleted, the system automatically promotes the oldest remaining version (v1) to be the new default (line 244). This critical business logic must be explicitly verified in
PetriNetService.deletePetriNet:
- Does the deletion logic include code to promote another version to default when the current default is deleted?
- What criteria determine which version becomes the new default (oldest remaining, newest remaining, creation order)?
- Is this auto-promotion behavior intentional and properly implemented, or does it rely on implicit ordering side effects?
Without verification of the implementation, the test's expectations cannot be confirmed.
- Introduced a new `defaultVersion` Boolean property to the `PetriNetSearch` class to enhance extensibility and enable version-specific searches.
# Conflicts: # nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.java
machacjozef
requested review from
renczesstefan and
tuplle
and removed request for
tuplle
4 participants
Description
Introduced new petriNet attribute
defaultVersion. The internal implementation no longer finds petri nets by the latest version, but finds the petri net, that is default. Only one version per identifier can be default.Implements NAE-2266
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?
Manually and by unit tests
PetriNetServiceTest
Test Configuration
Checklist:
Summary by CodeRabbit
Release Notes
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.