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 httpclient5 support #33

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Apache httpclient5 support #33

wants to merge 7 commits into from

Conversation

kjozsa
Copy link

@kjozsa kjozsa commented Feb 15, 2022

Hi Matthias,

we are using hcclient5's ClassicHttpRequest in our project and I needed an OAuth1 lib badly.. The fastest way seemed to be adding support for this in SignPost. I cleared up my PR, I hope it makes into the base project!

Should you have any comments or change requests regarding this code, let's get in touch..

cheers,
Kristof

@mttkay mttkay requested a review from simon04 February 17, 2022 06:30
@mttkay
Copy link
Owner

mttkay commented Feb 17, 2022

Hi, thanks for your contribution!

I have not maintained this repository in many years, but I see @simon04 has done so more recently -- do you have capacity to look at these changes Simon?

@kjozsa
Copy link
Author

kjozsa commented Feb 23, 2022

Hi @simon04, do you think you could review these changes some day? Thanks much..

@aakarshsingh
Copy link

@kjozsa Is there any thought of adding support for the HttpAsyncClient5 as well?

@aakarshsingh
Copy link

tmp_1645645179580.jpg

Here is the class layout for both classic (in blue) and async (green) methods of Apache 5. Theoretically, should be able to sign a 'BasicHttpRequest' to serve both types.

@kjozsa
Copy link
Author

kjozsa commented Mar 8, 2022

Hi all, I implemented full hcclient5 support, sync + async support are both ready now in my PR.

Note that for the unit tests I needed to bump Mockito to v4 and JUnit to the latest 4.x. I aimed for minimal changes, but this required updating a few more classes around the codebase..

Please review my changes and accept my contribution if satisfied.

@aakarshsingh
Copy link

This looks amazing. Thanks, @kjozsa.

I hope one of the maintainers can look at this soon.

@kjozsa
Copy link
Author

kjozsa commented Mar 16, 2022

@mttkay what do you think? Also, can you please help reaching out to @simon04?

Comment on lines +24 to +30
import org.apache.hc.core5.http.ClassicHttpResponse;
import org.apache.hc.core5.http.HttpEntity;
import org.apache.hc.core5.http.Method;
import org.apache.hc.core5.http.io.entity.EntityUtils;
import org.apache.hc.core5.http.message.BasicHttpResponse;

import java.io.IOException;

Choose a reason for hiding this comment

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

Suggested change
import org.apache.hc.core5.http.ClassicHttpResponse;
import org.apache.hc.core5.http.HttpEntity;
import org.apache.hc.core5.http.Method;
import org.apache.hc.core5.http.io.entity.EntityUtils;
import org.apache.hc.core5.http.message.BasicHttpResponse;
import java.io.IOException;
import org.apache.hc.core5.http.Method;

import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.util.Arrays;

Choose a reason for hiding this comment

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

Suggested change
import java.util.Arrays;

package oauth.signpost.commonshttp5.async;

import org.apache.hc.client5.http.async.methods.SimpleHttpResponse;
import org.apache.hc.core5.http.ClassicHttpResponse;

Choose a reason for hiding this comment

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

Suggested change
import org.apache.hc.core5.http.ClassicHttpResponse;


public Map<String, String> getAllHeaders() {
Header[] origHeaders = request.getHeaders();
HashMap<String, String> headers = new HashMap<String, String>();

Choose a reason for hiding this comment

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

Suggested change
HashMap<String, String> headers = new HashMap<String, String>();
HashMap<String, String> headers = new HashMap<>();


public Map<String, String> getAllHeaders() {
Header[] origHeaders = request.getHeaders();
HashMap<String, String> headers = new HashMap<String, String>();

Choose a reason for hiding this comment

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

Suggested change
HashMap<String, String> headers = new HashMap<String, String>();
HashMap<String, String> headers = new HashMap<>();

@yokikathir
Copy link

How is update?

HMAC-SHA1 will no longer be accepted as a signature method for TBA integrations. Before your account is upgraded to 2023.1, you must update your TBA integrations to use the HMAC-SHA256 signature method.

@kjozsa
Copy link
Author

kjozsa commented Jun 15, 2022

I don't want to show up as the drama queen in this story, but my conclusion is that it's a waste of time. No repo owners showed up or has shown any sign of interest in merging that code, so it probably makes no sense to push more energies into this.

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.

4 participants