Skip to content

Commit

Permalink
Have ValidatePaths also return invalid paths. (openconfig#2490)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
wenovus authored Jan 10, 2024
1 parent 040da5f commit 1c66488
Show file tree
Hide file tree
Showing 3 changed files with 120 additions and 52 deletions.
70 changes: 42 additions & 28 deletions tools/internal/ocpaths/ocpaths.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand All @@ -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 {
Expand All @@ -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()
}
98 changes: 76 additions & 22 deletions tools/internal/ocpaths/ocpaths_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
},
Expand All @@ -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",
},
Expand Down Expand Up @@ -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)
}
})
Expand All @@ -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",
Expand All @@ -194,31 +201,31 @@ 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",
},
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",
},
Expand All @@ -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",
Expand All @@ -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)
}
})
}
}
4 changes: 2 additions & 2 deletions tools/nosimage/validate/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down

0 comments on commit 1c66488

Please sign in to comment.