bug: add a data validation process to avoid merging incorrect operations

8 files changed, 152 insertions, 35 deletions

diff --git a/bug/bug.go b/bug/bug.go
index e548757e..3342ecfa 100644
--- a/bug/bug.go
+++ b/bug/bug.go
@@ -2,13 +2,13 @@ package bug
 
 import (
 	"encoding/json"
-	"errors"
 	"fmt"
 	"strings"
 
 	"github.com/MichaelMure/git-bug/repository"
 	"github.com/MichaelMure/git-bug/util/git"
 	"github.com/MichaelMure/git-bug/util/lamport"
+	"github.com/pkg/errors"
 )
 
 const bugsRefPattern = "refs/bugs/"
@@ -279,31 +279,31 @@ func refsToIds(refs []string) []string {
 	return ids
 }
 
-// IsValid check if the Bug data is valid
-func (bug *Bug) IsValid() bool {
+// Validate check if the Bug data is valid
+func (bug *Bug) Validate() error {
 	// non-empty
 	if len(bug.packs) == 0 && bug.staging.IsEmpty() {
-		return false
+		return fmt.Errorf("bug has no operations")
 	}
 
-	// check if each pack is valid
+	// check if each pack and operations are valid
 	for _, pack := range bug.packs {
-		if !pack.IsValid() {
-			return false
+		if err := pack.Validate(); err != nil {
+			return err
 		}
 	}
 
 	// check if staging is valid if needed
 	if !bug.staging.IsEmpty() {
-		if !bug.staging.IsValid() {
-			return false
+		if err := bug.staging.Validate(); err != nil {
+			return errors.Wrap(err, "staging")
 		}
 	}
 
 	// The very first Op should be a CreateOp
 	firstOp := bug.FirstOp()
 	if firstOp == nil || firstOp.OpType() != CreateOp {
-		return false
+		return fmt.Errorf("first operation should be a Create op")
 	}
 
 	// Check that there is no more CreateOp op
@@ -316,10 +316,10 @@ func (bug *Bug) IsValid() bool {
 	}
 
 	if createCount != 1 {
-		return false
+		return fmt.Errorf("only one Create op allowed")
 	}
 
-	return true
+	return nil
 }
 
 // Append an operation into the staging area, to be committed later
@@ -338,6 +338,10 @@ func (bug *Bug) Commit(repo repository.Repo) error {
 		return fmt.Errorf("can't commit a bug with no pending operation")
 	}
 
+	if err := bug.Validate(); err != nil {
+		return errors.Wrap(err, "can't commit a bug with invalid data")
+	}
+
 	// Write the Ops as a Git blob containing the serialized array
 	hash, err := bug.staging.Write(repo)
 	if err != nil {
diff --git a/bug/bug_actions.go b/bug/bug_actions.go
index 8bda15b7..7f2194ab 100644
--- a/bug/bug_actions.go
+++ b/bug/bug_actions.go
@@ -66,8 +66,8 @@ func MergeAll(repo repository.Repo, remote string) <-chan MergeResult {
 			}
 
 			// Check for error in remote data
-			if !remoteBug.IsValid() {
-				out <- newMergeStatus(MergeStatusInvalid, id, nil)
+			if err := remoteBug.Validate(); err != nil {
+				out <- newMergeInvalidStatus(id, err.Error())
 				continue
 			}
 
@@ -128,12 +128,26 @@ const (
 	MergeStatusNothing
 )
 
-func (ms MergeStatus) String() string {
-	switch ms {
+type MergeResult struct {
+	// Err is set when a terminal error occur in the process
+	Err error
+
+	Id     string
+	Status MergeStatus
+
+	// Only set for invalid status
+	Reason string
+
+	// Not set for invalid status
+	Bug *Bug
+}
+
+func (mr MergeResult) String() string {
+	switch mr.Status {
 	case MergeStatusNew:
 		return "new"
 	case MergeStatusInvalid:
-		return "invalid data"
+		return fmt.Sprintf("invalid data: %s", mr.Reason)
 	case MergeStatusUpdated:
 		return "updated"
 	case MergeStatusNothing:
@@ -143,15 +157,6 @@ func (ms MergeStatus) String() string {
 	}
 }
 
-type MergeResult struct {
-	// Err is set when a terminal error occur in the process
-	Err error
-
-	Id     string
-	Status MergeStatus
-	Bug    *Bug
-}
-
 func newMergeError(err error, id string) MergeResult {
 	return MergeResult{
 		Err: err,
@@ -168,3 +173,11 @@ func newMergeStatus(status MergeStatus, id string, bug *Bug) MergeResult {
 		Bug: bug,
 	}
 }
+
+func newMergeInvalidStatus(id string, reason string) MergeResult {
+	return MergeResult{
+		Id:     id,
+		Status: MergeStatusInvalid,
+		Reason: reason,
+	}
+}
diff --git a/bug/interface.go b/bug/interface.go
index c397bfbc..72dee61c 100644
--- a/bug/interface.go
+++ b/bug/interface.go
@@ -12,8 +12,8 @@ type Interface interface {
 	// HumanId return the Bug identifier truncated for human consumption
 	HumanId() string
 
-	// IsValid check if the Bug data is valid
-	IsValid() bool
+	// Validate check if the Bug data is valid
+	Validate() error
 
 	// Append an operation into the staging area, to be committed later
 	Append(op Operation)
diff --git a/bug/label.go b/bug/label.go
index b19a980f..b73222dd 100644
--- a/bug/label.go
+++ b/bug/label.go
@@ -3,6 +3,9 @@ package bug
 import (
 	"fmt"
 	"io"
+	"strings"
+
+	"github.com/MichaelMure/git-bug/util/text"
 )
 
 type Label string
@@ -27,3 +30,21 @@ func (l *Label) UnmarshalGQL(v interface{}) error {
 func (l Label) MarshalGQL(w io.Writer) {
 	w.Write([]byte(`"` + l.String() + `"`))
 }
+
+func (l Label) Validate() error {
+	str := string(l)
+
+	if text.Empty(str) {
+		return fmt.Errorf("empty")
+	}
+
+	if strings.Contains(str, "\n") {
+		return fmt.Errorf("should be a single line")
+	}
+
+	if !text.Safe(str) {
+		return fmt.Errorf("not fully printable")
+	}
+
+	return nil
+}
diff --git a/bug/operation.go b/bug/operation.go
index 6a8aa0cd..b148141c 100644
--- a/bug/operation.go
+++ b/bug/operation.go
@@ -2,6 +2,9 @@ package bug
 
 import (
 	"github.com/MichaelMure/git-bug/util/git"
+	"github.com/pkg/errors"
+
+	"fmt"
 	"time"
 )
 
@@ -25,13 +28,14 @@ type Operation interface {
 	Time() time.Time
 	// GetUnixTime return the unix timestamp when the operation was added
 	GetUnixTime() int64
-	// Apply the operation to a Snapshot to create the final state
-	Apply(snapshot Snapshot) Snapshot
+	// GetAuthor return the author of the operation
+	GetAuthor() Person
 	// GetFiles return the files needed by this operation
 	GetFiles() []git.Hash
-
-	// TODO: data validation (ex: a title is a single line)
-	// Validate() bool
+	// Apply the operation to a Snapshot to create the final state
+	Apply(snapshot Snapshot) Snapshot
+	// Validate check if the operation is valid (ex: a title is a single line)
+	Validate() error
 }
 
 // OpBase implement the common code for all operations
@@ -65,7 +69,35 @@ func (op OpBase) GetUnixTime() int64 {
 	return op.UnixTime
 }
 
+// GetAuthor return the author of the operation
+func (op OpBase) GetAuthor() Person {
+	return op.Author
+}
+
 // GetFiles return the files needed by this operation
 func (op OpBase) GetFiles() []git.Hash {
 	return nil
 }
+
+// Validate check the OpBase for errors
+func OpBaseValidate(op Operation, opType OperationType) error {
+	if op.OpType() != opType {
+		return fmt.Errorf("incorrect operation type (expected: %v, actual: %v)", opType, op.OpType())
+	}
+
+	if op.GetUnixTime() == 0 {
+		return fmt.Errorf("time not set")
+	}
+
+	if err := op.GetAuthor().Validate(); err != nil {
+		return errors.Wrap(err, "author")
+	}
+
+	for _, hash := range op.GetFiles() {
+		if !hash.IsValid() {
+			return fmt.Errorf("file with invalid hash %v", hash)
+		}
+	}
+
+	return nil
+}
diff --git a/bug/operation_pack.go b/bug/operation_pack.go
index fe26952f..03d538d5 100644
--- a/bug/operation_pack.go
+++ b/bug/operation_pack.go
@@ -7,6 +7,7 @@ import (
 
 	"github.com/MichaelMure/git-bug/repository"
 	"github.com/MichaelMure/git-bug/util/git"
+	"github.com/pkg/errors"
 )
 
 const formatVersion = 1
@@ -24,8 +25,10 @@ type OperationPack struct {
 	commitHash git.Hash
 }
 
+// hold the different operation type to instantiate to parse JSON
 var operations map[OperationType]reflect.Type
 
+// Register will register a new type of Operation to be able to parse the corresponding JSON
 func Register(t OperationType, op interface{}) {
 	if operations == nil {
 		operations = make(map[OperationType]reflect.Type)
@@ -96,8 +99,18 @@ func (opp *OperationPack) IsEmpty() bool {
 }
 
 // IsValid tell if the OperationPack is considered valid
-func (opp *OperationPack) IsValid() bool {
-	return !opp.IsEmpty()
+func (opp *OperationPack) Validate() error {
+	if opp.IsEmpty() {
+		return fmt.Errorf("empty")
+	}
+
+	for _, op := range opp.Operations {
+		if err := op.Validate(); err != nil {
+			return errors.Wrap(err, "op")
+		}
+	}
+
+	return nil
 }
 
 // Write will serialize and store the OperationPack as a git blob and return
diff --git a/bug/person.go b/bug/person.go
index d9f1108b..9fa86804 100644
--- a/bug/person.go
+++ b/bug/person.go
@@ -2,9 +2,11 @@ package bug
 
 import (
 	"errors"
+	"fmt"
 	"strings"
 
 	"github.com/MichaelMure/git-bug/repository"
+	"github.com/MichaelMure/git-bug/util/text"
 )
 
 type Person struct {
@@ -37,3 +39,27 @@ func GetUser(repo repository.Repo) (Person, error) {
 func (p Person) Match(query string) bool {
 	return strings.Contains(strings.ToLower(p.Name), strings.ToLower(query))
 }
+
+func (p Person) Validate() error {
+	if text.Empty(p.Name) {
+		return fmt.Errorf("name is not set")
+	}
+
+	if strings.Contains(p.Name, "\n") {
+		return fmt.Errorf("name should be a single line")
+	}
+
+	if !text.Safe(p.Name) {
+		return fmt.Errorf("name is not fully printable")
+	}
+
+	if strings.Contains(p.Email, "\n") {
+		return fmt.Errorf("email should be a single line")
+	}
+
+	if !text.Safe(p.Email) {
+		return fmt.Errorf("email is not fully printable")
+	}
+
+	return nil
+}
diff --git a/bug/status.go b/bug/status.go
index f15924e2..737c8d31 100644
--- a/bug/status.go
+++ b/bug/status.go
@@ -47,3 +47,11 @@ func StatusFromString(str string) (Status, error) {
 		return 0, fmt.Errorf("unknow status")
 	}
 }
+
+func (s Status) Validate() error {
+	if s != OpenStatus && s != ClosedStatus {
+		return fmt.Errorf("invalid")
+	}
+
+	return nil
+}