[NAE-2439] Implement new filter data types#446
Open
Retoocs wants to merge 4 commits into
Open
Conversation
- introduce new filter fields - remove old filter field
- remove transactional in Importer - handle deprecated filter data type when importing process - remove use of deleted filter attributes
- implement FilterTest
- force component use for dedicated filter fields - fix cloning filter fields
WalkthroughThis PR decomposes a generic ChangesFilter Field Architecture Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
added
new feature
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/main/java/com/netgrif/application/engine/menu/domain/FilterBody.java (1)
18-23:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRemoving these fields also removes accessors that current callers still use.
This change drops the Lombok-generated
setAllowedNets(...),setIcon(...), andsetMetadata(...)methods, butActionDelegate.groovystill calls them in the provided context (for example around Lines 1619-1622, 2036-2039, and 2466-2469). Those filter/menu creation paths will now fail at runtime in Groovy with a missing-method error unless the callers are migrated in the same PR or compatibility setters are kept temporarily.🤖 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 `@src/main/java/com/netgrif/application/engine/menu/domain/FilterBody.java` around lines 18 - 23, The removal of fields from FilterBody removed Lombok-generated setters (setAllowedNets, setIcon, setMetadata) that ActionDelegate.groovy still invokes; restore binary compatibility by reintroducing those setters on FilterBody (either add back the removed fields with appropriate types or add explicit public setter methods named setAllowedNets, setIcon, setMetadata that accept the same parameter types as before) so ActionDelegate.groovy calls (around the spots that used those setters) continue to work without runtime missing-method errors.
🤖 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
`@src/main/java/com/netgrif/application/engine/importer/service/FieldFactory.java`:
- Around line 251-254: The code in FieldFactory currently maps the deprecated
FILTER token directly to buildCaseFilterField, which will misclassify legacy
filters; update the FILTER branch in the switch inside FieldFactory to inspect
the incoming legacy definition (e.g., a "filterType" discriminator or other
distinguishing attributes on the data object) and route to the correct builder
(e.g., buildCaseFilterField or the task/process-specific builder), and if the
discriminator is missing or ambiguous, fail fast by throwing a descriptive
exception rather than silently coercing; reference the FILTER and CASE_FILTER
enum values and the buildCaseFilterField method when implementing the
discriminator check and error path.
In `@src/main/java/com/netgrif/application/engine/importer/service/Importer.java`:
- Line 243: The method signature change to protected void
resolveUserRef(CaseActorRef userRef) and related uses reference types
(CaseActorRef, ActorRef) that don't exist in
com.netgrif.application.engine.importer.model in this branch; revert the
signature and any other changed method/type usages to the existing importer
model types (restore the original parameter/type declarations used before the
PR) or update imports to the regenerated importer model only if that regenerated
model is landed in this stack; specifically update resolveUserRef and any
methods referencing CaseActorRef/ActorRef to use the existing model classes
present in the branch (or postpone these signature changes until the new
importer model is merged).
In
`@src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy`:
- Line 40: The test class FilterImportExportTest has been globally disabled with
`@Disabled` because it references removed DataField properties filterMetadata and
allowedNets; add a tracking reference so we don’t lose the TODO — insert a
concise TODO/JIRA comment next to the `@Disabled` annotation or at the top of the
class mentioning the ticket (e.g. TODO: RE-ENABLE after migrating to new filter
field architecture — JIRA-XXXX) and note which symbols need updating
(filterMetadata, allowedNets on DataField) so the future change and re-enabling
steps are clear.
In `@src/test/groovy/com/netgrif/application/engine/filters/FilterTest.groovy`:
- Line 61: The FileInputStream created for PROCESS_PATH when calling
petriNetService.importPetriNet(...) is never closed and may leak; wrap the
stream creation and the import call in a try-with-resources (or use Groovy's
withCloseable) so the FileInputStream is automatically closed after use.
Specifically, ensure the FileInputStream used to set PetriNet testProcess via
petriNetService.importPetriNet(new FileInputStream(PROCESS_PATH),
VersionType.MAJOR, superCreator.loggedSuper).getNet() is created inside a
try(resource) block (or passed through withCloseable) so the stream is closed
even on exceptions.
---
Outside diff comments:
In `@src/main/java/com/netgrif/application/engine/menu/domain/FilterBody.java`:
- Around line 18-23: The removal of fields from FilterBody removed
Lombok-generated setters (setAllowedNets, setIcon, setMetadata) that
ActionDelegate.groovy still invokes; restore binary compatibility by
reintroducing those setters on FilterBody (either add back the removed fields
with appropriate types or add explicit public setter methods named
setAllowedNets, setIcon, setMetadata that accept the same parameter types as
before) so ActionDelegate.groovy calls (around the spots that used those
setters) continue to work without runtime missing-method errors.
🪄 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: 9b66e756-f883-4739-bd3b-ff0706ec6d0e
📒 Files selected for processing (24)
src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/CaseFilterField.groovysrc/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FieldType.groovysrc/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FilterField.groovysrc/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/ProcessFilterField.groovysrc/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/TaskFilterField.groovysrc/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovysrc/main/java/com/netgrif/application/engine/importer/service/FieldFactory.javasrc/main/java/com/netgrif/application/engine/importer/service/Importer.javasrc/main/java/com/netgrif/application/engine/menu/domain/FilterBody.javasrc/main/java/com/netgrif/application/engine/menu/services/MenuItemService.javasrc/main/java/com/netgrif/application/engine/workflow/domain/Case.javasrc/main/java/com/netgrif/application/engine/workflow/domain/DataField.javasrc/main/java/com/netgrif/application/engine/workflow/service/DataService.javasrc/main/java/com/netgrif/application/engine/workflow/service/FilterImportExportService.javasrc/main/java/com/netgrif/application/engine/workflow/service/interfaces/IFilterImportExportService.javasrc/main/java/com/netgrif/application/engine/workflow/web/responsebodies/LocalisedCaseFilterField.javasrc/main/java/com/netgrif/application/engine/workflow/web/responsebodies/LocalisedFieldFactory.javasrc/main/java/com/netgrif/application/engine/workflow/web/responsebodies/LocalisedFilterField.javasrc/main/java/com/netgrif/application/engine/workflow/web/responsebodies/LocalisedProcessFilterField.javasrc/main/java/com/netgrif/application/engine/workflow/web/responsebodies/LocalisedTaskFilterField.javasrc/main/resources/petriNets/engine-processes/import_filters.xmlsrc/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovysrc/test/groovy/com/netgrif/application/engine/filters/FilterTest.groovysrc/test/resources/petriNets/filter_test.xml
💤 Files with no reviewable changes (8)
- src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FilterField.groovy
- src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IFilterImportExportService.java
- src/main/java/com/netgrif/application/engine/workflow/web/responsebodies/LocalisedFilterField.java
- src/main/resources/petriNets/engine-processes/import_filters.xml
- src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java
- src/main/java/com/netgrif/application/engine/workflow/service/FilterImportExportService.java
- src/main/java/com/netgrif/application/engine/workflow/domain/DataField.java
- src/main/java/com/netgrif/application/engine/workflow/domain/Case.java
Comment on lines
+251
to
+254
| case FILTER: // "FILTER" is deprecated | ||
| case CASE_FILTER: | ||
| field = buildCaseFilterField(data); | ||
| break; |
There was a problem hiding this comment.
Don't coerce every legacy FILTER import into a case filter.
Lines 251-254 currently map the deprecated FILTER type straight to CaseFilterField, so any legacy task/process filter imported through the old generic type will silently change behavior during migration. Please infer the dedicated filter field type from the legacy definition before constructing it, or fail fast when that information is unavailable.
🤖 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
`@src/main/java/com/netgrif/application/engine/importer/service/FieldFactory.java`
around lines 251 - 254, The code in FieldFactory currently maps the deprecated
FILTER token directly to buildCaseFilterField, which will misclassify legacy
filters; update the FILTER branch in the switch inside FieldFactory to inspect
the incoming legacy definition (e.g., a "filterType" discriminator or other
distinguishing attributes on the data object) and route to the correct builder
(e.g., buildCaseFilterField or the task/process-specific builder), and if the
discriminator is missing or ambiguous, fail fast by throwing a descriptive
exception rather than silently coercing; reference the FILTER and CASE_FILTER
enum values and the buildCaseFilterField method when implementing the
discriminator check and error path.
|
|
||
| @Transactional | ||
| protected void resolveUserRef(CaseUserRef userRef) { | ||
| protected void resolveUserRef(CaseActorRef userRef) { |
There was a problem hiding this comment.
Use importer model types that actually exist in this branch.
These signature changes are a hard build break: the current com.netgrif.application.engine.importer.model package does not contain CaseActorRef or ActorRef, and the pipeline is already failing on both lines. If this PR depends on the Petriflow schema change, the regenerated importer model needs to land in the same stack before merge; otherwise keep these methods typed to the existing model classes.
Also applies to: 732-732
🧰 Tools
🪛 GitHub Actions: Pull Request / 1_Build.txt
[error] 243-243: Compilation error (cannot find symbol). Missing class 'CaseActorRef' referenced in com.netgrif.application.engine.importer.service.Importer.
🪛 GitHub Actions: Pull Request / Build
[error] 243-243: Compilation error in Maven compiler plugin: cannot find symbol. Symbol: class CaseActorRef, location: class com.netgrif.application.engine.importer.service.Importer.
🤖 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 `@src/main/java/com/netgrif/application/engine/importer/service/Importer.java`
at line 243, The method signature change to protected void
resolveUserRef(CaseActorRef userRef) and related uses reference types
(CaseActorRef, ActorRef) that don't exist in
com.netgrif.application.engine.importer.model in this branch; revert the
signature and any other changed method/type usages to the existing importer
model types (restore the original parameter/type declarations used before the
PR) or update imports to the regenerated importer model only if that regenerated
model is landed in this stack; specifically update resolveUserRef and any
methods referencing CaseActorRef/ActorRef to use the existing model classes
present in the branch (or postpone these signature changes until the new
importer model is merged).
| import javax.xml.validation.Validator | ||
| import java.util.stream.Collectors | ||
|
|
||
| @Disabled |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add a tracking reference for re-enabling this test suite.
The entire test class is now disabled because it references removed functionality (Lines 185-186 reference filterMetadata and allowedNets on DataField). To track this technical debt, consider adding a TODO comment or JIRA ticket reference explaining when and how this test suite will be updated for the new filter field architecture.
📝 Suggested TODO comment
-@Disabled
+@Disabled("TODO(NAE-XXXX): Update filter import/export tests for new specialized filter field types")
`@ExtendWith`(SpringExtension.class)📝 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
| @Disabled | |
| `@Disabled`("TODO(NAE-XXXX): Update filter import/export tests for new specialized filter field types") | |
| `@ExtendWith`(SpringExtension.class) |
🤖 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
`@src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy`
at line 40, The test class FilterImportExportTest has been globally disabled
with `@Disabled` because it references removed DataField properties filterMetadata
and allowedNets; add a tracking reference so we don’t lose the TODO — insert a
concise TODO/JIRA comment next to the `@Disabled` annotation or at the top of the
class mentioning the ticket (e.g. TODO: RE-ENABLE after migrating to new filter
field architecture — JIRA-XXXX) and note which symbols need updating
(filterMetadata, allowedNets on DataField) so the future change and re-enabling
steps are clear.
|
|
||
| @Test | ||
| void filterTest() { | ||
| PetriNet testProcess = petriNetService.importPetriNet(new FileInputStream(PROCESS_PATH), VersionType.MAJOR, superCreator.loggedSuper).getNet() |
There was a problem hiding this comment.
Close the FileInputStream to prevent resource leak.
The FileInputStream created on Line 61 is never explicitly closed, which can lead to a resource leak.
💧 Proposed fix to close the resource
Groovy supports try-with-resources syntax. Wrap the file operation:
- PetriNet testProcess = petriNetService.importPetriNet(new FileInputStream(PROCESS_PATH), VersionType.MAJOR, superCreator.loggedSuper).getNet()
+ PetriNet testProcess
+ new FileInputStream(PROCESS_PATH).withCloseable { stream ->
+ testProcess = petriNetService.importPetriNet(stream, VersionType.MAJOR, superCreator.loggedSuper).getNet()
+ }📝 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
| PetriNet testProcess = petriNetService.importPetriNet(new FileInputStream(PROCESS_PATH), VersionType.MAJOR, superCreator.loggedSuper).getNet() | |
| PetriNet testProcess | |
| new FileInputStream(PROCESS_PATH).withCloseable { stream -> | |
| testProcess = petriNetService.importPetriNet(stream, VersionType.MAJOR, superCreator.loggedSuper).getNet() | |
| } |
🤖 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 `@src/test/groovy/com/netgrif/application/engine/filters/FilterTest.groovy` at
line 61, The FileInputStream created for PROCESS_PATH when calling
petriNetService.importPetriNet(...) is never closed and may leak; wrap the
stream creation and the import call in a try-with-resources (or use Groovy's
withCloseable) so the FileInputStream is automatically closed after use.
Specifically, ensure the FileInputStream used to set PetriNet testProcess via
petriNetService.importPetriNet(new FileInputStream(PROCESS_PATH),
VersionType.MAJOR, superCreator.loggedSuper).getNet() is created inside a
try(resource) block (or passed through withCloseable) so the stream is closed
even on exceptions.
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
Introduced new filter fields. Removed old
FilterField. New filter fields are more simpleImplements NAE-2439
Dependencies
No new dependencies were introduced
Third party dependencies
No new dependencies were introduced
Blocking Pull requests
Depends on petriflow/petriflow#30
How Has Been This Tested?
Manually and by unit test
FilterTest.groovy
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
Improvements
Tests