Okay, this all makes sense to me. I wasn't going to make this kind of break without a check-in. I'll wait for your commit and then move on with an HttpOp and (new) HttpClientMaker approach. I think packing the client-building API into HttpOp alongside all the operations would be convenient for some folks, but a bit confusing for many others.
--- A. Soroka The University of Virginia Library > On May 17, 2016, at 6:51 AM, Andy Seaborne <[email protected]> wrote: > > What the authentication support provides is a way for the app to setup the > environment have HttpOp act on it. Often, the app is not itself dealing with > HttpOp, only indirectly via varous ARQ remote operations. > > Yes - this may break existing code but I suspect rarely if at all. > > The choice seems to be stick with the deprecated HttpClient classes or make > some kind of break. > > For the majority of auth uses though I am assuming it is "setup the auth in > the environment" and use ARQ. This should not need to change. > > > yes - at he moment, authentication is late in the game - too late for the new > httpClient API. > > I think we need to make Jena API change and break the idea we can modify an > HttpClient. > > This splits HttpOp into two separate parts: all the calls that take a > HttpClient, but not HttpAuthenticator (these are all the calls that are > performing the operation on the HttpClient unchanged, and some code to create > HttpClients with, optionally, authentication. Indeed, they could be two > classes, HttpOp and HttpClientMaker. > > HttpClientMaker has operations that takes a HttpClientBuilder and the > HttpAuthenticator. If there is one that is make(HttpAuthenticator) using the > default builder, the app needs to chnage by write two lines: make HttpClient, > call HttpOp, where it was one. > > HttpClient hc = HttpClientMaker.make(HttpAuthenticator) ; > HttpOp.....(hc, ...) > > > > The internal "exec" becomes simpler. > > private static void exec(String url, HttpUriRequest request, > String acceptHeader, > HttpResponseHandler handler, > HttpClient httpClient, > HttpContext httpContext) { > > a first step is to move the auth stuff first and: > > private static void exec(String url, HttpUriRequest request, > String acceptHeader, > HttpResponseHandler handler, > HttpClient httpClient, > HttpContext httpContext, > HttpAuthenticator authenticator) { > httpClient = ensureClient(httpClient, authenticator); > httpContext = ensureContext(httpContext); > applyAuthentication(asAbstractClient(httpClient), url, > httpContext, authenticator); > exec(url, request, acceptHeader, handler, > httpClient, httpContext); > } > > [As I happen to have done that change locally to check it, I'll commit it > anyway.] > > > Andy > > On 16/05/16 17:29, A. Soroka wrote: >> Okay, coming back with a quick design question. The current type for >> authentication (o.a.j.atlas.web.auth.HttpAuthenticator), works via >> the following signature: >> >> public void apply(AbstractHttpClient client, HttpContext httpContext, >> URI target); >> >> The idea here (IIUC) is to apply authentication to an _extant_ HTTP >> client, context and URI. The problem for option 2 is that this is >> pretty late in the game, obviously later than client construction. >> Is the idea to bake in the HttpAuthenticator as part of the >> (immutable) state of a subclass of o.a.h.c.HttpClient that >> automatically applies the behavior at request execution? That tangles >> Jena and o.a.http.client types in a way that seems to me to be a bit >> odd, but my sense of Jena's idiom is pretty undeveloped. Just want to >> make sure I am on the right road. It would mean altering >> FormsAuthenticator to factor cookies out of the client into the >> context, and that means an HttpClientContext, so we'd end up with >> something like: >> >> public void apply(HttpClient client, HttpClientContext httpContext, >> URI target); >> >> which would break HttpAuthenticator impls out there in the world, but >> not horrifically. It would also mean that the client in that >> signature would have immutable state going forward, and that has more >> potential, I think, to break impls, but anyone doing a custom >> authenticator should also be able to factor that state out of the >> client into the context like I describe above. >> >> --- A. Soroka The University of Virginia Library >> >>> On May 15, 2016, at 8:33 AM, Andy Seaborne <[email protected]> >>> wrote: >>> >>> On 11/05/16 16:38, A. Soroka wrote: >>>> I recently did some work on a project for those exact >>>> deprecation warnings (introducing HttpClientBuilder). I would be >>>> happy to take a crack at those if you want to make me a ticket. I >>>> don't remember it being too hairy. >>> >>> https://issues.apache.org/jira/browse/JENA-576 >>> >>> The uprade looks easy but there is one area >>> >>> In the old API http-client, authentication is done by modifying the >>> HttpClient object. In the new API, HttpClient (CloseableHttpClient) >>> is immutable and chnages need to be made at the HttpClientBuilder >>> stage. >>> >>> That impacts the HttpOp interface - some functions are of the form >>> HttpClient+HttpAuthenticator. And the public alls all go to one >>> operation that takes HttpClient+HttpAuthenticator. >>> >>> So I think (I have not tried) the forms: >>> >>> 1/ operation(HttpClient, ....) // No auth unless already in >>> HttpClient operation(HttpClientBuilder, HttpAuthenticator, ....) >>> >>> i.e. HttpClientBuilder if HttpAuthenticator >>> >>> 2/ Or move the creation/setup of authentication out of the >>> operations call flow and provide creator operations. >>> >>> Remove all operation(HttpClient, HttpAuthenticator, ....) >>> >>> and have >>> >>> HttpClient hc >>> >>> hc = createHttpClient(HttpAuthenticator, ....) // default >>> builder. hc = createHttpClient(HttpClientBuilder, >>> HttpAuthenticator, ....) >>> >>> i.e. separate creation and auth setup. >>> >>> This, to me, looks more in tune with the new API >>> >>> 3/ Stick to the deprecated AbstractHttpClient style. >>> >>> >>> ATM I favour (2). We don't preserve exact current HttpOp API in >>> (1) or (2) so let's do the better design. 3.2.0 if necessary (but >>> I don't believe that rigid semantic versions makes sense in large >>> system with semi-independent sub-systems; version number changes >>> when many users aren't affected can be more confusion than help; >>> "semantic version" is a guidance not a rule). >>> >>> Andy >> >
