-
Notifications
You must be signed in to change notification settings - Fork 1.6k
GH-2068: Add lightweight converting adapter #2165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
9003aa5
603202f
eeb9961
8430134
a21b3fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,127 @@ | ||
/* | ||
* Copyright 2016-2022 the original author or authors. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* https://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package org.springframework.kafka.listener.adapter; | ||
|
||
import java.util.Arrays; | ||
import java.util.Map; | ||
import java.util.stream.Collectors; | ||
|
||
import org.apache.kafka.clients.consumer.Consumer; | ||
import org.apache.kafka.clients.consumer.ConsumerRecord; | ||
import org.apache.kafka.common.header.Header; | ||
|
||
import org.springframework.kafka.listener.AcknowledgingConsumerAwareMessageListener; | ||
import org.springframework.kafka.listener.AcknowledgingMessageListener; | ||
import org.springframework.kafka.listener.ConsumerAwareMessageListener; | ||
import org.springframework.kafka.listener.MessageListener; | ||
import org.springframework.kafka.support.Acknowledgment; | ||
import org.springframework.messaging.Message; | ||
import org.springframework.messaging.converter.GenericMessageConverter; | ||
import org.springframework.messaging.converter.MessageConversionException; | ||
import org.springframework.messaging.converter.MessageConverter; | ||
import org.springframework.messaging.support.GenericMessage; | ||
import org.springframework.util.Assert; | ||
|
||
/** | ||
* A {@link AcknowledgingConsumerAwareMessageListener} adapter that implements | ||
* converting received {@link ConsumerRecord} using specified {@link MessageConverter} | ||
* and then passes result to specified {@link MessageListener}. | ||
* | ||
* @param <V> the desired value type after conversion. | ||
* | ||
* @author Adrian Chlebosz | ||
* @since 3.0 | ||
* @see AcknowledgingConsumerAwareMessageListener | ||
*/ | ||
@SuppressWarnings("rawtypes") | ||
public class ConvertingMessageListener<V> implements AcknowledgingConsumerAwareMessageListener<Object, Object> { | ||
|
||
private final MessageListener delegate; | ||
private final Class<V> desiredValueType; | ||
|
||
private MessageConverter messageConverter; | ||
|
||
/** | ||
* Construct an instance with the provided {@link MessageListener} and {@link Class} | ||
* as a desired type of {@link ConsumerRecord}'s value after conversion. Default value of | ||
* {@link MessageConverter} is used, which is {@link GenericMessageConverter}. | ||
* | ||
* @param delegateMessageListener the {@link MessageListener} to use when passing converted {@link ConsumerRecord} further. | ||
* @param desiredValueType the {@link Class} setting desired type of {@link ConsumerRecord}'s value. | ||
*/ | ||
public ConvertingMessageListener(MessageListener<?, V> delegateMessageListener, Class<V> desiredValueType) { | ||
Assert.notNull(delegateMessageListener, "'delegateMessageListener' cannot be null"); | ||
Assert.notNull(desiredValueType, "'desiredValueType' cannot be null"); | ||
this.delegate = delegateMessageListener; | ||
this.desiredValueType = desiredValueType; | ||
|
||
this.messageConverter = new GenericMessageConverter(); | ||
} | ||
|
||
/** | ||
* Set a {@link MessageConverter}. | ||
* @param messageConverter the message converter to use for conversion of incoming {@link ConsumerRecord}. | ||
* @since 3.0 | ||
*/ | ||
public void setMessageConverter(MessageConverter messageConverter) { | ||
Assert.notNull(messageConverter, "'messageConverter' cannot be null"); | ||
this.messageConverter = messageConverter; | ||
} | ||
|
||
@Override | ||
@SuppressWarnings("unchecked") | ||
public void onMessage(ConsumerRecord data, Acknowledgment acknowledgment, Consumer consumer) { | ||
ConsumerRecord convertedConsumerRecord = convertConsumerRecord(data); | ||
if (this.delegate instanceof AcknowledgingConsumerAwareMessageListener) { | ||
this.delegate.onMessage(convertedConsumerRecord, acknowledgment, consumer); | ||
} | ||
else if (this.delegate instanceof ConsumerAwareMessageListener) { | ||
this.delegate.onMessage(convertedConsumerRecord, consumer); | ||
} | ||
else if (this.delegate instanceof AcknowledgingMessageListener) { | ||
this.delegate.onMessage(convertedConsumerRecord, acknowledgment); | ||
} | ||
|
||
this.delegate.onMessage(convertedConsumerRecord); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needs to be in an |
||
} | ||
|
||
private ConsumerRecord convertConsumerRecord(ConsumerRecord data) { | ||
Header[] headerArray = data.headers().toArray(); | ||
Map<String, Object> headerMap = Arrays.stream(headerArray) | ||
.collect(Collectors.toMap(Header::key, Header::value)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should use a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I analysed the code one more time and figured out that creating GenericMessage with headers is pointless, because they don't play any role in conversion of message payload. Also in newly built ConsumerRecord they are just copied form received record. Based on that I deleted these lines related to extracting headers and take care only about payload in conversion. What's your opinion? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only thought I had was if someone might want access to the headers to decide the conversion strategy. Let's say, the listener is expecting But, I agree, it would be unfortunate to take the overhead of mapping the headers when the converter doesn't need them. I would therefore recommend supporting a header mapper property, but only map the headers if one is provided. |
||
|
||
Message message = new GenericMessage<>(data.value(), headerMap); | ||
Object converted = this.messageConverter.fromMessage(message, this.desiredValueType); | ||
|
||
if (converted == null) { | ||
throw new MessageConversionException(message, "Message cannot be converted by used MessageConverter"); | ||
} | ||
|
||
return rebuildConsumerRecord(data, converted); | ||
} | ||
|
||
private static ConsumerRecord rebuildConsumerRecord(ConsumerRecord data, Object converted) { | ||
return new ConsumerRecord<>( | ||
data.topic(), | ||
data.partition(), | ||
data.offset(), | ||
data.key(), | ||
converted | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about the headers and other metadata (timestamp etc.) ? |
||
); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,112 @@ | ||
/* | ||
* Copyright 2016-2022 the original author or authors. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* https://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package org.springframework.kafka.listener.adapter; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
||
import org.apache.kafka.clients.consumer.ConsumerRecord; | ||
import org.junit.jupiter.api.Test; | ||
|
||
import org.springframework.kafka.listener.MessageListener; | ||
import org.springframework.messaging.converter.MappingJackson2MessageConverter; | ||
import org.springframework.messaging.converter.MessageConversionException; | ||
|
||
import com.fasterxml.jackson.core.JsonProcessingException; | ||
import com.fasterxml.jackson.databind.ObjectMapper; | ||
|
||
/** | ||
* @author Adrian Chlebosz | ||
* @since 3.0.0 | ||
* | ||
*/ | ||
class ConvertingMessageListenerTests { | ||
|
||
private final ObjectMapper mapper = new ObjectMapper(); | ||
|
||
@Test | ||
public void testMessageListenerIsInvokedWithConvertedSimpleRecord() { | ||
var consumerRecord = new ConsumerRecord<>("foo", 0, 0, "key", 0); | ||
|
||
var delegateListener = (MessageListener<String, Long>) (data) -> assertThat(data.value()).isNotNull(); | ||
var convertingMessageListener = new ConvertingMessageListener<>( | ||
delegateListener, | ||
Long.class | ||
); | ||
|
||
convertingMessageListener.onMessage(consumerRecord, null, null); | ||
} | ||
|
||
@Test | ||
public void testMessageListenerIsInvokedWithRecordConvertedByCustomConverter() throws JsonProcessingException { | ||
var toBeConverted = new ToBeConverted("foo"); | ||
var toBeConvertedJson = mapper.writeValueAsString(toBeConverted); | ||
var consumerRecord = new ConsumerRecord<>("foo", 0, 0, "key", toBeConvertedJson); | ||
|
||
var delegateListener = (MessageListener<String, ToBeConverted>) (data) -> { | ||
assertThat(data.value()).isNotNull(); | ||
assertThat(data.value().getA()).isEqualTo("foo"); | ||
}; | ||
var convertingMessageListener = new ConvertingMessageListener<>( | ||
delegateListener, | ||
ToBeConverted.class | ||
); | ||
convertingMessageListener.setMessageConverter(new MappingJackson2MessageConverter()); | ||
|
||
convertingMessageListener.onMessage(consumerRecord, null, null); | ||
} | ||
|
||
@Test | ||
public void testConversionFailsWhileUsingDefaultConverterForComplexObject() throws JsonProcessingException { | ||
var toBeConverted = new ToBeConverted("foo"); | ||
var toBeConvertedJson = mapper.writeValueAsString(toBeConverted); | ||
var consumerRecord = new ConsumerRecord<>("foo", 0, 0, "key", toBeConvertedJson); | ||
|
||
var delegateListener = (MessageListener<String, ToBeConverted>) (data) -> { | ||
assertThat(data.value()).isNotNull(); | ||
assertThat(data.value().getA()).isEqualTo("foo"); | ||
}; | ||
var convertingMessageListener = new ConvertingMessageListener<>( | ||
delegateListener, | ||
ToBeConverted.class | ||
); | ||
|
||
assertThatThrownBy( | ||
() -> convertingMessageListener.onMessage(consumerRecord, null, null) | ||
).isInstanceOf(MessageConversionException.class); | ||
} | ||
|
||
private static class ToBeConverted { | ||
private String a; | ||
|
||
ToBeConverted() { | ||
} | ||
|
||
ToBeConverted(String a) { | ||
this.a = a; | ||
} | ||
|
||
public String getA() { | ||
return a; | ||
} | ||
|
||
public void setA(String a) { | ||
this.a = a; | ||
} | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also implement
DelegatingMessageListener
so the container can determine what type of listener the delegate is.