Test and document behavior of sumfiles with duplicate entries

Add tests that verify the `ghasum update` and `ghasum verify` commands both reject sumfiles with duplicate (non-header) entries. Additionally test that `ghasum update -force` fixes duplicate (non-header) entries. To support this change some test re-organization was in order. The pre- existing "invalid sumfile" tests were specifically testing against a sumfile with a syntax error, hence these have been renamed (given the new tests are also for an "invalid sumfile"). Additionally, the implementation for sumfile parsing has been updated to improve the error messages produced, which are asserted in the new and changed tests. Similarly, the `SPECIFICATION.md` is updated to make explicit that duplicate sumfile entries should be rejected. As an aside, this uncovered a off-by-one bug in the line number formula in the implementation for sumfile parsing and corresponding incorrectly implemented unit test - both of which are fixed here.
chains-project · Mar 21, 2024 · ac1b345 · ac1b345
1 parent 8dc7b4f
commit ac1b345
Show file tree

Hide file tree

Showing 7 changed files with 89 additions and 33 deletions.
diff --git a/SPECIFICATION.md b/SPECIFICATION.md
@@ -120,6 +120,10 @@ version 1
 <body>
 ```
 
+Every entry in the `<body>` of the sumfile must have a unique identifier. If two
+entries have the same identifier the sumfile must be rejected and the program
+exit with a non-zero exit code.
+
 ### Version 1
 
 Sumfile version 1 expects at least one header, namely `version 1`. Any other

diff --git a/internal/sumfile/errors.go b/internal/sumfile/errors.go
@@ -20,11 +20,15 @@ var (
 	// ErrCorrupted is the error when a checksum file is corrupted.
 	ErrCorrupted = errors.New("checksums are corrupted")
 
+	// ErrDuplicate is the error when a checksum file contains two entries for the
+	// same identifier.
+	ErrDuplicate = errors.New("duplicate entry found")
+
 	// ErrHeaders is the error for when checksum headers are invalid.
 	ErrHeaders = errors.New("checksum headers are invalid")
 
-	// ErrInvalid is the error for when checksums are invalid.
-	ErrInvalid = errors.New("checksums are invalid")
+	// ErrMissing is the error when an entry is missing an id (part) or checksums.
+	ErrMissing = errors.New("missing id or checksum")
 
 	// ErrSyntax is the error when a checksum file has a syntax error.
 	ErrSyntax = errors.New("syntax error")

diff --git a/internal/sumfile/version1.go b/internal/sumfile/version1.go
@@ -27,8 +27,7 @@ func decodeV1(lines []string) ([]Entry, error) {
 		// split "line" into "id[@id..]" "sum"
 		j := strings.IndexRune(line, ' ')
 		if j <= 0 || j >= len(line)-1 {
-			err := fmt.Errorf("syntax error on line %d", i+2)
-			return nil, errors.Join(ErrCorrupted, err)
+			return nil, fmt.Errorf("%v on line %d", ErrSyntax, i+3)
 		}
 
 		entries[i] = Entry{
@@ -37,16 +36,16 @@ func decodeV1(lines []string) ([]Entry, error) {
 		}
 	}
 
-	if !validV1(entries) {
-		return nil, ErrInvalid
+	if err := validV1(entries); err != nil {
+		return nil, errors.Join(ErrCorrupted, err)
 	}
 
 	return entries, nil
 }
 
 func encodeV1(entries []Entry) (string, error) {
-	if !validV1(entries) {
-		return "", ErrInvalid
+	if err := validV1(entries); err != nil {
+		return "", errors.Join(ErrCorrupted, err)
 	}
 
 	var sb strings.Builder
@@ -71,20 +70,24 @@ func encodeV1(entries []Entry) (string, error) {
 	return strings.Join(lines, ""), nil
 }
 
-func validV1(entries []Entry) bool {
-	if hasDuplicates(entries) || hasMissing(entries) {
-		return false
+func validV1(entries []Entry) error {
+	if hasDuplicates(entries) {
+		return ErrDuplicate
+	}
+
+	if hasMissing(entries) {
+		return ErrMissing
 	}
 
 	for _, entry := range entries {
 		if strings.ContainsAny(entry.Checksum, "\n ") {
-			return false
+			return ErrSyntax
 		}
 
 		if strings.ContainsAny(strings.Join(entry.ID, ""), "\n @") {
-			return false
+			return ErrSyntax
 		}
 	}
 
-	return true
+	return nil
 }
diff --git a/internal/sumfile/version1_test.go b/internal/sumfile/version1_test.go
@@ -15,7 +15,6 @@
 package sumfile
 
 import (
-	"errors"
 	"fmt"
 	"slices"
 	"strings"
@@ -27,7 +26,7 @@ func TestVersion1(t *testing.T) {
 	t.Parallel()
 
 	correct := func(entries []Entry) bool {
-		if !validV1(entries) {
+		if err := validV1(entries); err != nil {
 			return true
 		}
 
@@ -47,7 +46,7 @@ func TestVersion1(t *testing.T) {
 	}
 
 	decodable := func(entries []Entry) bool {
-		if !validV1(entries) {
+		if err := validV1(entries); err != nil {
 			return true
 		}
 
@@ -65,7 +64,7 @@ func TestVersion1(t *testing.T) {
 	deterministic := func(entries []Entry) bool {
 		got1, err1 := encodeV1(entries)
 		got2, err2 := encodeV1(entries)
-		return got1 == got2 && errors.Is(err1, err2)
+		return got1 == got2 && ((err1 == nil) == (err2 == nil))
 	}
 
 	if err := quick.Check(deterministic, nil); err != nil {
@@ -191,7 +190,7 @@ func TestDecodeV1(t *testing.T) {
 					t.Fatal("Unexpected success")
 				}
 
-				if got, want := err.Error(), fmt.Sprintf("line %d", tc.want); strings.Contains(got, want) {
+				if got, want := err.Error(), fmt.Sprintf("line %d", tc.want); !strings.Contains(got, want) {
 					t.Errorf("Incorrect line number (got %q, want %q)", got, want)
 				}
 			})

diff --git a/testdata/update/error.txtar b/testdata/update/error.txtar
@@ -10,11 +10,18 @@ stderr 'ghasum has not yet been initialized'
 stderr 'an unexpected error occurred'
 stderr 'ghasum has not yet been initialized'
 
-# Invalid sumfile
-! exec ghasum update invalid-sumfile/
+# Sumfile with syntax error
+! exec ghasum verify sumfile-syntax/
 ! stdout 'Ok'
 stderr 'an unexpected error occurred'
-stderr 'could not decode the checksum file'
+stderr 'syntax error on line 3'
+
+# Sumfile with duplicate entries
+! exec ghasum verify sumfile-duplicate/
+! stdout 'Ok'
+stderr 'an unexpected error occurred'
+stderr 'checksums are corrupted'
+stderr 'duplicate entry found'
 
 # Invalid workflow
 ! exec ghasum update invalid-workflow/
@@ -29,10 +36,6 @@ stderr '.github/workflows/workflow.yml'
 stderr 'an unexpected error occurred'
 stderr 'no such file or directory'
 
--- invalid-sumfile/.github/workflows/gha.sum --
-version 1
-
-this-action/is-missing@a-checksum
 -- invalid-workflow/.github/workflows/gha.sum --
 version 1
 
@@ -50,6 +53,15 @@ jobs:
     uses: actions/checkout@v4
 -- no-actions/.keep --
 This file exists to create a repo that does not use Github Actions.
+-- sumfile-duplicate/.github/workflows/gha.sum --
+version 1
+
+actions/checkout@v4 rfcqiYqRAnhqMjwnw/oJp2lqI5zRjHTtu2vQ9/ss4x0=
+actions/checkout@v4 oJp2lqI5zRjHTtu2vQ9/rfcqiYqRAnhqMjwnw/ss4x0=
+-- sumfile-syntax/.github/workflows/gha.sum --
+version 1
+
+this-action/is-missing@a-checksum
 -- uninitialized/.github/workflows/workflow.yml --
 name: Example workflow
 on: [push]

diff --git a/testdata/update/force.txtar b/testdata/update/force.txtar
@@ -4,6 +4,12 @@ stdout 'Ok'
 ! stderr .
 cmp entries/.github/workflows/gha.sum .want/gha.sum
 
+# Duplicate entries
+exec ghasum update -cache .cache/ -force duplicate/
+stdout 'Ok'
+! stderr .
+cmp duplicate/.github/workflows/gha.sum .want/gha.sum
+
 # Error in headers
 exec ghasum update -cache .cache/ -force headers/
 stdout 'Ok'
@@ -28,6 +34,22 @@ stdout 'Ok'
 ! stderr .
 cmp no-version/.github/workflows/gha.sum .want/gha.sum
 
+-- duplicate/.github/workflows/gha.sum --
+version 1
+
+actions/[email protected] KsR9XQGH7ydTl01vlD8pIZrXhkzXyjcnzhmP+/KaJZI=
+actions/[email protected] KaJZI=/KsR9XQGH7ydTl01vlD8pIZrXhkzXyjcnzhmP+
+-- duplicate/.github/workflows/workflow.yml --
+name: Example workflow
+on: [push]
+
+jobs:
+  example:
+    name: example
+    runs-on: ubuntu-22.04
+    steps:
+    - name: Checkout repository
+      uses: actions/[email protected]
 -- entries/.github/workflows/gha.sum --
 version 1
 

diff --git a/testdata/verify/error.txtar b/testdata/verify/error.txtar
@@ -10,11 +10,18 @@ stderr 'ghasum has not yet been initialized'
 stderr 'an unexpected error occurred'
 stderr 'ghasum has not yet been initialized'
 
-# Invalid sumfile
-! exec ghasum verify invalid-sumfile/
+# Sumfile with syntax error
+! exec ghasum verify sumfile-syntax/
 ! stdout 'Ok'
 stderr 'an unexpected error occurred'
-stderr 'could not decode the checksum file'
+stderr 'syntax error on line 3'
+
+# Sumfile with duplicate entries
+! exec ghasum verify sumfile-duplicate/
+! stdout 'Ok'
+stderr 'an unexpected error occurred'
+stderr 'checksums are corrupted'
+stderr 'duplicate entry found'
 
 # Invalid workflow
 ! exec ghasum verify invalid-workflow/
@@ -56,10 +63,6 @@ jobs:
     steps:
     - name: Checkout repository
       uses: actions/checkout@v4
--- invalid-sumfile/.github/workflows/gha.sum --
-version 1
-
-this-action/is-missing@a-checksum
 -- invalid-workflow/.github/workflows/gha.sum --
 version 1
 
@@ -77,6 +80,15 @@ jobs:
     uses: actions/checkout@v4
 -- no-actions/.keep --
 This file exists to create a repo that does not use Github Actions.
+-- sumfile-duplicate/.github/workflows/gha.sum --
+version 1
+
+actions/checkout@v4 rfcqiYqRAnhqMjwnw/oJp2lqI5zRjHTtu2vQ9/ss4x0=
+actions/checkout@v4 oJp2lqI5zRjHTtu2vQ9/rfcqiYqRAnhqMjwnw/ss4x0=
+-- sumfile-syntax/.github/workflows/gha.sum --
+version 1
+
+this-action/is-missing@a-checksum
 -- uninitialized/.github/workflows/workflow.yml --
 name: Example workflow
 on: [push]