From 0f5de934cfef50e01e2d1cbf71175e20ac53b5f0 Mon Sep 17 00:00:00 2001 From: Mayya Sharipova Date: Wed, 24 Mar 2021 13:16:13 -0400 Subject: [PATCH 1/3] Fix binary docvalue_fields with padding Previously docvalue_fields for binary values with paddings did not output padding. We consider it to be a bug because: 1) es would not be able parse these values 2) output from source filtering and fields API is different and does output padding. This patches fixes this by outputing padding for binary docvalue_fields where it is present. Closes #70244 --- .../painless/60_script_doc_values_binary.yml | 15 ++---- .../test/search/350_binary_field.yml | 49 +++++++++++++++++++ .../search/fields/SearchFieldsIT.java | 4 +- .../elasticsearch/search/DocValueFormat.java | 1 - .../search/DocValueFormatTests.java | 4 +- 5 files changed, 57 insertions(+), 16 deletions(-) create mode 100644 rest-api-spec/src/main/resources/rest-api-spec/test/search/350_binary_field.yml diff --git a/modules/lang-painless/src/yamlRestTest/resources/rest-api-spec/test/painless/60_script_doc_values_binary.yml b/modules/lang-painless/src/yamlRestTest/resources/rest-api-spec/test/painless/60_script_doc_values_binary.yml index 3a549e6c79bf9..d6871412a73c9 100644 --- a/modules/lang-painless/src/yamlRestTest/resources/rest-api-spec/test/painless/60_script_doc_values_binary.yml +++ b/modules/lang-painless/src/yamlRestTest/resources/rest-api-spec/test/painless/60_script_doc_values_binary.yml @@ -27,20 +27,13 @@ - do: search: - rest_total_hits_as_int: true body: script_fields: - field: + field1: script: source: "doc['binary'].get(0).utf8ToString()" - - match: { hits.hits.0.fields.field.0: "Some binary blob" } - - - do: - search: - rest_total_hits_as_int: true - body: - script_fields: - field: + field2: script: source: "doc['binary'].value.utf8ToString()" - - match: { hits.hits.0.fields.field.0: "Some binary blob" } + - match: { hits.hits.0.fields.field1.0: "Some binary blob" } + - match: { hits.hits.0.fields.field2.0: "Some binary blob" } diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search/350_binary_field.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search/350_binary_field.yml new file mode 100644 index 0000000000000..0596c9bb0c709 --- /dev/null +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search/350_binary_field.yml @@ -0,0 +1,49 @@ +--- +"binary": + - skip: + features: ["headers"] + version: " - 7.99.99" + reason: "docvalues_fields on binary field were corrected in 8.0" + - do: + indices.create: + index: test + body: + mappings: + properties: + binary: + type: binary + doc_values: true + + - do: + #other formats (e.g. cbor) may not support parsing of binary + headers: + Content-Type: application/json + index: + index: test + id: 1 + body: + binary: U29tZSBiaW5hcnkgYmxvYg== + + - do: + indices.refresh: {} + + - do: + search: + index: test + body: + docvalue_fields: [ "binary" ] + - match: { hits.hits.0.fields.binary.0: "U29tZSBiaW5hcnkgYmxvYg==" } + + - do: + search: + index: test + body: + fields: [ "binary" ] + - match: { hits.hits.0.fields.binary.0: "U29tZSBiaW5hcnkgYmxvYg==" } + + - do: + search: + index: test + body: + _source: ["binary"] + - match: { hits.hits.0._source.binary: "U29tZSBiaW5hcnkgYmxvYg==" } diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/fields/SearchFieldsIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/fields/SearchFieldsIT.java index 0d283e4f49459..7aec3fa84abb3 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/search/fields/SearchFieldsIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/search/fields/SearchFieldsIT.java @@ -810,7 +810,7 @@ public void testDocValueFields() throws Exception { assertThat(searchResponse.getHits().getAt(0).getFields().get("boolean_field").getValue(), equalTo((Object) true)); assertThat(searchResponse.getHits().getAt(0).getFields().get("text_field").getValue(), equalTo("foo")); assertThat(searchResponse.getHits().getAt(0).getFields().get("keyword_field").getValue(), equalTo("foo")); - assertThat(searchResponse.getHits().getAt(0).getFields().get("binary_field").getValue(), equalTo("KmQ")); + assertThat(searchResponse.getHits().getAt(0).getFields().get("binary_field").getValue(), equalTo("KmQ=")); assertThat(searchResponse.getHits().getAt(0).getFields().get("ip_field").getValue(), equalTo("::1")); builder = client().prepareSearch().setQuery(matchAllQuery()) @@ -835,7 +835,7 @@ public void testDocValueFields() throws Exception { assertThat(searchResponse.getHits().getAt(0).getFields().get("boolean_field").getValue(), equalTo((Object) true)); assertThat(searchResponse.getHits().getAt(0).getFields().get("text_field").getValue(), equalTo("foo")); assertThat(searchResponse.getHits().getAt(0).getFields().get("keyword_field").getValue(), equalTo("foo")); - assertThat(searchResponse.getHits().getAt(0).getFields().get("binary_field").getValue(), equalTo("KmQ")); + assertThat(searchResponse.getHits().getAt(0).getFields().get("binary_field").getValue(), equalTo("KmQ=")); assertThat(searchResponse.getHits().getAt(0).getFields().get("ip_field").getValue(), equalTo("::1")); builder = client().prepareSearch().setQuery(matchAllQuery()) diff --git a/server/src/main/java/org/elasticsearch/search/DocValueFormat.java b/server/src/main/java/org/elasticsearch/search/DocValueFormat.java index 7ee9521a03aa7..95735de5e1359 100644 --- a/server/src/main/java/org/elasticsearch/search/DocValueFormat.java +++ b/server/src/main/java/org/elasticsearch/search/DocValueFormat.java @@ -165,7 +165,6 @@ public void writeTo(StreamOutput out) { @Override public String format(BytesRef value) { return Base64.getEncoder() - .withoutPadding() .encodeToString(Arrays.copyOfRange(value.bytes, value.offset, value.offset + value.length)); } diff --git a/server/src/test/java/org/elasticsearch/search/DocValueFormatTests.java b/server/src/test/java/org/elasticsearch/search/DocValueFormatTests.java index e497528924605..6af61b8e30b52 100644 --- a/server/src/test/java/org/elasticsearch/search/DocValueFormatTests.java +++ b/server/src/test/java/org/elasticsearch/search/DocValueFormatTests.java @@ -126,10 +126,10 @@ public void testRawFormat() { public void testBinaryFormat() { assertEquals("", DocValueFormat.BINARY.format(new BytesRef())); - assertEquals("KmQ", DocValueFormat.BINARY.format(new BytesRef(new byte[] {42, 100}))); + assertEquals("KmQ=", DocValueFormat.BINARY.format(new BytesRef(new byte[] {42, 100}))); assertEquals(new BytesRef(), DocValueFormat.BINARY.parseBytesRef("")); - assertEquals(new BytesRef(new byte[] {42, 100}), DocValueFormat.BINARY.parseBytesRef("KmQ")); + assertEquals(new BytesRef(new byte[] {42, 100}), DocValueFormat.BINARY.parseBytesRef("KmQ=")); } public void testBooleanFormat() { From 73be1d918a4e58be64be6dc4764091a090305d6c Mon Sep 17 00:00:00 2001 From: Mayya Sharipova Date: Fri, 26 Mar 2021 11:25:12 -0400 Subject: [PATCH 2/3] Address Nik's feedback --- .../rest-api-spec/test/search/350_binary_field.yml | 4 +--- .../index/mapper/BinaryFieldMapperTests.java | 11 ++++++++--- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search/350_binary_field.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search/350_binary_field.yml index 0596c9bb0c709..6fbdb575fccb5 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search/350_binary_field.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search/350_binary_field.yml @@ -20,13 +20,11 @@ Content-Type: application/json index: index: test + refresh: true id: 1 body: binary: U29tZSBiaW5hcnkgYmxvYg== - - do: - indices.refresh: {} - - do: search: index: test diff --git a/server/src/test/java/org/elasticsearch/index/mapper/BinaryFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/BinaryFieldMapperTests.java index 0cbe10cff3c55..e378d516b9e50 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/BinaryFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/BinaryFieldMapperTests.java @@ -19,7 +19,9 @@ import java.io.IOException; import java.io.OutputStream; +import java.nio.charset.StandardCharsets; import java.util.Arrays; +import java.util.Base64; import static org.hamcrest.Matchers.instanceOf; @@ -121,9 +123,12 @@ public void testStoredValue() throws IOException { @Override protected Object generateRandomInputValue(MappedFieldType ft) { - assumeFalse("We can't parse the binary doc values we send", true); - // AwaitsFix https://github.com/elastic/elasticsearch/issues/70244 - return null; + if (rarely()) { + return null; + } else { + byte[] value = randomAlphaOfLengthBetween(1, 30).getBytes(StandardCharsets.UTF_8); + return Base64.getEncoder().encodeToString(value); + } } @Override From 3aa1286d84fe1628740d286e77e7314a92dbe9e4 Mon Sep 17 00:00:00 2001 From: Mayya Sharipova Date: Fri, 26 Mar 2021 11:58:39 -0400 Subject: [PATCH 3/3] Address Nik's feedback 2 --- .../index/mapper/BinaryFieldMapperTests.java | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/BinaryFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/BinaryFieldMapperTests.java index e378d516b9e50..a30b17478d99c 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/BinaryFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/BinaryFieldMapperTests.java @@ -19,7 +19,6 @@ import java.io.IOException; import java.io.OutputStream; -import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.Base64; @@ -123,12 +122,9 @@ public void testStoredValue() throws IOException { @Override protected Object generateRandomInputValue(MappedFieldType ft) { - if (rarely()) { - return null; - } else { - byte[] value = randomAlphaOfLengthBetween(1, 30).getBytes(StandardCharsets.UTF_8); - return Base64.getEncoder().encodeToString(value); - } + if (rarely()) return null; + byte[] value = randomByteArrayOfLength(randomIntBetween(1, 50)); + return Base64.getEncoder().encodeToString(value); } @Override