Skip to content

Commit

Permalink
Remove matching rules in shadow repo queries
Browse files Browse the repository at this point in the history 
These matching rules (namely stringIgnoreCase) are harmful: They can
lead to the use of SQL query functions like lower() that prohibit
the use of indices, therefore worsening the performance.

These matching rules can be safely avoided, because the attribute values
are normalized before storing in the repository.

See MID-5547.
  • Loading branch information
@mederly
mederly committed Sep 11, 2019
1 parent e7b0bf3 commit d9b179e
Show file tree
Hide file tree
Showing 9 changed files with 3 additions and 117 deletions.
6 changes: 2 additions & 4 deletions ...ioning-impl/src/main/java/com/evolveum/midpoint/provisioning/impl/ConstraintsChecker.java
View file
Expand Up @@ -216,10 +216,8 @@ private boolean checkUniqueness(String oid, PrismProperty identifier, ObjectQuer
return true;
}

// Here was an attempt to call cacheRepositoryService.searchObjects directly (because we use noFetch, so the net result is searching in repo anyway).
// The idea was that it is faster and cacheable. However, it is not correct. We have to apply definition to query before execution, e.g.
// because there could be a matching rule; see ShadowManager.processQueryMatchingRuleFilter.
// Besides that, now the constraint checking is cached at a higher level, so this is not a big issue any more.
// Note that we should not call repository service directly here. The query values need to be normalized according to
// attribute matching rules.
Collection<SelectorOptions<GetOperationOptions>> options = SelectorOptions.createCollection(GetOperationOptions.createNoFetch());
List<PrismObject<ShadowType>> foundObjects = shadowCache.searchObjects(query, options, task, result);
LOGGER.trace("Uniqueness check of {} resulted in {} results:\n{}\nquery:\n{}",
Expand Down
7 changes: 0 additions & 7 deletions ...rovisioning-impl/src/main/java/com/evolveum/midpoint/provisioning/impl/ShadowManager.java
View file
Expand Up @@ -685,13 +685,6 @@ private <T> void processQueryMatchingRuleFilter(ObjectFilter filter, RefinedObje
LOGGER.trace("Replacing values for attribute {} in search filter with normalized values because there is a matching rule, normalized values: {}",
attrName, newValues);
}
if (eqFilter.getMatchingRule() == null) {
QName supportedMatchingRule = valueClass != null ?
repositoryService.getApproximateSupportedMatchingRule(valueClass, matchingRuleQName) : matchingRuleQName;
eqFilter.setMatchingRule(supportedMatchingRule);
LOGGER.trace("Setting matching rule to {} (supported by repo as a replacement for {} to search for {})",
supportedMatchingRule, matchingRuleQName, valueClass);
}
}

// Used when new resource object is discovered
Expand Down
20 changes: 0 additions & 20 deletions repo/repo-api/src/main/java/com/evolveum/midpoint/repo/api/RepositoryService.java
View file
Expand Up @@ -641,26 +641,6 @@ <T extends ShadowType> List<PrismObject<T>> listResourceObjectShadows(String res
<O extends ObjectType> boolean selectorMatches(ObjectSelectorType objectSelector, PrismObject<O> object,
ObjectFilterExpressionEvaluator filterEvaluator, Trace logger, String logMessagePrefix) throws SchemaException, ObjectNotFoundException, ExpressionEvaluationException, CommunicationException, ConfigurationException, SecurityViolationException;

/**
* Returns matching rule supported by the repository for a given data type (String, PolyString, ...), for
* originally intended matching rule.
*
* New matching rule must NOT be less selective than the original one. I.e. if values V1, V2 would not match
* under the original one, they must not also match under the replacement. Therefore it is safe to replace
* distinguishedName with stringIgnoreCase (but not e.g. the other way around; nor exchangeEmailAddresses
* can be replaced by stringIgnoreCase, because the prefix part is case sensitive).
*
* The assumption is that for unsupported matching rules the repository will store normalized values. And it
* will normalize any values that are used in queries. This is the obligation of the client. So, theoretically,
* it is safe to replace any such matching rule with default (exact) matching rule. But if we replace it with
* something that does not return false positives (i.e. something that is not less sensitive), we get some
* resiliency w.r.t. non-normalized values in repository. TODO TODO TODO think again
*
* If the original matching rule is not supported by the given data type (e.g. trying to use exchangeEmailAddress
* on PolyString), the result may be arbitrary. TODO think again also about this
*/
QName getApproximateSupportedMatchingRule(Class<?> dataType, QName originalMatchingRule);

void applyFullTextSearchConfiguration(FullTextSearchConfigurationType fullTextSearch);

FullTextSearchConfigurationType getFullTextSearchConfiguration();
Expand Down
11 changes: 0 additions & 11 deletions repo/repo-cache/src/main/java/com/evolveum/midpoint/repo/cache/RepositoryCache.java
View file
Expand Up @@ -33,7 +33,6 @@
import org.springframework.stereotype.Component;

import javax.annotation.PreDestroy;
import javax.xml.namespace.QName;
import java.util.Objects;
import java.util.*;
import java.util.concurrent.ConcurrentHashMap;
Expand Down Expand Up @@ -1650,16 +1649,6 @@ public RepositoryQueryDiagResponse executeQueryDiagnostics(RepositoryQueryDiagRe
}
}

