JoinPaths: don't add "/" before the query string (Azure#19278)

* JoinPaths: do not unconditionally add a forward slash before the query string

Preserve a trailing slash if the last path element has it

Fixes Azure#19245

* azblob GetSASURL: don't add a forward slash before the query string

* slight change based on url.JoinPath

also fix container.GetSASURL

Co-authored-by: Joel Hendrix <jhendrix@microsoft.com>

Author: drakkan

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.

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.

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)
+		}
 	}
 }
 

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

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
 }

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
 }

sdk/storage/azblob/service/client.go

@@ -9,14 +9,14 @@ package service
 import (
 	"context"
 	"errors"
-	"github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/blob"
 	"net/http"
 	"strings"
 	"time"
 
 	"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()