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

Apache httpclient 5 #4950

Merged
merged 3 commits into from
Apr 13, 2022
Merged

Conversation

zUniQueX
Copy link
Contributor

@zUniQueX zUniQueX commented Dec 29, 2021

Migrate the Apache HTTP Client to version 5.x. The migration guide provides several different migration steps, but this PR only migrates to the classic APIs, since the async APIs have less support for InputStream / OutputStream based content processing.

Notable changes:

  • HttpRequestRetryHandler was replaced by the new HttpRequestRetryStrategy. Therefore the client property Apache5ClientProperties#RETRY_HANDLER is renamed to Apache5ClientProperties#RETRY_STRATEGY
  • The CredentialsProvider now only allows to retrieve credentials. For setting credentials, the superinterface CredentialsStore has to be used

@jansupol
Copy link
Contributor

jansupol commented Jan 4, 2022

Thanks for the new PR.
Jersey probably needs to keep the existing Apache HTTP 4.5 client module for the Jersey users who do not wish to move over to version 5, yet.
The proper way to process is to create an extra new module apache5-connector, IMO.

@zUniQueX
Copy link
Contributor Author

zUniQueX commented Jan 4, 2022

@jansupol Then I'll modify this PR to use a seperate module for the new apache-connector and remove the other updates to HttpClient 5.x to prevent any dependency conflicts.

If I may ask this: I'm relatively new to OSS contributions and don't know exactly how to get the licenses and copyrights correct.
I would just copy the modified apache-connector to an other module. May I just do this or do I have to update authors/copyright notices?

@senivam
Copy link
Contributor

senivam commented Jan 5, 2022

@zUniQueX regarding copyrights - for new files (creating new module and copying files from another module into that module basically means that all files in that module are new) it's required to keep license as

/*
 * Copyright (c) 2022 Oracle and/or its affiliates. All rights reserved.
 *
 * This program and the accompanying materials are made available under the
 * terms of the Eclipse Public License v. 2.0, which is available at
 * http://www.eclipse.org/legal/epl-2.0.
 *
 * This Source Code may also be made available under the following Secondary
 * Licenses when the conditions for such availability set forth in the
 * Eclipse Public License v. 2.0 are satisfied: GNU General Public License,
 * version 2 with the GNU Classpath Exception, which is available at
 * https://www.gnu.org/software/classpath/license.html.
 *
 * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
 */

with only one year (2022) as a creation year in the first sentence.

The 2nd year in the copyright sentence appears when a file is modified during another year than the creation year.

And when PR is submitted there is a copyright check during the related build which would fail the build if there is no license/copyright or any of indicated years (creation/modification) is wrong. So, if the build passes OK this means everything (including copyright) is correct.

That's basically all regarding CP/license in created/modified files. And thank you for the contribution, it's appreciated.

@zUniQueX
Copy link
Contributor Author

zUniQueX commented Jan 5, 2022

@senivam Thanks for the explanation.

Copy the existing Apache connector to a new module and migrate it to HttpClient 5.x Classic APIs

Signed-off-by: Steffen Nießing <[email protected]>
@zUniQueX
Copy link
Contributor Author

zUniQueX commented Jan 7, 2022

@jansupol I've modified this PR to implement the changes according to your suggestion. Therefore I moved the files to a separate module, modified the copyright years while keeping the JavaDoc authors, modified the client properties to use a separate namespace and removed some unused code, which was left over for earlier HttpClient 4.x support.

@jansupol jansupol linked an issue Jan 21, 2022 that may be closed by this pull request
@zUniQueX
Copy link
Contributor Author

I just looked for an easy way maintaining both connector implementations and tests together, but haven't found a nice solution.

@jansupol Should we deprecate the Apache 4.x connector to remove it in 3.1.0? Or should we maintain both versions concurrently?

@jansupol
Copy link
Contributor

@zUniQueX First of all I want to say that this PR looks good. I am reviewing it in more detail, but so far it looks good.
Currently, we are trying to figure out #4960 and it may have an impact on this PR.

We should not remove the 4.x connector in 3.1, maybe we can do it in 4.0 (Jakarta EE 11). We would need to maintain both.
While both modules look similar at this point, we expect the 5.x connector to expand (HTTP/2 support), so the codebase of both modules can become quite different. You are right that the maintenance of both might not be easy.

@zUniQueX
Copy link
Contributor Author

Okay, this sounds reasonable to me. So far, I just performed the migration steps and didn't look further into HTTP/2 support. But I understand, that this should be supported in a later version.

@zUniQueX
Copy link
Contributor Author

zUniQueX commented Feb 4, 2022

Should we now backport #4969 to this? Or do other things on the existing Apache connector have to be changed prior to merging this?

Port fix from eclipse-ee4j#4969 to HttpClient 5.x

Signed-off-by: Steffen Nießing <[email protected]>
@zUniQueX
Copy link
Contributor Author

Sorry, it took a while to get back to this. I hope I wasn't blocking the release of 2.36.

@jansupol
Copy link
Contributor

@zUniQueX We are sorry it takes so long to review - we definitely plan to have this in 2.36 - I assume around the end of April.

@zUniQueX
Copy link
Contributor Author

@jansupol Thanks for the fast answer. I'm personally not waiting for this to release, I just wanted to make sure I'm not blocking your release as this is listed in the Wiki for 2.36.

@jansupol
Copy link
Contributor

CQ 24005

Remove exception on null content type

Signed-off-by: Steffen Nießing <[email protected]>
@senivam senivam merged commit 673e229 into eclipse-ee4j:master Apr 13, 2022
@senivam
Copy link
Contributor

senivam commented Apr 13, 2022

CQ is resolved, merging...

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.

Add support for Apache HTTP Client 5.x
3 participants