You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@syncope.apache.org by il...@apache.org on 2023/01/23 14:12:13 UTC
[syncope] branch master updated (ad267b8177 -> 086617475c)
This is an automated email from the ASF dual-hosted git repository.
ilgrosso pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/syncope.git
from ad267b8177 Upgrading Spring Boot and Nimbus JOSE JWT
new d254cc688b Upgrading Groovy
new 086617475c [SYNCOPE-1725] Accumulating OR and AND conditions before generating the final ES query
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../jpa/dao/ElasticsearchAnySearchDAO.java | 50 +++++-
.../jpa/dao/ElasticsearchAnySearchDAOTest.java | 199 ++++++++++++++++++++-
pom.xml | 2 +-
3 files changed, 238 insertions(+), 13 deletions(-)
[syncope] 01/02: Upgrading Groovy
Posted by il...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
ilgrosso pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/syncope.git
commit d254cc688b244fdd2e169b66558c58dbbb86ed5f
Author: Francesco Chicchiriccò <il...@apache.org>
AuthorDate: Mon Jan 23 08:27:59 2023 +0100
Upgrading Groovy
---
pom.xml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pom.xml b/pom.xml
index 0056338b55..db00bfdbeb 100644
--- a/pom.xml
+++ b/pom.xml
@@ -423,7 +423,7 @@ under the License.
<jasypt.version>1.9.3</jasypt.version>
- <groovy.version>4.0.7</groovy.version>
+ <groovy.version>4.0.8</groovy.version>
<flowable.version>7.0.0.M1</flowable.version>
<flowable-old.version>6.8.0</flowable-old.version>
[syncope] 02/02: [SYNCOPE-1725] Accumulating OR and AND conditions before generating the final ES query
Posted by il...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
ilgrosso pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/syncope.git
commit 086617475c46a089043c763122d2e0b49fb72821
Author: Francesco Chicchiriccò <il...@apache.org>
AuthorDate: Mon Jan 23 15:10:38 2023 +0100
[SYNCOPE-1725] Accumulating OR and AND conditions before generating the final ES query
---
.../jpa/dao/ElasticsearchAnySearchDAO.java | 50 +++++-
.../jpa/dao/ElasticsearchAnySearchDAOTest.java | 199 ++++++++++++++++++++-
2 files changed, 237 insertions(+), 12 deletions(-)
diff --git a/ext/elasticsearch/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/ElasticsearchAnySearchDAO.java b/ext/elasticsearch/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/ElasticsearchAnySearchDAO.java
index a04f980710..8fe9dd4388 100644
--- a/ext/elasticsearch/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/ElasticsearchAnySearchDAO.java
+++ b/ext/elasticsearch/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/ElasticsearchAnySearchDAO.java
@@ -24,13 +24,14 @@ import co.elastic.clients.elasticsearch._types.FieldValue;
import co.elastic.clients.elasticsearch._types.SearchType;
import co.elastic.clients.elasticsearch._types.SortOptions;
import co.elastic.clients.elasticsearch._types.SortOrder;
+import co.elastic.clients.elasticsearch._types.query_dsl.BoolQuery;
+import co.elastic.clients.elasticsearch._types.query_dsl.DisMaxQuery;
import co.elastic.clients.elasticsearch._types.query_dsl.Query;
import co.elastic.clients.elasticsearch._types.query_dsl.QueryBuilders;
import co.elastic.clients.elasticsearch.core.CountRequest;
import co.elastic.clients.elasticsearch.core.SearchRequest;
import co.elastic.clients.elasticsearch.core.search.Hit;
import co.elastic.clients.json.JsonData;
-import java.io.IOException;
import java.lang.reflect.Field;
import java.util.ArrayList;
import java.util.HashSet;
@@ -239,10 +240,12 @@ public class ElasticsearchAnySearchDAO extends AbstractAnySearchDAO {
index(ElasticsearchUtils.getAnyIndex(AuthContextUtils.getDomain(), kind)).
query(getQuery(base, recursive, adminRealms, cond, kind)).
build();
+ LOG.debug("Count JSON request: {}", request);
+
try {
return (int) client.count(request).count();
- } catch (IOException e) {
- LOG.error("Search error", e);
+ } catch (Exception e) {
+ LOG.error("While counting in Elasticsearch", e);
return 0;
}
}
@@ -304,6 +307,7 @@ public class ElasticsearchAnySearchDAO extends AbstractAnySearchDAO {
size(itemsPerPage < 0 ? indexMaxResultWindow : itemsPerPage).
sort(sortBuilders(kind, orderBy)).
build();
+ LOG.debug("Search JSON request: {}", request);
@SuppressWarnings("rawtypes")
List<Hit<Map>> esResult = null;
@@ -415,15 +419,43 @@ public class ElasticsearchAnySearchDAO extends AbstractAnySearchDAO {
break;
case AND:
- query = new Query.Builder().bool(QueryBuilders.bool().
- must(getQuery(cond.getLeft(), kind)).must(getQuery(cond.getRight(), kind)).build()).
- build();
+ List<Query> andCompound = new ArrayList<>();
+
+ Query andLeft = getQuery(cond.getLeft(), kind);
+ if (andLeft._kind() == Query.Kind.Bool && !((BoolQuery) andLeft._get()).must().isEmpty()) {
+ andCompound.addAll(((BoolQuery) andLeft._get()).must());
+ } else {
+ andCompound.add(andLeft);
+ }
+
+ Query andRight = getQuery(cond.getRight(), kind);
+ if (andRight._kind() == Query.Kind.Bool && !((BoolQuery) andRight._get()).must().isEmpty()) {
+ andCompound.addAll(((BoolQuery) andRight._get()).must());
+ } else {
+ andCompound.add(andRight);
+ }
+
+ query = new Query.Builder().bool(QueryBuilders.bool().must(andCompound).build()).build();
break;
case OR:
- query = new Query.Builder().disMax(QueryBuilders.disMax().
- queries(getQuery(cond.getLeft(), kind), getQuery(cond.getRight(), kind)).build()).
- build();
+ List<Query> orCompound = new ArrayList<>();
+
+ Query orLeft = getQuery(cond.getLeft(), kind);
+ if (orLeft._kind() == Query.Kind.DisMax) {
+ orCompound.addAll(((DisMaxQuery) orLeft._get()).queries());
+ } else {
+ orCompound.add(orLeft);
+ }
+
+ Query orRight = getQuery(cond.getRight(), kind);
+ if (orRight._kind() == Query.Kind.DisMax) {
+ orCompound.addAll(((DisMaxQuery) orRight._get()).queries());
+ } else {
+ orCompound.add(orRight);
+ }
+
+ query = new Query.Builder().disMax(QueryBuilders.disMax().queries(orCompound).build()).build();
break;
default:
diff --git a/ext/elasticsearch/persistence-jpa/src/test/java/org/apache/syncope/core/persistence/jpa/dao/ElasticsearchAnySearchDAOTest.java b/ext/elasticsearch/persistence-jpa/src/test/java/org/apache/syncope/core/persistence/jpa/dao/ElasticsearchAnySearchDAOTest.java
index 2145da00ac..02f4b7893f 100644
--- a/ext/elasticsearch/persistence-jpa/src/test/java/org/apache/syncope/core/persistence/jpa/dao/ElasticsearchAnySearchDAOTest.java
+++ b/ext/elasticsearch/persistence-jpa/src/test/java/org/apache/syncope/core/persistence/jpa/dao/ElasticsearchAnySearchDAOTest.java
@@ -21,11 +21,16 @@ package org.apache.syncope.core.persistence.jpa.dao;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import co.elastic.clients.elasticsearch._types.FieldValue;
import co.elastic.clients.elasticsearch._types.SearchType;
+import co.elastic.clients.elasticsearch._types.query_dsl.BoolQuery;
+import co.elastic.clients.elasticsearch._types.query_dsl.DisMaxQuery;
import co.elastic.clients.elasticsearch._types.query_dsl.Query;
import co.elastic.clients.elasticsearch._types.query_dsl.QueryBuilders;
import co.elastic.clients.elasticsearch.core.SearchRequest;
@@ -36,6 +41,7 @@ import java.util.Set;
import org.apache.commons.lang3.tuple.Triple;
import org.apache.syncope.common.lib.SyncopeConstants;
import org.apache.syncope.common.lib.types.AnyTypeKind;
+import org.apache.syncope.core.persistence.api.attrvalue.validation.PlainAttrValidationManager;
import org.apache.syncope.core.persistence.api.dao.DynRealmDAO;
import org.apache.syncope.core.persistence.api.dao.GroupDAO;
import org.apache.syncope.core.persistence.api.dao.RealmDAO;
@@ -46,6 +52,7 @@ import org.apache.syncope.core.persistence.api.entity.AnyUtils;
import org.apache.syncope.core.persistence.api.entity.AnyUtilsFactory;
import org.apache.syncope.core.persistence.api.entity.DynRealm;
import org.apache.syncope.core.persistence.api.entity.EntityFactory;
+import org.apache.syncope.core.persistence.api.entity.PlainAttrValue;
import org.apache.syncope.core.persistence.api.entity.PlainSchema;
import org.apache.syncope.core.persistence.api.entity.Realm;
import org.apache.syncope.core.persistence.jpa.entity.JPAPlainSchema;
@@ -81,6 +88,9 @@ public class ElasticsearchAnySearchDAOTest {
@Mock
private AnyUtilsFactory anyUtilsFactory;
+ @Mock
+ private PlainAttrValidationManager validator;
+
private ElasticsearchAnySearchDAO searchDAO;
@BeforeEach
@@ -94,7 +104,7 @@ public class ElasticsearchAnySearchDAOTest {
null,
entityFactory,
anyUtilsFactory,
- null,
+ validator,
null,
10000);
}
@@ -154,7 +164,7 @@ public class ElasticsearchAnySearchDAOTest {
public void searchRequest4groupOwner() throws IOException {
// 1. mock
AnyUtils anyUtils = mock(AnyUtils.class);
- when(anyUtils.getField("id")).thenReturn(ReflectionUtils.findField(JPAUser.class, "id"));
+ when(anyUtils.getField("key")).thenReturn(ReflectionUtils.findField(JPAUser.class, "id"));
when(anyUtils.newPlainAttrValue()).thenReturn(new JPAUPlainAttrValue());
when(anyUtilsFactory.getInstance(AnyTypeKind.USER)).thenReturn(anyUtils);
@@ -171,7 +181,7 @@ public class ElasticsearchAnySearchDAOTest {
Set<String> adminRealms = Set.of(RealmUtils.getGroupOwnerRealm("/any", "groupKey"));
AnyCond anyCond = new AnyCond(AttrCond.Type.ISNOTNULL);
- anyCond.setSchema("id");
+ anyCond.setSchema("key");
SearchRequest request = new SearchRequest.Builder().
index(ElasticsearchUtils.getAnyIndex(AuthContextUtils.getDomain(), AnyTypeKind.USER)).
@@ -191,4 +201,187 @@ public class ElasticsearchAnySearchDAOTest {
usingRecursiveComparison().isEqualTo(request.query());
}
}
+
+ @Test
+ public void issueSYNCOPE1725() throws IOException {
+ // 1. mock
+ AnyUtils anyUtils = mock(AnyUtils.class);
+ when(anyUtils.getField("key")).thenReturn(ReflectionUtils.findField(JPAUser.class, "id"));
+ JPAUPlainAttrValue value = new JPAUPlainAttrValue();
+ when(anyUtils.newPlainAttrValue()).thenReturn(value);
+
+ when(anyUtilsFactory.getInstance(AnyTypeKind.USER)).thenReturn(anyUtils);
+
+ when(entityFactory.newEntity(PlainSchema.class)).thenReturn(new JPAPlainSchema());
+
+ doAnswer(ic -> {
+ value.setStringValue(ic.getArgument(1));
+ return null;
+ }).when(validator).validate(any(PlainSchema.class), anyString(), any(PlainAttrValue.class));
+
+ AnyCond cond1 = new AnyCond(AttrCond.Type.EQ);
+ cond1.setSchema("key");
+ cond1.setExpression("1");
+
+ AnyCond cond2 = new AnyCond(AttrCond.Type.EQ);
+ cond2.setSchema("key");
+ cond2.setExpression("2");
+
+ AnyCond cond3 = new AnyCond(AttrCond.Type.EQ);
+ cond3.setSchema("key");
+ cond3.setExpression("3");
+
+ AnyCond cond4 = new AnyCond(AttrCond.Type.EQ);
+ cond4.setSchema("key");
+ cond4.setExpression("4");
+
+ AnyCond cond5 = new AnyCond(AttrCond.Type.EQ);
+ cond5.setSchema("key");
+ cond5.setExpression("5");
+
+ AnyCond cond6 = new AnyCond(AttrCond.Type.EQ);
+ cond6.setSchema("key");
+ cond6.setExpression("6");
+
+ try (MockedStatic<ElasticsearchUtils> utils = Mockito.mockStatic(ElasticsearchUtils.class)) {
+ utils.when(() -> ElasticsearchUtils.getAnyIndex(
+ SyncopeConstants.MASTER_DOMAIN, AnyTypeKind.USER)).thenReturn("master_user");
+
+ Query query = searchDAO.getQuery(
+ SearchCond.getAnd(
+ List.of(SearchCond.getLeaf(cond1),
+ SearchCond.getLeaf(cond2),
+ SearchCond.getLeaf(cond3),
+ SearchCond.getLeaf(cond4),
+ SearchCond.getLeaf(cond5),
+ SearchCond.getLeaf(cond6))),
+ AnyTypeKind.USER);
+ assertEquals(Query.Kind.Bool, query._kind());
+ assertEquals(6, ((BoolQuery) query._get()).must().size());
+ assertThat(
+ new Query.Builder().bool(QueryBuilders.bool().
+ must(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("1").build()).build()).
+ must(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("2").build()).build()).
+ must(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("3").build()).build()).
+ must(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("4").build()).build()).
+ must(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("5").build()).build()).
+ must(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("6").build()).build()).
+ build()).build()).
+ usingRecursiveComparison().isEqualTo(query);
+
+ query = searchDAO.getQuery(
+ SearchCond.getOr(
+ List.of(SearchCond.getLeaf(cond1),
+ SearchCond.getLeaf(cond2),
+ SearchCond.getLeaf(cond3),
+ SearchCond.getLeaf(cond4),
+ SearchCond.getLeaf(cond5),
+ SearchCond.getLeaf(cond6))),
+ AnyTypeKind.USER);
+ assertEquals(Query.Kind.DisMax, query._kind());
+ assertEquals(6, ((DisMaxQuery) query._get()).queries().size());
+ assertThat(
+ new Query.Builder().disMax(QueryBuilders.disMax().
+ queries(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("1").build()).build()).
+ queries(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("2").build()).build()).
+ queries(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("3").build()).build()).
+ queries(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("4").build()).build()).
+ queries(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("5").build()).build()).
+ queries(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("6").build()).build()).
+ build()).build()).
+ usingRecursiveComparison().isEqualTo(query);
+
+ query = searchDAO.getQuery(
+ SearchCond.getAnd(List.of(
+ SearchCond.getOr(List.of(
+ SearchCond.getLeaf(cond1),
+ SearchCond.getLeaf(cond2),
+ SearchCond.getLeaf(cond3))),
+ SearchCond.getOr(List.of(
+ SearchCond.getLeaf(cond4),
+ SearchCond.getLeaf(cond5),
+ SearchCond.getLeaf(cond6))))),
+ AnyTypeKind.USER);
+ assertEquals(Query.Kind.Bool, query._kind());
+ assertEquals(2, ((BoolQuery) query._get()).must().size());
+ Query left = ((BoolQuery) query._get()).must().get(0);
+ assertEquals(Query.Kind.DisMax, left._kind());
+ assertEquals(3, ((DisMaxQuery) left._get()).queries().size());
+ Query right = ((BoolQuery) query._get()).must().get(1);
+ assertEquals(Query.Kind.DisMax, right._kind());
+ assertEquals(3, ((DisMaxQuery) right._get()).queries().size());
+ assertThat(
+ new Query.Builder().bool(QueryBuilders.bool().
+ must(new Query.Builder().disMax(QueryBuilders.disMax().
+ queries(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("1").build()).build()).
+ queries(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("2").build()).build()).
+ queries(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("3").build()).build()).build()).
+ build()).
+ must(new Query.Builder().disMax(QueryBuilders.disMax().
+ queries(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("4").build()).build()).
+ queries(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("5").build()).build()).
+ queries(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("6").build()).build()).build()).
+ build()).
+ build()).build()).
+ usingRecursiveComparison().isEqualTo(query);
+
+ query = searchDAO.getQuery(
+ SearchCond.getOr(List.of(
+ SearchCond.getAnd(List.of(
+ SearchCond.getLeaf(cond1),
+ SearchCond.getLeaf(cond2),
+ SearchCond.getLeaf(cond3))),
+ SearchCond.getAnd(List.of(
+ SearchCond.getLeaf(cond4),
+ SearchCond.getLeaf(cond5),
+ SearchCond.getLeaf(cond6))))),
+ AnyTypeKind.USER);
+ assertEquals(Query.Kind.DisMax, query._kind());
+ assertEquals(2, ((DisMaxQuery) query._get()).queries().size());
+ left = ((DisMaxQuery) query._get()).queries().get(0);
+ assertEquals(Query.Kind.Bool, left._kind());
+ assertEquals(3, ((BoolQuery) left._get()).must().size());
+ right = ((DisMaxQuery) query._get()).queries().get(1);
+ assertEquals(Query.Kind.Bool, right._kind());
+ assertEquals(3, ((BoolQuery) right._get()).must().size());
+ assertThat(
+ new Query.Builder().disMax(QueryBuilders.disMax().
+ queries(new Query.Builder().bool(QueryBuilders.bool().
+ must(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("1").build()).build()).
+ must(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("2").build()).build()).
+ must(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("3").build()).build()).build()).
+ build()).
+ queries(new Query.Builder().bool(QueryBuilders.bool().
+ must(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("4").build()).build()).
+ must(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("5").build()).build()).
+ must(new Query.Builder().term(
+ QueryBuilders.term().field("id").value("6").build()).build()).build()).
+ build()).
+ build()).build()).
+ usingRecursiveComparison().isEqualTo(query);
+ }
+ }
}