Add Project Delete to API (#66)

* carve out scope into a separate branch for a PR

* refactor getOpts to be private

* narrowed to just the scope proto

* wrap in a transaction

* remove user from scopes PR

* make scope type immutable

* refactored with more referential integrity (thanks mike)

* refactor based on pair programming with Mike

* better VetForWrite that supports db.Options but that requires GetOpts and Options to be exported.

* stop printing out all the db logs

* refactor withScope to be private

* refactor the VetForWrite a bit based on Todd's PR comment

* fix typo

Co-authored-by: Michael Gaffney <mgaffney@users.noreply.github.com>

* fix ctx propagation and Scope returns

* fix ctx propagation and use a const for std retry count

* change Std to Crud

* fix ctx propagation

* make sure update with field masks doesn't update a field not in the mask and force a read after update.

* add description

* remove comment about future

* clean error strings

* clean up enum string rep

* stdMeta

* fix return value to be just nil

* fix returns and errors

* update test to match new error string

* change len of domain to 24

* provide common function for new public ids

* use common db.NewPublicId() and provide the correct prefix

* move list out of CrudActions and provide a CrudlActions shortcut as well

* some comments about the purpose of VetForWrite()

* move to random 10

* Adding project API interface.

* Creation of Project handler and Tests

* Move to using the iam objects.

* Setting up testify framework for mocking and assertions.

* Fix iam options typo references.

* Adding tests for CreateProject and UpdateProject.

This also adds some additional argument validation.

* Merging in PR 40

* Renamed fakeRepo to mockRepo

* Changing around TODOs to capture current plans with the code.

* Remove the repo interface and use a real DB for tests.

* Fixed a few tests.

* Adding checks for properly formatted identifiers.

* Updating code to account for recent merges.  Relaxing some restrictions on what an id can look like.

* Added small comment explaining why RegisterGrpcGatewayer interface exists.

* Move project service up a call level.

Long term I think it would make sense to define these services even higher up the call stack, potentially at the initialization of the controller server, near where the DB is initialized.

* Making these fields align with those in the resources under hosts/...

* Hard code the service initialization in the handler registration code.

* Regenerate project related code proto code.

* Removing type field for project.

* Adds basic project handling in CLI/SDK (#47)

*    Adds SDK for creating and reading projects
*    Reorganizes a bit
*    Adds CLI for creating and reading projects
*    Fixes some some generation bugs
*    Adds redirect support for default org ID

* Fix protobuf import references being moved.

* Update client test to look for "orgs" and "projects" instead of "org" and "project"

* Update expected segments test

* Fix setAddr handling when default organization is in use

* Set client in target after create or read

* Adding functionality and tests for update masks.

* Additional tests for update.  Disable update non existant project test.

* Make not providing an update mask be an invalid argument.

* Make empty update mask paths and no mutable fields in the path an InvalidArgument error.

* Fix get Project test.

* Adding back in the Update a non existant project test case.

* Adding seperate path structured tests.

* Adding delete from repository API call.

* Update calls to LookupScope and DeleteScope.

* Pass in only what is needed for interfacing with the repo.

* Creating delete sdk methods for the first time.

* Create delete generation functionality in go sdk.

* Use assert for tests instead of if statements.

Co-authored-by: Jim Lambert <jimlambrt@Jims-MBP-3.home>
Co-authored-by: Jim <jlambert@hashicorp.com>
Co-authored-by: Michael Gaffney <mgaffney@users.noreply.github.com>
Co-authored-by: Jeff Mitchell <jeffrey.mitchell@gmail.com>

api/internal/genapi/delete.go

@@ -0,0 +1,107 @@
+package main
+
+import (
+	"bytes"
+	"fmt"
+	"io/ioutil"
+	"os"
+	"strings"
+	"text/template"
+)
+
+type deleteInfo struct {
+	baseType   string
+	targetType string
+	path       string
+}
+
+var deleteFuncs = map[string][]*deleteInfo{
+	"scopes": {
+		{
+			"Organization",
+			"Project",
+			"projects/%s",
+		},
+	},
+}
+
+func writeDeleteFuncs() {
+	for outPkg, funcs := range deleteFuncs {
+		outFile := os.Getenv("GEN_BASEPATH") + fmt.Sprintf("/api/%s/delete.gen.go", outPkg)
+		outBuf := bytes.NewBuffer([]byte(fmt.Sprintf(
+			`// Code generated by "make api"; DO NOT EDIT.
+package %s
+`, outPkg)))
+		for _, deleteInfo := range funcs {
+			deleteFuncTemplate.Execute(outBuf, struct {
+				BaseType        string
+				TargetType      string
+				LowerTargetType string
+				Path            string
+			}{
+				BaseType:        deleteInfo.baseType,
+				TargetType:      deleteInfo.targetType,
+				LowerTargetType: strings.ToLower(deleteInfo.targetType),
+				Path:            deleteInfo.path,
+			})
+		}
+		if err := ioutil.WriteFile(outFile, outBuf.Bytes(), 0644); err != nil {
+			fmt.Printf("error writing file %q: %v\n", outFile, err)
+			os.Exit(1)
+		}
+	}
+}
+
+var deleteFuncTemplate = template.Must(template.New("").Parse(
+	`
+// Delete{{ .TargetType }} returns true iff the {{ .LowerTargetType }} existed when the delete attempt was made. 
+func (s {{ .BaseType }}) Delete{{ .TargetType }}(ctx context.Context, {{ .LowerTargetType }} *{{ .TargetType }}) (bool, *api.Error, error) {
+	if s.Client == nil {
+		return false, nil, fmt.Errorf("nil client in Delete{{ .TargetType }} request")
+	}
+	if s.Id == "" {
+		{{ if (eq .BaseType "Organization") }}
+		// Assume the client has been configured with organization already and
+		// move on
+		{{ else if (eq .BaseType "Project") }}
+		// Assume the client has been configured with project already and move
+		// on
+		{{ else }}
+		return nil, nil, fmt.Errorf("missing {{ .BaseType }} ID in Delete{{ .TargetType }} request")
+		{{ end }}
+	} else {
+		// If it's explicitly set here, override anything that might be in the
+		// client
+		{{ if (eq .BaseType "Organization") }}
+		ctx = context.WithValue(ctx, "org", s.Id)
+		{{ else if (eq .BaseType "Project") }}
+		ctx = context.WithValue(ctx, "project", s.Id)
+		{{ end }}
+	}
+	if {{ .LowerTargetType }}.Id == "" {
+		return false, nil, fmt.Errorf("empty {{ .LowerTargetType }} ID field in Delete{{ .TargetType }} request")
+	}
+
+	req, err := s.Client.NewRequest(ctx, "DELETE", fmt.Sprintf("{{ .Path }}", {{ .LowerTargetType }}.Id), nil)
+	if err != nil {
+		return false, nil, fmt.Errorf("error creating Delete{{ .TargetType }} request: %w", err)
+	}
+
+	resp, err := s.Client.Do(req)
+	if err != nil {
+		return false, nil, fmt.Errorf("error performing client request during Delete{{ .TargetType }} call: %w", err)
+	}
+
+	type deleteResponse struct {
+		Existed bool
+	}
+	target := &deleteResponse{}
+
+	apiErr, err := resp.Decode(target)
+	if err != nil {
+		return false, nil, fmt.Errorf("error decoding Delete{{ .TargetType }} repsonse: %w", err)
+	}
+
+	return target.Existed, apiErr, nil
+}
+`))

api/internal/genapi/main.go

@@ -6,4 +6,5 @@ func main() {
 	writeCreateFuncs()
 	writeReadFuncs()
 	writeUpdateFuncs()
+	writeDeleteFuncs()
 }

api/scopes/delete.gen.go

@@ -0,0 +1,53 @@
+// Code generated by "make api"; DO NOT EDIT.
+package scopes
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/hashicorp/watchtower/api"
+)
+
+// DeleteProject returns true iff the project existed when the delete attempt was made.
+func (s Organization) DeleteProject(ctx context.Context, project *Project) (bool, *api.Error, error) {
+	if s.Client == nil {
+		return false, nil, fmt.Errorf("nil client in DeleteProject request")
+	}
+	if s.Id == "" {
+
+		// Assume the client has been configured with organization already and
+		// move on
+
+	} else {
+		// If it's explicitly set here, override anything that might be in the
+		// client
+
+		ctx = context.WithValue(ctx, "org", s.Id)
+
+	}
+	if project.Id == "" {
+		return false, nil, fmt.Errorf("empty project ID field in DeleteProject request")
+	}
+
+	req, err := s.Client.NewRequest(ctx, "DELETE", fmt.Sprintf("projects/%s", project.Id), nil)
+	if err != nil {
+		return false, nil, fmt.Errorf("error creating DeleteProject request: %w", err)
+	}
+
+	resp, err := s.Client.Do(req)
+	if err != nil {
+		return false, nil, fmt.Errorf("error performing client request during DeleteProject call: %w", err)
+	}
+
+	type deleteResponse struct {
+		Existed bool
+	}
+	target := &deleteResponse{}
+
+	apiErr, err := resp.Decode(target)
+	if err != nil {
+		return false, nil, fmt.Errorf("error decoding DeleteProject repsonse: %w", err)
+	}
+
+	return target.Existed, apiErr, nil
+}

api/scopes/project_test.go

@@ -36,7 +36,13 @@ func TestProjects_Crud(t *testing.T) {
 	p, apiErr, err = org.UpdateProject(tc.Context(), p)
 	checkProject("update", p, apiErr, err, "bar")
 
-	// TODO: Delete
+	existed, apiErr, err := org.DeleteProject(tc.Context(), p)
+	assert.NoError(t, err)
+	assert.True(t, existed, "Expected existing project when deleted, but it wasn't.")
+
+	existed, apiErr, err = org.DeleteProject(tc.Context(), p)
+	assert.NoError(t, err)
+	assert.False(t, existed, "Expected project to not exist when deleted, but it did.")
 
 	// TODO: Error conditions once the proper errors are being returned.
 	// Probably as parallel subtests against the same DB.

internal/servers/controller/handlers/projects/project_service.go

@@ -11,7 +11,6 @@ import (
 	pb "github.com/hashicorp/watchtower/internal/gen/controller/api/resources/scopes"
 	pbs "github.com/hashicorp/watchtower/internal/gen/controller/api/services"
 	"github.com/hashicorp/watchtower/internal/iam"
-	"google.golang.org/genproto/protobuf/field_mask"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 )
@@ -43,7 +42,7 @@ func (s Service) GetProject(ctx context.Context, req *pbs.GetProjectRequest) (*p
 	if err := validateGetProjectRequest(req); err != nil {
 		return nil, err
 	}
-	p, err := s.getFromRepo(ctx, req)
+	p, err := s.getFromRepo(ctx, req.GetId())
 	if err != nil {
 		return nil, err
 	}
@@ -56,7 +55,7 @@ func (s Service) CreateProject(ctx context.Context, req *pbs.CreateProjectReques
 	if err := validateCreateProjectRequest(req); err != nil {
 		return nil, err
 	}
-	p, err := s.createInRepo(ctx, req)
+	p, err := s.createInRepo(ctx, req.GetOrgId(), req.GetItem())
 	if err != nil {
 		return nil, err
 	}
@@ -70,7 +69,7 @@ func (s Service) UpdateProject(ctx context.Context, req *pbs.UpdateProjectReques
 	if err := validateUpdateProjectRequest(req); err != nil {
 		return nil, err
 	}
-	p, err := s.updateInRepo(ctx, req)
+	p, err := s.updateInRepo(ctx, req.GetOrgId(), req.GetId(), req.GetUpdateMask().GetPaths(), req.GetItem())
 	if err != nil {
 		return nil, err
 	}
@@ -79,27 +78,39 @@ func (s Service) UpdateProject(ctx context.Context, req *pbs.UpdateProjectReques
 	return resp, nil
 }
 
-func (s Service) getFromRepo(ctx context.Context, req *pbs.GetProjectRequest) (*pb.Project, error) {
-	p, err := s.repo.LookupScope(ctx, req.Id)
+func (s Service) DeleteProject(ctx context.Context, req *pbs.DeleteProjectRequest) (*pbs.DeleteProjectResponse, error) {
+	if err := validateDeleteProjectRequest(req); err != nil {
+		return nil, err
+	}
+	existed, err := s.deleteFromRepo(ctx, req.GetId())
+	if err != nil {
+		return nil, err
+	}
+	resp := &pbs.DeleteProjectResponse{}
+	resp.Existed = existed
+	return resp, nil
+}
+
+func (s Service) getFromRepo(ctx context.Context, id string) (*pb.Project, error) {
+	p, err := s.repo.LookupScope(ctx, id)
 	if err != nil {
 		return nil, err
 	}
 	if p == nil {
-		return nil, status.Errorf(codes.NotFound, "Could not find Project with id %q", req.GetId())
+		return nil, status.Errorf(codes.NotFound, "Could not find Project with id %q", id)
 	}
 	return toProto(p), nil
 }
 
-func (s Service) createInRepo(ctx context.Context, req *pbs.CreateProjectRequest) (*pb.Project, error) {
-	in := req.GetItem()
+func (s Service) createInRepo(ctx context.Context, orgID string, item *pb.Project) (*pb.Project, error) {
 	opts := []iam.Option{}
-	if in.GetName() != nil {
-		opts = append(opts, iam.WithName(in.GetName().GetValue()))
+	if item.GetName() != nil {
+		opts = append(opts, iam.WithName(item.GetName().GetValue()))
 	}
-	if in.GetDescription() != nil {
-		opts = append(opts, iam.WithDescription(in.GetDescription().GetValue()))
+	if item.GetDescription() != nil {
+		opts = append(opts, iam.WithDescription(item.GetDescription().GetValue()))
 	}
-	p, err := iam.NewProject(req.GetOrgId(), opts...)
+	p, err := iam.NewProject(orgID, opts...)
 	if err != nil {
 		return nil, err
 	}
@@ -113,20 +124,19 @@ func (s Service) createInRepo(ctx context.Context, req *pbs.CreateProjectRequest
 	return toProto(out), nil
 }
 
-func (s Service) updateInRepo(ctx context.Context, req *pbs.UpdateProjectRequest) (*pb.Project, error) {
-	item := req.GetItem()
-	opts := []iam.Option{iam.WithPublicId(req.GetId())}
+func (s Service) updateInRepo(ctx context.Context, orgID, projID string, mask []string, item *pb.Project) (*pb.Project, error) {
+	opts := []iam.Option{iam.WithPublicId(projID)}
 	if desc := item.GetDescription(); desc != nil {
 		opts = append(opts, iam.WithDescription(desc.GetValue()))
 	}
 	if name := item.GetName(); name != nil {
 		opts = append(opts, iam.WithName(name.GetValue()))
 	}
-	p, err := iam.NewProject(req.GetOrgId(), opts...)
+	p, err := iam.NewProject(orgID, opts...)
 	if err != nil {
 		return nil, err
 	}
-	dbMask, err := toDbUpdateMask(req.GetUpdateMask())
+	dbMask, err := toDbUpdateMask(mask)
 	if err != nil {
 		return nil, err
 	}
@@ -143,11 +153,19 @@ func (s Service) updateInRepo(ctx context.Context, req *pbs.UpdateProjectRequest
 	return toProto(out), nil
 }
 
+func (s Service) deleteFromRepo(ctx context.Context, projId string) (bool, error) {
+	rows, err := s.repo.DeleteScope(ctx, projId)
+	if err != nil {
+		return false, status.Errorf(codes.Internal, "Unable to delete project: %v", err)
+	}
+	return rows > 0, nil
+}
+
 // toDbUpdateMask converts the wire format's FieldMask into a list of strings containing FieldMask paths used
-func toDbUpdateMask(fm *field_mask.FieldMask) ([]string, error) {
+func toDbUpdateMask(paths []string) ([]string, error) {
 	dbPaths := []string{}
 	invalid := []string{}
-	for _, p := range fm.GetPaths() {
+	for _, p := range paths {
 		for _, f := range strings.Split(p, ",") {
 			if dbField, ok := wireToStorageMask[strings.TrimSpace(f)]; ok {
 				dbPaths = append(dbPaths, dbField)
@@ -247,6 +265,19 @@ func validateUpdateProjectRequest(req *pbs.UpdateProjectRequest) error {
 	return nil
 }
 
+func validateDeleteProjectRequest(req *pbs.DeleteProjectRequest) error {
+	if err := validateAncestors(req); err != nil {
+		return err
+	}
+	if err := validateID(req.GetId(), "p_"); err != nil {
+		return err
+	}
+	if err := validateID(req.GetOrgId(), "o_"); err != nil {
+		return err
+	}
+	return nil
+}
+
 func validateID(id, prefix string) error {
 	if !strings.HasPrefix(id, prefix) {
 		return status.Errorf(codes.InvalidArgument, "ID start with a %q prefix, provided %q", prefix, id)

internal/servers/controller/handlers/projects/service_test.go

@@ -119,6 +119,105 @@ func TestGet(t *testing.T) {
 	}
 }
 
+func TestDelete(t *testing.T) {
+	proj, repo := createDefaultProjectAndRepo(t)
+
+	proj2, err := iam.NewProject(proj.GetParentId())
+	if err != nil {
+		t.Fatalf("Couldn't allocate a second project: %v", err)
+	}
+	proj2, err = repo.CreateScope(context.Background(), proj2)
+	if err != nil {
+		t.Fatalf("Couldn't persist a second project %v", err)
+	}
+
+	s := projects.NewService(repo)
+
+	cases := []struct {
+		name    string
+		req     *pbs.DeleteProjectRequest
+		res     *pbs.DeleteProjectResponse
+		errCode codes.Code
+	}{
+		{
+			name: "Delete an Existing Project",
+			req: &pbs.DeleteProjectRequest{
+				OrgId: proj2.GetParentId(),
+				Id:    proj2.GetPublicId(),
+			},
+			res: &pbs.DeleteProjectResponse{
+				Existed: true,
+			},
+			errCode: codes.OK,
+		},
+		{
+			name: "Delete bad project id Project",
+			req: &pbs.DeleteProjectRequest{
+				OrgId: proj2.GetParentId(),
+				Id:    "p_doesntexis",
+			},
+			res: &pbs.DeleteProjectResponse{
+				Existed: false,
+			},
+			errCode: codes.OK,
+		},
+		{
+			name: "Delete bad org id Project",
+			req: &pbs.DeleteProjectRequest{
+				OrgId: "o_doesntexis",
+				Id:    proj2.GetPublicId(),
+			},
+			res: &pbs.DeleteProjectResponse{
+				Existed: false,
+			},
+			errCode: codes.OK,
+		},
+		{
+			name: "Bad org formatting",
+			req: &pbs.DeleteProjectRequest{
+				OrgId: "bad_format",
+				Id:    proj2.GetPublicId(),
+			},
+			res:     nil,
+			errCode: codes.InvalidArgument,
+		},
+		{
+			name: "Bad Project Id formatting",
+			req: &pbs.DeleteProjectRequest{
+				OrgId: proj2.GetParentId(),
+				Id:    "bad_format",
+			},
+			res:     nil,
+			errCode: codes.InvalidArgument,
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			assert := assert.New(t)
+			got, gErr := s.DeleteProject(context.Background(), tc.req)
+			assert.Equal(tc.errCode, status.Code(gErr), "DeleteProject(%+v) got error %v, wanted %v", tc.req, gErr, tc.errCode)
+			assert.EqualValuesf(tc.res, got, "DeleteProject(%q) got response %q, wanted %q", tc.req, got, tc.res)
+		})
+	}
+}
+
+func TestDelete_twice(t *testing.T) {
+	assert := assert.New(t)
+	proj, repo := createDefaultProjectAndRepo(t)
+
+	s := projects.NewService(repo)
+	req := &pbs.DeleteProjectRequest{
+		OrgId: proj.GetParentId(),
+		Id:    proj.GetPublicId(),
+	}
+	got, gErr := s.DeleteProject(context.Background(), req)
+	assert.NoError(gErr, "First attempt")
+	assert.True(got.GetExisted(), "Expected existed to be true for the first delete.")
+	got, gErr = s.DeleteProject(context.Background(), req)
+	assert.NoError(gErr, "Second attempt")
+	assert.False(got.GetExisted(), "Expected existed to be false for the second delete.")
+}
+
 func TestCreate(t *testing.T) {
 	defaultProj, repo := createDefaultProjectAndRepo(t)
 	defaultProjCreated, err := ptypes.Timestamp(defaultProj.GetCreateTime().GetTimestamp())