Replace IO layer to use Apache HC + trivial cleanups by slisaasquatch · Pull Request #390 · killbilling/recurly-java-library · GitHub

As you probably know, I've created a few smaller pull requests in the past, and those were actually brought over from my fork, which is what we use today on production at SaaSquatch.

The pull request you see here represents the entirety of my fork, which is at this moment up and running on SaaSquatch servers, and has been battle tested for over a year without any issue.

While my previous smaller pull requests mainly focus on fixing obvious bugs and issues, this large pull request mainly aims to "modernize" this library to make it work well on a modern environment, as well as to make some minor improvements.

Here are some key points:

Even though I said the word "modernize", this fork still targets Java 6.
The outdated version of async-http-client (hereinafter referred to as AHC) has been replaced with Apache HC (version 4). The old version of AHC conflicts with some newer dependencies we use, and Apache HC is IMO the perfect choice as the IO layer for a couple of reasons:
- It targets Java 6 (HC5 is already out, but that targets Java 7).
- Apache has been very good about API stability and not making breaking changes. HC5 is out already, and that is under a different namespace, so this library will not conflict with projects that depend on HC5.
- It's a blocking client, which makes exception handling a breeze. recurly-java-library is blocking anyway and really doesn't need an async HTTP client, and as you probably already know, AHC unnecessarily complicates exception handling.
- It comes with a handful of useful utilities that this library needs. For example, the QueryParams class right now is building parameters by manually concatenating Strings without any kind of URL encoding. URLEncodedUtils from Apache HC solves exactly that.
While switching to Apache HC, there are obviously classes from AHC that need to be replaced with their equivalents, and you will clearly see them in the pull request.
I have attempted to maintain all of the existing behaviors and quirks of AHwith Apache HC. These include:
- (Not quite) unlimited number of connections. I've configured Apache HC to do 256 maximum connections, which should be more than enough in practice. If you would like me to increase that, please let me know.
- The default timeouts from AHC have been brought over. Although I think the default timeouts are a little too generous.
- The InputStreams returned are in-memory ByteArrayInputStreams. This is one of the quirks of AHC, as well as a number of other non-blocking Netty based HTTP clients. Apache HC supports streaming InputStreams, but I'm arbitrarily buffering it in memory to replicate the old behavior.
There are some minor cleanups and improvements. I'll try to list all of them here:
- I switched to using Guava to do Base64 encoding instead of using DataTypeConverter, which can cause problems with Java 9+.
- As I've mentioned above, QueryParams now uses a proper library to generate query parameters.
- I've replaced all the references to UTF-8 with Guava's Charsets.UTF_8.
- I've replaced all the hard coded HTTP header names with Guava's HttpHeaders.
- The validateHost method was scattered all over the place. Now it's only called in one place.
- The validHosts List has been replaced with a Set, which should be much faster at doing contains.
- I've created a shared XmlMapper in RecurlyObject, and it's used by RecurlyClient and Notification. There are already TODO comments for it, and it actually supports our use case at SaaSquatch. We need to be able to create "lightweight" RecurlyClients to use multiple API keys. The createHttpClient method can already be overridden so I can already use a shared instance of the HTTP client, the XmlMapper is just the next and final thing that prevents us from creating a "lightweight" RecurlyClient.
I don't know if you already know this but a while back I actually created a ticket (not a pull request) with a different approach of doing an API key agnostic RecurlyClient. I do agree that those changes were finicky and hard to verify. I do not believe that this pull request has those problems.
And my final point is that, even though this is a pretty big pull request, there are 0 breaking changes on the API layer, and I have also attempted to maintain existing implicit behaviors. And again, this is the version of this library that we at SaaSquatch have been using.

If you have any questions or suggestions, please don't hesitate to ask.

Thanks!

Hi @slisaasquatch. Great to see another contribution!

Due to the size and nature of this PR, I'm going to bring it to the team for review. I'll circle back with questions as they arise.

Hi @slisaasquatch. We ran integration tests on your PR and found one test failing, pertaining to terminateSubscription(). Otherwise, it looks good!

testCreateSubscriptions(com.ning.billing.recurly.TestRecurlyClient) Time elapsed: 5.321 sec <<< FAILURE!
com.ning.billing.recurly.RecurlyAPIException: RecurlyAPIError{description='The "in" parameter is missing, must be one of 'full', 'partial', 'none'', symbol='refund_invalid', details='null', httpStatusCode='400', responseMetadata='ResponseMetadata{requestId=59db5a746fd50d22-ATL, cfRay=59db5a746fd50d22-ATL, statusCode=400}'}
 at com.ning.billing.recurly.RecurlyClient.callRecurlyXmlContent(RecurlyClient.java:2535)
 at com.ning.billing.recurly.RecurlyClient.callRecurlySafeXmlContent(RecurlyClient.java:2469)
 at com.ning.billing.recurly.RecurlyClient.doPUT(RecurlyClient.java:2426)
 at com.ning.billing.recurly.RecurlyClient.doPUT(RecurlyClient.java:2400)
 at com.ning.billing.recurly.RecurlyClient.terminateSubscription(RecurlyClient.java:654)
 at com.ning.billing.recurly.TestRecurlyClient.testCreateSubscriptions(TestRecurlyClient.java:905)

