Qualify library names in app.yaml to avoid cross-registry conflicts

Closes #617
Signed-off-by: Oren Shomron <shomron@gmail.com>

pkg/actions/pkg_remove.go

@@ -71,12 +71,7 @@ func RunPkgRemove(m map[string]interface{}) error {
 
 // Run removes packages
 func (pr *PkgRemove) Run() error {
-	desc, err := pkg.Parse(pr.pkgName)
-	if err != nil {
-		return err
-	}
-
-	oldCfg, err := pr.libUpdateFn(desc.Name, pr.envName, nil)
+	oldCfg, err := pr.libUpdateFn(pr.pkgName, pr.envName, nil)
 	if err != nil {
 		return err
 	}

pkg/app/base_app.go

@@ -233,12 +233,32 @@ func (ba *baseApp) AddRegistry(newReg *RegistryConfig, isOverride bool) error {
 	return ba.save()
 }
 
+// libKeyByDesc scans for a library in the referenced LibraryConfigs map.
+// Matching is by registry/name or name if registry is not provided.
+// Versions are ignored.
+// Complexity: O(n)
+// Returns key of matching library, or empty string if no match is found.
+func libKeyByDesc(desc LibraryConfig, libs LibraryConfigs) string {
+	for k, v := range libs {
+		if desc.Name != v.Name {
+			continue
+		}
+		if desc.Registry == "" {
+			return k
+		}
+		if desc.Registry == v.Registry {
+			return k
+		}
+	}
+	return ""
+}
+
 // UpdateLib adds or updates a library reference.
 // env is optional - if provided the reference is scoped under the environment,
 // otherwise it is globally scoped.
 // If spec if nil, the library reference will be removed.
 // Returns the previous reference for the named library, if one existed.
-func (ba *baseApp) UpdateLib(name string, env string, libSpec *LibraryConfig) (*LibraryConfig, error) {
+func (ba *baseApp) UpdateLib(id string, env string, libSpec *LibraryConfig) (*LibraryConfig, error) {
 	if err := ba.load(); err != nil {
 		return nil, errors.Wrap(err, "load configuration")
 	}
@@ -247,8 +267,13 @@ func (ba *baseApp) UpdateLib(name string, env string, libSpec *LibraryConfig) (*
 		return nil, errors.Errorf("invalid app - configuration is nil")
 	}
 
-	if libSpec != nil && libSpec.Name != name {
-		return nil, errors.Errorf("library name mismatch: %v vs %v", libSpec.Name, name)
+	desc, err := parseLibraryConfig(id)
+	if err != nil {
+		return nil, errors.Wrapf(err, "parsing id: %s", id)
+	}
+
+	if libSpec != nil && libSpec.Name != desc.Name {
+		return nil, errors.Errorf("library name mismatch: %v vs %v", libSpec.Name, desc.Name)
 	}
 
 	// TODO support app overrides
@@ -256,18 +281,10 @@ func (ba *baseApp) UpdateLib(name string, env string, libSpec *LibraryConfig) (*
 	var oldSpec *LibraryConfig
 	switch env {
 	case "":
-		if ba.config.Libraries == nil {
-			ba.config.Libraries = LibraryConfigs{}
-		}
 		// Globally scoped
-		oldSpec = ba.config.Libraries[name]
-		if libSpec == nil {
-			if oldSpec == nil {
-				return nil, errors.Errorf("package not found: %s", name)
-			}
-			delete(ba.config.Libraries, name)
-		} else {
-			ba.config.Libraries[name] = libSpec
+		oldSpec, err = updateLibInScope(desc, libSpec, &ba.config.Libraries)
+		if err != nil {
+			return nil, errors.Wrap(err, "updating library in global scope")
 		}
 	default:
 		// Scoped by environment
@@ -276,19 +293,9 @@ func (ba *baseApp) UpdateLib(name string, env string, libSpec *LibraryConfig) (*
 			return nil, errors.Errorf("invalid environment: %v", env)
 		}
 
-		if e.Libraries == nil {
-			// We may want to move this into EnvrionmentConfig unmarshaling code.
-			e.Libraries = LibraryConfigs{}
-		}
-		oldSpec = e.Libraries[name]
-
-		if libSpec == nil {
-			if oldSpec == nil {
-				return nil, errors.Errorf("package %s not found in environment: %s", name, env)
-			}
-			delete(e.Libraries, name)
-		} else {
-			e.Libraries[name] = libSpec
+		oldSpec, err = updateLibInScope(desc, libSpec, &e.Libraries)
+		if err != nil {
+			return nil, errors.Wrap(err, "updating library in environment scope")
 		}
 
 		if err := ba.config.UpdateEnvironmentConfig(env, e); err != nil {
@@ -299,6 +306,29 @@ func (ba *baseApp) UpdateLib(name string, env string, libSpec *LibraryConfig) (*
 	return oldSpec, ba.save()
 }
 
+func updateLibInScope(desc LibraryConfig, libSpec *LibraryConfig, scope *LibraryConfigs) (*LibraryConfig, error) {
+	if scope == nil {
+		return nil, errors.New("nil scope")
+	}
+	if *scope == nil {
+		*scope = LibraryConfigs{}
+	}
+
+	oldSpecKey := libKeyByDesc(desc, *scope)
+	oldSpec := (*scope)[oldSpecKey]
+	if libSpec == nil {
+		if oldSpecKey == "" || oldSpec == nil {
+			return nil, errors.Errorf("package not found: %v", desc)
+		}
+		delete(*scope, oldSpecKey)
+	} else {
+		newKey := qualifyLibName(libSpec.Registry, libSpec.Name)
+		(*scope)[newKey] = libSpec
+	}
+
+	return oldSpec, nil
+}
+
 // UpdateRegistry updates a registry spec and persists in app[.override].yaml
 func (ba *baseApp) UpdateRegistry(spec *RegistryConfig) error {
 	if err := ba.load(); err != nil {

pkg/app/schema.go

@@ -19,6 +19,8 @@ import (
 	"encoding/json"
 	"fmt"
 	"path/filepath"
+	"regexp"
+	"strings"
 
 	"github.com/blang/semver"
 	"github.com/ghodss/yaml"
@@ -362,6 +364,10 @@ type GitVersionSpec struct {
 // LibraryConfigs is a mapping of a library configurations by name.
 type LibraryConfigs map[string]*LibraryConfig
 
+// libraryConfigs is an alias that allows us to leverage default JSON encoding
+// in our custom MarshalJSON handler without triggering infinite recursion.
+type libraryConfigs LibraryConfigs
+
 // UnmarshalJSON implements the json.Unmarshaler interface.
 // We implement some compatibility conversions.
 func (l *LibraryConfigs) UnmarshalJSON(b []byte) error {
@@ -377,14 +383,37 @@ func (l *LibraryConfigs) UnmarshalJSON(b []byte) error {
 			continue
 		}
 
-		v.Name = k
-		result[k] = v
+		name := libName(k)
+		qualifiedName := qualifyLibName(v.Registry, name)
+
+		v.Name = name
+		result[qualifiedName] = v
 	}
 
 	*l = result
 	return nil
 }
 
+// MarshalJSON implements the json.Unmarshaler interface.
+// We implement some compatibility conversions.
+func (l *LibraryConfigs) MarshalJSON() ([]byte, error) {
+	lc := libraryConfigs{}
+
+	for k, v := range *l {
+		if v == nil {
+			continue
+		}
+
+		name := libName(k)
+		qualifiedName := qualifyLibName(v.Registry, name)
+
+		v.Name = name
+		lc[qualifiedName] = v
+	}
+
+	return json.Marshal(lc)
+}
+
 // libraryConfig is an alias that allows us to leverage default JSON decoding
 // in our custom UnmarshalJSON handler without triggering infinite recursion.
 type libraryConfig LibraryConfig
@@ -579,3 +608,53 @@ func (l LibraryConfig) String() string {
 		return fmt.Sprintf("%s/%s@%s", l.Registry, l.Name, l.Version)
 	}
 }
+
+func libName(name string) string {
+	segments := strings.Split(name, "/")
+	if len(segments) < 2 {
+		return name
+	}
+	return segments[len(segments)-1]
+}
+
+func qualifyLibName(registry, name string) string {
+	if registry == "" {
+		return name
+	}
+
+	return fmt.Sprintf("%s/%s", registry, name)
+}
+
+var (
+	errInvalidSpec = fmt.Errorf("package name should be in the form `<registry>/<library>@<version>`")
+	reDescriptor   = regexp.MustCompile(`^([A-Za-z0-9\-]+)(\/[^@]+)?(@[^@]+)?$`)
+)
+
+// parseLibraryConfig parses a library identifier into its components
+// <registry>/<name>@<version>
+// See pkg.Parse().
+func parseLibraryConfig(id string) (LibraryConfig, error) {
+	var registry, name, version string
+
+	matches := reDescriptor.FindStringSubmatch(id)
+	if len(matches) == 0 {
+		return LibraryConfig{}, errInvalidSpec
+	}
+
+	if matches[2] == "" {
+		// No registry
+		name = strings.TrimPrefix(matches[1], "/")
+	} else {
+		// Registry and name
+		registry = matches[1]
+		name = strings.TrimPrefix(matches[2], "/")
+	}
+
+	version = strings.TrimPrefix(matches[3], "@")
+
+	return LibraryConfig{
+		Registry: registry,
+		Name:     name,
+		Version:  version,
+	}, nil
+}

pkg/app/schema_test.go

@@ -547,3 +547,45 @@ func assertContents(t *testing.T, fs afero.Fs, expectedPath, contentPath string)
 
 	require.Equal(t, string(expected), string(got), "unexpected %q contents", contentPath)
 }
+
+func Test_parseLibraryConfig(t *testing.T) {
+	cases := []struct {
+		name     string
+		expected LibraryConfig
+		isErr    bool
+	}{
+		{
+			name:     "parts-infra/contour",
+			expected: LibraryConfig{Registry: "parts-infra", Name: "contour"},
+		},
+		{
+			name:     "contour",
+			expected: LibraryConfig{Name: "contour"},
+		},
+		{
+			name:     "parts-infra/contour@0.1.0",
+			expected: LibraryConfig{Registry: "parts-infra", Name: "contour", Version: "0.1.0"},
+		},
+		{
+			name:     "contour@0.1.0",
+			expected: LibraryConfig{Registry: "", Name: "contour", Version: "0.1.0"},
+		},
+		{
+			name:  "@foo/bar@baz@doh",
+			isErr: true,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			d, err := parseLibraryConfig(tc.name)
+			if tc.isErr {
+				require.Error(t, err)
+				return
+			}
+
+			require.NoError(t, err)
+			require.Equal(t, tc.expected, d)
+		})
+	}
+}

pkg/registry/package_manager.go

@@ -359,14 +359,14 @@ func (m *packageManager) PackagesForEnv(e *app.EnvironmentConfig) ([]pkg.Package
 		combined[k] = cfg
 	}
 
-	for k, libraryConfig := range combined {
+	for _, libraryConfig := range combined {
 		protocol, ok := registryProtocol(m.app, libraryConfig.Registry)
 		if !ok {
 			return nil, errors.Errorf("registry %q required by library %q is not defined in the configuration",
-				libraryConfig.Registry, k)
+				libraryConfig.Registry, libraryConfig.Name)
 		}
 
-		p, err := m.loadPackage(protocol, k, libraryConfig.Registry, libraryConfig.Version, pkg.TrueInstallChecker{})
+		p, err := m.loadPackage(protocol, libraryConfig.Name, libraryConfig.Registry, libraryConfig.Version, pkg.TrueInstallChecker{})
 		if err != nil {
 			return nil, err
 		}