[NAE-2446] Migration for NAE v7 phase 1#448
Open
renczesstefan wants to merge 6 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.
WalkthroughThis PR introduces a comprehensive MongoDB migration framework for bulk document operations on Cases, Tasks, and Petri Nets. It includes configurable error handling policies, QueryDSL predicate support, pagination with cursor streaming, and Elasticsearch integration for case/task indexing. The framework provides a public facade ( ChangesMigration System Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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: 8
🤖 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 53-58: Update the field Javadoc for migrationErrors in
AbstractMigrationHelper to reflect that it is a List<MigrationError> (not a
map): describe that migrationErrors is a collection holding MigrationError
entries encountered during migration, each representing an individual migration
failure; remove references to keys, string identifiers, and lists-per-key, and
adjust any thread-safety wording to match the actual implementation (e.g., note
whether the list is synchronized or not) so the comment accurately matches the
migrationErrors field.
- Around line 192-194: The Javadoc block in AbstractMigrationHelper is malformed
because the first line of the comment lacks the leading "*" and runs into the
next line; update the Javadoc for the method (the comment starting at the block
above the iterator/bulk-operation method in class AbstractMigrationHelper) by
adding the missing "*" at the start of the first line so each line begins with
"*" and the comment renders correctly as a standard Javadoc block.
- Around line 226-266: The outer catch in the iterate loop is re-invoking
handleMigrationError for exceptions that have already been routed (and possibly
re-thrown) by prepareOperations or effectiveProcessOperations, which causes
double-handling and masks original errors; modify the catch block around the
cursor iteration so it checks for MigrationErrorException (or a marker that
handleMigrationError already threw) and rethrows it immediately without calling
handleMigrationError again, and only call handleMigrationError for other
unexpected exceptions (so keep references to prepareOperations,
effectiveProcessOperations, handleMigrationError, MigrationErrorException and
finishMigrationErrorPolicy to locate the logic).
- Around line 317-321: The THROW_AFTER_LIMIT branch incorrectly throws
immediately when policy.maxErrors == 0 and bypasses throwOriginal; change the
logic in the MigrationErrorHandlingMode.THROW_AFTER_LIMIT case to first treat
maxErrors==0 as "no limit" (i.e. only apply the limit when policy.maxErrors > 0)
and when the limit is reached call the central throwError(...) path instead of
directly throwing new MigrationErrorException so throwOriginal and cacheErrors
behavior are honored; specifically update the condition around
getErrors().size() and policy.maxErrors to require policy.maxErrors > 0 and
route handling through throwError(...) (respecting cacheErrors and existing
error caching) rather than constructing/throwing MigrationErrorException inline.
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/CaseMigrationHelper.groovy`:
- Around line 448-450: The error logging and error handler call in removeCase
are mislabelled: update the message string and the handleMigrationError
operation parameter to reference "removeCase" (or "removeCase: delete case"
phrasing) instead of "migratePetriNet"; specifically modify the local variable
message and the handleMigrationError(errorPolicy, "migratePetriNet", type,
useCase.stringId, message) invocation in the removeCase method so both
accurately reflect removeCase while keeping errorPolicy, type and
useCase.stringId unchanged.
In
`@application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/MigrationProperties.java`:
- Around line 143-147: The docs say maxErrors == 0 means "no limit" but the
handler throws immediately because it checks getErrors().size() >=
policy.maxErrors; update AbstractMigrationHelper.handleMigrationError to treat 0
as unlimited by changing the throw condition to only apply when policy.maxErrors
> 0 (e.g., require policy.maxErrors > 0 && getErrors().size() >=
policy.maxErrors), keeping existing behavior for positive limits and preserving
references to getErrors(), policy.maxErrors, and the THROW_AFTER_LIMIT policy.
In
`@application-engine/src/main/java/com/netgrif/application/engine/utils/MongodbUtils.java`:
- Around line 9-15: MongodbUtils currently exposes only the static method
toQuery but is instantiable; make the intent explicit by adding a private
constructor to MongodbUtils (e.g., private MongodbUtils() { throw new
AssertionError("No instances."); } or simply private MongodbUtils() {} ) so
callers cannot instantiate the class; update class MongodbUtils to include this
private constructor while leaving the static toQuery(MongoTemplate, Class<T>,
Predicate...) unchanged.
In
`@application-engine/src/test/groovy/com/netgrif/application/engine/migration/MigrationTest.groovy`:
- Around line 69-104: The beforeEach setup resets the DBs but not the
centralized migration error cache, causing flaky tests; add a call to clear the
error cache (e.g., migrationHelper.clearErrors() or the appropriate
clearErrors() method on the migration helper) immediately after
testHelper.truncateDbs() inside the beforeEach method so the shared error state
is reset before importing nets and creating cases, ensuring
migrationHelper.hasErrors() checks in other tests are not impacted by prior
runs.
🪄 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: 4ceca0d7-f7c3-4af9-9ed4-eec4c19742fd
📒 Files selected for processing (16)
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/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/ElasticSearchTest.groovy
- application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovy
- 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.
renczesstefan
changed the title
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-2446
Dependencies
No new dependencies were introduced
Third party dependencies
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
This was tested manually and with unit tests.
Test Configuration
Checklist:
Summary by CodeRabbit
New Features