Fix #363: Creation of template with the same name (#371)

Signed-off-by: parauliya <aman@infracloud.io> ## Description This PR will fix allow user to create a template with the same name after deleting it. Before this, it was not possible because of an issue also after deleting a template all the workflow which were created with that template before deleting it will be able to access the template since we template will be deleted softly. ## Why is this needed Fixes: #363 ## How Has This Been Tested? Tested all the scenarios mentioned in the linked issue manually using vagrant setup. ## How are existing users impacted? What migration steps/scripts do we need? Have written a migration script for db. ## Checklist: I have: - [ ] updated the documentation and/or roadmap (if required) - [x] added unit or e2e tests - [ ] provided instructions on how to upgrade
tinkerbell · Dec 7, 2020 · 1992c1c · 1992c1c
2 parents fe762e8 + 0f7e7de
commit 1992c1c
Show file tree

Hide file tree

Showing 12 changed files with 171 additions and 45 deletions.
diff --git a/cmd/tink-cli/cmd/template/create.go b/cmd/tink-cli/cmd/template/create.go
@@ -69,7 +69,7 @@ func readAll(reader io.Reader) []byte {
 func addFlags() {
 	flags := createCmd.PersistentFlags()
 	flags.StringVarP(&filePath, "path", "p", "", "path to the template file")
-	flags.StringVarP(&templateName, "name", "n", "", "unique name for the template (alphanumeric)")
+	flags.StringVarP(&templateName, "name", "n", "", "unique name for the template (alphanumeric and case sensitive)")
 	_ = createCmd.MarkPersistentFlagRequired(fName)
 }
 

diff --git a/db/db.go b/db/db.go
@@ -36,7 +36,7 @@ type hardware interface {
 
 type template interface {
 	CreateTemplate(ctx context.Context, name string, data string, id uuid.UUID) error
-	GetTemplate(ctx context.Context, fields map[string]string) (string, string, string, error)
+	GetTemplate(ctx context.Context, fields map[string]string, deleted bool) (string, string, string, error)
 	DeleteTemplate(ctx context.Context, name string) error
 	ListTemplates(in string, fn func(id, n string, in, del *timestamp.Timestamp) error) error
 	UpdateTemplate(ctx context.Context, name string, data string, id uuid.UUID) error
@@ -114,7 +114,7 @@ func get(ctx context.Context, db *sql.DB, query string, args ...interface{}) (st
 func buildGetCondition(fields map[string]string) (string, error) {
 	for column, field := range fields {
 		if field != "" {
-			return fmt.Sprintf("%s = '%s' AND", column, field), nil
+			return fmt.Sprintf("%s = '%s'", column, field), nil
 		}
 	}
 	return "", errors.New("one GetBy field must be set to build a get condition")

diff --git a/db/migration/202012041103-remove-unique-constraints.go b/db/migration/202012041103-remove-unique-constraints.go
@@ -0,0 +1,12 @@
+package migration
+
+import migrate "github.com/rubenv/sql-migrate"
+
+func Get202012041103() *migrate.Migration {
+	return &migrate.Migration{
+		Id: "202010221010-remove-unique-index",
+		Up: []string{`
+                ALTER TABLE template DROP CONSTRAINT template_name_key;
+                `},
+	}
+}
diff --git a/db/migration/migration.go b/db/migration/migration.go
@@ -7,6 +7,7 @@ func GetMigrations() *migrate.MemoryMigrationSource {
 		Migrations: []*migrate.Migration{
 			Get202009171251(),
 			Get202010221010(),
+			Get202012041103(),
 		},
 	}
 }
diff --git a/db/mock/mock.go b/db/mock/mock.go
@@ -13,6 +13,7 @@ import (
 type DB struct {
 	// workflow
 	CreateWorkflowFunc               func(ctx context.Context, wf db.Workflow, data string, id uuid.UUID) error
+	GetWorkflowFunc                  func(ctx context.Context, id string) (db.Workflow, error)
 	GetfromWfDataTableFunc           func(ctx context.Context, req *pb.GetWorkflowDataRequest) ([]byte, error)
 	InsertIntoWfDataTableFunc        func(ctx context.Context, req *pb.UpdateWorkflowDataRequest) error
 	GetWorkflowMetadataFunc          func(ctx context.Context, req *pb.GetWorkflowDataRequest) ([]byte, error)
@@ -24,5 +25,5 @@ type DB struct {
 	InsertIntoWorkflowEventTableFunc func(ctx context.Context, wfEvent *pb.WorkflowActionStatus, time time.Time) error
 	// template
 	TemplateDB      map[string]interface{}
-	GetTemplateFunc func(ctx context.Context, fields map[string]string) (string, string, string, error)
+	GetTemplateFunc func(ctx context.Context, fields map[string]string, deleted bool) (string, string, string, error)
 }
diff --git a/db/mock/template.go b/db/mock/template.go
@@ -9,8 +9,9 @@ import (
 )
 
 type Template struct {
-	ID   uuid.UUID
-	Data string
+	ID      uuid.UUID
+	Data    string
+	Deleted bool
 }
 
 // CreateTemplate creates a new workflow template
@@ -20,20 +21,28 @@ func (d DB) CreateTemplate(ctx context.Context, name string, data string, id uui
 	}
 
 	if _, ok := d.TemplateDB[name]; ok {
-		return errors.New("Template name already exist in the database")
+		tmpl := d.TemplateDB[name]
+		switch stmpl := tmpl.(type) {
+		case Template:
+			if !stmpl.Deleted {
+				return errors.New("Template name already exist in the database")
+			}
+		default:
+			return errors.New("Not a Template type in the database")
+		}
 	}
-
 	d.TemplateDB[name] = Template{
-		ID:   id,
-		Data: data,
+		ID:      id,
+		Data:    data,
+		Deleted: false,
 	}
 
 	return nil
 }
 
 // GetTemplate returns a workflow template
-func (d DB) GetTemplate(ctx context.Context, fields map[string]string) (string, string, string, error) {
-	return d.GetTemplateFunc(ctx, fields)
+func (d DB) GetTemplate(ctx context.Context, fields map[string]string, deleted bool) (string, string, string, error) {
+	return d.GetTemplateFunc(ctx, fields, deleted)
 }
 
 // DeleteTemplate deletes a workflow template

diff --git a/db/mock/workflow.go b/db/mock/workflow.go
@@ -41,7 +41,7 @@ func (d DB) GetWorkflowsForWorker(id string) ([]string, error) {
 
 // GetWorkflow returns a workflow
 func (d DB) GetWorkflow(ctx context.Context, id string) (db.Workflow, error) {
-	return db.Workflow{}, nil
+	return d.GetWorkflowFunc(ctx, id)
 }
 
 // DeleteWorkflow deletes a workflow

diff --git a/db/template.go b/db/template.go
@@ -19,11 +19,17 @@ func (d TinkDB) CreateTemplate(ctx context.Context, name string, data string, id
 		return err
 	}
 
+	fields := map[string]string{
+		"name": name,
+	}
+	_, _, _, err = d.GetTemplate(ctx, fields, false)
+	if err != sql.ErrNoRows {
+		return errors.New("Template with name '" + name + "' already exist")
+	}
 	tx, err := d.instance.BeginTx(ctx, &sql.TxOptions{Isolation: sql.LevelSerializable})
 	if err != nil {
 		return errors.Wrap(err, "BEGIN transaction")
 	}
-
 	_, err = tx.Exec(`
 	INSERT INTO
 		template (created_at, updated_at, name, data, id)
@@ -45,20 +51,31 @@ func (d TinkDB) CreateTemplate(ctx context.Context, name string, data string, id
 	return nil
 }
 
-// GetTemplate returns a workflow template
-func (d TinkDB) GetTemplate(ctx context.Context, fields map[string]string) (string, string, string, error) {
+// GetTemplate returns template which is not deleted
+func (d TinkDB) GetTemplate(ctx context.Context, fields map[string]string, deleted bool) (string, string, string, error) {
 	getCondition, err := buildGetCondition(fields)
 	if err != nil {
 		return "", "", "", errors.Wrap(err, "failed to get template")
 	}
 
-	query := `
+	var query string
+	if !deleted {
+		query = `
 	SELECT id, name, data
 	FROM template
 	WHERE
-		` + getCondition + `
+		` + getCondition + ` AND 
 		deleted_at IS NULL
 	`
+	} else {
+		query = `
+	SELECT id, name, data
+	FROM template
+	WHERE
+		` + getCondition + `
+	`
+	}
+
 	row := d.instance.QueryRowContext(ctx, query)
 	id := []byte{}
 	name := []byte{}

diff --git a/grpc-server/template.go b/grpc-server/template.go
@@ -62,7 +62,7 @@ func (s *server) GetTemplate(ctx context.Context, in *template.GetRequest) (*tem
 		"id":   in.GetId(),
 		"name": in.GetName(),
 	}
-	id, n, d, err := s.db.GetTemplate(ctx, fields)
+	id, n, d, err := s.db.GetTemplate(ctx, fields, false)
 	logger.Info("done " + msg)
 	if err != nil {
 		metrics.CacheErrors.With(labels).Inc()

diff --git a/grpc-server/template_test.go b/grpc-server/template_test.go
@@ -83,7 +83,10 @@ func TestCreateTemplate(t *testing.T) {
 			args: args{
 				db: mock.DB{
 					TemplateDB: map[string]interface{}{
-						"template_1": template1,
+						"template_1": mock.Template{
+							Data:    template1,
+							Deleted: false,
+						},
 					},
 				},
 				name:     "template_2",
@@ -98,7 +101,10 @@ func TestCreateTemplate(t *testing.T) {
 			args: args{
 				db: mock.DB{
 					TemplateDB: map[string]interface{}{
-						"template_1": template1,
+						"template_1": mock.Template{
+							Data:    template1,
+							Deleted: false,
+						},
 					},
 				},
 				name:     "template_1",
@@ -108,6 +114,24 @@ func TestCreateTemplate(t *testing.T) {
 				expectedError: true,
 			},
 		},
+
+		"SuccessfulTemplateCreationAfterDeletingWithSameName": {
+			args: args{
+				db: mock.DB{
+					TemplateDB: map[string]interface{}{
+						"template_1": mock.Template{
+							Data:    template1,
+							Deleted: true,
+						},
+					},
+				},
+				name:     "template_1",
+				template: template2,
+			},
+			want: want{
+				expectedError: false,
+			},
+		},
 	}
 
 	ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
@@ -124,6 +148,7 @@ func TestCreateTemplate(t *testing.T) {
 				assert.Nil(t, err)
 				assert.NotEmpty(t, res)
 			}
+			tc.args.db.ClearTemplateDB()
 		})
 	}
 }
@@ -148,7 +173,7 @@ func TestGetTemplate(t *testing.T) {
 					TemplateDB: map[string]interface{}{
 						templateName1: template1,
 					},
-					GetTemplateFunc: func(ctx context.Context, fields map[string]string) (string, string, string, error) {
+					GetTemplateFunc: func(ctx context.Context, fields map[string]string, deleted bool) (string, string, string, error) {
 						t.Log("in get template func")
 
 						if fields["id"] == templateID1 || fields["name"] == templateName1 {
@@ -171,7 +196,7 @@ func TestGetTemplate(t *testing.T) {
 					TemplateDB: map[string]interface{}{
 						templateName1: template1,
 					},
-					GetTemplateFunc: func(ctx context.Context, fields map[string]string) (string, string, string, error) {
+					GetTemplateFunc: func(ctx context.Context, fields map[string]string, deleted bool) (string, string, string, error) {
 						t.Log("in get template func")
 
 						if fields["id"] == templateID1 || fields["name"] == templateName1 {
@@ -194,7 +219,7 @@ func TestGetTemplate(t *testing.T) {
 					TemplateDB: map[string]interface{}{
 						templateName1: template1,
 					},
-					GetTemplateFunc: func(ctx context.Context, fields map[string]string) (string, string, string, error) {
+					GetTemplateFunc: func(ctx context.Context, fields map[string]string, deleted bool) (string, string, string, error) {
 						t.Log("in get template func")
 
 						if fields["id"] == templateID1 || fields["name"] == templateName1 {
@@ -217,7 +242,7 @@ func TestGetTemplate(t *testing.T) {
 					TemplateDB: map[string]interface{}{
 						templateName1: template1,
 					},
-					GetTemplateFunc: func(ctx context.Context, fields map[string]string) (string, string, string, error) {
+					GetTemplateFunc: func(ctx context.Context, fields map[string]string, deleted bool) (string, string, string, error) {
 						t.Log("in get template func")
 
 						if fields["id"] == templateID1 || fields["name"] == templateName1 {
@@ -240,7 +265,7 @@ func TestGetTemplate(t *testing.T) {
 					TemplateDB: map[string]interface{}{
 						templateName1: template1,
 					},
-					GetTemplateFunc: func(ctx context.Context, fields map[string]string) (string, string, string, error) {
+					GetTemplateFunc: func(ctx context.Context, fields map[string]string, deleted bool) (string, string, string, error) {
 						t.Log("in get template func")
 
 						if fields["id"] == templateID1 || fields["name"] == templateName1 {
@@ -263,7 +288,7 @@ func TestGetTemplate(t *testing.T) {
 					TemplateDB: map[string]interface{}{
 						templateName1: template1,
 					},
-					GetTemplateFunc: func(ctx context.Context, fields map[string]string) (string, string, string, error) {
+					GetTemplateFunc: func(ctx context.Context, fields map[string]string, deleted bool) (string, string, string, error) {
 						t.Log("in get template func")
 
 						if fields["id"] == templateID1 || fields["name"] == templateName1 {

diff --git a/grpc-server/workflow.go b/grpc-server/workflow.go
@@ -48,8 +48,15 @@ func (s *server) CreateWorkflow(ctx context.Context, in *workflow.CreateRequest)
 	defer timer.ObserveDuration()
 
 	logger.Info(msg)
+	fields := map[string]string{
+		"id": in.GetTemplate(),
+	}
+	_, _, templateData, err := s.db.GetTemplate(ctx, fields, false)
+	if err != nil {
+		return &workflow.CreateResponse{}, errors.Wrapf(err, errFailedToGetTemplate, in.GetTemplate())
+	}
+	data, err := renderTemplate(in.GetTemplate(), templateData, []byte(in.Hardware))
 
-	data, err := createYaml(ctx, s.db, in.Template, in.Hardware)
 	if err != nil {
 		metrics.CacheErrors.With(labels).Inc()
 		logger.Error(err)
@@ -102,7 +109,15 @@ func (s *server) GetWorkflow(ctx context.Context, in *workflow.GetRequest) (*wor
 		}
 		l.Error(err)
 	}
-	yamlData, err := createYaml(ctx, s.db, w.Template, w.Hardware)
+
+	fields := map[string]string{
+		"id": w.Template,
+	}
+	_, _, templateData, err := s.db.GetTemplate(ctx, fields, true)
+	if err != nil {
+		return &workflow.Workflow{}, errors.Wrapf(err, errFailedToGetTemplate, w.Template)
+	}
+	data, err := renderTemplate(w.Template, templateData, []byte(w.Hardware))
 	if err != nil {
 		return &workflow.Workflow{}, err
 	}
@@ -111,7 +126,7 @@ func (s *server) GetWorkflow(ctx context.Context, in *workflow.GetRequest) (*wor
 		Template: w.Template,
 		Hardware: w.Hardware,
 		State:    state[w.State],
-		Data:     yamlData,
+		Data:     data,
 	}
 	l := logger.With("workflowID", w.ID)
 	l.Info("done " + msg)
@@ -270,17 +285,6 @@ func (s *server) ShowWorkflowEvents(req *workflow.GetRequest, stream workflow.Wo
 	return nil
 }
 
-func createYaml(ctx context.Context, db db.Database, templateID string, devices string) (string, error) {
-	fields := map[string]string{
-		"id": templateID,
-	}
-	_, _, templateData, err := db.GetTemplate(ctx, fields)
-	if err != nil {
-		return "", errors.Wrapf(err, errFailedToGetTemplate, templateID)
-	}
-	return renderTemplate(templateID, templateData, []byte(devices))
-}
-
 func renderTemplate(templateID, templateData string, devices []byte) (string, error) {
 	var hardware map[string]interface{}
 	err := json.Unmarshal(devices, &hardware)