store: Improve conflicting root error message

v1/bundle/store.go

@@ -12,6 +12,8 @@ import (
 	"fmt"
 	"maps"
 	"path/filepath"
+	"slices"
+	"sort"
 	"strings"
 	"sync"
 
@@ -1059,32 +1061,40 @@ func lookup(path storage.Path, data map[string]any) (any, bool) {
 	return value, ok
 }
 
-func hasRootsOverlap(ctx context.Context, store storage.Store, txn storage.Transaction, bundles map[string]*Bundle) error {
-	collisions := map[string][]string{}
-	allBundles, err := ReadBundleNamesFromStore(ctx, store, txn)
+func hasRootsOverlap(ctx context.Context, store storage.Store, txn storage.Transaction, newBundles map[string]*Bundle) error {
+	storeBundles, err := ReadBundleNamesFromStore(ctx, store, txn)
 	if suppressNotFound(err) != nil {
 		return err
 	}
 
 	allRoots := map[string][]string{}
+	bundlesWithEmptyRoots := map[string]bool{}
 
 	// Build a map of roots for existing bundles already in the system
-	for _, name := range allBundles {
+	for _, name := range storeBundles {
 		roots, err := ReadBundleRootsFromStore(ctx, store, txn, name)
 		if suppressNotFound(err) != nil {
 			return err
 		}
 		allRoots[name] = roots
+		if slices.Contains(roots, "") {
+			bundlesWithEmptyRoots[name] = true
+		}
 	}
 
 	// Add in any bundles that are being activated, overwrite existing roots
 	// with new ones where bundles are in both groups.
-	for name, bundle := range bundles {
+	for name, bundle := range newBundles {
 		allRoots[name] = *bundle.Manifest.Roots
+		if slices.Contains(*bundle.Manifest.Roots, "") {
+			bundlesWithEmptyRoots[name] = true
+		}
 	}
 
 	// Now check for each new bundle if it conflicts with any of the others
-	for name, bundle := range bundles {
+	collidingBundles := map[string]bool{}
+	conflictSet := map[string]bool{}
+	for name, bundle := range newBundles {
 		for otherBundle, otherRoots := range allRoots {
 			if name == otherBundle {
 				// Skip the current bundle being checked
@@ -1094,22 +1104,41 @@ func hasRootsOverlap(ctx context.Context, store storage.Store, txn storage.Trans
 			// Compare the "new" roots with other existing (or a different bundles new roots)
 			for _, newRoot := range *bundle.Manifest.Roots {
 				for _, otherRoot := range otherRoots {
-					if RootPathsOverlap(newRoot, otherRoot) {
-						collisions[otherBundle] = append(collisions[otherBundle], newRoot)
+					if !RootPathsOverlap(newRoot, otherRoot) {
+						continue
+					}
+
+					collidingBundles[name] = true
+					collidingBundles[otherBundle] = true
+
+					// Different message required if the roots are same
+					if newRoot == otherRoot {
+						conflictSet[fmt.Sprintf("root %s is in multiple bundles", newRoot)] = true
+					} else {
+						paths := []string{newRoot, otherRoot}
+						sort.Strings(paths)
+						conflictSet[fmt.Sprintf("%s overlaps %s", paths[0], paths[1])] = true
 					}
 				}
 			}
 		}
 	}
 
-	if len(collisions) > 0 {
-		var bundleNames []string
-		for name := range collisions {
-			bundleNames = append(bundleNames, name)
-		}
-		return fmt.Errorf("detected overlapping roots in bundle manifest with: %s", bundleNames)
+	if len(collidingBundles) == 0 {
+		return nil
 	}
-	return nil
+
+	bundleNames := strings.Join(util.KeysSorted(collidingBundles), ", ")
+
+	if len(bundlesWithEmptyRoots) > 0 {
+		return fmt.Errorf(
+			"bundles [%s] have overlapping roots and cannot be activated simultaneously because bundle(s) [%s] specify empty root paths ('') which overlap with any other bundle root",
+			bundleNames,
+			strings.Join(util.KeysSorted(bundlesWithEmptyRoots), ", "),
+		)
+	}
+
+	return fmt.Errorf("detected overlapping roots in manifests for these bundles: [%s] (%s)", bundleNames, strings.Join(util.KeysSorted(conflictSet), ", "))
 }
 
 func applyPatches(ctx context.Context, store storage.Store, txn storage.Transaction, patches []PatchOperation) error {

v1/bundle/store_test.go

@@ -6621,52 +6621,78 @@ func TestHasRootsOverlap(t *testing.T) {
 	ctx := context.Background()
 
 	cases := []struct {
-		note        string
-		storeRoots  map[string]*[]string
-		bundleRoots map[string]*[]string
-		overlaps    bool
+		note           string
+		storeRoots     map[string]*[]string
+		newBundleRoots map[string]*[]string
+		expectedError  string
 	}{
 		{
-			note:        "no overlap with existing roots",
-			storeRoots:  map[string]*[]string{"bundle1": {"a", "b"}},
-			bundleRoots: map[string]*[]string{"bundle2": {"c"}},
-			overlaps:    false,
+			note:           "no overlap between store and new bundles",
+			storeRoots:     map[string]*[]string{"bundle1": {"a", "b"}},
+			newBundleRoots: map[string]*[]string{"bundle2": {"c"}},
+		},
+		{
+			note:           "no overlap between store and multiple new bundles",
+			storeRoots:     map[string]*[]string{"bundle1": {"a", "b"}},
+			newBundleRoots: map[string]*[]string{"bundle2": {"c"}, "bundle3": {"d"}},
+		},
+		{
+			note:           "no overlap with empty store",
+			storeRoots:     map[string]*[]string{},
+			newBundleRoots: map[string]*[]string{"bundle1": {"a", "b"}},
+		},
+		{
+			note:           "no overlap between multiple new bundles with empty store",
+			storeRoots:     map[string]*[]string{},
+			newBundleRoots: map[string]*[]string{"bundle1": {"a", "b"}, "bundle2": {"c"}},
+		},
+		{
+			note:           "overlap between multiple new bundles with empty store",
+			storeRoots:     map[string]*[]string{},
+			newBundleRoots: map[string]*[]string{"bundle1": {"a", "b"}, "bundle2": {"a", "c"}},
+			expectedError:  "detected overlapping roots in manifests for these bundles: [bundle1, bundle2] (root a is in multiple bundles)",
 		},
 		{
-			note:        "no overlap with existing roots multiple bundles",
-			storeRoots:  map[string]*[]string{"bundle1": {"a", "b"}},
-			bundleRoots: map[string]*[]string{"bundle2": {"c"}, "bundle3": {"d"}},
-			overlaps:    false,
+			note:           "overlap between store and new bundle",
+			storeRoots:     map[string]*[]string{"bundle1": {"a", "b"}},
+			newBundleRoots: map[string]*[]string{"bundle2": {"c", "a"}},
+			expectedError:  "detected overlapping roots in manifests for these bundles: [bundle1, bundle2] (root a is in multiple bundles)",
 		},
 		{
-			note:        "no overlap no existing roots",
-			storeRoots:  map[string]*[]string{},
-			bundleRoots: map[string]*[]string{"bundle1": {"a", "b"}},
-			overlaps:    false,
+			note:           "overlap between store and multiple new bundles",
+			storeRoots:     map[string]*[]string{"bundle1": {"a", "b"}},
+			newBundleRoots: map[string]*[]string{"bundle2": {"c", "a"}, "bundle3": {"a"}},
+			expectedError:  "detected overlapping roots in manifests for these bundles: [bundle1, bundle2, bundle3] (root a is in multiple bundles)",
 		},
 		{
-			note:        "no overlap without existing roots multiple bundles",
-			storeRoots:  map[string]*[]string{},
-			bundleRoots: map[string]*[]string{"bundle1": {"a", "b"}, "bundle2": {"c"}},
-			overlaps:    false,
+			note:           "overlap between store bundle and new empty root bundle",
+			storeRoots:     map[string]*[]string{"bundle1": {"a", "b"}},
+			newBundleRoots: map[string]*[]string{"bundle2": {""}},
+			expectedError:  "bundles [bundle1, bundle2] have overlapping roots and cannot be activated simultaneously because bundle(s) [bundle2] specify empty root paths ('') which overlap with any other bundle root",
 		},
 		{
-			note:        "overlap without existing roots multiple bundles",
-			storeRoots:  map[string]*[]string{},
-			bundleRoots: map[string]*[]string{"bundle1": {"a", "b"}, "bundle2": {"a", "c"}},
-			overlaps:    true,
+			note:           "overlap between multiple new empty root bundles",
+			storeRoots:     map[string]*[]string{},
+			newBundleRoots: map[string]*[]string{"bundle1": {""}, "bundle2": {""}},
+			expectedError:  "bundles [bundle1, bundle2] have overlapping roots and cannot be activated simultaneously because bundle(s) [bundle1, bundle2] specify empty root paths ('') which overlap with any other bundle root",
 		},
 		{
-			note:        "overlap with existing roots",
-			storeRoots:  map[string]*[]string{"bundle1": {"a", "b"}},
-			bundleRoots: map[string]*[]string{"bundle2": {"c", "a"}},
-			overlaps:    true,
+			note:           "overlap between new empty root and new regular root bundles",
+			storeRoots:     map[string]*[]string{},
+			newBundleRoots: map[string]*[]string{"bundle1": {"a"}, "bundle2": {""}},
+			expectedError:  "bundles [bundle1, bundle2] have overlapping roots and cannot be activated simultaneously because bundle(s) [bundle2] specify empty root paths ('') which overlap with any other bundle root",
 		},
 		{
-			note:        "overlap with existing roots multiple bundles",
-			storeRoots:  map[string]*[]string{"bundle1": {"a", "b"}},
-			bundleRoots: map[string]*[]string{"bundle2": {"c", "a"}, "bundle3": {"a"}},
-			overlaps:    true,
+			note:           "overlap between nested paths",
+			storeRoots:     map[string]*[]string{},
+			newBundleRoots: map[string]*[]string{"bundle1": {"a"}, "bundle2": {"a/b"}},
+			expectedError:  "detected overlapping roots in manifests for these bundles: [bundle1, bundle2] (a overlaps a/b)",
+		},
+		{
+			note:           "overlap between store nested path and new bundle path",
+			storeRoots:     map[string]*[]string{"bundle1": {"a/b"}},
+			newBundleRoots: map[string]*[]string{"bundle2": {"a"}},
+			expectedError:  "detected overlapping roots in manifests for these bundles: [bundle1, bundle2] (a overlaps a/b)",
 		},
 	}
 
@@ -6683,7 +6709,7 @@ func TestHasRootsOverlap(t *testing.T) {
 			}
 
 			bundles := map[string]*Bundle{}
-			for name, roots := range tc.bundleRoots {
+			for name, roots := range tc.newBundleRoots {
 				bundles[name] = &Bundle{
 					Manifest: Manifest{
 						Roots: roots,
@@ -6692,10 +6718,15 @@ func TestHasRootsOverlap(t *testing.T) {
 			}
 
 			err := hasRootsOverlap(ctx, mockStore, txn, bundles)
-			if !tc.overlaps && err != nil {
-				t.Fatalf("unepected error: %s", err)
-			} else if tc.overlaps && (err == nil || !strings.Contains(err.Error(), "detected overlapping roots in bundle manifest")) {
-				t.Fatalf("expected overlapping roots error, got: %s", err)
+			if tc.expectedError != "" {
+				if err == nil {
+					t.Fatalf("expected error %q, got nil", tc.expectedError)
+				}
+				if err.Error() != tc.expectedError {
+					t.Fatalf("expected error message %q, got %q", tc.expectedError, err.Error())
+				}
+			} else if err != nil {
+				t.Fatalf("unexpected error: %s", err)
 			}
 
 			err = mockStore.Commit(ctx, txn)

v1/plugins/bundle/plugin_test.go

@@ -3282,7 +3282,7 @@ func ensureBundleOverlapStatus(t *testing.T, p *Plugin, bundleNames []string, ex
 			t.Fatalf("expected bundle %s to be in an error state", name)
 		} else if !expectedErrs[i] && hasErr {
 			t.Fatalf("unexpected error state for bundle %s", name)
-		} else if hasErr && expectedErrs[i] && !strings.Contains(p.status[name].Message, "detected overlapping roots") {
+		} else if hasErr && expectedErrs[i] && !strings.Contains(p.status[name].Message, "overlapping roots") {
 			t.Fatalf("expected bundle overlap error for bundle %s, got: %s", name, p.status[name].Message)
 		}
 	}
@@ -7209,8 +7209,8 @@ result := true`,
 	if status.Code != errCode {
 		t.Fatalf("Expected status code to be %s, found %s", errCode, status.Code)
 	}
-	if !strings.Contains(status.Message, "detected overlapping") {
-		t.Fatalf(`Expected status message to contain "detected overlapping roots", found %s`, status.Message)
+	if !strings.Contains(status.Message, "specify empty root paths") {
+		t.Fatalf(`Expected status message to contain "specify empty root paths", found %s`, status.Message)
 	}
 }