Repository: vmware/govmomi (go)

Issue:
[BUG] `object.Datastore.NewURL()` uses the wrong field for dsName query
**Describe the bug**
The `Datastore.NewURL()` function composes a `url.URL` to get a file via the `/folder` endpoint. This endpoint requires the `dsName` query parameter to be set to the name of the datastore. In the current implementation the function `d.Name()` is used to fill this parameter. However, this function is embedded from `Common.Name()` and does not return the real name of the datastore. Instead it returns its inventory path or "" most of the time.

**To Reproduce**
Use the `Datastore.NewURL()` function to generate a download URL

**Expected behavior**
Instead of `d.Name()` the function should use `d.ObjectName(context.Context)` which receives the real name of the datastore from the VSX/ESX itself.

**Affected version**
<= v.0.32

**Additional context**
The solution poses a problem, since the  `d.ObjectName(context.Context)` call does require a context for the request scope. This is very unfortunate, but the current implementation is pretty useless anyways (and might not be used, otherwise this must have come up earlier.)

I would prepare a PR to fix this, if my proposed fix is okay for you (or another solution of course)
Cheers


Pull request description:
api: add object.Datastore.FindInventoryPath
Helper to set Datastore InventoryPath and DatacenterPath, needed by NewURL() to compose an upload/download endpoint URL

Fixes #3267


Patch:
diff --git a/object/datastore.go b/object/datastore.go
index b3b7f0bb..991ad4f8 100644
--- a/object/datastore.go
+++ b/object/datastore.go
@@ -68,6 +68,31 @@ func NewDatastore(c *vim25.Client, ref types.ManagedObjectReference) *Datastore
 	}
 }
 
+// FindInventoryPath sets InventoryPath and DatacenterPath,
+// needed by NewURL() to compose an upload/download endpoint URL
+func (d *Datastore) FindInventoryPath(ctx context.Context) error {
+	entities, err := mo.Ancestors(ctx, d.c, d.c.ServiceContent.PropertyCollector, d.r)
+	if err != nil {
+		return err
+	}
+
+	val := "/"
+
+	for _, entity := range entities {
+		if entity.Parent == nil {
+			continue // root folder
+		}
+		val = path.Join(val, entity.Name)
+		if entity.Self.Type == "Datacenter" {
+			d.DatacenterPath = val
+		}
+	}
+
+	d.InventoryPath = val
+
+	return nil
+}
+
 func (d Datastore) Path(path string) string {
 	var p DatastorePath
 	if p.FromString(path) {
@@ -101,11 +126,6 @@ func (d Datastore) NewURL(path string) *url.URL {
 	}
 }
 
-// URL is deprecated, use NewURL instead.
-func (d Datastore) URL(ctx context.Context, dc *Datacenter, path string) (*url.URL, error) {
-	return d.NewURL(path), nil
-}
-
 func (d Datastore) Browser(ctx context.Context) (*HostDatastoreBrowser, error) {
 	var do mo.Datastore
 
@@ -186,6 +206,10 @@ func (d Datastore) HostContext(ctx context.Context, host *HostSystem) context.Co
 // that can be used along with the ticket cookie to access the given path.  An host is chosen at random unless the
 // the given Context was created with a specific host via the HostContext method.
 func (d Datastore) ServiceTicket(ctx context.Context, path string, method string) (*url.URL, *http.Cookie, error) {
+	if d.InventoryPath == "" {
+		_ = d.FindInventoryPath(ctx)
+	}
+
 	u := d.NewURL(path)
 
 	host, ok := ctx.Value(datastoreServiceTicketHostKey{}).(*HostSystem)


Test patch:
diff --git a/object/datastore_test.go b/object/datastore_test.go
index 5b4f790f..e43320c9 100644
--- a/object/datastore_test.go
+++ b/object/datastore_test.go
@@ -14,7 +14,85 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package object
+package object_test
+
+import (
+	"context"
+	"strings"
+	"testing"
+
+	"github.com/vmware/govmomi/object"
+	"github.com/vmware/govmomi/simulator"
+	"github.com/vmware/govmomi/vim25"
+	"github.com/vmware/govmomi/vim25/soap"
+)
 
 // Datastore should implement the Reference interface.
-var _ Reference = Datastore{}
+var _ object.Reference = object.Datastore{}
+
+func TestDatastoreFindInventoryPath(t *testing.T) {
+	model := simulator.VPX()
+	model.Datacenter = 2
+	model.Datastore = 2
+
+	simulator.Test(func(ctx context.Context, c *vim25.Client) {
+		refs := simulator.Map.All("Datastore")
+
+		for _, obj := range refs {
+			ds := object.NewDatastore(c, obj.Reference())
+
+			upload := func() error {
+				p := soap.DefaultUpload
+				r := strings.NewReader(ds.Name())
+				p.ContentLength = r.Size()
+				return ds.Upload(ctx, r, "name.txt", &p)
+
+			}
+
+			// with InventoryPath not set
+			if err := upload(); err != nil {
+				t.Error(err)
+			}
+
+			if ds.InventoryPath != "" {
+				t.Error("InventoryPath should still be empty")
+			}
+
+			// Set InventoryPath and DatacenterPath
+			err := ds.FindInventoryPath(ctx)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			if !strings.HasPrefix(ds.InventoryPath, ds.DatacenterPath) {
+				t.Errorf("InventoryPath=%s, DatacenterPath=%s", ds.InventoryPath, ds.DatacenterPath)
+			}
+
+			// with InventoryPath set
+			if err := upload(); err != nil {
+				t.Error(err)
+			}
+
+			tests := []struct {
+				path, kind string
+			}{
+				{ds.InventoryPath, "Datastore"},
+				{ds.DatacenterPath, "Datacenter"},
+			}
+
+			for _, test := range tests {
+				ref, err := object.NewSearchIndex(c).FindByInventoryPath(ctx, test.path)
+				if err != nil {
+					t.Fatal(err)
+				}
+
+				if ref == nil {
+					t.Fatalf("failed to find %s", test.path)
+				}
+				if ref.Reference().Type != test.kind {
+					t.Errorf("%s is not a %s", test.path, test.kind)
+				}
+			}
+		}
+	}, model)
+}