V2.12.0 regression fix for org.GetCatalogByName and org.GetCatalogById

govcd/catalog_test.go

@@ -597,3 +597,116 @@ func (vcd *TestVCD) TestGetVappTemplateByHref(check *C) {
 	check.Assert(vappTemplate.VAppTemplate.Type, Equals, types.MimeVAppTemplate)
 	check.Assert(vappTemplate.VAppTemplate.Name, Equals, catalogItem.CatalogItem.Name)
 }
+
+// Test_GetCatalogByNameSharedCatalog creates a separate Org and VDC just to create Catalog and share it with main Org
+// One should be able to find shared catalogs from different Organizations
+func (vcd *TestVCD) Test_GetCatalogByNameSharedCatalog(check *C) {
+	fmt.Printf("Running: %s\n", check.TestName())
+
+	newOrg1, vdc, sharedCatalog := createSharedCatalogInNewOrg(vcd, check, check.TestName())
+
+	// Try to find the catalog inside Org which owns it - newOrg1
+	catalogByName, err := newOrg1.GetCatalogByName(sharedCatalog.Catalog.Name, true)
+	check.Assert(err, IsNil)
+	check.Assert(catalogByName.Catalog.Name, Equals, sharedCatalog.Catalog.Name)
+
+	// Try to find the catalog in another Org with which this catalog is shared (vcd.Org)
+	sharedCatalogByName, err := vcd.org.GetCatalogByName(sharedCatalog.Catalog.Name, false)
+	check.Assert(err, IsNil)
+	check.Assert(sharedCatalogByName.Catalog.Name, Equals, sharedCatalog.Catalog.Name)
+
+	cleanupCatalogOrgVdc(check, err, sharedCatalog, vdc, vcd, newOrg1)
+}
+
+// Test_GetCatalogByIdSharedCatalog creates a separate Org and VDC just to create Catalog and share it with main Org
+// One should be able to find shared catalogs from different Organizations
+func (vcd *TestVCD) Test_GetCatalogByIdSharedCatalog(check *C) {
+	fmt.Printf("Running: %s\n", check.TestName())
+
+	newOrg1, vdc, sharedCatalog := createSharedCatalogInNewOrg(vcd, check, check.TestName())
+
+	// Try to find the sharedCatalog inside Org which owns it - newOrg1
+	catalogById, err := newOrg1.GetCatalogById(sharedCatalog.Catalog.ID, true)
+	check.Assert(err, IsNil)
+	check.Assert(catalogById.Catalog.Name, Equals, sharedCatalog.Catalog.Name)
+
+	// Try to find the sharedCatalog in another Org with which this sharedCatalog is shared (vcd.Org)
+	sharedCatalogById, err := vcd.org.GetCatalogById(sharedCatalog.Catalog.ID, false)
+	check.Assert(err, IsNil)
+	check.Assert(sharedCatalogById.Catalog.Name, Equals, sharedCatalog.Catalog.Name)
+
+	cleanupCatalogOrgVdc(check, err, sharedCatalog, vdc, vcd, newOrg1)
+}
+
+// Test_GetCatalogByNamePrefersLocal tests that local catalog (in the same Org) is prioritised against shared catalogs
+// in other Orgs. It does so by creating another Org with shared Catalog named just like the one in testing catalog
+func (vcd *TestVCD) Test_GetCatalogByNamePrefersLocal(check *C) {
+	fmt.Printf("Running: %s\n", check.TestName())
+
+	// Create a catalog  in new org with exactly the same name as in vcd.Org
+	newOrg1, vdc, sharedCatalog := createSharedCatalogInNewOrg(vcd, check, vcd.config.VCD.Catalog.Name)
+
+	// Make sure that the Owner Org HREF is the local one for vcd.Org catalog named vcd.config.VCD.Catalog.Name
+	catalogByNameInTestOrg, err := vcd.org.GetCatalogByName(vcd.config.VCD.Catalog.Name, true)
+	check.Assert(err, IsNil)
+	check.Assert(catalogByNameInTestOrg.parent.orgName(), Equals, vcd.org.Org.Name)
+
+	// Make sure that the Owner Org HREF is the local one for vcd.Org catalog named vcd.config.VCD.Catalog.Name
+	catalogByNameInNewOrg, err := newOrg1.GetCatalogByName(vcd.config.VCD.Catalog.Name, true)
+	check.Assert(err, IsNil)
+	check.Assert(catalogByNameInNewOrg.parent.orgName(), Equals, newOrg1.Org.Name)
+
+	cleanupCatalogOrgVdc(check, err, sharedCatalog, vdc, vcd, newOrg1)
+}
+
+func createSharedCatalogInNewOrg(vcd *TestVCD, check *C, newCatalogName string) (*Org, *Vdc, Catalog) {
+	newOrgName1 := spawnTestOrg(vcd, check, "org")
+
+	newOrg1, err := vcd.client.GetOrgByName(newOrgName1)
+	check.Assert(err, IsNil)
+
+	// Spawn a VDC inside newly created Org so that there is storage to create new catalog
+	vdc := spawnTestVdc(vcd, check, newOrgName1)
+
+	catalog, err := newOrg1.CreateCatalog(newCatalogName, "Catalog for testing")
+	check.Assert(err, IsNil)
+	AddToCleanupList(newCatalogName, "catalog", newOrgName1, check.TestName())
+
+	// Share new Catalog in newOrgName1 with default test Org vcd.Org
+	readOnly := "ReadOnly"
+	accessControl := &types.ControlAccessParams{
+		IsSharedToEveryone:  false,
+		EveryoneAccessLevel: &readOnly,
+		AccessSettings: &types.AccessSettingList{
+			AccessSetting: []*types.AccessSetting{&types.AccessSetting{
+				Subject: &types.LocalSubject{
+					HREF: vcd.org.Org.HREF,
+					Name: vcd.org.Org.Name,
+					Type: types.MimeOrg,
+				},
+				AccessLevel: "ReadOnly",
+			}},
+		},
+	}
+	err = catalog.SetAccessControl(accessControl, false)
+	check.Assert(err, IsNil)
+
+	return newOrg1, vdc, catalog
+}
+
+func cleanupCatalogOrgVdc(check *C, err error, sharedCatalog Catalog, vdc *Vdc, vcd *TestVCD, newOrg1 *Org) {
+	// Cleanup catalog, vdc and org
+	err = sharedCatalog.Delete(true, true)
+	check.Assert(err, IsNil)
+
+	// There are cases where it just takes a a few seconds after catalog deletion when one can delete VDC
+	time.Sleep(2 * time.Second)
+
+	err = vdc.DeleteWait(true, true)
+	check.Assert(err, IsNil)
+
+	adminOrg, err := vcd.client.GetAdminOrgByName(newOrg1.Org.Name)
+	check.Assert(err, IsNil)
+	err = adminOrg.Delete(true, true)
+	check.Assert(err, IsNil)
+}

