From be4facef1b663849e9be33fb62a3cb444ae0d918 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Wed, 24 Jul 2019 18:44:04 +0100 Subject: [PATCH] Relax check on default data MimeType If there is more than one non-basic codec (e.g. CBOR and JSON) RSocketRequester.Builder takes the mime type of the first one rather than giving up. It is a valid scenario (JSON for server responding to browser, and CBOR for client talking to server) and it is the default situation in Boot, and after all the point here is to pick some default as best as we can with the worst possible outcome being a server refusing the connection if it doesn't support the mime type. Beyond that applications can set the dataMimeType on the builder explicitly. To match that change this commit also ensures RSocketMessageHandler rejects proactively data mime types it does not support at the point of accepting a connection. --- .../DefaultRSocketRequesterBuilder.java | 17 ++++-------- .../messaging/rsocket/RSocketRequester.java | 7 +++-- .../support/RSocketMessageHandler.java | 27 +++++++++++++++---- .../DefaultRSocketRequesterBuilderTests.java | 17 ------------ 4 files changed, 30 insertions(+), 38 deletions(-) diff --git a/spring-messaging/src/main/java/org/springframework/messaging/rsocket/DefaultRSocketRequesterBuilder.java b/spring-messaging/src/main/java/org/springframework/messaging/rsocket/DefaultRSocketRequesterBuilder.java index 247f66abee..197d77f967 100644 --- a/spring-messaging/src/main/java/org/springframework/messaging/rsocket/DefaultRSocketRequesterBuilder.java +++ b/spring-messaging/src/main/java/org/springframework/messaging/rsocket/DefaultRSocketRequesterBuilder.java @@ -145,21 +145,14 @@ final class DefaultRSocketRequesterBuilder implements RSocketRequester.Builder { if (this.dataMimeType != null) { return this.dataMimeType; } - // Look for non-basic Decoder (e.g. CBOR, Protobuf) - MimeType selected = null; - List> decoders = strategies.decoders(); - for (Decoder candidate : decoders) { + // First non-basic Decoder (e.g. CBOR, Protobuf) + for (Decoder candidate : strategies.decoders()) { if (!isCoreCodec(candidate) && !candidate.getDecodableMimeTypes().isEmpty()) { - Assert.state(selected == null, - () -> "Cannot select default data MimeType based on configured decoders: " + decoders); - selected = getMimeType(candidate); + return getMimeType(candidate); } } - if (selected != null) { - return selected; - } - // Fall back on 1st decoder (e.g. String) - for (Decoder decoder : decoders) { + // First core decoder (e.g. String) + for (Decoder decoder : strategies.decoders()) { if (!decoder.getDecodableMimeTypes().isEmpty()) { return getMimeType(decoder); } diff --git a/spring-messaging/src/main/java/org/springframework/messaging/rsocket/RSocketRequester.java b/spring-messaging/src/main/java/org/springframework/messaging/rsocket/RSocketRequester.java index ef9643632f..a1b834f6b3 100644 --- a/spring-messaging/src/main/java/org/springframework/messaging/rsocket/RSocketRequester.java +++ b/spring-messaging/src/main/java/org/springframework/messaging/rsocket/RSocketRequester.java @@ -128,11 +128,10 @@ public interface RSocketRequester { /** * Configure the payload data MimeType to specify on the {@code SETUP} * frame that applies to the whole connection. - *

If this is not set, the builder will try to select the mime type - * based on the presence of a single + *

