Skip to content

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

Merged
merged 5 commits into from
Jun 1, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
/*
* 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.MessageConversionException;
import org.springframework.messaging.converter.MessageConverter;
import org.springframework.messaging.converter.SimpleMessageConverter;
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 <T> the key type.
* @param <U> the value type.
* @param <V> the desired value type after conversion.
*
* @author Adrian Chlebosz
* @since 3.0
* @see AcknowledgingConsumerAwareMessageListener
*/
public class ConvertingMessageListener<T, U, V> implements AcknowledgingConsumerAwareMessageListener<T, U> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I look at this from the end-users perspective where they have to think what exactly need to be specified for the T and U when they really don't care about them since T is always going to be an original one and U is always going to be converted to V independently of U.
Therefore I find them redundant and want to avoid questions about them and trying to explain that we need them merely to satisfy an internal code consistency for compiler.
I believe we still can find some hard casting behavior internally even if it could be handled with a rawtypes suppression...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed in eeb9961


private final MessageListener<T, V> delegate;
private final MessageConverter messageConverter;
private final Class<V> desiredValueType;

/**
* 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 SimpleMessageConverter}.
*
* @param delegate 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<T, V> delegate, Class<V> desiredValueType) {
Assert.notNull(delegate, "Delegate message listener cannot be null");
Assert.notNull(desiredValueType, "Desired value type cannot be null");

this.delegate = delegate;
this.desiredValueType = desiredValueType;

this.messageConverter = new SimpleMessageConverter();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SimpleMessageConverter does not make too much sense for a logic of this listener.
It just does this: return (ClassUtils.isAssignableValue(targetClass, payload) ? payload : null);. So, if we expect in this converter whatever is there has been deserialized for us, then we just don't need to use this converter at all.
Perhaps a GenericMessageConverter would be better...

Copy link
Contributor Author

@breader124 breader124 Mar 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed in eeb9961

}

/**
* Construct an instance with the provided {@link MessageListener}, {@link MessageConverter} and {@link Class}
* as a desired type of {@link ConsumerRecord}'s value after conversion.
*
* @param delegate the {@link MessageListener} to use when passing converted {@link ConsumerRecord} further.
* @param messageConverter the {@link MessageConverter} to use for conversion.
* @param desiredValueType the {@link Class} setting desired type of {@link ConsumerRecord}'s value.
*/
public ConvertingMessageListener(MessageListener<T, V> delegate, MessageConverter messageConverter, Class<V> desiredValueType) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If messageConverter is optional and we have a default one, it is better to extract it into a setter instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed in eeb9961

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, stop making comments like this.
You just push commit to the PR and we are notified.
We also will see your changes and how they are related to the review we have left before.
Otherwise it is spamming my mail box: first your new commit and this "duplication" comments in their individual emails 😄

Assert.notNull(delegate, "Delegate message listener cannot be null");
Assert.notNull(messageConverter, "Message converter cannot be null");
Assert.notNull(desiredValueType, "Desired value type cannot be null");

this.delegate = delegate;
this.messageConverter = messageConverter;
this.desiredValueType = desiredValueType;
}

@Override
public void onMessage(ConsumerRecord<T, U> data, Acknowledgment acknowledgment, Consumer<?, ?> consumer) { // NOSONAR
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that we really need a // NOSONAR over here. The method doesn't look so complicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed in eeb9961

ConsumerRecord<T, V> 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be in an else block, otherwise the listener is called twice.

}

private ConsumerRecord<T, V> convertConsumerRecord(ConsumerRecord<T, U> data) { // NOSONAR
Header[] headerArray = data.headers().toArray();
Map<String, Object> headerMap = Arrays.stream(headerArray)
.collect(Collectors.toMap(Header::key, Header::value));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use a KafkaHeaderMapper here (configurable, with SimpleKafkaHeaderMapper default).

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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 ConsumerRecord<String, Foo> where Foo is an interface (or abstract class) with two implementations Bar and Baz; there might be type information in the headers to help the converter decide which one to instantiate.

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<U> message = new GenericMessage<>(data.value(), headerMap);
V converted = (V) 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 ConsumerRecord<T, V> rebuildConsumerRecord(ConsumerRecord<T, U> data, V converted) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method should be static.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed in eeb9961

return new ConsumerRecord<>(
data.topic(),
data.partition(),
data.offset(),
data.key(),
converted
Copy link
Contributor

Choose a reason for hiding this comment

The 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, Integer>) (data) -> assertThat(data.value()).isNotNull();
var convertingMessageListener = new ConvertingMessageListener<String, Integer, Integer>(
delegateListener,
Integer.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<String, String, ToBeConverted>(
delegateListener,
new MappingJackson2MessageConverter(),
ToBeConverted.class
);

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<String, String, ToBeConverted>(
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;
}
}

}