diff --git a/sdk/azcore/CHANGELOG.md b/sdk/azcore/CHANGELOG.md index e709d8469cc3..a62228773f6d 100644 --- a/sdk/azcore/CHANGELOG.md +++ b/sdk/azcore/CHANGELOG.md @@ -8,6 +8,7 @@ ### Bugs Fixed * Don't retry a request if the `Retry-After` delay is greater than the configured `RetryOptions.MaxRetryDelay`. +* `runtime.JoinPaths`: do not unconditionally add a forward slash before the query string ### Other Changes * Removed logging URL from retry policy as it's redundant. diff --git a/sdk/azcore/runtime/request.go b/sdk/azcore/runtime/request.go index 118588d828d3..7b00c7613fda 100644 --- a/sdk/azcore/runtime/request.go +++ b/sdk/azcore/runtime/request.go @@ -15,6 +15,7 @@ import ( "fmt" "io" "mime/multipart" + "path" "reflect" "strings" "time" @@ -56,19 +57,23 @@ func JoinPaths(root string, paths ...string) string { root, qps = splitPath[0], splitPath[1] } - for i := 0; i < len(paths); i++ { - root = strings.TrimRight(root, "/") - paths[i] = strings.TrimLeft(paths[i], "/") - root += "/" + paths[i] + p := path.Join(paths...) + // path.Join will remove any trailing slashes. + // if one was provided, preserve it. + if strings.HasSuffix(paths[len(paths)-1], "/") && !strings.HasSuffix(p, "/") { + p += "/" } if qps != "" { - if !strings.HasSuffix(root, "/") { - root += "/" - } - return root + "?" + qps + p = p + "?" + qps + } + + if strings.HasSuffix(root, "/") && strings.HasPrefix(p, "/") { + root = root[:len(root)-1] + } else if !strings.HasSuffix(root, "/") && !strings.HasPrefix(p, "/") { + p = "/" + p } - return root + return root + p } // EncodeByteArray will base-64 encode the byte slice v. diff --git a/sdk/azcore/runtime/request_test.go b/sdk/azcore/runtime/request_test.go index 4ef327e63774..723cff3e5a90 100644 --- a/sdk/azcore/runtime/request_test.go +++ b/sdk/azcore/runtime/request_test.go @@ -544,17 +544,59 @@ func TestRequestSetBodyContentLengthHeader(t *testing.T) { } func TestJoinPaths(t *testing.T) { - if path := JoinPaths(""); path != "" { - t.Fatalf("unexpected path %s", path) + type joinTest struct { + root string + paths []string + expected string } - expected := "http://test.contoso.com/path/one/path/two/path/three/path/four/" - if path := JoinPaths("http://test.contoso.com/", "/path/one", "path/two", "/path/three/", "path/four/"); path != expected { - t.Fatalf("got %s, expected %s", path, expected) + + tests := []joinTest{ + { + root: "", + paths: nil, + expected: "", + }, + { + root: "/", + paths: nil, + expected: "/", + }, + { + root: "http://test.contoso.com/", + paths: []string{"/path/one", "path/two", "/path/three/", "path/four/"}, + expected: "http://test.contoso.com/path/one/path/two/path/three/path/four/", + }, + { + root: "http://test.contoso.com", + paths: []string{"path/one", "path/two", "/path/three/", "path/four/"}, + expected: "http://test.contoso.com/path/one/path/two/path/three/path/four/", + }, + { + root: "http://test.contoso.com/?qp1=abc&qp2=def", + paths: []string{"/path/one", "path/two"}, + expected: "http://test.contoso.com/path/one/path/two?qp1=abc&qp2=def", + }, + { + root: "http://test.contoso.com?qp1=abc&qp2=def", + paths: []string{"path/one", "path/two/"}, + expected: "http://test.contoso.com/path/one/path/two/?qp1=abc&qp2=def", + }, + { + root: "http://test.contoso.com/?qp1=abc&qp2=def", + paths: []string{"path/one", "path/two/"}, + expected: "http://test.contoso.com/path/one/path/two/?qp1=abc&qp2=def", + }, + { + root: "http://test.contoso.com/?qp1=abc&qp2=def", + paths: []string{"/path/one", "path/two/"}, + expected: "http://test.contoso.com/path/one/path/two/?qp1=abc&qp2=def", + }, } - expected = "http://test.contoso.com/path/one/path/two/?qp1=abc&qp2=def" - if path := JoinPaths("http://test.contoso.com/?qp1=abc&qp2=def", "/path/one", "path/two"); path != expected { - t.Fatalf("got %s, expected %s", path, expected) + for _, tt := range tests { + if path := JoinPaths(tt.root, tt.paths...); path != tt.expected { + t.Fatalf("got %s, expected %s", path, tt.expected) + } } } diff --git a/sdk/storage/azblob/CHANGELOG.md b/sdk/storage/azblob/CHANGELOG.md index d16356160cca..e55b278911b5 100644 --- a/sdk/storage/azblob/CHANGELOG.md +++ b/sdk/storage/azblob/CHANGELOG.md @@ -8,6 +8,8 @@ ### Bugs Fixed +* `GetSASURL()`: for container and blob clients, don't add a forward slash before the query string + ### Other Changes ## 0.5.0 (2022-09-29) @@ -18,7 +20,7 @@ ### Features Added -* Added [UserDelegationCredential](https://learn.microsoft.com/rest/api/storageservices/create-user-delegation-sas) which resolves [#18976](https://github.com/Azure/azure-sdk-for-go/issues/18976), [#16916](https://github.com/Azure/azure-sdk-for-go/issues/16916), [#18977](https://github.com/Azure/azure-sdk-for-go/issues/18977) +* Added [UserDelegationCredential](https://learn.microsoft.com/rest/api/storageservices/create-user-delegation-sas) which resolves [#18976](https://github.com/Azure/azure-sdk-for-go/issues/18976), [#16916](https://github.com/Azure/azure-sdk-for-go/issues/16916), [#18977](https://github.com/Azure/azure-sdk-for-go/issues/18977) * Added [Restore Container API](https://learn.microsoft.com/rest/api/storageservices/restore-container). ### Bugs Fixed diff --git a/sdk/storage/azblob/blob/client.go b/sdk/storage/azblob/blob/client.go index 90e101806490..3e5cd948dc7f 100644 --- a/sdk/storage/azblob/blob/client.go +++ b/sdk/storage/azblob/blob/client.go @@ -11,7 +11,6 @@ import ( "errors" "io" "os" - "strings" "sync" "time" @@ -261,11 +260,7 @@ func (b *Client) GetSASURL(permissions sas.BlobPermissions, start time.Time, exp return "", err } - endpoint := b.URL() - if !strings.HasSuffix(endpoint, "/") { - endpoint += "/" - } - endpoint += "?" + qps.Encode() + endpoint := b.URL() + "?" + qps.Encode() return endpoint, nil } diff --git a/sdk/storage/azblob/container/client.go b/sdk/storage/azblob/container/client.go index 97339976709c..0d7b13c6e7b5 100644 --- a/sdk/storage/azblob/container/client.go +++ b/sdk/storage/azblob/container/client.go @@ -10,7 +10,6 @@ import ( "context" "errors" "net/http" - "strings" "time" "github.com/Azure/azure-sdk-for-go/sdk/azcore" @@ -317,11 +316,7 @@ func (c *Client) GetSASURL(permissions sas.ContainerPermissions, start time.Time return "", err } - endpoint := c.URL() - if !strings.HasSuffix(endpoint, "/") { - endpoint += "/" - } - endpoint += "?" + qps.Encode() + endpoint := c.URL() + "?" + qps.Encode() return endpoint, nil } diff --git a/sdk/storage/azblob/service/client.go b/sdk/storage/azblob/service/client.go index d206f0341076..39e36e296f03 100644 --- a/sdk/storage/azblob/service/client.go +++ b/sdk/storage/azblob/service/client.go @@ -9,7 +9,6 @@ package service import ( "context" "errors" - "github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/blob" "net/http" "strings" "time" @@ -17,6 +16,7 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime" + "github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/blob" "github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/container" "github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/internal/base" "github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/internal/exported" @@ -260,6 +260,7 @@ func (s *Client) GetSASURL(resources sas.AccountResourceTypes, permissions sas.A endpoint := s.URL() if !strings.HasSuffix(endpoint, "/") { + // add a trailing slash to be consistent with the portal endpoint += "/" } endpoint += "?" + qps.Encode()