Consistent handling of HTTP response header across HTTP comp

[tfennelly]

This user forum thread raised an issue wrt to an inconsistency between how the HttpRouter is passing back Http response codes and how the JBossRemoting Gateway is extracting/expecting them from the message as it populates the JBoss Remoting response object.

The HttpRouter constructs a HttpResponse object. Into this, it populates the headers as HttpHeader instances, as well as setting the response payload. It then sets all this in a fixed (non-default) body location.

The SOAPClient doesn't map response headers at all, but it does use the MessagePayloadProxy for setting the response payload back on the ESB Message.

The JBR Gateway expects the http headers to be set as ResponseHeader instances in the message properties (org.jboss.soa.esb.message.ResponseHeader). It extracts the response payload from the message using a MessagePayloadProxy (so it expects it to be set in the Message Body).

The Embedded Tomcat Http Gateway works differently again. It expects the response headers to be set in a Map under a predefined (fixed) key in the message properties. It does however use the MessagePayloadProxy to get the response payload from the ESB Message.

So my preference would be:

1. All http invokers (HttpRouter, SOAPClient, ??) set the response headers in the message properties as ResponseHeader instances) and use the MessagePayloadProxy to set the response payload. The HttpRouter and SOAPClient both use the HttpClientFactory, so they should be able to use a common codebase for this.

2. All http gateways to expect the http response headers to be presented as outlined in #1 above.[/url]

[kconner]

David is looking into the consistent handling of headers as part of the webservice proxy work. I have assigned the issue over to him now that he has the right JIRA access permissions.

Kev

[dward]

How much freedom do I have here? Basically, how many people will I upset if message properties are no longer available under HttpMessageComposer.HTTP_RESPONSE_HEADER_MAP (http gateway) or HttpResponse.RESPONSE_KEY (http router)?

I've been poking around and it looks like a pretty easy thing to homogenize it all, but what about people who have written custom actions specifically looking for those exact message properties themsleves? I guess I could leave them there *in addition* to always using ResponseHeaders in the message properties, but then the problem is 1) the message sizes would be bigger, 2) it would be downright ugly.

I would like to just clean this up, and in release notes somewhere say, "if you're looking for HttpMessageComposer.HTTP_RESPONSE_HEADER_MAP or HttpResponse.RESPONSE_KEY, it ain't gonna be there no more".

Thoughts?

[dward]

(rather, message properties or body location)

[kconner]

In general we try to keep some level of backwards compatibility with these changes but there is going to be more disruption coming up for that provider in any case.

I think, at least for this one, that changing and documenting are good enough. Once we get Tom's new gateway in there will be more compatibility issues to address.

Does this sound reasonable?

Kev

[dward]

Hmmm... for some reason I didn't see a notification email that you responded to this thread.

Anyway, yes, it sounds reasonable. It actually makes it easier. Thanks; I'll get it in today.

[tfennelly]

We're just talking about the embedded http gateway here re breaking backward compatibility, right? Making this work for the HttpRouter, JBR Gateway etc will mean using the "in addition" approach talked about by Dave, with the data potentially ref'd by the message from multiple places, right?

[dward]

No, I thought it was a homogenization across the board... Kev, I thought we were clear on this but now please re-clarrify.

[kconner]

"tfennelly" wrote:
We're just talking about the embedded http gateway here re breaking backward compatibility, right? Making this work for the HttpRouter, JBR Gateway etc will mean using the "in addition" approach talked about by Dave, with the data potentially ref'd by the message from multiple places, right?

The only area we discussed in the meeting was the http gateway, which we already know is going, and my previous comment was definitely directed at that provider.

If this extends into the others then we need to discuss it further. Perhaps I misunderstood the question from earlier.

Kev

[kconner]

"dward" wrote:
No, I thought it was a homogenization across the board... Kev, I thought we were clear on this but now please re-clarrify.

