[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 I'm fine if you would merge this and f you'd like to close 576 for the release. I will work on the docs separately. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 That `http_op_3` commit https://github.com/apache/jena/commit/944e5208ff0fbb4e87fcab7417b760252d9aabfc did come over into this PR as commit https://github.com/apache/jena/pull/151/commits/722b3f371abf5d31ce1f1564da134a7a76f03ca0. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 The state is that it builds and passes all tests. The dependency is upgraded and the use of the deprecated API from HTTP Commons is removed (at least all that I could find). I now see that I didn't push the last commit I cherry-picked-- sorry! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue: https://github.com/apache/jena/pull/151 I don't know what state the PR is in. I don't see the changes I made today to sort things out on the PR so I am unclear what "it" is and what your happy comment refers to. I can merge it,or you can, as-is together with http_op_3 commit 944e520 and then resolve JENA-576 so it is recorded for this release. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 For my money, we can merge this. I think we might want to keep JENA-576 open until @rvesse is happy with the new documentation. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue: https://github.com/apache/jena/pull/151 @ajs6f What's the next step? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 _O frabjous day! Callooh! Callay!_ Full green light build. Thanks enormously for this help, @afs. I'm still very much a beginner with the various form of test machinery in the codebase. After we merge this, I'll file a ticket for polishing the new `ServerCtl` and "factoring it across" to `fuseki2-embedded`. So I still owe @rvesse good examples of complex authN setups, but that is a task for documentation. I don't think we have anything left on this ticket/PR-- I think it can stop blocking release! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue: https://github.com/apache/jena/pull/151 http_op_3 branch should be good now : ServerCtl moved to fuseki1 and used uniformly. After this is finished, it coudl be cleaned up or more reasonably, extract that code, add the capabilities to jena-fuseki2-embedded and change jena-jdbc-driver-remote accordingly (just imports by that stage?). Several commits. If ti works to merge from that branch, they'll be more "git checking" everything is pulled in. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue: https://github.com/apache/jena/pull/151 It looks like only a partial set of changes from http_op_3. Maybe I got the remote repo into a inconsistent state. I pushed some (I hoped) unrelated cleanup today. I will now also backport ServerCtl to Fuseki1 - the logging messages from RetryExec were suppressed and hiding a problem. This happens when the pooling client is not closed when the server is freed and reallocated. Branch http_op_3 has many small changes across Fuseki1 and jena-jdbc-driver-remote testing as I made connect handling and server restart consistent, and also remove some debugging code left in the PR that clears the httpClient (sets it to null) but didn't close the old one. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
The errors I get in `TS_Fuseki` are always in `TestSPARQLProtocol` or
`TestQuery`, which use `resetServer()` and load up sample data over which to
work. They are `Connection reset` exceptions, oddly. It's as though something
is happening between `resetServer()` and the moment when the `DatasetAccessor`
is used to push data. I've tried using a non-pooling client, on a guess, but
that changed nothing. The two tests succeed on their own, but run in a suite,
they fail (at least without `static { allocServer(); }`).
```
org.apache.jena.atlas.web.HttpException: java.net.SocketException:
Connection reset
at org.apache.jena.riot.web.HttpOp.exec(HttpOp.java:1098)
at org.apache.jena.riot.web.HttpOp.execHttpPut(HttpOp.java:963)
at
org.apache.jena.web.DatasetGraphAccessorHTTP.doPut(DatasetGraphAccessorHTTP.java:158)
at
org.apache.jena.web.DatasetGraphAccessorHTTP.httpPut(DatasetGraphAccessorHTTP.java:148)
at org.apache.jena.web.DatasetAdapter.putModel(DatasetAdapter.java:63)
at
org.apache.jena.fuseki.TestSPARQLProtocol.beforeClass(TestSPARQLProtocol.java:43)
...
Caused by: java.net.SocketException: Connection reset
at java.net.SocketInputStream.read(SocketInputStream.java:209)
at java.net.SocketInputStream.read(SocketInputStream.java:141)
...
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
There is a strange pattern for me using the code as pushed here: without
`static { allocServer() ; }` various tests fail in `fuseki1`'s `TS_Fuseki`, but
only ever those that use `resetServer()`. With it back in, all tests in
`TS_Fuseki` succeed. I am investigating further.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
That's a weird error, now that I look at it. It suggests that a server
hasn't spun up, but `TestQuery` has
```
// DRY - test protocol?
@BeforeClass public static void beforeClass()
{
ServerTest.allocServer() ;
ServerTest.resetServer() ;
DatasetAccessor du = DatasetAccessorFactory.createHTTP(serviceREST)
;
du.putModel(model1) ;
du.putModel(gn1, model2) ;
}
```
but all the code underneath ` ServerTest.allocServer() ;` looks synchronous.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
RIght, but when I remove `static { allocServer() ; }` the tests for
`jena-fuseki1` no longer pass with
```
org.apache.jena.fuseki.TestQuery Time elapsed: 0.125 sec <<< ERROR!
org.apache.jena.atlas.web.HttpException:
org.apache.http.NoHttpResponseException: localhost:51667 failed to respond
at org.apache.jena.fuseki.TestQuery.beforeClass(TestQuery.java:53)
Caused by: org.apache.http.NoHttpResponseException: localhost:51667 failed
to respond
at org.apache.jena.fuseki.TestQuery.beforeClass(TestQuery.java:53)
```
I'm now using a `ServerTest` copied straight from `http_op_3`-- let me push
what I have.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
(going off pull/151.diff, which isn't always perfect):
There is still a global server created in fuseki1/ServerTest by:
```
static { allocServer() ; }
```
I don't know why that has been enabled. I have now completely removed it
from the code - see a latest state of http_op_3.
I can't see what code state you're working from. Could you push what you
have?
There were more tidying up left over than I expected - I may have not
committed the right set though a visual check didn't suggest any problems.
What you see is a symptom of the old and new server being around at the
same time. I believe that the pooling client can cause old server to hang
around because, as it is localhost, the pool has open TCP connections. That's
why `ServerCtl.setPoolingHttpClient` closes down the pool with explicit
`.close`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue: https://github.com/apache/jena/pull/151 Do you have any changes other than those I picked up with the merge? If not, the simple way is to work from http_op_3 which I can do - it includes all your commits on this PR (unlike http_op_2 where I didn't do the merge cleanly so I got the changes but lost the commit structure). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 I will bang on this more this weekend and try going back down the road you outlined, @afs . Maybe I missed something in the way I brought your commit over. If I don't find something, I'll go back through the test server management machinery. There must be something there. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 @afs I'm still getting the errors: ``` Failed tests: TestRemoteEndpointConnectionWithResultSetTypes>AbstractJenaConnectionTests.connection_statement_query_select_max_rows_01:531 expected:<10> but was:<0> TestRemoteEndpointConnectionWithResultSetTypes>AbstractJenaConnectionTests.connection_statement_query_select_max_rows_02:578 expected:<1> but was:<0> TestRemoteEndpointConnectionWithResultSetTypes>AbstractJenaConnectionTests.connection_statement_query_select_max_rows_03:625 expected:<10> but was:<0> ``` running `mvn clean install` your branch `http_op_3` directly or now that I have cherry-picked https://github.com/apache/jena/commit/36b678933ecf2b5553f9345ef84e683cf0f181f0 onto this PR. Did I miss something in the plan you laid out? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 Hang on, I didn't realize that that was just a simple cherry-pick, doing it now. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 @afs Are you saying to take https://github.com/apache/jena/commit/36b678933ecf2b5553f9345ef84e683cf0f181f0 and add it to this PR, and then we can look at finally merging this into master? Yes, I can totally do that, although it's COB today here and I am traveling all day tomorrow, so I will not be able to get to it until Saturday. But I will surely do it then. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue: https://github.com/apache/jena/pull/151 > Let's port the changes here, where the discussion has been. Branch [http_op_3](https://github.com/afs/jena/tree/http_op_3) is made by: 1. Current Jena master. 1. This PR 1. Resolve merge conflict 1. Commit merge : 8c2935e 1. Overlay the java files from branch http_op_2 1. Commit changes: 36b6789 So that is the correctly consolidated set of changes. @ajs6f - would it work for you to commit 36b6789 and you make it this PR? Or what else? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 I've opened https://issues.apache.org/jira/browse/JENA-1243 to track the other clear-up. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 1. Let's port the changes here, where the discussion has been. 2. Absolutely fine with me (to do the other changes elsewhere). I'll make a ticket for those things now so that we don't lose the thread. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue: https://github.com/apache/jena/pull/151 Can we do the clear-up separately, please? I really want to get he HTTP changes into the code base as this is in practical terms a blocker on the release preparation so I'm keen to get what we have into the codebase so it can settle even if the settling is short. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user rvesse commented on the issue: https://github.com/apache/jena/pull/151 The incorrect/ overridden plug-ins can certainly be removed. They are probably just cruft from the early days of development. Historically AspectJ was used in order to inject trace level logging into every single method in the JDBC code base. The logic behind this was that the intent was for the driver to be used with generic JDBC tools where attaching a debugger would have been difficult/impossible and so you would've had to rely on logging to do debugging. This probably doesn't apply any more so you may as well remove it. There may be associated entries in the license/notice that should also be removed. But this was three years ago so my memory maybe incorrect. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 Holy moly, @afs , thanks for the huge assist! I was out yesterday (Yom Kippur) and am now catching up. I will leave comments specific to the HTTP question on the new PR, but I will try to speak to the other points here. 1. I am certainly happy to see a spurious plugin go. I have no problem with including that change in this package of changes, if that's okay with everyone else. Or it could be a quick separate ticket/commit, to keep thing straighter in the history. 2. I have no idea why `maven-source-plugin ` is overridden. Baggage from long-ago when this material was imported into the main code base? 3. I haven't seen AspectJ used anywhere, but I haven't been all through the `jdbc` code. Same as the first point-- I have no problem removing it here or elsewhere. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue: https://github.com/apache/jena/pull/151 New branch : [http_op_2](https://github.com/afs/jena/tree/http_op_2) Green line! This is PR#151, the clean-ups in Jena master and some server control for jena-jdbc-driver-remote. In particular, the pooling `HttpClient` is reset when the server is stop/started. Notes: 1. The `UnsupportedOperationException` has gone away and I don't know why. 1. (not new) the "WithAuth" tests are marked "Ignore" due to exhausting ports. Maybe we should have a few tests for auth at some time. Other: 1. jena-jdbc uses the plugin `com.mycila.maven-license-plugin` ... except this gets it wrong! The comment must start `/**` not just `/*` which we use elsewhere. It also runs late, whereas the RAT runs early. I suggest we remove this plugin as it duplicates functionality. 2. jena-jdbc overrides the maven-source-plugin version for no reason I can see. 3. Is aspect4j really needed anymore? It is quite out of date. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
I've cleaned up allocating/deallocing the Fuseki server in case it hides
some timing issues.
Commit AFS-Debug-3 which I have also tried with a multisecond wait after
the server is stopped and started as the first test suite ends and a second one
starts.
Also, the reset is done by clearing the datasets programmatically.
Hence, the problem is not `UpdateExecutionFactory.createRemote` itself.
I now get (+ messages about server start/stop) 2 low level messages from
the commons HTTP code but a green line.
Could some connection state be carried across the server restart? Or is
this reason across server restart?
```
*** connection_statement_query_select_max_rows_01
*** connection_prepared_statement_select_01
*** connection_statement_query_select_max_rows_01
15:08:29 INFO RetryExec :: I/O exception
(java.net.SocketException) caught when processing request to
{}->http://localhost:42668: Broken pipe
15:08:29 INFO RetryExec :: Retrying request to
{}->http://localhost:42668
*** connection_prepared_statement_select_01
```
Those "RetryExec" appeared. If, however, there is one server across the run
of both test classes, these do not occur.
Not using a pooling client also fixes them. AFS_TS_Debug : set the used
HttpClient to null.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
This
```
HttpClient hc = HttpOp.createPoolingHttpClient() ;
HttpOp.setDefaultHttpClient(hc);
String queryString = "SELECT * { ?s ?p ?o }" ;
String service = "http://sparql.org/books/sparql; ;
Query query = QueryFactory.create(queryString) ;
QueryExecution qExec = QueryExecutionFactory.sparqlService(service,
query) ;
QueryExecUtils.executeQuery(qExec);
```
causes this:
```
Exception in thread "main" java.lang.UnsupportedOperationException
at
org.apache.http.impl.client.InternalHttpClient.getParams(InternalHttpClient.java:210)
at
org.apache.jena.sparql.engine.http.HttpQuery.execGet(HttpQuery.java:325)
at org.apache.jena.sparql.engine.http.HttpQuery.exec(HttpQuery.java:292)
at
org.apache.jena.sparql.engine.http.QueryEngineHTTP.execResultSetInner(QueryEngineHTTP.java:362)
at
org.apache.jena.sparql.engine.http.QueryEngineHTTP.execSelect(QueryEngineHTTP.java:354)
at
org.apache.jena.sparql.util.QueryExecUtils.doSelectQuery(QueryExecUtils.java:196)
at
org.apache.jena.sparql.util.QueryExecUtils.executeQuery(QueryExecUtils.java:78)
at
org.apache.jena.sparql.util.QueryExecUtils.executeQuery(QueryExecUtils.java:66)
at
org.apache.jena.sparql.util.QueryExecUtils.executeQuery(QueryExecUtils.java:62)
at dev.DevMain.main(DevMain.java:65)
```
because `HttpQuery` does this:
```
private void selectClient() {
// May use configured default client where appropriate
this.client = HttpOp.getDefaultHttpClient();
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 Hm. This is really quite squirrely. If there is something subtle wrong with `ServerTest::resetServer`, I wonder what I did in my PR to make it show up? Anyway, I will start tracing it. Thanks for the continued diagnostic help! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 That stuff in `AbstractRemoteEndpointResultSetTests` is cruft-- client swapping is taken care of in `TS_JdbcDriverRemote` itself. But it doesn't seem to have any effect on the mysterious failures. But I will be taking it out in a further commit-- thanks for catching it! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
I have just pushed a branch to https://github.com/afs/jena/tree/http_op
where the tests+setup are extracted into a single class
`AFS_TestRemoteEndpointConnection` and if this is called twice (see
`AFS_TS_Debug`) I get one failure on the second run.
The other test is now no more than add a quad and then:
```
JenaConnection conn = this.getConnection(ds);
conn.close();
```
In fact, adding and deleting the test quad still makes the test fail.
My best guess at the moment is that `ServerTest.resetServer();` is broken
(look for `//AFS Suspect`).
It is the only use of `UpdateExecutionFactory.createRemote`.
If a no-op SPARQL Update is done:
```
UpdateRequest request = UpdateFactory.create("INSERT DATA {}") ;
UpdateProcessor proc = UpdateExecutionFactory.createRemote(request,
ServerTest.serviceUpdate) ;
```
there is still a test failure. This is the [AFS-debug-2
commit](https://github.com/afs/jena/commit/7f4030ffd0db394ac1e5ad1bbca2c84a2edce4c7).
Dropping the server and recreating - everything is fine.
Resetting the server in the overall test suite, not between classes -
everything is fine.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue: https://github.com/apache/jena/pull/151 I found mention of `HttpOp.createCachingHttpClient` in `AbstractRemoteEndpointResultSetTests` - is that correct? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 It's as though some kind of static state is being impacted. But where?!?! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
Yes, I can get it to fail with
```
@RunWith(Suite.class)
@Suite.SuiteClasses({ TestRemoteEndpointConnection.class,
TestRemoteEndpointConnection.class })
public class TestSpecial {}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 I can't get the deadlocking to occur, but by running tests in the order you give, I can see the three failures in `TestRemoteEndpointConnectionWithResultSetTypes`. I don't see anything special about those tests, except that they use `Statement::setMaxRows` so I'm investigating that path now. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 Wow, this is getting quite weird. I don't get any of that stuff. I will get on this and find out why. Deadlock is really unexpected! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue: https://github.com/apache/jena/pull/151 I pulled this PR into a clean copy of Jena (I did `git pull https://github.com/apache/jena/ pull/151/head` in to a branch I get test failures in jena-jdbc-driver-remote: with maven: ``` Failed tests: TestRemoteEndpointConnection>AbstractJenaConnectionTests.connection_statement_query_select_max_rows_01:531 expected:<10> but was:<0> TestRemoteEndpointConnection>AbstractJenaConnectionTests.connection_statement_query_select_max_rows_02:578 expected:<1> but was:<0> It looks like state across tests is TestRemoteEndpointConnection>AbstractJenaConnectionTests.connection_statement_query_select_max_rows_03:625 expected:<10> but was:<0> TestRemoteEndpointConnectionWithResultSetTypes>AbstractJenaConnectionTests.connection_statement_query_select_max_rows_01:531 expected:<10> but was:<0> TestRemoteEndpointConnectionWithResultSetTypes>AbstractJenaConnectionTests.connection_statement_query_select_max_rows_02:578 expected:<1> but was:<0> TestRemoteEndpointConnectionWithResultSetTypes>AbstractJenaConnectionTests.connection_statement_query_select_max_rows_03:625 expected:<10> but was:<0> ``` In Eclipse: If I run `TestRemoteEndpointConnection` on its own it runs. If I run the tests in the `connections` package, `TestRemoteEndpointConnectionWithResultSetTypes` has 3 failures but not `TestRemoteEndpointConnection`. Running `TestRemoteEndpointConnectionWithResultSetTypes` on it own it works. Also in Eclipse (right-click on the project, runAs -> JUnit test), , running all the tests in jena-jdbc-driver-remote locks up. (Running in Eclipse with maven gives the 6 failures.) What is different this time in the lock-up is that the connections are in state ESTABLISHED, not TIME_WAIT. This shows that there is deadlock, not resource problems. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 Just as an example, I could imagine supporting fluent methods that would let people use Jena's language selection machinery (i.e. `Lang`) instead of using HTTP `Accept` directly. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 Like I said, I'm fine with making a new fluent API. Can you describe some of the things you see as common in particular to a semweb app? `HttpOp` as it stands doesn't appear to do anything particular to the semweb. Or are you including in your thought other classes like `HttpQuery` and `QueryEngineHTTP`? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue: https://github.com/apache/jena/pull/151 To some extent, yes a fluent API is duplication - as I said I only made a quick sketch - but the principle of `HttpOp` is to make the use of general HTTP mechanism provided in forms that wrap up common cases that a semantic web app needs. That is, a bit more focused than expose the application, and jena's own internal uses, to all the details of Apache HttpComponents. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 And as a point of info, I didn't actually end up changing the `HttpOp` API all that much, just `HttpAuthenticator` => `HttpClient`. The additional parameters for `HttpContext` turned out to already be there. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 I'm totally fine with a "high-level" rethink, but at some point, aren't we just duplicating this: https://hc.apache.org/httpcomponents-client-ga/tutorial/html/fluent.html ? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 As for the fix, specifically I went to `HttpOp` and made sure that any HTTP clients built there get closed properly. HTTP clients that get passed in don't get closed because that's the job of the client (i.e. user of Jena, not HTTP client) that passed them in. Also I checked through the tests in `jena-jdbc` and made sure they don't multiply clients pointlessly and use some pooling for faster test execution. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
Should we take a step back and reconsider whether updating the current API
in HttpOp is the right thing to do.
Maybe we ought to
* introduce a different style more fluid
* reconsider a design where the caller is responsible for setting up more
of the `HttpClient` and `HttpOp` provides operations for nothing special (no
auth) + ops that use a redefined `HttpClient`. This is to reduce method bloat.
On (1): something like (quick sketch)
HttpOp.get("http://example/;)
.setAccept("application/rdf-xml") // Override default
.setHttpClientBuilder() // Usually no needed, for special
setup only
.exec() ;
where there are implicit builder objects from, `.get(url)`, `.post(url)`,
`.put(url)`, `'delete(url)`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 Sorry, @afs , I'm working on this now but not keeping this conversation up to date. In fact, my most recent comment I now believe to have been wrong. See [here](https://hc.apache.org/httpcomponents-client-ga/tutorial/html/fundamentals.html#d5e213) and also that `HttpClientBuilder` is specifically marked as not thread-safe. So I stayed with the original move (thread `HttpClient` through the call stack) and got much more careful about closing resources when done with them. That solved the problem you found. I _think_ that this PR is ready to go. I say I _think_ because I've been looking at it a lot yesterday and today, and I'd like to leave it alone for a day and look at it with fresh eyes. But comments/criticism welcome immediately! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue: https://github.com/apache/jena/pull/151 For information, what's the fix? I've unmerged this all from my local copy and will start again when we agree it's ready. If it's easier, maybe drop this and open a new PR when ready? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
Erm, good/bad news.
1. I can fix the problem @afs found.
2. I don't want to, because I realize now that I did this whole PR wrong.
If you read up this page, you'll see where both @afs and I remark that it's
odd that `HttpClient`s can't be built from extant `HttpClient`s. It took me
until now to understand why. You can call `build()` on an `HttpClientBuilder`
as many times as you want for as many clients as you want with the exact same
config. I should never have rethreaded the call hierarchies with `HttpClient`.
It should have been `HttpClientBuilder` all the time, all the way down to
`HttpOp` which, within `exec()`, should build and close a client, leaving the
builder for infinite reuse.
{sigh}
So in order to do this right, I need to redo a good piece of it. That will
get rid of a lot of subtle things I had to add in the test code to make this
work in its current form (should have realized something was wrong). This is
now my top priority because this saga has gone on too long already (yay
all-volunteer projects :) ). I will write a further comment when I have an ETA,
hopefully not long.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 Got a fix, but I still need to make sure that I didn't break anything else and that I'm not wasting resources and I'm hitting EOB for the day. I will send a commit tomorrow morning (EST). Thanks for the help, @afs ! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 I've been able to reproduce. Investigating... --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue: https://github.com/apache/jena/pull/151 It could be emergent or trigger on my OS. JDBC remote driver uses QueryEngineHTTP with a retained client and that hits deprecated code. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 Weird. Maybe something changed out from under me. This is a pretty old PR. I'll get on it (thanks for the diagnostic info). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue: https://github.com/apache/jena/pull/151 Problem encountered: the JDBC remote driver tests lock up. I have tracked it down to `TestRemoteEndpointResultsWithGraphUris` run on its own (it locks up in Eclipse as well) at 113 of the 121 tests. `netstat` reports many connections in `TIME_WAIT` suggesting that we have the problem with port use and/or failing to close connections. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 Ooh, just noticed that. I will take a look at it today and fix it. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue: https://github.com/apache/jena/pull/151 If you prefer, I'll merge it. (What about the merge conflict?) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 It would be great if you could use this stuff in RDFConnector to trial it. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 Yes, as I understand it, we can merge. I haven't merged it because I try never to merge my own stuff unless it's pretty trivial. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 @rvesse That is _exactly_ what has slowed this down! :) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user rvesse commented on the issue: https://github.com/apache/jena/pull/151 @ajs6f provided that those examples are available at some point then this is fine with me. I'm not sure how we could incorporate test cases to this aspect because you would need an endpoint protected by forms authentication for the purposes of the tests. In theory you could configure shiro with Fuseki to do that but as Fuseki lacks a login page that wouldn't work in practice. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 Code-wise, not that I know of. But I promised @rvesse examples of doing form-based authN for query endpoints. If we're okay with those appearing in docs (instead of the test suite) then I think we could go ahead with me providing those docs pre-release. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue: https://github.com/apache/jena/pull/151 Any reason not to apply this? It is in danger of becoming a bit of blocker on release 3.1.1. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 Okay. sounds like I owe the following work for this: - Enriching `HttpOp` to include methods that accept `HttpContext` - A nice suite of examples that @rvesse approves as a path forward --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user rvesse commented on the issue: https://github.com/apache/jena/pull/151 In a sense I don't have a strong opinion on what the final API should look like or if a higher level API like `HttpAuthenticator` needs to continue to exist. Providing there was sufficient examples to demonstrate how to perform different kinds of authentication - at a minimum HTTP Basic, Forms Auth i.e. Cookies and separate credentials for different endpoints - with migration documentation showing users how to migrate forwards then I would be happy --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 But since that construct goes via `HttpClientCOntext`, doesn't that take us back to methods with `HttpContext` as a parameter? Or am I misunderstanding the suggestion? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue: https://github.com/apache/jena/pull/151 I came across "preemptive authentication" for HttpClient which apparently does not a preconfigured `HttpClient`. [HttpComponents 4.5.x tutorial section, 4.6](https://hc.apache.org/httpcomponents-client-4.5.x/tutorial/html/authentication.html#d5e717) It works pre-populating the authentication data cache and calling: ``` CloseableHttpResponse response = httpclient.execute(target, http-request, HttpClientContext); ``` It's not quite the style of `org.apache.jena.atlas.web.auth.HttpAuthenticator` but it's close. Would this be better than `HttpClientBuilder`+`HttpAuthenticator`? Mu first impression is that it is much the same - just different. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 @rvesse -- Does @afs 's plan to offer methods that accept `HttpClientBuilder` meet your concerns about supporting complex authN scenarios? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 Okay, so we should be able to treat 'HttpClientBuilder' as a kind of config object. Set one up and pass it in the way you would pass in config (including authN). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 One question with passing in a builder is the extent to which clients will be reusable. Perhaps that's where setting a default client or using the custom-client-per-service feature (available now) would be helpful? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 @afs Said "(It seems odd that the can't get the setting out of an HttpClient to feed into a builder.)" YES. It is _intensely_ annoying. It seems like a massive oversight to me, but as far as I can tell, it is the case. What's worse, several other classes that are of not dissimilar character _do_ feature convenient copy/builder ops. (E.g. `oah.client.config.RequestConfig.copy(RequestConfig)`). I will be checking out whether they're correcting this for the 5-series. (Under dev now.) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue: https://github.com/apache/jena/pull/151 @rvesse so the proposal is operations that take a `HttpClient` and operations that take an `HttpClientBuilder` and `HttpAuthenticator`? (It seems odd that the can't get the setting out of an `HttpClient` to feed into a builder.) `HttpAuthenticator.apply` takes a `HttpClientBuilder`. That would work for me. We have to change the existing design because `HttpClient` is now immutable. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 @afs Yes, the addition of `HttpOp` methods as you describe is what I was thinking of. Not too much there, but enough to let people move their own `HttpContext`s around, which would be pretty darn handy for exactly the kind of authN scenarios that concern @rvesse . --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 @paulhoule One difficulty with `HttpRequestInterceptor` is a good thing to bring up-- in the newer HTTP Commons client APIs, clients are immutable and if you want to add, e.g., interceptors, you need to do that when you build your client. OTOH, you can "late add" authN of some kinds using the more specific machinery that HTTP Commons provides via the methods in `HttpClientContext`. The interceptor pattern is more general, but as noted, you have to set it up early in the process. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 @paulhoule There is `org.apache.http.HttpRequestInterceptor`, which I think is what HTTP Commons itself supplies for that kind of purpose. But there are also a host of parts built into the rest of the HTTP Commons client API for authN, some of which you probably already know, like `oah.client.CredentialsProvider` or `oah.auth.AuthScheme`. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 @rvesse : Basically, I think we share the same picture of the situation. I wouldn't claim that this new API presents quite the same "ergonomics" as what we now have. I think the heart of the matter is well-expressed by your characterization: "it replaces a nicely encapsulated API with requiring users to directly interact with a lower level API". The question is: is that okay? Because the concern here is HTTP/networking, and I think it's at least arguable that HTTP/networking really isn't a concern of Jena. It's something that Jena itself relies on a library to achieve, and I'm suggesting that we don't really gain all that much for our users by repacking that functionality, unless it is cheap to do and there is some kind of value we can add while so doing. (And obviously, I don't think that's so much the case here.) You're quite right that complex authN schemes (and forms-based stuff is a good example) get a little more onerous for the user with this new API. I would suggest, though, that we can mitigate that pretty well with good examples and docs. Showing people how to get their cookie and then hand around the `HttpContext` that holds it shouldn't be too hard. I can put myself on the hook to write some examples in the tests (or some other appropriate place), if it would help with your concerns? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user paulhoule commented on the issue: https://github.com/apache/jena/pull/151 @rvesse can authentication be handled by something like a Decorator? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user rvesse commented on the issue: https://github.com/apache/jena/pull/151 It also potentially makes it much harder to define complex authentication schemes. Four example the current API allows you to define an authenticator that presents different credentials to different URLs e.g. I might need different credentials for accessing the `/query` and `/update` endpoints of a Fuseki instance. In the new API it seems I would have to manually manage this and make sure I present the right client or context to each API call or encapsulating object --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user rvesse commented on the issue: https://github.com/apache/jena/pull/151 My major concern with this pull request is that it replaces a nicely encapsulated API with requiring users to directly interact with a lower level API. It seems like there should be room for compromise i.e. Keeping `HttpAuthenticator` but perhaps having it simply operate over a `HttpClientBuilder` instead I am also unclear how more complex authentication such as forms based authentication which requires some sort of out of band login and presenting some token with each request e.g. a cookie. Part of the reason the original API was introduced by myself was that I needed to interact with systems that relied upon forms based authentication. As I understand that the proposed new API I would have to manage that login myself and pass the resulting `HttpContext` everywhere that needs it? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue: https://github.com/apache/jena/pull/151 So to check my understanding: For each operation that takes a `HttpClient` there is one that takes a `HttpClient` and an `HttpContext`. Not so bad - we don't have the `HttpAutheticator` ones any more. Of course, if java had default parameters values like scala ... --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 Right, I think we're fine as long as Jena itself is actually the client (e.g. as part of `SERVICE` action in a query). It was the situation in which some app is using HTTP via `HttpOp` in which an input stream could "leak". But I think that's really just not Jena's problem, right? If you walk away with a resource from the network, you need to deal with it properly. Jena can't run after you pulling on your shirt to remind you. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue: https://github.com/apache/jena/pull/151 The need for closing responses isn't new - all typed input streams should be closed in the codebase currently because I had to go hunting them all down with the old `HttpOp` code. So, hopefully, theer are no issues here. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 Reading through the current HTTP Commons docs leaves me with the impression that we do not need to close clients after use in `HttpOp`, but that we do need to see to it that _responses_ are closed. However, tracing through to `oaj.riot.web.HttpResponseLib.nullResponse` in conjunction with advice [here](https://hc.apache.org/httpcomponents-client-ga/tutorial/html/fundamentals.html#d5e145) assures me that we are doing what we need to do. The only case that I find uncertain is in methods like `TypedInputStream execHttpGet(String url)` wherein we really cannot be sure that the request is properly closed. I'm not sure what to do about this short of trying to take `HttpOp` completely internal, which seems like a massive overreaction. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 [A useful description of `HttpContext`.](https://hc.apache.org/httpcomponents-client-ga/tutorial/html/fundamentals.html#d5e223) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 I'm imagining a bag of new methods in `HttpOp` similar to what's there now, with the additional `HttpContext` param, and the methods that are now there would call through those new methods with `null` in that slot, much like what happens now with many of the other params. Then I would have to extend the availability of `HttpContext` out into `HttpQuery` and `QueryEngineHTTP`, etc. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue: https://github.com/apache/jena/pull/151 What would a sidecar parameter look like in this situation? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 As I understand it (for what that's worth) `HttpContext` is mostly about the idea of a "conversation", so if you've got some kind of session going on, or a series of request-response cycles that compose some kind of larger stateful activity, `HttpContext` is where you stash that state/context. (E.g. a cookie store.) So I guess since we want to provide the kinds of facilities needed for SPARQL protocols, and authN is definitely part of that story, maybe we would want to provide `HttpContext` as a sidecar param? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue: https://github.com/apache/jena/pull/151 Another thought: Does `HttpContext` with every `HttpClient` usage make an sense now? We now rely on `HttpClient` a lot more and `HttpContext` seems mainly for cookies. Maybe, versions of calls with `HttpClient` but without `HttpContext` might make sense or might not. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user ajs6f commented on the issue: https://github.com/apache/jena/pull/151 I'm not totally clear about the style for `CloseableHttpClient`, either. I'll poke around and see if I can get a better reading on that. And I'll start jotting up some docs. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jena issue #151: JENA-576: Moving away from deprecated HttpCommons Client AP...
Github user afs commented on the issue: https://github.com/apache/jena/pull/151 I haven't got my head around the new API yet - this is just a preliminary comment. It is a bit of a change around authentication but I think it is the right thing to do. It seems to me that the only other choice is not to migrate to the new API which over time will become a nuisance. Documenting the migration (for the release notes) will be helpful - maybe an example for user/password in the HttpOp class javadoc. If someone can grok that, they can see how to migrate their own code. One thought - it looks like the new style is to treat `CloseableHttpClient` as a one time use object. That means a try-resource in `HttpOp.exec`. I'm not sure how widespread this is. `getDefaultHttpClient` `setDefaultHttpClient` should be changed to be a `HttpClientBuilder`? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
