Fix uploads API when using NewBackends #733
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Geeze, sorry about the trouble here, and really excellent debugging work to get to the bottom of it. I think I'm going to try and unwind the Thanks! LGTM. |
|
Released as 54.1.1. |
brandur-stripe
pushed a commit
that referenced
this pull request
Nov 30, 2018
Traditionally, stripe-go has done something a little weird in that it considered the `/v1/` in API URLs as part of the URL (e.g., `APIURL` or `UploadURL`) instead of part of the paths of individual API calls. So when calling `POST /v1/charges` we'd call `POST /charges` and rely on the fact that `/v1/` was expected to be present in the URL. This has always rubbed me the wrong way a little bit, and caused a little bit of trouble in #733 as we'd been doing something a little hacky when initializing new configs for backends. This has some potential backwards compatibility problems in that people may have been injecting custom URLs (e.g., if they had a testing setup a little like we do for stripe-mock) that included the `/v1/` prefix. I've tried to address this by trimming a `/v1/` prefix when creating a backend with a configured URL. For example: ``` go switch backendType { case APIBackend: if config.URL == "" { config.URL = apiURL } // For a long time we had the `/v1` prefix as part of a configured URL // rather than in the per-package URLs throughout the library. Continue // to support this for the time being by stripping one tha's been // passed for better backwards compatibility. config.URL = strings.TrimPrefix(config.URL, "/v1") ``` I believe that will address the problem. There's some risk that I missed a URL string in the project because there are a lot of them. I tried to minimize the risk there by using a tool-assisted project-wide find + replace, then manually examining every string in the project that looks like a URL path (e.g., starts with `"/`).
brandur-stripe
pushed a commit
that referenced
this pull request
Nov 30, 2018
Traditionally, stripe-go has done something a little weird in that it considered the `/v1/` in API URLs as part of the URL (e.g., `APIURL` or `UploadURL`) instead of part of the paths of individual API calls. So when calling `POST /v1/charges` we'd call `POST /charges` and rely on the fact that `/v1/` was expected to be present in the URL. This has always rubbed me the wrong way a little bit, and caused a little bit of trouble in #733 as we'd been doing something a little hacky when initializing new configs for backends. This has some potential backwards compatibility problems in that people may have been injecting custom URLs (e.g., if they had a testing setup a little like we do for stripe-mock) that included the `/v1/` prefix. I've tried to address this by trimming a `/v1/` prefix when creating a backend with a configured URL. For example: ``` go switch backendType { case APIBackend: if config.URL == "" { config.URL = apiURL } // For a long time we had the `/v1` suffix as part of a configured URL // rather than in the per-package URLs throughout the library. Continue // to support this for the time being by stripping one that's been // passed for better backwards compatibility. config.URL = strings.TrimSuffix(config.URL, "/v1") ``` I believe that will address the problem. There's some risk that I missed a URL string in the project because there are a lot of them. I tried to minimize the risk there by using a tool-assisted project-wide find + replace, then manually examining every string in the project that looks like a URL path (e.g., starts with `"/`).
brandur-stripe
pushed a commit
that referenced
this pull request
Nov 30, 2018
Traditionally, stripe-go has done something a little weird in that it considered the `/v1/` in API URLs as part of the URL (e.g., `APIURL` or `UploadURL`) instead of part of the paths of individual API calls. So when calling `POST /v1/charges` we'd call `POST /charges` and rely on the fact that `/v1/` was expected to be present in the URL. This has always rubbed me the wrong way a little bit, and caused a little bit of trouble in #733 as we'd been doing something a little hacky when initializing new configs for backends. This has some potential backwards compatibility problems in that people may have been injecting custom URLs (e.g., if they had a testing setup a little like we do for stripe-mock) that included the `/v1/` prefix. I've tried to address this by trimming a `/v1/` prefix when creating a backend with a configured URL. For example: ``` go switch backendType { case APIBackend: if config.URL == "" { config.URL = apiURL } // For a long time we had the `/v1` suffix as part of a configured URL // rather than in the per-package URLs throughout the library. Continue // to support this for the time being by stripping one that's been // passed for better backwards compatibility. config.URL = strings.TrimSuffix(config.URL, "/v1") ``` I believe that will address the problem. There's some risk that I missed a URL string in the project because there are a lot of them. I tried to minimize the risk there by using a tool-assisted project-wide find + replace, then manually examining every string in the project that looks like a URL path (e.g., starts with `"/`).
brandur-stripe
pushed a commit
that referenced
this pull request
Nov 30, 2018
Traditionally, stripe-go has done something a little weird in that it considered the `/v1/` in API URLs as part of the URL (e.g., `APIURL` or `UploadURL`) instead of part of the paths of individual API calls. So when calling `POST /v1/charges` we'd call `POST /charges` and rely on the fact that `/v1/` was expected to be present in the URL. This has always rubbed me the wrong way a little bit, and caused a little bit of trouble in #733 as we'd been doing something a little hacky when initializing new configs for backends. This has some potential backwards compatibility problems in that people may have been injecting custom URLs (e.g., if they had a testing setup a little like we do for stripe-mock) that included the `/v1/` prefix. I've tried to address this by trimming a `/v1/` prefix when creating a backend with a configured URL. For example: ``` go switch backendType { case APIBackend: if config.URL == "" { config.URL = apiURL } // For a long time we had the `/v1` suffix as part of a configured URL // rather than in the per-package URLs throughout the library. Continue // to support this for the time being by stripping one that's been // passed for better backwards compatibility. config.URL = strings.TrimSuffix(config.URL, "/v1") config.URL = strings.TrimSuffix(config.URL, "/v1/") ``` I believe that will address the problem. There's some risk that I missed a URL string in the project because there are a lot of them. I tried to minimize the risk there by using a tool-assisted project-wide find + replace, then manually examining every string in the project that looks like a URL path (e.g., starts with `"/`).
brandur-stripe
pushed a commit
that referenced
this pull request
Nov 30, 2018
Traditionally, stripe-go has done something a little weird in that it considered the `/v1/` in API URLs as part of the URL (e.g., `APIURL` or `UploadURL`) instead of part of the paths of individual API calls. So when calling `POST /v1/charges` we'd call `POST /charges` and rely on the fact that `/v1/` was expected to be present in the URL. This has always rubbed me the wrong way a little bit, and caused a little bit of trouble in #733 as we'd been doing something a little hacky when initializing new configs for backends. This has some potential backwards compatibility problems in that people may have been injecting custom URLs (e.g., if they had a testing setup a little like we do for stripe-mock) that included the `/v1/` suffix. I've tried to address this by trimming a `/v1/` suffix when creating a backend with a configured URL. For example: ``` go switch backendType { case APIBackend: if config.URL == "" { config.URL = apiURL } // For a long time we had the `/v1` suffix as part of a configured URL // rather than in the per-package URLs throughout the library. Continue // to support this for the time being by stripping one that's been // passed for better backwards compatibility. config.URL = strings.TrimSuffix(config.URL, "/v1") config.URL = strings.TrimSuffix(config.URL, "/v1/") ``` I believe that will address the problem. There's some risk that I missed a URL string in the project because there are a lot of them. I tried to minimize the risk there by using a tool-assisted project-wide find + replace, then manually examining every string in the project that looks like a URL path (e.g., starts with `"/`).
brandur-stripe
pushed a commit
that referenced
this pull request
Dec 1, 2018
Traditionally, stripe-go has done something a little weird in that it considered the `/v1/` in API URLs as part of the URL (e.g., `APIURL` or `UploadURL`) instead of part of the paths of individual API calls. So when calling `POST /v1/charges` we'd call `POST /charges` and rely on the fact that `/v1/` was expected to be present in the URL. This has always rubbed me the wrong way a little bit, and caused a little bit of trouble in #733 as we'd been doing something a little hacky when initializing new configs for backends. This has some potential backwards compatibility problems in that people may have been injecting custom URLs (e.g., if they had a testing setup a little like we do for stripe-mock) that included the `/v1/` suffix. I've tried to address this by trimming a `/v1/` suffix when creating a backend with a configured URL. For example: ``` go switch backendType { case APIBackend: if config.URL == "" { config.URL = apiURL } // For a long time we had the `/v1` suffix as part of a configured URL // rather than in the per-package URLs throughout the library. Continue // to support this for the time being by stripping one that's been // passed for better backwards compatibility. config.URL = strings.TrimSuffix(config.URL, "/v1") config.URL = strings.TrimSuffix(config.URL, "/v1/") ``` I believe that will address the problem. There's some risk that I missed a URL string in the project because there are a lot of them. I tried to minimize the risk there by using a tool-assisted project-wide find + replace, then manually examining every string in the project that looks like a URL path (e.g., starts with `"/`).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When testing uploading files using an API client, I found that it would fail consistently with a strange error:
{"code":"invalid_utf8_in_post_body","status":400,"message":"Invalid UTF-8 characters found in POST body: \"application/octet-stream\". For assistance, please contact support@stripe.com.","type":"invalid_request_error"}Adding some log statements to the
backendImplementation.Dofunction, I found that it was trying to send afiles.Newrequest tohttps://api.stripe.com/v1/v1/filesEventually I tracked the problem down to the
NewBackendsfunction. It reused the config object when creating the api and uploads backends. TheGetBackendWithConfigfunction then modified the config object. So when it was called again with the same config object, it would already have the URL field filled out.Simply put, we need two copies of the config object to avoid causing problems with the modified config object.