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

Priority header not mapped to JMS Message #3179

Closed
micheljung opened this issue Feb 13, 2020 · 5 comments
Closed

Priority header not mapped to JMS Message #3179

micheljung opened this issue Feb 13, 2020 · 5 comments
Assignees
Milestone

Comments

@micheljung
Copy link
Contributor

Affects Version(s): 5.2.3.RELEASE

The DefaultJmsHeaderMapper copies message headers from Spring Integration Messages to JMS Messages.

According to Mapping Message Headers to and from JMS Message, the priority header should be mapped, too:

previously, the priority header was only used for outbound messages

However, this is not tested by DefaultJmsHeaderMapperTests and has been broken 5 years ago.
by this statement:

if (StringUtils.hasText(headerName) && !headerName.startsWith(JmsHeaders.PREFIX)
&& jmsMessage.getObjectProperty(headerName) == null) {

Since priority is often a primitive (in my case, org.apache.activemq.command.Message#priority is a byte but org.springframework.integration.jms.StubTextMessage#priority also uses int), checking it for null doesn't do the trick.

Interestingly, there's also spring-jms's SimpleJmsHeaderMapper which looks almost identical but behaves correctly (in this case):

https://github.com/spring-projects/spring-framework/blob/a4179b479599c62a89b5e67dbfa0d28ad61c638b/spring-jms/src/main/java/org/springframework/jms/support/SimpleJmsHeaderMapper.java#L99

@micheljung
Copy link
Contributor Author

My workaround:

...
Jms.outboundAdapter(connectionFactory)
        .configureJmsTemplate(spec -> spec.jmsMessageConverter(outboundMessageConverter()))
...

  // TODO remove once https://github.com/spring-projects/spring-integration/issues/3179 is fixed
  private static MessageConverter outboundMessageConverter() {
    return new SimpleMessageConverter() {

      @Override
      @SuppressWarnings({"NullableProblems", "ConstantConditions"})
      protected ObjectMessage createMessageForSerializable(Serializable object, Session session) throws JMSException {
        if (!(object instanceof Message)) {
          throw new IllegalStateException("Unexpected type: " + object.getClass());
        }
        ObjectMessage message = super.createMessageForSerializable(object, session);
        message.setJMSPriority(((Message<?>) object).getHeaders().get(MessageHeaders.PRIORITY, Integer.class));
        return message;
      }
    };
  }

@artembilan
Copy link
Member

We do map priority in Spring Integration with slightly different approach:

if (this.jmsTemplate instanceof DynamicJmsTemplate && this.jmsTemplate.isExplicitQosEnabled()) {
			Integer priority = StaticMessageHeaderAccessor.getPriority(message);
			if (priority != null) {
				DynamicJmsTemplateProperties.setPriority(priority);
			}

See JmsSendingMessageHandler. It is supplied with the DynamicJmsTemplate which has this code:

	public int getPriority() {
		Integer priority = DynamicJmsTemplateProperties.getPriority();
		if (priority == null) {
			return super.getPriority();
		}
		Assert.isTrue(priority >= 0 && priority <= 9, "JMS priority must be in the range of 0-9");
		return priority;
	}

So, consider to enable explicitQosEnabled(true) in that configureJmsTemplate().

@artembilan artembilan added the status: waiting-for-reporter Needs a feedback from the reporter label Feb 13, 2020
@artembilan
Copy link
Member

Now I recall the reason: not all JMS providers allow to modify properties like JMSPriority in messages before sending on the client.
That might work with ActiveMQ, but definitely might not work with others.

Also pay attention that you have been misled: the MessageHeaders.PRIORITY is not mapped into a setJMSPriority() in that SimpleJmsHeaderMapper because it is not a JMSPriority.
See the mentioned logic:

	if (StringUtils.hasText(headerName) && !headerName.startsWith(JmsHeaders.PREFIX)
						&& jmsMessage.getObjectProperty(headerName) == null) {

So, we try to check here a priority property. Meanwhile the real setJMSPriority() is mapped into a JMSPriority. See org.apache.activemq.filter.PropertyExpression for example.

Therefore a summary: or stay with your custom MessageConverter if your JMS provider allows to modify a message, or follow a solution we suggest out-of-the-box: the explicitQosEnabled(true) and respective MessageHeaders.PRIORITY header in the Message<?> you send to this Jms.outboundAdapter().

Does it make sense?

@micheljung
Copy link
Contributor Author

Does it make sense?

Let's say I understood enough of it :-) explicitQosEnabled(true) did the trick! Do you think this should be documented more clearly in the JMS documentation? I saw it's documented for outbound-gatway:

The default priority of request messages. Overridden by the message priority header, if present. Its range is 0 to 9. This setting takes effect only if explicit-qos-enabled is true.

Not however in Outbound Channel Adapter or Mapping Message Headers to and from JMS Message

Thank you for your help!

@artembilan
Copy link
Member

Yep! Indeed it has to be documented. And I definitely think that "Mapping headers" section is the right way to go.

Thanks for the pointer!

@artembilan artembilan added documentation type: enhancement and removed status: waiting-for-reporter Needs a feedback from the reporter labels Feb 18, 2020
@artembilan artembilan added this to the 5.3.M3 milestone Feb 18, 2020
@artembilan artembilan self-assigned this Feb 19, 2020
artembilan added a commit to artembilan/spring-integration that referenced this issue Feb 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants