-
Notifications
You must be signed in to change notification settings - Fork 11
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
Flexible HttpClient configuration for interactions with Fedora #113
Conversation
This provides a new configuration parameter for org.fcrepo.apix.jena.cfg: The httpclient.osgi.jndi.service.name property can be used to specify the service name in OSGi for HttpClient implementations. This allows the use of custom-configured HttpClient instances by the various API-X registries when communicating with Fedora.
*/ | ||
public CloseableHttpClient getClient() throws Exception { | ||
|
||
final Collection<ServiceReference<CloseableHttpClient>> refs = bundleContext.getServiceReferences( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a lot of debugging, I finally realized that the reason I haven't gotten my PR to work against your PR is because I am publishing an HttpClient
and you are expecting a CloseableHttpClient
. What is the reason you are requiring a CloseableHttpClient
(which is an impl class) instead of the interface...? Presumably, you are not actually closing the client...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it's because CloseableHttpClient
is the only way to get CloseableHttpResponse
s that can be used with try-with-resources. I think we'd have to resort to casting or factor out the try-with-resources in order to support plain ol' HttpClient. Casting in HttpClientFetcher might actually work. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first thought is that this isn't our fault, this is a real flaw in the API. I hope they are fixing this for 5. Be that as it may, and given that the only actual impls of the interface that come with Commons itself are all CloseableHttpClient
s, I don't have a big problem with casting (accompanied by a TODO
and even maybe a ticket to remind to do better when we can).
Alternatively, we can register, not the client, but the interceptor. IOW, you build your own client in API-X (presumably via one of the HttpClientBuilderFactory
services that HTTP Commons registers for you) and draw any HttpRequestInterceptor
s or HttpResponsenterceptor
s registered under the right filters and pack them on.
Does either sound inherently more appealing? The latter is "cleaner", but really starts to tangle the Commons HTTP client API into our code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, an API flaw (probably for the sake of backward compatibility with pre-java-7).
Of all those, I think casting is more appealing. I don't know enough about HttpClient to have a sense of whether interceptors would be sufficient.
OK, I'll push to the PR once it passes its local ITs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, interceptors are sufficient-- that's what we are using on the CLAW-supplied client we are offering you to inject. That having been said, I have no prob with casting, since that means less work for me. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a few false starts getting OSGi wiring happy, I just updated the PR to use the casting approach. Now it looks for org.apache.http.client.HttpClient
!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Building it now.
Any provided HttpClient now is (a) accessible by an HttpClientFetcher, and (b) is the basis of the default registry.
Switch out current or default HttpClient for highest-ranked HttpClient in OSGi service registry immediately when it becomes available
Also place an authenticated HttpClient in fcrepo-api-x-test and publish as an OSGi service
This doesn't quite work yet, so service indexing ITs are disabled
As noted in a comment on the CLAW issue, updates to the service indexing ITs are pending |
Just FYI, I was able to get the camel http4 component to use a provided |
Tracking the question of |
Allows the user to provide an
HttpClient
for interacting with Fedora, and provides for more flexible authn configuration of the default/providedHttpClient
used by API-X for this purpose. This allows API-X to adapt to arbitrarily complex authn protocols that may protect Fedora (such asJWT
)HttpClient
published to the service registry for communication with Fedora.HttpClients
in the OSGi service registry, and immediately starts usingHttpClients
as soon as they are available.HttpClient
used by API-X.Resolves #112
Resolves #114