[NAE-2460] PFQL placeholder support#461
Conversation
- add LoggedUser placeholder to PFQL
- update tests - add todo
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds PFQL support for ChangesPFQL loggedUser placeholder support
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)
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
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: 3
🤖 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 189-203: handleObjectIdListComparison currently adds the logged
user id once for any loggedUser.* entries, which incorrectly treats all
placeholders as the same value. Update the StringListContext handling so each
ctx.loggedUserStringAttribute() element is resolved individually using its own
attribute and converted through getObjectIdValue, instead of always using
loggedUser.getId(). Make sure the logic in QueryLangEvaluator distinguishes
loggedUser.id from other logged user attributes and rejects unsupported non-id
attributes in this ObjectId comparison path.
- Around line 220-235: The stringRange endpoint resolution in
handleInRangeStringComparison is using indexed token accessors that can swap
left/right sides and bypass logged-user placeholder parsing. Update this method
to iterate ctx.children in order, resolve each endpoint by its actual node type,
and route loggedUserStringAttribute nodes through
handleLoggedUserStringAttribute(...) instead of using getText(). Ensure mixed
literal/placeholder ranges preserve the original endpoint order.
In `@src/test/java/com/netgrif/application/engine/pfql/QueryLangTest.java`:
- Around line 908-916: The `case: id in (...)` behavior is inconsistent between
Mongo and Elastic because `handleObjectIdListComparison()` in the query handling
path normalizes every `loggedUser.*` token to the user ID, while the Elastic
query builder preserves `loggedUser.username` and `loggedUser.fullName` values;
update the Elastic-side handling to match the Mongo normalization, or centralize
the normalization so both backends resolve `id` list comparisons the same way in
`QueryLangTest`-covered behavior.
🪄 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: d3d81d01-9a1c-499b-b54c-e099c00f8af3
📒 Files selected for processing (4)
src/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/utils/SearchUtils.javasrc/test/java/com/netgrif/application/engine/pfql/QueryLangTest.java
- resolve PR comments
coderabbitai
Bot
removed
the
improvement
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test/java/com/netgrif/application/engine/pfql/QueryLangTest.java`:
- Around line 326-330: Add coverage for range comparisons that use loggedUser
placeholders so the evaluator path is exercised in both Mongo and Elastic tests.
Update the relevant query language tests around the existing comparison cases to
include a query like a range using loggedUser.username or loggedUser.fullName,
and assert the resolved user value is used rather than the raw token text. Use
QueryLangEvaluator.handleInRangeStringComparison and the loggedUser.* handling
methods as the key symbols to locate the fix and ensure the tests fail until the
placeholder is properly resolved.
🪄 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: 3e583106-22da-4008-9375-5eade5f9eabd
📒 Files selected for processing (2)
src/main/java/com/netgrif/application/engine/pfql/service/QueryLangEvaluator.javasrc/test/java/com/netgrif/application/engine/pfql/QueryLangTest.java
- fix QueryLangEvaluator.handleInRangeStringComparison
- fix test
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 (1)
src/main/java/com/netgrif/application/engine/pfql/service/QueryLangEvaluator.java (1)
857-858: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDerive the Elastic process-id list from resolved ObjectIds.
Line 858 reintroduces non-ObjectId placeholders into the Elastic query even though Line 857 filters object-id semantics via
handleObjectIdListComparison(...). For example,processId in (loggedUser.id, loggedUser.username)resolves Mongo to only the user id, but Elastic gets both the id and username. MirrorexitIdListand derive the string list fromobjectIdList.🐛 Proposed fix
boolean not = ctx.inListStringComparison().NOT() != null; List<ObjectId> objectIdList = handleObjectIdListComparison(ctx.inListStringComparison().stringList()); - List<String> stringList = handleStringListComparison(ctx.inListStringComparison().stringList()); + List<String> stringList = objectIdList.stream().map(ObjectId::toString).collect(Collectors.toList()); setMongoQuery(ctx, buildObjectIdPredicateInList(qObjectId, objectIdList, not)); setElasticQuery(ctx, buildElasticQueryInList("processId", stringList, not));🤖 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 857 - 858, The Elastic process-id list is being built from the raw string list instead of the resolved ObjectIds, so placeholders that don’t map to an ObjectId slip back into the query. In QueryLangEvaluator, update the in-list handling around handleObjectIdListComparison(...) to derive the String list from objectIdList, mirroring exitIdList, and ensure only the resolved ids are used for the Elastic query path.
🤖 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/pfql/service/QueryLangEvaluator.java`:
- Around line 857-858: The Elastic process-id list is being built from the raw
string list instead of the resolved ObjectIds, so placeholders that don’t map to
an ObjectId slip back into the query. In QueryLangEvaluator, update the in-list
handling around handleObjectIdListComparison(...) to derive the String list from
objectIdList, mirroring exitIdList, and ensure only the resolved ids are used
for the Elastic query path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 047b2eeb-204e-494b-9b86-ab1bb5bf019c
📒 Files selected for processing (2)
src/main/java/com/netgrif/application/engine/pfql/service/QueryLangEvaluator.javasrc/test/java/com/netgrif/application/engine/pfql/QueryLangTest.java
2 participants
Description
Added placeholders
loggedUser.id,loggedUser.username,loggedUser.fullNameandloggedUser.anonymous. The placeholders can be used:.anonymous).anonymous).id)Implements NAE-2460
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?
By unit test:
QueryLangTestTest Configuration
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests