Skip to content
This repository was archived by the owner on Sep 9, 2020. It is now read-only.

Commit 22b825a

Browse files
carolynvscarolynvs
authored
Merge pull request #717 from darkowlzz/702-stricter-manifest-validation
Stricter manifest validation - errors & warnings
2 parents f4027ef + dae3ea7 commit 22b825a

File tree

2 files changed

+213
-47
lines changed

2 files changed

+213
-47
lines changed

‎manifest.go

Lines changed: 69 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,12 @@ import (
2020
// ManifestName is the manifest file name used by dep.
2121
const ManifestName = "Gopkg.toml"
2222

23+
// Errors
24+
var errInvalidConstraint = errors.New("\"constraint\" must be a TOML array of tables")
25+
var errInvalidOverride = errors.New("\"override\" must be a TOML array of tables")
26+
var errInvalidRequired = errors.New("\"required\" must be a TOML list of strings")
27+
var errInvalidIgnored = errors.New("\"ignored\" must be a TOML list of strings")
28+
2329
// Manifest holds manifest file data and implements gps.RootManifest.
2430
type Manifest struct {
2531
Constraints gps.ProjectConstraints
@@ -44,11 +50,11 @@ type rawProject struct {
4450
}
4551

4652
func validateManifest(s string) ([]error, error) {
47-
var errs []error
53+
var warns []error
4854
// Load the TomlTree from string
4955
tree, err := toml.Load(s)
5056
if err != nil {
51-
return errs, errors.Wrap(err, "Unable to load TomlTree from string")
57+
return warns, errors.Wrap(err, "Unable to load TomlTree from string")
5258
}
5359
// Convert tree to a map
5460
manifest := tree.ToMap()
@@ -61,46 +67,83 @@ func validateManifest(s string) ([]error, error) {
6167
case "metadata":
6268
// Check if metadata is of Map type
6369
if reflect.TypeOf(val).Kind() != reflect.Map {
64-
errs = append(errs, errors.New("metadata should be a TOML table"))
70+
warns = append(warns, errors.New("metadata should be a TOML table"))
6571
}
6672
case "constraint", "override":
73+
valid := true
6774
// Invalid if type assertion fails. Not a TOML array of tables.
6875
if rawProj, ok := val.([]interface{}); ok {
69-
// Iterate through each array of tables
70-
for _, v := range rawProj {
71-
// Check the individual field's key to be valid
72-
for key, value := range v.(map[string]interface{}) {
73-
// Check if the key is valid
74-
switch key {
75-
case "name", "branch", "version", "source":
76-
// valid key
77-
case "revision":
78-
if valueStr, ok := value.(string); ok {
79-
if abbrevRevHash.MatchString(valueStr) {
80-
errs = append(errs, fmt.Errorf("revision %q should not be in abbreviated form", valueStr))
76+
// Check element type. Must be a map. Checking one element would be
77+
// enough because TOML doesn't allow mixing of types.
78+
if reflect.TypeOf(rawProj[0]).Kind() != reflect.Map {
79+
valid = false
80+
}
81+
82+
if valid {
83+
// Iterate through each array of tables
84+
for _, v := range rawProj {
85+
// Check the individual field's key to be valid
86+
for key, value := range v.(map[string]interface{}) {
87+
// Check if the key is valid
88+
switch key {
89+
case "name", "branch", "version", "source":
90+
// valid key
91+
case "revision":
92+
if valueStr, ok := value.(string); ok {
93+
if abbrevRevHash.MatchString(valueStr) {
94+
warns = append(warns, fmt.Errorf("revision %q should not be in abbreviated form", valueStr))
95+
}
8196
}
97+
case "metadata":
98+
// Check if metadata is of Map type
99+
if reflect.TypeOf(value).Kind() != reflect.Map {
100+
warns = append(warns, fmt.Errorf("metadata in %q should be a TOML table", prop))
101+
}
102+
default:
103+
// unknown/invalid key
104+
warns = append(warns, fmt.Errorf("Invalid key %q in %q", key, prop))
82105
}
83-
case "metadata":
84-
// Check if metadata is of Map type
85-
if reflect.TypeOf(value).Kind() != reflect.Map {
86-
errs = append(errs, fmt.Errorf("metadata in %q should be a TOML table", prop))
87-
}
88-
default:
89-
// unknown/invalid key
90-
errs = append(errs, fmt.Errorf("Invalid key %q in %q", key, prop))
91106
}
92107
}
93108
}
94109
} else {
95-
errs = append(errs, fmt.Errorf("%v should be a TOML array of tables", prop))
110+
valid = false
111+
}
112+
113+
if !valid {
114+
if prop == "constraint" {
115+
return warns, errInvalidConstraint
116+
}
117+
if prop == "override" {
118+
return warns, errInvalidOverride
119+
}
96120
}
97121
case "ignored", "required":
122+
valid := true
123+
if rawList, ok := val.([]interface{}); ok {
124+
// Check element type of the array. TOML doesn't let mixing of types in
125+
// array. Checking one element would be enough. Empty array is valid.
126+
if len(rawList) > 0 && reflect.TypeOf(rawList[0]).Kind() != reflect.String {
127+
valid = false
128+
}
129+
} else {
130+
valid = false
131+
}
132+
133+
if !valid {
134+
if prop == "ignored" {
135+
return warns, errInvalidIgnored
136+
}
137+
if prop == "required" {
138+
return warns, errInvalidRequired
139+
}
140+
}
98141
default:
99-
errs = append(errs, fmt.Errorf("Unknown field in manifest: %v", prop))
142+
warns = append(warns, fmt.Errorf("Unknown field in manifest: %v", prop))
100143
}
101144
}
102145

103-
return errs, nil
146+
return warns, nil
104147
}
105148

106149
// readManifest returns a Manifest read from r and a slice of validation warnings.

‎manifest_test.go

Lines changed: 144 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -124,22 +124,89 @@ func TestReadManifestErrors(t *testing.T) {
124124
func TestValidateManifest(t *testing.T) {
125125
cases := []struct {
126126
tomlString string
127-
want []error
127+
wantWarn []error
128+
wantError error
128129
}{
129130
{
130131
tomlString: `
131-
[[constraint]]
132-
name = "github.com/foo/bar"
132+
required = ["github.com/foo/bar"]
133+
`,
134+
wantWarn: []error{},
135+
wantError: nil,
136+
},
137+
{
138+
tomlString: `
139+
required = "github.com/foo/bar"
140+
`,
141+
wantWarn: []error{},
142+
wantError: errInvalidRequired,
143+
},
144+
{
145+
tomlString: `
146+
required = []
147+
`,
148+
wantWarn: []error{},
149+
wantError: nil,
150+
},
151+
{
152+
tomlString: `
153+
required = [1, 2, 3]
154+
`,
155+
wantWarn: []error{},
156+
wantError: errInvalidRequired,
157+
},
158+
{
159+
tomlString: `
160+
[[required]]
161+
name = "foo"
162+
`,
163+
wantWarn: []error{},
164+
wantError: errInvalidRequired,
165+
},
166+
{
167+
tomlString: `
168+
ignored = ["foo"]
169+
`,
170+
wantWarn: []error{},
171+
wantError: nil,
172+
},
173+
{
174+
tomlString: `
175+
ignored = "foo"
176+
`,
177+
wantWarn: []error{},
178+
wantError: errInvalidIgnored,
179+
},
180+
{
181+
tomlString: `
182+
ignored = []
183+
`,
184+
wantWarn: []error{},
185+
wantError: nil,
186+
},
187+
{
188+
tomlString: `
189+
ignored = [1, 2, 3]
190+
`,
191+
wantWarn: []error{},
192+
wantError: errInvalidIgnored,
193+
},
194+
{
195+
tomlString: `
196+
[[ignored]]
197+
name = "foo"
133198
`,
134-
want: []error{},
199+
wantWarn: []error{},
200+
wantError: errInvalidIgnored,
135201
},
136202
{
137203
tomlString: `
138204
[metadata]
139205
authors = "foo"
140206
version = "1.0.0"
141207
`,
142-
want: []error{},
208+
wantWarn: []error{},
209+
wantError: nil,
143210
},
144211
{
145212
tomlString: `
@@ -153,11 +220,12 @@ func TestValidateManifest(t *testing.T) {
153220
name = "github.com/foo/bar"
154221
version = ""
155222
`,
156-
want: []error{
223+
wantWarn: []error{
157224
errors.New("Unknown field in manifest: foo"),
158225
errors.New("Unknown field in manifest: bar"),
159226
errors.New("Unknown field in manifest: version"),
160227
},
228+
wantError: nil,
161229
},
162230
{
163231
tomlString: `
@@ -166,17 +234,66 @@ func TestValidateManifest(t *testing.T) {
166234
[[constraint]]
167235
name = "github.com/foo/bar"
168236
`,
169-
want: []error{errors.New("metadata should be a TOML table")},
237+
wantWarn: []error{errors.New("metadata should be a TOML table")},
238+
wantError: nil,
239+
},
240+
{
241+
tomlString: `
242+
[[constraint]]
243+
name = "github.com/foo/bar"
244+
`,
245+
wantWarn: []error{},
246+
wantError: nil,
247+
},
248+
{
249+
tomlString: `
250+
[[constraint]]
251+
`,
252+
wantWarn: []error{},
253+
wantError: nil,
170254
},
171255
{
172256
tomlString: `
173257
constraint = "foo"
258+
`,
259+
wantWarn: []error{},
260+
wantError: errInvalidConstraint,
261+
},
262+
{
263+
tomlString: `
264+
constraint = ["foo", "bar"]
265+
`,
266+
wantWarn: []error{},
267+
wantError: errInvalidConstraint,
268+
},
269+
{
270+
tomlString: `
271+
[[override]]
272+
name = "github.com/foo/bar"
273+
`,
274+
wantWarn: []error{},
275+
wantError: nil,
276+
},
277+
{
278+
tomlString: `
279+
[[override]]
280+
`,
281+
wantWarn: []error{},
282+
wantError: nil,
283+
},
284+
{
285+
tomlString: `
174286
override = "bar"
175287
`,
176-
want: []error{
177-
errors.New("constraint should be a TOML array of tables"),
178-
errors.New("override should be a TOML array of tables"),
179-
},
288+
wantWarn: []error{},
289+
wantError: errInvalidOverride,
290+
},
291+
{
292+
tomlString: `
293+
override = ["foo", "bar"]
294+
`,
295+
wantWarn: []error{},
296+
wantError: errInvalidOverride,
180297
},
181298
{
182299
tomlString: `
@@ -189,12 +306,13 @@ func TestValidateManifest(t *testing.T) {
189306
[[override]]
190307
nick = "foo"
191308
`,
192-
want: []error{
309+
wantWarn: []error{
193310
errors.New("Invalid key \"location\" in \"constraint\""),
194311
errors.New("Invalid key \"link\" in \"constraint\""),
195312
errors.New("Invalid key \"nick\" in \"override\""),
196313
errors.New("metadata in \"constraint\" should be a TOML table"),
197314
},
315+
wantError: nil,
198316
},
199317
{
200318
tomlString: `
@@ -204,23 +322,26 @@ func TestValidateManifest(t *testing.T) {
204322
[constraint.metadata]
205323
color = "blue"
206324
`,
207-
want: []error{},
325+
wantWarn: []error{},
326+
wantError: nil,
208327
},
209328
{
210329
tomlString: `
211330
[[constraint]]
212331
name = "github.com/foo/bar"
213332
revision = "b86ad16"
214333
`,
215-
want: []error{errors.New("revision \"b86ad16\" should not be in abbreviated form")},
334+
wantWarn: []error{errors.New("revision \"b86ad16\" should not be in abbreviated form")},
335+
wantError: nil,
216336
},
217337
{
218338
tomlString: `
219339
[[constraint]]
220340
name = "foobar.com/hg"
221341
revision = "8d43f8c0b836"
222342
`,
223-
want: []error{errors.New("revision \"8d43f8c0b836\" should not be in abbreviated form")},
343+
wantWarn: []error{errors.New("revision \"8d43f8c0b836\" should not be in abbreviated form")},
344+
wantError: nil,
224345
},
225346
}
226347

@@ -236,19 +357,21 @@ func TestValidateManifest(t *testing.T) {
236357

237358
for _, c := range cases {
238359
errs, err := validateManifest(c.tomlString)
239-
if err != nil {
240-
t.Fatal(err)
360+
361+
// compare validation errors
362+
if err != c.wantError {
363+
t.Fatalf("Manifest errors are not as expected: \n\t(GOT) %v \n\t(WNT) %v", err, c.wantError)
241364
}
242365

243366
// compare length of error slice
244-
if len(errs) != len(c.want) {
245-
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)
367+
if len(errs) != len(c.wantWarn) {
368+
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.wantWarn), c.wantWarn)
246369
}
247370

248371
// check if the expected errors exist in actual errors slice
249372
for _, er := range errs {
250-
if !contains(c.want, er) {
251-
t.Fatalf("Manifest errors are not as expected: \n\t(MISSING) %v\n\t(FROM) %v", er, c.want)
373+
if !contains(c.wantWarn, er) {
374+
t.Fatalf("Manifest errors are not as expected: \n\t(MISSING) %v\n\t(FROM) %v", er, c.wantWarn)
252375
}
253376
}
254377
}

0 commit comments

Comments
 (0)