asserts: require components in validation sets to have revisions if t…

…heir associated snap has one (#14520)

asserts/validation_set.go

@@ -98,15 +98,15 @@ func (s *ValidationSetSnap) ID() string {
 }
 
 func checkValidationSetSnap(snap map[string]interface{}) (*ValidationSetSnap, error) {
-	name, err := checkNotEmptyStringWhat(snap, "name", "of snap")
+	snapName, err := checkNotEmptyStringWhat(snap, "name", "of snap")
 	if err != nil {
 		return nil, err
 	}
-	if err := naming.ValidateSnap(name); err != nil {
-		return nil, fmt.Errorf("invalid snap name %q", name)
+	if err := naming.ValidateSnap(snapName); err != nil {
+		return nil, fmt.Errorf("invalid snap name %q", snapName)
 	}
 
-	what := fmt.Sprintf("of snap %q", name)
+	what := fmt.Sprintf("of snap %q", snapName)
 
 	snapID, err := checkStringMatchesWhat(snap, "id", what, naming.ValidSnapID)
 	if err != nil {
@@ -130,22 +130,22 @@ func checkValidationSetSnap(snap map[string]interface{}) (*ValidationSetSnap, er
 		return nil, fmt.Errorf(`cannot specify revision %s at the same time as stating its presence is invalid`, what)
 	}
 
-	components, err := checkValidationSetComponents(snap, what)
+	components, err := checkValidationSetComponents(snapName, snap, snapRevision)
 	if err != nil {
 		return nil, err
 	}
 
 	return &ValidationSetSnap{
-		Name:       name,
+		Name:       snapName,
 		SnapID:     snapID,
 		Presence:   presence,
 		Revision:   snapRevision,
 		Components: components,
 	}, nil
 }
 
-func checkValidationSetComponents(snap map[string]interface{}, what string) (map[string]ValidationSetComponent, error) {
-	mapping, err := checkMapWhat(snap, "components", what)
+func checkValidationSetComponents(snapName string, snap map[string]interface{}, snapRevision int) (map[string]ValidationSetComponent, error) {
+	mapping, err := checkMapWhat(snap, "components", fmt.Sprintf("of snap %q", snapName))
 	if err != nil {
 		return nil, errors.New(`"components" field in "snaps" header must be a map`)
 	}
@@ -166,7 +166,7 @@ func checkValidationSetComponents(snap map[string]interface{}, what string) (map
 			return nil, errors.New(`each field in "components" map must be either a map or a string`)
 		}
 
-		component, err := checkValidationSetComponent(parsed, name)
+		component, err := checkValidationSetComponent(name, parsed, snapName, snapRevision)
 		if err != nil {
 			return nil, err
 		}
@@ -176,12 +176,12 @@ func checkValidationSetComponents(snap map[string]interface{}, what string) (map
 	return components, nil
 }
 
-func checkValidationSetComponent(comp map[string]interface{}, name string) (ValidationSetComponent, error) {
-	if err := naming.ValidateSnap(name); err != nil {
-		return ValidationSetComponent{}, fmt.Errorf("invalid component name %q", name)
+func checkValidationSetComponent(compName string, comp map[string]interface{}, snapName string, snapRevision int) (ValidationSetComponent, error) {
+	if err := naming.ValidateSnap(compName); err != nil {
+		return ValidationSetComponent{}, fmt.Errorf("invalid component name %q", compName)
 	}
 
-	what := fmt.Sprintf("of component %q", name)
+	what := fmt.Sprintf("of component %q", naming.NewComponentRef(snapName, compName))
 
 	presence, err := checkPresence(comp, what, validValidationSetSnapPresences)
 	if err != nil {
@@ -194,7 +194,15 @@ func checkValidationSetComponent(comp map[string]interface{}, name string) (Vali
 	}
 
 	if revision != 0 && presence == PresenceInvalid {
-		return ValidationSetComponent{}, fmt.Errorf(`cannot specify component revision %s at the same time as stating its presence is invalid`, what)
+		return ValidationSetComponent{}, fmt.Errorf("cannot specify component revision %s at the same time as stating its presence is invalid", what)
+	}
+
+	if snapRevision != 0 && revision == 0 {
+		return ValidationSetComponent{}, fmt.Errorf("must specify revision %s since its associated snap specifies a revision", what)
+	}
+
+	if snapRevision == 0 && revision != 0 {
+		return ValidationSetComponent{}, fmt.Errorf("cannot specify revision %s if its associated snap does not specify a revision", what)
 	}
 
 	return ValidationSetComponent{

asserts/validation_set_test.go

@@ -127,13 +127,15 @@ func (vss *validationSetSuite) TestDecodeInvalid(c *C) {
 		{"presence: optional\n", "presence: no\n", `presence of snap "baz-linux" must be one of required\|optional\|invalid`},
 		{"revision: 99\n", "revision: 0\n", `"revision" of snap "baz-linux" must be >=1: 0`},
 		{"presence: optional\n", "presence: invalid\n", `cannot specify revision of snap "baz-linux" at the same time as stating its presence is invalid`},
-		{"OTHER", "    components:\n      comp: no\n", `presence of component "comp" must be one of required\|optional\|invalid`},
-		{"OTHER", "    components:\n      comp:\n        revision: 1\n        presence: invalid\n", `cannot specify component revision of component "comp" at the same time as stating its presence is invalid`},
+		{"OTHER", "    components:\n      comp: no\n", `presence of component "baz-linux\+comp" must be one of required\|optional\|invalid`},
+		{"OTHER", "    components:\n      comp:\n        revision: 1\n        presence: invalid\n", `cannot specify component revision of component "baz-linux\+comp" at the same time as stating its presence is invalid`},
 		{"OTHER", "    components:\n      c: optional\n", `invalid component name "c"`},
-		{"OTHER", "    components:\n      comp:\n        revision: 1\n", `"presence" of component "comp" is mandatory`},
+		{"OTHER", "    components:\n      comp:\n        revision: 1\n", `"presence" of component "baz-linux\+comp" is mandatory`},
 		{"OTHER", "    components:\n      comp:\n        - test\n", `each field in "components" map must be either a map or a string`},
-		{"OTHER", "    components:\n      comp:\n        revision: -1\n        presence: optional\n", `"revision" of component "comp" must be >=1: -1`},
+		{"OTHER", "    components:\n      comp:\n        revision: -1\n        presence: optional\n", `"revision" of component "baz-linux\+comp" must be >=1: -1`},
 		{"OTHER", "    components: some-string", `"components" field in "snaps" header must be a map`},
+		{"OTHER", "    components:\n      comp:\n        presence: optional\n", `must specify revision of component "baz-linux\+comp" since its associated snap specifies a revision`},
+		{"OTHER", "  -\n    name: foo-linux\n    id: foolinuxidididididididididididid\n    components:\n      comp:\n        revision: 1\n        presence: optional\n", `cannot specify revision of component "foo-linux\+comp" if its associated snap does not specify a revision`},
 	}
 
 	for _, test := range invalidTests {
@@ -178,10 +180,15 @@ func (vss *validationSetSuite) TestSnapComponents(c *C) {
 	encoded := strings.Replace(validationSetExample, "TSLINE", vss.tsLine, 1)
 
 	const components = `    components:
-      string-only: optional
       with-revision:
         revision: 10
         presence: required
+  -
+    name: foo-linux
+    id: foolinuxidididididididididididid
+    presence: optional
+    components:
+      string-only: optional
       no-revision:
         presence: invalid
 `
@@ -194,17 +201,18 @@ func (vss *validationSetSuite) TestSnapComponents(c *C) {
 
 	valset := a.(*asserts.ValidationSet)
 	snaps := valset.Snaps()
-	c.Assert(snaps, HasLen, 1)
-	c.Assert(snaps[0].Components, HasLen, 3)
+	c.Assert(snaps, HasLen, 2)
 
 	c.Check(snaps[0].Components, DeepEquals, map[string]asserts.ValidationSetComponent{
-		"string-only": {
-			Presence: asserts.PresenceOptional,
-		},
 		"with-revision": {
 			Presence: asserts.PresenceRequired,
 			Revision: 10,
 		},
+	})
+	c.Check(snaps[1].Components, DeepEquals, map[string]asserts.ValidationSetComponent{
+		"string-only": {
+			Presence: asserts.PresenceOptional,
+		},
 		"no-revision": {
 			Presence: asserts.PresenceInvalid,
 		},