refactor: object list special filter queries [DHIS2-17200] #19046
Conversation
This file contains 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
jbee
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...c/main/java/org/hisp/dhis/webapi/controller/organisationunit/OrganisationUnitController.java
Fixed
Show fixed
Hide fixed
jbee
commented
Nov 22, 2024
| @@ -250,6 +251,14 @@ static UserDetails createUserDetails( | |||
|
|
|||
| void setId(Long id); | |||
|
|
|||
| default boolean canIssueUserRole(UserRole role, boolean canGrantOwnUserRole) { | |||
There was a problem hiding this comment.
FYI: An ID based version of the existing canIssueUserRole in User
jbee
commented
Nov 22, 2024
| case "null" -> Restrictions.isNull(path); | ||
| case "!null" -> Restrictions.isNotNull(path); | ||
| case "empty" -> Restrictions.isEmpty(path); | ||
| default -> throw new QueryParserException("`" + operator + "` is not a valid operator."); | ||
| }; | ||
| } | ||
|
|
||
| @Nonnull | ||
| @SuppressWarnings("rawtypes") | ||
| private Collection parseValues(Property property, Class<?> valueType, Object arg) { |
There was a problem hiding this comment.
FYI: duplication extracted to this method
jbee
commented
Nov 22, 2024
| return values; | ||
| } | ||
|
|
||
| private Collection<Disjunction> getDisjunctionsFromCustomMentions( |
There was a problem hiding this comment.
FYI: logic extracted from outside of the engine to be inside as it operates on the 3 parts on a filter
jbee
commented
Nov 22, 2024
| @@ -108,68 +111,11 @@ public List<T> query(Query query) { | |||
| CriteriaQuery<T> criteriaQuery = builder.createQuery(klass); | |||
| Root<T> root = criteriaQuery.from(klass); | |||
|
|
|||
| if (query.isEmpty()) { | |||
There was a problem hiding this comment.
FYI: This path was eliminated as it should be equal to the "else" path except for the starting point being an empty conjunction.
Note that this path did not add orders because isEmpty is the absence of filters and orders. In the refactored code the effect is the same but the orders are always added when non-empty.
jbee
commented
Nov 22, 2024
| o -> | ||
| o.isAscending() | ||
| ? builder.asc(root.get(o.getProperty().getName())) | ||
| : builder.desc(root.get(o.getProperty().getName()))) |
There was a problem hiding this comment.
FYI: I think using getName() instead of getFieldName() was a bug - new code uses a common extracted method for fetch and count query
jbee
commented
Nov 22, 2024
Comment on lines
177
to
178
| boolean hasUserGroupFilter = filters.stream().anyMatch(f -> f.startsWith("userGroups.")); | ||
| params.setPrefetchUserGroups(hasUserGroupFilter); |
There was a problem hiding this comment.
FYI: This is no longer needed because the query will only result in a list of UIDs
… expression Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
jbee
changed the title
vietnguyen
approved these changes
Nov 25, 2024
david-mackessy
approved these changes
Nov 25, 2024
jbee
added a commit
that referenced
this pull request
Nov 25, 2024
* fix: non-standard object list queries [DHIS2-17200] * fix: users may use standard pager * refactor: special filters as pre-stage query for ids * chore: revert now unnecessary change * refactor: use query filters for OU special params * fix: failing integration test causes * refactor: parameter objects for getObject and getObjectList * fix: dependency issues * fix: params in tests * fix: don't order count query, use getFieldName() for properties in criteria API * fix: fields parameter and filter and order parameter mapping * fix: strip regex from path variables for path * fix: filter parameter splitting * refactor: always use AbstractCrudController with list params parameter * refactor: always use object list parameter for read-only CRUD controller * fix: broken superclass and import * fix: add missing @EntityType definitions when/where used * fix: metadata import service * fix: filters on getObject * fix: dimension controller issues * fix: maven dependencies * fix: attempt to disable dependency analzye run for e2e tests * fix: sonar issues and add tests * fix: always add sorting and cache hints in JPA query engine * fix: more sonar issues * refactor: user special filters as UID pre-query, checked exceptions in filter creation * fix: query user IDs * fix: user query for IDs and always false detection * fix: sonar TODOs and issues * fix: in operator with empty value list is always false * chore: cleanup OU special filters * fix: code scanning alert no. 9132: User-controlled data in arithmetic expression Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --------- Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
|
|
Thir PR was merged but somehow it does not show as merged, therefore I am closing it now |
This was referenced Nov 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Object list (
GETfor/api/dataElement,/api/users, ...) sometimes support special filters that are not specified viafilter=but dedicated request parameters likeinactiveSincefor users. When such filters are involved the paging and total count could be off. The cause was that special filters run a separate query, the result of that was in-memory filtered in the standard query engine. While that works for the page results the totals and the paging cannot be made correct as neither the first nor the second step see the full picture.This PR fixes that by integrating special filters in one of two ways:
This effectively means the main query running in the query engine will always see the full picture and thus have correct totals and paging.
In a few cases the starting list a query runs against is not a DB table but computed by calling another service method custom to the use case. In such cases there is always a pure in-memory filtering performed by the query service.
For totals to be correct these will call
query2 times, once without paging for the total count, once with paging for the result page. This isn't a change but has been cleaned up to follow the same pattern (use mostly the same code).Performance
In many instances this also improves performance and resource consumption as only a single query runs which does paging in the DB instead of loading all matches into memory and page based on that list.
Programmatic Query Modifiers
There are two ways to modify the query programmatically:
addFilteroraddOrderCriterionto theQuerybefore it runsThe second is required since the first cannot express more complex filters which were needed to implement the existing special filters.
Object List Parameter Object
Special filters are provided by a method that can be overridden by controllers. In order to give them access to the special filters they define the parameter object needed to become a generic of the abstract controller. Only this way a endpoint method with the same method signature can have varying parameters. This required a refactoring that would introduce such a parameter as the only alternative would have been to keep relying on
Map<String,String>for parameters which makes them entirely in-transparent.Automatic Testing
Some tests got adjusted, some new one added.
Manual Testing
A good way to test might be to use the OpenAPI documentation to learn about the special filters that do exist and then use them and compare the results with older versions.
Endpoints with additional filters
/api/organisationUnits/api/users/api/userRoles/api/messageConversations/api/dataElementOperands/api/programs/api/trackedEntityAttributes/api/validationRules