From 878ae28bee6a23e8af19529c4d7eb9b40ef9105f Mon Sep 17 00:00:00 2001 From: Sunny Date: Mon, 8 May 2017 19:56:00 +0530 Subject: [PATCH 1/4] Add metadata to manifest & warn on unknown fields - Use manifest buffer to create a map of TomlTree and find all the unknown fields in the tree. Raise warnings for unknown fields. - For "metadata" field, ensure it's of map type, else raise warning. --- manifest.go | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/manifest.go b/manifest.go index 621edf1a6a..479e399ac1 100644 --- a/manifest.go +++ b/manifest.go @@ -7,15 +7,21 @@ package dep import ( "bytes" "io" + "reflect" "sort" "github.com/golang/dep/gps" + "github.com/golang/dep/internal" "github.com/pelletier/go-toml" "github.com/pkg/errors" ) const ManifestName = "Gopkg.toml" +var ManifestFields = []string{ + "dependencies", "overrides", "ignored", "required", "metadata", +} + type Manifest struct { Dependencies gps.ProjectConstraints Ovr gps.ProjectConstraints @@ -38,12 +44,59 @@ type rawProject struct { Source string `toml:"source,omitempty"` } +func validateManifest(b *bytes.Buffer) error { + // Load the TomlTree from reader + tree, err := toml.Load(b.String()) + if err != nil { + return errors.Wrap(err, "Unable to load TomlTree from string") + } + // Convert tree to a map + manifest := tree.ToMap() + + // Look for unknown fields and raise warnings + for prop, val := range manifest { + isValidField := false + for _, v := range ManifestFields { + if prop == v { + isValidField = true + + // For metadata, check if it is a map type + if prop == "metadata" { + isValidMetadata := false + if metadata, ok := val.([]interface{}); ok { + for _, v := range metadata { + if reflect.TypeOf(v).Kind() == reflect.Map { + isValidMetadata = true + } else { + isValidMetadata = false + break + } + } + } + if !isValidMetadata { + internal.Logf("WARNING: metadata should consist of key-value pairs") + } + } + } + } + if !isValidField { + internal.Logf("WARNING: Unknown field in manifest: %v", prop) + } + } + + return nil +} + func readManifest(r io.Reader) (*Manifest, error) { buf := &bytes.Buffer{} _, err := buf.ReadFrom(r) if err != nil { return nil, errors.Wrap(err, "Unable to read byte stream") } + err = validateManifest(buf) + if err != nil { + return nil, errors.Wrap(err, "Manifest validation failed") + } raw := rawManifest{} err = toml.Unmarshal(buf.Bytes(), &raw) From 81292174033a1ef014715317351c2d44ece27720 Mon Sep 17 00:00:00 2001 From: Sunny Date: Tue, 9 May 2017 02:17:24 +0530 Subject: [PATCH 2/4] Return []error from ValidateManifest and add tests - Modifies ValidateManifest() to return []error, as a collection of all the errors, instead of logging directly. - Adds tests for ValidateManifest(). --- manifest.go | 23 ++++++++++------ manifest_test.go | 72 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 8 deletions(-) diff --git a/manifest.go b/manifest.go index 479e399ac1..f2f5026967 100644 --- a/manifest.go +++ b/manifest.go @@ -6,6 +6,7 @@ package dep import ( "bytes" + "fmt" "io" "reflect" "sort" @@ -44,11 +45,12 @@ type rawProject struct { Source string `toml:"source,omitempty"` } -func validateManifest(b *bytes.Buffer) error { - // Load the TomlTree from reader - tree, err := toml.Load(b.String()) +func validateManifest(s string) ([]error, error) { + var errs []error + // Load the TomlTree from string + tree, err := toml.Load(s) if err != nil { - return errors.Wrap(err, "Unable to load TomlTree from string") + return errs, errors.Wrap(err, "Unable to load TomlTree from string") } // Convert tree to a map manifest := tree.ToMap() @@ -74,17 +76,17 @@ func validateManifest(b *bytes.Buffer) error { } } if !isValidMetadata { - internal.Logf("WARNING: metadata should consist of key-value pairs") + errs = append(errs, errors.New("metadata should consist of key-value pairs")) } } } } if !isValidField { - internal.Logf("WARNING: Unknown field in manifest: %v", prop) + errs = append(errs, fmt.Errorf("Unknown field in manifest: %v", prop)) } } - return nil + return errs, nil } func readManifest(r io.Reader) (*Manifest, error) { @@ -93,10 +95,15 @@ func readManifest(r io.Reader) (*Manifest, error) { if err != nil { return nil, errors.Wrap(err, "Unable to read byte stream") } - err = validateManifest(buf) + + // Validate manifest and log warnings + errs, err := validateManifest(buf.String()) if err != nil { return nil, errors.Wrap(err, "Manifest validation failed") } + for _, e := range errs { + internal.Logf("WARNING: %v", e) + } raw := rawManifest{} err = toml.Unmarshal(buf.Bytes(), &raw) diff --git a/manifest_test.go b/manifest_test.go index 56594f41e6..e998b886ff 100644 --- a/manifest_test.go +++ b/manifest_test.go @@ -5,6 +5,7 @@ package dep import ( + "errors" "reflect" "strings" "testing" @@ -119,3 +120,74 @@ func TestReadManifestErrors(t *testing.T) { } } } + +func TestValidateManifest(t *testing.T) { + cases := []struct { + tomlString string + want []error + }{ + { + tomlString: ` + [[dependencies]] + name = "github.com/foo/bar" + `, + want: []error{}, + }, + { + tomlString: ` + foo = "some-value" + version = 14 + + [[bar]] + author = "xyz" + + [[dependencies]] + name = "github.com/foo/bar" + version = "" + `, + want: []error{ + errors.New("Unknown field in manifest: foo"), + errors.New("Unknown field in manifest: bar"), + errors.New("Unknown field in manifest: version"), + }, + }, + { + tomlString: ` + metadata = "project-name" + + [[dependencies]] + name = "github.com/foo/bar" + `, + want: []error{errors.New("metadata should consist of key-value pairs")}, + }, + } + + // constains for error + contains := func(s []error, e error) bool { + for _, a := range s { + if a.Error() == e.Error() { + return true + } + } + return false + } + + for _, c := range cases { + errs, err := validateManifest(c.tomlString) + if err != nil { + t.Fatal(err) + } + + // compare length of error slice + if len(errs) != len(c.want) { + t.Fatalf("Number of manifest errors are not as expected: \n\t(GOT) %v errors(%v)\n\t(WNT) %v errors(%v).", len(errs), errs, len(c.want), c.want) + } + + // check if the expected errors exist in actual errors slice + for _, er := range errs { + if !contains(c.want, er) { + t.Fatalf("Manifest errors are not as expected: \n\t(MISSING) %v\n\t(FROM) %v", er, c.want) + } + } + } +} From e518855cf9cce3cf6f3fe09c6d51f7b75ab6403d Mon Sep 17 00:00:00 2001 From: Sunny Date: Tue, 9 May 2017 09:02:45 +0530 Subject: [PATCH 3/4] Improve manifest validation - Improves property validation by checking for TOML array of tables. - Adds checks for invalid fields in dependencies and overrides. --- manifest.go | 55 ++++++++++++++++++++++++------------------------ manifest_test.go | 38 +++++++++++++++++++++++++++++++-- 2 files changed, 64 insertions(+), 29 deletions(-) diff --git a/manifest.go b/manifest.go index f2f5026967..183917b2f5 100644 --- a/manifest.go +++ b/manifest.go @@ -8,7 +8,6 @@ import ( "bytes" "fmt" "io" - "reflect" "sort" "github.com/golang/dep/gps" @@ -19,10 +18,6 @@ import ( const ManifestName = "Gopkg.toml" -var ManifestFields = []string{ - "dependencies", "overrides", "ignored", "required", "metadata", -} - type Manifest struct { Dependencies gps.ProjectConstraints Ovr gps.ProjectConstraints @@ -55,33 +50,39 @@ func validateManifest(s string) ([]error, error) { // Convert tree to a map manifest := tree.ToMap() - // Look for unknown fields and raise warnings + // Look for unknown fields and collect errors for prop, val := range manifest { - isValidField := false - for _, v := range ManifestFields { - if prop == v { - isValidField = true - - // For metadata, check if it is a map type - if prop == "metadata" { - isValidMetadata := false - if metadata, ok := val.([]interface{}); ok { - for _, v := range metadata { - if reflect.TypeOf(v).Kind() == reflect.Map { - isValidMetadata = true - } else { - isValidMetadata = false - break - } + switch prop { + case "metadata": + // Invalid if type assertion fails. Not a TOML array of tables. + // This check is enough for map validation because any non-key-value + // pair inside array of tables becomes an invalid TOML and parser + // would fail. + if _, ok := val.([]interface{}); !ok { + errs = append(errs, errors.New("metadata should be a TOML array of tables")) + } + case "dependencies", "overrides": + // Invalid if type assertion fails. Not a TOML array of tables. + if rawProj, ok := val.([]interface{}); ok { + // Iterate through each array of tables + for _, v := range rawProj { + // Check the individual field's key to be valid + for key, _ := range v.(map[string]interface{}) { + // Check if the key is valid + switch key { + case "name", "branch", "revision", "version", "source": + // valid key + default: + // unknown/invalid key + errs = append(errs, fmt.Errorf("Invalid key %q in %q", key, prop)) } } - if !isValidMetadata { - errs = append(errs, errors.New("metadata should consist of key-value pairs")) - } } + } else { + errs = append(errs, fmt.Errorf("%v should be a TOML array of tables", prop)) } - } - if !isValidField { + case "ignored", "required": + default: errs = append(errs, fmt.Errorf("Unknown field in manifest: %v", prop)) } } diff --git a/manifest_test.go b/manifest_test.go index e998b886ff..1ce10277e5 100644 --- a/manifest_test.go +++ b/manifest_test.go @@ -129,7 +129,15 @@ func TestValidateManifest(t *testing.T) { { tomlString: ` [[dependencies]] - name = "github.com/foo/bar" + name = "github.com/foo/bar" + `, + want: []error{}, + }, + { + tomlString: ` + [[metadata]] + authors = "foo" + version = "1.0.0" `, want: []error{}, }, @@ -158,7 +166,33 @@ func TestValidateManifest(t *testing.T) { [[dependencies]] name = "github.com/foo/bar" `, - want: []error{errors.New("metadata should consist of key-value pairs")}, + want: []error{errors.New("metadata should be a TOML array of tables")}, + }, + { + tomlString: ` + dependencies = "foo" + overrides = "bar" + `, + want: []error{ + errors.New("dependencies should be a TOML array of tables"), + errors.New("overrides should be a TOML array of tables"), + }, + }, + { + tomlString: ` + [[dependencies]] + name = "github.com/foo/bar" + location = "some-value" + link = "some-other-value" + + [[overrides]] + nick = "foo" + `, + want: []error{ + errors.New("Invalid key \"location\" in \"dependencies\""), + errors.New("Invalid key \"link\" in \"dependencies\""), + errors.New("Invalid key \"nick\" in \"overrides\""), + }, }, } From c588d9ee4aaf3f61124acab27a60022fb6b47d9e Mon Sep 17 00:00:00 2001 From: Sunny Date: Tue, 9 May 2017 11:13:34 +0530 Subject: [PATCH 4/4] Replace metadata array of tables with just table - Makes metadata TOML table. - Allows metadata table in "dependencies" and "overrides". --- manifest.go | 17 ++++++++++------- manifest_test.go | 16 ++++++++++++++-- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/manifest.go b/manifest.go index 183917b2f5..6bd7e518bf 100644 --- a/manifest.go +++ b/manifest.go @@ -8,6 +8,7 @@ import ( "bytes" "fmt" "io" + "reflect" "sort" "github.com/golang/dep/gps" @@ -54,12 +55,9 @@ func validateManifest(s string) ([]error, error) { for prop, val := range manifest { switch prop { case "metadata": - // Invalid if type assertion fails. Not a TOML array of tables. - // This check is enough for map validation because any non-key-value - // pair inside array of tables becomes an invalid TOML and parser - // would fail. - if _, ok := val.([]interface{}); !ok { - errs = append(errs, errors.New("metadata should be a TOML array of tables")) + // Check if metadata is of Map type + if reflect.TypeOf(val).Kind() != reflect.Map { + errs = append(errs, errors.New("metadata should be a TOML table")) } case "dependencies", "overrides": // Invalid if type assertion fails. Not a TOML array of tables. @@ -67,11 +65,16 @@ func validateManifest(s string) ([]error, error) { // Iterate through each array of tables for _, v := range rawProj { // Check the individual field's key to be valid - for key, _ := range v.(map[string]interface{}) { + for key, value := range v.(map[string]interface{}) { // Check if the key is valid switch key { case "name", "branch", "revision", "version", "source": // valid key + case "metadata": + // Check if metadata is of Map type + if reflect.TypeOf(value).Kind() != reflect.Map { + errs = append(errs, fmt.Errorf("metadata in %q should be a TOML table", prop)) + } default: // unknown/invalid key errs = append(errs, fmt.Errorf("Invalid key %q in %q", key, prop)) diff --git a/manifest_test.go b/manifest_test.go index 1ce10277e5..f7b69dac49 100644 --- a/manifest_test.go +++ b/manifest_test.go @@ -135,7 +135,7 @@ func TestValidateManifest(t *testing.T) { }, { tomlString: ` - [[metadata]] + [metadata] authors = "foo" version = "1.0.0" `, @@ -166,7 +166,7 @@ func TestValidateManifest(t *testing.T) { [[dependencies]] name = "github.com/foo/bar" `, - want: []error{errors.New("metadata should be a TOML array of tables")}, + want: []error{errors.New("metadata should be a TOML table")}, }, { tomlString: ` @@ -184,6 +184,7 @@ func TestValidateManifest(t *testing.T) { name = "github.com/foo/bar" location = "some-value" link = "some-other-value" + metadata = "foo" [[overrides]] nick = "foo" @@ -192,8 +193,19 @@ func TestValidateManifest(t *testing.T) { errors.New("Invalid key \"location\" in \"dependencies\""), errors.New("Invalid key \"link\" in \"dependencies\""), errors.New("Invalid key \"nick\" in \"overrides\""), + errors.New("metadata in \"dependencies\" should be a TOML table"), }, }, + { + tomlString: ` + [[dependencies]] + name = "github.com/foo/bar" + + [dependencies.metadata] + color = "blue" + `, + want: []error{}, + }, } // constains for error