In terminateSubscription(), the URL constructed in doPUT is something like https://[your-subdomain].recurly.com:443/v2/subscriptions/53c736ab60f3b962217d224fc88b842e/terminate?refund=full?per_page=20, which is triggering the error above. We can repair this by refactoring terminateSubscription() like so:

final QueryParams qp = new QueryParams();
    qp.put("refund", refund.toString());
    doPUT(Subscription.SUBSCRIPTION_RESOURCE + "/" + urlEncode(subscription.getUuid()) + "/terminate",
       subscription, Subscription.class, qp);

The resulting URL constructed is now https://[your-subdomain].recurly.com:443/v2/subscriptions/53e20e3cab9f3d6aeceeda498688af2c/terminate?per_page=20&refund=full. This way we can retain your improvements.

There are other instances where this solution can be applied that the integration tests don't catch, e.g. postponeSubscription(). We're open to suggestions on this approach. Let us know how we can help!

Yep, the issue makes sense. I just pushed the fix for terminateSubscription and postponeSubscription.

@slisaasquatch Thanks for taking the time to do this and contribute it back! As you might know, we've put all our modernization efforts into our new and official Java library but we still intend to support our customers on this library for as long as possible. So, we're trying to be as conservative as possible with this library as it is heavily used in our customers' legacy systems.

Having said that, I think this is a positive change and worth the risks associated. I appreciate that you've attempted to make it backwards compatible (on an API level). We've prodded this enough to have the confidence to say that that effort was successful. Still, I have some questions about the upgrade process for our other users. I practiced an upgrade locally and all seemed to go well, but do you think this will require any special instructions for people who may use a shaded version of the jar? Or possible conflicts with other dependencies? Let us know if there is anything else we can test out to get some more confidence. We're admittedly a little unsure about all the possible ways this may be getting used in the wild.

I think it should be a seamless upgrade for most people, unless they use the ning AHC transitive dependency, in which case they just need to include it explicitly. I think shading will only make a difference for people that are incorrectly using the shaded dependency introduced by libraries that depend on this library.
The only conflict Apache HC has that I know of is Android, but since this library doesn't seem to work well on Android anyway, I don't think it's a big deal.

I think shading will only make a difference for people that are incorrectly using the shaded dependency introduced by libraries that depend on this library.

Good to hear! Should be smooth then.

The only conflict Apache HC has that I know of is Android, but since this library doesn't seem to work well on Android anyway, I don't think it's a big deal.

Good callout, but that consequence should be acceptable. We don't support using this API from mobile anyway.

slisaasquatch added 5 commits May 8, 2020 18:56

Switch to Apache HC + trivial cleanups

5702fc8

Shared XmlMapper

7eef5b0

Rearrange methods

c169f6f

Restore cast

e8d3c3e

Update RecurlyClient.java

790cf67

joannasese self-assigned this May 12, 2020

Fix query param formatting

98c1525

joannasese merged commit f97f221 into killbilling:master Jun 12, 2020

joannasese mentioned this pull request Jun 22, 2020

Set content-length header to 0 when entity is null #394

Closed

glasser mentioned this pull request Jul 6, 2020

Encoding of strings read from Recurly is sensitive to current locale #403

Closed

Replace IO layer to use Apache HC + trivial cleanups by slisaasquatch · Pull Request #390 · killbilling/recurly-java-library · GitHub

Replace IO layer to use Apache HC + trivial cleanups - #390

Replace IO layer to use Apache HC + trivial cleanups#390
joannasese merged 6 commits into
killbilling:masterfrom
saasquatch:issue-20200508-3

slisaasquatch commented May 9, 2020 •
edited
Loading

Uh oh!

Uh oh!

joannasese commented May 12, 2020 •
edited by bhelx
Loading

Uh oh!

Uh oh!

joannasese commented Jun 3, 2020 •
edited
Loading

Uh oh!

Uh oh!

slisaasquatch commented Jun 3, 2020

Uh oh!

bhelx commented Jun 8, 2020

Uh oh!

slisaasquatch commented Jun 8, 2020

Uh oh!

bhelx commented Jun 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Replace IO layer to use Apache HC + trivial cleanups by slisaasquatch · Pull Request #390 · killbilling/recurly-java-library · GitHub

Replace IO layer to use Apache HC + trivial cleanups - #390

Replace IO layer to use Apache HC + trivial cleanups#390joannasese merged 6 commits intokillbilling:masterkillbilling/recurly-java-library:masterfrom saasquatch:issue-20200508-3saasquatch/recurly-java-library:issue-20200508-3Copy head branch name to clipboard

Conversation

slisaasquatch commented May 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joannasese commented May 12, 2020 • edited by bhelx Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joannasese commented Jun 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slisaasquatch commented Jun 3, 2020

Uh oh!

bhelx commented Jun 8, 2020

Uh oh!

slisaasquatch commented Jun 8, 2020

Uh oh!

bhelx commented Jun 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Replace IO layer to use Apache HC + trivial cleanups#390
joannasese merged 6 commits into
killbilling:masterfrom
saasquatch:issue-20200508-3

slisaasquatch commented May 9, 2020 •
edited
Loading

joannasese commented May 12, 2020 •
edited by bhelx
Loading

joannasese commented Jun 3, 2020 •
edited
Loading