govcd/common_test.go

@@ -793,6 +793,20 @@ func spawnTestVdc(vcd *TestVCD, check *C, adminOrgName string) *Vdc {
 	return vdc
 }
 
+// spawnTestOrg spawns an Org to be used in tests
+func spawnTestOrg(vcd *TestVCD, check *C, nameSuffix string) string {
+	newOrg, err := vcd.client.GetAdminOrgByName(vcd.config.VCD.Org)
+	check.Assert(err, IsNil)
+	newOrgName := check.TestName() + "-" + nameSuffix
+	task, err := CreateOrg(vcd.client, newOrgName, newOrgName, newOrgName, newOrg.AdminOrg.OrgSettings, true)
+	check.Assert(err, IsNil)
+	err = task.WaitTaskCompletion()
+	check.Assert(err, IsNil)
+	AddToCleanupList(newOrgName, "org", "", check.TestName())
+
+	return newOrgName
+}
+
 func getVdcProviderVdcHref(vcd *TestVCD, check *C) string {
 	results, err := vcd.client.QueryWithNotEncodedParams(nil, map[string]string{
 		"type":   "providerVdc",

govcd/org.go

@@ -415,9 +415,11 @@ func (org *Org) queryOrgVdcById(vdcId string) (*types.QueryResultOrgVdcRecordTyp
 // queryCatalogByName returns a single CatalogRecord
 func (org *Org) queryCatalogByName(catalogName string) (*types.CatalogRecord, error) {
 	filterMap := map[string]string{
-		"org":     org.Org.HREF, // Org ID is not allowed for non System
-		"orgName": org.Org.Name,
-		"name":    catalogName,
+		// Not injecting `org` or `orgName` here because shared catalogs may also appear here and they would have org
+		// parent Org
+		//"org":     org.Org.HREF,
+		//"orgName": org.Org.Name,
+		"name": catalogName,
 	}
 	allCatalogs, err := queryCatalogList(org.client, filterMap)
 	if err != nil {
@@ -428,19 +430,44 @@ func (org *Org) queryCatalogByName(catalogName string) (*types.CatalogRecord, er
 		return nil, ErrorEntityNotFound
 	}
 
+	// To conform with this API standard it would be best to return an error if more than 1 item is found, but because
+	// previous method of getting Catalog by Name returned the first result we are doing the same here
+	//if len(allCatalogs) > 1 {
+	//	return nil, fmt.Errorf("found more than 1 Catalog with Name '%s'", catalogName)
+	//}
+
+	var localCatalog *types.CatalogRecord
+	// if multiple results are found - return the one defined in `org` (local)
 	if len(allCatalogs) > 1 {
-		return nil, fmt.Errorf("found more than 1 VDC with Name '%s'", catalogName)
+		util.Logger.Printf("[DEBUG] org.queryCatalogByName found %d Catalogs by name '%s'", len(allCatalogs), catalogName)
+		for _, catalog := range allCatalogs {
+			util.Logger.Printf("[DEBUG] org.queryCatalogByName found a Catalog by name '%s' in Org '%s'", catalogName, catalog.OrgName)
+			if catalog.OrgName == org.Org.Name {
+				util.Logger.Printf("[DEBUG] org.queryCatalogByName Catalog '%s' is local for Org '%s'. Prioritising it",
+					catalogName, org.Org.Name)
+				// Not interrupting the loop here to still dump all results to logs
+				localCatalog = catalog
+			}
+		}
 	}
 
+	// local catalog was found - return it
+	if localCatalog != nil {
+		return localCatalog, nil
+	}
+
+	// If only one catalog is found or multiple catalogs with no local ones - return the first one
 	return allCatalogs[0], nil
 }
 
 // queryCatalogById returns a single QueryResultOrgVdcRecordType
 func (org *Org) queryCatalogById(catalogId string) (*types.CatalogRecord, error) {
 	filterMap := map[string]string{
-		"org":     org.Org.HREF,
-		"orgName": org.Org.Name,
-		"id":      catalogId,
+		// Not injecting `org` or `orgName` here because shared catalogs may also appear here and they would have org
+		// parent Org
+		//"org":     org.Org.HREF,
+		//"orgName": org.Org.Name,
+		"id": catalogId,
 	}
 	allCatalogs, err := queryCatalogList(org.client, filterMap)
 

govcd/org_test.go

@@ -1107,17 +1107,3 @@ func validateQueryOrgVdcResults(vcd *TestVCD, check *C, name, orgName string, ex
 		fmt.Println()
 	}
 }
-
-// spawnTestOrg spawns an Org to be used in tests
-func spawnTestOrg(vcd *TestVCD, check *C, nameSuffix string) string {
-	newOrg, err := vcd.client.GetAdminOrgByName(vcd.config.VCD.Org)
-	check.Assert(err, IsNil)
-	newOrgName := check.TestName() + "-" + nameSuffix
-	task, err := CreateOrg(vcd.client, newOrgName, newOrgName, newOrgName, newOrg.AdminOrg.OrgSettings, true)
-	check.Assert(err, IsNil)
-	err = task.WaitTaskCompletion()
-	check.Assert(err, IsNil)
-	AddToCleanupList(newOrgName, "org", "", check.TestName())
-
-	return newOrgName
-}