@Override
public QName getApproximateSupportedMatchingRule(Class<?> dataType, QName originalMatchingRule) {
Long startTime = repoOpStart();
try {
return repositoryService.getApproximateSupportedMatchingRule(dataType, originalMatchingRule);
} finally {
repoOpEnd(startTime);
}
}

@Override
public void applyFullTextSearchConfiguration(FullTextSearchConfigurationType fullTextSearch) {
Long startTime = repoOpStart();
Expand Down
16 changes: 1 addition & 15 deletions .../repo-sql-impl/src/main/java/com/evolveum/midpoint/repo/sql/SqlRepositoryServiceImpl.java
View file
Expand Up @@ -68,9 +68,6 @@
import com.evolveum.midpoint.repo.sql.helpers.ObjectUpdater;
import com.evolveum.midpoint.repo.sql.helpers.OrgClosureManager;
import com.evolveum.midpoint.repo.sql.helpers.SequenceHelper;
import com.evolveum.midpoint.repo.sql.query2.matcher.DefaultMatcher;
import com.evolveum.midpoint.repo.sql.query2.matcher.PolyStringMatcher;
import com.evolveum.midpoint.repo.sql.query2.matcher.StringMatcher;
import com.evolveum.midpoint.schema.GetOperationOptions;
import com.evolveum.midpoint.schema.LabeledString;
import com.evolveum.midpoint.schema.RelationRegistry;
Expand Down Expand Up @@ -1214,18 +1211,7 @@ public <O extends ObjectType> boolean isAncestor(PrismObject<O> object, String o
return isAnySubordinate(object.getOid(), oidList);
}

@Override
public QName getApproximateSupportedMatchingRule(Class<?> dataType, QName originalMatchingRule) {
if (String.class.equals(dataType)) {
return StringMatcher.getApproximateSupportedMatchingRule(originalMatchingRule);
} else if (PolyString.class.equals(dataType) || PolyStringType.class.equals(dataType)) {
return PolyStringMatcher.getApproximateSupportedMatchingRule(originalMatchingRule);
} else {
return DefaultMatcher.getApproximateSupportedMatchingRule(originalMatchingRule);
}
}

@Override
@Override
public void applyFullTextSearchConfiguration(FullTextSearchConfigurationType fullTextSearch) {
LOGGER.info("Applying full text search configuration ({} entries)",
fullTextSearch != null ? fullTextSearch.getIndexed().size() : 0);
Expand Down
4 changes: 0 additions & 4 deletions ...-sql-impl/src/main/java/com/evolveum/midpoint/repo/sql/query2/matcher/DefaultMatcher.java
View file
Expand Up @@ -25,8 +25,4 @@ public Condition match(RootHibernateQuery hibernateQuery, ItemRestrictionOperati

return basicMatch(hibernateQuery, operation, propertyName, value, false);
}

public static QName getApproximateSupportedMatchingRule(QName originalMatchingRule) {
return originalMatchingRule; // TODO ok?
}
}
13 changes: 0 additions & 13 deletions repo/repo-sql-impl/src/main/java/com/evolveum/midpoint/repo/sql/query2/matcher/Matcher.java
View file
Expand Up @@ -29,19 +29,6 @@ public abstract class Matcher<T> {

private static final Trace LOGGER = TraceManager.getTrace(Matcher.class);

/**
* Create hibernate {@link Criterion} based on matcher defined in filter.
*
*
* @param hibernateQuery
* @param operation
* @param propertyPath
* @param value
* @param matcher Now type of {@link String}, but will be updated to {@link javax.xml.namespace.QName}
* type after query-api update
* @return
* @throws QueryException
*/
public abstract Condition match(RootHibernateQuery hibernateQuery, ItemRestrictionOperation operation, String propertyPath, T value, String matcher)
throws QueryException;

Expand Down
22 changes: 0 additions & 22 deletions ...l-impl/src/main/java/com/evolveum/midpoint/repo/sql/query2/matcher/PolyStringMatcher.java
View file
Expand Up @@ -38,24 +38,6 @@ public class PolyStringMatcher extends Matcher<PolyString> {
public static final String ORIG_IGNORE_CASE = "origIgnoreCase";
public static final String NORM_IGNORE_CASE = "normIgnoreCase";

private static final List<QName> SUPPORTED_MATCHING_RULES = Arrays
.asList(PrismConstants.DEFAULT_MATCHING_RULE_NAME,
PrismConstants.POLY_STRING_STRICT_MATCHING_RULE_NAME,
PrismConstants.POLY_STRING_ORIG_MATCHING_RULE_NAME,
PrismConstants.POLY_STRING_NORM_MATCHING_RULE_NAME,
new QName(STRICT_IGNORE_CASE),
new QName(ORIG_IGNORE_CASE),
new QName(NORM_IGNORE_CASE));
private static final Map<QName, QName> MATCHING_RULES_CONVERGENCE_MAP = new HashMap<>();
static {
// Nothing here - the below (String-specific) matching rules should NOT be used for polystrings
// TODO think again ... currently the approximate matching rule is the same as original one
// MATCHING_RULES_CONVERGENCE_MAP.put(DistinguishedNameMatchingRule.NAME, PolyStringNormMatchingRule.NAME); //ok?
// MATCHING_RULES_CONVERGENCE_MAP.put(ExchangeEmailAddressesMatchingRule.NAME, PolyStringNormMatchingRule.NAME); //ok?
// MATCHING_RULES_CONVERGENCE_MAP.put(UuidMatchingRule.NAME, PolyStringNormMatchingRule.NAME);
// MATCHING_RULES_CONVERGENCE_MAP.put(XmlMatchingRule.NAME, DefaultMatchingRule.NAME); //ok?
}

@Override
public Condition match(RootHibernateQuery hibernateQuery, ItemRestrictionOperation operation, String propertyName, PolyString value, String matcher)
throws QueryException {
Expand Down Expand Up @@ -92,8 +74,4 @@ private Condition createOrigMatch(RootHibernateQuery hibernateQuery, ItemRestric
String realValue = value != null ? value.getOrig() : null;
return basicMatch(hibernateQuery, operation, propertyName + '.' + RPolyString.F_ORIG, realValue, ignoreCase);
}

public static QName getApproximateSupportedMatchingRule(QName originalMatchingRule) {
return Matcher.getApproximateSupportedMatchingRule(originalMatchingRule, SUPPORTED_MATCHING_RULES, MATCHING_RULES_CONVERGENCE_MAP);
}
}
21 changes: 0 additions & 21 deletions ...o-sql-impl/src/main/java/com/evolveum/midpoint/repo/sql/query2/matcher/StringMatcher.java
View file
Expand Up @@ -16,12 +16,6 @@
import com.evolveum.midpoint.util.logging.TraceManager;
import org.apache.commons.lang3.StringUtils;

import javax.xml.namespace.QName;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

/**
* @author lazyman
*/
Expand All @@ -33,17 +27,6 @@ public class StringMatcher extends Matcher<String> {
public static final String IGNORE_CASE = PrismConstants.STRING_IGNORE_CASE_MATCHING_RULE_NAME.getLocalPart();
public static final String DEFAULT = PrismConstants.DEFAULT_MATCHING_RULE_NAME.getLocalPart();

private static final List<QName> SUPPORTED_MATCHING_RULES = Arrays.asList(PrismConstants.DEFAULT_MATCHING_RULE_NAME, PrismConstants.STRING_IGNORE_CASE_MATCHING_RULE_NAME);
private static final Map<QName, QName> MATCHING_RULES_CONVERGENCE_MAP = new HashMap<>();
static {
MATCHING_RULES_CONVERGENCE_MAP.put(PrismConstants.DISTINGUISHED_NAME_MATCHING_RULE_NAME, PrismConstants.DEFAULT_MATCHING_RULE_NAME); // temporary code (TODO change in 3.6)
MATCHING_RULES_CONVERGENCE_MAP.put(PrismConstants.UUID_MATCHING_RULE_NAME, PrismConstants.DEFAULT_MATCHING_RULE_NAME); // temporary code (TODO change in 3.6)
//MATCHING_RULES_CONVERGENCE_MAP.put(DistinguishedNameMatchingRule.NAME, StringIgnoreCaseMatchingRule.NAME);
//MATCHING_RULES_CONVERGENCE_MAP.put(UuidMatchingRule.NAME, StringIgnoreCaseMatchingRule.NAME);
MATCHING_RULES_CONVERGENCE_MAP.put(PrismConstants.EXCHANGE_EMAIL_ADDRESSES_MATCHING_RULE_NAME, PrismConstants.DEFAULT_MATCHING_RULE_NAME); // prefix is case sensitive
MATCHING_RULES_CONVERGENCE_MAP.put(PrismConstants.XML_MATCHING_RULE_NAME, PrismConstants.DEFAULT_MATCHING_RULE_NAME);
}

@Override
public Condition match(RootHibernateQuery hibernateQuery, ItemRestrictionOperation operation, String propertyName, String value, String matcher)
throws QueryException {
Expand All @@ -64,8 +47,4 @@ public Condition match(RootHibernateQuery hibernateQuery, ItemRestrictionOperati

return basicMatch(hibernateQuery, operation, propertyName, value, ignoreCase);
}

public static QName getApproximateSupportedMatchingRule(QName originalMatchingRule) {
return Matcher.getApproximateSupportedMatchingRule(originalMatchingRule, SUPPORTED_MATCHING_RULES, MATCHING_RULES_CONVERGENCE_MAP);
}
}

0 comments on commit d9b179e

Please sign in to comment.