This is an automated email from the ASF dual-hosted git repository. ilgrosso pushed a commit to branch 2_0_X in repository https://gitbox.apache.org/repos/asf/syncope.git
The following commit(s) were added to refs/heads/2_0_X by this push: new a5f2bcd [SYNCOPE-1417] Raise exception when more than one plain attribute is requested for ordering a5f2bcd is described below commit a5f2bcd8509d6afba0a189a29ce8d71e35af123d Author: Francesco Chicchiriccò <ilgro...@apache.org> AuthorDate: Fri Dec 14 10:48:19 2018 +0100 [SYNCOPE-1417] Raise exception when more than one plain attribute is requested for ordering --- .../core/persistence/jpa/dao/JPAAnySearchDAO.java | 49 ++++++++++---- .../core/persistence/jpa/outer/AnySearchTest.java | 77 ++++++++++++++++------ .../org/apache/syncope/fit/core/SearchITCase.java | 20 +++++- 3 files changed, 110 insertions(+), 36 deletions(-) diff --git a/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPAAnySearchDAO.java b/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPAAnySearchDAO.java index 1fcd907..718b0bb 100644 --- a/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPAAnySearchDAO.java +++ b/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPAAnySearchDAO.java @@ -127,7 +127,7 @@ public class JPAAnySearchDAO extends AbstractAnySearchDAO { Pair<String, Set<String>> filter = getAdminRealmsFilter(adminRealms, svs, parameters); // 1. get the query string from the search condition - Pair<StringBuilder, Set<String>> queryInfo = + Pair<StringBuilder, Set<String>> queryInfo = getQuery(buildEffectiveCond(cond, filter.getRight()), parameters, svs); StringBuilder queryString = queryInfo.getLeft(); @@ -164,8 +164,8 @@ public class JPAAnySearchDAO extends AbstractAnySearchDAO { Pair<String, Set<String>> filter = getAdminRealmsFilter(adminRealms, svs, parameters); // 1. get the query string from the search condition - Pair<StringBuilder, Set<String>> queryInfo = getQuery(buildEffectiveCond(cond, filter.getRight()), - parameters, svs); + Pair<StringBuilder, Set<String>> queryInfo = + getQuery(buildEffectiveCond(cond, filter.getRight()), parameters, svs); StringBuilder queryString = queryInfo.getLeft(); @@ -197,6 +197,8 @@ public class JPAAnySearchDAO extends AbstractAnySearchDAO { // 6. Prepare the result (avoiding duplicates) return buildResult(query.getResultList(), kind); + } catch (SyncopeClientException e) { + throw e; } catch (Exception e) { LOG.error("While searching for {}", kind, e); } @@ -240,7 +242,9 @@ public class JPAAnySearchDAO extends AbstractAnySearchDAO { } private StringBuilder buildWhere( - final SearchSupport svs, final Set<String> involvedPlainAttrs, final OrderBySupport obs) { + final SearchSupport svs, + final Set<String> involvedPlainAttrs, + final OrderBySupport obs) { Set<String> attrs = new HashSet<>(involvedPlainAttrs); for (OrderBySupport.Item item : obs.items) { @@ -324,6 +328,9 @@ public class JPAAnySearchDAO extends AbstractAnySearchDAO { OrderBySupport obs = new OrderBySupport(); + Set<String> orderByUniquePlainSchemas = new HashSet<>(); + Set<String> orderByNonUniquePlainSchemas = new HashSet<>(); + for (OrderByClause clause : filterOrderBy(orderBy)) { OrderBySupport.Item item = new OrderBySupport.Item(); @@ -333,6 +340,20 @@ public class JPAAnySearchDAO extends AbstractAnySearchDAO { if (ReflectionUtils.findField(attrUtils.anyClass(), fieldName) == null) { PlainSchema schema = schemaDAO.find(fieldName); if (schema != null) { + if (schema.isUniqueConstraint()) { + orderByUniquePlainSchemas.add(schema.getKey()); + } else { + orderByNonUniquePlainSchemas.add(schema.getKey()); + } + if (orderByUniquePlainSchemas.size() > 1 || orderByNonUniquePlainSchemas.size() > 1) { + SyncopeClientException invalidSearch = + SyncopeClientException.build(ClientExceptionType.InvalidSearchExpression); + invalidSearch.getElements().add("Order by more than one attribute is not allowed; " + + "remove one from " + (orderByUniquePlainSchemas.size() > 1 + ? orderByUniquePlainSchemas : orderByNonUniquePlainSchemas)); + throw invalidSearch; + } + // keep track of involvement of non-mandatory schemas in the order by clauses obs.nonMandatorySchemas = !"true".equals(schema.getMandatoryCondition()); @@ -382,10 +403,12 @@ public class JPAAnySearchDAO extends AbstractAnySearchDAO { } private Pair<StringBuilder, Set<String>> getQuery( - final SearchCond cond, final List<Object> parameters, final SearchSupport svs) { - StringBuilder query = new StringBuilder(); - Set<String> involvedAttributes = new HashSet<>(); + final SearchCond cond, + final List<Object> parameters, + final SearchSupport svs) { + StringBuilder query = new StringBuilder(); + Set<String> involvedPlainAttrs = new HashSet<>(); switch (cond.getType()) { case LEAF: case NOT_LEAF: @@ -425,7 +448,7 @@ public class JPAAnySearchDAO extends AbstractAnySearchDAO { query.append(getQuery(cond.getAttributeCond(), cond.getType() == SearchCond.Type.NOT_LEAF, parameters, svs)); try { - involvedAttributes.add(check(cond.getAttributeCond(), svs.anyTypeKind).getLeft().getKey()); + involvedPlainAttrs.add(check(cond.getAttributeCond(), svs.anyTypeKind).getLeft().getKey()); } catch (IllegalArgumentException e) { // ignore } @@ -437,10 +460,10 @@ public class JPAAnySearchDAO extends AbstractAnySearchDAO { case AND: Pair<StringBuilder, Set<String>> leftAndInfo = getQuery(cond.getLeftSearchCond(), parameters, svs); - involvedAttributes.addAll(leftAndInfo.getRight()); + involvedPlainAttrs.addAll(leftAndInfo.getRight()); Pair<StringBuilder, Set<String>> rigthAndInfo = getQuery(cond.getRightSearchCond(), parameters, svs); - involvedAttributes.addAll(rigthAndInfo.getRight()); + involvedPlainAttrs.addAll(rigthAndInfo.getRight()); String andSubQuery = leftAndInfo.getKey().toString(); // Add extra parentheses @@ -453,10 +476,10 @@ public class JPAAnySearchDAO extends AbstractAnySearchDAO { case OR: Pair<StringBuilder, Set<String>> leftOrInfo = getQuery(cond.getLeftSearchCond(), parameters, svs); - involvedAttributes.addAll(leftOrInfo.getRight()); + involvedPlainAttrs.addAll(leftOrInfo.getRight()); Pair<StringBuilder, Set<String>> rigthOrInfo = getQuery(cond.getRightSearchCond(), parameters, svs); - involvedAttributes.addAll(rigthOrInfo.getRight()); + involvedPlainAttrs.addAll(rigthOrInfo.getRight()); String orSubQuery = leftOrInfo.getKey().toString(); // Add extra parentheses @@ -470,7 +493,7 @@ public class JPAAnySearchDAO extends AbstractAnySearchDAO { default: } - return Pair.of(query, involvedAttributes); + return Pair.of(query, involvedPlainAttrs); } private String getQuery( diff --git a/core/persistence-jpa/src/test/java/org/apache/syncope/core/persistence/jpa/outer/AnySearchTest.java b/core/persistence-jpa/src/test/java/org/apache/syncope/core/persistence/jpa/outer/AnySearchTest.java index 9d9d287..31c9e17 100644 --- a/core/persistence-jpa/src/test/java/org/apache/syncope/core/persistence/jpa/outer/AnySearchTest.java +++ b/core/persistence-jpa/src/test/java/org/apache/syncope/core/persistence/jpa/outer/AnySearchTest.java @@ -21,15 +21,21 @@ package org.apache.syncope.core.persistence.jpa.outer; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import java.util.ArrayList; import java.util.List; +import org.apache.syncope.common.lib.SyncopeClientException; import org.apache.syncope.common.lib.types.AnyTypeKind; +import org.apache.syncope.common.lib.types.ClientExceptionType; import org.apache.syncope.common.lib.types.StandardEntitlement; import org.apache.syncope.core.persistence.api.dao.GroupDAO; import org.apache.syncope.core.persistence.api.dao.RealmDAO; import org.apache.syncope.core.persistence.api.dao.RoleDAO; import org.apache.syncope.core.persistence.api.dao.AnySearchDAO; +import org.apache.syncope.core.persistence.api.dao.search.AnyCond; import org.apache.syncope.core.persistence.api.dao.search.AttributeCond; +import org.apache.syncope.core.persistence.api.dao.search.OrderByClause; import org.apache.syncope.core.persistence.api.dao.search.RoleCond; import org.apache.syncope.core.persistence.api.dao.search.SearchCond; import org.apache.syncope.core.persistence.api.entity.user.DynRoleMembership; @@ -57,27 +63,6 @@ public class AnySearchTest extends AbstractTest { private RoleDAO roleDAO; @Test - public void issueSYNCOPE95() { - for (Group group : groupDAO.findAll(1, 100)) { - groupDAO.delete(group.getKey()); - } - groupDAO.flush(); - - AttributeCond coolLeafCond = new AttributeCond(AttributeCond.Type.EQ); - coolLeafCond.setSchema("cool"); - coolLeafCond.setExpression("true"); - - SearchCond cond = SearchCond.getLeafCond(coolLeafCond); - assertTrue(cond.isValid()); - - List<User> users = searchDAO.search(cond, AnyTypeKind.USER); - assertNotNull(users); - assertEquals(1, users.size()); - - assertEquals("c9b2dec2-00a7-4855-97c0-d854842b4b24", users.get(0).getKey()); - } - - @Test public void searchByDynMembership() { // 1. create role with dynamic membership Role role = entityFactory.newEntity(Role.class); @@ -107,4 +92,54 @@ public class AnySearchTest extends AbstractTest { assertEquals(1, users.size()); assertEquals("c9b2dec2-00a7-4855-97c0-d854842b4b24", users.get(0).getKey()); } + + @Test + public void issueSYNCOPE95() { + for (Group group : groupDAO.findAll(1, 100)) { + groupDAO.delete(group.getKey()); + } + groupDAO.flush(); + + AttributeCond coolLeafCond = new AttributeCond(AttributeCond.Type.EQ); + coolLeafCond.setSchema("cool"); + coolLeafCond.setExpression("true"); + + SearchCond cond = SearchCond.getLeafCond(coolLeafCond); + assertTrue(cond.isValid()); + + List<User> users = searchDAO.search(cond, AnyTypeKind.USER); + assertNotNull(users); + assertEquals(1, users.size()); + + assertEquals("c9b2dec2-00a7-4855-97c0-d854842b4b24", users.get(0).getKey()); + } + + @Test + public void issueSYNCOPE1417() { + AnyCond usernameLeafCond = new AnyCond(AnyCond.Type.EQ); + usernameLeafCond.setSchema("username"); + usernameLeafCond.setExpression("rossini"); + AttributeCond idRightCond = new AttributeCond(AttributeCond.Type.LIKE); + idRightCond.setSchema("fullname"); + idRightCond.setExpression("Giuseppe V%"); + SearchCond searchCondition = SearchCond.getOrCond( + SearchCond.getLeafCond(usernameLeafCond), SearchCond.getLeafCond(idRightCond)); + + List<OrderByClause> orderByClauses = new ArrayList<>(); + OrderByClause orderByClause = new OrderByClause(); + orderByClause.setField("surname"); + orderByClause.setDirection(OrderByClause.Direction.DESC); + orderByClauses.add(orderByClause); + orderByClause = new OrderByClause(); + orderByClause.setField("firstname"); + orderByClause.setDirection(OrderByClause.Direction.ASC); + orderByClauses.add(orderByClause); + + try { + searchDAO.search(searchCondition, orderByClauses, AnyTypeKind.USER); + fail(); + } catch (SyncopeClientException e) { + assertEquals(ClientExceptionType.InvalidSearchExpression, e.getType()); + } + } } diff --git a/fit/core-reference/src/test/java/org/apache/syncope/fit/core/SearchITCase.java b/fit/core-reference/src/test/java/org/apache/syncope/fit/core/SearchITCase.java index 54b81f4..65003df 100644 --- a/fit/core-reference/src/test/java/org/apache/syncope/fit/core/SearchITCase.java +++ b/fit/core-reference/src/test/java/org/apache/syncope/fit/core/SearchITCase.java @@ -23,6 +23,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import java.util.Collection; import javax.ws.rs.core.Response; @@ -31,12 +32,12 @@ import org.apache.commons.collections4.IterableUtils; import org.apache.commons.collections4.Predicate; import org.apache.commons.lang3.RandomStringUtils; import org.apache.syncope.client.lib.SyncopeClient; +import org.apache.syncope.common.lib.SyncopeClientException; import org.apache.syncope.common.lib.SyncopeConstants; import org.apache.syncope.common.lib.patch.AnyObjectPatch; import org.apache.syncope.common.lib.patch.AttrPatch; import org.apache.syncope.common.lib.patch.MembershipPatch; import org.apache.syncope.common.lib.patch.UserPatch; -import org.apache.syncope.common.lib.search.SpecialAttr; import org.apache.syncope.common.lib.to.AnyObjectTO; import org.apache.syncope.common.lib.to.AnyTypeTO; import org.apache.syncope.common.lib.to.PagedResult; @@ -45,6 +46,7 @@ import org.apache.syncope.common.lib.to.MembershipTO; import org.apache.syncope.common.lib.to.RoleTO; import org.apache.syncope.common.lib.to.UserTO; import org.apache.syncope.common.lib.types.AnyTypeKind; +import org.apache.syncope.common.lib.types.ClientExceptionType; import org.apache.syncope.common.rest.api.beans.AnyQuery; import org.apache.syncope.common.rest.api.service.RoleService; import org.apache.syncope.fit.AbstractITCase; @@ -306,7 +308,7 @@ public class SearchITCase extends AbstractITCase { and("username").equalTo("bellini").query()). build()); assertEquals(users, issueSYNCOPE1321); - + // SYNCOPE-1416 (check the search for attributes of type different from stringvalue) PagedResult<UserTO> issueSYNCOPE1416 = userService.search(new AnyQuery.Builder(). realm(SyncopeConstants.ROOT_REALM). @@ -627,4 +629,18 @@ public class SearchITCase extends AbstractITCase { assertNotNull(groups); assertFalse(groups.getResult().isEmpty()); } + + @Test + public void issueSYNCOPE1417() { + try { + userService.search(new AnyQuery.Builder().realm(SyncopeConstants.ROOT_REALM). + fiql(SyncopeClient.getUserSearchConditionBuilder().is("userId").equalTo("*@apache.org").query()). + orderBy(SyncopeClient.getOrderByClauseBuilder().asc("surname").desc("firstname").build()).build()); + if (!ElasticsearchDetector.isElasticSearchEnabled(syncopeService)) { + fail(); + } + } catch (SyncopeClientException e) { + assertEquals(ClientExceptionType.InvalidSearchExpression, e.getType()); + } + } }