Repository: apigee/registry (go)

Issue:
Registry server: updates to existing resources incorrectly abort
This script consistently produces two errors.

```
#!/bin/sh
registry rpc admin delete-project --name projects/test --force

registry rpc admin update-project --project.name=projects/test --allow_missing
registry rpc admin update-project --project.name=projects/test --allow_missing

registry rpc update-api --api.name=projects/test/locations/global/apis/a --allow_missing
registry rpc update-api --api.name=projects/test/locations/global/apis/a --allow_missing
```

The second `update-project` call fails with this message:
```
Error: rpc error: code = Aborted desc = rpc error: code = AlreadyExists desc = pq: duplicate key value violates unique constraint "projects_pkey"
```

The second `update-api` also fails with a similar message, but is repeatedly (and unsuccessfully) retried by the GAPIC, which is configured to retry methods of the `Registry` service, until we get this failure:
```
Error: context deadline exceeded
```

The server logs show the problem:
```
  INFO[0117] Handling request.         method=DeleteProject request_id=2e9196ee resource=projects/test
  INFO[0117] Success.                  duration=9.394589ms method=DeleteProject request_id=2e9196ee resource=projects/test status_code=OK
  INFO[0117] Handling request.         method=UpdateProject request_id=502e2636
  INFO[0117] Success.                  duration=3.740473ms method=UpdateProject request_id=502e2636 resource=projects/test status_code=OK
  INFO[0117] Handling request.         method=UpdateProject request_id=8b5dc188
 ERROR[0117] Aborted.                  duration=924.63µs error=rpc error: code = AlreadyExists desc = pq: duplicate key value violates unique constraint "projects_pkey" method=UpdateProject request_id=8b5dc188 status_code=Aborted
  INFO[0118] Handling request.         method=UpdateApi request_id=aa0f5504
  INFO[0118] Success.                  duration=4.140671ms method=UpdateApi request_id=aa0f5504 resource=projects/test/locations/global/apis/a status_code=OK
  INFO[0118] Handling request.         method=UpdateApi request_id=e0330ef6
 ERROR[0118] Aborted.                  duration=1.165076ms error=rpc error: code = AlreadyExists desc = pq: duplicate key value violates unique constraint "apis_pkey" method=UpdateApi request_id=e0330ef6 status_code=Aborted
  INFO[0118] Handling request.         method=UpdateApi request_id=b58a4ded
 ERROR[0118] Aborted.                  duration=1.571125ms error=rpc error: code = AlreadyExists desc = pq: duplicate key value violates unique constraint "apis_pkey" method=UpdateApi request_id=b58a4ded status_code=Aborted
  INFO[0118] Handling request.         method=UpdateApi request_id=2acbedfe
 ERROR[0118] Aborted.                  duration=1.488233ms error=rpc error: code = AlreadyExists desc = pq: duplicate key value violates unique constraint "apis_pkey" method=UpdateApi request_id=2acbedfe status_code=Aborted
  INFO[0118] Handling request.         method=UpdateApi request_id=cdc4f756
 ERROR[0118] Aborted.                  duration=1.39005ms error=rpc error: code = AlreadyExists desc = pq: duplicate key value violates unique constraint "apis_pkey" method=UpdateApi request_id=cdc4f756 status_code=Aborted
  INFO[0118] Handling request.         method=UpdateApi request_id=90881969
 ERROR[0118] Aborted.                  duration=1.643127ms error=rpc error: code = AlreadyExists desc = pq: duplicate key value violates unique constraint "apis_pkey" method=UpdateApi request_id=90881969 status_code=Aborted
  INFO[0118] Handling request.         method=UpdateApi request_id=5997e052
 ERROR[0118] Aborted.                  duration=1.545654ms error=rpc error: code = AlreadyExists desc = pq: duplicate key value violates unique constraint "apis_pkey" method=UpdateApi request_id=5997e052 status_code=Aborted
  INFO[0119] Handling request.         method=UpdateApi request_id=19b52751
 ERROR[0119] Aborted.                  duration=1.412112ms error=rpc error: code = AlreadyExists desc = pq: duplicate key value violates unique constraint "apis_pkey" method=UpdateApi request_id=19b52751 status_code=Aborted
  INFO[0120] Handling request.         method=UpdateApi request_id=066ad256
 ERROR[0120] Aborted.                  duration=1.3567ms error=rpc error: code = AlreadyExists desc = pq: duplicate key value violates unique constraint "apis_pkey" method=UpdateApi request_id=066ad256 status_code=Aborted
  INFO[0120] Handling request.         method=UpdateApi request_id=74fcaa91
 ERROR[0120] Aborted.                  duration=1.440423ms error=rpc error: code = AlreadyExists desc = pq: duplicate key value violates unique constraint "apis_pkey" method=UpdateApi request_id=74fcaa91 status_code=Aborted
  INFO[0120] Handling request.         method=UpdateApi request_id=14efe248
 ERROR[0120] Aborted.                  duration=1.39249ms error=rpc error: code = AlreadyExists desc = pq: duplicate key value violates unique constraint "apis_pkey" method=UpdateApi request_id=14efe248 status_code=Aborted
  INFO[0121] Handling request.         method=UpdateApi request_id=a11d0f36
 ERROR[0121] Aborted.                  duration=1.584405ms error=rpc error: code = AlreadyExists desc = pq: duplicate key value violates unique constraint "apis_pkey" method=UpdateApi request_id=a11d0f36 status_code=Aborted
  INFO[0123] Handling request.         method=UpdateApi request_id=a638f662
 ERROR[0123] Aborted.                  duration=1.587956ms error=rpc error: code = AlreadyExists desc = pq: duplicate key value violates unique constraint "apis_pkey" method=UpdateApi request_id=a638f662 status_code=Aborted
  INFO[0126] Handling request.         method=UpdateApi request_id=88f13696
 ERROR[0126] Aborted.                  duration=1.473983ms error=rpc error: code = AlreadyExists desc = pq: duplicate key value violates unique constraint "apis_pkey" method=UpdateApi request_id=88f13696 status_code=Aborted
  INFO[0127] Handling request.         method=UpdateApi request_id=30ecb083
 ERROR[0127] Aborted.                  duration=1.564204ms error=rpc error: code = AlreadyExists desc = pq: duplicate key value violates unique constraint "apis_pkey" method=UpdateApi request_id=30ecb083 status_code=Aborted
  INFO[0128] Handling request.         method=UpdateApi request_id=6af4ed20
 ERROR[0128] Aborted.                  duration=1.450902ms error=rpc error: code = AlreadyExists desc = pq: duplicate key value violates unique constraint "apis_pkey" method=UpdateApi request_id=6af4ed20 status_code=Aborted
  INFO[0133] Handling request.         method=UpdateApi request_id=663d402d
 ERROR[0133] Aborted.                  duration=1.663167ms error=rpc error: code = AlreadyExists desc = pq: duplicate key value violates unique constraint "apis_pkey" method=UpdateApi request_id=663d402d status_code=Aborted
  INFO[0137] Handling request.         method=UpdateApi request_id=fb59509d
 ERROR[0137] Aborted.                  duration=1.586835ms error=rpc error: code = AlreadyExists desc = pq: duplicate key value violates unique constraint "apis_pkey" method=UpdateApi request_id=fb59509d status_code=Aborted
```



