Repository: openconfig/goyang
Pull request #199 (2021-08-20 23:51:50)

Simplify some code relating to prefixes.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/42311467/badge)](https://coveralls.io/builds/42311467)

Coverage increased (+0.08%) to 81.686% when pulling **faaf64bd380bb13295e8aa2646ff1db6d937c05a on simplify-prefix** into **1fb7893798e04830b4d7fbbcd6672b95668fcf73 on master**.


Patch:
diff --git a/pkg/yang/entry.go b/pkg/yang/entry.go
index 81b46c8..6ff3bb9 100644
--- a/pkg/yang/entry.go
+++ b/pkg/yang/entry.go
@@ -1093,7 +1093,16 @@ func (e *Entry) ApplyDeviate() []error {
 					}
 
 					if devSpec.Default != "" {
-						deviatedNode.Default = ""
+						switch dt {
+						case DeviationAdd:
+							if deviatedNode.Default != "" {
+								appendErr(fmt.Errorf("tried to deviate add a default statement when one already exists: %q", deviatedNode.Default))
+							} else {
+								deviatedNode.Default = devSpec.Default
+							}
+						case DeviationReplace:
+							deviatedNode.Default = devSpec.Default
+						}
 					}
 
 					if devSpec.Mandatory != TSUnset {
@@ -1136,8 +1145,12 @@ func (e *Entry) ApplyDeviate() []error {
 						deviatedNode.Config = TSUnset
 					}
 
-					if devSpec.Default == "" {
-						deviatedNode.Default = ""
+					if devSpec.Default != "" {
+						if devSpec.Default == deviatedNode.Default {
+							deviatedNode.Default = ""
+						} else {
+							appendErr(fmt.Errorf("%s: tried to deviate delete a default statement that doesn't exist or with a non-matching keyword", Source(e.Node)))
+						}
 					}
 
 					if devSpec.Mandatory != TSUnset {
@@ -1243,7 +1256,7 @@ func (e *Entry) Find(name string) *Entry {
 
 		// Since this module might use a different prefix that isn't
 		// the prefix that the module itself uses then we need to resolve
-		// the module into its local prefix to find it.
+		// the remotely-used prefix into its locally-used prefix to find it.
 		pfxMap := map[string]string{
 			// Seed the map with the local module - we use GetPrefix just
 			// in case the module is a submodule.
@@ -1366,7 +1379,7 @@ func (e *Entry) InstantiatingModule() (string, error) {
 
 	module, err := e.Modules().FindModuleByNamespace(n.Name)
 	if err != nil {
-		return "", fmt.Errorf("could not find module %q when retrieving namespace for %s", n.Name, e.Name)
+		return "", fmt.Errorf("could not find module %q when retrieving namespace for %s: %v", n.Name, e.Name, err)
 	}
 	return module.Name, nil
 }
diff --git a/pkg/yang/find.go b/pkg/yang/find.go
index fc5cd71..602c705 100644
--- a/pkg/yang/find.go
+++ b/pkg/yang/find.go
@@ -21,25 +21,21 @@ import (
 	"strings"
 )
 
-// trimPrefix trims the current module's prefix from the given name. If the
-// name is not prefixed with it, the same string is returned unchanged.
-// TODO(borman): we need to properly handle prefixs and not depend on
-// not having collisions.
-func trimPrefix(n Node, name string) string {
-	parts := strings.Split(name, ":")
-	if len(parts) == 1 {
-		return name
-	}
-	// We should have a single *Module parent.  It contains our prefix.
-	for {
-		if m, ok := n.(*Module); ok {
-			if m.Prefix != nil && m.Prefix.Name == parts[0] {
-				return parts[1]
-			}
-			return name
-		}
-		n = n.ParentNode()
+// localPrefix returns the local prefix used by the containing (sub)module to
+// refer to its own module.
+func localPrefix(n Node) string {
+	return RootNode(n).GetPrefix()
+}
+
+// trimLocalPrefix trims the current module's prefix from the given name. If the
+// name is not prefixed with the local module's prefix or is unprefixed
+// entirely, then the same string is returned unchanged.
+func trimLocalPrefix(n Node, name string) string {
+	pfx := localPrefix(n)
+	if pfx != "" {
+		pfx += ":"
 	}
+	return strings.TrimPrefix(name, pfx)
 }
 
 // FindGrouping finds the grouping named name in one of the parent node's
@@ -47,7 +43,7 @@ func trimPrefix(n Node, name string) string {
 // by FindGrouping during traversal.  If no parent has the named grouping,
 // nil is returned. Imported and included modules are also checked.
 func FindGrouping(n Node, name string, seen map[string]bool) *Grouping {
-	name = trimPrefix(n, name)
+	name = trimLocalPrefix(n, name)
 	for n != nil {
 		// Grab the Grouping field of the underlying structure.  n is
 		// always a pointer to a structure,
@@ -63,8 +59,8 @@ func FindGrouping(n Node, name string, seen map[string]bool) *Grouping {
 		v = e.FieldByName("Import")
 		if v.IsValid() {
 			for _, i := range v.Interface().([]*Import) {
-				// TODO(borman): This is arguably wrong.  We
-				// need to do prefix matching.
+				// If the prefix matches the import statement,
+				// then search for the trimmed name in that module.
 				pname := strings.TrimPrefix(name, i.Prefix.Name+":")
 				if pname == name {
 					continue
diff --git a/pkg/yang/modules.go b/pkg/yang/modules.go
index 8bba228..286fdd8 100644
--- a/pkg/yang/modules.go
+++ b/pkg/yang/modules.go
@@ -228,7 +228,7 @@ func (ms *Modules) FindModuleByNamespace(ns string) (*Module, error) {
 		}
 	}
 	if found == nil {
-		return nil, fmt.Errorf("%s: no such namespace", ns)
+		return nil, fmt.Errorf("%q: no such namespace", ns)
 	}
 	// Don't cache negative results because new modules could be added.
 	ms.byNS[ns] = found
diff --git a/pkg/yang/node.go b/pkg/yang/node.go
index e9818ae..9c2f8b2 100644
--- a/pkg/yang/node.go
+++ b/pkg/yang/node.go
@@ -96,26 +96,12 @@ func getPrefix(s string) (string, string) {
 // relative to where n was defined.  If the prefix cannot be resolved then nil
 // is returned.
 func FindModuleByPrefix(n Node, prefix string) *Module {
-	n = RootNode(n)
+	mod := RootNode(n)
 
-	mod := n.(*Module)
-	if prefix == "" {
+	if prefix == "" || prefix == mod.GetPrefix() {
 		return mod
 	}
 
-	switch mod.Kind() {
-	case "module":
-		if mod.Prefix.Name == prefix {
-			return mod
-		}
-	case "submodule":
-		if prefix == mod.BelongsTo.Prefix.Name {
-			return mod
-		}
-	default:
-		panic("root that is not a module or submodule")
-	}
-
 	for _, i := range mod.Import {
 		if prefix == i.Prefix.Name {
 			return i.Module
@@ -199,9 +185,9 @@ func FindNode(n Node, path string) (Node, error) {
 		parts = parts[1:]
 
 		// TODO(borman): merge this with FindModuleByPrefix?
-		n = RootNode(n)
 		// The base is always a module
-		mod := n.(*Module)
+		mod := RootNode(n)
+		n = mod
 		prefix, _ := getPrefix(parts[0])
 		if mod.Kind() == "submodule" {
 			m := mod.Modules.Modules[mod.BelongsTo.Name]
diff --git a/pkg/yang/types.go b/pkg/yang/types.go
index 4d255ee..b3efb0e 100644
--- a/pkg/yang/types.go
+++ b/pkg/yang/types.go
@@ -64,7 +64,7 @@ func (d *typeDictionary) find(n Node, name string) *Typedef {
 	return d.dict[n][name]
 }
 
-// findExternal finds the externally defined typedef name in the module imported
+// findExternal finds the externally-defined typedef name in a module imported
 // by n's root with the specified prefix.
 func (d *typeDictionary) findExternal(n Node, prefix, name string) (*Typedef, error) {
 	root := FindModuleByPrefix(n, prefix)
diff --git a/pkg/yang/yang.go b/pkg/yang/yang.go
index 9c92803..e37bd45 100644
--- a/pkg/yang/yang.go
+++ b/pkg/yang/yang.go
@@ -185,13 +185,14 @@ func (s *Module) GetPrefix() string {
 	return pfx.Name
 }
 
+// getPrefix returns the local prefix of the module used to refer to itself.
 func (s *Module) getPrefix() *Value {
 	switch {
 	case s == nil:
 		return nil
-	case s.Prefix != nil:
+	case s.Kind() == "module" && s.Prefix != nil:
 		return s.Prefix
-	case s.BelongsTo != nil:
+	case s.Kind() == "submodule" && s.BelongsTo != nil:
 		return s.BelongsTo.Prefix
 	default:
 		return nil


Test patch:
diff --git a/pkg/yang/entry_test.go b/pkg/yang/entry_test.go
index 7bb2331..12f857a 100644
--- a/pkg/yang/entry_test.go
+++ b/pkg/yang/entry_test.go
@@ -405,7 +405,7 @@ func TestEntryNamespace(t *testing.T) {
 			descr:        "leaf directly defined within an augment to foo from orphan submodule qux-augment has empty namespace",
 			entry:        foo.Dir["foo-c"].Dir["qux-submod-leaf"],
 			ns:           "",
-			wantModError: `could not find module "" when retrieving namespace for qux-submod-leaf`,
+			wantModError: `could not find module "" when retrieving namespace for qux-submod-leaf: "": no such namespace`,
 		},
 		{
 			descr:   "children of a container within an augment to from baz have baz's namespace",
@@ -2760,6 +2760,11 @@ func TestDeviation(t *testing.T) {
 				entry: &Entry{
 					Config: TSFalse,
 				},
+			}, {
+				path: "/target/add/default",
+				entry: &Entry{
+					Default: "a default value",
+				},
 			}, {
 				path: "/target/add/mandatory",
 				entry: &Entry{
@@ -2804,6 +2809,27 @@ func TestDeviation(t *testing.T) {
 				},
 			}},
 		},
+	}, {
+		desc: "error case - deviation add that already has a default",
+		inFiles: map[string]string{
+			"deviate": `
+				module deviate {
+					prefix "d";
+					namespace "urn:d";
+
+					leaf a {
+						type string;
+						default "fish";
+					}
+
+					deviation /a {
+						deviate add {
+							default "fishsticks";
+						}
+					}
+				}`,
+		},
+		wantProcessErrSubstring: "already exists",
 	}, {
 		desc: "error case - deviate type not recognized",
 		inFiles: map[string]string{
@@ -2966,6 +2992,11 @@ func TestDeviation(t *testing.T) {
 				entry: &Entry{
 					Config: TSFalse,
 				},
+			}, {
+				path: "/target/replace/default",
+				entry: &Entry{
+					Default: "a default value",
+				},
 			}, {
 				path: "/target/replace/mandatory",
 				entry: &Entry{
@@ -3027,6 +3058,9 @@ func TestDeviation(t *testing.T) {
 				entry: &Entry{
 					Config: TSUnset,
 				},
+			}, {
+				path:  "/target/delete/default",
+				entry: &Entry{},
 			}, {
 				path: "/target/delete/mandatory",
 				entry: &Entry{
@@ -3071,6 +3105,27 @@ func TestDeviation(t *testing.T) {
 				},
 			}},
 		},
+	}, {
+		desc: "error case - deviation delete of default has different keyword value",
+		inFiles: map[string]string{
+			"deviate": `
+				module deviate {
+					prefix "d";
+					namespace "urn:d";
+
+					leaf a {
+						type string;
+						default "fish";
+					}
+
+					deviation /a {
+						deviate delete {
+							default "fishsticks";
+						}
+					}
+				}`,
+		},
+		wantProcessErrSubstring: "non-matching keyword",
 	}, {
 		desc: "error case - deviation delete of min-elements has different keyword value",
 		inFiles: map[string]string{
diff --git a/pkg/yang/modules_test.go b/pkg/yang/modules_test.go
index 877dc3a..004617b 100644
--- a/pkg/yang/modules_test.go
+++ b/pkg/yang/modules_test.go
@@ -138,7 +138,7 @@ func TestModulesFindByNamespace(t *testing.T) {
 	}{
 		{
 			namespace: "does-not-exist",
-			wantError: "does-not-exist: no such namespace",
+			wantError: `"does-not-exist": no such namespace`,
 		},
 		{
 			namespace: "urn:foo",