From 393279a322194ac9ccde83cf27dea4989c3c4d6f Mon Sep 17 00:00:00 2001 From: Tal Levy Date: Wed, 14 Jun 2017 18:58:36 -0700 Subject: [PATCH] fix: Sort Processor does not have proper behavior with targetField 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 37a2c16e24fa8..28e568233ebf5 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.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 void execute(IngestDocument document) { 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 45f872412122f..8fa3f90d6ae74 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 void testSortNullValue() throws Exception { } } - 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 void testSortWithTargetField() throws Exception { 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 void testSortWithTargetField() throws Exception { 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); } - }