fabriziofortino commented on a change in pull request #477:
URL: https://github.com/apache/jackrabbit-oak/pull/477#discussion_r795475976
##########
File path:
oak-core/src/test/java/org/apache/jackrabbit/oak/query/AbstractQueryTest.java
##########
@@ -312,10 +316,27 @@ protected void assertResultSize(String query, String
language, long expected) {
protected List<String> assertQuery(String sql, String language,
List<String> expected, boolean
skipSort) {
+ return assertQuery(sql, language, expected, skipSort, false);
+ }
+
+ protected List<String> assertQuery(String sql, String language,
+ List<String> expected, boolean
skipSort, boolean checkSort) {
List<String> paths = executeQuery(sql, language, true, skipSort);
- assertResult(expected, paths);
+ if (checkSort) {
+ assertResult_Sorted(expected, paths);
+ } else {
+ assertResult(expected, paths);
+ }
return paths;
+ }
+ protected static void assertResult_Sorted(@NotNull List<String> expected,
@NotNull List<String> actual) {
Review comment:
we should only use camelcase notation
```suggestion
protected static void assertResultSorted(@NotNull List<String> expected,
@NotNull List<String> actual) {
```
##########
File path:
oak-core/src/test/java/org/apache/jackrabbit/oak/query/AbstractQueryTest.java
##########
@@ -108,17 +108,21 @@ protected void createTestIndexNode() throws Exception {
root.commit();
}
- protected static Tree createTestIndexNode(Tree index, String type)
- throws Exception {
+ protected static Tree createTestIndexNode(String indexName, Tree index,
String type) {
Tree indexDef = index.addChild(INDEX_DEFINITIONS_NAME).addChild(
- TEST_INDEX_NAME);
+ indexName);
indexDef.setProperty(JcrConstants.JCR_PRIMARYTYPE,
INDEX_DEFINITIONS_NODE_TYPE, Type.NAME);
indexDef.setProperty(TYPE_PROPERTY_NAME, type);
indexDef.setProperty(REINDEX_PROPERTY_NAME, true);
return indexDef;
}
+ protected static Tree createTestIndexNode(Tree index, String type)
+ throws Exception {
Review comment:
I guess this can be removed
```suggestion
protected static Tree createTestIndexNode(Tree index, String type) {
```
##########
File path:
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticDocumentMaker.java
##########
@@ -138,22 +142,33 @@ protected boolean
isFulltextValuePersistedAtNode(PropertyDefinition pd) {
@Override
protected void indexTypedProperty(ElasticDocument doc, PropertyState
property, String pname, PropertyDefinition pd, int i) {
- int tag = property.getType().tag();
-
+ // Get the Type tag from the defined index definition here - and not
from the actual persisted property state - this way in case
+ // If the actual property value is different from the property type
defined in the index definition/mapping - this will try to convert the property
if possible,
+ // other wise will log a warning and not try and add the property to
index. If we try and index incompatible data types (like String to Date),
Review comment:
typo
```suggestion
// otherwise will log a warning and not try and add the property to
index. If we try and index incompatible data types (like String to Date),
```
##########
File path:
oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexQueryCommonTest.java
##########
@@ -634,6 +637,103 @@ public void testEqualityQuery_native() throws Exception {
});
}
+ @Test
+ public void testDateQueryWithIncorrectData() throws Exception {
+ Tree test = root.getTree("/").addChild("test");
+ test.addChild("test1").setProperty("propDate", "foo");
+ test.getChild("test1").setProperty("propa", "bar");
+ test.addChild("test2").setProperty("propDate",
"2021-01-22T01:02:03.000Z", Type.DATE);
+ test.addChild("test2").setProperty("propa", "bar");
+ test.addChild("test3").setProperty("propDate",
"2022-01-22T01:02:03.000Z", Type.DATE);
+ root.commit();
+
+ // Query on propa should work fine even if the data on propDate is of
incorrect type (i.e String instead of Date)
+ // It should return both /test/test1 -> where content for propDate is
of incorrect data type
+ // and /test/test2 -> where content for propDate is of correct data
type.
+ String query = "/jcr:root/test//*[propa='bar']";
+ assertEventually(() -> {
+ assertQuery(query, XPATH, Arrays.asList("/test/test2",
"/test/test1"));
+ });
+
+ // Check inequality query on propDate - this should not return
/test/test1 -> since that node should not have been indexed for propDate
+ // due to incorrect data type in the content for this property.
+ String query2 =
"/jcr:root/test//*[propDate!='2021-01-22T01:02:03.000Z']";
+ assertEventually(() -> {
+ assertQuery(query2, XPATH, Arrays.asList("/test/test3"));
+ });
+ }
+
+ @Test
+ public void testQueryWithDifferentDataTypesForSameProperty() throws
Exception {
+ // propa doesn't have any type defined in index - so by default it's a
String type property
+ Tree test = root.getTree("/").addChild("test");
+ test.addChild("test1").setProperty("propa", "bar");
+ test.addChild("test2").setProperty("propa", 10);
+ test.addChild("test3").setProperty("propa", 10L);
+ test.addChild("test4").setProperty("propa", true);
+ root.commit();
+
+ // Below queries will ensure propa is searchable with different data
types as content and behaviour is similar for lucene and elastic.
+ String query = "/jcr:root/test//*[propa='bar']";
+ assertEventually(() -> {
+ assertQuery(query, XPATH, Arrays.asList("/test/test1"));
+ });
+
+ String query2 = "/jcr:root/test//*[propa=true]";
+ assertEventually(() -> {
+ assertQuery(query2, XPATH, Arrays.asList("/test/test4"));
+ });
+
+ String query3 = "/jcr:root/test//*[propa=10]";
+ assertEventually(() -> {
+ assertQuery(query3, XPATH, Arrays.asList("/test/test2",
"/test/test3"));
+ });
+
+
+
+
+ }
+
+ @Test
+ public void testDateQueryWithCorrectData() throws Exception {
+ Tree test = root.getTree("/").addChild("test");
+ test.addChild("test1").setProperty("propa", "foo");
+ test.getChild("test1").setProperty("propDate",
"2021-01-22T01:02:03.000Z", Type.DATE);
+ test.addChild("test2").setProperty("propa", "foo");
+ root.commit();
+
+ // Test query returns correct node on querying on dateProp
+ String query =
"/jcr:root/test//*[propDate='2021-01-22T01:02:03.000Z']";
+ assertEventually(() -> {
+ assertQuery(query, XPATH, Arrays.asList("/test/test1"));
+ });
+
+ // Test query returns correct node on querying on String type property
+ String query2 = "/jcr:root/test//*[propa='foo']";
+ assertEventually(() -> {
+ assertQuery(query2, XPATH, Arrays.asList("/test/test1",
"/test/test2"));
+ });
+ }
+
+ @Test
+ public void testDateQueryWithCorrectData_Ordered() throws Exception {
+ Tree test = root.getTree("/").addChild("test");
+ test.addChild("test1").setProperty("propa", "foo");
+ test.getChild("test1").setProperty("propDate",
"2021-01-22T01:02:03.000Z", Type.DATE);
+ test.addChild("test2").setProperty("propa", "foo");
+ test.addChild("test2").setProperty("propDate",
"2019-01-22T01:02:03.000Z", Type.DATE);
+ test.addChild("test3").setProperty("propa", "foo");
+ test.addChild("test3").setProperty("propDate",
"2020-01-22T01:02:03.000Z", Type.DATE);
+ root.commit();
+
+ // Test query returns correct node on querying on dateProp
+ String query = "/jcr:root/test//*[propa='foo'] order by @propDate
descending";
+ assertEventually(() -> {
+ assertQuery(query, XPATH, Arrays.asList("/test/test1",
"/test/test3", "/test/test2"), true, true);
+ });
+ }
+
+
Review comment:
remove extra newlines
```suggestion
```
##########
File path:
oak-core/src/test/java/org/apache/jackrabbit/oak/query/AbstractQueryTest.java
##########
@@ -108,17 +108,21 @@ protected void createTestIndexNode() throws Exception {
root.commit();
}
- protected static Tree createTestIndexNode(Tree index, String type)
- throws Exception {
+ protected static Tree createTestIndexNode(String indexName, Tree index,
String type) {
Review comment:
nit: remove extra space
```suggestion
protected static Tree createTestIndexNode(String indexName, Tree index,
String type) {
```
##########
File path:
oak-core/src/test/java/org/apache/jackrabbit/oak/query/AbstractQueryTest.java
##########
@@ -312,10 +316,27 @@ protected void assertResultSize(String query, String
language, long expected) {
protected List<String> assertQuery(String sql, String language,
List<String> expected, boolean
skipSort) {
+ return assertQuery(sql, language, expected, skipSort, false);
+ }
+
+ protected List<String> assertQuery(String sql, String language,
+ List<String> expected, boolean
skipSort, boolean checkSort) {
List<String> paths = executeQuery(sql, language, true, skipSort);
- assertResult(expected, paths);
+ if (checkSort) {
+ assertResult_Sorted(expected, paths);
+ } else {
+ assertResult(expected, paths);
+ }
return paths;
+ }
+ protected static void assertResult_Sorted(@NotNull List<String> expected,
@NotNull List<String> actual) {
+ assertEquals("Result set size is different: " + actual,
expected.size(),
+ actual.size());
+
+ for (int i = 0; i < expected.size(); i++) {
+ assertEquals("Expected sorted result not found", expected.get(i),
actual.get(i));
+ }
Review comment:
can we avoid checking every single entry and just do an `assertEquals`
on the lists?
##########
File path:
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticDocumentMaker.java
##########
@@ -30,12 +30,16 @@
import org.apache.jackrabbit.oak.spi.state.NodeState;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
import java.io.IOException;
import java.util.List;
public class ElasticDocumentMaker extends
FulltextDocumentMaker<ElasticDocument> {
+ private static final Logger log =
LoggerFactory.getLogger(ElasticDocumentMaker.class);
Review comment:
```suggestion
private static final Logger LOG =
LoggerFactory.getLogger(ElasticDocumentMaker.class);
```
##########
File path:
oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexQueryCommonTest.java
##########
@@ -634,6 +637,103 @@ public void testEqualityQuery_native() throws Exception {
});
}
+ @Test
+ public void testDateQueryWithIncorrectData() throws Exception {
+ Tree test = root.getTree("/").addChild("test");
+ test.addChild("test1").setProperty("propDate", "foo");
+ test.getChild("test1").setProperty("propa", "bar");
+ test.addChild("test2").setProperty("propDate",
"2021-01-22T01:02:03.000Z", Type.DATE);
+ test.addChild("test2").setProperty("propa", "bar");
+ test.addChild("test3").setProperty("propDate",
"2022-01-22T01:02:03.000Z", Type.DATE);
+ root.commit();
+
+ // Query on propa should work fine even if the data on propDate is of
incorrect type (i.e String instead of Date)
+ // It should return both /test/test1 -> where content for propDate is
of incorrect data type
+ // and /test/test2 -> where content for propDate is of correct data
type.
+ String query = "/jcr:root/test//*[propa='bar']";
+ assertEventually(() -> {
+ assertQuery(query, XPATH, Arrays.asList("/test/test2",
"/test/test1"));
+ });
+
+ // Check inequality query on propDate - this should not return
/test/test1 -> since that node should not have been indexed for propDate
+ // due to incorrect data type in the content for this property.
+ String query2 =
"/jcr:root/test//*[propDate!='2021-01-22T01:02:03.000Z']";
+ assertEventually(() -> {
+ assertQuery(query2, XPATH, Arrays.asList("/test/test3"));
+ });
+ }
+
+ @Test
+ public void testQueryWithDifferentDataTypesForSameProperty() throws
Exception {
+ // propa doesn't have any type defined in index - so by default it's a
String type property
+ Tree test = root.getTree("/").addChild("test");
+ test.addChild("test1").setProperty("propa", "bar");
+ test.addChild("test2").setProperty("propa", 10);
+ test.addChild("test3").setProperty("propa", 10L);
+ test.addChild("test4").setProperty("propa", true);
+ root.commit();
+
+ // Below queries will ensure propa is searchable with different data
types as content and behaviour is similar for lucene and elastic.
+ String query = "/jcr:root/test//*[propa='bar']";
+ assertEventually(() -> {
+ assertQuery(query, XPATH, Arrays.asList("/test/test1"));
+ });
+
+ String query2 = "/jcr:root/test//*[propa=true]";
+ assertEventually(() -> {
+ assertQuery(query2, XPATH, Arrays.asList("/test/test4"));
+ });
+
+ String query3 = "/jcr:root/test//*[propa=10]";
+ assertEventually(() -> {
+ assertQuery(query3, XPATH, Arrays.asList("/test/test2",
"/test/test3"));
+ });
+
+
+
+
Review comment:
remove extra newlines
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
