From bcd739dacf673c81fb3cf79b2907a54bd99fef38 Mon Sep 17 00:00:00 2001 From: Armin Date: Mon, 9 Jul 2018 23:25:35 +0200 Subject: [PATCH] Fix assertIngestDocument wrongfully passing * Previously docA being subset of docB passed because iteration was over docA's keys only * Scalars in nested fields were not compared in all cases * Assertion errors were hard to interpret (message wasn't correct since it only mentioned the class type) * In cases where two paths contained different types a ClassCastException was thrown instead of an AssertionError * Fixes #28492 --- .../ingest/common/GrokProcessorTests.java | 2 +- .../ingest/common/JsonProcessorTests.java | 14 ++-- .../ingest/IngestDocumentMatcher.java | 57 ++++++++----- .../ingest/IngestDocumentMatcherTests.java | 82 +++++++++++++++++++ 4 files changed, 123 insertions(+), 32 deletions(-) create mode 100644 test/framework/src/test/java/org/elasticsearch/ingest/IngestDocumentMatcherTests.java diff --git a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/GrokProcessorTests.java b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/GrokProcessorTests.java index 0eba79523aca2..68654923ae926 100644 --- a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/GrokProcessorTests.java +++ b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/GrokProcessorTests.java @@ -129,7 +129,7 @@ public void testMissingField() { public void testMissingFieldWithIgnoreMissing() throws Exception { String fieldName = "foo.bar"; IngestDocument originalIngestDocument = RandomDocumentPicks.randomIngestDocument(random(), new HashMap<>()); - IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random(), new HashMap<>()); + IngestDocument ingestDocument = new IngestDocument(originalIngestDocument); GrokProcessor processor = new GrokProcessor(randomAlphaOfLength(10), Collections.singletonMap("ONE", "1"), Collections.singletonList("%{ONE:one}"), fieldName, false, true, ThreadWatchdog.noop()); processor.execute(ingestDocument); diff --git a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/JsonProcessorTests.java b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/JsonProcessorTests.java index 245285259b47a..2867ed1d24053 100644 --- a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/JsonProcessorTests.java +++ b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/JsonProcessorTests.java @@ -33,7 +33,6 @@ import java.util.List; import java.util.Map; -import static org.elasticsearch.ingest.IngestDocumentMatcher.assertIngestDocument; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; @@ -55,7 +54,7 @@ public void testExecute() throws Exception { IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random(), document); jsonProcessor.execute(ingestDocument); Map jsonified = ingestDocument.getFieldValue(randomTargetField, Map.class); - assertIngestDocument(ingestDocument.getFieldValue(randomTargetField, Object.class), jsonified); + assertEquals(ingestDocument.getFieldValue(randomTargetField, Object.class), jsonified); } public void testInvalidValue() { @@ -161,13 +160,10 @@ public void testAddToRoot() throws Exception { IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random(), document); jsonProcessor.execute(ingestDocument); - Map expected = new HashMap<>(); - expected.put("a", 1); - expected.put("b", 2); - expected.put("c", "see"); - IngestDocument expectedIngestDocument = RandomDocumentPicks.randomIngestDocument(random(), expected); - - assertIngestDocument(ingestDocument, expectedIngestDocument); + Map sourceAndMetadata = ingestDocument.getSourceAndMetadata(); + assertEquals(1, sourceAndMetadata.get("a")); + assertEquals(2, sourceAndMetadata.get("b")); + assertEquals("see", sourceAndMetadata.get("c")); } public void testAddBoolToRoot() { diff --git a/test/framework/src/main/java/org/elasticsearch/ingest/IngestDocumentMatcher.java b/test/framework/src/main/java/org/elasticsearch/ingest/IngestDocumentMatcher.java index bb058c5cfdbb5..a7a8a3f11b1be 100644 --- a/test/framework/src/main/java/org/elasticsearch/ingest/IngestDocumentMatcher.java +++ b/test/framework/src/main/java/org/elasticsearch/ingest/IngestDocumentMatcher.java @@ -20,48 +20,61 @@ package org.elasticsearch.ingest; import java.util.List; -import java.util.Locale; import java.util.Map; - -import static org.hamcrest.Matchers.equalTo; -import static org.junit.Assert.assertArrayEquals; -import static org.junit.Assert.assertThat; +import java.util.Objects; public class IngestDocumentMatcher { /** * Helper method to assert the equivalence between two IngestDocuments. * - * @param a first object to compare - * @param b second object to compare + * @param docA first document to compare + * @param docB second document to compare */ - public static void assertIngestDocument(Object a, Object b) { + public static void assertIngestDocument(IngestDocument docA, IngestDocument docB) { + if ((deepEquals(docA.getIngestMetadata(), docB.getIngestMetadata(), true) && + deepEquals(docA.getSourceAndMetadata(), docB.getSourceAndMetadata(), false)) == false) { + throw new AssertionError("Expected [" + docA + "] but received [" + docB + "]."); + } + } + + private static boolean deepEquals(Object a, Object b, boolean isIngestMeta) { if (a instanceof Map) { Map mapA = (Map) a; + if (b instanceof Map == false) { + return false; + } Map mapB = (Map) b; + if (mapA.size() != mapB.size()) { + return false; + } for (Map.Entry entry : mapA.entrySet()) { - if (entry.getValue() instanceof List || entry.getValue() instanceof Map) { - assertIngestDocument(entry.getValue(), mapB.get(entry.getKey())); + Object key = entry.getKey(); + // Don't compare the timestamp of ingest metadata since it will differ between executions + if ((isIngestMeta && "timestamp".equals(key)) == false + && deepEquals(entry.getValue(), mapB.get(key), false) == false) { + return false; } } + return true; } else if (a instanceof List) { List listA = (List) a; + if (b instanceof List == false) { + return false; + } List listB = (List) b; - for (int i = 0; i < listA.size(); i++) { + int countA = listA.size(); + if (countA != listB.size()) { + return false; + } + for (int i = 0; i < countA; i++) { Object value = listA.get(i); - if (value instanceof List || value instanceof Map) { - assertIngestDocument(value, listB.get(i)); + if (deepEquals(value, listB.get(i), false) == false) { + return false; } } - } else if (a instanceof byte[]) { - assertArrayEquals((byte[]) a, (byte[])b); - } else if (a instanceof IngestDocument) { - IngestDocument docA = (IngestDocument) a; - IngestDocument docB = (IngestDocument) b; - assertIngestDocument(docA.getSourceAndMetadata(), docB.getSourceAndMetadata()); - assertIngestDocument(docA.getIngestMetadata(), docB.getIngestMetadata()); + return true; } else { - String msg = String.format(Locale.ROOT, "Expected %s class to be equal to %s", a.getClass().getName(), b.getClass().getName()); - assertThat(msg, a, equalTo(b)); + return Objects.deepEquals(a, b); } } } diff --git a/test/framework/src/test/java/org/elasticsearch/ingest/IngestDocumentMatcherTests.java b/test/framework/src/test/java/org/elasticsearch/ingest/IngestDocumentMatcherTests.java new file mode 100644 index 0000000000000..e7ab18972604f --- /dev/null +++ b/test/framework/src/test/java/org/elasticsearch/ingest/IngestDocumentMatcherTests.java @@ -0,0 +1,82 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.ingest; + +import org.elasticsearch.test.ESTestCase; + +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; + +import static org.elasticsearch.ingest.IngestDocumentMatcher.assertIngestDocument; + +public class IngestDocumentMatcherTests extends ESTestCase { + + public void testAssertIngestDocumentFailsOnDifferentMapData() { + Map sourceAndMetadata1 = new HashMap<>(); + sourceAndMetadata1.put("foo", "bar"); + IngestDocument document1 = new IngestDocument(sourceAndMetadata1, new HashMap<>()); + IngestDocument document2 = new IngestDocument(new HashMap<>(), new HashMap<>()); + assertThrowsOnComparision(document1, document2); + } + + public void testAssertIngestDocumentFailsOnDifferentLengthListData() { + String rootKey = "foo"; + IngestDocument document1 = + new IngestDocument(Collections.singletonMap(rootKey, Arrays.asList("bar", "baz")), new HashMap<>()); + IngestDocument document2 = + new IngestDocument(Collections.singletonMap(rootKey, Collections.emptyList()), new HashMap<>()); + assertThrowsOnComparision(document1, document2); + } + + public void testAssertIngestDocumentFailsOnDifferentNestedListFieldData() { + String rootKey = "foo"; + IngestDocument document1 = + new IngestDocument(Collections.singletonMap(rootKey, Arrays.asList("bar", "baz")), new HashMap<>()); + IngestDocument document2 = + new IngestDocument(Collections.singletonMap(rootKey, Arrays.asList("bar", "blub")), new HashMap<>()); + assertThrowsOnComparision(document1, document2); + } + + public void testAssertIngestDocumentFailsOnDifferentNestedMapFieldData() { + String rootKey = "foo"; + IngestDocument document1 = + new IngestDocument(Collections.singletonMap(rootKey, Collections.singletonMap("bar", "baz")), new HashMap<>()); + IngestDocument document2 = + new IngestDocument(Collections.singletonMap(rootKey, Collections.singletonMap("bar", "blub")), new HashMap<>()); + assertThrowsOnComparision(document1, document2); + } + + public void testThrowsAssertionErrorOnTypeConflict() { + String rootKey = "foo"; + IngestDocument document1 = + new IngestDocument(Collections.singletonMap(rootKey, Collections.singletonList("baz")), new HashMap<>()); + IngestDocument document2 = new IngestDocument( + Collections.singletonMap(rootKey, Collections.singletonMap("blub", "blab")), new HashMap<>() + ); + assertThrowsOnComparision(document1, document2); + } + + private static void assertThrowsOnComparision(IngestDocument document1, IngestDocument document2) { + expectThrows(AssertionError.class, () -> assertIngestDocument(document1, document2)); + expectThrows(AssertionError.class, () -> assertIngestDocument(document2, document1)); + } +}