Pull request description:
Revert #742, add tests to detect future regressions
This appears to resolve #795.

New tests mimic the script in #795 that was used to isolate the problem.

Patch:
diff --git a/server/registry/actions_apis.go b/server/registry/actions_apis.go
index 8d1a0e1..4b56be9 100644
--- a/server/registry/actions_apis.go
+++ b/server/registry/actions_apis.go
@@ -170,22 +170,23 @@ func (s *RegistryServer) UpdateApi(ctx context.Context, req *rpc.UpdateApiReques
 	}
 	var response *rpc.Api
 	if err := s.runInTransaction(ctx, func(ctx context.Context, db *storage.Client) error {
-		api, err := models.NewApi(name, req.GetApi())
-		if err != nil {
-			return err
-		}
-		mask := models.ExpandMask(req.GetApi(), req.GetUpdateMask())
-		if err := db.SaveApi(ctx, api, mask); err != nil {
-			if status.Code(err) == codes.NotFound && req.GetAllowMissing() {
-				response, err = s.createApi(ctx, db, name, req.GetApi())
-				if status.Code(err) == codes.AlreadyExists {
-					err = status.Error(codes.Aborted, err.Error())
-				}
+		db.LockApis(ctx)
+		api, err := db.GetApi(ctx, name)
+		if err == nil {
+			if err := api.Update(req.GetApi(), models.ExpandMask(req.GetApi(), req.GetUpdateMask())); err != nil {
+				return err
+			}
+			if err := db.SaveApi(ctx, api); err != nil {
+				return err
 			}
+			response, err = api.Message()
+			return err
+		} else if status.Code(err) == codes.NotFound && req.GetAllowMissing() {
+			response, err = s.createApi(ctx, db, name, req.GetApi())
+			return err
+		} else {
 			return err
 		}
-		response, err = api.Message()
-		return err
 	}); err != nil {
 		return nil, err
 	}
diff --git a/server/registry/actions_projects.go b/server/registry/actions_projects.go
index 1369338..96c4af4 100644
--- a/server/registry/actions_projects.go
+++ b/server/registry/actions_projects.go
@@ -149,19 +149,21 @@ func (s *RegistryServer) UpdateProject(ctx context.Context, req *rpc.UpdateProje
 	}
 	var response *rpc.Project
 	if err := s.runInTransaction(ctx, func(ctx context.Context, db *storage.Client) error {
-		project := models.NewProject(name, req.GetProject())
-		mask := models.ExpandMask(req.GetProject(), req.GetUpdateMask())
-		if err := db.SaveProject(ctx, project, mask); err != nil {
-			if status.Code(err) == codes.NotFound && req.GetAllowMissing() {
-				response, err = s.createProject(ctx, db, name, req.GetProject())
-				if status.Convert(err).Code() == codes.AlreadyExists {
-					err = status.Error(codes.Aborted, err.Error())
-				}
+		db.LockProjects(ctx)
+		project, err := db.GetProject(ctx, name)
+		if err == nil {
+			project.Update(req.GetProject(), models.ExpandMask(req.GetProject(), req.GetUpdateMask()))
+			if err := db.SaveProject(ctx, project); err != nil {
+				return err
 			}
+			response = project.Message()
+			return nil
+		} else if status.Code(err) == codes.NotFound && req.GetAllowMissing() {
+			response, err = s.createProject(ctx, db, name, req.GetProject())
+			return err
+		} else {
 			return err
 		}
-		response = project.Message()
-		return err
 	}); err != nil {
 		return nil, err
 	}
diff --git a/server/registry/actions_versions.go b/server/registry/actions_versions.go
index 7cb1368..03da54b 100644
--- a/server/registry/actions_versions.go
+++ b/server/registry/actions_versions.go
@@ -170,22 +170,23 @@ func (s *RegistryServer) UpdateApiVersion(ctx context.Context, req *rpc.UpdateAp
 	}
 	var response *rpc.ApiVersion
 	if err = s.runInTransaction(ctx, func(ctx context.Context, db *storage.Client) error {
-		version, err := models.NewVersion(name, req.GetApiVersion())
-		if err != nil {
-			return err
-		}
-		mask := models.ExpandMask(req.GetApiVersion(), req.GetUpdateMask())
-		if err := db.SaveVersion(ctx, version, mask); err != nil {
-			if status.Code(err) == codes.NotFound && req.GetAllowMissing() {
-				response, err = s.createApiVersion(ctx, db, name, req.GetApiVersion())
-				if status.Convert(err).Code() == codes.AlreadyExists {
-					err = status.Error(codes.Aborted, err.Error())
-				}
+		db.LockVersions(ctx)
+		version, err := db.GetVersion(ctx, name)
+		if err == nil {
+			if err := version.Update(req.GetApiVersion(), models.ExpandMask(req.GetApiVersion(), req.GetUpdateMask())); err != nil {
+				return status.Error(codes.Internal, err.Error())
+			}
+			if err := db.SaveVersion(ctx, version); err != nil {
+				return err
 			}
+			response, err = version.Message()
+			return err
+		} else if status.Code(err) == codes.NotFound && req.GetAllowMissing() {
+			response, err = s.createApiVersion(ctx, db, name, req.GetApiVersion())
+			return err
+		} else {
 			return err
 		}
-		response, err = version.Message()
-		return err
 	}); err != nil {
 		return nil, err
 	}
diff --git a/server/registry/internal/storage/save.go b/server/registry/internal/storage/save.go
index 2f9b606..032268e 100644
--- a/server/registry/internal/storage/save.go
+++ b/server/registry/internal/storage/save.go
@@ -19,28 +19,25 @@ import (
 
 	"github.com/apigee/registry/server/registry/internal/storage/models"
 	"github.com/pkg/errors"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
-	"google.golang.org/protobuf/types/known/fieldmaskpb"
 	"gorm.io/gorm/clause"
 )
 
 // SaveProject will return codes.NotFound if key not found
-func (c *Client) SaveProject(ctx context.Context, v *models.Project, fieldMask *fieldmaskpb.FieldMask) error {
+func (c *Client) SaveProject(ctx context.Context, v *models.Project) error {
 	v.Key = v.Name()
-	return c.saveWithMask(ctx, v, fieldMask)
+	return c.save(ctx, v)
 }
 
 // SaveApi will return codes.NotFound if key not found
-func (c *Client) SaveApi(ctx context.Context, v *models.Api, fieldMask *fieldmaskpb.FieldMask) error {
+func (c *Client) SaveApi(ctx context.Context, v *models.Api) error {
 	v.Key = v.Name()
-	return c.saveWithMask(ctx, v, fieldMask)
+	return c.save(ctx, v)
 }
 
 // SaveVersion will return codes.NotFound if key not found
-func (c *Client) SaveVersion(ctx context.Context, v *models.Version, fieldMask *fieldmaskpb.FieldMask) error {
+func (c *Client) SaveVersion(ctx context.Context, v *models.Version) error {
 	v.Key = v.Name()
-	return c.saveWithMask(ctx, v, fieldMask)
+	return c.save(ctx, v)
 }
 
 // SaveSpecRevision will upsert if key not found
@@ -94,14 +91,3 @@ func (c *Client) save(ctx context.Context, v interface{}) error {
 	}).Create(v).Error
 	return grpcErrorForDBError(ctx, errors.Wrapf(err, "save %#v", v))
 }
-
-func (c *Client) saveWithMask(ctx context.Context, v interface{}, fieldMask *fieldmaskpb.FieldMask) error {
-	op := c.db.WithContext(ctx).
-		Select(fieldMask.GetPaths()).
-		Clauses(clause.Returning{})
-	err := op.Save(v).Error
-	if err == nil && op.RowsAffected == 0 {
-		err = status.Errorf(codes.NotFound, "%s not found in database", v)
-	}
-	return grpcErrorForDBError(ctx, err)
-}


Test patch:
diff --git a/server/registry/actions_apis_test.go b/server/registry/actions_apis_test.go
index 7153f5f..05a132c 100644
--- a/server/registry/actions_apis_test.go
+++ b/server/registry/actions_apis_test.go
@@ -1100,6 +1100,68 @@ func TestUpdateApiResponseCodes(t *testing.T) {
 	}
 }
 
+func TestUpdateApiSequence(t *testing.T) {
+	tests := []struct {
+		desc string
+		req  *rpc.UpdateApiRequest
+		want codes.Code
+	}{
+		{
+			desc: "create using update with allow_missing=false",
+			req: &rpc.UpdateApiRequest{
+				Api: &rpc.Api{
+					Name: "projects/my-project/locations/global/apis/a",
+				},
+				AllowMissing: false,
+			},
+			want: codes.NotFound,
+		},
+		{
+			desc: "create using update with allow_missing=true",
+			req: &rpc.UpdateApiRequest{
+				Api: &rpc.Api{
+					Name: "projects/my-project/locations/global/apis/a",
+				},
+				AllowMissing: true,
+			},
+			want: codes.OK,
+		},
+		{
+			desc: "update existing resource with allow_missing=true",
+			req: &rpc.UpdateApiRequest{
+				Api: &rpc.Api{
+					Name: "projects/my-project/locations/global/apis/a",
+				},
+				AllowMissing: true,
+			},
+			want: codes.OK,
+		},
+		{
+			desc: "update existing resource with allow_missing=false",
+			req: &rpc.UpdateApiRequest{
+				Api: &rpc.Api{
+					Name: "projects/my-project/locations/global/apis/a",
+				},
+				AllowMissing: true,
+			},
+			want: codes.OK,
+		},
+	}
+	ctx := context.Background()
+	server := defaultTestServer(t)
+	seed := &rpc.Project{Name: "projects/my-project"}
+	if err := seeder.SeedProjects(ctx, server, seed); err != nil {
+		t.Fatalf("Setup/Seeding: Failed to seed registry: %s", err)
+	}
+	for _, test := range tests {
+		t.Run(test.desc, func(t *testing.T) {
+			if _, err := server.UpdateApi(ctx, test.req); status.Code(err) != test.want {
+				t.Errorf("UpdateApi(%+v) returned status code %q, want %q: %v", test.req, status.Code(err), test.want, err)
+			}
+		})
+	}
+}
+
 func TestDeleteApi(t *testing.T) {
 	tests := []struct {
 		desc string
diff --git a/server/registry/actions_projects_test.go b/server/registry/actions_projects_test.go
index c9c2635..f450875 100644
--- a/server/registry/actions_projects_test.go
+++ b/server/registry/actions_projects_test.go
@@ -991,6 +991,67 @@ func TestUpdateProjectResponseCodes(t *testing.T) {
 	}
 }
 
+func TestUpdateProjectSequence(t *testing.T) {
+	if adminServiceUnavailable() {
+		t.Skip(testRequiresAdminService)
+	}
+	tests := []struct {
+		desc string
+		req  *rpc.UpdateProjectRequest
+		want codes.Code
+	}{
+		{
+			desc: "create using update with allow_missing=false",
+			req: &rpc.UpdateProjectRequest{
+				Project: &rpc.Project{
+					Name: "projects/my-project",
+				},
+				AllowMissing: false,
+			},
+			want: codes.NotFound,
+		},
+		{
+			desc: "create using update with allow_missing=true",
+			req: &rpc.UpdateProjectRequest{
+				Project: &rpc.Project{
+					Name: "projects/my-project",
+				},
+				AllowMissing: true,
+			},
+			want: codes.OK,
+		},
+		{
+			desc: "update existing resource with allow_missing=true",
+			req: &rpc.UpdateProjectRequest{
+				Project: &rpc.Project{
+					Name: "projects/my-project",
+				},
+				AllowMissing: true,
+			},
+			want: codes.OK,
+		},
+		{
+			desc: "update existing resource with allow_missing=false",
+			req: &rpc.UpdateProjectRequest{
+				Project: &rpc.Project{
+					Name: "projects/my-project",
+				},
+				AllowMissing: true,
+			},
+			want: codes.OK,
+		},
+	}
+	ctx := context.Background()
+	server := defaultTestServer(t)
+	for _, test := range tests {
+		t.Run(test.desc, func(t *testing.T) {
+			if _, err := server.UpdateProject(ctx, test.req); status.Code(err) != test.want {
+				t.Errorf("UpdateProject(%+v) returned status code %q, want %q: %v", test.req, status.Code(err), test.want, err)
+			}
+		})
+	}
+}
+
 func TestDeleteProject(t *testing.T) {
 	if adminServiceUnavailable() {
 		t.Skip(testRequiresAdminService)
diff --git a/server/registry/actions_versions_test.go b/server/registry/actions_versions_test.go
index 0af1172..a3ab789 100644
--- a/server/registry/actions_versions_test.go
+++ b/server/registry/actions_versions_test.go
@@ -1125,6 +1125,68 @@ func TestUpdateApiVersionResponseCodes(t *testing.T) {
 	}
 }
 
+func TestUpdateApiVersionSequence(t *testing.T) {
+	tests := []struct {
+		desc string
+		req  *rpc.UpdateApiVersionRequest
+		want codes.Code
+	}{
+		{
+			desc: "create using update with allow_missing=false",
+			req: &rpc.UpdateApiVersionRequest{
+				ApiVersion: &rpc.ApiVersion{
+					Name: "projects/my-project/locations/global/apis/a/versions/v",
+				},
+				AllowMissing: false,
+			},
+			want: codes.NotFound,
+		},
+		{
+			desc: "create using update with allow_missing=true",
+			req: &rpc.UpdateApiVersionRequest{
+				ApiVersion: &rpc.ApiVersion{
+					Name: "projects/my-project/locations/global/apis/a/versions/v",
+				},
+				AllowMissing: true,
+			},
+			want: codes.OK,
+		},
+		{
+			desc: "update existing resource with allow_missing=true",
+			req: &rpc.UpdateApiVersionRequest{
+				ApiVersion: &rpc.ApiVersion{
+					Name: "projects/my-project/locations/global/apis/a/versions/v",
+				},
+				AllowMissing: true,
+			},
+			want: codes.OK,
+		},
+		{
+			desc: "update existing resource with allow_missing=false",
+			req: &rpc.UpdateApiVersionRequest{
+				ApiVersion: &rpc.ApiVersion{
+					Name: "projects/my-project/locations/global/apis/a/versions/v",
+				},
+				AllowMissing: true,
+			},
+			want: codes.OK,
+		},
+	}
+	ctx := context.Background()
+	server := defaultTestServer(t)
+	seed := &rpc.Api{Name: "projects/my-project/locations/global/apis/a"}
+	if err := seeder.SeedApis(ctx, server, seed); err != nil {
+		t.Fatalf("Setup/Seeding: Failed to seed registry: %s", err)
+	}
+	for _, test := range tests {
+		t.Run(test.desc, func(t *testing.T) {
+			if _, err := server.UpdateApiVersion(ctx, test.req); status.Code(err) != test.want {
+				t.Errorf("UpdateApiVersion(%+v) returned status code %q, want %q: %v", test.req, status.Code(err), test.want, err)
+			}
+		})
+	}
+}
+
 func TestDeleteApiVersion(t *testing.T) {
 	tests := []struct {
 		desc string