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

Issue 116 - Header Propagation Part 2 #149

Merged
merged 3 commits into from Jan 11, 2019

Conversation

andymc12
Copy link
Contributor

@andymc12 andymc12 commented Jan 8, 2019

This should complete issue #116 - it adds the @RegisterClientHeaders annotation (by default this will propagate all opted-in JAX-RS headers specified via MP Config property) and the ClientHeadersFactory interface that a user could implement to generate headers themselves.

It includes TCK tests and spec updates. Note that the TCK does not test JAX-RS header propagation (it does test custom ClientHeadersFactory implementations) - the reason for this is because that would add dependencies on a JAX-RS server implementation - and would complicate the test environment. I have verified that my local CXF implementation will propagate JAX-RS headers (using CXF in Jetty as the JAX-RS server).

Signed-off-by: Andy McCright j.andrew.mccright@gmail.com

Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>
@andymc12 andymc12 added this to the 1.2 milestone Jan 8, 2019
@andymc12 andymc12 self-assigned this Jan 8, 2019
@eclipsewebmaster
Copy link
Contributor

Issue tracker reference:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=116

1 similar comment
@eclipsewebmaster
Copy link
Contributor

Issue tracker reference:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=116

@andymc12 andymc12 changed the title [116] Header Propagation Part 2 Issue 116 - Header Propagation Part 2 Jan 9, 2019
* {@link DefaultClientHeadersFactoryImpl} will be used. This implementation will simply propagate headers (specified via MP Config property)
* from an inbound JAX-RS request (if applicable) to the outbound request.
* <p>
* If a ClientHeadersFactory listed is not found on the classpath, it is ignored.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we want to ignore factory classes that are not available on classpath?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That line is intended to describe an error path behavior. For example, if a user has a rest client interface like:

@RegisterClientHeaders(MyMissingHeaderFactory.class)
public interface MyServiceWithMissingHeaderFactory { ...

and MyMissingHeaderFactory is not packaged with the app - and the implementation cannot find/load it - then the behavior would be to continue the request without invoking the default client headers factory. Ideally the implementation would also log a warning.

We could change this behavior so that if the class is missing, then we would throw a deployment exception - preventing the rest client instance from being created? I am fine either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't looked at implementing it yet but I think from the user perspective it would be better if it failed with an exception. It should speed up spotting the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I'll update and submit a new commit with this change.

@michalszynkiewicz
Copy link
Contributor

@andymc12 I don't fully understand what does "TCK does not test JAX-RS header propagation" mean.
Could you add a link to the CXF test for it that you mention?

@andymc12
Copy link
Contributor Author

andymc12 commented Jan 9, 2019

@andymc12 I don't fully understand what does "TCK does not test JAX-RS header propagation" mean.
Could you add a link to the CXF test for it that you mention?

Sure - it just means that the TCK tests a custom implementation of the the ClientHeadersFactory, but it doesn't test that the MP server correctly propagates HTTP headers using the DefaultClientHeadersFactoryImpl - the MP server implementation is supposed to propagate all listed incoming (via JAX-RS service) HTTP headers to the outgoing rest client request. There is a unit test that the DefaultClientHeadersFactoryImpl works as advertised, but nothing that tests that the MP server actually propagates the specified headers.

The test case that I used in CXF has not been merged yet, but you can see it in my branch at:
https://github.com/andymc12/cxf/tree/mpRest12initial-part2/systests/microprofile/client/jaxrs/src/test/java/org/apache/cxf/systest/microprofile/rest/client

Thanks!

Also adding test for custom ClientHeadersFactory.

Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>
Copy link
Contributor

@kenfinnigan kenfinnigan left a comment

Choose a reason for hiding this comment

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

Think it looks good, maybe just change copyright year to 2019?

Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>
@andymc12
Copy link
Contributor Author

Good catch @kenfinnigan ! I've updated the copyright dates to be 2019. Thanks for the review!

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.

None yet

4 participants