The aim is certainly to have them consistent across the board, yes, and one of the options would be to break compatibility with the other areas.

Can you list all the affected areas here, describing what would be changed if we broke compatibility?

Kev

[kconner]

From what I can see we have three different areas
- http-gateway, we can break
- jbr-gateway, we can use a different composer
- httprouter, has a single location for responses

Lets wait and see how David is proposing to change these before commenting further.

Kev

[dward]

Okay, I have the following working in my local copy, and I can commit it to trunk ASAP if people agree. I think this is the best approach:

SOAPClient:
- response content added via payloadProxy [EXISTS - UNCHANGED]
- response headers added to message properties [MISSING - ADDED]

Wise SOAPClient:
- response content added via payloadProxy [EXISTS - UNCHANGED]
- response headers added to message properties [MISSING - UNCHANGED (reason is that there is no apparent API availabile in Wise to get out the http response headers)]

routing.HttpRouter:
(ignored since it is deprecated)

routing.http.HttpRouter:
- response content added via payloadProxy [EXISTS - UNCHANGED]
- response headers added to message properties [MISSING - ADDED]
- response headers available in body location HttpResponse.RESPONSE_KEY [EXISTS - UNCHANGED (left for backward compatibility)]

SOAPProxy:
(wraps use of routing.http.HttpRouter so it's all good)

SOAPProcessor:
- response content added via payloadProxy [EXISTS - UNCHANGED]
- response headers added to message properties [EXISTS - UNCHANGED]

HttpGatewayListener->HttpDispatchServlet->HttpMessageComposer
- payloadProxy body written to response output [EXISTS - UNCHANGED]
- message properties added to response headers [MISSING - ADDED]
- response headers available in message property HttpMessageComposer.HTTP_RESPONSE_HEADER_MAP [EXISTS - UNCHANGED (left for backward compatibility)]

JBossRemotingGatewayListener -> JBossRemotingMessageComposer
- payloadProxy body written to response output [EXISTS - UNCHANGED]
- message properties added to response headers [EXISTS - UNCHANGED]

Modified files:
product/rosetta/src/org/jboss/soa/esb/actions/routing/http/HttpRouter.java
product/rosetta/src/org/jboss/soa/esb/listeners/gateway/HttpGatewayListener.java
product/rosetta/src/org/jboss/soa/esb/listeners/gateway/HttpMessageComposer.java
product/rosetta/src/org/jboss/soa/esb/listeners/gateway/JBossRemotingGatewayListener.java
product/rosetta/src/org/jboss/soa/esb/message/ResponseHeader.java
product/services/soap/src/main/java/org/jboss/soa/esb/actions/soap/SOAPClient.java
product/services/soap/src/main/java/org/jboss/soa/esb/actions/soap/SOAPProcessor.java
product/services/soap/src/test/java/org/jboss/soa/esb/actions/soap/SOAPClient_Response_UnitTest.java

[dward]

"dward" wrote:
HttpGatewayListener->HttpDispatchServlet->HttpMessageComposer
- payloadProxy body written to response output [EXISTS - UNCHANGED]
- message properties added to response headers [MISSING - ADDED]
- response headers available in message property HttpMessageComposer.HTTP_RESPONSE_HEADER_MAP [EXISTS - UNCHANGED (left for backward compatibility)]

I meant to say:

HttpGatewayListener->HttpDispatchServlet->HttpMessageComposer
- payloadProxy body written to response output [EXISTS - UNCHANGED]
- message properties added to response headers [MISSING - ADDED]
- message property HttpMessageComposer.HTTP_RESPONSE_HEADER_MAP added to response headers [EXISTS - UNCHANGED (left for backward compatibility)]

[dward]

PS: You can test drive the above now here:
http://anonsvn.jboss.org/repos/labs/labs/jbossesb/workspace/dward/wsproxy/

[kconner]

David, this looks good. Lets get this into the codebase and we can take things from there.

Kev