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

6 files changed, 219 insertions, 6 deletions

diff --git a/operations/add_comment.go b/operations/add_comment.go
index 8c4b8b9c..01572eb7 100644
--- a/operations/add_comment.go
+++ b/operations/add_comment.go
@@ -1,8 +1,11 @@
 package operations
 
 import (
+	"fmt"
+
 	"github.com/MichaelMure/git-bug/bug"
 	"github.com/MichaelMure/git-bug/util/git"
+	"github.com/MichaelMure/git-bug/util/text"
 )
 
 // AddCommentOperation will add a new comment in the bug
@@ -33,6 +36,22 @@ func (op AddCommentOperation) GetFiles() []git.Hash {
 	return op.Files
 }
 
+func (op AddCommentOperation) Validate() error {
+	if err := bug.OpBaseValidate(op, bug.AddCommentOp); err != nil {
+		return err
+	}
+
+	if text.Empty(op.Message) {
+		return fmt.Errorf("message is empty")
+	}
+
+	if !text.Safe(op.Message) {
+		return fmt.Errorf("message is not fully printable")
+	}
+
+	return nil
+}
+
 func NewAddCommentOp(author bug.Person, message string, files []git.Hash) AddCommentOperation {
 	return AddCommentOperation{
 		OpBase:  bug.NewOpBase(bug.AddCommentOp, author),
@@ -42,11 +61,15 @@ func NewAddCommentOp(author bug.Person, message string, files []git.Hash) AddCom
 }
 
 // Convenience function to apply the operation
-func Comment(b bug.Interface, author bug.Person, message string) {
-	CommentWithFiles(b, author, message, nil)
+func Comment(b bug.Interface, author bug.Person, message string) error {
+	return CommentWithFiles(b, author, message, nil)
 }
 
-func CommentWithFiles(b bug.Interface, author bug.Person, message string, files []git.Hash) {
+func CommentWithFiles(b bug.Interface, author bug.Person, message string, files []git.Hash) error {
 	addCommentOp := NewAddCommentOp(author, message, files)
+	if err := addCommentOp.Validate(); err != nil {
+		return err
+	}
 	b.Append(addCommentOp)
+	return nil
 }
diff --git a/operations/create.go b/operations/create.go
index a671171e..efd4492f 100644
--- a/operations/create.go
+++ b/operations/create.go
@@ -1,8 +1,12 @@
 package operations
 
 import (
+	"fmt"
+	"strings"
+
 	"github.com/MichaelMure/git-bug/bug"
 	"github.com/MichaelMure/git-bug/util/git"
+	"github.com/MichaelMure/git-bug/util/text"
 )
 
 // CreateOperation define the initial creation of a bug
@@ -34,6 +38,30 @@ func (op CreateOperation) GetFiles() []git.Hash {
 	return op.Files
 }
 
+func (op CreateOperation) Validate() error {
+	if err := bug.OpBaseValidate(op, bug.CreateOp); err != nil {
+		return err
+	}
+
+	if text.Empty(op.Title) {
+		return fmt.Errorf("title is empty")
+	}
+
+	if strings.Contains(op.Title, "\n") {
+		return fmt.Errorf("title should be a single line")
+	}
+
+	if !text.Safe(op.Title) {
+		return fmt.Errorf("title is not fully printable")
+	}
+
+	if !text.Safe(op.Message) {
+		return fmt.Errorf("message is not fully printable")
+	}
+
+	return nil
+}
+
 func NewCreateOp(author bug.Person, title, message string, files []git.Hash) CreateOperation {
 	return CreateOperation{
 		OpBase:  bug.NewOpBase(bug.CreateOp, author),
@@ -51,6 +79,11 @@ func Create(author bug.Person, title, message string) (*bug.Bug, error) {
 func CreateWithFiles(author bug.Person, title, message string, files []git.Hash) (*bug.Bug, error) {
 	newBug := bug.NewBug()
 	createOp := NewCreateOp(author, title, message, files)
+
+	if err := createOp.Validate(); err != nil {
+		return nil, err
+	}
+
 	newBug.Append(createOp)
 
 	return newBug, nil
diff --git a/operations/label_change.go b/operations/label_change.go
index a3cc9898..507651df 100644
--- a/operations/label_change.go
+++ b/operations/label_change.go
@@ -5,6 +5,7 @@ import (
 	"sort"
 
 	"github.com/MichaelMure/git-bug/bug"
+	"github.com/pkg/errors"
 )
 
 var _ bug.Operation = LabelChangeOperation{}
@@ -49,6 +50,30 @@ AddLoop:
 	return snapshot
 }
 
+func (op LabelChangeOperation) Validate() error {
+	if err := bug.OpBaseValidate(op, bug.LabelChangeOp); err != nil {
+		return err
+	}
+
+	for _, l := range op.Added {
+		if err := l.Validate(); err != nil {
+			return errors.Wrap(err, "added label")
+		}
+	}
+
+	for _, l := range op.Removed {
+		if err := l.Validate(); err != nil {
+			return errors.Wrap(err, "removed label")
+		}
+	}
+
+	if len(op.Added)+len(op.Removed) <= 0 {
+		return fmt.Errorf("no label change")
+	}
+
+	return nil
+}
+
 func NewLabelChangeOperation(author bug.Person, added, removed []bug.Label) LabelChangeOperation {
 	return LabelChangeOperation{
 		OpBase:  bug.NewOpBase(bug.LabelChangeOp, author),
@@ -108,6 +133,10 @@ func ChangeLabels(b bug.Interface, author bug.Person, add, remove []string) ([]L
 
 	labelOp := NewLabelChangeOperation(author, added, removed)
 
+	if err := labelOp.Validate(); err != nil {
+		return nil, err
+	}
+
 	b.Append(labelOp)
 
 	return results, nil
diff --git a/operations/operations_test.go b/operations/operations_test.go
new file mode 100644
index 00000000..6fba7917
--- /dev/null
+++ b/operations/operations_test.go
@@ -0,0 +1,69 @@
+package operations
+
+import (
+	"testing"
+
+	"github.com/MichaelMure/git-bug/bug"
+	"github.com/MichaelMure/git-bug/util/git"
+)
+
+func TestValidate(t *testing.T) {
+	rene := bug.Person{
+		Name:  "René Descartes",
+		Email: "rene@descartes.fr",
+	}
+
+	good := []bug.Operation{
+		NewCreateOp(rene, "title", "message", nil),
+		NewSetTitleOp(rene, "title2", "title1"),
+		NewAddCommentOp(rene, "message2", nil),
+		NewSetStatusOp(rene, bug.ClosedStatus),
+		NewLabelChangeOperation(rene, []bug.Label{"added"}, []bug.Label{"removed"}),
+	}
+
+	for _, op := range good {
+		if err := op.Validate(); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	bad := []bug.Operation{
+		// opbase
+		NewSetStatusOp(bug.Person{Name: "", Email: "rene@descartes.fr"}, bug.ClosedStatus),
+		NewSetStatusOp(bug.Person{Name: "René Descartes\u001b", Email: "rene@descartes.fr"}, bug.ClosedStatus),
+		NewSetStatusOp(bug.Person{Name: "René Descartes", Email: "rene@descartes.fr\u001b"}, bug.ClosedStatus),
+		NewSetStatusOp(bug.Person{Name: "René \nDescartes", Email: "rene@descartes.fr"}, bug.ClosedStatus),
+		NewSetStatusOp(bug.Person{Name: "René Descartes", Email: "rene@\ndescartes.fr"}, bug.ClosedStatus),
+		CreateOperation{OpBase: bug.OpBase{
+			Author:        rene,
+			UnixTime:      0,
+			OperationType: bug.CreateOp,
+		},
+			Title:   "title",
+			Message: "message",
+		},
+
+		NewCreateOp(rene, "multi\nline", "message", nil),
+		NewCreateOp(rene, "title", "message", []git.Hash{git.Hash("invalid")}),
+		NewCreateOp(rene, "title\u001b", "message", nil),
+		NewCreateOp(rene, "title", "message\u001b", nil),
+		NewSetTitleOp(rene, "multi\nline", "title1"),
+		NewSetTitleOp(rene, "title", "multi\nline"),
+		NewSetTitleOp(rene, "title\u001b", "title2"),
+		NewSetTitleOp(rene, "title", "title2\u001b"),
+		NewAddCommentOp(rene, "", nil),
+		NewAddCommentOp(rene, "message\u001b", nil),
+		NewAddCommentOp(rene, "message", []git.Hash{git.Hash("invalid")}),
+		NewSetStatusOp(rene, 1000),
+		NewSetStatusOp(rene, 0),
+		NewLabelChangeOperation(rene, []bug.Label{}, []bug.Label{}),
+		NewLabelChangeOperation(rene, []bug.Label{"multi\nline"}, []bug.Label{}),
+	}
+
+	for i, op := range bad {
+		if err := op.Validate(); err == nil {
+			t.Fatal("validation should have failed", i, op)
+		}
+	}
+
+}
diff --git a/operations/set_status.go b/operations/set_status.go
index 37ebac10..07dcf208 100644
--- a/operations/set_status.go
+++ b/operations/set_status.go
@@ -2,6 +2,7 @@ package operations
 
 import (
 	"github.com/MichaelMure/git-bug/bug"
+	"github.com/pkg/errors"
 )
 
 // SetStatusOperation will change the status of a bug
@@ -19,6 +20,18 @@ func (op SetStatusOperation) Apply(snapshot bug.Snapshot) bug.Snapshot {
 	return snapshot
 }
 
+func (op SetStatusOperation) Validate() error {
+	if err := bug.OpBaseValidate(op, bug.SetStatusOp); err != nil {
+		return err
+	}
+
+	if err := op.Status.Validate(); err != nil {
+		return errors.Wrap(err, "status")
+	}
+
+	return nil
+}
+
 func NewSetStatusOp(author bug.Person, status bug.Status) SetStatusOperation {
 	return SetStatusOperation{
 		OpBase: bug.NewOpBase(bug.SetStatusOp, author),
@@ -27,13 +40,21 @@ func NewSetStatusOp(author bug.Person, status bug.Status) SetStatusOperation {
 }
 
 // Convenience function to apply the operation
-func Open(b bug.Interface, author bug.Person) {
+func Open(b bug.Interface, author bug.Person) error {
 	op := NewSetStatusOp(author, bug.OpenStatus)
+	if err := op.Validate(); err != nil {
+		return err
+	}
 	b.Append(op)
+	return nil
 }
 
 // Convenience function to apply the operation
-func Close(b bug.Interface, author bug.Person) {
+func Close(b bug.Interface, author bug.Person) error {
 	op := NewSetStatusOp(author, bug.ClosedStatus)
+	if err := op.Validate(); err != nil {
+		return err
+	}
 	b.Append(op)
+	return nil
 }
diff --git a/operations/set_title.go b/operations/set_title.go
index 7aa76268..46addce6 100644
--- a/operations/set_title.go
+++ b/operations/set_title.go
@@ -1,7 +1,11 @@
 package operations
 
 import (
+	"fmt"
+	"strings"
+
 	"github.com/MichaelMure/git-bug/bug"
+	"github.com/MichaelMure/git-bug/util/text"
 )
 
 // SetTitleOperation will change the title of a bug
@@ -20,6 +24,34 @@ func (op SetTitleOperation) Apply(snapshot bug.Snapshot) bug.Snapshot {
 	return snapshot
 }
 
+func (op SetTitleOperation) Validate() error {
+	if err := bug.OpBaseValidate(op, bug.SetTitleOp); err != nil {
+		return err
+	}
+
+	if text.Empty(op.Title) {
+		return fmt.Errorf("title is empty")
+	}
+
+	if strings.Contains(op.Title, "\n") {
+		return fmt.Errorf("title should be a single line")
+	}
+
+	if !text.Safe(op.Title) {
+		return fmt.Errorf("title should be fully printable")
+	}
+
+	if strings.Contains(op.Was, "\n") {
+		return fmt.Errorf("previous title should be a single line")
+	}
+
+	if !text.Safe(op.Was) {
+		return fmt.Errorf("previous title should be fully printable")
+	}
+
+	return nil
+}
+
 func NewSetTitleOp(author bug.Person, title string, was string) SetTitleOperation {
 	return SetTitleOperation{
 		OpBase: bug.NewOpBase(bug.SetTitleOp, author),
@@ -29,7 +61,7 @@ func NewSetTitleOp(author bug.Person, title string, was string) SetTitleOperatio
 }
 
 // Convenience function to apply the operation
-func SetTitle(b bug.Interface, author bug.Person, title string) {
+func SetTitle(b bug.Interface, author bug.Person, title string) error {
 	it := bug.NewOperationIterator(b)
 
 	var lastTitleOp bug.Operation
@@ -48,5 +80,11 @@ func SetTitle(b bug.Interface, author bug.Person, title string) {
 	}
 
 	setTitleOp := NewSetTitleOp(author, title, was)
+
+	if err := setTitleOp.Validate(); err != nil {
+		return err
+	}
+
 	b.Append(setTitleOp)
+	return nil
 }