MethodParameterMessageMapper - javadoc vs code [INT-656]

[spring-operator]

Jérôme Delagnes opened INT-656 and commented

Hello,

MethodParameterMessageMapper javadoc says:

However, if a Map or Properties object is expected, and the payload is not itself assignable to that type, then the MessageHeaders' values will be passed in the case of a Map-typed parameter, or the MessageHeaders' String-based values will be passed in the case of a Properties-typed parameter.

So, if I use the following method:

public class TestService {
    public String headersAndPayload(Map<String, Object> headers, String payload) {
    ...
    }
}

I expect:

• in headers parameter: the message headers
• in the the payload parameter: the message payload

But it's not the case.

Do I misunderstand the javadoc ?

---

Affects: 1.0.2.SR1

Attachments:

• MethodParameterMessageMapper-patch.zip (4.89 kB)

[spring-operator]

Jérôme Delagnes commented

Here the patch and the updated test cases

[spring-operator]

Iwein Fuld commented

Thanks, Keep 'm coming!

I don't think you've signed committer agreements yet, so I am not allowed to look at you patch %). I think I can manage without fortunately. Soon we'll have a webbased solution for agreeing to the terms.

[spring-operator]

Jérôme Delagnes commented

No problem let me known when the webbased solution will be up. Where can i find the terms ?

[spring-operator]

Iwein Fuld commented

Mark I noted the following while refactoring MethodParameterMessageMapper:

• it seems we can make @Headers almost obsolete (
• the semantics should be better defined, now there are hardly any specs, so maintaining backwards compatibility is a swamp
• should the default headers (timestamp, id) be passed in too? I found that this does happen in case of a Map, but not in case of a Properties, which seems like a quirk.

From the code this last bit seems intentional:

else if (metadata.hasHeadersAnnotation()) {
     if (Properties.class.isAssignableFrom(expectedType)) {
                                                                                                                                                                                                                                                                                                  args[i] = this.getStringTypedHeaders(message);                    
     }
     else {
                                                                                                                                                                                                                          args[i] = message.getHeaders();                   
     }
}

Jerome, could you have a critical look at my fix and see if this is what you're looking for (just click Fisheye to see related commits)? The whole thing is a bit more tricky than I hoped for, but I think I'm on the right track.

[spring-operator]

Iwein Fuld commented

I finished up with some extra javadoc showing some examples.

I prefer a careful review of this as the changes cut quite deep. Getting the semantics defined properly is important I think, because maintenance on this type of code is going to be time consuming in the best case scenario.

[spring-operator]

Jérôme Delagnes commented

Iwein, unfortunately I don't look your fix yet because I've been quite busy since the beginning of June :-(

When I created this issue, my first objective was to get message headers without using spring annotation (@Headers, @Header).

According to the MethodParameterMessageMapper documentation I believed that was possible but when I tried I found some differences between code and javadoc (test with: service-activator, transformer, outbound-channel-adapter).

It's not working because of some blocking points:

1. MethodParameterMessageMapper does not correctly passes headers value to Properties/Map parameter (this issue).
2. HandlerMethodUtils.isValidHandlerMethod allows only one method parameter without "header" annotation.

If I use the hello world spring integration sample, I would like to access headers as follows (without any configuration change):

public class HelloService {
    public String sayHello(Map<String, Object> headers, String name) {
            // ... do something with headers ...
        return "Hello " + name;
    }
}

Well, another point I want you to talk about is error handling.

In th same way I want my java beans free of spring integration java imports. So I'd like to manage errors in the following way (argument order doesn't matter) :

public class ErrorHanldlingPossibleMethods {
   /**
    * @param headers   - headers of the failed message
    * @param payload   - payload of the failed message
    * @param throwable - cause of the error
    */
   public void handleError(Map<String, Object> headers, String payload, Throwable throwable) {
     // ... do something ...
   }

   /**
    * @param headers   - headers of the failed message
    * @param payload   - payload of the failed message
    */
   public void handleError(Map<String, Object> headers, String payload) {
     // ... do something ...
   }

   /**
    * @param headers   - headers of the failed message
    * @param throwable - cause of the error
    */
   public void handleError(Map<String, Object> headers, Throwable throwable) {
     // ... do something ...
   }

   /**
    * @param headers   - headers of the failed message
    */
   public void handleError(Map<String, Object> headers) {
     // ... do something ...
   }

   /**
    * @param payload   - payload of the failed message
    * @param throwable - cause of the error
    */
   public void handleError(String payload, Throwable throwable) {
     // ... do something ...
   }

   /**
    * @param payload   - payload of the failed message
    */
   public void handleError(String payload) {
     // ... do something ...
   }

   /**
    * @param throwable - cause of the error
    */
   public void handleError(Throwable throwable) {
     // ... do somthing ...
   }
}

The semantics is acceptable for you ? Do I must create a separated issue for this point ?

[spring-operator]

Mark Fisher commented

Jérôme,

Please create a separate issue for the error handing ideas. We will consider this in 2.0. Also, with EL support there will be more flexibility for binding message content to method arguments in general.

Regards, Mark

[spring-operator]

Mark Fisher commented

I am marking this as resolved after the latest changes today.

Please click on the Fisheye tab for this issue and explore the various commit logs for more detail. In particular, the test cases provide many examples of valid and and invalid methods backing up the javadoc that has been updated in MethodParameterMessageMapper.

[spring-operator]

Iwein Fuld commented

A test is incorrect. fromMessageWithMapMethodAndMapPayload would also pass if the headers would be passed in instead of the payload, I suggest making a difference in the maps, like below.

@Test
@SuppressWarnings("unchecked")
public void fromMessageWithMapMethodAndMapPayload() throws Exception {
     Method method = TestService.class.getMethod("mapPayload", Map.class);
     MethodParameterMessageMapper mapper = new MethodParameterMessageMapper(method);
     Map<String, Integer> payload = new HashMap<String, Integer>();
     payload.put("attrib1", new Integer(124));
     payload.put("attrib2", new Integer(356));
     Message<Map<String, Integer>> message = MessageBuilder.withPayload(payload)
               .setHeader("attrib1", new Integer(123))
               .setHeader("attrib2", new Integer(456)).build();
     Object[] args = mapper.fromMessage(message);
     Map<String, Integer> result = (Map<String, Integer>) args[0];
     assertEquals(2, result.size());
     assertEquals(new Integer(124), result.get("attrib1"));
     assertEquals(new Integer(356), result.get("attrib2"));
}

[spring-operator]

Mark Fisher commented

Thanks for spotting that Iwein! I've updated the test method accordingly (and it does still pass).