From 4182935b7a61a3ba02b2f9fa3db0b50c918bdb06 Mon Sep 17 00:00:00 2001 From: Arjen Poutsma Date: Tue, 13 Nov 2018 10:55:54 +0100 Subject: [PATCH] Revert optimization in StringDecoder This commit reverts the first optimizations listed in fa096dc60f0e78dd1d0d061f9fb7053954a52fb4, as the default delimiters do vary, namely by the charset given in the message mime type. The mimetype charset might not be compatible with ASCII (i.e. anything but UTF-8 or ISO-8859-1, for instance it might be UTF-16), and will not successfully find the default delimiters as a consequence. Added test to indicate the bug. --- .../core/codec/StringDecoder.java | 40 ++++++++++--------- .../core/codec/StringDecoderTests.java | 40 +++++++++++++++---- 2 files changed, 54 insertions(+), 26 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/core/codec/StringDecoder.java b/spring-core/src/main/java/org/springframework/core/codec/StringDecoder.java index 7825c3f8bc9..8aca34f420c 100644 --- a/spring-core/src/main/java/org/springframework/core/codec/StringDecoder.java +++ b/spring-core/src/main/java/org/springframework/core/codec/StringDecoder.java @@ -23,6 +23,8 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; import java.util.stream.Collectors; import org.reactivestreams.Publisher; @@ -35,6 +37,7 @@ import org.springframework.core.io.buffer.DefaultDataBufferFactory; import org.springframework.core.io.buffer.PooledDataBuffer; import org.springframework.core.log.LogFormatUtils; import org.springframework.lang.Nullable; +import org.springframework.util.Assert; import org.springframework.util.MimeType; import org.springframework.util.MimeTypeUtils; @@ -63,20 +66,18 @@ public final class StringDecoder extends AbstractDataBufferDecoder { /** The default delimiter strings to use, i.e. {@code \r\n} and {@code \n}. */ public static final List DEFAULT_DELIMITERS = Arrays.asList("\r\n", "\n"); - private static final List DEFAULT_DELIMITER_BYTES = DEFAULT_DELIMITERS.stream() - .map(str -> str.getBytes(StandardCharsets.UTF_8)) - .collect(Collectors.toList()); - - @Nullable private final List delimiters; private final boolean stripDelimiter; + private final ConcurrentMap> delimitersCache = new ConcurrentHashMap<>(); + - private StringDecoder(@Nullable List delimiters, boolean stripDelimiter, MimeType... mimeTypes) { + private StringDecoder(List delimiters, boolean stripDelimiter, MimeType... mimeTypes) { super(mimeTypes); - this.delimiters = delimiters != null ? new ArrayList<>(delimiters) : null; + Assert.notEmpty(delimiters, "'delimiters' must not be empty"); + this.delimiters = new ArrayList<>(delimiters); this.stripDelimiter = stripDelimiter; } @@ -90,9 +91,7 @@ public final class StringDecoder extends AbstractDataBufferDecoder { public Flux decode(Publisher inputStream, ResolvableType elementType, @Nullable MimeType mimeType, @Nullable Map hints) { - List delimiterBytes = this.delimiters != null ? - this.delimiters.stream().map(s -> s.getBytes(getCharset(mimeType))).collect(Collectors.toList()) : - DEFAULT_DELIMITER_BYTES; + List delimiterBytes = getDelimiterBytes(mimeType); Flux inputFlux = Flux.from(inputStream) .flatMapIterable(dataBuffer -> splitOnDelimiter(dataBuffer, delimiterBytes)) @@ -103,6 +102,13 @@ public final class StringDecoder extends AbstractDataBufferDecoder { return super.decode(inputFlux, elementType, mimeType, hints); } + private List getDelimiterBytes(@Nullable MimeType mimeType) { + return this.delimitersCache.computeIfAbsent(getCharset(mimeType), + charset -> this.delimiters.stream() + .map(s -> s.getBytes(charset)) + .collect(Collectors.toList())); + } + /** * Splits the given data buffer on delimiter boundaries. The returned Flux contains a * {@link #END_FRAME} buffer after each delimiter. @@ -234,17 +240,16 @@ public final class StringDecoder extends AbstractDataBufferDecoder { * Create a {@code StringDecoder} for {@code "text/plain"}. */ public static StringDecoder textPlainOnly() { - return textPlainOnly(null, true); + return textPlainOnly(DEFAULT_DELIMITERS, true); } /** * Create a {@code StringDecoder} for {@code "text/plain"}. - * @param delimiters delimiter strings to use to split the input stream, if - * {@code null} by default {@link #DEFAULT_DELIMITERS} is used. + * @param delimiters delimiter strings to use to split the input stream * @param stripDelimiter whether to remove delimiters from the resulting * input strings. */ - public static StringDecoder textPlainOnly(@Nullable List delimiters, boolean stripDelimiter) { + public static StringDecoder textPlainOnly(List delimiters, boolean stripDelimiter) { return new StringDecoder(delimiters, stripDelimiter, new MimeType("text", "plain", DEFAULT_CHARSET)); } @@ -263,17 +268,16 @@ public final class StringDecoder extends AbstractDataBufferDecoder { * Create a {@code StringDecoder} that supports all MIME types. */ public static StringDecoder allMimeTypes() { - return allMimeTypes(null, true); + return allMimeTypes(DEFAULT_DELIMITERS, true); } /** * Create a {@code StringDecoder} that supports all MIME types. - * @param delimiters delimiter strings to use to split the input stream, if - * {@code null} by default {@link #DEFAULT_DELIMITERS} is used. + * @param delimiters delimiter strings to use to split the input stream * @param stripDelimiter whether to remove delimiters from the resulting * input strings. */ - public static StringDecoder allMimeTypes(@Nullable List delimiters, boolean stripDelimiter) { + public static StringDecoder allMimeTypes(List delimiters, boolean stripDelimiter) { return new StringDecoder(delimiters, stripDelimiter, new MimeType("text", "plain", DEFAULT_CHARSET), MimeTypeUtils.ALL); } diff --git a/spring-core/src/test/java/org/springframework/core/codec/StringDecoderTests.java b/spring-core/src/test/java/org/springframework/core/codec/StringDecoderTests.java index 60effa9b012..cc8bca59184 100644 --- a/spring-core/src/test/java/org/springframework/core/codec/StringDecoderTests.java +++ b/spring-core/src/test/java/org/springframework/core/codec/StringDecoderTests.java @@ -16,7 +16,7 @@ package org.springframework.core.codec; -import java.nio.charset.StandardCharsets; +import java.nio.charset.Charset; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -29,8 +29,11 @@ import reactor.test.StepVerifier; import org.springframework.core.ResolvableType; import org.springframework.core.io.buffer.AbstractDataBufferAllocatingTestCase; import org.springframework.core.io.buffer.DataBuffer; +import org.springframework.util.MimeType; import org.springframework.util.MimeTypeUtils; +import static java.nio.charset.StandardCharsets.UTF_16BE; +import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.*; /** @@ -69,22 +72,43 @@ public class StringDecoderTests extends AbstractDataBufferAllocatingTestCase { @Test public void decodeMultibyteCharacter() { - String s = "üéø"; - Flux source = toSingleByteDataBuffers(s); + String u = "ü"; + String e = "é"; + String o = "ø"; + String s = String.format("%s\n%s\n%s", u, e, o); + Flux source = toDataBuffers(s, 1, UTF_8); Flux output = this.decoder.decode(source, ResolvableType.forClass(String.class), null, Collections.emptyMap()); StepVerifier.create(output) - .expectNext(s) + .expectNext(u, e, o) .verifyComplete(); } - private Flux toSingleByteDataBuffers(String s) { - byte[] bytes = s.getBytes(StandardCharsets.UTF_8); + @Test + public void decodeMultibyteCharacterUtf16() { + String u = "ü"; + String e = "é"; + String o = "ø"; + String s = String.format("%s\n%s\n%s", u, e, o); + Flux source = toDataBuffers(s, 2, UTF_16BE); + + MimeType mimeType = MimeTypeUtils.parseMimeType("text/plain;charset=utf-16be"); + Flux output = this.decoder.decode(source, ResolvableType.forClass(String.class), + mimeType, Collections.emptyMap()); + StepVerifier.create(output) + .expectNext(u, e, o) + .verifyComplete(); + } + + private Flux toDataBuffers(String s, int length, Charset charset) { + byte[] bytes = s.getBytes(charset); List dataBuffers = new ArrayList<>(); - for (byte b : bytes) { - dataBuffers.add(this.bufferFactory.wrap(new byte[]{b})); + for (int i = 0; i < bytes.length; i += length) { + DataBuffer dataBuffer = this.bufferFactory.allocateBuffer(length); + dataBuffer.write(bytes, i, length); + dataBuffers.add(dataBuffer); } return Flux.fromIterable(dataBuffers); }