From 1c664882a7503f63526462a5d71f70a2b456f90f Mon Sep 17 00:00:00 2001 From: Wen Bo Li <50884368+wenovus@users.noreply.github.com> Date: Tue, 9 Jan 2024 18:11:23 -0800 Subject: [PATCH] Have ValidatePaths also return invalid paths. (#2490) * Have ValidatePaths also return invalid paths. This is interesting to know if we're only interested in the valid paths of a set of OCPaths. For example, this is useful when we want to filter out paths that are already invalid somewhere else when reporting validation errors. * Improve return type * Fix * Component -> PlatformTypes --- tools/internal/ocpaths/ocpaths.go | 70 ++++++++++-------- tools/internal/ocpaths/ocpaths_test.go | 98 ++++++++++++++++++++------ tools/nosimage/validate/validate.go | 4 +- 3 files changed, 120 insertions(+), 52 deletions(-) diff --git a/tools/internal/ocpaths/ocpaths.go b/tools/internal/ocpaths/ocpaths.go index fd086243d3d..26e329578af 100644 --- a/tools/internal/ocpaths/ocpaths.go +++ b/tools/internal/ocpaths/ocpaths.go @@ -61,8 +61,8 @@ var ( // OCPathKey contains the fields that uniquely identify an OC path. type OCPathKey struct { - Path string - Component string + Path string + PlatformType string } // OCPath is the parsed version of the spreadsheet's paths. @@ -97,22 +97,14 @@ func getSchemaFakeroot(publicPath string) (*yang.Entry, error) { return root, nil } -func validatePath(ocpathProto *ppb.OCPath, root *yang.Entry) (*OCPath, error) { - ocpath := &OCPath{ - Key: OCPathKey{ - Path: ocpathProto.GetName(), - Component: ocpathProto.GetOcpathConstraint().GetPlatformType(), - }, - FeatureprofileID: ocpathProto.GetFeatureprofileid(), - } - +func validatePath(ocpath *OCPath, root *yang.Entry) error { // Validate path path := ocpath.Key.Path if !strings.HasPrefix(path, "/") { - return nil, fmt.Errorf("path does not begin with slash: %q", path) + return fmt.Errorf("path does not begin with slash: %q", path) } if strings.HasSuffix(path, "/") { - return nil, fmt.Errorf("path must not end with slash: %q", path) + return fmt.Errorf("path must not end with slash: %q", path) } if entry := root.Find(path); entry == nil { deepestEntry := root @@ -130,23 +122,23 @@ func validatePath(ocpathProto *ppb.OCPath, root *yang.Entry) (*OCPath, error) { } deepestEntry = next } - return nil, fmt.Errorf("path not found: %q, remaining path: %q", path, entryNotFound) + return fmt.Errorf("path not found: %q, remaining path: %q", path, entryNotFound) } else if !entry.IsLeaf() && !entry.IsLeafList() { - return nil, fmt.Errorf("path %q is not a leaf: got kind %s", path, entry.Kind) + return fmt.Errorf("path %q is not a leaf: got kind %s", path, entry.Kind) } // Validate component - component := ocpath.Key.Component + component := ocpath.Key.PlatformType isComponentPath := strings.HasPrefix(path, componentPrefix) switch { case !isComponentPath && component != "": - return nil, fmt.Errorf("non-component path %q has component value %q", path, component) + return fmt.Errorf("non-component path %q has component value %q", path, component) case !isComponentPath: default: if _, ok := validComponentNames[component]; !ok { - return nil, fmt.Errorf("path %q has invalid component %q (must be one of %v)", path, component, validComponentNamesSorted) + return fmt.Errorf("path %q has invalid component %q (must be one of %v)", path, component, validComponentNamesSorted) } - ocpath.Key.Component = component + ocpath.Key.PlatformType = component } // featureprofileID is optional. Only validate the string format if it exists. @@ -155,11 +147,11 @@ func validatePath(ocpathProto *ppb.OCPath, root *yang.Entry) (*OCPath, error) { case featureprofileIDMatcher.MatchString(featureprofileID): ocpath.FeatureprofileID = featureprofileID default: - return nil, fmt.Errorf("unexpected featureprofileID string %q for path %v (must match regex %q)", featureprofileID, path, featureprofileIDRegex) + return fmt.Errorf("unexpected featureprofileID string %q for path %v (must match regex %q)", featureprofileID, path, featureprofileIDRegex) } } - return ocpath, nil + return nil } func insert(dstMap map[OCPathKey]*OCPath, src *OCPath) error { @@ -173,28 +165,50 @@ func insert(dstMap map[OCPathKey]*OCPath, src *OCPath) error { return nil } +func convertOCPath(ocpathProto *ppb.OCPath) *OCPath { + return &OCPath{ + Key: OCPathKey{ + Path: ocpathProto.GetName(), + PlatformType: ocpathProto.GetOcpathConstraint().GetPlatformType(), + }, + FeatureprofileID: ocpathProto.GetFeatureprofileid(), + } +} + // ValidatePaths parses and validates ocpaths, and puts them into a more // user-friendly Go structure. -func ValidatePaths(ocpathsProto []*ppb.OCPath, publicPath string) (map[OCPathKey]*OCPath, error) { +// +// The first set of paths contain only valid path, while the second contain only invalid paths. +func ValidatePaths(ocpathsProto []*ppb.OCPath, publicPath string) (map[OCPathKey]*OCPath, map[OCPathKey]*OCPath, error) { root, err := getSchemaFakeroot(publicPath) if err != nil { - return nil, err + return nil, nil, err } ocpaths := map[OCPathKey]*OCPath{} + invalidOCPaths := map[OCPathKey]*OCPath{} errs := errlist.List{ Separator: "\n", } for _, ocpathProto := range ocpathsProto { - ocpath, err := validatePath(ocpathProto, root) - if err != nil { - errs.Add(err) - } else if ocpath == nil { + ocpath := convertOCPath(ocpathProto) + if ocpath == nil { errs.Add(fmt.Errorf("failed to parse proto: %v", ocpathProto)) + } else if err := validatePath(ocpath, root); err != nil { + errs.Add(err) + if ocpath != nil { + invalidOCPaths[ocpath.Key] = ocpath + } } else if err := insert(ocpaths, ocpath); err != nil { errs.Add(err) } } - return ocpaths, errs.Err() + if len(ocpaths) == 0 { + ocpaths = nil + } + if len(invalidOCPaths) == 0 { + invalidOCPaths = nil + } + return ocpaths, invalidOCPaths, errs.Err() } diff --git a/tools/internal/ocpaths/ocpaths_test.go b/tools/internal/ocpaths/ocpaths_test.go index 5adaa4fcc96..f4582df917f 100644 --- a/tools/internal/ocpaths/ocpaths_test.go +++ b/tools/internal/ocpaths/ocpaths_test.go @@ -48,8 +48,8 @@ func TestValidatePath(t *testing.T) { }, wantOCPath: &OCPath{ Key: OCPathKey{ - Path: "/interfaces/interface/config/name", - Component: "", + Path: "/interfaces/interface/config/name", + PlatformType: "", }, FeatureprofileID: "interface_base", }, @@ -69,8 +69,8 @@ func TestValidatePath(t *testing.T) { }, wantOCPath: &OCPath{ Key: OCPathKey{ - Path: "/components/component/state/name", - Component: "CPU", + Path: "/components/component/state/name", + PlatformType: "CPU", }, FeatureprofileID: "interface_base", }, @@ -160,12 +160,18 @@ func TestValidatePath(t *testing.T) { for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { - got, err := validatePath(tt.inOcPathProto, root) + ocpath := convertOCPath(tt.inOcPathProto) + + err := validatePath(ocpath, root) if diff := errdiff.Substring(err, tt.wantErrSubstr); diff != "" { t.Errorf(diff) } - if diff := cmp.Diff(tt.wantOCPath, got); diff != "" { + if err != nil { + return + } + + if diff := cmp.Diff(tt.wantOCPath, ocpath); diff != "" { t.Errorf("(-want, +got):\n%s", diff) } }) @@ -177,6 +183,7 @@ func TestValidatePaths(t *testing.T) { desc string inOcPathsProto []*ppb.OCPath wantOCPaths map[OCPathKey]*OCPath + wantInvalids map[OCPathKey]*OCPath wantErr bool }{{ desc: "valid", @@ -194,8 +201,8 @@ func TestValidatePaths(t *testing.T) { }}, wantOCPaths: map[OCPathKey]*OCPath{ { - Path: "/interfaces/interface/config/name", - Component: "", + Path: "/interfaces/interface/config/name", + PlatformType: "", }: { Key: OCPathKey{ Path: "/interfaces/interface/config/name", @@ -203,22 +210,22 @@ func TestValidatePaths(t *testing.T) { FeatureprofileID: "interface_base", }, { - Path: "/components/component/config/description", - Component: "CPU", + Path: "/components/component/config/description", + PlatformType: "CPU", }: { Key: OCPathKey{ - Path: "/components/component/config/description", - Component: "CPU", + Path: "/components/component/config/description", + PlatformType: "CPU", }, FeatureprofileID: "interface_base", }, { - Path: "/components/component/config/description", - Component: "PORT", + Path: "/components/component/config/description", + PlatformType: "PORT", }: { Key: OCPathKey{ - Path: "/components/component/config/description", - Component: "PORT", + Path: "/components/component/config/description", + PlatformType: "PORT", }, FeatureprofileID: "interface_basic", }, @@ -229,9 +236,20 @@ func TestValidatePaths(t *testing.T) { Name: "/interfaces/interface/config", Featureprofileid: "interface_base", }}, + wantInvalids: map[OCPathKey]*OCPath{ + { + Path: "/interfaces/interface/config", + PlatformType: "", + }: { + Key: OCPathKey{ + Path: "/interfaces/interface/config", + }, + FeatureprofileID: "interface_base", + }, + }, wantErr: true, }, { - desc: "duplicate", + desc: "duplicate-and-invalid", inOcPathsProto: []*ppb.OCPath{{ Name: "/interfaces/interface/config/name", Featureprofileid: "interface_base", @@ -243,23 +261,59 @@ func TestValidatePaths(t *testing.T) { Name: "/components/component/config/description", OcpathConstraint: &ppb.OCPathConstraint{Constraint: &ppb.OCPathConstraint_PlatformType{PlatformType: "CPU"}}, Featureprofileid: "interface_basic", + }, { + Name: "/interfaces/interface/config", + Featureprofileid: "interface_base", }}, + wantOCPaths: map[OCPathKey]*OCPath{ + { + Path: "/interfaces/interface/config/name", + PlatformType: "", + }: { + Key: OCPathKey{ + Path: "/interfaces/interface/config/name", + }, + FeatureprofileID: "interface_base", + }, + { + Path: "/components/component/config/description", + PlatformType: "CPU", + }: { + Key: OCPathKey{ + Path: "/components/component/config/description", + PlatformType: "CPU", + }, + FeatureprofileID: "interface_base", + }, + }, + wantInvalids: map[OCPathKey]*OCPath{ + { + Path: "/interfaces/interface/config", + PlatformType: "", + }: { + Key: OCPathKey{ + Path: "/interfaces/interface/config", + }, + FeatureprofileID: "interface_base", + }, + }, wantErr: true, }} for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { - got, err := ValidatePaths(tt.inOcPathsProto, "testdata/models") + got, gotInvalids, err := ValidatePaths(tt.inOcPathsProto, "testdata/models") if (err != nil) != tt.wantErr { - t.Fatalf("gotErr: %v, wantErr: %v", err, tt.wantErr) - } - if err != nil { - return + t.Errorf("gotErr: %v, wantErr: %v", err, tt.wantErr) } if diff := cmp.Diff(tt.wantOCPaths, got); diff != "" { t.Errorf("(-want, +got):\n%s", diff) } + + if diff := cmp.Diff(tt.wantInvalids, gotInvalids); diff != "" { + t.Errorf("(-want, +got):\n%s", diff) + } }) } } diff --git a/tools/nosimage/validate/validate.go b/tools/nosimage/validate/validate.go index aed6814e129..19ab7b95187 100644 --- a/tools/nosimage/validate/validate.go +++ b/tools/nosimage/validate/validate.go @@ -109,9 +109,9 @@ func main() { os.Exit(1) } - paths, err := ocpaths.ValidatePaths(profile.GetOcpaths().GetOcpaths(), publicPath) + paths, invalidPaths, err := ocpaths.ValidatePaths(profile.GetOcpaths().GetOcpaths(), publicPath) if err != nil { - fmt.Println(err) + fmt.Printf("profile contains %d invalid OCPaths:\n%v", len(invalidPaths), err) os.Exit(1) } fmt.Printf("profile contains %d valid OCPaths\n", len(paths))