SOLR-15111: Use JDK8 Base64 instead of own implementation#2252
SOLR-15111: Use JDK8 Base64 instead of own implementation#2252asalamon74 wants to merge 1 commit intoapache:masterfrom
Conversation
madrob
left a comment
There was a problem hiding this comment.
I'm a little surprised to see StandardCharsets.ISO_8859_1 in use, most of the bytes we work with are UTF-8. Can you clarify?
| BytesRef bytes = field.binaryValue(); | ||
| if (bytes != null) { | ||
| f.add( "binary", Base64.byteArrayToBase64(bytes.bytes, bytes.offset, bytes.length)); | ||
| f.add( "binary", StandardCharsets.ISO_8859_1.decode(Base64.getEncoder().encode(bytes.wrapToByteBuffer())).toString()); |
There was a problem hiding this comment.
I don't think this is correct, why are we decoding something that we just encoded?
|
ISO_8859_1 In the first version I used UTF_8 but later I checked the source code of java.util.Base64 ( http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/util/Base64.java ) and they are using ISO-8859-1 internally, so I thought to use the same. Probably we could use both character encodings, since base64 encoded strings only use a small subset of the characters. encode + decode First I base64 encode the |
|
Unfortunately, we can’t use the solution provided from stack overflow -http://apache.org/legal/resolved.html#stackoverflow Can you contact the original author and ask for permission to use this? Otherwise we will need somebody who hasn’t looked this code to create a clean room implementation. |
0d759fc to
856d60d
Compare
|
We also used a different way for String conversion, I modified the lines. |
Description
JDK8 has a builtin Base64 encoder and decoder, there is no need to use own implementaion for this.
Solution
Eliminate own implementation.
Tests
Unit tests.
Checklist
Please review the following and check all that apply:
masterbranch../gradlew check.