From 568e655fdbfd8161f22cc368bfa49c301ad2366a Mon Sep 17 00:00:00 2001 From: Michael McCandless Date: Thu, 12 Jan 2017 19:00:39 -0500 Subject: [PATCH] Source filtering: only accept array items if the previous include pattern matches (#22593) Source filtering was always accepting array items even if the include pattern did not match. Closes #22557 --- .../xcontent/support/XContentMapValues.java | 8 +-- .../support/XContentMapValuesTests.java | 70 +++++++++++-------- 2 files changed, 44 insertions(+), 34 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java b/core/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java index a1affb4fe57..09d9dabac75 100644 --- a/core/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java +++ b/core/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java @@ -263,7 +263,7 @@ public class XContentMapValues { List filteredValue = filter((Iterable) value, subIncludeAutomaton, subIncludeState, excludeAutomaton, excludeState, matchAllAutomaton); - if (includeAutomaton.isAccept(includeState) || filteredValue.isEmpty() == false) { + if (filteredValue.isEmpty() == false) { filtered.put(key, filteredValue); } @@ -286,6 +286,7 @@ public class XContentMapValues { CharacterRunAutomaton excludeAutomaton, int initialExcludeState, CharacterRunAutomaton matchAllAutomaton) { List filtered = new ArrayList<>(); + boolean isInclude = includeAutomaton.isAccept(initialIncludeState); for (Object value : iterable) { if (value instanceof Map) { int includeState = includeAutomaton.step(initialIncludeState, '.'); @@ -304,9 +305,8 @@ public class XContentMapValues { if (filteredValue.isEmpty() == false) { filtered.add(filteredValue); } - } else { - // TODO: we have tests relying on this behavior on arrays even - // if the path does not match, but this looks like a bug? + } else if (isInclude) { + // #22557: only accept this array value if the key we are on is accepted: filtered.add(value); } } diff --git a/core/src/test/java/org/elasticsearch/common/xcontent/support/XContentMapValuesTests.java b/core/src/test/java/org/elasticsearch/common/xcontent/support/XContentMapValuesTests.java index 58c4f630bb6..a607b48f79f 100644 --- a/core/src/test/java/org/elasticsearch/common/xcontent/support/XContentMapValuesTests.java +++ b/core/src/test/java/org/elasticsearch/common/xcontent/support/XContentMapValuesTests.java @@ -41,6 +41,7 @@ import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; import static org.hamcrest.Matchers.hasEntry; import static org.hamcrest.Matchers.hasKey; +import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.nullValue; import static org.hamcrest.core.IsEqual.equalTo; @@ -148,8 +149,8 @@ public class XContentMapValuesTests extends ESTestCase { extValue = XContentMapValues.extractValue("path1.test", map); assertThat(extValue, instanceOf(List.class)); - List extListValue = (List) extValue; - assertThat(extListValue.size(), equalTo(2)); + List extListValue = (List) extValue; + assertThat(extListValue, hasSize(2)); builder = XContentFactory.jsonBuilder().startObject() .startObject("path1") @@ -168,7 +169,7 @@ public class XContentMapValuesTests extends ESTestCase { assertThat(extValue, instanceOf(List.class)); extListValue = (List) extValue; - assertThat(extListValue.size(), equalTo(2)); + assertThat(extListValue, hasSize(2)); assertThat(extListValue.get(0).toString(), equalTo("value1")); assertThat(extListValue.get(1).toString(), equalTo("value2")); @@ -234,9 +235,9 @@ public class XContentMapValuesTests extends ESTestCase { Map map = new HashMap<>(); map.put("obj", "value"); map.put("obj_name", "value_name"); - Map filterdMap = XContentMapValues.filter(map, new String[]{"obj_name"}, Strings.EMPTY_ARRAY); - assertThat(filterdMap.size(), equalTo(1)); - assertThat((String) filterdMap.get("obj_name"), equalTo("value_name")); + Map filteredMap = XContentMapValues.filter(map, new String[]{"obj_name"}, Strings.EMPTY_ARRAY); + assertThat(filteredMap.size(), equalTo(1)); + assertThat((String) filteredMap.get("obj_name"), equalTo("value_name")); } @@ -251,19 +252,17 @@ public class XContentMapValuesTests extends ESTestCase { put("nested", 2); put("nested_2", 3); }})); - Map falteredMap = XContentMapValues.filter(map, new String[]{"array.nested"}, Strings.EMPTY_ARRAY); - assertThat(falteredMap.size(), equalTo(1)); + Map filteredMap = XContentMapValues.filter(map, new String[]{"array.nested"}, Strings.EMPTY_ARRAY); + assertThat(filteredMap.size(), equalTo(1)); - // Selecting members of objects within arrays (ex. [ 1, { nested: "value"} ]) always returns all values in the array (1 in the ex) - // this is expected behavior as this types of objects are not supported in ES - assertThat((Integer) ((List) falteredMap.get("array")).get(0), equalTo(1)); - assertThat(((Map) ((List) falteredMap.get("array")).get(1)).size(), equalTo(1)); - assertThat((Integer) ((Map) ((List) falteredMap.get("array")).get(1)).get("nested"), equalTo(2)); + assertThat(((List) filteredMap.get("array")), hasSize(1)); + assertThat(((Map) ((List) filteredMap.get("array")).get(0)).size(), equalTo(1)); + assertThat((Integer) ((Map) ((List) filteredMap.get("array")).get(0)).get("nested"), equalTo(2)); - falteredMap = XContentMapValues.filter(map, new String[]{"array.*"}, Strings.EMPTY_ARRAY); - assertThat(falteredMap.size(), equalTo(1)); - assertThat((Integer) ((List) falteredMap.get("array")).get(0), equalTo(1)); - assertThat(((Map) ((List) falteredMap.get("array")).get(1)).size(), equalTo(2)); + filteredMap = XContentMapValues.filter(map, new String[]{"array.*"}, Strings.EMPTY_ARRAY); + assertThat(filteredMap.size(), equalTo(1)); + assertThat(((List) filteredMap.get("array")), hasSize(1)); + assertThat(((Map) ((List) filteredMap.get("array")).get(0)).size(), equalTo(2)); map.clear(); map.put("field", "value"); @@ -272,16 +271,16 @@ public class XContentMapValuesTests extends ESTestCase { put("field", "value"); put("field2", "value2"); }}); - falteredMap = XContentMapValues.filter(map, new String[]{"obj.field"}, Strings.EMPTY_ARRAY); - assertThat(falteredMap.size(), equalTo(1)); - assertThat(((Map) falteredMap.get("obj")).size(), equalTo(1)); - assertThat((String) ((Map) falteredMap.get("obj")).get("field"), equalTo("value")); + filteredMap = XContentMapValues.filter(map, new String[]{"obj.field"}, Strings.EMPTY_ARRAY); + assertThat(filteredMap.size(), equalTo(1)); + assertThat(((Map) filteredMap.get("obj")).size(), equalTo(1)); + assertThat((String) ((Map) filteredMap.get("obj")).get("field"), equalTo("value")); - falteredMap = XContentMapValues.filter(map, new String[]{"obj.*"}, Strings.EMPTY_ARRAY); - assertThat(falteredMap.size(), equalTo(1)); - assertThat(((Map) falteredMap.get("obj")).size(), equalTo(2)); - assertThat((String) ((Map) falteredMap.get("obj")).get("field"), equalTo("value")); - assertThat((String) ((Map) falteredMap.get("obj")).get("field2"), equalTo("value2")); + filteredMap = XContentMapValues.filter(map, new String[]{"obj.*"}, Strings.EMPTY_ARRAY); + assertThat(filteredMap.size(), equalTo(1)); + assertThat(((Map) filteredMap.get("obj")).size(), equalTo(2)); + assertThat((String) ((Map) filteredMap.get("obj")).get("field"), equalTo("value")); + assertThat((String) ((Map) filteredMap.get("obj")).get("field2"), equalTo("value2")); } @@ -323,7 +322,7 @@ public class XContentMapValuesTests extends ESTestCase { filteredMap = XContentMapValues.filter(map, new String[]{"array"}, new String[]{"*.field2"}); assertThat(filteredMap.size(), equalTo(1)); - assertThat(((List) filteredMap.get("array")).size(), equalTo(2)); + assertThat(((List) filteredMap.get("array")), hasSize(2)); assertThat((Integer) ((List) filteredMap.get("array")).get(0), equalTo(1)); assertThat(((Map) ((List) filteredMap.get("array")).get(1)).size(), equalTo(1)); assertThat(((Map) ((List) filteredMap.get("array")).get(1)).get("field").toString(), equalTo("value")); @@ -436,20 +435,20 @@ public class XContentMapValuesTests extends ESTestCase { assertThat(filteredSource.size(), equalTo(1)); assertThat(filteredSource, hasKey("obj1")); - assertThat(((Map) filteredSource.get("obj1")).size(), Matchers.equalTo(0)); + assertThat(((Map) filteredSource.get("obj1")).size(), equalTo(0)); // explicit include filteredSource = XContentMapValues.filter(mapTuple.v2(), new String[]{"obj1"}, new String[]{"*.obj2"}); assertThat(filteredSource.size(), equalTo(1)); assertThat(filteredSource, hasKey("obj1")); - assertThat(((Map) filteredSource.get("obj1")).size(), Matchers.equalTo(0)); + assertThat(((Map) filteredSource.get("obj1")).size(), equalTo(0)); // wild card include filteredSource = XContentMapValues.filter(mapTuple.v2(), new String[]{"*.obj2"}, new String[]{"*.obj3"}); assertThat(filteredSource.size(), equalTo(1)); assertThat(filteredSource, hasKey("obj1")); assertThat(((Map) filteredSource.get("obj1")), hasKey("obj2")); - assertThat(((Map) ((Map) filteredSource.get("obj1")).get("obj2")).size(), Matchers.equalTo(0)); + assertThat(((Map) ((Map) filteredSource.get("obj1")).get("obj2")).size(), equalTo(0)); } @SuppressWarnings({"unchecked"}) @@ -589,4 +588,15 @@ public class XContentMapValuesTests extends ESTestCase { assertEquals(Collections.singletonMap("foobar", 2), XContentMapValues.filter(map, new String[] {"foobar"}, new String[0])); assertEquals(Collections.singletonMap("foobaz", 3), XContentMapValues.filter(map, new String[0], new String[] {"foobar"})); } + + public void testPrefix() { + Map map = new HashMap<>(); + map.put("photos", Arrays.asList(new String[] {"foo", "bar"})); + map.put("photosCount", 2); + + Map filtered = XContentMapValues.filter(map, new String[] {"photosCount"}, new String[0]); + Map expected = new HashMap<>(); + expected.put("photosCount", 2); + assertEquals(expected, filtered); + } }