[NAE-2448] Migration for NAE v7 phase 2#451
Open
renczesstefan wants to merge 7 commits into
Open
Conversation
Added comprehensive test cases for the `nae_2432` process migration. Introduced `MigrationHelper` to facilitate data field addition, transition updates, and error handling during migrations. Created `MigrationError` to encapsulate migration failure states with detailed metadata. Updated PetriNet versions from v1.0 to v2.0 and verified seamless case migration, role updates, and task modifications.
…ifier` for PetriNet retrieval
Update migration tests: rename PetriNet files and identifiers from `nae_2432` to `migration_test`
Remove unused `webEnvironment` property from Elastic test annotations
- Add support for case removal synchronized between MongoDB and Elasticsearch in `MigrationHelper` and `CaseMigrationHelper`. - Introduce `updateCasesCursor` for ObjectId-based case updates. - Refactor and streamline `migratePetriNet` logic to update case IDs. - Implement MongoDB bulk operations with upsert using `FindAndReplaceOptions`. - Update migration tests with constants, improved assertions, and enhanced logic.
- Update error handling logic in `CaseMigrationHelper` and `AbstractMigrationHelper`, including better handling of migration limits and error policies. - Replace error map with a thread-safe `CopyOnWriteArrayList` in migration processes. - Add safeguards for `MigrationErrorException` handling. - Make `MongodbUtils` a utility class with a private constructor to prevent instantiation. - Enhance migration tests with error clearing before execution.
Introduce `MigrationBeansConfiguration` and remove `@Component` annotation from `MigrationHelper`
WalkthroughThis PR introduces a comprehensive migration framework for workflow cases, tasks, and Petri net definitions. The system enables bulk MongoDB updates with configurable error handling, Elasticsearch re-indexing, dataset transformations, and role/permission synchronization across versioned Petri nets. ChangesPetri Net and Workflow Case Migration Framework
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/AbstractMigrationHelper.groovy`:
- Around line 193-195: Fix the malformed Javadoc on AbstractMigrationHelper by
removing the leading space and splitting the merged lines into a properly
formatted Javadoc description: start with /** on its own line, then each
sentence on its own line prefixed with " *", describing that the method iterates
documents, applies updates, executes bulk operations, supports pagination via
page size, allows custom bulk processing and optional sleep between pages;
ensure the closing " */" is on its own line and attach this comment to the
corresponding method in AbstractMigrationHelper.
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/CaseMigrationHelper.groovy`:
- Around line 340-361: Update the Javadoc to reference the correct parameter
name (toRemove) and guard the unsafe cast in removeChoices(Case useCase,
Map<String, List<String>> toRemove): before casting dataField.value to Set,
verify its type (e.g., instanceof Collection or Set) and only call removeAll on
it when safe; if the field holds a single String (enum) handle that case by
comparing/removing the value accordingly, and ensure you only call removeAll on
dataField.choices when dataField.choices != null as already done.
- Around line 368-381: The method changeFileFieldToFileList is casting
existingValue to FileFieldValue unsafely; update it to check the runtime type of
existingValue before adding to FileListFieldValue.namesPaths (e.g., use
instanceof or an explicit type check on existingValue) and only add when it's a
FileFieldValue, otherwise skip and optionally log a warning on dataField or
useCase to aid debugging; adjust handling in the block referencing
existingValue, FileFieldValue, FileListFieldValue, dataField and namesPaths so
no ClassCastException can occur during migration.
- Around line 443-455: The removeCase method validates MongoDB deletion but
doesn't handle failures from elasticCaseService.remove(useCase.getStringId());
wrap that call in a try-catch inside removeCase, catch Exception (or a more
specific exception if appropriate), log the error and call
handleMigrationError(errorPolicy, "removeCase", type, useCase.stringId, <error
message>) so Elasticsearch removal failures are reported using the same error
policy; ensure successful trace logging remains after the try block and return
or continue consistent with existing error handling.
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/PetriNetMigrationHelper.groovy`:
- Around line 5-15: PetriNetMigrationHelper.groovy references the unqualified
symbol VersionType (e.g. VersionType.MAJOR) but lacks its import; add the
missing import statement for
com.netgrif.application.engine.objects.petrinet.domain.VersionType to the top
import block so the Groovy compiler can resolve VersionType when used in
PetriNetMigrationHelper (search for the VersionType.MAJOR usage to verify).
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/TaskMigrationHelper.groovy`:
- Around line 235-242: The addRoleToExistingTasks method currently calls
updateTasks without forwarding the caller's error policy, causing it to always
use defaultErrorPolicy; change the call in addRoleToExistingTasks to accept and
pass an explicit ErrorPolicy argument (propagate the active policy from
MigrationHelper.withErrorPolicy) so updateTasks receives the caller's policy
instead of falling back to defaultErrorPolicy(); update the updateTasks
invocation signature usage in addRoleToExistingTasks to include the policy
parameter (and ensure MigrationHelper.withErrorPolicy callers pass their policy
through) while keeping the existing role/permissions closure unchanged.
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/migration/MigrationHelper.groovy`:
- Around line 600-606: collectErrors currently returns from the finally block
which swallows exceptions from migrationCode.call(); change the method to avoid
returning inside finally: call clearErrors(), declare a List<MigrationError>
variable (e.g. errors), invoke migrationCode.call() inside try, in finally
assign errors = popErrors(), and after the try/finally return errors so any
exception thrown by migrationCode.call() will propagate while still ensuring
popErrors() runs (refer to collectErrors, migrationCode.call, clearErrors,
popErrors).
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/migration/model/MigrationError.groovy`:
- Around line 132-134: The getter getEntityType() currently declares return type
String but the field entityType is Class<?>; update the method signature to
"Class<?> getEntityType()" and return the entityType field unchanged so the
types match, i.e., change the return type to Class<?> in the getEntityType()
method (or if string semantics were intended, alternatively convert the field to
String and adjust any constructor/assignments to store a String instead).
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/migration/model/MigrationErrorPolicy.groovy`:
- Around line 100-108: The factory method throwAfterLimit currently rejects
maxErrors <= 0 which contradicts setMaxErrors, the
MigrationProperties.ErrorPolicy javadoc and
AbstractMigrationHelper.finishMigrationErrorPolicy; update throwAfterLimit to
accept maxErrors == 0 (i.e., only reject negative values) so THROW_AFTER_LIMIT
with maxErrors == 0 is allowed and handled consistently, ensuring behavior
aligns with MigrationErrorHandlingMode.THROW_AFTER_LIMIT and the logic in
AbstractMigrationHelper.finishMigrationErrorPolicy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aad25209-009a-4016-9393-71fbf79e87bb
📒 Files selected for processing (17)
application-engine/src/main/groovy/com/netgrif/application/engine/migration/MigrationHelper.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/AbstractMigrationHelper.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/CaseMigrationHelper.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/PetriNetMigrationHelper.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/TaskMigrationHelper.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/migration/model/MigrationError.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/migration/model/MigrationErrorHandlingMode.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/migration/model/MigrationErrorPolicy.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/migration/throwable/MigrationErrorException.groovyapplication-engine/src/main/java/com/netgrif/application/engine/configuration/MigrationBeansConfiguration.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/properties/MigrationProperties.javaapplication-engine/src/main/java/com/netgrif/application/engine/utils/MongodbUtils.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticSearchTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/migration/MigrationTest.groovyapplication-engine/src/test/resources/petriNets/migration_test_v1.xmlapplication-engine/src/test/resources/petriNets/migration_test_v2.xml
💤 Files with no reviewable changes (2)
- application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovy
- application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticSearchTest.groovy
Comment on lines
+193
to
+195
| /** | ||
| Iterates over the documents in the collection, applies updates, and executes bulk operations. * The iteration is paginated based on the provided or default page size, and supports customizable | ||
| * bulk operation processing and optional sleep intervals between pages. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Javadoc formatting issue.
The opening line of the Javadoc comment has a leading space and appears to have merged content from multiple lines, making it harder to read.
Proposed fix
/**
- Iterates over the documents in the collection, applies updates, and executes bulk operations. * The iteration is paginated based on the provided or default page size, and supports customizable
+ * Iterates over the documents in the collection, applies updates, and executes bulk operations.
+ * The iteration is paginated based on the provided or default page size, and supports customizable
* bulk operation processing and optional sleep intervals between pages.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| Iterates over the documents in the collection, applies updates, and executes bulk operations. * The iteration is paginated based on the provided or default page size, and supports customizable | |
| * bulk operation processing and optional sleep intervals between pages. | |
| /** | |
| * Iterates over the documents in the collection, applies updates, and executes bulk operations. | |
| * The iteration is paginated based on the provided or default page size, and supports customizable | |
| * bulk operation processing and optional sleep intervals between pages. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/AbstractMigrationHelper.groovy`
around lines 193 - 195, Fix the malformed Javadoc on AbstractMigrationHelper by
removing the leading space and splitting the merged lines into a properly
formatted Javadoc description: start with /** on its own line, then each
sentence on its own line prefixed with " *", describing that the method iterates
documents, applies updates, executes bulk operations, supports pagination via
page size, allows custom bulk processing and optional sleep between pages;
ensure the closing " */" is on its own line and attach this comment to the
corresponding method in AbstractMigrationHelper.
Comment on lines
+340
to
+361
| /** | ||
| * Removes choices from enumeration or multichoice field | ||
| * @param useCase Instance of Case | ||
| * @param toAdd Map<field id, list of choices to add into data field> | ||
| */ | ||
| static void removeChoices(Case useCase, Map<String, List<String>> toRemove) { | ||
| log.debug("Starting removeChoices for case: ${useCase.stringId}, fields: ${toRemove.keySet()}") | ||
| toRemove.each { dataFieldID, choicesToRemove -> | ||
| log.trace("Removing choices ${choicesToRemove} from field ${dataFieldID} in case: ${useCase.stringId}") | ||
| DataField dataField = useCase.dataSet[dataFieldID] | ||
| if (!dataField) { | ||
| return | ||
| } | ||
| if (dataField.value != null) { | ||
| (dataField.value as Set).removeAll(choicesToRemove) | ||
| } | ||
|
|
||
| if (dataField.choices != null) { | ||
| dataField.choices.removeAll(choicesToRemove) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Documentation and unsafe cast issues.
-
Javadoc parameter name mismatch (line 343): The documentation says
@param toAddbut the actual parameter is namedtoRemove. -
Unsafe cast to Set (line 354): The code casts
dataField.valuetoSetwithout validation. If this method is called on a field that is not a multichoice or enumeration field, it will throw aClassCastExceptionat runtime, potentially halting the migration process.
Proposed fix
Fix the documentation and add type validation:
/**
* Removes choices from enumeration or multichoice field
* `@param` useCase Instance of Case
- * `@param` toAdd Map<field id, list of choices to add into data field>
+ * `@param` toRemove Map<field id, list of choices to remove from data field>
*/
static void removeChoices(Case useCase, Map<String, List<String>> toRemove) {
log.debug("Starting removeChoices for case: ${useCase.stringId}, fields: ${toRemove.keySet()}")
toRemove.each { dataFieldID, choicesToRemove ->
log.trace("Removing choices ${choicesToRemove} from field ${dataFieldID} in case: ${useCase.stringId}")
DataField dataField = useCase.dataSet[dataFieldID]
if (!dataField) {
return
}
- if (dataField.value != null) {
+ if (dataField.value != null && dataField.value instanceof Set) {
(dataField.value as Set).removeAll(choicesToRemove)
+ } else if (dataField.value != null) {
+ log.warn("Field ${dataFieldID} value is not a Set, skipping choice removal")
}
if (dataField.choices != null) {
dataField.choices.removeAll(choicesToRemove)
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Removes choices from enumeration or multichoice field | |
| * @param useCase Instance of Case | |
| * @param toAdd Map<field id, list of choices to add into data field> | |
| */ | |
| static void removeChoices(Case useCase, Map<String, List<String>> toRemove) { | |
| log.debug("Starting removeChoices for case: ${useCase.stringId}, fields: ${toRemove.keySet()}") | |
| toRemove.each { dataFieldID, choicesToRemove -> | |
| log.trace("Removing choices ${choicesToRemove} from field ${dataFieldID} in case: ${useCase.stringId}") | |
| DataField dataField = useCase.dataSet[dataFieldID] | |
| if (!dataField) { | |
| return | |
| } | |
| if (dataField.value != null) { | |
| (dataField.value as Set).removeAll(choicesToRemove) | |
| } | |
| if (dataField.choices != null) { | |
| dataField.choices.removeAll(choicesToRemove) | |
| } | |
| } | |
| } | |
| /** | |
| * Removes choices from enumeration or multichoice field | |
| * `@param` useCase Instance of Case | |
| * `@param` toRemove Map<field id, list of choices to remove from data field> | |
| */ | |
| static void removeChoices(Case useCase, Map<String, List<String>> toRemove) { | |
| log.debug("Starting removeChoices for case: ${useCase.stringId}, fields: ${toRemove.keySet()}") | |
| toRemove.each { dataFieldID, choicesToRemove -> | |
| log.trace("Removing choices ${choicesToRemove} from field ${dataFieldID} in case: ${useCase.stringId}") | |
| DataField dataField = useCase.dataSet[dataFieldID] | |
| if (!dataField) { | |
| return | |
| } | |
| if (dataField.value != null && dataField.value instanceof Set) { | |
| (dataField.value as Set).removeAll(choicesToRemove) | |
| } else if (dataField.value != null) { | |
| log.warn("Field ${dataFieldID} value is not a Set, skipping choice removal") | |
| } | |
| if (dataField.choices != null) { | |
| dataField.choices.removeAll(choicesToRemove) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/CaseMigrationHelper.groovy`
around lines 340 - 361, Update the Javadoc to reference the correct parameter
name (toRemove) and guard the unsafe cast in removeChoices(Case useCase,
Map<String, List<String>> toRemove): before casting dataField.value to Set,
verify its type (e.g., instanceof Collection or Set) and only call removeAll on
it when safe; if the field holds a single String (enum) handle that case by
comparing/removing the value accordingly, and ensure you only call removeAll on
dataField.choices when dataField.choices != null as already done.
Comment on lines
+368
to
+381
| static void changeFileFieldToFileList(Case useCase, String fieldId) { | ||
| log.debug("Starting changeFileFieldToFileList for case: ${useCase.stringId}, field: ${fieldId}") | ||
| FileListFieldValue fileListFieldValue = new FileListFieldValue() | ||
| DataField dataField = useCase.dataSet[fieldId] | ||
| if (!dataField) { | ||
| return | ||
| } | ||
| def existingValue = dataField.value | ||
| if (existingValue != null) { | ||
| fileListFieldValue.namesPaths.add(existingValue as FileFieldValue) | ||
| } | ||
| dataField.value = fileListFieldValue | ||
| log.trace("Converted field ${fieldId} from FileFieldValue to FileListFieldValue in case: ${useCase.stringId}") | ||
| } |
There was a problem hiding this comment.
Unsafe cast to FileFieldValue.
Line 377 casts existingValue to FileFieldValue without type validation. If the field value is not a FileFieldValue (e.g., due to data inconsistency or incorrect usage), this will throw a ClassCastException at runtime, potentially halting the migration.
Proposed fix
Add type validation before casting:
static void changeFileFieldToFileList(Case useCase, String fieldId) {
log.debug("Starting changeFileFieldToFileList for case: ${useCase.stringId}, field: ${fieldId}")
FileListFieldValue fileListFieldValue = new FileListFieldValue()
DataField dataField = useCase.dataSet[fieldId]
if (!dataField) {
return
}
def existingValue = dataField.value
- if (existingValue != null) {
+ if (existingValue != null && existingValue instanceof FileFieldValue) {
fileListFieldValue.namesPaths.add(existingValue as FileFieldValue)
+ } else if (existingValue != null) {
+ log.warn("Field ${fieldId} value is not a FileFieldValue (actual type: ${existingValue.class.simpleName}), skipping conversion")
+ return
}
dataField.value = fileListFieldValue
log.trace("Converted field ${fieldId} from FileFieldValue to FileListFieldValue in case: ${useCase.stringId}")
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/CaseMigrationHelper.groovy`
around lines 368 - 381, The method changeFileFieldToFileList is casting
existingValue to FileFieldValue unsafely; update it to check the runtime type of
existingValue before adding to FileListFieldValue.namesPaths (e.g., use
instanceof or an explicit type check on existingValue) and only add when it's a
FileFieldValue, otherwise skip and optionally log a warning on dataField or
useCase to aid debugging; adjust handling in the block referencing
existingValue, FileFieldValue, FileListFieldValue, dataField and namesPaths so
no ClassCastException can occur during migration.
Comment on lines
+443
to
+455
| void removeCase(Case useCase, MigrationErrorPolicy errorPolicy = defaultErrorPolicy()) { | ||
| log.debug("Starting removeCase for case: ${useCase.stringId}") | ||
| DeleteResult deleteResult = mongoTemplate.remove(useCase) | ||
| log.trace("MongoDB delete result for case ${useCase.stringId}: acknowledged=${deleteResult.wasAcknowledged()}, deletedCount=${deleteResult.deletedCount}") | ||
| if (!deleteResult.wasAcknowledged()) { | ||
| String message = "Failed to delete case ${useCase.stringId} from MongoDB" | ||
| log.error(message) | ||
| handleMigrationError(errorPolicy, "removeCase", type, useCase.stringId, message) | ||
| return | ||
| } | ||
| elasticCaseService.remove(useCase.getStringId()) | ||
| log.trace("Successfully removed case ${useCase.stringId} from Elasticsearch") | ||
| } |
There was a problem hiding this comment.
Missing error handling for Elasticsearch removal.
The method validates the MongoDB deletion result (line 447) and handles failures via handleMigrationError, but the subsequent Elasticsearch removal at line 453 has no error handling. If elasticCaseService.remove() throws an exception, it will propagate uncaught, potentially leaving the migration in an inconsistent state or causing it to abort without proper error reporting.
Proposed fix
Add error handling for Elasticsearch removal:
elasticCaseService.remove(useCase.getStringId())
log.trace("Successfully removed case ${useCase.stringId} from Elasticsearch")
+ } catch (Exception ex) {
+ String message = "Failed to remove case ${useCase.stringId} from Elasticsearch after MongoDB deletion"
+ log.error(message, ex)
+ handleMigrationError(errorPolicy, "removeCase", type, useCase.stringId, message, ex)
}Note: You'll need to wrap the Elasticsearch call in a try-catch block.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/CaseMigrationHelper.groovy`
around lines 443 - 455, The removeCase method validates MongoDB deletion but
doesn't handle failures from elasticCaseService.remove(useCase.getStringId());
wrap that call in a try-catch inside removeCase, catch Exception (or a more
specific exception if appropriate), log the error and call
handleMigrationError(errorPolicy, "removeCase", type, useCase.stringId, <error
message>) so Elasticsearch removal failures are reported using the same error
policy; ensure successful trace logging remains after the try block and return
or continue consistent with existing error handling.
Comment on lines
+5
to
+15
| import com.netgrif.application.engine.importer.service.Importer | ||
| import com.netgrif.application.engine.objects.petrinet.domain.I18nString | ||
| import com.netgrif.application.engine.objects.petrinet.domain.PetriNet | ||
| import com.netgrif.application.engine.objects.petrinet.domain.Transition | ||
| import com.netgrif.application.engine.objects.petrinet.domain.dataset.ActorField | ||
| import com.netgrif.application.engine.objects.petrinet.domain.dataset.Field | ||
| import com.netgrif.application.engine.objects.petrinet.domain.dataset.FieldType | ||
| import com.netgrif.application.engine.objects.petrinet.domain.events.Event | ||
| import com.netgrif.application.engine.objects.petrinet.domain.events.EventType | ||
| import com.netgrif.application.engine.objects.petrinet.domain.roles.ProcessRole | ||
| import com.netgrif.application.engine.objects.workflow.domain.Case |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
FILE="application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/PetriNetMigrationHelper.groovy"
echo "Usage sites:"
rg -n "VersionType" "$FILE"
echo "Import sites:"
rg -n "^import .*VersionType" "$FILE" || trueRepository: netgrif/application-engine
Length of output: 261
🏁 Script executed:
#!/bin/bash
set -e
FILE="application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/PetriNetMigrationHelper.groovy"
echo "Package + current imports:"
sed -n '1,40p' "$FILE" | cat -n
echo "Definition candidates for VersionType:"
rg -n --hidden --no-ignore-vcs "enum VersionType|class VersionType|interface VersionType" application-engine/src/main/groovy application-engine/src/main/java || true
echo "Any static imports or wildcard imports for VersionType:"
rg -n "^import .*VersionType|^import .*\\*|^import static .*VersionType" "$FILE" || trueRepository: netgrif/application-engine
Length of output: 2711
🏁 Script executed:
#!/bin/bash
set -e
FILE="application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/PetriNetMigrationHelper.groovy"
echo "Package + current imports:"
sed -n '1,40p' "$FILE" | cat -n
echo "Definition candidates for VersionType:"
rg -n "enum VersionType|class VersionType|interface VersionType" application-engine/src/main/groovy application-engine/src/main/java || true
echo "Any static imports or wildcard imports for VersionType:"
rg -n "^import .*VersionType|^import .*\\*|^import static .*VersionType" "$FILE" || trueRepository: netgrif/application-engine
Length of output: 2711
🏁 Script executed:
#!/bin/bash
set -e
FILE="application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/PetriNetMigrationHelper.groovy"
echo "Package + current imports:"
sed -n '1,60p' "$FILE" | cat -n
echo "Definition candidates for VersionType:"
rg -n "enum VersionType|class VersionType|interface VersionType" application-engine/src/main/groovy application-engine/src/main/java || true
echo "Any static imports or wildcard imports for VersionType:"
rg -n "^import .*VersionType|^import .*\\*|^import static .*VersionType" "$FILE" || trueRepository: netgrif/application-engine
Length of output: 3563
🏁 Script executed:
#!/bin/bash
set -e
# Find VersionType declarations in the repo
echo "== Declarations =="
rg -n --hidden --no-ignore-vcs -S "enum[[:space:]]+VersionType|class[[:space:]]+VersionType|interface[[:space:]]+VersionType" application-engine/src/main || true
# Find all usages of VersionType in the repo (limited output)
echo "== Usages (limited) =="
rg -n -S "VersionType\." application-engine/src/main | head -n 50
# Find usages specifically in the reviewed file (for context)
echo "== Usages in PetriNetMigrationHelper =="
rg -n -S "VersionType\." application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/PetriNetMigrationHelper.groovy || trueRepository: netgrif/application-engine
Length of output: 2193
🏁 Script executed:
#!/bin/bash
set -e
echo "== Full repo declarations/definitions of VersionType =="
rg -n --hidden --no-ignore-vcs -S "enum[[:space:]]+VersionType|class[[:space:]]+VersionType|interface[[:space:]]+VersionType" . || true
echo "== Full repo references to objects.petrinet.domain.VersionType =="
rg -n --hidden --no-ignore-vcs -S "objects\.petrinet\.domain\.VersionType" . || true
echo "== Full repo references to 'VersionType' imports =="
rg -n --hidden --no-ignore-vcs -S "^import .*VersionType" . || true
echo "== Where 'VersionType.MAJOR' appears =="
rg -n --hidden --no-ignore-vcs -S "VersionType\.MAJOR" . | head -n 50 || trueRepository: netgrif/application-engine
Length of output: 37338
🏁 Script executed:
#!/bin/bash
set -e
echo "== Full repo declarations/definitions of VersionType =="
rg -n --hidden --no-ignore-vcs -S "enum[[:space:]]+VersionType|class[[:space:]]+VersionType|interface[[:space:]]+VersionType" . || true
echo "== Full repo references to objects.petrinet.domain.VersionType =="
rg -n --hidden --no-ignore-vcs -S "objects\.petrinet\.domain\.VersionType" . || true
echo "== Full repo references to 'VersionType' imports =="
rg -n --hidden --no-ignore-vcs -S "^import .*VersionType" . || true
echo "== Where 'VersionType.MAJOR' appears =="
rg -n --hidden --no-ignore-vcs -S "VersionType\.MAJOR" . | head -n 50 || trueRepository: netgrif/application-engine
Length of output: 37841
🏁 Script executed:
#!/bin/bash
set -e
echo "== Full repo declarations/definitions of VersionType =="
rg -n --hidden --no-ignore-vcs -S "enum[[:space:]]+VersionType|class[[:space:]]+VersionType|interface[[:space:]]+VersionType" . || true
echo "== Full repo references to objects.petrinet.domain.VersionType =="
rg -n --hidden --no-ignore-vcs -S "objects\.petrinet\.domain\.VersionType" . || true
echo "== Full repo references to 'VersionType' imports =="
rg -n --hidden --no-ignore-vcs -S "^import .*VersionType" . || true
echo "== Where 'VersionType.MAJOR' appears =="
rg -n --hidden --no-ignore-vcs -S "VersionType\.MAJOR" . | head -n 50 || trueRepository: netgrif/application-engine
Length of output: 37704
🏁 Script executed:
#!/bin/bash
set -e
echo "== Full repo declarations/definitions of VersionType =="
rg -n --hidden --no-ignore-vcs -S "enum[[:space:]]+VersionType|class[[:space:]]+VersionType|interface[[:space:]]+VersionType" . || true
echo "== Full repo references to objects.petrinet.domain.VersionType =="
rg -n --hidden --no-ignore-vcs -S "objects\.petrinet\.domain\.VersionType" . || true
echo "== Full repo references to 'VersionType' imports =="
rg -n --hidden --no-ignore-vcs -S "^import .*VersionType" . || true
echo "== Where 'VersionType.MAJOR' appears =="
rg -n --hidden --no-ignore-vcs -S "VersionType\.MAJOR" . | head -n 50 || trueRepository: netgrif/application-engine
Length of output: 38159
Add missing VersionType import (Groovy compile).
VersionType.MAJOR is referenced in PetriNetMigrationHelper.groovy (line 142), but this file has no import com.netgrif.application.engine.objects.petrinet.domain.VersionType (and no wildcard/static import), so the unqualified VersionType symbol is not resolved in Groovy.
Suggested fix
import com.netgrif.application.engine.objects.petrinet.domain.I18nString
import com.netgrif.application.engine.objects.petrinet.domain.PetriNet
+import com.netgrif.application.engine.objects.petrinet.domain.VersionType
import com.netgrif.application.engine.objects.petrinet.domain.Transition📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import com.netgrif.application.engine.importer.service.Importer | |
| import com.netgrif.application.engine.objects.petrinet.domain.I18nString | |
| import com.netgrif.application.engine.objects.petrinet.domain.PetriNet | |
| import com.netgrif.application.engine.objects.petrinet.domain.Transition | |
| import com.netgrif.application.engine.objects.petrinet.domain.dataset.ActorField | |
| import com.netgrif.application.engine.objects.petrinet.domain.dataset.Field | |
| import com.netgrif.application.engine.objects.petrinet.domain.dataset.FieldType | |
| import com.netgrif.application.engine.objects.petrinet.domain.events.Event | |
| import com.netgrif.application.engine.objects.petrinet.domain.events.EventType | |
| import com.netgrif.application.engine.objects.petrinet.domain.roles.ProcessRole | |
| import com.netgrif.application.engine.objects.workflow.domain.Case | |
| import com.netgrif.application.engine.importer.service.Importer | |
| import com.netgrif.application.engine.objects.petrinet.domain.I18nString | |
| import com.netgrif.application.engine.objects.petrinet.domain.PetriNet | |
| import com.netgrif.application.engine.objects.petrinet.domain.VersionType | |
| import com.netgrif.application.engine.objects.petrinet.domain.Transition | |
| import com.netgrif.application.engine.objects.petrinet.domain.dataset.ActorField | |
| import com.netgrif.application.engine.objects.petrinet.domain.dataset.Field | |
| import com.netgrif.application.engine.objects.petrinet.domain.dataset.FieldType | |
| import com.netgrif.application.engine.objects.petrinet.domain.events.Event | |
| import com.netgrif.application.engine.objects.petrinet.domain.events.EventType | |
| import com.netgrif.application.engine.objects.petrinet.domain.roles.ProcessRole | |
| import com.netgrif.application.engine.objects.workflow.domain.Case |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/PetriNetMigrationHelper.groovy`
around lines 5 - 15, PetriNetMigrationHelper.groovy references the unqualified
symbol VersionType (e.g. VersionType.MAJOR) but lacks its import; add the
missing import statement for
com.netgrif.application.engine.objects.petrinet.domain.VersionType to the top
import block so the Groovy compiler can resolve VersionType when used in
PetriNetMigrationHelper (search for the VersionType.MAJOR usage to verify).
Comment on lines
+235
to
+242
| void addRoleToExistingTasks(ProcessRole role, PetriNet net, List<String> transitionIds, Map<String, Boolean> permissions) { | ||
| log.debug("Starting addRoleToExistingTasks for role: ${role.getName()}, net: ${net.identifier}, transitionIds: ${transitionIds}") | ||
| log.trace("Calling updateTasks to add role with permissions: ${permissions}") | ||
| updateTasks({ Task task -> | ||
| log.trace("Add role '${role.getName()}' with roleId=${role.getImportId()} to transitionId=${task.getTransitionId()} in task ${task.stringId}") | ||
| task.addRole(role.getStringId(), permissions) | ||
| }, QTask.task.transitionId.in(transitionIds) & QTask.task.processId.eq(net.getStringId())) | ||
| } |
There was a problem hiding this comment.
Propagate facade error policy in addRoleToExistingTasks.
This path always falls back to defaultErrorPolicy() because it calls updateTasks(...) without an explicit policy. Calls wrapped in MigrationHelper.withErrorPolicy(...) won’t honor the caller’s policy here.
Suggested fix
- void addRoleToExistingTasks(ProcessRole role, PetriNet net, List<String> transitionIds, Map<String, Boolean> permissions) {
+ void addRoleToExistingTasks(ProcessRole role, PetriNet net, List<String> transitionIds, Map<String, Boolean> permissions,
+ MigrationErrorPolicy errorPolicy = defaultErrorPolicy()) {
log.debug("Starting addRoleToExistingTasks for role: ${role.getName()}, net: ${net.identifier}, transitionIds: ${transitionIds}")
log.trace("Calling updateTasks to add role with permissions: ${permissions}")
updateTasks({ Task task ->
log.trace("Add role '${role.getName()}' with roleId=${role.getImportId()} to transitionId=${task.getTransitionId()} in task ${task.stringId}")
task.addRole(role.getStringId(), permissions)
- }, QTask.task.transitionId.in(transitionIds) & QTask.task.processId.eq(net.getStringId()))
+ }, QTask.task.transitionId.in(transitionIds) & QTask.task.processId.eq(net.getStringId()), errorPolicy)
}// application-engine/src/main/groovy/com/netgrif/application/engine/migration/MigrationHelper.groovy
- taskMigrationHelper.addRoleToExistingTasks(role, net, transitionIds, permissions)
+ taskMigrationHelper.addRoleToExistingTasks(role, net, transitionIds, permissions, getCurrentErrorPolicy())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/TaskMigrationHelper.groovy`
around lines 235 - 242, The addRoleToExistingTasks method currently calls
updateTasks without forwarding the caller's error policy, causing it to always
use defaultErrorPolicy; change the call in addRoleToExistingTasks to accept and
pass an explicit ErrorPolicy argument (propagate the active policy from
MigrationHelper.withErrorPolicy) so updateTasks receives the caller's policy
instead of falling back to defaultErrorPolicy(); update the updateTasks
invocation signature usage in addRoleToExistingTasks to include the policy
parameter (and ensure MigrationHelper.withErrorPolicy callers pass their policy
through) while keeping the existing role/permissions closure unchanged.
Comment on lines
+600
to
+606
| List<MigrationError> collectErrors(Closure migrationCode) { | ||
| clearErrors() | ||
| try { | ||
| migrationCode.call() | ||
| } finally { | ||
| return popErrors() | ||
| } |
There was a problem hiding this comment.
Do not return from finally in collectErrors.
The return in finally suppresses any exception from migrationCode.call(), including unexpected runtime failures.
Suggested fix
List<MigrationError> collectErrors(Closure migrationCode) {
clearErrors()
try {
migrationCode.call()
} finally {
- return popErrors()
+ // keep finally for cleanup only; do not return here
}
+ return popErrors()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| List<MigrationError> collectErrors(Closure migrationCode) { | |
| clearErrors() | |
| try { | |
| migrationCode.call() | |
| } finally { | |
| return popErrors() | |
| } | |
| List<MigrationError> collectErrors(Closure migrationCode) { | |
| clearErrors() | |
| try { | |
| migrationCode.call() | |
| } finally { | |
| // keep finally for cleanup only; do not return here | |
| } | |
| return popErrors() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/migration/MigrationHelper.groovy`
around lines 600 - 606, collectErrors currently returns from the finally block
which swallows exceptions from migrationCode.call(); change the method to avoid
returning inside finally: call clearErrors(), declare a List<MigrationError>
variable (e.g. errors), invoke migrationCode.call() inside try, in finally
assign errors = popErrors(), and after the try/finally return errors so any
exception thrown by migrationCode.call() will propagate while still ensuring
popErrors() runs (refer to collectErrors, migrationCode.call, clearErrors,
popErrors).
Comment on lines
+132
to
+134
| String getEntityType() { | ||
| return entityType | ||
| } |
There was a problem hiding this comment.
Return type mismatch: getter returns String but field is Class<?>.
The entityType field is declared as Class<?> (line 34) and stored as such (line 66), but getEntityType() declares its return type as String. While Groovy will implicitly convert the Class<?> to String via toString(), this is semantically incorrect and misleading for callers who expect a String but receive a stringified class name.
Proposed fix
Change the return type to match the field type:
- String getEntityType() {
+ Class<?> getEntityType() {
return entityType
}Alternatively, if the string representation is intentional, store it as String from the start by modifying the constructor:
MigrationError(LocalDateTime timestamp, String helper, String operation, Class<?> entityType, String entityId, String message, Throwable cause) {
this.timestamp = timestamp
this.helper = helper
this.operation = operation
- this.entityType = entityType
+ this.entityType = entityType?.simpleName ?: entityType?.toString()
this.entityId = entityId
this.message = message
this.cause = cause
}and update the field declaration:
- private Class<?> entityType
+ private String entityType📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| String getEntityType() { | |
| return entityType | |
| } | |
| Class<?> getEntityType() { | |
| return entityType | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/migration/model/MigrationError.groovy`
around lines 132 - 134, The getter getEntityType() currently declares return
type String but the field entityType is Class<?>; update the method signature to
"Class<?> getEntityType()" and return the entityType field unchanged so the
types match, i.e., change the return type to Class<?> in the getEntityType()
method (or if string semantics were intended, alternatively convert the field to
String and adjust any constructor/assignments to store a String instead).
Comment on lines
+100
to
+108
| static MigrationErrorPolicy throwAfterLimit(int maxErrors) { | ||
| if (maxErrors <= 0) { | ||
| throw new IllegalArgumentException("maxErrors must be > 0 for THROW_AFTER_LIMIT") | ||
| } | ||
| return new MigrationErrorPolicy( | ||
| mode: MigrationErrorHandlingMode.THROW_AFTER_LIMIT, | ||
| maxErrors: maxErrors | ||
| ) | ||
| } |
There was a problem hiding this comment.
Validation inconsistency between factory and runtime behavior.
The throwAfterLimit factory rejects maxErrors <= 0, but the rest of the framework allows it:
setMaxErrors(line 154) permitsmaxErrors >= 0MigrationProperties.ErrorPolicyjavadoc explicitly documents thatmaxErrors = 0means "no limit enforced"AbstractMigrationHelper.finishMigrationErrorPolicy(line 368) treatsTHROW_AFTER_LIMITwithmaxErrors <= 0as equivalent toTHROW_AFTER_PROCESSING
This prevents users from configuring THROW_AFTER_LIMIT mode with maxErrors = 0 via the factory, even though the documented and runtime semantics support it.
Proposed fix
Either relax the factory validation to match the documented behavior:
static MigrationErrorPolicy throwAfterLimit(int maxErrors) {
- if (maxErrors <= 0) {
- throw new IllegalArgumentException("maxErrors must be > 0 for THROW_AFTER_LIMIT")
+ if (maxErrors < 0) {
+ throw new IllegalArgumentException("maxErrors must be >= 0 for THROW_AFTER_LIMIT")
}
return new MigrationErrorPolicy(Or update the documentation and runtime logic to reject maxErrors = 0 for THROW_AFTER_LIMIT.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/migration/model/MigrationErrorPolicy.groovy`
around lines 100 - 108, The factory method throwAfterLimit currently rejects
maxErrors <= 0 which contradicts setMaxErrors, the
MigrationProperties.ErrorPolicy javadoc and
AbstractMigrationHelper.finishMigrationErrorPolicy; update throwAfterLimit to
accept maxErrors == 0 (i.e., only reject negative values) so THROW_AFTER_LIMIT
with maxErrors == 0 is allowed and handled consistently, ensuring behavior
aligns with MigrationErrorHandlingMode.THROW_AFTER_LIMIT and the logic in
AbstractMigrationHelper.finishMigrationErrorPolicy.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Implements NAE-2448
Dependencies
No new dependencies were introduced.
Third party dependencies
No new dependencies were introduced.
Blocking Pull requests
Depends on #448
How Has Been This Tested?
This was tested manually and with unit tests.
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
Tests