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

Message Headers Improvement #1054

Merged
merged 5 commits into from
Dec 18, 2023
Merged

Message Headers Improvement #1054

merged 5 commits into from
Dec 18, 2023

Conversation

scottf
Copy link
Contributor

@scottf scottf commented Dec 13, 2023

When a message is made, copy the headers and make them read only. Currently headers could be modified since they are just a reference to what the dev put in the message, but the length calculations are already done and changing headers will have a bad side effect.

@scottf scottf merged commit 4d7b99d into main Dec 18, 2023
1 check passed
@scottf scottf deleted the new-message-copies-headers branch December 18, 2023 16:39
}
this.readOnly = readOnly;
if (readOnly) {
valuesMap = Collections.unmodifiableMap(tempValuesMap);
Copy link

@sdklib sdklib Jan 4, 2024

Choose a reason for hiding this comment

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

Collections$UnmodifiableMap throws an UnsupportedOperationException when computeIfAbsent is called which is happening in the _add method. Unsure if this is really intended

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sdklib Are you trying to modify the headers after the message is created? This PR was intended to prevent this.

Copy link

@sdklib sdklib Jan 4, 2024

Choose a reason for hiding this comment

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

Yup, we're using AOP to add a correlation id for proper logging across microservices to the headers.

After updating to the newest version, some unit tests fail, due to readOnly being true and the AOP Pointcut failing to add the correlation ID to the header.

The fix was easy, I was just wondering if the response should really be UnsupportedOperationException or maybe better returning a Headers.java (e.g. ReadOnlyHeaders.java) clone that doesn't have the add method in the first place. Or something similar~ :-)

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 would have to change the api to return a different object, and this would require a major version. Also, before, since I did not effectively copy the headers, the original headers object could have been modified. I'm updating the readme with a note about the change for the 2.17.2 version.

Copy link

Choose a reason for hiding this comment

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

Feared so, never the less, thank you so much! 👍

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

Successfully merging this pull request may close these issues.

2 participants