Remove TODOs on shadow path limitations during JSON unmarshalling (#479)

* Remove TODOs and add comment on limitations of current approach.

generator/generator.go

@@ -61,15 +61,14 @@ var (
 	compressPaths           = flag.Bool("compress_paths", false, "If set to true, the schema's paths are compressed, according to OpenConfig YANG module conventions. Path structs generation currently only supports compressed paths.")
 
 	// Common flags used for GoStruct and PathStruct generation.
-	yangPaths              = flag.String("path", "", "Comma separated list of paths to be recursively searched for included modules or submodules within the defined YANG modules.")
-	excludeModules         = flag.String("exclude_modules", "", "Comma separated set of module names that should be excluded from code generation this can be used to ensure overlapping namespaces can be ignored.")
-	packageName            = flag.String("package_name", "ocstructs", "The name of the Go package that should be generated.")
-	ignoreCircDeps         = flag.Bool("ignore_circdeps", false, "If set to true, circular dependencies between submodules are ignored.")
-	fakeRootName           = flag.String("fakeroot_name", "", "The name of the fake root entity.")
-	skipEnumDedup          = flag.Bool("skip_enum_deduplication", false, "If set to true, all leaves of type enumeration will have a unique enum output for them, rather than sharing a common type (default behaviour).")
-	preferOperationalState = flag.Bool("prefer_operational_state", false, "If set to true, state (config false) fields in the YANG schema are preferred over intended config leaves in the generated Go code with compressed schema paths. This flag is only valid for compress_paths=true and exclude_state=false.")
-	// TODO(wenbli): Invalid shadow paths currently do not cause an error during JSON unmarshalling.
-	ignoreShadowSchemaPaths              = flag.Bool("ignore_shadow_schema_paths", false, "If set to true when compress_paths=true, the shadowed schema path will be ignored while unmarshalling instead of causing an error. A shadow schema path is a config or state path which is selected over the other during schema compression when both config and state versions of the node exist. NOTE: Invalid shadow paths currently do not cause an error during JSON unmarshalling.")
+	yangPaths                            = flag.String("path", "", "Comma separated list of paths to be recursively searched for included modules or submodules within the defined YANG modules.")
+	excludeModules                       = flag.String("exclude_modules", "", "Comma separated set of module names that should be excluded from code generation this can be used to ensure overlapping namespaces can be ignored.")
+	packageName                          = flag.String("package_name", "ocstructs", "The name of the Go package that should be generated.")
+	ignoreCircDeps                       = flag.Bool("ignore_circdeps", false, "If set to true, circular dependencies between submodules are ignored.")
+	fakeRootName                         = flag.String("fakeroot_name", "", "The name of the fake root entity.")
+	skipEnumDedup                        = flag.Bool("skip_enum_deduplication", false, "If set to true, all leaves of type enumeration will have a unique enum output for them, rather than sharing a common type (default behaviour).")
+	preferOperationalState               = flag.Bool("prefer_operational_state", false, "If set to true, state (config false) fields in the YANG schema are preferred over intended config leaves in the generated Go code with compressed schema paths. This flag is only valid for compress_paths=true and exclude_state=false.")
+	ignoreShadowSchemaPaths              = flag.Bool("ignore_shadow_schema_paths", false, "If set to true when compress_paths=true, the shadowed schema path will be ignored while unmarshalling instead of causing an error. A shadow schema path is a config or state path which is selected over the other during schema compression when both config and state versions of the node exist.")
 	shortenEnumLeafNames                 = flag.Bool("shorten_enum_leaf_names", false, "If also set to true when compress_paths=true, all leaves of type enumeration will by default not be prefixed with the name of its residing module.")
 	useDefiningModuleForTypedefEnumNames = flag.Bool("typedef_enum_with_defmod", false, "If set to true, all typedefs of type enumeration or identity will be prefixed with the name of its module of definition instead of its residing module.")
 	appendEnumSuffixForSimpleUnionEnums  = flag.Bool("enum_suffix_for_simple_union_enums", false, "If set to true when typedef_enum_with_defmod is also true, all inlined enumerations within unions will be suffixed with \"Enum\", instead of adding the suffix only for inlined enumerations within typedef unions.")

ygen/codegen.go

@@ -136,8 +136,6 @@ type TransformationOpts struct {
 	// IgnoreShadowSchemaPaths indicates whether when OpenConfig path
 	// compression is enabled, that the shadowed paths are to be ignored
 	// while while unmarshalling.
-	// TODO(wenbli): Invalid shadow paths currently do not cause an error
-	// during JSON unmarshalling.
 	IgnoreShadowSchemaPaths bool
 	// GenerateFakeRoot specifies whether an entity that represents the
 	// root of the YANG schema tree should be generated in the generated

ytypes/container.go

@@ -191,15 +191,13 @@ func unmarshalStruct(schema *yang.Entry, parent interface{}, jsonTree map[string
 		}
 		allSchemaPaths = append(allSchemaPaths, sp...)
 
-		// If there are shadow schema paths, also whitelist these to avoid an unmarshalling error.
-		// TODO(wenbli): Since only the first-level path name is checked for invalid names,
-		// any JSON with invalid paths under the current shadow path name would not cause an
-		// error.  e.g. If config is shadowed, and ("config/bar" is the jsonValue, but only
-		// "config/foo" exists, there would be no error, even though unmarshalling
-		// "state/bar" would cause an error, and so should "config/bar").
-		// To improve the check, one way would be to add a new unmarshal flag
-		// "skipLeafModification" that is supplied to downstream unmarshal* calls whenever
-		// the path traversal enters a shadowed part of the schema.
+		// If there are shadow schema paths, also add them to the allowlist
+		// avoid an unmarshalling error.
+		// NOTE: This is more permissive than ideal in that it doesn't
+		// catch other types of non-compliance errors, i.e. if the JSON
+		// shadow node is a container (should not occur under
+		// OpenConfig YANG rules), or if the JSON cannot be
+		// unmarshalled due to type mismatch.
 		ssp, err := shadowDataTreePaths(schema, cschema, ft)
 		if err != nil {
 			return err

ytypes/container_test.go

@@ -426,6 +426,13 @@ func TestUnmarshalContainer(t *testing.T) {
 			json:    `{ "container-field": { "leaf2-field": 43, "config": { "leaf1-field": 42 } } }`,
 			wantErr: `parent container container-field (type *ytypes.ContainerStructPreferStateNoShadow): JSON contains unexpected field config`,
 		},
+		{
+			desc:    "fail ignoring invalid shadow leaf",
+			schema:  containerSchema,
+			parent:  &ParentContainerStructPreferState{},
+			json:    `{ "container-field": { "leaf2-field": 43, "config": { "non-existent-field": 42 } } }`,
+			wantErr: `parent container container-field (type *ytypes.ContainerStructPreferState): JSON contains unexpected field non-existent-field`,
+		},
 	}
 
 	var jsonTree interface{}

ytypes/leaf_test.go

@@ -1096,6 +1096,11 @@ func TestUnmarshalLeafJSONEncoding(t *testing.T) {
 			json: `{"state" : { "inner-int8-leaf" : -42} }`,
 			want: LeafContainerStruct{},
 		},
+		{
+			desc:    "non-existent state fail ignoring",
+			json:    `{"state" : { "non-existent-leaf" : -42} }`,
+			wantErr: `parent container container-schema (type *ytypes.LeafContainerStruct): JSON contains unexpected field non-existent-leaf`,
+		},
 		{
 			desc: "uint8 success",
 			json: `{"uint8-leaf" : 42}`,