Put /v1/ prefix as part of all paths instead of URL

[brandur]

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:

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
"/).

r? @remi-stripe Any thoughts on this?
cc @stripe-internal/api-libraries

[oralordos]

Is that supposed to be a prefix or a suffix? If I pass in a URL of https://api.stripe.com/v1, that would be a suffix. It is a prefix of the path, but then you would need to parse the URL to get the path.

[brandur-stripe]

@oralordos Hah, great catch. I was just writing a test and realized that I got this backwards as it failed ... pushing a fix momentarily.

[brandur-stripe]

Alright, I corrected the patch, amended the commit message/description above, added support for trimming /v1/ in addition to /v1 because it's likely a common mistake, and added a test to check that this works as expected:

func TestGetBackendWithConfig_TrimV1Suffix(t *testing.T) {
	{
		backend := stripe.GetBackendWithConfig(
			stripe.APIBackend,
			&stripe.BackendConfig{
				URL: "https://api.stripe.com/v1",
			},
		).(*stripe.BackendImplementation)

		// The `/v1` suffix has been stripped.
		assert.Equal(t, "https://api.stripe.com", backend.URL)
	}

	// Also support trimming a `/v1/` with an extra trailing slash which is
	// probably an often seen mistake.
	{
		backend := stripe.GetBackendWithConfig(
			stripe.APIBackend,
			&stripe.BackendConfig{
				URL: "https://api.stripe.com/v1/",
			},
		).(*stripe.BackendImplementation)

		assert.Equal(t, "https://api.stripe.com", backend.URL)
	}

	// No-op otherwise.
	{
		backend := stripe.GetBackendWithConfig(
			stripe.APIBackend,
			&stripe.BackendConfig{
				URL: "https://api.stripe.com",
			},
		).(*stripe.BackendImplementation)

		assert.Equal(t, "https://api.stripe.com", backend.URL)
	}
}

[remi-stripe]

one minor comment but lgtm otherwise

[remi-stripe]

minor but should we also trim the trailing slash so that if you pass https://api.stripe.com/ it works too? And similarly ensure there's a leading slash in the other part of the url passed from CallRaw? Does not seem like a big deal but I know I've hit that before and since we're already trying to be useful.

[brandur-stripe]

minor but should we also trim the trailing slash so that if you pass https://api.stripe.com/ it works too?

Great point! We can nicen up this code quite a bit by just (1) trimming a trailing slash, then (2) trimming just /v1 if present. I made those changes.

And similarly ensure there's a leading slash in the other part of the url passed from CallRaw? Does not seem like a big deal but I know I've hit that before and since we're already trying to be useful.

Good point. I left this out for now only because we seem to already be compensating for that possibility in NewRequest (which is called from within any of the other Call methods):

func (s *BackendImplementation) NewRequest(method, path, key, contentType string, params *Params) (*http.Request, error) {
	if !strings.HasPrefix(path, "/") {
		path = "/" + path
	}

	...

I'd actually prefer that this become a panic because there's really no reason to support this misuse, but since the prefix check is already in there, I think it's fine to just leave it like that for now.

[brandur-stripe]

Released as 54.2.0.