From d4d70d886ed5da1b657dca4c1e5af80ce9cd07ef Mon Sep 17 00:00:00 2001 From: Mikhail Khludnev Date: Sat, 16 Nov 2024 02:59:01 +0300 Subject: [PATCH] Search dv only IP masks (#16628) * search dv only ip masks Signed-off-by: mikhail-khludnev * search dv only ip masks: changes Signed-off-by: Mikhail Khludnev * drop fancy closures Signed-off-by: mikhail-khludnev * drop fancy closures. one more Signed-off-by: mikhail-khludnev * fix unit tests; add some more dvOnly Signed-off-by: mikhail-khludnev * drop skipping dvOnly in 2.x Signed-off-by: mikhail-khludnev * drop redundant brackets Signed-off-by: mikhail-khludnev * extract conditions Signed-off-by: mikhail-khludnev * asserts Signed-off-by: mikhail-khludnev * spotless apply Signed-off-by: mikhail-khludnev * bring back skip before Signed-off-by: mikhail-khludnev * combine asserts Signed-off-by: mikhail-khludnev * inline, copy-paste Signed-off-by: mikhail-khludnev --------- Signed-off-by: mikhail-khludnev Signed-off-by: Mikhail Khludnev Co-authored-by: mikhail-khludnev --- CHANGELOG.md | 1 + .../test/search/340_doc_values_field.yml | 92 ++++++++++++++- .../index/mapper/IpFieldMapper.java | 86 +++++++------- .../index/mapper/IpFieldTypeTests.java | 107 +++++++++++++++++- 4 files changed, 240 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e65dbbdb908eb..00754925ea111 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Support retrieving doc values of unsigned long field ([#16543](https://github.com/opensearch-project/OpenSearch/pull/16543)) - Fix rollover alias supports restored searchable snapshot index([#16483](https://github.com/opensearch-project/OpenSearch/pull/16483)) - Fix permissions error on scripted query against remote snapshot ([#16544](https://github.com/opensearch-project/OpenSearch/pull/16544)) +- Fix `doc_values` only (`index:false`) IP field searching for masks ([#16628](https://github.com/opensearch-project/OpenSearch/pull/16628)) ### Security diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search/340_doc_values_field.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search/340_doc_values_field.yml index a133060f07c6f..647aaf2c9088b 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search/340_doc_values_field.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search/340_doc_values_field.yml @@ -449,6 +449,28 @@ - match: { hits.total: 2 } + - do: + search: + rest_total_hits_as_int: true + index: test-iodvq + body: + query: + term: + ip_field: "192.168.0.1/24" + + - match: { hits.total: 3 } + + - do: + search: + rest_total_hits_as_int: true + index: test-iodvq + body: + query: + term: + ip_field: "192.168.0.1/31" + + - match: { hits.total: 1 } + - do: search: rest_total_hits_as_int: true @@ -987,6 +1009,28 @@ - match: { hits.total: 2 } + - do: + search: + rest_total_hits_as_int: true + index: test-index + body: + query: + term: + ip_field: "192.168.0.1/24" + + - match: { hits.total: 3 } + + - do: + search: + rest_total_hits_as_int: true + index: test-index + body: + query: + term: + ip_field: "192.168.0.1/31" + + - match: { hits.total: 1 } + - do: search: rest_total_hits_as_int: true @@ -1077,8 +1121,8 @@ "search on fields with only doc_values enabled": - skip: features: [ "headers" ] - version: " - 2.99.99" - reason: "searching with only doc_values was added in 3.0.0" + version: " - 2.18.99" + reason: "searching with only doc_values was finally added in 2.19.0" - do: indices.create: index: test-doc-values @@ -1372,6 +1416,28 @@ - match: { hits.total: 2 } + - do: + search: + rest_total_hits_as_int: true + index: test-doc-values + body: + query: + terms: + ip_field: ["192.168.0.1", "192.168.0.2"] + + - match: { hits.total: 2 } + + - do: + search: + rest_total_hits_as_int: true + index: test-doc-values + body: + query: + terms: + ip_field: ["192.168.0.1/31", "192.168.0.3"] + + - match: { hits.total: 2 } + - do: search: rest_total_hits_as_int: true @@ -1516,6 +1582,28 @@ - match: { hits.total: 2 } + - do: + search: + rest_total_hits_as_int: true + index: test-doc-values + body: + query: + term: + ip_field: "192.168.0.1/31" + + - match: { hits.total: 1 } + + - do: + search: + rest_total_hits_as_int: true + index: test-doc-values + body: + query: + term: + ip_field: "192.168.0.1/24" + + - match: { hits.total: 3 } + - do: search: rest_total_hits_as_int: true diff --git a/server/src/main/java/org/opensearch/index/mapper/IpFieldMapper.java b/server/src/main/java/org/opensearch/index/mapper/IpFieldMapper.java index db8da8a949d6f..c51cada9f3143 100644 --- a/server/src/main/java/org/opensearch/index/mapper/IpFieldMapper.java +++ b/server/src/main/java/org/opensearch/index/mapper/IpFieldMapper.java @@ -64,6 +64,7 @@ import java.util.Map; import java.util.function.BiFunction; import java.util.function.Supplier; +import java.util.stream.Collectors; /** * A {@link FieldMapper} for ip addresses. @@ -225,9 +226,9 @@ protected Object parseSourceValue(Object value) { @Override public Query termQuery(Object value, @Nullable QueryShardContext context) { failIfNotIndexedAndNoDocValues(); - Query query; + final PointRangeQuery pointQuery; if (value instanceof InetAddress) { - query = InetAddressPoint.newExactQuery(name(), (InetAddress) value); + pointQuery = (PointRangeQuery) InetAddressPoint.newExactQuery(name(), (InetAddress) value); } else { if (value instanceof BytesRef) { value = ((BytesRef) value).utf8ToString(); @@ -235,32 +236,27 @@ public Query termQuery(Object value, @Nullable QueryShardContext context) { String term = value.toString(); if (term.contains("/")) { final Tuple cidr = InetAddresses.parseCidr(term); - query = InetAddressPoint.newPrefixQuery(name(), cidr.v1(), cidr.v2()); + pointQuery = (PointRangeQuery) InetAddressPoint.newPrefixQuery(name(), cidr.v1(), cidr.v2()); } else { InetAddress address = InetAddresses.forString(term); - query = InetAddressPoint.newExactQuery(name(), address); + pointQuery = (PointRangeQuery) InetAddressPoint.newExactQuery(name(), address); } } - if (isSearchable() && hasDocValues()) { - String term = value.toString(); - if (term.contains("/")) { - final Tuple cidr = InetAddresses.parseCidr(term); - return InetAddressPoint.newPrefixQuery(name(), cidr.v1(), cidr.v2()); - } - return new IndexOrDocValuesQuery( - query, - SortedSetDocValuesField.newSlowExactQuery(name(), new BytesRef(((PointRangeQuery) query).getLowerPoint())) + Query dvQuery = null; + if (hasDocValues()) { + dvQuery = SortedSetDocValuesField.newSlowRangeQuery( + name(), + new BytesRef(pointQuery.getLowerPoint()), + new BytesRef(pointQuery.getUpperPoint()), + true, + true ); } - if (hasDocValues()) { - String term = value.toString(); - if (term.contains("/")) { - final Tuple cidr = InetAddresses.parseCidr(term); - return InetAddressPoint.newPrefixQuery(name(), cidr.v1(), cidr.v2()); - } - return SortedSetDocValuesField.newSlowExactQuery(name(), new BytesRef(((PointRangeQuery) query).getLowerPoint())); + if (isSearchable() && hasDocValues()) { + return new IndexOrDocValuesQuery(pointQuery, dvQuery); + } else { + return isSearchable() ? pointQuery : dvQuery; } - return query; } @Override @@ -285,36 +281,46 @@ public Query termsQuery(List values, QueryShardContext context) { } addresses[i++] = address; } - return InetAddressPoint.newSetQuery(name(), addresses); + Query dvQuery = null; + if (hasDocValues()) { + List bytesRefs = Arrays.stream(addresses) + .distinct() + .map(InetAddressPoint::encode) + .map(BytesRef::new) + .collect(Collectors.toList()); + dvQuery = SortedSetDocValuesField.newSlowSetQuery(name(), bytesRefs); + } + Query pointQuery = null; + if (isSearchable()) { + pointQuery = InetAddressPoint.newSetQuery(name(), addresses); + } + if (isSearchable() && hasDocValues()) { + return new IndexOrDocValuesQuery(pointQuery, dvQuery); + } else { + return isSearchable() ? pointQuery : dvQuery; + } } @Override public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper, QueryShardContext context) { failIfNotIndexedAndNoDocValues(); return rangeQuery(lowerTerm, upperTerm, includeLower, includeUpper, (lower, upper) -> { - Query query = InetAddressPoint.newRangeQuery(name(), lower, upper); - if (isSearchable() && hasDocValues()) { - return new IndexOrDocValuesQuery( - query, - SortedSetDocValuesField.newSlowRangeQuery( - ((PointRangeQuery) query).getField(), - new BytesRef(((PointRangeQuery) query).getLowerPoint()), - new BytesRef(((PointRangeQuery) query).getUpperPoint()), - true, - true - ) - ); - } + PointRangeQuery pointQuery = (PointRangeQuery) InetAddressPoint.newRangeQuery(name(), lower, upper); + Query dvQuery = null; if (hasDocValues()) { - return SortedSetDocValuesField.newSlowRangeQuery( - ((PointRangeQuery) query).getField(), - new BytesRef(((PointRangeQuery) query).getLowerPoint()), - new BytesRef(((PointRangeQuery) query).getUpperPoint()), + dvQuery = SortedSetDocValuesField.newSlowRangeQuery( + pointQuery.getField(), + new BytesRef(pointQuery.getLowerPoint()), + new BytesRef(pointQuery.getUpperPoint()), true, true ); } - return query; + if (isSearchable() && hasDocValues()) { + return new IndexOrDocValuesQuery(pointQuery, dvQuery); + } else { + return isSearchable() ? pointQuery : dvQuery; + } }); } diff --git a/server/src/test/java/org/opensearch/index/mapper/IpFieldTypeTests.java b/server/src/test/java/org/opensearch/index/mapper/IpFieldTypeTests.java index 0a2435553b19e..a5403ef81481f 100644 --- a/server/src/test/java/org/opensearch/index/mapper/IpFieldTypeTests.java +++ b/server/src/test/java/org/opensearch/index/mapper/IpFieldTypeTests.java @@ -50,6 +50,8 @@ import java.net.InetAddress; import java.util.Arrays; import java.util.Collections; +import java.util.List; +import java.util.Objects; public class IpFieldTypeTests extends FieldTypeTestCase { @@ -76,7 +78,7 @@ public void testValueForSearch() { } public void testTermQuery() { - MappedFieldType ft = new IpFieldMapper.IpFieldType("field"); + MappedFieldType ft = new IpFieldMapper.IpFieldType("field", true, false, true, null, Collections.emptyMap()); String ip = "2001:db8::2:1"; @@ -104,20 +106,94 @@ public void testTermQuery() { String prefix = ip + "/64"; query = InetAddressPoint.newPrefixQuery("field", InetAddresses.forString(ip), 64); - assertEquals(query, ft.termQuery(prefix, null)); + assertEquals( + new IndexOrDocValuesQuery( + query, + SortedSetDocValuesField.newSlowRangeQuery( + "field", + ipToByteRef("2001:db8:0:0:0:0:0:0"), + ipToByteRef("2001:db8:0:0:ffff:ffff:ffff:ffff"), + true, + true + ) + ), + ft.termQuery(prefix, null) + ); ip = "192.168.1.7"; prefix = ip + "/16"; query = InetAddressPoint.newPrefixQuery("field", InetAddresses.forString(ip), 16); - assertEquals(query, ft.termQuery(prefix, null)); + assertEquals( + new IndexOrDocValuesQuery( + query, + SortedSetDocValuesField.newSlowRangeQuery( + "field", + ipToByteRef("::ffff:192.168.0.0"), + ipToByteRef("::ffff:192.168.255.255"), + true, + true + ) + ), + ft.termQuery(prefix, null) + ); MappedFieldType unsearchable = new IpFieldMapper.IpFieldType("field", false, false, false, null, Collections.emptyMap()); IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> unsearchable.termQuery("::1", null)); assertEquals("Cannot search on field [field] since it is both not indexed, and does not have doc_values enabled.", e.getMessage()); } + public void testDvOnlyTermQuery() { + IpFieldMapper.IpFieldType dvOnly = new IpFieldMapper.IpFieldType("field", false, false, true, null, Collections.emptyMap()); + String ip = "2001:db8::2:1"; + + Query query = InetAddressPoint.newExactQuery("field", InetAddresses.forString(ip)); + + assertEquals( + SortedSetDocValuesField.newSlowExactQuery("field", new BytesRef(((PointRangeQuery) query).getLowerPoint())), + dvOnly.termQuery(ip, null) + ); + + ip = "192.168.1.7"; + query = InetAddressPoint.newExactQuery("field", InetAddresses.forString(ip)); + assertEquals( + SortedSetDocValuesField.newSlowExactQuery("field", new BytesRef(((PointRangeQuery) query).getLowerPoint())), + dvOnly.termQuery(ip, null) + ); + + ip = "2001:db8::2:1"; + String prefix = ip + "/64"; + + assertEquals( + SortedSetDocValuesField.newSlowRangeQuery( + "field", + ipToByteRef("2001:db8:0:0:0:0:0:0"), + ipToByteRef("2001:db8:0:0:ffff:ffff:ffff:ffff"), + true, + true + ), + dvOnly.termQuery(prefix, null) + ); + + ip = "192.168.1.7"; + prefix = ip + "/16"; + assertEquals( + SortedSetDocValuesField.newSlowRangeQuery( + "field", + ipToByteRef("::ffff:192.168.0.0"), + ipToByteRef("::ffff:192.168.255.255"), + true, + true + ), + dvOnly.termQuery(prefix, null) + ); + } + + private static BytesRef ipToByteRef(String ipString) { + return new BytesRef(Objects.requireNonNull(InetAddresses.ipStringToBytes(ipString))); + } + public void testTermsQuery() { - MappedFieldType ft = new IpFieldMapper.IpFieldType("field"); + MappedFieldType ft = new IpFieldMapper.IpFieldType("field", true, false, false, null, Collections.emptyMap()); assertEquals( InetAddressPoint.newSetQuery("field", InetAddresses.forString("::2"), InetAddresses.forString("::5")), @@ -139,6 +215,29 @@ public void testTermsQuery() { ); } + public void testDvOnlyTermsQuery() { + MappedFieldType dvOnly = new IpFieldMapper.IpFieldType("field", false, false, true, null, Collections.emptyMap()); + + assertEquals( + SortedSetDocValuesField.newSlowSetQuery("field", List.of(ipToByteRef("::2"), ipToByteRef("::5"))), + dvOnly.termsQuery(Arrays.asList(InetAddresses.forString("::2"), InetAddresses.forString("::5")), null) + ); + assertEquals( + SortedSetDocValuesField.newSlowSetQuery("field", List.of(ipToByteRef("::2"), ipToByteRef("::5"))), + dvOnly.termsQuery(Arrays.asList("::2", "::5"), null) + ); + + // if the list includes a prefix query we fallback to a bool query + assertEquals( + new ConstantScoreQuery( + new BooleanQuery.Builder().add(dvOnly.termQuery("::42", null), Occur.SHOULD) + .add(dvOnly.termQuery("::2/16", null), Occur.SHOULD) + .build() + ), + dvOnly.termsQuery(Arrays.asList("::42", "::2/16"), null) + ); + } + public void testRangeQuery() { MappedFieldType ft = new IpFieldMapper.IpFieldType("field"); Query query = InetAddressPoint.newRangeQuery("field", InetAddresses.forString("::"), InetAddressPoint.MAX_VALUE);