Repository: traPtitech/NeoShowcase
Pull request #550 (2023-06-01 09:03:03)

fix(breaking): Updateではフィールドはoptionalとして扱う
## なぜやるか
https://github.com/traPtitech/NeoShowcase/pull/548#discussion_r1212698655

## やったこと
Update(Application/Repository) ではid以外のフィールドがoptionalとして扱われるようにした
例外的に空文字は指定していないものとして扱われる (connect-go の limitation)


Patch:
diff --git a/api/proto/neoshowcase/protobuf/gateway.proto b/api/proto/neoshowcase/protobuf/gateway.proto
index 38c919a1..82265c08 100644
--- a/api/proto/neoshowcase/protobuf/gateway.proto
+++ b/api/proto/neoshowcase/protobuf/gateway.proto
@@ -70,6 +70,7 @@ message CreateRepositoryRequest {
   CreateRepositoryAuth auth = 3;
 }
 
+// UpdateRepositoryRequest id以外はoptional
 message UpdateRepositoryRequest {
   string id = 1;
   string name = 2;
@@ -213,6 +214,7 @@ message CreateApplicationRequest {
   bool start_on_create = 7;
 }
 
+// UpdateApplicationRequest id以外はoptional
 message UpdateApplicationRequest {
   string id = 1;
   string name = 2;
diff --git a/dashboard/src/api/neoshowcase/protobuf/gateway_pb.ts b/dashboard/src/api/neoshowcase/protobuf/gateway_pb.ts
index dc9931dd..443d5b17 100644
--- a/dashboard/src/api/neoshowcase/protobuf/gateway_pb.ts
+++ b/dashboard/src/api/neoshowcase/protobuf/gateway_pb.ts
@@ -593,6 +593,8 @@ export class CreateRepositoryRequest extends Message<CreateRepositoryRequest> {
 }
 
 /**
+ * UpdateRepositoryRequest id以外はoptional
+ *
  * @generated from message neoshowcase.protobuf.UpdateRepositoryRequest
  */
 export class UpdateRepositoryRequest extends Message<UpdateRepositoryRequest> {
@@ -1544,6 +1546,8 @@ export class CreateApplicationRequest extends Message<CreateApplicationRequest>
 }
 
 /**
+ * UpdateApplicationRequest id以外はoptional
+ *
  * @generated from message neoshowcase.protobuf.UpdateApplicationRequest
  */
 export class UpdateApplicationRequest extends Message<UpdateApplicationRequest> {
diff --git a/pkg/infrastructure/grpc/api_app_service.go b/pkg/infrastructure/grpc/api_app_service.go
index 58c5882b..f44a6bf2 100644
--- a/pkg/infrastructure/grpc/api_app_service.go
+++ b/pkg/infrastructure/grpc/api_app_service.go
@@ -82,13 +82,13 @@ func (s *APIService) UpdateApplication(ctx context.Context, req *connect.Request
 	}
 
 	err = s.svc.UpdateApplication(ctx, msg.Id, &domain.UpdateApplicationArgs{
-		Name:             optional.From(msg.Name),
-		RefName:          optional.From(msg.RefName),
+		Name:             optional.FromNonZero(msg.Name),
+		RefName:          optional.FromNonZero(msg.RefName),
 		UpdatedAt:        optional.From(time.Now()),
-		Config:           optional.From(pbconvert.FromPBApplicationConfig(msg.Config)),
-		Websites:         optional.From(websites),
-		PortPublications: optional.From(ds.Map(msg.PortPublications, pbconvert.FromPBPortPublication)),
-		OwnerIDs:         optional.From(msg.OwnerIds),
+		Config:           optional.Map(optional.FromNonZero(msg.Config), pbconvert.FromPBApplicationConfig),
+		Websites:         optional.FromNonZeroSlice(websites),
+		PortPublications: optional.FromNonZeroSlice(ds.Map(msg.PortPublications, pbconvert.FromPBPortPublication)),
+		OwnerIDs:         optional.FromNonZeroSlice(msg.OwnerIds),
 	})
 	if err != nil {
 		return nil, handleUseCaseError(err)
diff --git a/pkg/infrastructure/grpc/api_repository_service.go b/pkg/infrastructure/grpc/api_repository_service.go
index b9245653..38af9c7c 100644
--- a/pkg/infrastructure/grpc/api_repository_service.go
+++ b/pkg/infrastructure/grpc/api_repository_service.go
@@ -55,10 +55,10 @@ func (s *APIService) GetRepository(ctx context.Context, req *connect.Request[pb.
 func (s *APIService) UpdateRepository(ctx context.Context, req *connect.Request[pb.UpdateRepositoryRequest]) (*connect.Response[emptypb.Empty], error) {
 	msg := req.Msg
 	args := &domain.UpdateRepositoryArgs{
-		Name:     optional.From(msg.Name),
-		URL:      optional.From(msg.Url),
-		Auth:     optional.From(pbconvert.FromPBRepositoryAuth(msg.Auth)),
-		OwnerIDs: optional.From(msg.OwnerIds),
+		Name:     optional.FromNonZero(msg.Name),
+		URL:      optional.FromNonZero(msg.Url),
+		Auth:     optional.Map(optional.FromNonZero(msg.Auth), pbconvert.FromPBRepositoryAuth),
+		OwnerIDs: optional.FromNonZeroSlice(msg.OwnerIds),
 	}
 	err := s.svc.UpdateRepository(ctx, msg.Id, args)
 	if err != nil {
diff --git a/pkg/infrastructure/grpc/pb/gateway.pb.go b/pkg/infrastructure/grpc/pb/gateway.pb.go
index 4db64d7e..d4eb6a0e 100644
--- a/pkg/infrastructure/grpc/pb/gateway.pb.go
+++ b/pkg/infrastructure/grpc/pb/gateway.pb.go
@@ -986,6 +986,7 @@ func (x *CreateRepositoryRequest) GetAuth() *CreateRepositoryAuth {
 	return nil
 }
 
+// UpdateRepositoryRequest id以外はoptional
 type UpdateRepositoryRequest struct {
 	state         protoimpl.MessageState
 	sizeCache     protoimpl.SizeCache
@@ -2191,6 +2192,7 @@ func (x *CreateApplicationRequest) GetStartOnCreate() bool {
 	return false
 }
 
+// UpdateApplicationRequest id以外はoptional
 type UpdateApplicationRequest struct {
 	state         protoimpl.MessageState
 	sizeCache     protoimpl.SizeCache
diff --git a/pkg/infrastructure/repository/application.go b/pkg/infrastructure/repository/application.go
index e5e45f5b..17a21d2c 100644
--- a/pkg/infrastructure/repository/application.go
+++ b/pkg/infrastructure/repository/application.go
@@ -185,7 +185,12 @@ func (r *applicationRepository) UpdateApplication(ctx context.Context, id string
 		cols = append(cols, models.ApplicationColumns.UpdatedAt)
 	}
 
-	_, err = app.Update(ctx, tx, boil.Whitelist(cols...))
+	if len(cols) > 0 {
+		_, err = app.Update(ctx, tx, boil.Whitelist(cols...))
+		if err != nil {
+			return errors.Wrap(err, "updating application")
+		}
+	}
 
 	if args.Config.Valid {
 		mac := repoconvert.FromDomainApplicationConfig(app.ID, &args.Config.V)
diff --git a/pkg/infrastructure/repository/artifact.go b/pkg/infrastructure/repository/artifact.go
index ed413e54..324d3541 100644
--- a/pkg/infrastructure/repository/artifact.go
+++ b/pkg/infrastructure/repository/artifact.go
@@ -77,10 +77,13 @@ func (r *artifactRepository) UpdateArtifact(ctx context.Context, id string, args
 		cols = append(cols, models.ArtifactColumns.DeletedAt)
 	}
 
-	_, err = artifact.Update(ctx, r.db, boil.Whitelist(cols...))
-	if err != nil {
-		return errors.Wrap(err, "failed to update artifact")
+	if len(cols) > 0 {
+		_, err = artifact.Update(ctx, r.db, boil.Whitelist(cols...))
+		if err != nil {
+			return errors.Wrap(err, "failed to update artifact")
+		}
 	}
+
 	return nil
 }
 
diff --git a/pkg/infrastructure/repository/build.go b/pkg/infrastructure/repository/build.go
index dd9efeb8..fc4cd56f 100644
--- a/pkg/infrastructure/repository/build.go
+++ b/pkg/infrastructure/repository/build.go
@@ -124,9 +124,11 @@ func (r *buildRepository) UpdateBuild(ctx context.Context, id string, args domai
 		cols = append(cols, models.BuildColumns.FinishedAt)
 	}
 
-	_, err = build.Update(ctx, tx, boil.Whitelist(cols...))
-	if err != nil {
-		return errors.Wrap(err, "failed to update build")
+	if len(cols) > 0 {
+		_, err = build.Update(ctx, tx, boil.Whitelist(cols...))
+		if err != nil {
+			return errors.Wrap(err, "failed to update build")
+		}
 	}
 
 	err = tx.Commit()
diff --git a/pkg/infrastructure/repository/git_repository.go b/pkg/infrastructure/repository/git_repository.go
index ae534001..0df9d007 100644
--- a/pkg/infrastructure/repository/git_repository.go
+++ b/pkg/infrastructure/repository/git_repository.go
@@ -129,9 +129,11 @@ func (r *gitRepositoryRepository) UpdateRepository(ctx context.Context, id strin
 		cols = append(cols, models.RepositoryColumns.URL)
 	}
 
-	_, err = repo.Update(ctx, tx, boil.Whitelist(cols...))
-	if err != nil {
-		return errors.Wrap(err, "failed to update repository")
+	if len(cols) > 0 {
+		_, err = repo.Update(ctx, tx, boil.Whitelist(cols...))
+		if err != nil {
+			return errors.Wrap(err, "failed to update repository")
+		}
 	}
 
 	if args.Auth.Valid {
diff --git a/pkg/util/optional/of.go b/pkg/util/optional/of.go
index c67ccd42..aefe1bb2 100644
--- a/pkg/util/optional/of.go
+++ b/pkg/util/optional/of.go
@@ -26,3 +26,25 @@ func (o Of[T]) ValueOrZero() T {
 	var t T
 	return t
 }
+
+func Map[T, U any](o Of[T], mapper func(T) U) Of[U] {
+	if o.Valid {
+		return From(mapper(o.V))
+	}
+	return None[U]()
+}
+
+func FromNonZero[T comparable](v T) Of[T] {
+	var zero T
+	if v == zero {
+		return None[T]()
+	}
+	return From(v)
+}
+
+func FromNonZeroSlice[T any](s []T) Of[[]T] {
+	if s == nil {
+		return None[[]T]()
+	}
+	return From(s)
+}


Test patch:
diff --git a/pkg/util/optional/of_test.go b/pkg/util/optional/of_test.go
new file mode 100644
index 00000000..2532351b
--- /dev/null
+++ b/pkg/util/optional/of_test.go
@@ -0,0 +1,123 @@
+package optional
+
+import (
+	"reflect"
+	"testing"
+)
+
+func TestMap(t *testing.T) {
+	type testCase[T any, U any] struct {
+		name   string
+		o      Of[T]
+		mapper func(T) U
+		want   Of[U]
+	}
+	isNotEmpty := func(s string) bool { return s != "" }
+	tests := []testCase[string, bool]{
+		{
+			"valid 1",
+			From("aaa"),
+			isNotEmpty,
+			From(true),
+		},
+		{
+			"valid 2",
+			From(""),
+			isNotEmpty,
+			From(false),
+		},
+		{
+			"empty",
+			None[string](),
+			isNotEmpty,
+			None[bool](),
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := Map(tt.o, tt.mapper); !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("Map() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+type nonZeroTestCase[T comparable] struct {
+	name string
+	v    T
+	want Of[T]
+}
+
+func runTestFromNonZero[T comparable](t *testing.T, tests []nonZeroTestCase[T]) {
+	t.Helper()
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := FromNonZero(tt.v); !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("FromNonZero() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestFromNonZero(t *testing.T) {
+	s := "test"
+	testsPtr := []nonZeroTestCase[*string]{
+		{
+			"non zero",
+			&s,
+			From(&s),
+		},
+		{
+			"zero",
+			nil,
+			None[*string](),
+		},
+	}
+	runTestFromNonZero(t, testsPtr)
+
+	testsStr := []nonZeroTestCase[string]{
+		{
+			"non zero",
+			"aa",
+			From("aa"),
+		},
+		{
+			"zero",
+			"",
+			None[string](),
+		},
+	}
+	runTestFromNonZero(t, testsStr)
+}
+
+func TestFromNonZeroSlice(t *testing.T) {
+	type testCase[T any] struct {
+		name string
+		s    []T
+		want Of[[]T]
+	}
+	tests := []testCase[string]{
+		{
+			"non zero",
+			[]string{"a"},
+			From([]string{"a"}),
+		},
+		{
+			"non zero (empty slice)",
+			[]string{},
+			From([]string{}),
+		},
+		{
+			"zero",
+			nil,
+			None[[]string](),
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := FromNonZeroSlice(tt.s); !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("FromNonZeroSlice() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}