If this is not set, it will be set to the MimeType of the first * {@link RSocketStrategies.Builder#decoder(Decoder[]) non-default} - * {@code Decoder}, or the first default decoder otherwise - * (i.e. {@code String}) if no others are configured. + * {@code Decoder}, or otherwise fall back on the MimeType of the first + * (default) decoder. */ RSocketRequester.Builder dataMimeType(@Nullable MimeType mimeType); diff --git a/spring-messaging/src/main/java/org/springframework/messaging/rsocket/annotation/support/RSocketMessageHandler.java b/spring-messaging/src/main/java/org/springframework/messaging/rsocket/annotation/support/RSocketMessageHandler.java index a66bf95379..52392bdb28 100644 --- a/spring-messaging/src/main/java/org/springframework/messaging/rsocket/annotation/support/RSocketMessageHandler.java +++ b/spring-messaging/src/main/java/org/springframework/messaging/rsocket/annotation/support/RSocketMessageHandler.java @@ -304,7 +304,13 @@ public class RSocketMessageHandler extends MessageMappingMessageHandler { */ public SocketAcceptor serverResponder() { return (setupPayload, sendingRSocket) -> { - MessagingRSocket responder = createResponder(setupPayload, sendingRSocket); + MessagingRSocket responder; + try { + responder = createResponder(setupPayload, sendingRSocket); + } + catch (Throwable ex) { + return Mono.error(ex); + } return responder.handleConnectionSetupPayload(setupPayload).then(Mono.just(responder)); }; } @@ -335,14 +341,15 @@ public class RSocketMessageHandler extends MessageMappingMessageHandler { String s = setupPayload.dataMimeType(); MimeType dataMimeType = StringUtils.hasText(s) ? MimeTypeUtils.parseMimeType(s) : this.defaultDataMimeType; Assert.notNull(dataMimeType, "No `dataMimeType` in ConnectionSetupPayload and no default value"); + Assert.isTrue(isDataMimeTypeSupported(dataMimeType), "Data MimeType '" + dataMimeType + "' not supported"); s = setupPayload.metadataMimeType(); - MimeType metadataMimeType = StringUtils.hasText(s) ? MimeTypeUtils.parseMimeType(s) : this.defaultMetadataMimeType; - Assert.notNull(metadataMimeType, "No `metadataMimeType` in ConnectionSetupPayload and no default value"); + MimeType metaMimeType = StringUtils.hasText(s) ? MimeTypeUtils.parseMimeType(s) : this.defaultMetadataMimeType; + Assert.notNull(metaMimeType, "No `metadataMimeType` in ConnectionSetupPayload and no default value"); RSocketStrategies strategies = this.rsocketStrategies; Assert.notNull(strategies, "No RSocketStrategies. Was afterPropertiesSet not called?"); - RSocketRequester requester = RSocketRequester.wrap(rsocket, dataMimeType, metadataMimeType, strategies); + RSocketRequester requester = RSocketRequester.wrap(rsocket, dataMimeType, metaMimeType, strategies); Assert.state(this.metadataExtractor != null, () -> "No MetadataExtractor. Was afterPropertiesSet not called?"); @@ -350,10 +357,20 @@ public class RSocketMessageHandler extends MessageMappingMessageHandler { Assert.state(getRouteMatcher() != null, () -> "No RouteMatcher. Was afterPropertiesSet not called?"); - return new MessagingRSocket(dataMimeType, metadataMimeType, this.metadataExtractor, requester, + return new MessagingRSocket(dataMimeType, metaMimeType, this.metadataExtractor, requester, this, getRouteMatcher(), strategies); } + private boolean isDataMimeTypeSupported(MimeType dataMimeType) { + for (Encoder encoder : getEncoders()) { + for (MimeType encodable : encoder.getEncodableMimeTypes()) { + if (encodable.isCompatibleWith(dataMimeType)) { + return true; + } + } + } + return false; + } public static ClientRSocketFactoryConfigurer clientResponder(Object... handlers) { return new ResponderConfigurer(handlers); diff --git a/spring-messaging/src/test/java/org/springframework/messaging/rsocket/DefaultRSocketRequesterBuilderTests.java b/spring-messaging/src/test/java/org/springframework/messaging/rsocket/DefaultRSocketRequesterBuilderTests.java index bd497b5860..d0ed124cd5 100644 --- a/spring-messaging/src/test/java/org/springframework/messaging/rsocket/DefaultRSocketRequesterBuilderTests.java +++ b/spring-messaging/src/test/java/org/springframework/messaging/rsocket/DefaultRSocketRequesterBuilderTests.java @@ -46,7 +46,6 @@ import org.springframework.util.MimeTypeUtils; import org.springframework.util.ReflectionUtils; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.BDDMockito.given; @@ -131,22 +130,6 @@ public class DefaultRSocketRequesterBuilderTests { .isEqualTo(MimeTypeUtils.APPLICATION_JSON); } - @Test - public void defaultDataMimeTypeWithMultipleCustomDecoderRegitered() { - RSocketStrategies strategies = RSocketStrategies.builder() - .decoder(new TestJsonDecoder(MimeTypeUtils.APPLICATION_JSON)) - .decoder(new TestJsonDecoder(MimeTypeUtils.APPLICATION_XML)) - .build(); - - assertThatThrownBy(() -> - RSocketRequester - .builder() - .rsocketStrategies(strategies) - .connect(this.transport) - .block()) - .hasMessageContaining("Cannot select default data MimeType"); - } - @Test public void dataMimeTypeSet() { RSocketRequester requester = RSocketRequester.builder()