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

Unified access to headers for WebServiceConnections [SWS-894] #972

Closed
gregturn opened this issue Apr 3, 2015 · 12 comments
Closed

Unified access to headers for WebServiceConnections [SWS-894] #972

gregturn opened this issue Apr 3, 2015 · 12 comments

Comments

@gregturn
Copy link
Member

@gregturn gregturn commented Apr 3, 2015

Marten Deinum opened SWS-894 and commented

Currently when needing access to headers one needs to check the type of the WebServicConnection retrieve the HttpServletRequest and call getHeader on the resulting object.

We use both HTTP and JMS in our application and some of our interceptors are containing if statements to determine if it is HTTP or JMS and retrieve the headers appropriatly.

Would be nice if a unified interface HeadersAwareWebServiceConnection comes to mind. This interface would then have a getHeader method to retrieve the header. Maybe some additional methods like headerNames or containsHeader might be useful as well.

This would probably be useful for both HTTP flavors and JMS implementation of the WebServiceConnection.


Affects: 2.2.1

@gregturn
Copy link
Member Author

@gregturn gregturn commented Apr 17, 2015

Greg Turnquist commented

@Marten,

Care to share examples here of HTTP and JMS checks you have in place to retrieve the headers?

@gregturn
Copy link
Member Author

@gregturn gregturn commented Apr 20, 2015

Marten Deinum commented

I'm not at liberty to post any actual code but the code looks somewhat like the following...

TransportContext transportContext = TransportContextHolder.getTransportContext();
String header;
if (transportContext != null) {
    WebServiceConnection connection = transportContext.getConnection();
    if (connection instanceof HttpServletConnection} {
        HttpServletRequest request = ((HttpServletConnection) connection).getHttpServletRequest();
       header = request.getHeader(STATIC_HEADER_NAME);
    } else if (connection instanceof JmsReceiverConnection) {
        Iterator<String> headers = ((JmsReceiverConnection) connection).getHeaders(STATIC_HEADER_NAME);
        if (headers.hasNext()) {
            header = headers.next();
        }
   }
}

The check for null is in there because during testing there is no TransportContext and our tests would fail otherwise.

@gregturn
Copy link
Member Author

@gregturn gregturn commented Apr 20, 2015

Greg Turnquist commented

Thanks. I'll see what I can do.

@gregturn
Copy link
Member Author

@gregturn gregturn commented Apr 20, 2015

Marten Deinum commented

Looking closer at the API there is already something like that in the super class AbstractReceiverConnection which I didn't notice before... That already defines the methods like getHeaders etc.

For me that is enough...

@gregturn
Copy link
Member Author

@gregturn gregturn commented Apr 20, 2015

Marten Deinum commented

The API is protected however, not sure what would break if that would be made public and/or put in a feature specific interface.

@gregturn
Copy link
Member Author

@gregturn gregturn commented Apr 20, 2015

Greg Turnquist commented

I was aware of that, which is why I wanted to see your use case in more detail. There is the ability to find headers, but in some cases, they are request headers. In other concrete implementations, they are response headers.

I don't see widening visibility as an issue for backwards compatibility. If those methods appear to serve your needs and are visible through both JMS and HTTP variants, it might be possible to extract a common interface.

@gregturn
Copy link
Member Author

@gregturn gregturn commented Apr 20, 2015

Greg Turnquist commented

I've prototyped making the APIs public instead of protected. Check out https://github.com/spring-projects/spring-ws/commits/SWS-894, specifically 73a3edb. So far, it doesn't break any test cases.

I am trying to evaluate the original context of why they were made protected to see if the reason still applies or not.

@gregturn
Copy link
Member Author

@gregturn gregturn commented Apr 21, 2015

Arjen Poutsma commented

They were protected because they didn't need to be public (or at least: not until now). The less you expose, the less you have to keep backward compatible :).

Even though the AbstractReceiverConnection seems sufficient, I would still introduce a HeadersAwareWebServiceConnection interface, to be implemented by ARC. From the perspective of the user it is much nicer to depend on an interface instead of an abstract class.

@gregturn
Copy link
Member Author

@gregturn gregturn commented Apr 21, 2015

Arjen Poutsma commented

I would also expose the response headers by making addResponseHeader(String name, String value) public as well, and adding it to HeadersAwareWebServiceConnection, even though this JIRA does not request it. Seems only consistent.

@gregturn
Copy link
Member Author

@gregturn gregturn commented Apr 21, 2015

Greg Turnquist commented

I cleaned up that branch with said changes. Seeing that there is also an AbstractSenderConnection class with symmetrical properties, I created a similar interface for the other side and made similar adjustments.

@gregturn
Copy link
Member Author

@gregturn gregturn commented Apr 21, 2015

Greg Turnquist commented

Pull request has been fashioned here => #32

I'll let this simmer before merging.

@gregturn
Copy link
Member Author

@gregturn gregturn commented May 5, 2015

Greg Turnquist commented

Fixed via 4b179e8

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

Successfully merging a pull request may close this issue.

None yet
1 participant