chore(service): remove parametrized service constructor (#931)

Because

- Using parameter structs for injecting dependencies is dangerous,
especially when constructors are consumed by other packages / repos.
Explicit parameters produce compilation errors when new dependencies are
added and the client package doesn't update the constructor method call.

This commit

- Transforms `service.NewService` to a constructor method with one
argument per dependency.

cmd/main/main.go

@@ -285,30 +285,28 @@ func main() {
 	workerUID, _ := uuid.NewV4()
 
 	service := service.NewService(
-		service.ServiceConfig{
-			Repository:     repo,
-			RedisClient:    redisClient,
-			TemporalClient: temporalClient,
-			ACLClient:      &aclClient,
-			Converter: service.NewConverter(service.ConverterConfig{
-				MgmtClient:      mgmtPrivateServiceClient,
-				RedisClient:     redisClient,
-				ACLClient:       &aclClient,
-				Repository:      repo,
-				InstillCoreHost: config.Config.Server.InstillCoreHost,
-				ComponentStore:  compStore,
-			}),
-			MgmtPublicServiceClient:      mgmtPublicServiceClient,
-			MgmtPrivateServiceClient:     mgmtPrivateServiceClient,
-			MinioClient:                  minioClient,
-			ComponentStore:               compStore,
-			Memory:                       ms,
-			WorkerUID:                    workerUID,
-			RetentionHandler:             nil,
-			BinaryFetcher:                binaryFetcher,
-			ArtifactPublicServiceClient:  artifactPublicServiceClient,
-			ArtifactPrivateServiceClient: artifactPrivateServiceClient,
-		},
+		repo,
+		redisClient,
+		temporalClient,
+		&aclClient,
+		service.NewConverter(service.ConverterConfig{
+			MgmtClient:      mgmtPrivateServiceClient,
+			RedisClient:     redisClient,
+			ACLClient:       &aclClient,
+			Repository:      repo,
+			InstillCoreHost: config.Config.Server.InstillCoreHost,
+			ComponentStore:  compStore,
+		}),
+		mgmtPublicServiceClient,
+		mgmtPrivateServiceClient,
+		minioClient,
+		compStore,
+		ms,
+		workerUID,
+		nil,
+		binaryFetcher,
+		artifactPublicServiceClient,
+		artifactPrivateServiceClient,
 	)
 
 	privateGrpcS := grpc.NewServer(grpcServerOpts...)

pkg/service/main.go

@@ -117,50 +117,45 @@ type service struct {
 	artifactPrivateServiceClient artifactpb.ArtifactPrivateServiceClient
 }
 
-// ServiceConfig is the configuration for the service
-type ServiceConfig struct {
-	Repository                   repository.Repository
-	RedisClient                  *redis.Client
-	TemporalClient               client.Client
-	ACLClient                    acl.ACLClientInterface
-	Converter                    Converter
-	MgmtPublicServiceClient      mgmtpb.MgmtPublicServiceClient
-	MgmtPrivateServiceClient     mgmtpb.MgmtPrivateServiceClient
-	MinioClient                  miniox.MinioI
-	ComponentStore               *componentstore.Store
-	Memory                       memory.MemoryStore
-	WorkerUID                    uuid.UUID
-	RetentionHandler             MetadataRetentionHandler
-	BinaryFetcher                external.BinaryFetcher
-	ArtifactPublicServiceClient  artifactpb.ArtifactPublicServiceClient
-	ArtifactPrivateServiceClient artifactpb.ArtifactPrivateServiceClient
-}
-
 // NewService initiates a service instance
 func NewService(
-	cfg ServiceConfig,
+	repository repository.Repository,
+	redisClient *redis.Client,
+	temporalClient client.Client,
+	aclClient acl.ACLClientInterface,
+	converter Converter,
+	mgmtPublicServiceClient mgmtpb.MgmtPublicServiceClient,
+	mgmtPrivateServiceClient mgmtpb.MgmtPrivateServiceClient,
+	minioClient miniox.MinioI,
+	componentStore *componentstore.Store,
+	memory memory.MemoryStore,
+	workerUID uuid.UUID,
+	retentionHandler MetadataRetentionHandler,
+	binaryFetcher external.BinaryFetcher,
+	artifactPublicServiceClient artifactpb.ArtifactPublicServiceClient,
+	artifactPrivateServiceClient artifactpb.ArtifactPrivateServiceClient,
 ) Service {
 	zapLogger, _ := logger.GetZapLogger(context.Background())
-	if cfg.RetentionHandler == nil {
-		cfg.RetentionHandler = NewRetentionHandler()
+	if retentionHandler == nil {
+		retentionHandler = NewRetentionHandler()
 	}
 
 	return &service{
-		repository:                   cfg.Repository,
-		redisClient:                  cfg.RedisClient,
-		temporalClient:               cfg.TemporalClient,
-		mgmtPublicServiceClient:      cfg.MgmtPublicServiceClient,
-		mgmtPrivateServiceClient:     cfg.MgmtPrivateServiceClient,
-		component:                    cfg.ComponentStore,
-		aclClient:                    cfg.ACLClient,
-		converter:                    cfg.Converter,
-		minioClient:                  cfg.MinioClient,
-		memory:                       cfg.Memory,
+		repository:                   repository,
+		redisClient:                  redisClient,
+		temporalClient:               temporalClient,
+		mgmtPublicServiceClient:      mgmtPublicServiceClient,
+		mgmtPrivateServiceClient:     mgmtPrivateServiceClient,
+		component:                    componentStore,
+		aclClient:                    aclClient,
+		converter:                    converter,
+		minioClient:                  minioClient,
+		memory:                       memory,
 		log:                          zapLogger,
-		workerUID:                    cfg.WorkerUID,
-		retentionHandler:             cfg.RetentionHandler,
-		binaryFetcher:                cfg.BinaryFetcher,
-		artifactPublicServiceClient:  cfg.ArtifactPublicServiceClient,
-		artifactPrivateServiceClient: cfg.ArtifactPrivateServiceClient,
+		workerUID:                    workerUID,
+		retentionHandler:             retentionHandler,
+		binaryFetcher:                binaryFetcher,
+		artifactPublicServiceClient:  artifactPublicServiceClient,
+		artifactPrivateServiceClient: artifactPrivateServiceClient,
 	}
 }

pkg/service/pipeline_test.go

@@ -8,17 +8,22 @@ import (
 	"github.com/go-redis/redismock/v9"
 	"github.com/gofrs/uuid"
 	"github.com/gojuno/minimock/v3"
+	"github.com/redis/go-redis/v9"
 	"go.temporal.io/sdk/client"
 
 	"github.com/instill-ai/pipeline-backend/config"
+	"github.com/instill-ai/pipeline-backend/pkg/acl"
 	"github.com/instill-ai/pipeline-backend/pkg/datamodel"
 	"github.com/instill-ai/pipeline-backend/pkg/external"
 	"github.com/instill-ai/pipeline-backend/pkg/memory"
 	"github.com/instill-ai/pipeline-backend/pkg/mock"
 	"github.com/instill-ai/pipeline-backend/pkg/repository"
 	"github.com/instill-ai/pipeline-backend/pkg/resource"
+	miniox "github.com/instill-ai/x/minio"
 
 	componentstore "github.com/instill-ai/pipeline-backend/pkg/component/store"
+	artifactpb "github.com/instill-ai/protogen-go/artifact/artifact/v1alpha"
+	mgmtpb "github.com/instill-ai/protogen-go/core/mgmt/v1beta"
 	pb "github.com/instill-ai/protogen-go/vdp/pipeline/v1beta"
 )
 
@@ -71,19 +76,17 @@ func TestService_UpdateNamespacePipelineByID(t *testing.T) {
 	})
 
 	workerUID, _ := uuid.NewV4()
-	service := NewService(
-		ServiceConfig{
-			Repository:               repo,
-			RedisClient:              redisClient,
-			TemporalClient:           temporalClient,
-			ACLClient:                aclClient,
-			Converter:                converter,
-			MgmtPrivateServiceClient: mgmtPrivateClient,
-			MinioClient:              nil,
-			ComponentStore:           compStore,
-			Memory:                   memory.NewMemoryStore(),
-			WorkerUID:                workerUID,
-			RetentionHandler:         nil,
+	service := newService(
+		serviceConfig{
+			repository:               repo,
+			redisClient:              redisClient,
+			temporalClient:           temporalClient,
+			aCLClient:                aclClient,
+			converter:                converter,
+			mgmtPrivateServiceClient: mgmtPrivateClient,
+			componentStore:           compStore,
+			memory:                   memory.NewMemoryStore(),
+			workerUID:                workerUID,
 		},
 	)
 
@@ -119,3 +122,46 @@ func TestService_UpdateNamespacePipelineByID(t *testing.T) {
 	c.Assert(err, quicktest.IsNil)
 	c.Assert(updatedPbPipeline, quicktest.IsNotNil)
 }
+
+type serviceConfig struct {
+	repository                   repository.Repository
+	redisClient                  *redis.Client
+	temporalClient               client.Client
+	aCLClient                    acl.ACLClientInterface
+	converter                    Converter
+	mgmtPublicServiceClient      mgmtpb.MgmtPublicServiceClient
+	mgmtPrivateServiceClient     mgmtpb.MgmtPrivateServiceClient
+	minioClient                  miniox.MinioI
+	componentStore               *componentstore.Store
+	memory                       memory.MemoryStore
+	workerUID                    uuid.UUID
+	retentionHandler             MetadataRetentionHandler
+	binaryFetcher                external.BinaryFetcher
+	artifactPublicServiceClient  artifactpb.ArtifactPublicServiceClient
+	artifactPrivateServiceClient artifactpb.ArtifactPrivateServiceClient
+}
+
+// newService is a compact helper to instantiate a new service, which allows us
+// to only define the dependencies we'll use in a test. This approach shouldn't
+// be used in production code, where we want every dependency to be injected
+// every time, so we rely on the compiler to guard us against missing
+// dependency injections.
+func newService(cfg serviceConfig) Service {
+	return NewService(
+		cfg.repository,
+		cfg.redisClient,
+		cfg.temporalClient,
+		cfg.aCLClient,
+		cfg.converter,
+		cfg.mgmtPublicServiceClient,
+		cfg.mgmtPrivateServiceClient,
+		cfg.minioClient,
+		cfg.componentStore,
+		cfg.memory,
+		cfg.workerUID,
+		cfg.retentionHandler,
+		cfg.binaryFetcher,
+		cfg.artifactPublicServiceClient,
+		cfg.artifactPrivateServiceClient,
+	)
+}

pkg/service/pipelinerun_test.go

@@ -177,10 +177,10 @@ func TestService_ListPipelineRuns(t *testing.T) {
 
 			repo := repository.NewRepository(tx, redisClient)
 
-			svc := NewService(ServiceConfig{
-				Repository:               repo,
-				MgmtPrivateServiceClient: mgmtPrivateClient,
-				MinioClient:              mockMinio,
+			svc := newService(serviceConfig{
+				repository:               repo,
+				mgmtPrivateServiceClient: mgmtPrivateClient,
+				minioClient:              mockMinio,
 			})
 
 			ctx := context.Background()
@@ -389,11 +389,11 @@ func TestService_ListPipelineRuns_OrgResource(t *testing.T) {
 
 			repo := repository.NewRepository(tx, redisClient)
 
-			svc := NewService(ServiceConfig{
-				Repository:               repo,
-				RedisClient:              redisClient,
-				MgmtPrivateServiceClient: mgmtPrivateClient,
-				MinioClient:              mockMinio,
+			svc := newService(serviceConfig{
+				repository:               repo,
+				redisClient:              redisClient,
+				mgmtPrivateServiceClient: mgmtPrivateClient,
+				minioClient:              mockMinio,
 			})
 
 			ctx := context.Background()
@@ -489,10 +489,10 @@ func TestService_ListPipelineRunsByRequester(t *testing.T) {
 
 	repo := repository.NewRepository(tx, redisClient)
 
-	svc := NewService(ServiceConfig{
-		Repository:               repo,
-		MgmtPrivateServiceClient: mgmtPrivateClient,
-		MinioClient:              mockMinio,
+	svc := newService(serviceConfig{
+		repository:               repo,
+		mgmtPrivateServiceClient: mgmtPrivateClient,
+		minioClient:              mockMinio,
 	})
 
 	ctx := context.Background()