Skip to content

Commit

Permalink
Optimize approverRelation "OR" query
Browse files Browse the repository at this point in the history 
The "OR" REF query over multiple relations (specified by
`approverRelation` property) was inefficient on some databases.
(Especially for old generic repo, where multiple left joins were
used in such cases.)

It is now replaced by a set of independent queries, with the results
merged afterwards.

We assume that there will not be a large number of relations used, so
the number of queries will be low.

This resolves MID-8134.
  • Loading branch information
@mederly
mederly committed Mar 16, 2023
1 parent ecfafe7 commit 7e655c6
Show file tree
Hide file tree
Showing 6 changed files with 181 additions and 30 deletions.
3 changes: 2 additions & 1 deletion .../main/java/com/evolveum/midpoint/wf/impl/processes/itemApproval/ApprovalSchemaHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ public void prepareStage(ApprovalStageDefinitionType stageDef, RelationResolver
resolvedApprovers.addAll(referenceResolver.resolveReference(ref, "approver ref"));
}
// resolves approver relations
resolvedApprovers.addAll(relationResolver.getApprovers(stageDef.getApproverRelation()));
resolvedApprovers.addAll(
relationResolver.getApprovers(stageDef.getApproverRelation()));
stageDef.getApproverRef().clear();
stageDef.getApproverRef().addAll(resolvedApprovers);
stageDef.getApproverRelation().clear();
Expand Down
55 changes: 33 additions & 22 deletions ...java/com/evolveum/midpoint/wf/impl/processors/primary/aspect/BasePrimaryChangeAspect.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,16 @@
package com.evolveum.midpoint.wf.impl.processors.primary.aspect;

import static com.evolveum.midpoint.prism.PrismObject.asObjectable;
import static com.evolveum.midpoint.prism.PrismObject.asObjectableList;

import java.util.Collections;
import java.util.List;
import java.util.stream.Collectors;
import javax.annotation.PostConstruct;
import javax.xml.namespace.QName;

import com.evolveum.midpoint.schema.util.ObjectSet;

