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

Add fix for per-request ApiBase #1501

Merged
merged 3 commits into from
Jan 4, 2023

Conversation

richardm-stripe
Copy link
Contributor

Notify

r? @pakrym-stripe

Summary

Add tests for the feature introduced in #1476 and fixes two bugs (these bugs did not get released!)

  • One bug broke .toBuilderFullCopy(). It didn't copy over the new field baseUrl.
  • The other bug was that new RequestOptions() defaulted to baseUrl = Stripe.getApiBase() when really it should have defaulted to null. This means that code like in Quote.java
  public InputStream pdf(Map<String, Object> params, RequestOptions options)
      throws StripeException {
    String url =
        ApiResource.fullUrl(
            Stripe.getUploadBase(),
            options,
            String.format("/v1/quotes/%s/pdf", ApiResource.urlEncodeId(this.getId())));
    return ApiResource.requestStream(ApiResource.RequestMethod.GET, url, params, options);
  }

would have regressed and been sending requests to api.stripe.com instead of files.stripe.com as intended.

@richardm-stripe
Copy link
Contributor Author

r? @pakrym-stripe added another commit: 770bc13

It looks like I missed replacing String.format with Stripe.fullUrl in a couple non-codegenned methods.

I'm now confident I got them all. I did
grep -r Stripe.getApiBase -B1 (same for uploadBase and connectBase) to get all the places this is called. Then I did some vim magic to join any line that ended with "Stripe.fullUrl(\n" with the next line, then I deleted any line that contained "Stripe.fullUrl" then I looked at all the remaining instances of Stripe.apiBase and they were all in tests or error messages.

@richardm-stripe richardm-stripe merged commit aa87441 into master Jan 4, 2023
@richardm-stripe richardm-stripe deleted the richardm-add-apiBase-fix-and-tests branch January 4, 2023 16:42
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.

2 participants