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

feat!: Upgrading java-http-client to use apache httpclient5 #148

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ryantomlinson95
Copy link

Fixes #147

sendgrid-java, specifically the constructor that consumes a Client is currently incompatible with apache httpclient5 due to it's dependency on this repository, a simplified version of the http client. In order for projects on httpclient5 to use sendgrid-java, this dependency needs to be updated first.

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the Contribution Guidelines and my PR follows them
  • I have titled the PR appropriately
  • I have updated my branch with the main branch
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation about the functionality in the appropriate .md file
  • I have added inline documentation to the code I modified

If you have questions, please file a support ticket, or create a GitHub Issue in this repository.

sendgrid-java is currently incompatible with apache httpclient5 due to it's dependency on this simplified version of the http client. In order for projects on httpclient5 to use sendgrid-java, this dependency needs to be updated first.
@ryantomlinson95
Copy link
Author

I used the official Apache guide for the migration: https://hc.apache.org/httpcomponents-client-5.3.x/migration-guide/migration-to-classic.html, as well as other resources like some of the namespace mappings from here: https://docs.openrewrite.org/recipes/apache/httpclient5/upgradeapachehttpclient_5_classmapping.

throw ex;
}
uri = buildUri(request.getBaseUri(), request.getEndpoint(), request.getQueryParams());
httpGet = new HttpGet(uri.toString());
Copy link

@jkosternl jkosternl Sep 7, 2024

Choose a reason for hiding this comment

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

You can even move the declaration and assignment to the same line, to merge line 170 with 173. The same is valid for the other methods: post, patch, put below.. etc.
Looking good, for the rest. 👍🏻

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, fixed!

Copy link

@jkosternl jkosternl left a comment

Choose a reason for hiding this comment

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

Looking good to me! (I am not a maintainer on this project btw). Hopefully it gets merged and approved by others as well.

@ryantomlinson95
Copy link
Author

ryantomlinson95 commented Sep 9, 2024

My suggestion for this PR would be to release it under a new major version, such that it is clear that it is a breaking change for consumers. Then we can also update sendgrid-java with a major version change, again to not break existing functionality for consumers of that library.

@ryantomli
Copy link

Opened a linking issue in the main repo to hopefully make this more visible: sendgrid/sendgrid-java#772

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.

Bump Sendgrid http client library to use latest apache http client 5
3 participants