Skip to content
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

fix some defects of setting default Message headers in MessagingMessageListenerAdapter #2908

Closed

Conversation

NathanQingyangXu
Copy link
Contributor

@NathanQingyangXu NathanQingyangXu commented Nov 18, 2023

This PR aims to fix some minor defects in MessagingMessageListenerAdapter, including:

  • improved the checkHeaders() method
  • added the logic to apply checkHeaders() to Message element in Collection type return of method annotated with @SendTo
  • added a unit testing case covering default header setting for Iterable<Message<?>> return type

@@ -514,11 +515,11 @@ private Message<?> checkHeaders(Object result, String topic, @Nullable Object so
if (needsTopic) {
builder.setHeader(KafkaHeaders.TOPIC, topic);
}
if (needsCorrelation && sourceIsMessage) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems the second condtion has been implied in the first condition

builder.setHeader(this.correlationHeaderName,
((Message<?>) source).getHeaders().get(this.correlationHeaderName));
}
if (sourceIsMessage && reply.getHeaders().get(KafkaHeaders.REPLY_PARTITION) == null) {
Copy link
Contributor Author

@NathanQingyangXu NathanQingyangXu Nov 18, 2023

Choose a reason for hiding this comment

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

seems only PARTITION header matters for reply side, which has been covered in existing needsPartition variable.

@@ -501,24 +502,24 @@ else if (result instanceof Message) {
}
}

private Message<?> checkHeaders(Object result, String topic, @Nullable Object source) { // NOSONAR (complexity)
Message<?> reply = (Message<?>) result;
private Message<?> checkHeaders(Message<?> reply, @Nullable String topic, @Nullable Object source) { // NOSONAR (complexity)
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 think choosing Message<?> as the first argument type makes more sense, for this method is only meant to be used with that precondition.

Copy link
Contributor Author

@NathanQingyangXu NathanQingyangXu Nov 18, 2023

Choose a reason for hiding this comment

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

not sure whether we can use Nullable to topic, but from its invocation context, seems we could not rule out the nullness possibility?

@NathanQingyangXu NathanQingyangXu changed the title fix minor defect in MessagingMessageListenerAdapter#checkHeaders() fix some defects in MessagingMessageListenerAdapter#checkHeaders() Nov 19, 2023
@NathanQingyangXu NathanQingyangXu changed the title fix some defects in MessagingMessageListenerAdapter#checkHeaders() fix some defects in MessagingMessageListenerAdapter Nov 19, 2023
@NathanQingyangXu NathanQingyangXu changed the title fix some defects in MessagingMessageListenerAdapter fix some defects of setting default Message headers in MessagingMessageListenerAdapter Nov 19, 2023
@artembilan artembilan added this to the 3.1.1 milestone Dec 13, 2023
@sobychacko
Copy link
Contributor

@NathanQingyangXu Thanks for the PR! It is now merged upstream.

@NathanQingyangXu
Copy link
Contributor Author

@NathanQingyangXu Thanks for the PR! It is now merged upstream.

thanks. I really appreciate that the PR got merged finally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants