Merge pull request #34770 from dnephin/fix-client-with-empty-id

Fix volume inspect with empty ID

client/client.go

@@ -212,18 +212,11 @@ func (cli *Client) getAPIPath(p string, query url.Values) string {
 	var apiPath string
 	if cli.version != "" {
 		v := strings.TrimPrefix(cli.version, "v")
-		apiPath = path.Join(cli.basePath, "/v"+v+p)
+		apiPath = path.Join(cli.basePath, "/v"+v, p)
 	} else {
 		apiPath = path.Join(cli.basePath, p)
 	}
-
-	u := &url.URL{
-		Path: apiPath,
-	}
-	if len(query) > 0 {
-		u.RawQuery = query.Encode()
-	}
-	return u.String()
+	return (&url.URL{Path: apiPath, RawQuery: query.Encode()}).String()
 }
 
 // ClientVersion returns the API version used by this client.

client/client_test.go

@@ -105,11 +105,11 @@ func TestNewEnvClient(t *testing.T) {
 }
 
 func TestGetAPIPath(t *testing.T) {
-	cases := []struct {
-		v string
-		p string
-		q url.Values
-		e string
+	testcases := []struct {
+		version  string
+		path     string
+		query    url.Values
+		expected string
 	}{
 		{"", "/containers/json", nil, "/containers/json"},
 		{"", "/containers/json", url.Values{}, "/containers/json"},
@@ -123,16 +123,10 @@ func TestGetAPIPath(t *testing.T) {
 		{"v1.22", "/networks/kiwl$%^", nil, "/v1.22/networks/kiwl$%25%5E"},
 	}
 
-	for _, cs := range cases {
-		c, err := NewClient("unix:///var/run/docker.sock", cs.v, nil, nil)
-		if err != nil {
-			t.Fatal(err)
-		}
-		g := c.getAPIPath(cs.p, cs.q)
-		assert.Equal(t, g, cs.e)
-
-		err = c.Close()
-		assert.NoError(t, err)
+	for _, testcase := range testcases {
+		c := Client{version: testcase.version, basePath: "/"}
+		actual := c.getAPIPath(testcase.path, testcase.query)
+		assert.Equal(t, actual, testcase.expected)
 	}
 }
 

client/volume_inspect.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"io/ioutil"
 	"net/http"
+	"path"
 
 	"github.com/docker/docker/api/types"
 	"golang.org/x/net/context"
@@ -18,8 +19,15 @@ func (cli *Client) VolumeInspect(ctx context.Context, volumeID string) (types.Vo
 
 // VolumeInspectWithRaw returns the information about a specific volume in the docker host and its raw representation
 func (cli *Client) VolumeInspectWithRaw(ctx context.Context, volumeID string) (types.Volume, []byte, error) {
+	// The empty ID needs to be handled here because with an empty ID the
+	// request url will not contain a trailing / which calls the volume list API
+	// instead of volume inspect
+	if volumeID == "" {
+		return types.Volume{}, nil, volumeNotFoundError{volumeID}
+	}
+
 	var volume types.Volume
-	resp, err := cli.get(ctx, "/volumes/"+volumeID, nil, nil)
+	resp, err := cli.get(ctx, path.Join("/volumes", volumeID), nil, nil)
 	if err != nil {
 		if resp.statusCode == http.StatusNotFound {
 			return volume, nil, volumeNotFoundError{volumeID}

client/volume_inspect_test.go

@@ -10,6 +10,9 @@ import (
 	"testing"
 
 	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/internal/testutil"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"golang.org/x/net/context"
 )
 
@@ -19,9 +22,7 @@ func TestVolumeInspectError(t *testing.T) {
 	}
 
 	_, err := client.VolumeInspect(context.Background(), "nothing")
-	if err == nil || err.Error() != "Error response from daemon: Server error" {
-		t.Fatalf("expected a Server Error, got %v", err)
-	}
+	testutil.ErrorContains(t, err, "Error response from daemon: Server error")
 }
 
 func TestVolumeInspectNotFound(t *testing.T) {
@@ -30,13 +31,34 @@ func TestVolumeInspectNotFound(t *testing.T) {
 	}
 
 	_, err := client.VolumeInspect(context.Background(), "unknown")
-	if err == nil || !IsErrVolumeNotFound(err) {
-		t.Fatalf("expected a volumeNotFound error, got %v", err)
+	assert.True(t, IsErrNotFound(err))
+}
+
+func TestVolumeInspectWithEmptyID(t *testing.T) {
+	expectedURL := "/volumes/"
+
+	client := &Client{
+		client: newMockClient(func(req *http.Request) (*http.Response, error) {
+			assert.Equal(t, req.URL.Path, expectedURL)
+			return &http.Response{
+				StatusCode: http.StatusNotFound,
+				Body:       ioutil.NopCloser(bytes.NewReader(nil)),
+			}, nil
+		}),
 	}
+	_, err := client.VolumeInspect(context.Background(), "")
+	testutil.ErrorContains(t, err, "No such volume: ")
+
 }
 
 func TestVolumeInspect(t *testing.T) {
 	expectedURL := "/volumes/volume_id"
+	expected := types.Volume{
+		Name:       "name",
+		Driver:     "driver",
+		Mountpoint: "mountpoint",
+	}
+
 	client := &Client{
 		client: newMockClient(func(req *http.Request) (*http.Response, error) {
 			if !strings.HasPrefix(req.URL.Path, expectedURL) {
@@ -45,11 +67,7 @@ func TestVolumeInspect(t *testing.T) {
 			if req.Method != "GET" {
 				return nil, fmt.Errorf("expected GET method, got %s", req.Method)
 			}
-			content, err := json.Marshal(types.Volume{
-				Name:       "name",
-				Driver:     "driver",
-				Mountpoint: "mountpoint",
-			})
+			content, err := json.Marshal(expected)
 			if err != nil {
 				return nil, err
 			}
@@ -60,17 +78,7 @@ func TestVolumeInspect(t *testing.T) {
 		}),
 	}
 
-	v, err := client.VolumeInspect(context.Background(), "volume_id")
-	if err != nil {
-		t.Fatal(err)
-	}
-	if v.Name != "name" {
-		t.Fatalf("expected `name`, got %s", v.Name)
-	}
-	if v.Driver != "driver" {
-		t.Fatalf("expected `driver`, got %s", v.Driver)
-	}
-	if v.Mountpoint != "mountpoint" {
-		t.Fatalf("expected `mountpoint`, got %s", v.Mountpoint)
-	}
+	volume, err := client.VolumeInspect(context.Background(), "volume_id")
+	require.NoError(t, err)
+	assert.Equal(t, expected, volume)
 }