[NAE-2450] Filter field PFQL support#454
Open
Retoocs wants to merge 6 commits into
Open
Conversation
- create general throwable package - make PFQL conditions optional - update QueryLangEvaluator to generate empty database queries to find all documents - implement new PFQL search endpoints - update tests
- remove prefix autocomplete in ActionDelegate for PFQL methods - implement PFQL endpoints dedicated for Elasticsearch - update tests
- fix PFQL task search endpoint to interact only with Mongo database
- update tests to test permissions
WalkthroughThe PR extends PFQL grammar to make filter conditions optional, removes ChangesPFQL Optional Conditions, Prefix Removal, and New Search Endpoints
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 |
- simplify build of completePredicate
Retoocs
commented
Jun 17, 2026
|
|
||
| public Map<String, String> tags; | ||
|
|
||
| public String query; |
Contributor
Author
There was a problem hiding this comment.
Check ElasticTaskSearchRequest. There is also attribute query
coderabbitai
Bot
added
breaking change
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/pfql/service/QueryLangEvaluator.java`:
- Around line 160-162: The issue is that unconstrained queries on singular
resources (like task, case, user, process) are currently allowed and fall back
to full-scan results. Modify the null condition checks in the QueryLangEvaluator
class at the four locations (lines 160, 178, 196, and 214) to restrict the
handleNoneConditions() fallback to plural/multiple resources only. Before
calling handleNoneConditions() when ctx.processConditionsAndPaging() is null,
add a check to ensure that the query is for a multiple resource (multiple ==
true); if it is a singular resource (multiple == false), reject the query with
appropriate error handling instead of allowing it to proceed without conditions.
In
`@src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java`:
- Around line 192-204: The operation parameter of type MergeFilterOperation is
not being used when combining multiple search predicates in the searchPfql
method. Currently, the code hardcodes ExpressionUtils::and in the reduce
operation regardless of the operation parameter value. To fix this, replace the
hardcoded .reduce(ExpressionUtils::and) call with conditional logic that checks
the operation parameter and applies either AND or OR operation accordingly when
reducing the stream of predicates to create the completePredicate.
In
`@src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.java`:
- Around line 137-141: The exception handling in the catch blocks for
IllegalArgumentException and generic Exception is discarding the original
exception information by not preserving it as the cause when throwing
BadRequestException and InternalServerErrorException. Modify both throw
statements to pass the caught exception e as the cause parameter to the
respective exception constructors (e.g., BadRequestException and
InternalServerErrorException), ensuring that stack-trace causality is preserved
for better production diagnostics and debugging.
In
`@src/test/java/com/netgrif/application/engine/pfql/CaseSearchServiceTest.java`:
- Around line 179-185: The login() and logout() methods in CaseSearchServiceTest
rely on explicit test invocation, meaning if a test fails after calling login(),
the logout() won't execute and authentication state will leak into subsequent
tests. Add an `@AfterEach` annotated cleanup method that unconditionally calls
SecurityContextHolder.getContext().clearContext() to ensure the SecurityContext
is reset after every test execution regardless of pass or fail status. This
replaces the current approach of manually calling logout() in individual tests.
In
`@src/test/java/com/netgrif/application/engine/pfql/TaskSearchServiceTest.java`:
- Around line 147-153: The login and logout methods in TaskSearchServiceTest
rely on manual logout calls to clear authentication, which creates a risk of
authentication state leaking to subsequent tests if an assertion fails before
logout is invoked. Add a new method annotated with `@AfterEach` that calls
SecurityContextHolder.clearContext() to ensure the SecurityContext is always
cleaned up after each test execution, regardless of whether the test passes or
fails, eliminating the dependency on the manual logout method being called.
🪄 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: 47744b06-9d39-4de2-8733-b13b1fbadb7a
📒 Files selected for processing (30)
src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovysrc/main/java/com/netgrif/application/engine/files/interfaces/IStorageService.javasrc/main/java/com/netgrif/application/engine/files/local/LocalStorageService.javasrc/main/java/com/netgrif/application/engine/files/minio/MinIoStorageService.javasrc/main/java/com/netgrif/application/engine/menu/domain/templates/SingleTaskViewTemplate.javasrc/main/java/com/netgrif/application/engine/menu/domain/templates/Template.javasrc/main/java/com/netgrif/application/engine/pfql/domain/antlr4/QueryLang.g4src/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/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/SearchUtils.javasrc/main/java/com/netgrif/application/engine/utils/throwable/BadRequestException.javasrc/main/java/com/netgrif/application/engine/utils/throwable/InternalServerErrorException.javasrc/main/java/com/netgrif/application/engine/workflow/service/LegacyTaskSearchService.javasrc/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.javasrc/main/java/com/netgrif/application/engine/workflow/web/PublicTaskController.javasrc/main/java/com/netgrif/application/engine/workflow/web/TaskController.javasrc/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.javasrc/main/java/com/netgrif/application/engine/workflow/web/requestbodies/TaskSearchRequest.javasrc/main/resources/petriNets/engine-processes/menu/case_view_configuration.xmlsrc/main/resources/petriNets/engine-processes/menu/single_task_view_configuration.xmlsrc/main/resources/petriNets/engine-processes/menu/task_view_configuration.xmlsrc/test/groovy/com/netgrif/application/engine/pfql/QueryLangActionTest.groovysrc/test/java/com/netgrif/application/engine/pfql/CaseSearchServiceTest.javasrc/test/java/com/netgrif/application/engine/pfql/QueryLangTest.javasrc/test/java/com/netgrif/application/engine/pfql/TaskSearchServiceTest.javasrc/test/resources/petriNets/query_lang_test.xml
💤 Files with no reviewable changes (6)
- src/main/java/com/netgrif/application/engine/pfql/service/userresource/UserSearchService.java
- src/main/java/com/netgrif/application/engine/pfql/service/processresource/ProcessSearchService.java
- src/main/java/com/netgrif/application/engine/pfql/service/caseresource/CaseSearchService.java
- src/test/groovy/com/netgrif/application/engine/pfql/QueryLangActionTest.groovy
- src/main/java/com/netgrif/application/engine/pfql/service/utils/SearchUtils.java
- src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy
Comment on lines
+160
to
+162
| if (ctx.processConditionsAndPaging() == null || ctx.processConditionsAndPaging().processConditions() == null) { | ||
| handleNoneConditions(); | ||
| } else { |
There was a problem hiding this comment.
Prevent unconstrained single-resource queries from falling back to full-scan predicates.
Lines 160, 178, 196, and 214 currently route missing conditions to handleNoneConditions() for both plural and singular resources. That makes queries like task, case, user, process legal and unconstrained, so downstream single-result searches can return an arbitrary first match. Restrict this fallback to multiple == true and reject missing conditions for singular queries.
Suggested fix
@@
- if (ctx.processConditionsAndPaging() == null || ctx.processConditionsAndPaging().processConditions() == null) {
- handleNoneConditions();
+ if (ctx.processConditionsAndPaging() == null || ctx.processConditionsAndPaging().processConditions() == null) {
+ if (!multiple) {
+ throw new IllegalArgumentException("Single process query must contain conditions");
+ }
+ handleNoneConditions();
@@
- if (ctx.caseConditionsAndPaging() == null || ctx.caseConditionsAndPaging().caseConditions() == null) {
- handleNoneConditions();
+ if (ctx.caseConditionsAndPaging() == null || ctx.caseConditionsAndPaging().caseConditions() == null) {
+ if (!multiple) {
+ throw new IllegalArgumentException("Single case query must contain conditions");
+ }
+ handleNoneConditions();
@@
- if (ctx.taskConditionsAndPaging() == null || ctx.taskConditionsAndPaging().taskConditions() == null) {
- handleNoneConditions();
+ if (ctx.taskConditionsAndPaging() == null || ctx.taskConditionsAndPaging().taskConditions() == null) {
+ if (!multiple) {
+ throw new IllegalArgumentException("Single task query must contain conditions");
+ }
+ handleNoneConditions();
@@
- if (ctx.userConditionsAndPaging() == null || ctx.userConditionsAndPaging().userConditions() == null) {
- handleNoneConditions();
+ if (ctx.userConditionsAndPaging() == null || ctx.userConditionsAndPaging().userConditions() == null) {
+ if (!multiple) {
+ throw new IllegalArgumentException("Single user query must contain conditions");
+ }
+ handleNoneConditions();Also applies to: 178-180, 196-198, 214-216
🤖 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 160 - 162, The issue is that unconstrained queries on singular
resources (like task, case, user, process) are currently allowed and fall back
to full-scan results. Modify the null condition checks in the QueryLangEvaluator
class at the four locations (lines 160, 178, 196, and 214) to restrict the
handleNoneConditions() fallback to plural/multiple resources only. Before
calling handleNoneConditions() when ctx.processConditionsAndPaging() is null,
add a check to ensure that the query is for a multiple resource (multiple ==
true); if it is a singular resource (multiple == false), reject the query with
appropriate error handling instead of allowing it to proceed without conditions.
Comment on lines
+192
to
+204
| public PagedModel<LocalisedTaskResource> searchPfql(Authentication auth, Pageable pageable, SingleTaskSearchRequestAsList searchBody, MergeFilterOperation operation, PagedResourcesAssembler<Task> assembler, Locale locale) { | ||
| try { | ||
| Predicate completePredicate = searchBody.getList().stream() | ||
| .map((request) -> { | ||
| if (request.query == null || request.query.isEmpty()) { | ||
| return searchService.buildSingleQuery(request, (LoggedUser) auth.getPrincipal(), locale); | ||
| } else { | ||
| QueryLangEvaluator evaluator = SearchUtils.evaluateQuery(request.query); | ||
| return evaluator.getFullMongoQuery(); | ||
| } | ||
| }) | ||
| .reduce(ExpressionUtils::and).orElse(null); | ||
| Page<Task> tasks = taskService.search(completePredicate, pageable); |
There was a problem hiding this comment.
operation is ignored when combining predicates (Line 203).
searchPfql always reduces with ExpressionUtils::and, so requests with operation=OR are evaluated as AND.
Proposed fix
- .reduce(ExpressionUtils::and).orElse(null);
+ .reduce(operation == MergeFilterOperation.AND ? ExpressionUtils::and : ExpressionUtils::or)
+ .orElse(null);📝 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
| public PagedModel<LocalisedTaskResource> searchPfql(Authentication auth, Pageable pageable, SingleTaskSearchRequestAsList searchBody, MergeFilterOperation operation, PagedResourcesAssembler<Task> assembler, Locale locale) { | |
| try { | |
| Predicate completePredicate = searchBody.getList().stream() | |
| .map((request) -> { | |
| if (request.query == null || request.query.isEmpty()) { | |
| return searchService.buildSingleQuery(request, (LoggedUser) auth.getPrincipal(), locale); | |
| } else { | |
| QueryLangEvaluator evaluator = SearchUtils.evaluateQuery(request.query); | |
| return evaluator.getFullMongoQuery(); | |
| } | |
| }) | |
| .reduce(ExpressionUtils::and).orElse(null); | |
| Page<Task> tasks = taskService.search(completePredicate, pageable); | |
| public PagedModel<LocalisedTaskResource> searchPfql(Authentication auth, Pageable pageable, SingleTaskSearchRequestAsList searchBody, MergeFilterOperation operation, PagedResourcesAssembler<Task> assembler, Locale locale) { | |
| try { | |
| Predicate completePredicate = searchBody.getList().stream() | |
| .map((request) -> { | |
| if (request.query == null || request.query.isEmpty()) { | |
| return searchService.buildSingleQuery(request, (LoggedUser) auth.getPrincipal(), locale); | |
| } else { | |
| QueryLangEvaluator evaluator = SearchUtils.evaluateQuery(request.query); | |
| return evaluator.getFullMongoQuery(); | |
| } | |
| }) | |
| .reduce(operation == MergeFilterOperation.AND ? ExpressionUtils::and : ExpressionUtils::or) | |
| .orElse(null); | |
| Page<Task> tasks = taskService.search(completePredicate, pageable); |
🧰 Tools
🪛 ast-grep (0.43.0)
[warning] 203-203: Avoid LDAP injections
Context: taskService.search(completePredicate, pageable)
Note: [CWE-90] Improper Neutralization of Special Elements used in an LDAP Query ('LDAP Injection'). Security best practice.
(ldap-injection-java)
🤖 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/web/AbstractTaskController.java`
around lines 192 - 204, The operation parameter of type MergeFilterOperation is
not being used when combining multiple search predicates in the searchPfql
method. Currently, the code hardcodes ExpressionUtils::and in the reduce
operation regardless of the operation parameter value. To fix this, replace the
hardcoded .reduce(ExpressionUtils::and) call with conditional logic that checks
the operation parameter and applies either AND or OR operation accordingly when
reducing the stream of predicates to create the completePredicate.
Comment on lines
+137
to
+141
| } catch (IllegalArgumentException e) { | ||
| throw new BadRequestException("Bad request: " + e.getMessage()); | ||
| } catch (Exception e) { | ||
| log.error("Something went wrong while searching cases by PFQL", e); | ||
| throw new InternalServerErrorException("Something went wrong"); |
There was a problem hiding this comment.
Preserve exception causes when mapping PFQL errors to HTTP exceptions.
Line 138 and Line 141 discard the original Throwable, which drops stack-trace causality and weakens production diagnostics.
Suggested patch
diff --git a/src/main/java/com/netgrif/application/engine/utils/throwable/InternalServerErrorException.java b/src/main/java/com/netgrif/application/engine/utils/throwable/InternalServerErrorException.java
@@
public class InternalServerErrorException extends RuntimeException {
public InternalServerErrorException(String message) {
super(message);
}
+
+ public InternalServerErrorException(String message, Throwable cause) {
+ super(message, cause);
+ }
}
diff --git a/src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.java b/src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.java
@@
} catch (IllegalArgumentException e) {
- throw new BadRequestException("Bad request: " + e.getMessage());
+ throw new BadRequestException("Bad request: " + e.getMessage(), e);
} catch (Exception e) {
log.error("Something went wrong while searching cases by PFQL", e);
- throw new InternalServerErrorException("Something went wrong");
+ throw new InternalServerErrorException("Something went wrong", e);
}🧰 Tools
🪛 PMD (7.25.0)
[Medium] 138-138: PreserveStackTrace (Best Practices): Thrown exception does not preserve the stack trace of exception 'e' on all code paths
(PreserveStackTrace (Best Practices))
[Medium] 141-141: PreserveStackTrace (Best Practices): Thrown exception does not preserve the stack trace of exception 'e' on all code paths
(PreserveStackTrace (Best Practices))
🤖 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/web/WorkflowController.java`
around lines 137 - 141, The exception handling in the catch blocks for
IllegalArgumentException and generic Exception is discarding the original
exception information by not preserving it as the cause when throwing
BadRequestException and InternalServerErrorException. Modify both throw
statements to pass the caught exception e as the cause parameter to the
respective exception constructors (e.g., BadRequestException and
InternalServerErrorException), ensuring that stack-trace causality is preserved
for better production diagnostics and debugging.
Source: Linters/SAST tools
Comment on lines
+179
to
+185
| private void login(LoggedUser loggedUser) { | ||
| SecurityContextHolder.getContext().setAuthentication(new UsernamePasswordAuthenticationToken(loggedUser, null)); | ||
| } | ||
|
|
||
| private void logout() { | ||
| SecurityContextHolder.getContext().setAuthentication(null); | ||
| } |
There was a problem hiding this comment.
Harden SecurityContext cleanup to prevent cross-test leakage
Line 179–Line 185 cleanup is not failure-safe: if a test fails after login(...), logout() won’t run and auth state can leak into later tests. Add an @AfterEach cleanup and use clearContext().
Proposed fix
@@
-import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
@@
`@BeforeEach`
protected void beforeEach() {
testHelper.truncateDbs();
Optional<PetriNet> createdNetOpt = importHelper.createNet("/query_lang_test.xml");
assertTrue(createdNetOpt.isPresent());
testCase = importHelper.createCase("test", createdNetOpt.get());
}
+
+ `@AfterEach`
+ protected void afterEach() {
+ SecurityContextHolder.clearContext();
+ }
@@
private void logout() {
- SecurityContextHolder.getContext().setAuthentication(null);
+ SecurityContextHolder.clearContext();
}🤖 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/java/com/netgrif/application/engine/pfql/CaseSearchServiceTest.java`
around lines 179 - 185, The login() and logout() methods in
CaseSearchServiceTest rely on explicit test invocation, meaning if a test fails
after calling login(), the logout() won't execute and authentication state will
leak into subsequent tests. Add an `@AfterEach` annotated cleanup method that
unconditionally calls SecurityContextHolder.getContext().clearContext() to
ensure the SecurityContext is reset after every test execution regardless of
pass or fail status. This replaces the current approach of manually calling
logout() in individual tests.
Comment on lines
+147
to
+153
| private void login(LoggedUser loggedUser) { | ||
| SecurityContextHolder.getContext().setAuthentication(new UsernamePasswordAuthenticationToken(loggedUser, null)); | ||
| } | ||
|
|
||
| private void logout() { | ||
| SecurityContextHolder.getContext().setAuthentication(null); | ||
| } |
There was a problem hiding this comment.
Add failure-safe SecurityContext teardown
Line 147–Line 153 relies on manual logout in test flow. If an assertion fails before logout(), authentication can leak to subsequent tests. Add @AfterEach with SecurityContextHolder.clearContext().
Proposed fix
@@
-import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
@@
`@BeforeEach`
protected void beforeEach() {
testHelper.truncateDbs();
Optional<PetriNet> createdNetOpt = importHelper.createNet("/query_lang_test.xml");
assertTrue(createdNetOpt.isPresent());
Case testCase = importHelper.createCase("test", createdNetOpt.get());
testTaskId = testCase.getTasks().stream()
.filter(taskPair -> taskPair.getTransition().equals("t1"))
.map(TaskPair::getTask)
.findFirst().get();
}
+
+ `@AfterEach`
+ protected void afterEach() {
+ SecurityContextHolder.clearContext();
+ }
@@
private void logout() {
- SecurityContextHolder.getContext().setAuthentication(null);
+ SecurityContextHolder.clearContext();
}🤖 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/java/com/netgrif/application/engine/pfql/TaskSearchServiceTest.java`
around lines 147 - 153, The login and logout methods in TaskSearchServiceTest
rely on manual logout calls to clear authentication, which creates a risk of
authentication state leaking to subsequent tests if an assertion fails before
logout is invoked. Add a new method annotated with `@AfterEach` that calls
SecurityContextHolder.clearContext() to ensure the SecurityContext is always
cleaned up after each test execution, regardless of whether the test passes or
fails, eliminating the dependency on the manual logout method being called.
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
cases,tasks, ...)Implements NAE-2450
Dependencies
No new dependencies were introduced
Third party dependencies
No new dependencies were introduced
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
Manually and by unit tests
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
tasks:without filter criteria).Bug Fixes
Refactor