import org.jetbrains.annotations.NotNull;
import org.springframework.beans.factory.BeanNameAware;
import org.springframework.beans.factory.annotation.Autowired;
Expand All @@ -26,8 +29,6 @@
import com.evolveum.midpoint.model.impl.util.ModelImplUtils;
import com.evolveum.midpoint.prism.*;
import com.evolveum.midpoint.prism.query.ObjectFilter;
import com.evolveum.midpoint.prism.query.ObjectQuery;
import com.evolveum.midpoint.prism.query.builder.S_FilterExit;
import com.evolveum.midpoint.repo.api.RepositoryService;
import com.evolveum.midpoint.repo.common.SystemObjectCache;
import com.evolveum.midpoint.repo.common.expression.ExpressionEnvironmentThreadLocalHolder;
Expand Down Expand Up @@ -167,7 +168,8 @@ public ReferenceResolver createReferenceResolver(LensContext<?> modelContext, Ta
if (ref.getType() != null) {
clazz = prismContext.getSchemaRegistry().determineCompileTimeClass(ref.getType());
if (clazz == null) {
throw new SchemaException("Cannot determine type from " + ref.getType() + " in approver reference in " + sourceDescription);
throw new SchemaException(
"Cannot determine type from " + ref.getType() + " in approver reference in " + sourceDescription);
}
} else {
throw new SchemaException("Missing type in target reference in " + sourceDescription);
Expand All @@ -179,28 +181,37 @@ public ReferenceResolver createReferenceResolver(LensContext<?> modelContext, Ta

public RelationResolver createRelationResolver(PrismObject<?> object, OperationResult result) {
return relations -> {
if (object == null || object.getOid() == null || relations.isEmpty()) {
return Collections.emptyList();
if (object == null
|| object.getOid() == null
|| relations.isEmpty()) {
return List.of();
}
S_FilterExit q = prismContext.queryFor(FocusType.class).none();
for (QName approverRelation : relations) {
PrismReferenceValue approverReference = prismContext.itemFactory().createReferenceValue(object.getOid());
approverReference.setRelation(relationRegistry.normalizeRelation(approverRelation));
q = q.or().item(FocusType.F_ROLE_MEMBERSHIP_REF).ref(approverReference);
ObjectSet<ObjectType> approvers = new ObjectSet<>();
for (QName relation : relations) {
approvers.addAll(
findApproversByRelation(object, relation, result));
}
ObjectQuery query = q.build();
LOGGER.trace("Looking for approvers for {} using query:\n{}", object, DebugUtil.debugDumpLazily(query));
List<PrismObject<FocusType>> objects;
try {
objects = repositoryService.searchObjects(FocusType.class, query, null, result);
} catch (SchemaException e) {
throw new SystemException("Couldn't retrieve approvers for " + object + ": " + e.getMessage(), e);
}
List<PrismObject<FocusType>> distinctObjects = ObjectTypeUtil.keepDistinctObjects(objects);
LOGGER.trace("Query evaluation resulted in {} approver(s): {}", distinctObjects.size(), DebugUtil.toStringLazily(distinctObjects));
return distinctObjects.stream()
.map(object1 -> ObjectTypeUtil.createObjectRef(object1))
LOGGER.trace("Query evaluation resulted in {} approver(s): {}", approvers.size(), DebugUtil.toStringLazily(approvers));
return approvers.stream()
.map(o -> ObjectTypeUtil.createObjectRef(o))
.collect(Collectors.toList());
};
}

private @NotNull List<FocusType> findApproversByRelation(
PrismObject<?> object, QName relation, OperationResult result) {

PrismReferenceValue approverReference = prismContext.itemFactory().createReferenceValue(object.getOid());
approverReference.setRelation(relationRegistry.normalizeRelation(relation));
var query = prismContext.queryFor(FocusType.class)
.item(FocusType.F_ROLE_MEMBERSHIP_REF).ref(approverReference)
.build();
LOGGER.trace("Looking for approvers for {} using query:\n{}", object, DebugUtil.debugDumpLazily(query));
try {
return asObjectableList(
repositoryService.searchObjects(FocusType.class, query, null, result));
} catch (SchemaException e) {
throw new SystemException("Couldn't retrieve approvers for " + object + ": " + e.getMessage(), e);
}
}
}
99 changes: 93 additions & 6 deletions model/workflow-impl/src/test/java/com/evolveum/midpoint/wf/impl/other/TestMiscellaneous.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
*/
package com.evolveum.midpoint.wf.impl.other;

import static com.evolveum.midpoint.util.MiscUtil.emptyIfNull;

import static java.util.Collections.singletonList;
import static org.assertj.core.api.Assertions.assertThat;
import static org.testng.AssertJUnit.assertEquals;
Expand All @@ -15,6 +17,8 @@
import java.util.*;
import java.util.stream.Collectors;

import com.evolveum.midpoint.prism.query.*;

import org.jetbrains.annotations.NotNull;
import org.springframework.test.annotation.DirtiesContext;
import org.springframework.test.context.ContextConfiguration;
Expand All @@ -28,8 +32,6 @@
import com.evolveum.midpoint.prism.PrismContext;
import com.evolveum.midpoint.prism.PrismObject;
import com.evolveum.midpoint.prism.delta.ObjectDelta;
import com.evolveum.midpoint.prism.query.ObjectQuery;
import com.evolveum.midpoint.prism.query.RefFilter;
import com.evolveum.midpoint.schema.constants.ObjectTypes;
import com.evolveum.midpoint.schema.constants.SchemaConstants;
import com.evolveum.midpoint.schema.result.OperationResult;
Expand All @@ -50,6 +52,8 @@
import com.evolveum.midpoint.xml.ns._public.common.common_3.*;
import com.evolveum.prism.xml.ns._public.query_3.QueryType;

import javax.xml.namespace.QName;

@ContextConfiguration(locations = { "classpath:ctx-workflow-test-main.xml" })
@DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_CLASS)
public class TestMiscellaneous extends AbstractWfTestPolicy {
Expand Down Expand Up @@ -92,6 +96,10 @@ public class TestMiscellaneous extends AbstractWfTestPolicy {
TEST_DIR, "org-approvers.xml", "8b928d45-bb91-4a02-8418-6ae0d3b6a7d2");
private static final TestObject<RoleType> ROLE_APPROVED_BY_ORG = TestObject.file(
TEST_DIR, "role-approved-by-org.xml", "9a563d3e-12aa-4dc1-a6ee-de9e9b33974e");
private static final TestObject<RoleType> ROLE_APPROVED_BY_MULTIPLE_RELATIONS = TestObject.file(
TEST_DIR, "role-approved-by-multiple-relations.xml", "62d7fcdf-92b0-4c49-ae40-33b0a814ed56");
private static final TestObject<UserType> USER_APPROVER_BY_MULTIPLE_RELATIONS = TestObject.file(
TEST_DIR, "user-approver-by-multiple-relations.xml", "a9aca7bb-923e-4be6-9aa4-5c90af978207");

@Override
protected PrismObject<UserType> getDefaultActor() {
Expand Down Expand Up @@ -127,6 +135,8 @@ public void initSystem(Task initTask, OperationResult initResult) throws Excepti

ORG_APPROVERS.init(this, initTask, initResult);
ROLE_APPROVED_BY_ORG.init(this, initTask, initResult);
ROLE_APPROVED_BY_MULTIPLE_RELATIONS.init(this, initTask, initResult);
USER_APPROVER_BY_MULTIPLE_RELATIONS.init(this, initTask, initResult);
}

@Test
Expand Down Expand Up @@ -817,7 +827,7 @@ public void test400NotificationsWithAutoCompletion() throws Exception {
.assignment(ROLE_AUTOCOMPLETIONS.assignmentTo());
addObject(user, task, result);

then("role is not assigned, and a case is created");
then("user is not created but case exists");
assertNoUserByUsername(name);
assertCase(result, "after")
.display();
Expand Down Expand Up @@ -857,8 +867,6 @@ public void test410NoDeputyQueryWhenThereAreNoAssignees() throws Exception {
OperationResult result = task.getResult();
login(userAdministrator);

dummyTransport.clearMessages();

when("a user with role assignment is created");
String name = "test410";
UserType user = new UserType()
Expand All @@ -872,7 +880,7 @@ public void test410NoDeputyQueryWhenThereAreNoAssignees() throws Exception {
r -> r.getTraces().stream().anyMatch(
t -> isUserNullDeputyRefSearch(t)));

then("role is not assigned, and a case is created");
then("user is not created but case exists");
assertNoUserByUsername(name);
assertCase(result, "after")
.display();
Expand Down Expand Up @@ -906,5 +914,84 @@ private boolean isUserNullDeputyRefSearch(TraceType t) {
&& refFilter.hasNoValue();
}

/** Checks that there are separate queries if multiple approver relations are searched for. MID-8134. */
@Test
public void test420SeparateRelationQueries() throws Exception {
Task task = getTestTask();
OperationResult result = task.getResult();
login(userAdministrator);

when("a user with role assignment is created");
String name = "test420";
UserType user = new UserType()
.name(name)
.assignment(ROLE_APPROVED_BY_MULTIPLE_RELATIONS.assignmentTo());

setTracing(task, createDefaultTracingProfile()); // just to get the whole operation result
addObject(user, task, result);
OperationResultRepoSearchAsserter.forResult(result)
.forEachRepoSearch(
r -> r.getTraces().forEach(
t -> checkNoMultiRelationsOrFilter(t)));

then("user is not created but case exists");
assertNoUserByUsername(name);
var workItem = assertCase(result, "after")
.display()
.subcases()
.assertSubcases(2)
.singleWithoutApprovalSchema().display().end() // user ADD
.singleWithApprovalSchema() // assignment ADD
.display()
.workItems()
.single()
.assertAssignees(USER_APPROVER_BY_MULTIPLE_RELATIONS.oid)
.getRealValue();

when("work item is approved");
approveWorkItem(workItem, task, result);

CaseType parentCase = getCase(CaseTypeUtil.getCaseRequired(workItem).getParentRef().getOid());
waitForCaseClose(parentCase);

then("user with assignment exists");
assertUserAfterByUsername(name)
.assignments()
.assertRole(ROLE_APPROVED_BY_MULTIPLE_RELATIONS.oid);
}

private void checkNoMultiRelationsOrFilter(TraceType t) {
if (!(t instanceof RepositorySearchObjectsTraceType)) {
return;
}
RepositorySearchObjectsTraceType trace = (RepositorySearchObjectsTraceType) t;
QueryType queryBean = trace.getQuery();
if (queryBean == null) {
return;
}
ObjectQuery query;
try {
query = prismContext.getQueryConverter().createObjectQuery(
prismContext.getSchemaRegistry().determineCompileTimeClass(trace.getObjectType()),
queryBean);
} catch (SchemaException e) {
throw new AssertionError(e);
}
ObjectFilter filter = query.getFilter();
if (filter == null) {
return;
}
filter.accept(f -> {
if (f instanceof OrFilter) {
Set<QName> relations = ((OrFilter) f).getConditions().stream()
.filter(cond -> cond instanceof RefFilter)
.flatMap(cond -> emptyIfNull(((RefFilter) cond).getValues()).stream())
.map(value -> value.getRelation())
.collect(Collectors.toSet());
if (relations.size() > 1) {
throw new AssertionError("Multi-relation filter found: " + relations + ", " + f);
}
}
});
}
}
28 changes: 28 additions & 0 deletions model/workflow-impl/src/test/resources/miscellaneous/role-approved-by-multiple-relations.xml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<!--
~ Copyright (C) 2010-2023 Evolveum and contributors
~
~ This work is dual-licensed under the Apache License 2.0
~ and European Union Public License. See LICENSE file for details.
-->

<role xmlns="http://midpoint.evolveum.com/xml/ns/public/common/common-3"
xmlns:org="http://midpoint.evolveum.com/xml/ns/public/common/org-3"
oid="62d7fcdf-92b0-4c49-ae40-33b0a814ed56">
<name>approved-by-multiple-relations</name>
<assignment>
<policyRule>
<policyConstraints>
<assignment>
<operation>add</operation>
</assignment>
</policyConstraints>
<policyActions>
<approval>
<approverRelation>org:approver</approverRelation>
<approverRelation>org:owner</approverRelation>
<approverRelation>org:test</approverRelation>
</approval>
</policyActions>
</policyRule>
</assignment>
</role>
18 changes: 18 additions & 0 deletions model/workflow-impl/src/test/resources/miscellaneous/user-approver-by-multiple-relations.xml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<!--
~ Copyright (C) 2010-2023 Evolveum and contributors
~
~ This work is dual-licensed under the Apache License 2.0
~ and European Union Public License. See LICENSE file for details.
-->

<user xmlns="http://midpoint.evolveum.com/xml/ns/public/common/common-3"
xmlns:org="http://midpoint.evolveum.com/xml/ns/public/common/org-3"
oid="a9aca7bb-923e-4be6-9aa4-5c90af978207">
<name>approver-by-multiple-relations</name>
<assignment>
<targetRef oid="62d7fcdf-92b0-4c49-ae40-33b0a814ed56" type="RoleType" relation="org:approver"/>
</assignment>
<assignment>
<targetRef oid="62d7fcdf-92b0-4c49-ae40-33b0a814ed56" type="RoleType" relation="org:owner"/>
</assignment>
</user>
8 changes: 7 additions & 1 deletion .../src/main/java/com/evolveum/midpoint/test/asserter/OperationResultRepoSearchAsserter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import java.util.ArrayList;
import java.util.List;
import java.util.function.Consumer;
import java.util.function.Predicate;

import org.testng.AssertJUnit;
Expand Down Expand Up @@ -38,7 +39,7 @@ public static OperationResultRepoSearchAsserter<Void> forResult(OperationResult
return new OperationResultRepoSearchAsserter<>(result, null, null);
}

List<OperationResult> getRepoSearches() {
private List<OperationResult> getRepoSearches() {
if (searchResults == null) {
searchResults = new ArrayList<>();
result.accept(subresult -> {
Expand All @@ -61,6 +62,11 @@ public OperationResultRepoSearchAsserter<RA> assertSize(int expected) {
return this;
}

public OperationResultRepoSearchAsserter<RA> forEachRepoSearch(Consumer<OperationResult> consumer) {
getRepoSearches().forEach(consumer);
return this;
}

public OperationResultRepoSearchAsserter<RA> assertContains(Predicate<OperationResult> predicate) {
if (!contains(predicate)) {
fail("Expected that search results will contain " + predicate.toString() + ", but they did not; in " + desc());
Expand Down

0 comments on commit 7e655c6

Please sign in to comment.