[NAE-2443] PFQL support#450
Open
Retoocs wants to merge 20 commits into
Open
Conversation
- update ImportHelper to support params map - add PFQL from v8 task NAE-1997 - add PFQL tests - add antlr4 runtime and build plugin to pom
- add missing PFQL grammar
- add separate searchCase and searchCases functions
# Conflicts: # pom.xml
- start code encapsulation of SearchService - add prefix Legacy to old search services
- finish code encapsulation of SearchService
- implement not equals operator
- allow no trailing white space after operator in
- introduce action delegate PFQL methods - update validation in search services
- add user search API to ActionDelegate - implement authorization for process search - implement authorization for task search - implement authorization for case search - fix initialization of pageable in query - implement UserSearchService methods
- fix validation
- implement QueryLangActionTest - move some tests to java package
- implement tests for search services - remove redundant default pageable from IResourceSearchService
- add javadoc for ActionDelegate search methods - improve lowcode experience for action delegate dedicated search methods - add javadoc for IResourceSearchService default methods - improve QueryLangActionTest
- rework assertions in QueryLangTest
# Conflicts: # src/main/java/com/netgrif/application/engine/workflow/service/LegacyTaskSearchService.java
- after merge fix
WalkthroughThis PR implements a complete PFQL (Petri Flow Query Language) query system enabling declarative searches across cases, processes, tasks, and users. It defines an ANTLR4 grammar, evaluates queries into MongoDB predicates and Elasticsearch Lucene syntax, routes execution through resource-specific services, and integrates dual-backend search into core workflows with permission filtering. ChangesPFQL Query Language Search System
🎯 4 (Complex) | ⏱️ ~60 minutes new feature, improvement, breaking change, Large, Medium |
coderabbitai
Bot
added
improvement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
♻️ Duplicate comments (3)
src/main/java/com/netgrif/application/engine/pfql/service/QueryLangExplainEvaluator.java (3)
471-481:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix unconditional addition of sort order when attribute is invalid.
Lines 476-479 have the same logic error as in
exitCaseSorting: whenpropis null, both an error message and a malformed sort order are added.🐛 Proposed fix for sort order logic
if (prop == null) { sortOrders.add("Invalid attribute: " + attrOrd.processAttribute().getText()); + return; } sortOrders.add("attribute: " + prop + ", ordering: " + dir);🤖 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/pfql/service/QueryLangExplainEvaluator.java` around lines 471 - 481, exitProcessSorting incorrectly always adds a sort order even when the mapped property is null; update the loop in exitProcessSorting to only add the "attribute: ... ordering: ..." entry to sortOrders when prop is non-null (i.e. after checking processAttrToSortPropMapping for attrOrd.processAttribute()), and keep adding the error message ("Invalid attribute: ...") when prop is null; reference the method exitProcessSorting, the loop over ctx.processAttributeOrdering(), the variables attrOrd, prop, processAttrToSortPropMapping, and the sortOrders collection to locate and fix the logic.
495-505:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix unconditional addition of sort order when attribute is invalid.
Lines 500-503 have the same logic error: when
propis null, both an error message and a malformed sort order are added.🐛 Proposed fix for sort order logic
if (prop == null) { sortOrders.add("Invalid attribute: " + attrOrd.userAttribute().getText()); + return; } sortOrders.add("attribute: " + prop + ", ordering: " + dir);🤖 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/pfql/service/QueryLangExplainEvaluator.java` around lines 495 - 505, In QueryLangExplainEvaluator.exitUserSorting, attrOrd logic currently adds both an error and a malformed sort order when prop is null; change the flow so you look up prop from userAttrToSortPropMapping, and if prop is null add only the error to sortOrders ("Invalid attribute: ...") and skip adding the "attribute: ..., ordering: ..." entry (i.e. continue/return from the lambda), otherwise compute dir and add the valid sortOrders entry; update the attrOrd lambda in exitUserSorting to use an if/else (or guard clause) around the sortOrders.add for the valid prop.
483-493:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix unconditional addition of sort order when attribute is invalid.
Lines 488-491 have the same logic error: when
propis null, both an error message and a malformed sort order are added.🐛 Proposed fix for sort order logic
if (prop == null) { sortOrders.add("Invalid attribute: " + attrOrd.taskAttribute().getText()); + return; } sortOrders.add("attribute: " + prop + ", ordering: " + dir);🤖 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/pfql/service/QueryLangExplainEvaluator.java` around lines 483 - 493, In exitTaskSorting in QueryLangExplainEvaluator, avoid unconditionally adding a malformed sort order when the task attribute is invalid: after computing String prop = taskAttrToSortPropMapping.get(...), if prop is null only add the error message to sortOrders and skip adding the "attribute: ..., ordering: ..." entry (i.e., continue or use an else branch); keep the existing ordering calculation (Sort.Direction dir ...) and attrOrd references but ensure the sortOrders.add for the valid sort entry executes only when prop != null.
🤖 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/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy`:
- Around line 3267-3270: The wrapper methods (searchCases, searchTasks,
searchProcesses, searchUsers) redundantly call SearchUtils.ensureStartsWith...
before delegating to their corresponding paged* methods which already perform
the same normalization; remove the extra ensureStartsWith... call from each
wrapper so they simply delegate and return pagedSearchCases(query).content (and
analogous for pagedSearchTasks/pagedSearchProcesses/pagedSearchUsers), ensuring
you only keep the normalization in the paged* methods (functions: searchCases,
pagedSearchCases, searchTasks, pagedSearchTasks, searchProcesses,
pagedSearchProcesses, searchUsers, pagedSearchUsers).
- Around line 219-232: Fields in ActionDelegate (searchService,
caseSearchService, taskSearchService, processSearchService, userSearchService)
use concrete implementation types for most services; change them to the
corresponding service interfaces (e.g., ICaseSearchService, ITaskSearchService,
IProcessSearchService, IUserSearchService or whatever existing interface names
match each implementation) and update imports so the `@Autowired` fields are typed
to interfaces for consistency/testability; if an interface does not yet exist
for a service, create a minimal service interface and have the implementation
implement it, then inject that interface instead.
In
`@src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java`:
- Around line 608-614: The current getProcessRolesPredicate builds a
BooleanBuilder from user.getProcessRoles() and returns an empty builder when
roles are empty, causing null to propagate and bypass permission filtering;
update getProcessRolesPredicate so that if user.getProcessRoles() is empty it
returns a predicate that matches nothing (consistent with
getProcessRolesCriteria's semantics) instead of an empty/null value — e.g.
explicitly return a false/none predicate (use the same pattern or helper as
getProcessRolesCriteria) otherwise build the OR of
QPetriNet.petriNet.permissions.containsKey(role) for each role.
In
`@src/main/java/com/netgrif/application/engine/pfql/domain/antlr4/QueryLang.g4`:
- Around line 244-249: The current selector rules (dataValue, dataOptions,
places, tasksState, tasksUserId and the similar rules in the 264–329 range) bind
fieldId/placeId/taskId to JAVA_ID only, causing identifiers that lex as
reserved-word tokens to be rejected; add a parser-level alternative that accepts
either JAVA_ID or the keyword tokens (create a new rule like selectorId or
identifierSelector that is defined as JAVA_ID | <list of keyword tokens you need
to allow>) and replace occurrences of fieldId=JAVA_ID / placeId=JAVA_ID /
taskId=JAVA_ID with fieldId=selectorId (and the analogous names) in dataValue,
dataOptions, places, tasksState, tasksUserId and the other selector rules in
264–329 so keyword-token identifiers are parsed as valid IDs.
In
`@src/main/java/com/netgrif/application/engine/pfql/service/processresource/ProcessSearchService.java`:
- Around line 73-76: The code currently always runs MongoDB queries (see the
debug/trace logs around evaluator.getFullMongoQuery()) and ignores the
searchWithElastic flag; update ProcessSearchService to check the
evaluator/searchWithElastic flag at the start of each search path
(single-process, list, pageable, and count paths where you log/execute Mongo
queries) and immediately fail fast when Elasticsearch is requested by throwing
an explicit exception (e.g., UnsupportedOperationException or
IllegalStateException) with a clear message that Elasticsearch-based search is
not yet implemented; reference evaluator.isSearchWithElastic() or the
searchWithElastic parameter and the methods that call
evaluator.getFullMongoQuery() so callers get a deterministic failure instead of
silently executing MongoDB.
In
`@src/main/java/com/netgrif/application/engine/pfql/service/QueryLangErrorListener.java`:
- Around line 20-35: The underlineError method reads lines[line - 1] without
validating that (line - 1) is within bounds, which can throw
ArrayIndexOutOfBoundsException for empty/invalid input; update underlineError to
check the tokens input string and the computed lines array length and ensure 1
<= line <= lines.length before accessing lines[line - 1], and handle
out-of-range cases by returning a safe message or using a fallback (e.g., the
full input or a placeholder errorLine) so the method still builds
underlineErrorMsg without throwing; reference the underlineError method, the
local variables lines, line, errorLine, and offendingToken to locate where to
add these checks.
In
`@src/main/java/com/netgrif/application/engine/pfql/service/QueryLangEvaluator.java`:
- Around line 809-818: The commented-out implementations for task query features
are leaving unfinished behavior; in the methods exitStateComparison,
exitLastAssignedDateComparison, and exitLastFinishedDateComparison replace the
commented blocks with explicit TODO/FIXME comments that reference a created
tracking issue (or add the issue ID/URL) and make the unimplemented status
explicit (for example add a clear TODO note and either throw an
UnsupportedOperationException or log/raise a clear runtime error to fail fast)
so the missing implementation is discoverable and tracked for future work.
In
`@src/main/java/com/netgrif/application/engine/pfql/service/QueryLangExplainEvaluator.java`:
- Around line 444-469: In exitCaseSorting, the code unconditionally adds a
sortOrders entry even when prop is null; change the logic so that after
computing prop (using caseAttrToSortPropElasticMapping /
caseAttrToSortPropMapping and handling places/tasks...), you only add the
"attribute: ... ordering: ..." string when prop != null; otherwise keep adding
the error "Invalid attribute: ..." to sortOrders and do not add the second
entry. Update the block handling attrOrd in exitCaseSorting (references:
exitCaseSorting, sortOrders, caseAttrToSortPropElasticMapping,
caseAttrToSortPropMapping, attrOrd.caseAttribute()) accordingly.
In
`@src/main/java/com/netgrif/application/engine/pfql/service/SearchService.java`:
- Around line 54-55: The code in SearchService dereferences the result of
serviceRegistry.get(evaluator.getResourceType()) (used where Object result = ...
and in similar spots around the methods handling single/multiple searches)
without null-checking, which causes an NPE for unregistered QueryTypes; update
each location that calls serviceRegistry.get(evaluator.getResourceType()) to
validate the returned IResourceSearchService<?> (e.g., in the method containing
the existing Object result = ... line and the other two occurrences) and if null
throw a clear unchecked exception (IllegalArgumentException or
IllegalStateException) with a message including the evaluator.getResourceType()
(and any relevant evaluator id/context) so callers fail fast with a helpful
error instead of NPE.
In
`@src/main/java/com/netgrif/application/engine/pfql/service/utils/SearchUtils.java`:
- Around line 281-327: The version parsing in buildVersionPredicate currently
splits versionString and parses parts without validation, causing
ArrayIndexOutOfBoundsException/NumberFormatException for malformed input; update
buildVersionPredicate to first validate that versionString splits into exactly 3
segments and that each segment is a valid non-negative integer (e.g. with a
numeric check or try-parse) and throw a clear IllegalArgumentException if
validation fails, then proceed to parse and build the Predicate as before; apply
the same validation and error handling to buildVersionPredicateInList and
buildVersionPredicateInRange so all version-parsing entry points consistently
validate input before converting to long.
- Around line 122-144: The two methods toDateTime and toDate swallow
DateTimeParseException and rethrow a new IllegalArgumentException without
preserving the original exception; change the inner catch blocks so they capture
the caught DateTimeParseException (instead of using ignored) and pass it as the
cause when throwing the IllegalArgumentException (e.g., throw new
IllegalArgumentException("Invalid date/datetime format", e)) so the original
stack trace and message are preserved for debugging; update both toDateTime and
toDate accordingly.
In
`@src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java`:
- Around line 771-778: The call to loadUsers in TaskService.searchOne is
redundant because search(com.querydsl.core.types.Predicate) already calls
loadUsers(tasks) before returning; remove the extra loadUsers(tasks) line from
searchOne (which calls search(predicate, PageRequest.of(0, 1))) and simply
return the first element when tasks.getTotalElements() > 0, preserving the
existing null return when no tasks are found.
In
`@src/test/groovy/com/netgrif/application/engine/pfql/QueryLangActionTest.groovy`:
- Around line 73-76: The tests currently assert concrete class-name strings
returned via RESULT_CLASS_FIELD_ID (e.g.,
"Collections$UnmodifiableRandomAccessList"); change these to type-safe
assertions by retrieving the actual object from RESULT_FIELD_ID (use
testCase.getFieldValue(RESULT_FIELD_ID)) and assert its runtime type/interface
(e.g., assertTrue(result instanceof List), assertTrue(result instanceof Page) or
assertTrue(result instanceof YourDomainClass)) instead of comparing string class
names; update each occurrence (including references to RESULT_CLASS_FIELD_ID and
testCase.getFieldValue calls and assertEquals) to use assertTrue/assertThat with
instanceof checks.
In
`@src/test/java/com/netgrif/application/engine/pfql/CaseSearchServiceTest.java`:
- Around line 68-69: The test CaseSearchServiceTest uses Thread.sleep(2000)
before calling caseSearchService.searchOne which causes flakiness; replace these
fixed sleeps (lines around the calls to caseSearchService.searchOne) with a
bounded polling loop that repeatedly invokes caseSearchService.searchOne (or the
relevant search method) until the expected result is non-null or a deadline
(e.g., 5–10s) is reached, sleeping briefly between attempts (e.g., 100–200ms)
and failing the test if the timeout elapses; apply the same change to the other
occurrences noted (around lines 91–92, 115–116, 135–136).
In
`@src/test/java/com/netgrif/application/engine/pfql/utils/SearchTestUtils.java`:
- Around line 35-44: The current compareById(List<T> actual, List<T> expected,
Function<T, String> getId) only checks one-way containment and can pass when
actual has extra items; change it to verify both sides match exactly by mapping
actual and expected to Sets of IDs using getId and asserting equality (or assert
that actualIdsSet.equals(expectedIdsSet)), so the test fails if there are any
unexpected or missing IDs.
---
Duplicate comments:
In
`@src/main/java/com/netgrif/application/engine/pfql/service/QueryLangExplainEvaluator.java`:
- Around line 471-481: exitProcessSorting incorrectly always adds a sort order
even when the mapped property is null; update the loop in exitProcessSorting to
only add the "attribute: ... ordering: ..." entry to sortOrders when prop is
non-null (i.e. after checking processAttrToSortPropMapping for
attrOrd.processAttribute()), and keep adding the error message ("Invalid
attribute: ...") when prop is null; reference the method exitProcessSorting, the
loop over ctx.processAttributeOrdering(), the variables attrOrd, prop,
processAttrToSortPropMapping, and the sortOrders collection to locate and fix
the logic.
- Around line 495-505: In QueryLangExplainEvaluator.exitUserSorting, attrOrd
logic currently adds both an error and a malformed sort order when prop is null;
change the flow so you look up prop from userAttrToSortPropMapping, and if prop
is null add only the error to sortOrders ("Invalid attribute: ...") and skip
adding the "attribute: ..., ordering: ..." entry (i.e. continue/return from the
lambda), otherwise compute dir and add the valid sortOrders entry; update the
attrOrd lambda in exitUserSorting to use an if/else (or guard clause) around the
sortOrders.add for the valid prop.
- Around line 483-493: In exitTaskSorting in QueryLangExplainEvaluator, avoid
unconditionally adding a malformed sort order when the task attribute is
invalid: after computing String prop = taskAttrToSortPropMapping.get(...), if
prop is null only add the error message to sortOrders and skip adding the
"attribute: ..., ordering: ..." entry (i.e., continue or use an else branch);
keep the existing ordering calculation (Sort.Direction dir ...) and attrOrd
references but ensure the sortOrders.add for the valid sort entry executes only
when prop != null.
🪄 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: 1c08809e-97ab-4b0a-967e-c1b0147779a1
📒 Files selected for processing (41)
pom.xmlsrc/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovysrc/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovysrc/main/java/com/netgrif/application/engine/auth/service/AbstractUserService.javasrc/main/java/com/netgrif/application/engine/auth/service/interfaces/IUserService.javasrc/main/java/com/netgrif/application/engine/elastic/web/ElasticController.javasrc/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javasrc/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.javasrc/main/java/com/netgrif/application/engine/pfql/domain/antlr4/QueryLang.g4src/main/java/com/netgrif/application/engine/pfql/domain/enums/ComparisonType.javasrc/main/java/com/netgrif/application/engine/pfql/domain/enums/QueryType.javasrc/main/java/com/netgrif/application/engine/pfql/service/IResourceSearchService.javasrc/main/java/com/netgrif/application/engine/pfql/service/ISearchService.javasrc/main/java/com/netgrif/application/engine/pfql/service/QueryLangErrorListener.javasrc/main/java/com/netgrif/application/engine/pfql/service/QueryLangEvaluator.javasrc/main/java/com/netgrif/application/engine/pfql/service/QueryLangExplainEvaluator.javasrc/main/java/com/netgrif/application/engine/pfql/service/SearchService.javasrc/main/java/com/netgrif/application/engine/pfql/service/caseresource/CaseSearchService.javasrc/main/java/com/netgrif/application/engine/pfql/service/processresource/ProcessSearchService.javasrc/main/java/com/netgrif/application/engine/pfql/service/taskresource/TaskSearchService.javasrc/main/java/com/netgrif/application/engine/pfql/service/userresource/UserSearchService.javasrc/main/java/com/netgrif/application/engine/pfql/service/utils/QueryLangTreeNode.javasrc/main/java/com/netgrif/application/engine/pfql/service/utils/SearchUtils.javasrc/main/java/com/netgrif/application/engine/workflow/service/LegacyCaseSearchService.javasrc/main/java/com/netgrif/application/engine/workflow/service/LegacyTaskSearchService.javasrc/main/java/com/netgrif/application/engine/workflow/service/TaskService.javasrc/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.javasrc/main/java/com/netgrif/application/engine/workflow/service/interfaces/ITaskService.javasrc/main/java/com/netgrif/application/engine/workflow/service/interfaces/IWorkflowService.javasrc/test/groovy/com/netgrif/application/engine/pfql/QueryLangActionTest.groovysrc/test/groovy/com/netgrif/application/engine/workflow/CaseSearchTest.groovysrc/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovysrc/test/java/com/netgrif/application/engine/pfql/CaseSearchServiceTest.javasrc/test/java/com/netgrif/application/engine/pfql/ProcessSearchServiceTest.javasrc/test/java/com/netgrif/application/engine/pfql/QueryLangTest.javasrc/test/java/com/netgrif/application/engine/pfql/TaskSearchServiceTest.javasrc/test/java/com/netgrif/application/engine/pfql/UserSearchServiceTest.javasrc/test/java/com/netgrif/application/engine/pfql/utils/MongoDbUtils.javasrc/test/java/com/netgrif/application/engine/pfql/utils/SearchTestUtils.javasrc/test/resources/petriNets/pfql.xmlsrc/test/resources/petriNets/query_lang_test.xml
Comment on lines
+73
to
+76
| assertEquals("class com.netgrif.application.engine.petrinet.domain.PetriNet", testCase.getFieldValue(RESULT_CLASS_FIELD_ID)) | ||
| updateQuery("processes: identifier == '" + testProcess.identifier + "'") | ||
| pressButton(SEARCH_GENERAL_SEARCH_FIELD_ID) | ||
| assertEquals("class java.util.Collections\$UnmodifiableRandomAccessList", testCase.getFieldValue(RESULT_CLASS_FIELD_ID)) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Avoid asserting concrete class-name strings in action results
These assertions are tightly coupled to implementation class names (especially Collections$UnmodifiableRandomAccessList) and are brittle. Assert on object type/interface from RESULT_FIELD_ID (instanceof List, instanceof Page, domain class type) instead.
Also applies to: 83-84, 87-88, 91-92, 95-96, 98-99, 108-109, 112-113, 116-117, 120-121, 123-124, 133-134, 137-138, 141-142, 145-146, 148-149, 158-159, 162-163, 166-167, 170-171, 173-174
🤖 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/pfql/QueryLangActionTest.groovy`
around lines 73 - 76, The tests currently assert concrete class-name strings
returned via RESULT_CLASS_FIELD_ID (e.g.,
"Collections$UnmodifiableRandomAccessList"); change these to type-safe
assertions by retrieving the actual object from RESULT_FIELD_ID (use
testCase.getFieldValue(RESULT_FIELD_ID)) and assert its runtime type/interface
(e.g., assertTrue(result instanceof List), assertTrue(result instanceof Page) or
assertTrue(result instanceof YourDomainClass)) instead of comparing string class
names; update each occurrence (including references to RESULT_CLASS_FIELD_ID and
testCase.getFieldValue calls and assertEquals) to use assertTrue/assertThat with
instanceof checks.
- fix tests - add todo to LegacyCaseSearchService
There was a problem hiding this comment.
Actionable comments posted: 1
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/workflow/service/LegacyCaseSearchService.java (1)
97-104:⚠️ Potential issue | 🟡 MinorUpdate the stale TODO in
LegacyCaseSearchService.buildPermissionConstraints
The TODO at line 98 (“update with latest logic, that Samo has introduced”) is inconsistent with the current implementation, whose permission constraint pattern matchesLegacyTaskSearchService.buildPermissionConstraints; remove it or update it to reflect the actual logic.🤖 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/workflow/service/LegacyCaseSearchService.java` around lines 97 - 104, The TODO comment in LegacyCaseSearchService.buildPermissionConstraints is stale and misleading; update or remove it to reflect the actual implementation which already follows the same permission constraint pattern as LegacyTaskSearchService.buildPermissionConstraints—either delete the "// todo: update with latest logic, that Samo has introduced" line or replace it with a brief accurate note describing that the method composes view-role, negative-view-role, view-user and negative-view-user constraints by using buildViewRoleQueryConstraint, buildNegativeViewRoleQueryConstraint, buildViewUserQueryConstraint and buildNegativeViewUsersQueryConstraint respectively.
🤖 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/workflow/service/LegacyCaseSearchService.java`:
- Line 92: Remove the outdated TODO comment on line 92 in
LegacyCaseSearchService; since buildPermissionConstraints(...) is already being
called immediately afterwards, delete the "// todo: move latest logic to method
buildPermissionConstraints" comment to avoid confusion and keep the class clean
(referencing the LegacyCaseSearchService class and the
buildPermissionConstraints method to locate the stale comment).
---
Outside diff comments:
In
`@src/main/java/com/netgrif/application/engine/workflow/service/LegacyCaseSearchService.java`:
- Around line 97-104: The TODO comment in
LegacyCaseSearchService.buildPermissionConstraints is stale and misleading;
update or remove it to reflect the actual implementation which already follows
the same permission constraint pattern as
LegacyTaskSearchService.buildPermissionConstraints—either delete the "// todo:
update with latest logic, that Samo has introduced" line or replace it with a
brief accurate note describing that the method composes view-role,
negative-view-role, view-user and negative-view-user constraints by using
buildViewRoleQueryConstraint, buildNegativeViewRoleQueryConstraint,
buildViewUserQueryConstraint and buildNegativeViewUsersQueryConstraint
respectively.
🪄 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: a7a34253-d663-42fe-bed9-277880f5b5a9
📒 Files selected for processing (10)
src/main/java/com/netgrif/application/engine/workflow/service/LegacyCaseSearchService.javasrc/test/groovy/com/netgrif/application/engine/elastic/ReindexTest.groovysrc/test/java/com/netgrif/application/engine/MockService.javasrc/test/resources/all_data.xmlsrc/test/resources/all_data_pdf.xmlsrc/test/resources/ipc_data.xmlsrc/test/resources/ipc_set_data.xmlsrc/test/resources/pdf_test_3.xmlsrc/test/resources/petriNets/impersonation_test.xmlsrc/test/resources/variable_arc_test.xml
- resolve PR comments
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (1)
526-559:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Missing authorization checks in predicate-based methods.
The new
search,searchOne,count, andexistsmethods pass predicates directly to the repository without permission filtering. Compare with the existingsearch(PetriNetSearch, LoggedUser, ...)at line 473, which appliesgetProcessRolesCriteria(user)when the user is not an admin.As public API methods, these can be called by any code (including actions) and will bypass all permission constraints, allowing queries against PetriNets the caller should not access.
🔒 Recommended fix: add permission-aware overloads
Either:
- Add
LoggedUserparameter and compose permission constraints:public Page<PetriNet> search(Predicate predicate, Pageable pageable) { + return search(predicate, pageable, userService.getLoggedOrSystem().transformToLoggedUser()); +} + +public Page<PetriNet> search(Predicate predicate, Pageable pageable, LoggedUser user) { if (predicate == null) { return Page.empty(); } if (pageable == null) { pageable = Pageable.unpaged(); } + if (!user.isAdmin()) { + Predicate permissionConstraint = getProcessRolesPredicate(user); + predicate = ExpressionUtils.and(predicate, permissionConstraint); + } return repository.findAll(predicate, pageable); }
- Or document that callers must pre-filter and mark methods
protectedif internal-only.(Also apply the same pattern to
searchOne,count, andexists.)🤖 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/petrinet/service/PetriNetService.java` around lines 526 - 559, The predicate-based methods search(Predicate), searchOne(Predicate), count(Predicate) and exists(Predicate) currently call repository methods directly and therefore bypass permission filtering; update them to accept a LoggedUser (e.g., add overloads search(Predicate, LoggedUser, Pageable), searchOne(Predicate, LoggedUser), count(Predicate, LoggedUser), exists(Predicate, LoggedUser)) and when the user is not admin compose the incoming Predicate with getProcessRolesCriteria(user) before calling repository.findAll/ count/exists, or alternatively make the existing single-argument methods protected and document they require pre-filtered predicates so only the new permission-aware public overloads are exposed; apply the same composition logic used in search(PetriNetSearch, LoggedUser, ...) to all four methods (search, searchOne, count, exists).src/main/java/com/netgrif/application/engine/pfql/service/utils/SearchUtils.java (1)
281-323:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
version gte/ltecurrently match too broadly.
major.goe(major)/major.loe(major)already accept every version with the same major, so1.0.0satisfiesgte 1.1.1and1.9.0satisfieslte 1.1.1. Build the inclusive operators asgt/lt OR eq(or the equivalent lexicographic form) instead of using inclusive checks on higher-order components.🤖 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/pfql/service/utils/SearchUtils.java` around lines 281 - 323, buildVersionPredicate's GTE and LTE use inclusive comparisons on higher-order components (major.goe/loe) which incorrectly match versions like 1.0.0 for "gte 1.1.1"; fix the GTE and LTE cases in buildVersionPredicate to use lexicographic logic analogous to the GT and LT cases but including equality on the final component: for GTE: qVersion.major.gt(major) OR (qVersion.major.eq(major) AND qVersion.minor.gt(minor)) OR (qVersion.major.eq(major) AND qVersion.minor.eq(minor) AND qVersion.patch.goe(patch)); and for LTE: qVersion.major.lt(major) OR (qVersion.major.eq(major) AND qVersion.minor.lt(minor)) OR (qVersion.major.eq(major) AND qVersion.minor.eq(minor) AND qVersion.patch.loe(patch)); update the QueryLangParser.GTE and QueryLangParser.LTE switch branches accordingly using QVersion (qVersion.major/minor/patch) and keep EQ/NEQ/GT/LT as-is.src/main/java/com/netgrif/application/engine/pfql/service/QueryLangEvaluator.java (1)
1379-1398:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject unsupported Elastic sort attributes instead of silently dropping them.
When
searchWithElasticis true and the requested case sort falls into the TODO path,propstays null and the method just returns. That leaves paged results in backend-default order even though the client asked for a sort.🤖 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/pfql/service/QueryLangEvaluator.java` around lines 1379 - 1398, In QueryLangEvaluator locate the logic that builds sort properties (references: searchWithElastic, attrOrd, prop, caseAttrToSortPropElasticMapping, caseAttrToSortPropMapping and sortOrders.add) and change the behavior when prop resolves to null: instead of silently returning, throw a clear exception (e.g. IllegalArgumentException or a domain-specific exception) indicating the requested case sort attribute is not supported for Elastic search; ensure the error includes the original attribute text (attrOrd.caseAttribute().getText()) and the searchWithElastic context so callers receive an explicit failure rather than implicit backend-default ordering.
♻️ Duplicate comments (4)
src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (1)
3360-3363: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove redundant prefix normalization.
searchTasks,searchProcesses, andsearchUsersall normalize their query strings before delegating to theirpaged*counterparts, which also normalize. This is wasteful and inconsistent withsearchCases(line 3259), which correctly delegates without normalizing.♻️ Proposed fix
List<Task> searchTasks(String query) { - query = SearchUtils.ensureStartsWithTasks(query) return pagedSearchTasks(query).content }List<PetriNet> searchProcesses(String query) { - query = SearchUtils.ensureStartsWithProcesses(query) return pagedSearchProcesses(query).content }List<IUser> searchUsers(String query) { - query = SearchUtils.ensureStartsWithUsers(query) return pagedSearchUsers(query).content }Also applies to: 3462-3465, 3564-3567
🤖 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/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy` around lines 3360 - 3363, The methods searchTasks, searchProcesses, and searchUsers currently call SearchUtils.ensureStartsWithTasks/Processes/Users before delegating to their paged* counterparts which already perform the same normalization; remove the redundant prefix normalization from searchTasks, searchProcesses, and searchUsers so they simply delegate to pagedSearchTasks, pagedSearchProcesses, and pagedSearchUsers (returning .content where appropriate), matching the pattern used by searchCases and avoiding double-normalization.src/main/java/com/netgrif/application/engine/pfql/domain/antlr4/QueryLang.g4 (1)
250-262:⚠️ Potential issue | 🟠 Major | ⚡ Quick winComplete
javaIdcoverage for the remaining keyword tokens.This only fixes part of the original lexer-precedence bug.
javaIdstill excludes tokens such asAND,OR,NOT,EQ,NEQ,LT,GT,LTE,GTE,CONTAINS,IN,WHERE, andBOOLEAN, so selectors likedata.and.value,places.eq.marking, ortasks.where.userIdare still unparseable because those words are emitted as keyword tokens beforeJAVA_ID.Suggested minimal patch
javaId: JAVA_ID | ID | TITLE | IDENTIFIER | VERSION | CREATION_DATE | PROCESS_ID | PROCESS_IDENTIFIER | AUTHOR | PLACES | TRANSITION_ID | STATE | USER_ID | CASE_ID | LAST_ASSIGN | LAST_FINISH | NAME | SURNAME | EMAIL | DATA | VALUE | OPTIONS | MARKING | ENABLED | DISABLED + | WHERE + | AND | OR | NOT + | EQ | NEQ | LT | GT | LTE | GTE | CONTAINS | IN + | BOOLEAN | PAGE | SIZE | ASC | DESC | CASE | CASES | TASK | TASKS | USER | USERS | PROCESS | PROCESSES ;ANTLR4 lexer precedence when both a keyword token and an identifier token match the same input: does the earlier lexer rule win, causing the parser to receive the keyword token instead of the identifier token?🤖 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/pfql/domain/antlr4/QueryLang.g4` around lines 250 - 262, The javaId lexer alternative (javaId / JAVA_ID) omits many keyword tokens so inputs like data.and.value are tokenized as keywords (AND, OR, NOT, EQ, NEQ, LT, GT, LTE, GTE, CONTAINS, IN, WHERE, BOOLEAN) instead of identifiers; update the javaId alternative in QueryLang.g4 to include these missing token names (e.g., AND, OR, NOT, EQ, NEQ, LT, GT, LTE, GTE, CONTAINS, IN, WHERE, BOOLEAN) so the lexer will emit JAVA_ID for those words when used as identifiers, keeping the existing order of alternatives (javaId: JAVA_ID | ID | TITLE ... ) and only adding the missing tokens to that list.src/main/java/com/netgrif/application/engine/pfql/service/QueryLangEvaluator.java (2)
1326-1337:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
tasks.<taskId>.statefilters on case queries are currently a no-op.This handler never sets a Mongo/Elastic predicate and never flips
searchWithElastic, socase: tasks.t1.state eq enabledis accepted but executed without that filter.QueryLangExplainEvaluator.exitTasksStateComparison()already marks the same construct as Elastic-only, so explain/runtime diverge as well. Fail fast here until the Elastic query builder is wired. Based on supplied evaluator/explainer implementations, the runtime and explain paths currently disagree on this construct.🤖 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/pfql/service/QueryLangEvaluator.java` around lines 1326 - 1337, The exitTasksStateComparison handler currently does nothing causing runtime to accept but not execute tasks.<taskId>.state filters; change QueryLangEvaluator.exitTasksStateComparison to fail fast instead of being a no-op: detect calls to exitTasksStateComparison(...) and throw a clear runtime exception (e.g., UnsupportedOperationException or a QueryLang-specific evaluation exception) with a message stating that tasks.<taskId>.state comparisons are Elastic-only and not yet supported at runtime, so the filter is rejected until the Elastic query builder is implemented; this aligns QueryLangEvaluator with QueryLangExplainEvaluator behavior and prevents silent divergence.
809-819:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmpty task attribute handlers turn valid PFQL into an unfiltered
tasksearch.These callbacks never call
setMongoQuery()orsetElasticQuery(). BecauseprocessAndExpression()/processOrExpression()drop null children, queries liketask: state eq enabled,task: lastAssign gt ..., andtask: lastFinish lt ...collapse tofullMongoQuery == null/fullElasticQuery == nullinstead of failing fast. Throw an explicit error here until those branches are implemented.Also applies to: 863-918, 921-975
🤖 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/pfql/service/QueryLangEvaluator.java` around lines 809 - 819, The empty handlers like exitStateComparison are leaving both fullMongoQuery/fullElasticQuery null; update each unimplemented callback (e.g., exitStateComparison and the other empty handlers in this class) to fail fast by throwing a clear runtime exception instead of returning nothing — for example throw new UnsupportedOperationException("PFQL task attribute not implemented: " + ctx.getText()) — or a dedicated parser exception if one exists; ensure the thrown exception occurs before any early return so processAndExpression/processOrExpression can't drop the clause, and include the ctx text to aid debugging; once implemented, replace the exception with the proper setMongoQuery(...) and setElasticQuery(...) calls.
🤖 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.
Outside diff comments:
In
`@src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java`:
- Around line 526-559: The predicate-based methods search(Predicate),
searchOne(Predicate), count(Predicate) and exists(Predicate) currently call
repository methods directly and therefore bypass permission filtering; update
them to accept a LoggedUser (e.g., add overloads search(Predicate, LoggedUser,
Pageable), searchOne(Predicate, LoggedUser), count(Predicate, LoggedUser),
exists(Predicate, LoggedUser)) and when the user is not admin compose the
incoming Predicate with getProcessRolesCriteria(user) before calling
repository.findAll/ count/exists, or alternatively make the existing
single-argument methods protected and document they require pre-filtered
predicates so only the new permission-aware public overloads are exposed; apply
the same composition logic used in search(PetriNetSearch, LoggedUser, ...) to
all four methods (search, searchOne, count, exists).
In
`@src/main/java/com/netgrif/application/engine/pfql/service/QueryLangEvaluator.java`:
- Around line 1379-1398: In QueryLangEvaluator locate the logic that builds sort
properties (references: searchWithElastic, attrOrd, prop,
caseAttrToSortPropElasticMapping, caseAttrToSortPropMapping and sortOrders.add)
and change the behavior when prop resolves to null: instead of silently
returning, throw a clear exception (e.g. IllegalArgumentException or a
domain-specific exception) indicating the requested case sort attribute is not
supported for Elastic search; ensure the error includes the original attribute
text (attrOrd.caseAttribute().getText()) and the searchWithElastic context so
callers receive an explicit failure rather than implicit backend-default
ordering.
In
`@src/main/java/com/netgrif/application/engine/pfql/service/utils/SearchUtils.java`:
- Around line 281-323: buildVersionPredicate's GTE and LTE use inclusive
comparisons on higher-order components (major.goe/loe) which incorrectly match
versions like 1.0.0 for "gte 1.1.1"; fix the GTE and LTE cases in
buildVersionPredicate to use lexicographic logic analogous to the GT and LT
cases but including equality on the final component: for GTE:
qVersion.major.gt(major) OR (qVersion.major.eq(major) AND
qVersion.minor.gt(minor)) OR (qVersion.major.eq(major) AND
qVersion.minor.eq(minor) AND qVersion.patch.goe(patch)); and for LTE:
qVersion.major.lt(major) OR (qVersion.major.eq(major) AND
qVersion.minor.lt(minor)) OR (qVersion.major.eq(major) AND
qVersion.minor.eq(minor) AND qVersion.patch.loe(patch)); update the
QueryLangParser.GTE and QueryLangParser.LTE switch branches accordingly using
QVersion (qVersion.major/minor/patch) and keep EQ/NEQ/GT/LT as-is.
---
Duplicate comments:
In
`@src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy`:
- Around line 3360-3363: The methods searchTasks, searchProcesses, and
searchUsers currently call SearchUtils.ensureStartsWithTasks/Processes/Users
before delegating to their paged* counterparts which already perform the same
normalization; remove the redundant prefix normalization from searchTasks,
searchProcesses, and searchUsers so they simply delegate to pagedSearchTasks,
pagedSearchProcesses, and pagedSearchUsers (returning .content where
appropriate), matching the pattern used by searchCases and avoiding
double-normalization.
In
`@src/main/java/com/netgrif/application/engine/pfql/domain/antlr4/QueryLang.g4`:
- Around line 250-262: The javaId lexer alternative (javaId / JAVA_ID) omits
many keyword tokens so inputs like data.and.value are tokenized as keywords
(AND, OR, NOT, EQ, NEQ, LT, GT, LTE, GTE, CONTAINS, IN, WHERE, BOOLEAN) instead
of identifiers; update the javaId alternative in QueryLang.g4 to include these
missing token names (e.g., AND, OR, NOT, EQ, NEQ, LT, GT, LTE, GTE, CONTAINS,
IN, WHERE, BOOLEAN) so the lexer will emit JAVA_ID for those words when used as
identifiers, keeping the existing order of alternatives (javaId: JAVA_ID | ID |
TITLE ... ) and only adding the missing tokens to that list.
In
`@src/main/java/com/netgrif/application/engine/pfql/service/QueryLangEvaluator.java`:
- Around line 1326-1337: The exitTasksStateComparison handler currently does
nothing causing runtime to accept but not execute tasks.<taskId>.state filters;
change QueryLangEvaluator.exitTasksStateComparison to fail fast instead of being
a no-op: detect calls to exitTasksStateComparison(...) and throw a clear runtime
exception (e.g., UnsupportedOperationException or a QueryLang-specific
evaluation exception) with a message stating that tasks.<taskId>.state
comparisons are Elastic-only and not yet supported at runtime, so the filter is
rejected until the Elastic query builder is implemented; this aligns
QueryLangEvaluator with QueryLangExplainEvaluator behavior and prevents silent
divergence.
- Around line 809-819: The empty handlers like exitStateComparison are leaving
both fullMongoQuery/fullElasticQuery null; update each unimplemented callback
(e.g., exitStateComparison and the other empty handlers in this class) to fail
fast by throwing a clear runtime exception instead of returning nothing — for
example throw new UnsupportedOperationException("PFQL task attribute not
implemented: " + ctx.getText()) — or a dedicated parser exception if one exists;
ensure the thrown exception occurs before any early return so
processAndExpression/processOrExpression can't drop the clause, and include the
ctx text to aid debugging; once implemented, replace the exception with the
proper setMongoQuery(...) and setElasticQuery(...) calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4cc83f62-79fa-4799-8230-c8ce4b4bd677
📒 Files selected for processing (11)
src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovysrc/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.javasrc/main/java/com/netgrif/application/engine/pfql/domain/antlr4/QueryLang.g4src/main/java/com/netgrif/application/engine/pfql/service/QueryLangErrorListener.javasrc/main/java/com/netgrif/application/engine/pfql/service/QueryLangEvaluator.javasrc/main/java/com/netgrif/application/engine/pfql/service/QueryLangExplainEvaluator.javasrc/main/java/com/netgrif/application/engine/pfql/service/SearchService.javasrc/main/java/com/netgrif/application/engine/pfql/service/utils/SearchUtils.javasrc/main/java/com/netgrif/application/engine/workflow/service/TaskService.javasrc/test/java/com/netgrif/application/engine/pfql/QueryLangTest.javasrc/test/java/com/netgrif/application/engine/pfql/utils/SearchTestUtils.java
💤 Files with no reviewable changes (1)
- src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java
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
2 participants
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
Imported Query Language implementation with some changes
New features
Fixes
TaskService.searchTaskService.searchOneWorkflowService.searchWorkflowService.searchOther changes
Implements NAE-2443
Dependencies
New dependency:
Third party dependencies
antlr4-runtime: mandatory for PFQL implementationBlocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
With unit tests:
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
Tests