Fix binary docvalue_fields with padding (#70936)

mayya-sharipova · web-flow · commit 4afbf4541fad · 2021-03-29T09:38:31.000-04:00
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 Backport for #70826
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
@@ -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
@@ -0,0 +1,47 @@
+---
+"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
+            refresh: true
+            id: 1
+            body:
+                binary: U29tZSBiaW5hcnkgYmxvYg==
+
+    - 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
@@ -848,7 +848,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())
@@ -873,7 +873,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())
@@ -909,7 +909,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
@@ -168,7 +168,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/index/mapper/BinaryFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/BinaryFieldMapperTests.java
@@ -20,6 +20,7 @@
 import java.io.IOException;
 import java.io.OutputStream;
 import java.util.Arrays;
+import java.util.Base64;
 
 import static org.hamcrest.Matchers.instanceOf;
 
@@ -121,9 +122,9 @@ 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;
+        byte[] value = randomByteArrayOfLength(randomIntBetween(1, 50));
+        return Base64.getEncoder().encodeToString(value);
     }
 
     @Override
diff --git 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() {

Original file line number	Diff line number	Diff line change
`@@ -168,7 +168,6 @@ public void writeTo(StreamOutput out) {`
`168`	`168`	`@Override`
`169`	`169`	`public String format(BytesRef value) {`
`170`	`170`	`return Base64.getEncoder()`
`171`		`- .withoutPadding()`
`172`	`171`	`.encodeToString(Arrays.copyOfRange(value.bytes, value.offset, value.offset + value.length));`
`173`	`172`	`}`
`174`	`173`