From 2cd771a23014ccb526fd96b6da539ca7fd912a6a Mon Sep 17 00:00:00 2001 From: Tal Levy Date: Thu, 15 Jun 2017 05:28:54 -0700 Subject: [PATCH] fix: Sort Processor does not have proper behavior with targetField (#25237) to specify a `targetField`. This results in some interesting behavior that was missed in the review. This processor sorts in-place, so there is a side-effect in both the original field and the target field. Another bug was that the targetField was not being set if the list being sorted was fewer than two elements. The new behavior works like this: If targetField and fieldName are not the same, we copy the list. --- .../ingest/common/SortProcessor.java | 11 +++-- .../ingest/common/SortProcessorTests.java | 44 +++++++++++++++++-- 2 files changed, 45 insertions(+), 10 deletions(-) diff --git a/modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/SortProcessor.java b/modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/SortProcessor.java index 37a2c16e24f..28e568233eb 100644 --- a/modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/SortProcessor.java +++ b/modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/SortProcessor.java @@ -24,6 +24,7 @@ import org.elasticsearch.ingest.ConfigurationUtils; import org.elasticsearch.ingest.IngestDocument; import org.elasticsearch.ingest.Processor; +import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Map; @@ -99,17 +100,15 @@ public final class SortProcessor extends AbstractProcessor { throw new IllegalArgumentException("field [" + field + "] is null, cannot sort."); } - if (list.size() <= 1) { - return; - } + List copy = new ArrayList<>(list); if (order.equals(SortOrder.ASCENDING)) { - Collections.sort(list); + Collections.sort(copy); } else { - Collections.sort(list, Collections.reverseOrder()); + Collections.sort(copy, Collections.reverseOrder()); } - document.setFieldValue(targetField, list); + document.setFieldValue(targetField, copy); } @Override diff --git a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/SortProcessorTests.java b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/SortProcessorTests.java index 45f87241212..8fa3f90d6ae 100644 --- a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/SortProcessorTests.java +++ b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/SortProcessorTests.java @@ -275,7 +275,7 @@ public class SortProcessorTests extends ESTestCase { } } - public void testSortWithTargetField() throws Exception { + public void testDescendingSortWithTargetField() throws Exception { IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random()); int numItems = randomIntBetween(1, 10); List fieldValue = new ArrayList<>(numItems); @@ -285,6 +285,42 @@ public class SortProcessorTests extends ESTestCase { fieldValue.add(value); expectedResult.add(value); } + + Collections.sort(expectedResult, Collections.reverseOrder()); + + String fieldName = RandomDocumentPicks.addRandomField(random(), ingestDocument, fieldValue); + String targetFieldName = RandomDocumentPicks.randomFieldName(random()); + Processor processor = new SortProcessor(randomAlphaOfLength(10), fieldName, + SortOrder.DESCENDING, targetFieldName); + processor.execute(ingestDocument); + assertEquals(ingestDocument.getFieldValue(targetFieldName, List.class), expectedResult); + } + + public void testAscendingSortWithTargetField() throws Exception { + IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random()); + int numItems = randomIntBetween(1, 10); + List fieldValue = new ArrayList<>(numItems); + List expectedResult = new ArrayList<>(numItems); + for (int j = 0; j < numItems; j++) { + String value = randomAlphaOfLengthBetween(1, 10); + fieldValue.add(value); + expectedResult.add(value); + } + + Collections.sort(expectedResult); + + String fieldName = RandomDocumentPicks.addRandomField(random(), ingestDocument, fieldValue); + String targetFieldName = RandomDocumentPicks.randomFieldName(random()); + Processor processor = new SortProcessor(randomAlphaOfLength(10), fieldName, + SortOrder.ASCENDING, targetFieldName); + processor.execute(ingestDocument); + assertEquals(ingestDocument.getFieldValue(targetFieldName, List.class), expectedResult); + } + + public void testSortWithTargetFieldLeavesOriginalUntouched() throws Exception { + IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random()); + List fieldValue = Arrays.asList(1, 5, 4); + List expectedResult = new ArrayList<>(fieldValue); Collections.sort(expectedResult); SortOrder order = randomBoolean() ? SortOrder.ASCENDING : SortOrder.DESCENDING; @@ -292,11 +328,11 @@ public class SortProcessorTests extends ESTestCase { Collections.reverse(expectedResult); } - String fieldName = RandomDocumentPicks.addRandomField(random(), ingestDocument, fieldValue); - String targetFieldName = RandomDocumentPicks.randomFieldName(random()); + String fieldName = RandomDocumentPicks.addRandomField(random(), ingestDocument, new ArrayList<>(fieldValue)); + String targetFieldName = fieldName + "foo"; Processor processor = new SortProcessor(randomAlphaOfLength(10), fieldName, order, targetFieldName); processor.execute(ingestDocument); assertEquals(ingestDocument.getFieldValue(targetFieldName, List.class), expectedResult); + assertEquals(ingestDocument.getFieldValue(fieldName, List.class), fieldValue); } - }