From 950e1265f725a50983d9fb0000ebb3f9b9a5cf0b Mon Sep 17 00:00:00 2001 From: Dmitrii Andreev Date: Tue, 5 May 2026 17:14:35 -0500 Subject: [PATCH 1/3] HYPERFLEET-995 - refactor: move logic from handlers to service --- pkg/api/adapter_status_types.go | 10 +- pkg/api/cluster_types.go | 29 ++- pkg/api/node_pool_types.go | 29 ++- pkg/api/presenters/cluster.go | 4 +- pkg/api/presenters/cluster_test.go | 18 +- pkg/api/presenters/node_pool.go | 4 +- pkg/api/presenters/node_pool_test.go | 18 +- pkg/dao/CLAUDE.md | 2 +- pkg/dao/adapter_status.go | 12 - pkg/dao/cluster.go | 28 -- pkg/dao/mocks/cluster.go | 4 - pkg/dao/mocks/node_pool.go | 24 +- pkg/dao/node_pool.go | 87 ++----- pkg/handlers/cluster.go | 31 +-- pkg/handlers/cluster_nodepools.go | 94 +------ pkg/handlers/cluster_nodepools_test.go | 325 +++++------------------ pkg/handlers/cluster_test.go | 23 +- pkg/handlers/node_pool.go | 27 +- pkg/handlers/validation.go | 9 + pkg/services/adapter_status.go | 11 - pkg/services/cluster.go | 92 +++++-- pkg/services/cluster_test.go | 345 ++++++++++++++++++++++--- pkg/services/generic.go | 10 +- pkg/services/generic_test.go | 4 +- pkg/services/node_pool.go | 152 +++++++++-- pkg/services/node_pool_test.go | 315 ++++++++++++++++++---- pkg/services/status_helpers.go | 22 +- plugins/clusters/plugin.go | 2 +- plugins/nodePools/plugin.go | 2 + 29 files changed, 992 insertions(+), 741 deletions(-) diff --git a/pkg/api/adapter_status_types.go b/pkg/api/adapter_status_types.go index d9790356..de79264a 100644 --- a/pkg/api/adapter_status_types.go +++ b/pkg/api/adapter_status_types.go @@ -46,11 +46,13 @@ func (l AdapterStatusList) Index() AdapterStatusIndex { } func (as *AdapterStatus) BeforeCreate(tx *gorm.DB) error { - id, err := NewID() - if err != nil { - return fmt.Errorf("failed to generate adapter status ID: %w", err) + if as.ID == "" { + id, err := NewID() + if err != nil { + return fmt.Errorf("failed to generate adapter status ID: %w", err) + } + as.ID = id } - as.ID = id return nil } diff --git a/pkg/api/cluster_types.go b/pkg/api/cluster_types.go index 69c5a304..d840f718 100644 --- a/pkg/api/cluster_types.go +++ b/pkg/api/cluster_types.go @@ -24,8 +24,10 @@ type Cluster struct { Generation int32 `json:"generation" gorm:"default:1;not null"` } -type ClusterList []*Cluster -type ClusterIndex map[string]*Cluster +type ( + ClusterList []*Cluster + ClusterIndex map[string]*Cluster +) func (l ClusterList) Index() ClusterIndex { index := ClusterIndex{} @@ -36,14 +38,18 @@ func (l ClusterList) Index() ClusterIndex { } func (c *Cluster) BeforeCreate(tx *gorm.DB) error { - id, err := NewID() - if err != nil { - return fmt.Errorf("failed to generate cluster ID: %w", err) + if c.ID == "" { + id, err := NewID() + if err != nil { + return fmt.Errorf("failed to generate cluster ID: %w", err) + } + c.ID = id } - c.ID = id now := time.Now() - c.CreatedTime = now + if c.CreatedTime.IsZero() { + c.CreatedTime = now + } c.UpdatedTime = now if c.Generation == 0 { c.Generation = 1 @@ -64,3 +70,12 @@ type ClusterPatchRequest struct { Spec *map[string]interface{} `json:"spec,omitempty"` Labels *map[string]string `json:"labels,omitempty"` } + +func (c *Cluster) MarkDeleted(by string, t time.Time) { + c.DeletedTime = &t + c.DeletedBy = &by +} + +func (c *Cluster) IncrementGeneration() { + c.Generation++ +} diff --git a/pkg/api/node_pool_types.go b/pkg/api/node_pool_types.go index f8162d0b..34eaf2f2 100644 --- a/pkg/api/node_pool_types.go +++ b/pkg/api/node_pool_types.go @@ -28,8 +28,10 @@ type NodePool struct { Generation int32 `json:"generation" gorm:"default:1;not null"` } -type NodePoolList []*NodePool -type NodePoolIndex map[string]*NodePool +type ( + NodePoolList []*NodePool + NodePoolIndex map[string]*NodePool +) func (l NodePoolList) Index() NodePoolIndex { index := NodePoolIndex{} @@ -40,14 +42,18 @@ func (l NodePoolList) Index() NodePoolIndex { } func (np *NodePool) BeforeCreate(tx *gorm.DB) error { - id, err := NewID() - if err != nil { - return fmt.Errorf("failed to generate node pool ID: %w", err) + if np.ID == "" { + id, err := NewID() + if err != nil { + return fmt.Errorf("failed to generate node pool ID: %w", err) + } + np.ID = id } - np.ID = id now := time.Now() - np.CreatedTime = now + if np.CreatedTime.IsZero() { + np.CreatedTime = now + } np.UpdatedTime = now if np.Generation == 0 { np.Generation = 1 @@ -75,3 +81,12 @@ type NodePoolPatchRequest struct { Spec *map[string]interface{} `json:"spec,omitempty"` Labels *map[string]string `json:"labels,omitempty"` } + +func (np *NodePool) MarkDeleted(by string, t time.Time) { + np.DeletedTime = &t + np.DeletedBy = &by +} + +func (np *NodePool) IncrementGeneration() { + np.Generation++ +} diff --git a/pkg/api/presenters/cluster.go b/pkg/api/presenters/cluster.go index dbc7adcb..19a1c82c 100644 --- a/pkg/api/presenters/cluster.go +++ b/pkg/api/presenters/cluster.go @@ -11,7 +11,7 @@ import ( ) // ConvertCluster converts openapi.ClusterCreateRequest to api.Cluster (GORM model) -func ConvertCluster(req *openapi.ClusterCreateRequest, createdBy string) (*api.Cluster, error) { +func ConvertCluster(req *openapi.ClusterCreateRequest) (*api.Cluster, error) { // Marshal Spec specJSON, err := json.Marshal(req.Spec) if err != nil { @@ -40,8 +40,6 @@ func ConvertCluster(req *openapi.ClusterCreateRequest, createdBy string) (*api.C Spec: specJSON, Labels: labelsJSON, Generation: 1, - CreatedBy: createdBy, - UpdatedBy: createdBy, }, nil } diff --git a/pkg/api/presenters/cluster_test.go b/pkg/api/presenters/cluster_test.go index 3a61f3f9..40190d28 100644 --- a/pkg/api/presenters/cluster_test.go +++ b/pkg/api/presenters/cluster_test.go @@ -36,16 +36,13 @@ func TestConvertCluster_Complete(t *testing.T) { RegisterTestingT(t) req := createTestClusterRequest() - createdBy := "user123" - result, err := ConvertCluster(req, createdBy) + result, err := ConvertCluster(req) Expect(err).To(BeNil()) // Verify basic fields Expect(result.Kind).To(Equal("Cluster")) Expect(result.Name).To(Equal("test-cluster")) - Expect(result.CreatedBy).To(Equal(createdBy)) - Expect(result.UpdatedBy).To(Equal(createdBy)) // Verify defaults Expect(result.Generation).To(Equal(int32(1))) @@ -83,7 +80,7 @@ func TestConvertCluster_WithLabels(t *testing.T) { Spec: map[string]interface{}{"test": "spec"}, } - result, err := ConvertCluster(req, "user456") + result, err := ConvertCluster(req) Expect(err).To(BeNil()) var resultLabels map[string]string @@ -104,7 +101,7 @@ func TestConvertCluster_WithoutLabels(t *testing.T) { Spec: map[string]interface{}{"test": "spec"}, } - result, err := ConvertCluster(req, "user789") + result, err := ConvertCluster(req) Expect(err).To(BeNil()) var resultLabels map[string]string @@ -135,7 +132,7 @@ func TestConvertCluster_SpecMarshaling(t *testing.T) { Spec: complexSpec, } - result, err := ConvertCluster(req, "user000") + result, err := ConvertCluster(req) Expect(err).To(BeNil()) var resultSpec map[string]interface{} @@ -319,13 +316,12 @@ func TestConvertAndPresentCluster_RoundTrip(t *testing.T) { RegisterTestingT(t) originalReq := createTestClusterRequest() - createdBy := "user999@example.com" // Convert from OpenAPI request to domain - cluster, err := ConvertCluster(originalReq, createdBy) + cluster, err := ConvertCluster(originalReq) Expect(err).To(BeNil()) - // Simulate database fields (ID, timestamps) + // Simulate database fields (ID, timestamps, created_by/updated_by set by service layer) cluster.ID = "cluster-roundtrip-123" now := time.Now() cluster.CreatedTime = now @@ -339,8 +335,6 @@ func TestConvertAndPresentCluster_RoundTrip(t *testing.T) { Expect(*result.Id).To(Equal("cluster-roundtrip-123")) Expect(result.Kind).To(Equal(originalReq.Kind)) Expect(result.Name).To(Equal(originalReq.Name)) - Expect(result.CreatedBy).To(Equal(openapi_types.Email(createdBy))) - Expect(result.UpdatedBy).To(Equal(openapi_types.Email(createdBy))) // Verify Spec preserved Expect(result.Spec["region"]).To(Equal(originalReq.Spec["region"])) diff --git a/pkg/api/presenters/node_pool.go b/pkg/api/presenters/node_pool.go index 2b76a9b7..1dcac817 100644 --- a/pkg/api/presenters/node_pool.go +++ b/pkg/api/presenters/node_pool.go @@ -9,7 +9,7 @@ import ( ) // ConvertNodePool converts openapi.NodePoolCreateRequest to api.NodePool (GORM model) -func ConvertNodePool(req *openapi.NodePoolCreateRequest, ownerID, createdBy string) (*api.NodePool, error) { +func ConvertNodePool(req *openapi.NodePoolCreateRequest, ownerID string) (*api.NodePool, error) { // Marshal Spec specJSON, err := json.Marshal(req.Spec) if err != nil { @@ -38,8 +38,6 @@ func ConvertNodePool(req *openapi.NodePoolCreateRequest, ownerID, createdBy stri Labels: labelsJSON, OwnerID: ownerID, OwnerKind: "Cluster", - CreatedBy: createdBy, - UpdatedBy: createdBy, }, nil } diff --git a/pkg/api/presenters/node_pool_test.go b/pkg/api/presenters/node_pool_test.go index c6a4b1bb..a951141d 100644 --- a/pkg/api/presenters/node_pool_test.go +++ b/pkg/api/presenters/node_pool_test.go @@ -33,9 +33,8 @@ func TestConvertNodePool_Complete(t *testing.T) { req := createTestNodePoolRequest() ownerID := "cluster-owner-123" - createdBy := "user456" - result, err := ConvertNodePool(req, ownerID, createdBy) + result, err := ConvertNodePool(req, ownerID) Expect(err).To(BeNil()) // Verify basic fields @@ -43,8 +42,6 @@ func TestConvertNodePool_Complete(t *testing.T) { Expect(result.Name).To(Equal("test-nodepool")) Expect(result.OwnerID).To(Equal("cluster-owner-123")) Expect(result.OwnerKind).To(Equal("Cluster")) - Expect(result.CreatedBy).To(Equal("user456")) - Expect(result.UpdatedBy).To(Equal("user456")) // Verify Spec marshaled correctly var spec map[string]interface{} @@ -75,7 +72,7 @@ func TestConvertNodePool_WithKind(t *testing.T) { Labels: nil, } - result, err := ConvertNodePool(req, "cluster-123", "user789") + result, err := ConvertNodePool(req, "cluster-123") Expect(err).To(BeNil()) Expect(result.Kind).To(Equal("CustomNodePool")) @@ -92,7 +89,7 @@ func TestConvertNodePool_WithoutKind(t *testing.T) { Labels: nil, } - result, err := ConvertNodePool(req, "cluster-456", "user000") + result, err := ConvertNodePool(req, "cluster-456") Expect(err).To(BeNil()) Expect(result.Kind).To(Equal("NodePool")) // Default value @@ -114,7 +111,7 @@ func TestConvertNodePool_WithLabels(t *testing.T) { Labels: &labels, } - result, err := ConvertNodePool(req, "cluster-789", "user111") + result, err := ConvertNodePool(req, "cluster-789") Expect(err).To(BeNil()) var resultLabels map[string]string @@ -135,7 +132,7 @@ func TestConvertNodePool_WithoutLabels(t *testing.T) { Labels: nil, // Nil labels } - result, err := ConvertNodePool(req, "cluster-xyz", "user222") + result, err := ConvertNodePool(req, "cluster-xyz") Expect(err).To(BeNil()) var resultLabels map[string]string @@ -361,10 +358,9 @@ func TestConvertAndPresentNodePool_RoundTrip(t *testing.T) { originalReq := createTestNodePoolRequest() ownerID := "cluster-roundtrip-789" - createdBy := "user-roundtrip@example.com" // Convert from OpenAPI request to domain - nodePool, err := ConvertNodePool(originalReq, ownerID, createdBy) + nodePool, err := ConvertNodePool(originalReq, ownerID) Expect(err).To(BeNil()) // Simulate database fields (ID, timestamps) @@ -381,8 +377,6 @@ func TestConvertAndPresentNodePool_RoundTrip(t *testing.T) { Expect(*result.Id).To(Equal("nodepool-roundtrip-123")) Expect(*result.Kind).To(Equal(*originalReq.Kind)) Expect(result.Name).To(Equal(originalReq.Name)) - Expect(result.CreatedBy).To(Equal(openapi_types.Email(createdBy))) - Expect(result.UpdatedBy).To(Equal(openapi_types.Email(createdBy))) // Verify Spec preserved Expect(result.Spec["replicas"]).To(BeNumerically("==", originalReq.Spec["replicas"])) diff --git a/pkg/dao/CLAUDE.md b/pkg/dao/CLAUDE.md index ea97ecaa..31ada8b7 100644 --- a/pkg/dao/CLAUDE.md +++ b/pkg/dao/CLAUDE.md @@ -30,7 +30,7 @@ This is critical — without it, the middleware will commit a partially-failed t ## Patterns -- `Replace()` compares spec bytes to detect changes; auto-increments `Generation` only when spec actually changed +- Generation increment is handled by the service layer's `Patch` method via `IncrementGeneration()`; `Save()` persists the result - Use `clause.Associations` carefully — omit on updates to prevent cascading deletes of related records - All methods accept `context.Context` as first parameter for transaction propagation - Return stdlib `error` (not `*errors.ServiceError`) — service layer wraps DAO errors into ServiceErrors diff --git a/pkg/dao/adapter_status.go b/pkg/dao/adapter_status.go index 727f1272..0e91125c 100644 --- a/pkg/dao/adapter_status.go +++ b/pkg/dao/adapter_status.go @@ -12,7 +12,6 @@ import ( type AdapterStatusDao interface { Get(ctx context.Context, id string) (*api.AdapterStatus, error) Create(ctx context.Context, adapterStatus *api.AdapterStatus) (*api.AdapterStatus, error) - Replace(ctx context.Context, adapterStatus *api.AdapterStatus) (*api.AdapterStatus, error) Upsert(ctx context.Context, adapterStatus *api.AdapterStatus, existing *api.AdapterStatus) (*api.AdapterStatus, error) Delete(ctx context.Context, id string) error DeleteByResource(ctx context.Context, resourceType, resourceID string) error @@ -57,17 +56,6 @@ func (d *sqlAdapterStatusDao) Create( return adapterStatus, nil } -func (d *sqlAdapterStatusDao) Replace( - ctx context.Context, adapterStatus *api.AdapterStatus, -) (*api.AdapterStatus, error) { - g2 := (*d.sessionFactory).New(ctx) - if err := g2.Omit(clause.Associations).Save(adapterStatus).Error; err != nil { - db.MarkForRollback(ctx, err) - return nil, err - } - return adapterStatus, nil -} - func (d *sqlAdapterStatusDao) Upsert( ctx context.Context, adapterStatus *api.AdapterStatus, existing *api.AdapterStatus, ) (*api.AdapterStatus, error) { diff --git a/pkg/dao/cluster.go b/pkg/dao/cluster.go index 9f2c6f0d..a0629f58 100644 --- a/pkg/dao/cluster.go +++ b/pkg/dao/cluster.go @@ -1,7 +1,6 @@ package dao import ( - "bytes" "context" "gorm.io/gorm/clause" @@ -14,7 +13,6 @@ type ClusterDao interface { Get(ctx context.Context, id string) (*api.Cluster, error) GetForUpdate(ctx context.Context, id string) (*api.Cluster, error) Create(ctx context.Context, cluster *api.Cluster) (*api.Cluster, error) - Replace(ctx context.Context, cluster *api.Cluster) (*api.Cluster, error) Save(ctx context.Context, cluster *api.Cluster) error SaveStatusConditions(ctx context.Context, id string, statusConditions []byte) error Delete(ctx context.Context, id string) error @@ -59,32 +57,6 @@ func (d *sqlClusterDao) Create(ctx context.Context, cluster *api.Cluster) (*api. return cluster, nil } -func (d *sqlClusterDao) Replace(ctx context.Context, cluster *api.Cluster) (*api.Cluster, error) { - g2 := (*d.sessionFactory).New(ctx) - - // Get the existing cluster to compare spec - existing, err := d.Get(ctx, cluster.ID) - if err != nil { - db.MarkForRollback(ctx, err) - return nil, err - } - - // Compare spec and labels: if either changed, increment generation. - // Aggregated conditions are recomputed in the service layer. - if !bytes.Equal(existing.Spec, cluster.Spec) || !bytes.Equal(existing.Labels, cluster.Labels) { - cluster.Generation = existing.Generation + 1 - } else { - cluster.Generation = existing.Generation - } - - // Save the cluster - if err := g2.Omit(clause.Associations).Save(cluster).Error; err != nil { - db.MarkForRollback(ctx, err) - return nil, err - } - return cluster, nil -} - func (d *sqlClusterDao) Save(ctx context.Context, cluster *api.Cluster) error { g2 := (*d.sessionFactory).New(ctx) if err := g2.Omit(clause.Associations).Save(cluster).Error; err != nil { diff --git a/pkg/dao/mocks/cluster.go b/pkg/dao/mocks/cluster.go index 1b02d55c..8dfb6802 100644 --- a/pkg/dao/mocks/cluster.go +++ b/pkg/dao/mocks/cluster.go @@ -34,10 +34,6 @@ func (d *clusterDaoMock) Create(ctx context.Context, cluster *api.Cluster) (*api return cluster, nil } -func (d *clusterDaoMock) Replace(ctx context.Context, cluster *api.Cluster) (*api.Cluster, error) { - return nil, errors.NotImplemented("Cluster").AsError() -} - func (d *clusterDaoMock) Save(ctx context.Context, cluster *api.Cluster) error { d.clusters = append(d.clusters, cluster) return nil diff --git a/pkg/dao/mocks/node_pool.go b/pkg/dao/mocks/node_pool.go index 0c731af7..d271238c 100644 --- a/pkg/dao/mocks/node_pool.go +++ b/pkg/dao/mocks/node_pool.go @@ -2,7 +2,6 @@ package mocks import ( "context" - "time" "gorm.io/gorm" @@ -26,6 +25,15 @@ func (d *nodePoolDaoMock) Get(ctx context.Context, id string) (*api.NodePool, er return nil, gorm.ErrRecordNotFound } +func (d *nodePoolDaoMock) GetByIDAndOwner(ctx context.Context, id string, ownerID string) (*api.NodePool, error) { + for _, nodePool := range d.nodePools { + if nodePool.ID == id && nodePool.OwnerID == ownerID { + return nodePool, nil + } + } + return nil, gorm.ErrRecordNotFound +} + func (d *nodePoolDaoMock) GetForUpdate(ctx context.Context, id string) (*api.NodePool, error) { return d.Get(ctx, id) } @@ -45,10 +53,6 @@ func (d *nodePoolDaoMock) Create(ctx context.Context, nodePool *api.NodePool) (* return nodePool, nil } -func (d *nodePoolDaoMock) Replace(ctx context.Context, nodePool *api.NodePool) (*api.NodePool, error) { - return nil, errors.NotImplemented("NodePool").AsError() -} - func (d *nodePoolDaoMock) Save(ctx context.Context, nodePool *api.NodePool) error { d.nodePools = append(d.nodePools, nodePool) return nil @@ -58,14 +62,6 @@ func (d *nodePoolDaoMock) Delete(ctx context.Context, id string) error { return errors.NotImplemented("NodePool").AsError() } -func (d *nodePoolDaoMock) SoftDeleteByOwner(ctx context.Context, ownerID string, t time.Time, deletedBy string) error { - return errors.NotImplemented("NodePool").AsError() -} - -func (d *nodePoolDaoMock) FindSoftDeletedByOwner(ctx context.Context, ownerID string) (api.NodePoolList, error) { - return nil, errors.NotImplemented("NodePool").AsError() -} - func (d *nodePoolDaoMock) FindByIDs(ctx context.Context, ids []string) (api.NodePoolList, error) { return nil, errors.NotImplemented("NodePool").AsError() } @@ -74,7 +70,7 @@ func (d *nodePoolDaoMock) FindByOwner(ctx context.Context, ownerID string) (api. return nil, errors.NotImplemented("NodePool").AsError() } -func (d *nodePoolDaoMock) UpdateStatusConditionsByIDs(ctx context.Context, updates map[string][]byte) error { +func (d *nodePoolDaoMock) SaveAll(ctx context.Context, nodePools api.NodePoolList) error { return errors.NotImplemented("NodePool").AsError() } diff --git a/pkg/dao/node_pool.go b/pkg/dao/node_pool.go index 7a2f1041..51366bab 100644 --- a/pkg/dao/node_pool.go +++ b/pkg/dao/node_pool.go @@ -1,11 +1,8 @@ package dao import ( - "bytes" "context" - "time" - "gorm.io/gorm" "gorm.io/gorm/clause" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" @@ -14,17 +11,15 @@ import ( type NodePoolDao interface { Get(ctx context.Context, id string) (*api.NodePool, error) + GetByIDAndOwner(ctx context.Context, id string, ownerID string) (*api.NodePool, error) GetForUpdate(ctx context.Context, id string) (*api.NodePool, error) Create(ctx context.Context, nodePool *api.NodePool) (*api.NodePool, error) - Replace(ctx context.Context, nodePool *api.NodePool) (*api.NodePool, error) Save(ctx context.Context, nodePool *api.NodePool) error SaveStatusConditions(ctx context.Context, id string, statusConditions []byte) error Delete(ctx context.Context, id string) error FindByIDs(ctx context.Context, ids []string) (api.NodePoolList, error) FindByOwner(ctx context.Context, ownerID string) (api.NodePoolList, error) - FindSoftDeletedByOwner(ctx context.Context, ownerID string) (api.NodePoolList, error) - SoftDeleteByOwner(ctx context.Context, ownerID string, t time.Time, deletedBy string) error - UpdateStatusConditionsByIDs(ctx context.Context, updates map[string][]byte) error + SaveAll(ctx context.Context, nodePools api.NodePoolList) error ExistsByOwner(ctx context.Context, ownerID string) (bool, error) All(ctx context.Context) (api.NodePoolList, error) } @@ -48,6 +43,15 @@ func (d *sqlNodePoolDao) Get(ctx context.Context, id string) (*api.NodePool, err return &nodePool, nil } +func (d *sqlNodePoolDao) GetByIDAndOwner(ctx context.Context, id string, ownerID string) (*api.NodePool, error) { + g2 := (*d.sessionFactory).New(ctx) + var nodePool api.NodePool + if err := g2.Take(&nodePool, "id = ? AND owner_id = ?", id, ownerID).Error; err != nil { + return nil, err + } + return &nodePool, nil +} + func (d *sqlNodePoolDao) GetForUpdate(ctx context.Context, id string) (*api.NodePool, error) { g2 := (*d.sessionFactory).New(ctx) var nodePool api.NodePool @@ -76,32 +80,6 @@ func (d *sqlNodePoolDao) Create(ctx context.Context, nodePool *api.NodePool) (*a return nodePool, nil } -func (d *sqlNodePoolDao) Replace(ctx context.Context, nodePool *api.NodePool) (*api.NodePool, error) { - g2 := (*d.sessionFactory).New(ctx) - - // Get the existing nodePool to compare spec - existing, err := d.Get(ctx, nodePool.ID) - if err != nil { - db.MarkForRollback(ctx, err) - return nil, err - } - - // Compare spec and labels: if either changed, increment generation. - // Aggregated conditions are recomputed in the service layer. - if !bytes.Equal(existing.Spec, nodePool.Spec) || !bytes.Equal(existing.Labels, nodePool.Labels) { - nodePool.Generation = existing.Generation + 1 - } else { - nodePool.Generation = existing.Generation - } - - // Save the nodePool - if err := g2.Omit(clause.Associations).Save(nodePool).Error; err != nil { - db.MarkForRollback(ctx, err) - return nil, err - } - return nodePool, nil -} - func (d *sqlNodePoolDao) Save(ctx context.Context, nodePool *api.NodePool) error { g2 := (*d.sessionFactory).New(ctx) if err := g2.Omit(clause.Associations).Save(nodePool).Error; err != nil { @@ -120,31 +98,6 @@ func (d *sqlNodePoolDao) Delete(ctx context.Context, id string) error { return nil } -func (d *sqlNodePoolDao) SoftDeleteByOwner(ctx context.Context, ownerID string, t time.Time, deletedBy string) error { - g2 := (*d.sessionFactory).New(ctx) - result := g2.Model(&api.NodePool{}). - Where("owner_id = ? AND deleted_time IS NULL", ownerID). - Updates(map[string]interface{}{ - "deleted_time": t, - "deleted_by": deletedBy, - "generation": gorm.Expr("generation + 1"), - }) - if result.Error != nil { - db.MarkForRollback(ctx, result.Error) - return result.Error - } - return nil -} - -func (d *sqlNodePoolDao) FindSoftDeletedByOwner(ctx context.Context, ownerID string) (api.NodePoolList, error) { - g2 := (*d.sessionFactory).New(ctx) - var nodePools api.NodePoolList - if err := g2.Where("owner_id = ? AND deleted_time IS NOT NULL", ownerID).Find(&nodePools).Error; err != nil { - return nil, err - } - return nodePools, nil -} - func (d *sqlNodePoolDao) FindByIDs(ctx context.Context, ids []string) (api.NodePoolList, error) { g2 := (*d.sessionFactory).New(ctx) nodePools := api.NodePoolList{} @@ -163,20 +116,14 @@ func (d *sqlNodePoolDao) FindByOwner(ctx context.Context, ownerID string) (api.N return nodePools, nil } -func (d *sqlNodePoolDao) UpdateStatusConditionsByIDs(ctx context.Context, updates map[string][]byte) error { - g2 := (*d.sessionFactory).New(ctx) - if len(updates) == 0 { +func (d *sqlNodePoolDao) SaveAll(ctx context.Context, nodePools api.NodePoolList) error { + if len(nodePools) == 0 { return nil } - - for id, statusConditions := range updates { - result := g2.Model(&api.NodePool{}). - Where("id = ?", id). - Update("status_conditions", statusConditions) - if result.Error != nil { - db.MarkForRollback(ctx, result.Error) - return result.Error - } + g2 := (*d.sessionFactory).New(ctx) + if err := g2.Omit(clause.Associations).Save(nodePools).Error; err != nil { + db.MarkForRollback(ctx, err) + return err } return nil } diff --git a/pkg/handlers/cluster.go b/pkg/handlers/cluster.go index 82de137c..908611aa 100644 --- a/pkg/handlers/cluster.go +++ b/pkg/handlers/cluster.go @@ -1,7 +1,6 @@ package handlers import ( - "encoding/json" "net/http" "github.com/gorilla/mux" @@ -40,7 +39,7 @@ func (h ClusterHandler) Create(w http.ResponseWriter, r *http.Request) { Action: func() (interface{}, *errors.ServiceError) { ctx := r.Context() // Use the presenters.ConvertCluster helper to convert the request - clusterModel, err := presenters.ConvertCluster(&req, "system@hyperfleet.local") + clusterModel, err := presenters.ConvertCluster(&req) if err != nil { return nil, errors.GeneralError("Failed to convert cluster: %v", err) } @@ -71,32 +70,8 @@ func (h ClusterHandler) Patch(w http.ResponseWriter, r *http.Request) { Action: func() (interface{}, *errors.ServiceError) { ctx := r.Context() id := mux.Vars(r)["id"] - found, err := h.cluster.Get(ctx, id) - if err != nil { - return nil, err - } - - if found.DeletedTime != nil { - return nil, errors.ConflictState("Cluster '%s' is marked for deletion", id) - } - - if patch.Spec != nil { - specJSON, err := json.Marshal(*patch.Spec) - if err != nil { - return nil, errors.GeneralError("Failed to marshal spec: %v", err) - } - found.Spec = specJSON - } - - if patch.Labels != nil { - labelsJSON, err := json.Marshal(*patch.Labels) - if err != nil { - return nil, errors.GeneralError("Failed to marshal labels: %v", err) - } - found.Labels = labelsJSON - } - clusterModel, err := h.cluster.Replace(ctx, found) + clusterModel, err := h.cluster.Patch(ctx, id, &patch) if err != nil { return nil, err } @@ -119,7 +94,7 @@ func (h ClusterHandler) List(w http.ResponseWriter, r *http.Request) { listArgs := services.NewListArguments(r.URL.Query()) var clusters []api.Cluster - paging, err := h.generic.List(ctx, "username", listArgs, &clusters) + paging, err := h.generic.List(ctx, listArgs, &clusters) if err != nil { return nil, err } diff --git a/pkg/handlers/cluster_nodepools.go b/pkg/handlers/cluster_nodepools.go index 5fff71f3..76bb76bd 100644 --- a/pkg/handlers/cluster_nodepools.go +++ b/pkg/handlers/cluster_nodepools.go @@ -1,7 +1,6 @@ package handlers import ( - "encoding/json" "net/http" "github.com/gorilla/mux" @@ -16,18 +15,15 @@ import ( type ClusterNodePoolsHandler struct { clusterService services.ClusterService nodePoolService services.NodePoolService - generic services.GenericService } func NewClusterNodePoolsHandler( clusterService services.ClusterService, nodePoolService services.NodePoolService, - generic services.GenericService, ) *ClusterNodePoolsHandler { return &ClusterNodePoolsHandler{ clusterService: clusterService, nodePoolService: nodePoolService, - generic: generic, } } @@ -38,31 +34,20 @@ func (h ClusterNodePoolsHandler) List(w http.ResponseWriter, r *http.Request) { ctx := r.Context() clusterID := mux.Vars(r)["id"] - // Verify cluster exists - _, err := h.clusterService.Get(ctx, clusterID) - if err != nil { + if err := validatePathID(clusterID, "cluster id"); err != nil { return nil, err } - // Get nodepools with owner_id = clusterID listArgs := services.NewListArguments(r.URL.Query()) - // Add filter for owner_id - if listArgs.Search == "" { - listArgs.Search = "owner_id = '" + clusterID + "'" - } else { - listArgs.Search = listArgs.Search + " AND owner_id = '" + clusterID + "'" - } - var nodePools []api.NodePool - paging, err := h.generic.List(ctx, "username", listArgs, &nodePools) + nodePools, paging, err := h.nodePoolService.ListByCluster(ctx, clusterID, listArgs) if err != nil { return nil, err } - // Build list response items := make([]openapi.NodePool, 0, len(nodePools)) for _, nodePool := range nodePools { - presented, err := presenters.PresentNodePool(&nodePool) + presented, err := presenters.PresentNodePool(nodePool) if err != nil { return nil, errors.GeneralError("Failed to present nodepool: %v", err) } @@ -106,23 +91,11 @@ func (h ClusterNodePoolsHandler) Get(w http.ResponseWriter, r *http.Request) { clusterID := mux.Vars(r)["id"] nodePoolID := mux.Vars(r)["nodepool_id"] - // Verify cluster exists - _, err := h.clusterService.Get(ctx, clusterID) + nodePool, err := h.nodePoolService.GetByIDAndOwner(ctx, nodePoolID, clusterID) if err != nil { return nil, err } - // Get nodepool - nodePool, err := h.nodePoolService.Get(ctx, nodePoolID) - if err != nil { - return nil, err - } - - // Verify nodepool belongs to this cluster - if nodePool.OwnerID != clusterID { - return nil, errors.NotFound("NodePool '%s' not found for cluster '%s'", nodePoolID, clusterID) - } - presented, presErr := presenters.PresentNodePool(nodePool) if presErr != nil { return nil, errors.GeneralError("Failed to present nodepool: %v", presErr) @@ -135,7 +108,7 @@ func (h ClusterNodePoolsHandler) Get(w http.ResponseWriter, r *http.Request) { handleGet(w, r, cfg) } -// Delete soft-deletes a specific nodepool for a cluster +// SoftDelete soft-deletes a specific nodepool for a cluster func (h ClusterNodePoolsHandler) SoftDelete(w http.ResponseWriter, r *http.Request) { cfg := &handlerConfig{ Action: func() (interface{}, *errors.ServiceError) { @@ -143,23 +116,12 @@ func (h ClusterNodePoolsHandler) SoftDelete(w http.ResponseWriter, r *http.Reque clusterID := mux.Vars(r)["id"] nodePoolID := mux.Vars(r)["nodepool_id"] - // Verify cluster exists - _, err := h.clusterService.Get(ctx, clusterID) + _, err := h.nodePoolService.GetByIDAndOwner(ctx, nodePoolID, clusterID) if err != nil { return nil, err } - // Get nodepool to verify ownership - nodePool, err := h.nodePoolService.Get(ctx, nodePoolID) - if err != nil { - return nil, err - } - - if nodePool.OwnerID != clusterID { - return nil, errors.NotFound("NodePool '%s' not found for cluster '%s'", nodePoolID, clusterID) - } - - nodePool, err = h.nodePoolService.SoftDelete(ctx, nodePoolID) + nodePool, err := h.nodePoolService.SoftDelete(ctx, nodePoolID) if err != nil { return nil, err } @@ -190,45 +152,12 @@ func (h ClusterNodePoolsHandler) Patch(w http.ResponseWriter, r *http.Request) { clusterID := mux.Vars(r)["id"] nodePoolID := mux.Vars(r)["nodepool_id"] - cluster, err := h.clusterService.Get(ctx, clusterID) + _, err := h.nodePoolService.GetByIDAndOwner(ctx, nodePoolID, clusterID) if err != nil { return nil, err } - if cluster.DeletedTime != nil { - return nil, errors.ConflictState("Cluster '%s' is marked for deletion", clusterID) - } - - found, err := h.nodePoolService.Get(ctx, nodePoolID) - if err != nil { - return nil, err - } - - if found.OwnerID != clusterID { - return nil, errors.NotFound("NodePool '%s' not found for cluster '%s'", nodePoolID, clusterID) - } - - if found.DeletedTime != nil { - return nil, errors.ConflictState("NodePool '%s' is marked for deletion", nodePoolID) - } - - if patch.Spec != nil { - specJSON, jsonErr := json.Marshal(*patch.Spec) - if jsonErr != nil { - return nil, errors.GeneralError("Failed to marshal spec: %v", jsonErr) - } - found.Spec = specJSON - } - - if patch.Labels != nil { - labelsJSON, jsonErr := json.Marshal(*patch.Labels) - if jsonErr != nil { - return nil, errors.GeneralError("Failed to marshal labels: %v", jsonErr) - } - found.Labels = labelsJSON - } - - found, err = h.nodePoolService.Replace(ctx, found) + found, err := h.nodePoolService.Patch(ctx, nodePoolID, &patch) if err != nil { return nil, err } @@ -260,7 +189,6 @@ func (h ClusterNodePoolsHandler) Create(w http.ResponseWriter, r *http.Request) ctx := r.Context() clusterID := mux.Vars(r)["id"] - // Verify cluster exists cluster, err := h.clusterService.Get(ctx, clusterID) if err != nil { return nil, err @@ -270,13 +198,11 @@ func (h ClusterNodePoolsHandler) Create(w http.ResponseWriter, r *http.Request) return nil, errors.ConflictState("Cluster '%s' is marked for deletion", clusterID) } - // Use the presenters.ConvertNodePool helper to convert the request - nodePoolModel, convErr := presenters.ConvertNodePool(&req, cluster.ID, "system@hyperfleet.local") + nodePoolModel, convErr := presenters.ConvertNodePool(&req, cluster.ID) if convErr != nil { return nil, errors.GeneralError("Failed to convert nodepool: %v", convErr) } - // Create nodepool nodePoolModel, err = h.nodePoolService.Create(ctx, nodePoolModel) if err != nil { return nil, err diff --git a/pkg/handlers/cluster_nodepools_test.go b/pkg/handlers/cluster_nodepools_test.go index 958cd586..eea98f90 100644 --- a/pkg/handlers/cluster_nodepools_test.go +++ b/pkg/handlers/cluster_nodepools_test.go @@ -32,8 +32,8 @@ func TestClusterNodePoolsHandler_Get(t *testing.T) { nodePoolID := testNodePoolID tests := []struct { - setupMocks func(ctrl *gomock.Controller) ( //nolint:lll - *services.MockClusterService, *services.MockNodePoolService, *services.MockGenericService, + setupMocks func(ctrl *gomock.Controller) ( + *services.MockClusterService, *services.MockNodePoolService, ) name string clusterID string @@ -46,22 +46,12 @@ func TestClusterNodePoolsHandler_Get(t *testing.T) { clusterID: clusterID, nodePoolID: nodePoolID, setupMocks: func(ctrl *gomock.Controller) ( - *services.MockClusterService, *services.MockNodePoolService, *services.MockGenericService, + *services.MockClusterService, *services.MockNodePoolService, ) { mockClusterSvc := services.NewMockClusterService(ctrl) mockNodePoolSvc := services.NewMockNodePoolService(ctrl) - mockGenericSvc := services.NewMockGenericService(ctrl) - mockClusterSvc.EXPECT().Get(gomock.Any(), clusterID).Return(&api.Cluster{ - Meta: api.Meta{ - ID: clusterID, - CreatedTime: now, - UpdatedTime: now, - }, - Name: "test-cluster", - }, nil) - - mockNodePoolSvc.EXPECT().Get(gomock.Any(), nodePoolID).Return(&api.NodePool{ + mockNodePoolSvc.EXPECT().GetByIDAndOwner(gomock.Any(), nodePoolID, clusterID).Return(&api.NodePool{ Meta: api.Meta{ ID: nodePoolID, CreatedTime: now, @@ -77,52 +67,25 @@ func TestClusterNodePoolsHandler_Get(t *testing.T) { UpdatedBy: "user@example.com", }, nil) - return mockClusterSvc, mockNodePoolSvc, mockGenericSvc + return mockClusterSvc, mockNodePoolSvc }, expectedStatusCode: http.StatusOK, expectedError: false, }, { - name: "Error - Cluster not found", - clusterID: "non-existent", - nodePoolID: nodePoolID, - setupMocks: func(ctrl *gomock.Controller) ( - *services.MockClusterService, *services.MockNodePoolService, *services.MockGenericService, - ) { - mockClusterSvc := services.NewMockClusterService(ctrl) - mockNodePoolSvc := services.NewMockNodePoolService(ctrl) - mockGenericSvc := services.NewMockGenericService(ctrl) - - mockClusterSvc.EXPECT().Get(gomock.Any(), "non-existent").Return(nil, errors.NotFound("Cluster not found")) - - return mockClusterSvc, mockNodePoolSvc, mockGenericSvc - }, - expectedStatusCode: http.StatusNotFound, - expectedError: true, - }, - { - name: "Error - NodePool not found", + name: "Error - NodePool not found for cluster", clusterID: clusterID, nodePoolID: "non-existent", setupMocks: func(ctrl *gomock.Controller) ( - *services.MockClusterService, *services.MockNodePoolService, *services.MockGenericService, + *services.MockClusterService, *services.MockNodePoolService, ) { mockClusterSvc := services.NewMockClusterService(ctrl) mockNodePoolSvc := services.NewMockNodePoolService(ctrl) - mockGenericSvc := services.NewMockGenericService(ctrl) - - mockClusterSvc.EXPECT().Get(gomock.Any(), clusterID).Return(&api.Cluster{ - Meta: api.Meta{ - ID: clusterID, - CreatedTime: now, - UpdatedTime: now, - }, - Name: "test-cluster", - }, nil) - mockNodePoolSvc.EXPECT().Get(gomock.Any(), "non-existent").Return(nil, errors.NotFound("NodePool not found")) + mockNodePoolSvc.EXPECT().GetByIDAndOwner(gomock.Any(), "non-existent", clusterID). + Return(nil, errors.NotFound("NodePool not found")) - return mockClusterSvc, mockNodePoolSvc, mockGenericSvc + return mockClusterSvc, mockNodePoolSvc }, expectedStatusCode: http.StatusNotFound, expectedError: true, @@ -132,33 +95,15 @@ func TestClusterNodePoolsHandler_Get(t *testing.T) { clusterID: clusterID, nodePoolID: nodePoolID, setupMocks: func(ctrl *gomock.Controller) ( - *services.MockClusterService, *services.MockNodePoolService, *services.MockGenericService, + *services.MockClusterService, *services.MockNodePoolService, ) { mockClusterSvc := services.NewMockClusterService(ctrl) mockNodePoolSvc := services.NewMockNodePoolService(ctrl) - mockGenericSvc := services.NewMockGenericService(ctrl) - mockClusterSvc.EXPECT().Get(gomock.Any(), clusterID).Return(&api.Cluster{ - Meta: api.Meta{ - ID: clusterID, - CreatedTime: now, - UpdatedTime: now, - }, - Name: "test-cluster", - }, nil) + mockNodePoolSvc.EXPECT().GetByIDAndOwner(gomock.Any(), nodePoolID, clusterID). + Return(nil, errors.NotFound("NodePool '%s' not found for cluster '%s'", nodePoolID, clusterID)) - mockNodePoolSvc.EXPECT().Get(gomock.Any(), nodePoolID).Return(&api.NodePool{ - Meta: api.Meta{ - ID: nodePoolID, - CreatedTime: now, - UpdatedTime: now, - }, - Kind: "NodePool", - Name: "test-nodepool", - OwnerID: "different-cluster-789", // Different cluster - }, nil) - - return mockClusterSvc, mockNodePoolSvc, mockGenericSvc + return mockClusterSvc, mockNodePoolSvc }, expectedStatusCode: http.StatusNotFound, expectedError: true, @@ -169,17 +114,12 @@ func TestClusterNodePoolsHandler_Get(t *testing.T) { t.Run(tt.name, func(t *testing.T) { RegisterTestingT(t) - // Create gomock controller ctrl := gomock.NewController(t) defer ctrl.Finish() - // Setup mocks - mockClusterSvc, mockNodePoolSvc, mockGenericSvc := tt.setupMocks(ctrl) + mockClusterSvc, mockNodePoolSvc := tt.setupMocks(ctrl) + handler := NewClusterNodePoolsHandler(mockClusterSvc, mockNodePoolSvc) - // Create handler - handler := NewClusterNodePoolsHandler(mockClusterSvc, mockNodePoolSvc, mockGenericSvc) - - // Create request reqURL := "/api/hyperfleet/v1/clusters/" + tt.clusterID + "/nodepools/" + tt.nodePoolID req := httptest.NewRequest(http.MethodGet, reqURL, nil) req = mux.SetURLVars(req, map[string]string{ @@ -187,17 +127,12 @@ func TestClusterNodePoolsHandler_Get(t *testing.T) { "nodepool_id": tt.nodePoolID, }) - // Create response recorder rr := httptest.NewRecorder() - - // Call handler handler.Get(rr, req) - // Check status code Expect(rr.Code).To(Equal(tt.expectedStatusCode)) if !tt.expectedError { - // Parse response var response openapi.NodePool err := json.Unmarshal(rr.Body.Bytes(), &response) Expect(err).NotTo(HaveOccurred()) @@ -217,8 +152,8 @@ func TestClusterNodePoolsHandler_SoftDelete(t *testing.T) { nodePoolID := testNodePoolID tests := []struct { - setupMocks func(ctrl *gomock.Controller) ( //nolint:lll - *services.MockClusterService, *services.MockNodePoolService, *services.MockGenericService, + setupMocks func(ctrl *gomock.Controller) ( + *services.MockClusterService, *services.MockNodePoolService, ) name string clusterID string @@ -231,18 +166,12 @@ func TestClusterNodePoolsHandler_SoftDelete(t *testing.T) { clusterID: clusterID, nodePoolID: nodePoolID, setupMocks: func(ctrl *gomock.Controller) ( - *services.MockClusterService, *services.MockNodePoolService, *services.MockGenericService, + *services.MockClusterService, *services.MockNodePoolService, ) { mockClusterSvc := services.NewMockClusterService(ctrl) mockNodePoolSvc := services.NewMockNodePoolService(ctrl) - mockGenericSvc := services.NewMockGenericService(ctrl) - mockClusterSvc.EXPECT().Get(gomock.Any(), clusterID).Return(&api.Cluster{ - Meta: api.Meta{ID: clusterID, CreatedTime: now, UpdatedTime: now}, - Name: "test-cluster", - }, nil) - - mockNodePoolSvc.EXPECT().Get(gomock.Any(), nodePoolID).Return(&api.NodePool{ + mockNodePoolSvc.EXPECT().GetByIDAndOwner(gomock.Any(), nodePoolID, clusterID).Return(&api.NodePool{ Meta: api.Meta{ID: nodePoolID, CreatedTime: now, UpdatedTime: now}, OwnerID: clusterID, }, nil) @@ -263,75 +192,43 @@ func TestClusterNodePoolsHandler_SoftDelete(t *testing.T) { UpdatedBy: "user@example.com", }, nil) - return mockClusterSvc, mockNodePoolSvc, mockGenericSvc + return mockClusterSvc, mockNodePoolSvc }, expectedStatusCode: http.StatusAccepted, expectedError: false, }, { - name: "given cluster exists but nodepool belongs to a different cluster, when deleted, then returns 404 (ownership guard)", //nolint:lll + name: "given nodepool belongs to a different cluster, when deleted, then returns 404", clusterID: clusterID, nodePoolID: nodePoolID, setupMocks: func(ctrl *gomock.Controller) ( - *services.MockClusterService, *services.MockNodePoolService, *services.MockGenericService, + *services.MockClusterService, *services.MockNodePoolService, ) { mockClusterSvc := services.NewMockClusterService(ctrl) mockNodePoolSvc := services.NewMockNodePoolService(ctrl) - mockGenericSvc := services.NewMockGenericService(ctrl) - mockClusterSvc.EXPECT().Get(gomock.Any(), clusterID).Return(&api.Cluster{ - Meta: api.Meta{ID: clusterID, CreatedTime: now, UpdatedTime: now}, - Name: "test-cluster", - }, nil) - - mockNodePoolSvc.EXPECT().Get(gomock.Any(), nodePoolID).Return(&api.NodePool{ - Meta: api.Meta{ID: nodePoolID, CreatedTime: now, UpdatedTime: now}, - OwnerID: "different-cluster-789", // belongs to a different cluster - }, nil) - // SoftDelete must NOT be called when ownership check fails + mockNodePoolSvc.EXPECT().GetByIDAndOwner(gomock.Any(), nodePoolID, clusterID). + Return(nil, errors.NotFound("NodePool not found for cluster")) - return mockClusterSvc, mockNodePoolSvc, mockGenericSvc + return mockClusterSvc, mockNodePoolSvc }, expectedStatusCode: http.StatusNotFound, expectedError: true, }, { - name: "given cluster does not exist, when deleted, then returns 404", - clusterID: "non-existent", - nodePoolID: nodePoolID, - setupMocks: func(ctrl *gomock.Controller) ( - *services.MockClusterService, *services.MockNodePoolService, *services.MockGenericService, - ) { - mockClusterSvc := services.NewMockClusterService(ctrl) - mockNodePoolSvc := services.NewMockNodePoolService(ctrl) - mockGenericSvc := services.NewMockGenericService(ctrl) - - mockClusterSvc.EXPECT().Get(gomock.Any(), "non-existent").Return(nil, errors.NotFound("Cluster not found")) - - return mockClusterSvc, mockNodePoolSvc, mockGenericSvc - }, - expectedStatusCode: http.StatusNotFound, - expectedError: true, - }, - { - name: "given cluster exists but nodepool does not exist, when deleted, then returns 404", + name: "given nodepool does not exist, when deleted, then returns 404", clusterID: clusterID, nodePoolID: "non-existent", setupMocks: func(ctrl *gomock.Controller) ( - *services.MockClusterService, *services.MockNodePoolService, *services.MockGenericService, + *services.MockClusterService, *services.MockNodePoolService, ) { mockClusterSvc := services.NewMockClusterService(ctrl) mockNodePoolSvc := services.NewMockNodePoolService(ctrl) - mockGenericSvc := services.NewMockGenericService(ctrl) - - mockClusterSvc.EXPECT().Get(gomock.Any(), clusterID).Return(&api.Cluster{ - Meta: api.Meta{ID: clusterID, CreatedTime: now, UpdatedTime: now}, - Name: "test-cluster", - }, nil) - mockNodePoolSvc.EXPECT().Get(gomock.Any(), "non-existent").Return(nil, errors.NotFound("NodePool not found")) + mockNodePoolSvc.EXPECT().GetByIDAndOwner(gomock.Any(), "non-existent", clusterID). + Return(nil, errors.NotFound("NodePool not found")) - return mockClusterSvc, mockNodePoolSvc, mockGenericSvc + return mockClusterSvc, mockNodePoolSvc }, expectedStatusCode: http.StatusNotFound, expectedError: true, @@ -345,8 +242,8 @@ func TestClusterNodePoolsHandler_SoftDelete(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() - mockClusterSvc, mockNodePoolSvc, mockGenericSvc := tt.setupMocks(ctrl) - handler := NewClusterNodePoolsHandler(mockClusterSvc, mockNodePoolSvc, mockGenericSvc) + mockClusterSvc, mockNodePoolSvc := tt.setupMocks(ctrl) + handler := NewClusterNodePoolsHandler(mockClusterSvc, mockNodePoolSvc) reqURL := "/api/hyperfleet/v1/clusters/" + tt.clusterID + "/nodepools/" + tt.nodePoolID req := httptest.NewRequest(http.MethodDelete, reqURL, nil) @@ -381,8 +278,8 @@ func TestClusterNodePoolsHandler_Create(t *testing.T) { validBody := `{"name":"test-np","kind":"NodePool","spec":{"replicas":1}}` tests := []struct { - setupMocks func(ctrl *gomock.Controller) ( //nolint:lll - *services.MockClusterService, *services.MockNodePoolService, *services.MockGenericService, + setupMocks func(ctrl *gomock.Controller) ( + *services.MockClusterService, *services.MockNodePoolService, ) name string clusterID string @@ -393,11 +290,10 @@ func TestClusterNodePoolsHandler_Create(t *testing.T) { name: "Success - Create nodepool for active cluster", clusterID: clusterID, setupMocks: func(ctrl *gomock.Controller) ( - *services.MockClusterService, *services.MockNodePoolService, *services.MockGenericService, + *services.MockClusterService, *services.MockNodePoolService, ) { mockClusterSvc := services.NewMockClusterService(ctrl) mockNodePoolSvc := services.NewMockNodePoolService(ctrl) - mockGenericSvc := services.NewMockGenericService(ctrl) mockClusterSvc.EXPECT().Get(gomock.Any(), clusterID).Return(&api.Cluster{ Meta: api.Meta{ID: clusterID, CreatedTime: now, UpdatedTime: now}, @@ -416,7 +312,7 @@ func TestClusterNodePoolsHandler_Create(t *testing.T) { UpdatedBy: testSystemUser, }, nil) - return mockClusterSvc, mockNodePoolSvc, mockGenericSvc + return mockClusterSvc, mockNodePoolSvc }, expectedStatusCode: http.StatusCreated, expectedError: false, @@ -425,11 +321,10 @@ func TestClusterNodePoolsHandler_Create(t *testing.T) { name: "Error 409 - Cluster is soft-deleted", clusterID: clusterID, setupMocks: func(ctrl *gomock.Controller) ( - *services.MockClusterService, *services.MockNodePoolService, *services.MockGenericService, + *services.MockClusterService, *services.MockNodePoolService, ) { mockClusterSvc := services.NewMockClusterService(ctrl) mockNodePoolSvc := services.NewMockNodePoolService(ctrl) - mockGenericSvc := services.NewMockGenericService(ctrl) deletedTime := now mockClusterSvc.EXPECT().Get(gomock.Any(), clusterID).Return(&api.Cluster{ @@ -438,7 +333,7 @@ func TestClusterNodePoolsHandler_Create(t *testing.T) { DeletedTime: &deletedTime, }, nil) - return mockClusterSvc, mockNodePoolSvc, mockGenericSvc + return mockClusterSvc, mockNodePoolSvc }, expectedStatusCode: http.StatusConflict, expectedError: true, @@ -447,15 +342,14 @@ func TestClusterNodePoolsHandler_Create(t *testing.T) { name: "Error 404 - Cluster not found", clusterID: "non-existent", setupMocks: func(ctrl *gomock.Controller) ( - *services.MockClusterService, *services.MockNodePoolService, *services.MockGenericService, + *services.MockClusterService, *services.MockNodePoolService, ) { mockClusterSvc := services.NewMockClusterService(ctrl) mockNodePoolSvc := services.NewMockNodePoolService(ctrl) - mockGenericSvc := services.NewMockGenericService(ctrl) mockClusterSvc.EXPECT().Get(gomock.Any(), "non-existent").Return(nil, errors.NotFound("Cluster not found")) - return mockClusterSvc, mockNodePoolSvc, mockGenericSvc + return mockClusterSvc, mockNodePoolSvc }, expectedStatusCode: http.StatusNotFound, expectedError: true, @@ -469,8 +363,8 @@ func TestClusterNodePoolsHandler_Create(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() - mockClusterSvc, mockNodePoolSvc, mockGenericSvc := tt.setupMocks(ctrl) - handler := NewClusterNodePoolsHandler(mockClusterSvc, mockNodePoolSvc, mockGenericSvc) + mockClusterSvc, mockNodePoolSvc := tt.setupMocks(ctrl) + handler := NewClusterNodePoolsHandler(mockClusterSvc, mockNodePoolSvc) reqURL := "/api/hyperfleet/v1/clusters/" + tt.clusterID + "/nodepools" req := httptest.NewRequest(http.MethodPost, reqURL, strings.NewReader(validBody)) @@ -514,8 +408,8 @@ func TestClusterNodePoolsHandler_Patch(t *testing.T) { validBody := `{"spec":{"replicas":2}}` tests := []struct { - setupMocks func(ctrl *gomock.Controller) ( //nolint:lll - *services.MockClusterService, *services.MockNodePoolService, *services.MockGenericService, + setupMocks func(ctrl *gomock.Controller) ( + *services.MockClusterService, *services.MockNodePoolService, ) name string clusterID string @@ -528,30 +422,17 @@ func TestClusterNodePoolsHandler_Patch(t *testing.T) { clusterID: clusterID, nodePoolID: nodePoolID, setupMocks: func(ctrl *gomock.Controller) ( - *services.MockClusterService, *services.MockNodePoolService, *services.MockGenericService, + *services.MockClusterService, *services.MockNodePoolService, ) { mockClusterSvc := services.NewMockClusterService(ctrl) mockNodePoolSvc := services.NewMockNodePoolService(ctrl) - mockGenericSvc := services.NewMockGenericService(ctrl) - mockClusterSvc.EXPECT().Get(gomock.Any(), clusterID).Return(&api.Cluster{ - Meta: api.Meta{ID: clusterID, CreatedTime: now, UpdatedTime: now}, - Name: "test-cluster", - }, nil) - - mockNodePoolSvc.EXPECT().Get(gomock.Any(), nodePoolID).Return(&api.NodePool{ - Meta: api.Meta{ID: nodePoolID, CreatedTime: now, UpdatedTime: now}, - Kind: "NodePool", - Name: "test-nodepool", - OwnerID: clusterID, - Spec: []byte("{}"), - Labels: []byte("{}"), - StatusConditions: []byte("[]"), - CreatedBy: "user@example.com", - UpdatedBy: "user@example.com", + mockNodePoolSvc.EXPECT().GetByIDAndOwner(gomock.Any(), nodePoolID, clusterID).Return(&api.NodePool{ + Meta: api.Meta{ID: nodePoolID, CreatedTime: now, UpdatedTime: now}, + OwnerID: clusterID, }, nil) - mockNodePoolSvc.EXPECT().Replace(gomock.Any(), gomock.Any()).Return(&api.NodePool{ + mockNodePoolSvc.EXPECT().Patch(gomock.Any(), nodePoolID, gomock.Any()).Return(&api.NodePool{ Meta: api.Meta{ID: nodePoolID, CreatedTime: now, UpdatedTime: now}, Kind: "NodePool", Name: "test-nodepool", @@ -563,101 +444,48 @@ func TestClusterNodePoolsHandler_Patch(t *testing.T) { UpdatedBy: "user@example.com", }, nil) - return mockClusterSvc, mockNodePoolSvc, mockGenericSvc + return mockClusterSvc, mockNodePoolSvc }, expectedStatusCode: http.StatusOK, expectedError: false, }, - { - name: "Error 409 - Cluster is soft-deleted", - clusterID: clusterID, - nodePoolID: nodePoolID, - setupMocks: func(ctrl *gomock.Controller) ( - *services.MockClusterService, *services.MockNodePoolService, *services.MockGenericService, - ) { - mockClusterSvc := services.NewMockClusterService(ctrl) - mockNodePoolSvc := services.NewMockNodePoolService(ctrl) - mockGenericSvc := services.NewMockGenericService(ctrl) - - deletedTime := now - mockClusterSvc.EXPECT().Get(gomock.Any(), clusterID).Return(&api.Cluster{ - Meta: api.Meta{ID: clusterID, CreatedTime: now, UpdatedTime: now}, - Name: "test-cluster", - DeletedTime: &deletedTime, - }, nil) - - return mockClusterSvc, mockNodePoolSvc, mockGenericSvc - }, - expectedStatusCode: http.StatusConflict, - expectedError: true, - }, { name: "Error 409 - NodePool is soft-deleted", clusterID: clusterID, nodePoolID: nodePoolID, setupMocks: func(ctrl *gomock.Controller) ( - *services.MockClusterService, *services.MockNodePoolService, *services.MockGenericService, + *services.MockClusterService, *services.MockNodePoolService, ) { mockClusterSvc := services.NewMockClusterService(ctrl) mockNodePoolSvc := services.NewMockNodePoolService(ctrl) - mockGenericSvc := services.NewMockGenericService(ctrl) - mockClusterSvc.EXPECT().Get(gomock.Any(), clusterID).Return(&api.Cluster{ - Meta: api.Meta{ID: clusterID, CreatedTime: now, UpdatedTime: now}, - Name: "test-cluster", + mockNodePoolSvc.EXPECT().GetByIDAndOwner(gomock.Any(), nodePoolID, clusterID).Return(&api.NodePool{ + Meta: api.Meta{ID: nodePoolID, CreatedTime: now, UpdatedTime: now}, + OwnerID: clusterID, }, nil) - deletedTime := now - mockNodePoolSvc.EXPECT().Get(gomock.Any(), nodePoolID).Return(&api.NodePool{ - Meta: api.Meta{ID: nodePoolID, CreatedTime: now, UpdatedTime: now}, - Kind: "NodePool", - Name: "test-nodepool", - OwnerID: clusterID, - DeletedTime: &deletedTime, - }, nil) + mockNodePoolSvc.EXPECT().Patch(gomock.Any(), nodePoolID, gomock.Any()). + Return(nil, errors.ConflictState("NodePool '%s' is marked for deletion", nodePoolID)) - return mockClusterSvc, mockNodePoolSvc, mockGenericSvc + return mockClusterSvc, mockNodePoolSvc }, expectedStatusCode: http.StatusConflict, expectedError: true, }, { - name: "Error 404 - Cluster not found", - clusterID: "non-existent", - nodePoolID: nodePoolID, - setupMocks: func(ctrl *gomock.Controller) ( - *services.MockClusterService, *services.MockNodePoolService, *services.MockGenericService, - ) { - mockClusterSvc := services.NewMockClusterService(ctrl) - mockNodePoolSvc := services.NewMockNodePoolService(ctrl) - mockGenericSvc := services.NewMockGenericService(ctrl) - - mockClusterSvc.EXPECT().Get(gomock.Any(), "non-existent").Return(nil, errors.NotFound("Cluster not found")) - - return mockClusterSvc, mockNodePoolSvc, mockGenericSvc - }, - expectedStatusCode: http.StatusNotFound, - expectedError: true, - }, - { - name: "Error 404 - NodePool not found", + name: "Error 404 - NodePool not found for cluster", clusterID: clusterID, nodePoolID: "non-existent-np", setupMocks: func(ctrl *gomock.Controller) ( - *services.MockClusterService, *services.MockNodePoolService, *services.MockGenericService, + *services.MockClusterService, *services.MockNodePoolService, ) { mockClusterSvc := services.NewMockClusterService(ctrl) mockNodePoolSvc := services.NewMockNodePoolService(ctrl) - mockGenericSvc := services.NewMockGenericService(ctrl) - mockClusterSvc.EXPECT().Get(gomock.Any(), clusterID).Return(&api.Cluster{ - Meta: api.Meta{ID: clusterID, CreatedTime: now, UpdatedTime: now}, - Name: "test-cluster", - }, nil) + mockNodePoolSvc.EXPECT().GetByIDAndOwner(gomock.Any(), "non-existent-np", clusterID). + Return(nil, errors.NotFound("NodePool not found for cluster")) - mockNodePoolSvc.EXPECT().Get(gomock.Any(), "non-existent-np").Return(nil, errors.NotFound("NodePool not found")) - - return mockClusterSvc, mockNodePoolSvc, mockGenericSvc + return mockClusterSvc, mockNodePoolSvc }, expectedStatusCode: http.StatusNotFound, expectedError: true, @@ -667,30 +495,15 @@ func TestClusterNodePoolsHandler_Patch(t *testing.T) { clusterID: clusterID, nodePoolID: nodePoolID, setupMocks: func(ctrl *gomock.Controller) ( - *services.MockClusterService, *services.MockNodePoolService, *services.MockGenericService, + *services.MockClusterService, *services.MockNodePoolService, ) { mockClusterSvc := services.NewMockClusterService(ctrl) mockNodePoolSvc := services.NewMockNodePoolService(ctrl) - mockGenericSvc := services.NewMockGenericService(ctrl) - - mockClusterSvc.EXPECT().Get(gomock.Any(), clusterID).Return(&api.Cluster{ - Meta: api.Meta{ID: clusterID, CreatedTime: now, UpdatedTime: now}, - Name: "test-cluster", - }, nil) - mockNodePoolSvc.EXPECT().Get(gomock.Any(), nodePoolID).Return(&api.NodePool{ - Meta: api.Meta{ID: nodePoolID, CreatedTime: now, UpdatedTime: now}, - Kind: "NodePool", - Name: "test-nodepool", - OwnerID: "different-cluster-id", - Spec: []byte("{}"), - Labels: []byte("{}"), - StatusConditions: []byte("[]"), - CreatedBy: "user@example.com", - UpdatedBy: "user@example.com", - }, nil) + mockNodePoolSvc.EXPECT().GetByIDAndOwner(gomock.Any(), nodePoolID, clusterID). + Return(nil, errors.NotFound("NodePool '%s' not found for cluster '%s'", nodePoolID, clusterID)) - return mockClusterSvc, mockNodePoolSvc, mockGenericSvc + return mockClusterSvc, mockNodePoolSvc }, expectedStatusCode: http.StatusNotFound, expectedError: true, @@ -704,8 +517,8 @@ func TestClusterNodePoolsHandler_Patch(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() - mockClusterSvc, mockNodePoolSvc, mockGenericSvc := tt.setupMocks(ctrl) - handler := NewClusterNodePoolsHandler(mockClusterSvc, mockNodePoolSvc, mockGenericSvc) + mockClusterSvc, mockNodePoolSvc := tt.setupMocks(ctrl) + handler := NewClusterNodePoolsHandler(mockClusterSvc, mockNodePoolSvc) reqURL := "/api/hyperfleet/v1/clusters/" + tt.clusterID + "/nodepools/" + tt.nodePoolID req := httptest.NewRequest(http.MethodPatch, reqURL, strings.NewReader(validBody)) diff --git a/pkg/handlers/cluster_test.go b/pkg/handlers/cluster_test.go index 63709548..4bd1466e 100644 --- a/pkg/handlers/cluster_test.go +++ b/pkg/handlers/cluster_test.go @@ -39,17 +39,7 @@ func TestClusterHandler_Patch(t *testing.T) { mockClusterSvc := services.NewMockClusterService(ctrl) mockGenericSvc := services.NewMockGenericService(ctrl) - mockClusterSvc.EXPECT().Get(gomock.Any(), clusterID).Return(&api.Cluster{ - Meta: api.Meta{ID: clusterID, CreatedTime: now, UpdatedTime: now}, - Name: "test-cluster", - Spec: []byte("{}"), - Labels: []byte("{}"), - StatusConditions: []byte("[]"), - CreatedBy: testSystemUser, - UpdatedBy: testSystemUser, - }, nil) - - mockClusterSvc.EXPECT().Replace(gomock.Any(), gomock.Any()).Return(&api.Cluster{ + mockClusterSvc.EXPECT().Patch(gomock.Any(), clusterID, gomock.Any()).Return(&api.Cluster{ Meta: api.Meta{ID: clusterID, CreatedTime: now, UpdatedTime: now}, Name: "test-cluster", Spec: []byte(`{"region":"us-east1"}`), @@ -71,12 +61,8 @@ func TestClusterHandler_Patch(t *testing.T) { mockClusterSvc := services.NewMockClusterService(ctrl) mockGenericSvc := services.NewMockGenericService(ctrl) - deletedTime := now - mockClusterSvc.EXPECT().Get(gomock.Any(), clusterID).Return(&api.Cluster{ - Meta: api.Meta{ID: clusterID, CreatedTime: now, UpdatedTime: now}, - Name: "test-cluster", - DeletedTime: &deletedTime, - }, nil) + mockClusterSvc.EXPECT().Patch(gomock.Any(), clusterID, gomock.Any()). + Return(nil, errors.ConflictState("Cluster '%s' is marked for deletion", clusterID)) return mockClusterSvc, mockGenericSvc }, @@ -90,7 +76,8 @@ func TestClusterHandler_Patch(t *testing.T) { mockClusterSvc := services.NewMockClusterService(ctrl) mockGenericSvc := services.NewMockGenericService(ctrl) - mockClusterSvc.EXPECT().Get(gomock.Any(), "non-existent").Return(nil, errors.NotFound("Cluster not found")) + mockClusterSvc.EXPECT().Patch(gomock.Any(), "non-existent", gomock.Any()). + Return(nil, errors.NotFound("Cluster not found")) return mockClusterSvc, mockGenericSvc }, diff --git a/pkg/handlers/node_pool.go b/pkg/handlers/node_pool.go index 0530284a..11b887b9 100644 --- a/pkg/handlers/node_pool.go +++ b/pkg/handlers/node_pool.go @@ -1,7 +1,6 @@ package handlers import ( - "encoding/json" "net/http" "github.com/gorilla/mux" @@ -40,7 +39,7 @@ func (h NodePoolHandler) Create(w http.ResponseWriter, r *http.Request) { ctx := r.Context() // For standalone nodepools, owner_id would need to come from somewhere // This is likely not a supported use case, but using empty string for now - nodePoolModel, convErr := presenters.ConvertNodePool(&req, "", "system@hyperfleet.local") + nodePoolModel, convErr := presenters.ConvertNodePool(&req, "") if convErr != nil { return nil, errors.GeneralError("Failed to convert nodepool: %v", convErr) } @@ -71,28 +70,8 @@ func (h NodePoolHandler) Patch(w http.ResponseWriter, r *http.Request) { Action: func() (interface{}, *errors.ServiceError) { ctx := r.Context() id := mux.Vars(r)["id"] - found, err := h.nodePool.Get(ctx, id) - if err != nil { - return nil, err - } - - if patch.Spec != nil { - specJSON, err := json.Marshal(*patch.Spec) - if err != nil { - return nil, errors.GeneralError("Failed to marshal spec: %v", err) - } - found.Spec = specJSON - } - - if patch.Labels != nil { - labelsJSON, err := json.Marshal(*patch.Labels) - if err != nil { - return nil, errors.GeneralError("Failed to marshal labels: %v", err) - } - found.Labels = labelsJSON - } - nodePoolModel, err := h.nodePool.Replace(ctx, found) + nodePoolModel, err := h.nodePool.Patch(ctx, id, &patch) if err != nil { return nil, err } @@ -115,7 +94,7 @@ func (h NodePoolHandler) List(w http.ResponseWriter, r *http.Request) { listArgs := services.NewListArguments(r.URL.Query()) var nodePools []api.NodePool - paging, err := h.generic.List(ctx, "username", listArgs, &nodePools) + paging, err := h.generic.List(ctx, listArgs, &nodePools) if err != nil { return nil, err } diff --git a/pkg/handlers/validation.go b/pkg/handlers/validation.go index 50c92fd7..415986a6 100755 --- a/pkg/handlers/validation.go +++ b/pkg/handlers/validation.go @@ -4,10 +4,19 @@ import ( "reflect" "regexp" + "github.com/google/uuid" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" ) +func validatePathID(id, name string) *errors.ServiceError { + if _, err := uuid.Parse(id); err != nil { + return errors.Validation("invalid %s format", name) + } + return nil +} + // Cluster/NodePool name pattern: compliant with Kubernetes DNS Subdomain Names (RFC 1123) // Must start and end with alphanumeric, can contain hyphens in the middle var namePattern = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`) diff --git a/pkg/services/adapter_status.go b/pkg/services/adapter_status.go index ced928f6..d2f45a8f 100644 --- a/pkg/services/adapter_status.go +++ b/pkg/services/adapter_status.go @@ -18,7 +18,6 @@ import ( type AdapterStatusService interface { Get(ctx context.Context, id string) (*api.AdapterStatus, *errors.ServiceError) Create(ctx context.Context, adapterStatus *api.AdapterStatus) (*api.AdapterStatus, *errors.ServiceError) - Replace(ctx context.Context, adapterStatus *api.AdapterStatus) (*api.AdapterStatus, *errors.ServiceError) Upsert(ctx context.Context, adapterStatus *api.AdapterStatus) (*api.AdapterStatus, *errors.ServiceError) Delete(ctx context.Context, id string) *errors.ServiceError FindByResource( @@ -63,16 +62,6 @@ func (s *sqlAdapterStatusService) Create( return adapterStatus, nil } -func (s *sqlAdapterStatusService) Replace( - ctx context.Context, adapterStatus *api.AdapterStatus, -) (*api.AdapterStatus, *errors.ServiceError) { - adapterStatus, err := s.adapterStatusDao.Replace(ctx, adapterStatus) - if err != nil { - return nil, handleUpdateError("AdapterStatus", err) - } - return adapterStatus, nil -} - func (s *sqlAdapterStatusService) Delete(ctx context.Context, id string) *errors.ServiceError { if err := s.adapterStatusDao.Delete(ctx, id); err != nil { return handleDeleteError("AdapterStatus", errors.GeneralError("Unable to delete adapter status: %s", err)) diff --git a/pkg/services/cluster.go b/pkg/services/cluster.go index af72e49a..9710ed47 100644 --- a/pkg/services/cluster.go +++ b/pkg/services/cluster.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "fmt" "time" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" @@ -13,12 +14,14 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" ) +const defaultSystemUser = "system@hyperfleet.local" + //go:generate mockgen-v0.6.0 -source=cluster.go -package=services -destination=cluster_mock.go type ClusterService interface { Get(ctx context.Context, id string) (*api.Cluster, *errors.ServiceError) Create(ctx context.Context, cluster *api.Cluster) (*api.Cluster, *errors.ServiceError) - Replace(ctx context.Context, cluster *api.Cluster) (*api.Cluster, *errors.ServiceError) + Patch(ctx context.Context, id string, patch *api.ClusterPatchRequest) (*api.Cluster, *errors.ServiceError) SoftDelete(ctx context.Context, id string) (*api.Cluster, *errors.ServiceError) All(ctx context.Context) (api.ClusterList, *errors.ServiceError) FindByIDs(ctx context.Context, ids []string) (api.ClusterList, *errors.ServiceError) @@ -33,12 +36,14 @@ type ClusterService interface { func NewClusterService( clusterDao dao.ClusterDao, nodePoolDao dao.NodePoolDao, + nodePoolService NodePoolService, adapterStatusDao dao.AdapterStatusDao, adapterConfig *config.AdapterRequirementsConfig, ) ClusterService { return &sqlClusterService{ clusterDao: clusterDao, nodePoolDao: nodePoolDao, + nodePoolService: nodePoolService, adapterStatusDao: adapterStatusDao, adapterConfig: adapterConfig, } @@ -49,6 +54,7 @@ var _ ClusterService = &sqlClusterService{} type sqlClusterService struct { clusterDao dao.ClusterDao nodePoolDao dao.NodePoolDao + nodePoolService NodePoolService adapterStatusDao dao.AdapterStatusDao adapterConfig *config.AdapterRequirementsConfig } @@ -62,6 +68,12 @@ func (s *sqlClusterService) Get(ctx context.Context, id string) (*api.Cluster, * } func (s *sqlClusterService) Create(ctx context.Context, cluster *api.Cluster) (*api.Cluster, *errors.ServiceError) { + if cluster.CreatedBy == "" { + cluster.CreatedBy = defaultSystemUser + } + if cluster.UpdatedBy == "" { + cluster.UpdatedBy = defaultSystemUser + } if cluster.Generation == 0 { cluster.Generation = 1 } @@ -79,10 +91,33 @@ func (s *sqlClusterService) Create(ctx context.Context, cluster *api.Cluster) (* return updatedCluster, nil } -func (s *sqlClusterService) Replace(ctx context.Context, cluster *api.Cluster) (*api.Cluster, *errors.ServiceError) { - cluster, err := s.clusterDao.Replace(ctx, cluster) +func (s *sqlClusterService) Patch( + ctx context.Context, id string, patch *api.ClusterPatchRequest, +) (*api.Cluster, *errors.ServiceError) { + cluster, err := s.clusterDao.GetForUpdate(ctx, id) if err != nil { - return nil, handleUpdateError("Cluster", err) + return nil, handleGetError("Cluster", "id", id, err) + } + + if cluster.DeletedTime != nil { + return nil, errors.ConflictState("Cluster '%s' is marked for deletion", id) + } + + oldSpec := cluster.Spec + oldLabels := cluster.Labels + + if applyErr := applyClusterPatch(cluster, patch); applyErr != nil { + return nil, errors.Validation("Invalid patch data: %v", applyErr) + } + + if bytes.Equal(oldSpec, cluster.Spec) && bytes.Equal(oldLabels, cluster.Labels) { + return cluster, nil + } + + cluster.IncrementGeneration() + + if saveErr := s.clusterDao.Save(ctx, cluster); saveErr != nil { + return nil, handleUpdateError("Cluster", saveErr) } updated, svcErr := s.UpdateClusterStatusFromAdapters(ctx, cluster.ID) @@ -107,37 +142,26 @@ func (s *sqlClusterService) SoftDelete(ctx context.Context, id string) (*api.Clu return cluster, nil } - t := time.Now().UTC().Truncate(time.Microsecond) - deletedBy := "system@hyperfleet.local" - cluster.DeletedTime = &t - cluster.DeletedBy = &deletedBy - cluster.Generation++ + deletedTime := time.Now().UTC().Truncate(time.Microsecond) + deletedBy := defaultSystemUser + cluster.MarkDeleted(deletedBy, deletedTime) + cluster.IncrementGeneration() if saveErr := s.clusterDao.Save(ctx, cluster); saveErr != nil { return nil, handleSoftDeleteError("Cluster", saveErr) } - if cascadeErr := s.nodePoolDao.SoftDeleteByOwner(ctx, id, t, deletedBy); cascadeErr != nil { - return nil, handleSoftDeleteError("NodePool", cascadeErr) - } - - cluster, svcErr := s.UpdateClusterStatusFromAdapters(ctx, cluster.ID) + cluster, svcErr := s.UpdateClusterStatusFromAdapters(ctx, id) if svcErr != nil { return nil, svcErr } - // Update status for all cascade-deleted nodepools so their Ready condition reflects the generation bump. - nodePools, err := s.nodePoolDao.FindSoftDeletedByOwner(ctx, id) + allNodePools, err := s.nodePoolDao.FindByOwner(ctx, id) if err != nil { - return nil, errors.GeneralError("Failed to fetch cascade-deleted nodepools: %s", err) - } - if svcErr := updateNodePoolStatusesForCascadeDelete( - ctx, - nodePools, - s.nodePoolDao, - s.adapterStatusDao, - s.adapterConfig, - ); svcErr != nil { + return nil, errors.GeneralError("Failed to fetch nodepools for cascade delete: %s", err) + } + + if svcErr := s.nodePoolService.CascadeSoftDelete(ctx, allNodePools, deletedBy, deletedTime); svcErr != nil { return nil, svcErr } @@ -178,6 +202,24 @@ func (s *sqlClusterService) OnDelete(ctx context.Context, id string) error { return nil } +func applyClusterPatch(cluster *api.Cluster, patch *api.ClusterPatchRequest) error { + if patch.Spec != nil { + specJSON, err := json.Marshal(*patch.Spec) + if err != nil { + return fmt.Errorf("failed to marshal cluster spec: %w", err) + } + cluster.Spec = specJSON + } + if patch.Labels != nil { + labelsJSON, err := json.Marshal(*patch.Labels) + if err != nil { + return fmt.Errorf("failed to marshal cluster labels: %w", err) + } + cluster.Labels = labelsJSON + } + return nil +} + func clusterRefTime(c *api.Cluster) time.Time { if c == nil { return time.Time{} diff --git a/pkg/services/cluster_test.go b/pkg/services/cluster_test.go index 9b0e52ca..338f694b 100644 --- a/pkg/services/cluster_test.go +++ b/pkg/services/cluster_test.go @@ -66,11 +66,6 @@ func (d *mockClusterDao) Create(ctx context.Context, cluster *api.Cluster) (*api return cluster, nil } -func (d *mockClusterDao) Replace(ctx context.Context, cluster *api.Cluster) (*api.Cluster, error) { - d.clusters[cluster.ID] = cluster - return cluster, nil -} - func (d *mockClusterDao) Save(ctx context.Context, cluster *api.Cluster) error { d.clusters[cluster.ID] = cluster return nil @@ -132,11 +127,6 @@ func (d *mockAdapterStatusDao) Create(ctx context.Context, status *api.AdapterSt return status, nil } -func (d *mockAdapterStatusDao) Replace(ctx context.Context, status *api.AdapterStatus) (*api.AdapterStatus, error) { - d.statuses[status.ID] = status - return status, nil -} - func (d *mockAdapterStatusDao) Upsert( ctx context.Context, status *api.AdapterStatus, existing *api.AdapterStatus, ) (*api.AdapterStatus, error) { @@ -235,6 +225,85 @@ func (d *mockAdapterStatusDao) All(ctx context.Context) (api.AdapterStatusList, return result, nil } +type mockNodePoolService struct{} + +func newMockNodePoolService() *mockNodePoolService { return &mockNodePoolService{} } + +func (m *mockNodePoolService) Get(context.Context, string) (*api.NodePool, *errors.ServiceError) { + return nil, nil +} + +func (m *mockNodePoolService) Create(context.Context, *api.NodePool) (*api.NodePool, *errors.ServiceError) { + return nil, nil +} + +func (m *mockNodePoolService) Patch( + context.Context, string, *api.NodePoolPatchRequest, +) (*api.NodePool, *errors.ServiceError) { + return nil, nil +} + +func (m *mockNodePoolService) GetByIDAndOwner( + context.Context, string, string, +) (*api.NodePool, *errors.ServiceError) { + return nil, nil +} + +func (m *mockNodePoolService) ListByCluster( + context.Context, string, *ListArguments, +) (api.NodePoolList, *api.PagingMeta, *errors.ServiceError) { + return nil, nil, nil +} + +func (m *mockNodePoolService) SoftDelete(context.Context, string) (*api.NodePool, *errors.ServiceError) { + return nil, nil +} + +func (m *mockNodePoolService) CascadeSoftDelete( + _ context.Context, nodePools api.NodePoolList, deletedBy string, deletedTime time.Time, +) *errors.ServiceError { + if deletedBy == "" { + deletedBy = defaultSystemUser + } + if deletedTime.IsZero() { + deletedTime = time.Now().UTC().Truncate(time.Microsecond) + } + for _, np := range nodePools { + if np.DeletedTime == nil { + np.MarkDeleted(deletedBy, deletedTime) + np.IncrementGeneration() + } + } + return nil +} + +func (m *mockNodePoolService) Delete(context.Context, string) *errors.ServiceError { return nil } + +func (m *mockNodePoolService) All(context.Context) (api.NodePoolList, *errors.ServiceError) { + return nil, nil +} + +func (m *mockNodePoolService) FindByIDs( + context.Context, []string, +) (api.NodePoolList, *errors.ServiceError) { + return nil, nil +} + +func (m *mockNodePoolService) UpdateNodePoolStatusFromAdapters( + context.Context, string, +) (*api.NodePool, *errors.ServiceError) { + return nil, nil +} + +func (m *mockNodePoolService) ProcessAdapterStatus( + context.Context, string, *api.AdapterStatus, +) (*api.AdapterStatus, *errors.ServiceError) { + return nil, nil +} + +func (m *mockNodePoolService) OnUpsert(context.Context, string) error { return nil } +func (m *mockNodePoolService) OnDelete(context.Context, string) error { return nil } + var _ dao.AdapterStatusDao = &mockAdapterStatusDao{} // TestProcessAdapterStatus_FirstUnknownCondition tests that the first Unknown Available condition is stored @@ -245,7 +314,7 @@ func TestProcessAdapterStatus_FirstUnknownCondition(t *testing.T) { clusterDao := newMockClusterDao() adapterStatusDao := newMockAdapterStatusDao() config := testAdapterConfig() - service := NewClusterService(clusterDao, newMockNodePoolDao(), adapterStatusDao, config) + service := NewClusterService(clusterDao, newMockNodePoolDao(), newMockNodePoolService(), adapterStatusDao, config) ctx := context.Background() clusterID := testClusterID @@ -295,7 +364,7 @@ func TestProcessAdapterStatus_SubsequentUnknownCondition(t *testing.T) { adapterStatusDao := newMockAdapterStatusDao() config := testAdapterConfig() - service := NewClusterService(clusterDao, newMockNodePoolDao(), adapterStatusDao, config) + service := NewClusterService(clusterDao, newMockNodePoolDao(), newMockNodePoolService(), adapterStatusDao, config) ctx := context.Background() clusterID := testClusterID @@ -351,7 +420,7 @@ func TestProcessAdapterStatus_InvalidStatusReturnsValidationError(t *testing.T) clusterDao := newMockClusterDao() adapterStatusDao := newMockAdapterStatusDao() config := testAdapterConfig() - service := NewClusterService(clusterDao, newMockNodePoolDao(), adapterStatusDao, config) + service := NewClusterService(clusterDao, newMockNodePoolDao(), newMockNodePoolService(), adapterStatusDao, config) ctx := context.Background() clusterID := testClusterID @@ -394,7 +463,7 @@ func TestProcessAdapterStatus_EmptyStatusReturnsValidationError(t *testing.T) { clusterDao := newMockClusterDao() adapterStatusDao := newMockAdapterStatusDao() config := testAdapterConfig() - service := NewClusterService(clusterDao, newMockNodePoolDao(), adapterStatusDao, config) + service := NewClusterService(clusterDao, newMockNodePoolDao(), newMockNodePoolService(), adapterStatusDao, config) ctx := context.Background() clusterID := testClusterID @@ -437,7 +506,7 @@ func TestProcessAdapterStatus_TrueCondition(t *testing.T) { adapterStatusDao := newMockAdapterStatusDao() config := testAdapterConfig() - service := NewClusterService(clusterDao, newMockNodePoolDao(), adapterStatusDao, config) + service := NewClusterService(clusterDao, newMockNodePoolDao(), newMockNodePoolService(), adapterStatusDao, config) ctx := context.Background() clusterID := testClusterID @@ -500,7 +569,7 @@ func TestProcessAdapterStatus_FalseCondition(t *testing.T) { adapterStatusDao := newMockAdapterStatusDao() config := testAdapterConfig() - service := NewClusterService(clusterDao, newMockNodePoolDao(), adapterStatusDao, config) + service := NewClusterService(clusterDao, newMockNodePoolDao(), newMockNodePoolService(), adapterStatusDao, config) ctx := context.Background() clusterID := testClusterID @@ -563,7 +632,7 @@ func TestProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown(t *testin adapterStatusDao := newMockAdapterStatusDao() config := testAdapterConfig() - service := NewClusterService(clusterDao, newMockNodePoolDao(), adapterStatusDao, config) + service := NewClusterService(clusterDao, newMockNodePoolDao(), newMockNodePoolService(), adapterStatusDao, config) ctx := context.Background() clusterID := testClusterID @@ -629,7 +698,7 @@ func TestProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnknown(t *t adapterStatusDao := newMockAdapterStatusDao() config := testAdapterConfig() - service := NewClusterService(clusterDao, newMockNodePoolDao(), adapterStatusDao, config) + service := NewClusterService(clusterDao, newMockNodePoolDao(), newMockNodePoolService(), adapterStatusDao, config) ctx := context.Background() clusterID := testClusterID @@ -696,7 +765,8 @@ func TestClusterAvailableReadyTransitions(t *testing.T) { // Keep this small so we can cover transitions succinctly. adapterConfig.Required.Cluster = []string{"validation", "dns"} - service := NewClusterService(clusterDao, newMockNodePoolDao(), adapterStatusDao, adapterConfig) + service := NewClusterService( + clusterDao, newMockNodePoolDao(), newMockNodePoolService(), adapterStatusDao, adapterConfig) ctx := context.Background() clusterID := testClusterID @@ -853,7 +923,8 @@ func TestClusterStaleAdapterStatusUpdatePolicy(t *testing.T) { adapterConfig := testAdapterConfig() adapterConfig.Required.Cluster = []string{"validation", "dns"} - service := NewClusterService(clusterDao, newMockNodePoolDao(), adapterStatusDao, adapterConfig) + service := NewClusterService( + clusterDao, newMockNodePoolDao(), newMockNodePoolService(), adapterStatusDao, adapterConfig) ctx := context.Background() clusterID := testClusterID @@ -931,7 +1002,8 @@ func TestClusterSyntheticTimestampsStableWithoutAdapterStatus(t *testing.T) { adapterConfig := testAdapterConfig() adapterConfig.Required.Cluster = []string{"validation"} - service := NewClusterService(clusterDao, newMockNodePoolDao(), adapterStatusDao, adapterConfig) + service := NewClusterService( + clusterDao, newMockNodePoolDao(), newMockNodePoolService(), adapterStatusDao, adapterConfig) ctx := context.Background() clusterID := testClusterID @@ -1035,7 +1107,7 @@ func TestProcessAdapterStatus_MissingMandatoryCondition_Available(t *testing.T) clusterDao := newMockClusterDao() adapterStatusDao := newMockAdapterStatusDao() config := testAdapterConfig() - service := NewClusterService(clusterDao, newMockNodePoolDao(), adapterStatusDao, config) + service := NewClusterService(clusterDao, newMockNodePoolDao(), newMockNodePoolService(), adapterStatusDao, config) ctx := context.Background() clusterID := testClusterID @@ -1121,7 +1193,7 @@ func TestProcessAdapterStatus_AllMandatoryConditions_WithCustom(t *testing.T) { clusterDao := newMockClusterDao() adapterStatusDao := newMockAdapterStatusDao() config := testAdapterConfig() - service := NewClusterService(clusterDao, newMockNodePoolDao(), adapterStatusDao, config) + service := NewClusterService(clusterDao, newMockNodePoolDao(), newMockNodePoolService(), adapterStatusDao, config) ctx := context.Background() clusterID := testClusterID @@ -1182,7 +1254,7 @@ func TestProcessAdapterStatus_CustomConditionRemoval(t *testing.T) { clusterDao := newMockClusterDao() adapterStatusDao := newMockAdapterStatusDao() config := testAdapterConfig() - service := NewClusterService(clusterDao, newMockNodePoolDao(), adapterStatusDao, config) + service := NewClusterService(clusterDao, newMockNodePoolDao(), newMockNodePoolService(), adapterStatusDao, config) ctx := context.Background() clusterID := testClusterID @@ -1266,7 +1338,7 @@ func TestClusterSoftDelete(t *testing.T) { clusterDao := newMockClusterDao() nodePoolDao := newMockNodePoolDao() adapterStatusDao := newMockAdapterStatusDao() - service := NewClusterService(clusterDao, nodePoolDao, adapterStatusDao, testAdapterConfig()) + service := NewClusterService(clusterDao, nodePoolDao, newMockNodePoolService(), adapterStatusDao, testAdapterConfig()) ctx := context.Background() clusterID := "live-cluster" clusterDao.clusters[clusterID] = &api.Cluster{ @@ -1289,7 +1361,7 @@ func TestClusterSoftDelete(t *testing.T) { clusterDao := newMockClusterDao() nodePoolDao := newMockNodePoolDao() adapterStatusDao := newMockAdapterStatusDao() - service := NewClusterService(clusterDao, nodePoolDao, adapterStatusDao, testAdapterConfig()) + service := NewClusterService(clusterDao, nodePoolDao, newMockNodePoolService(), adapterStatusDao, testAdapterConfig()) ctx := context.Background() clusterID := "cluster-with-pools" clusterDao.clusters[clusterID] = &api.Cluster{Meta: api.Meta{ID: clusterID}, Generation: 1} @@ -1304,13 +1376,130 @@ func TestClusterSoftDelete(t *testing.T) { g.Expect(nodePoolDao.nodePools["np-2"].DeletedTime).NotTo(BeNil()) }) + t.Run("cascade delete bumps nodepool generation and sets deleted_by", func(t *testing.T) { + g := NewWithT(t) + clusterDao := newMockClusterDao() + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + service := NewClusterService(clusterDao, nodePoolDao, newMockNodePoolService(), adapterStatusDao, testAdapterConfig()) + ctx := context.Background() + clusterID := "cascade-gen" + clusterDao.clusters[clusterID] = &api.Cluster{Meta: api.Meta{ID: clusterID}, Generation: 1} + nodePoolDao.nodePools["np-1"] = &api.NodePool{Meta: api.Meta{ID: "np-1"}, OwnerID: clusterID, Generation: 3} + nodePoolDao.nodePools["np-2"] = &api.NodePool{Meta: api.Meta{ID: "np-2"}, OwnerID: clusterID, Generation: 1} + + _, svcErr := service.SoftDelete(ctx, clusterID) + + g.Expect(svcErr).To(BeNil()) + g.Expect(nodePoolDao.nodePools["np-1"].Generation).To(Equal(int32(4))) + g.Expect(nodePoolDao.nodePools["np-2"].Generation).To(Equal(int32(2))) + g.Expect(nodePoolDao.nodePools["np-1"].DeletedBy).NotTo(BeNil()) + g.Expect(*nodePoolDao.nodePools["np-1"].DeletedBy).To(Equal(systemActor)) + g.Expect(nodePoolDao.nodePools["np-2"].DeletedBy).NotTo(BeNil()) + g.Expect(*nodePoolDao.nodePools["np-2"].DeletedBy).To(Equal(systemActor)) + }) + + t.Run("cascade delete recomputes nodepool status conditions", func(t *testing.T) { + g := NewWithT(t) + clusterDao := newMockClusterDao() + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + adapterConfig := testAdapterConfig() + adapterConfig.Required.Nodepool = []string{"validation"} + adapterConfig.Required.Cluster = []string{} + npService := NewNodePoolService(nodePoolDao, nil, adapterStatusDao, adapterConfig, nil) + service := NewClusterService(clusterDao, nodePoolDao, npService, adapterStatusDao, adapterConfig) + ctx := context.Background() + clusterID := "cascade-conditions" + clusterDao.clusters[clusterID] = &api.Cluster{Meta: api.Meta{ID: clusterID}, Generation: 1} + nodePoolDao.nodePools["np-1"] = &api.NodePool{ + Meta: api.Meta{ID: "np-1"}, OwnerID: clusterID, Generation: 1, + } + + now := time.Now() + adapterConds := []api.AdapterCondition{ + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionTrue, LastTransitionTime: now}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: now}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: now}, + } + condJSON, _ := json.Marshal(adapterConds) + adapterStatusDao.statuses["NodePool:np-1:validation"] = &api.AdapterStatus{ + ResourceType: "NodePool", ResourceID: "np-1", Adapter: "validation", + ObservedGeneration: 1, Conditions: condJSON, + CreatedTime: now, LastReportTime: now, + } + + _, svcErr := service.SoftDelete(ctx, clusterID) + + g.Expect(svcErr).To(BeNil()) + np := nodePoolDao.nodePools["np-1"] + g.Expect(np.Generation).To(Equal(int32(2))) + g.Expect(np.StatusConditions).NotTo(BeNil()) + + var conds []api.ResourceCondition + g.Expect(json.Unmarshal(np.StatusConditions, &conds)).To(Succeed()) + + var reconciled, available *api.ResourceCondition + for i := range conds { + switch conds[i].Type { + case api.ConditionTypeReconciled: + reconciled = &conds[i] + case api.ConditionTypeAvailable: + available = &conds[i] + } + } + g.Expect(reconciled).NotTo(BeNil()) + g.Expect(reconciled.Status).To(Equal(api.ConditionFalse), + "Reconciled=False: adapter at gen=1, nodepool at gen=2 after MarkDeleted") + g.Expect(available).NotTo(BeNil()) + g.Expect(available.Status).To(Equal(api.ConditionTrue), + "Available=True: adapter still reports Available=True") + }) + + t.Run("already-deleted nodepools are not re-marked during cascade", func(t *testing.T) { + g := NewWithT(t) + clusterDao := newMockClusterDao() + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + service := NewClusterService(clusterDao, nodePoolDao, newMockNodePoolService(), adapterStatusDao, testAdapterConfig()) + ctx := context.Background() + clusterID := "cascade-mixed" + clusterDao.clusters[clusterID] = &api.Cluster{Meta: api.Meta{ID: clusterID}, Generation: 1} + + originalDeletedTime := time.Now().Add(-time.Hour).UTC().Truncate(time.Microsecond) + originalDeletedBy := "earlier-actor" + nodePoolDao.nodePools["np-live"] = &api.NodePool{ + Meta: api.Meta{ID: "np-live"}, OwnerID: clusterID, Generation: 1, + } + nodePoolDao.nodePools["np-already-deleted"] = &api.NodePool{ + Meta: api.Meta{ID: "np-already-deleted"}, OwnerID: clusterID, Generation: 5, + DeletedTime: &originalDeletedTime, DeletedBy: &originalDeletedBy, + } + + _, svcErr := service.SoftDelete(ctx, clusterID) + + g.Expect(svcErr).To(BeNil()) + live := nodePoolDao.nodePools["np-live"] + g.Expect(live.DeletedTime).NotTo(BeNil()) + g.Expect(live.Generation).To(Equal(int32(2))) + g.Expect(*live.DeletedBy).To(Equal(systemActor)) + + deleted := nodePoolDao.nodePools["np-already-deleted"] + g.Expect(deleted.DeletedTime.Equal(originalDeletedTime)).To(BeTrue(), + "already-deleted nodepool should keep original deleted_time") + g.Expect(*deleted.DeletedBy).To(Equal(originalDeletedBy), + "already-deleted nodepool should keep original deleted_by") + g.Expect(deleted.Generation).To(Equal(int32(5)), + "already-deleted nodepool should keep original generation") + }) + t.Run("given an already-deleted cluster, when soft-deleted again, then deleted_time and generation are unchanged", func(t *testing.T) { //nolint:lll g := NewWithT(t) // Given: clusterDao := newMockClusterDao() nodePoolDao := newMockNodePoolDao() adapterStatusDao := newMockAdapterStatusDao() - service := NewClusterService(clusterDao, nodePoolDao, adapterStatusDao, testAdapterConfig()) + service := NewClusterService(clusterDao, nodePoolDao, newMockNodePoolService(), adapterStatusDao, testAdapterConfig()) ctx := context.Background() clusterID := "already-deleted" originalTime := time.Now().Add(-time.Hour) @@ -1335,7 +1524,7 @@ func TestClusterSoftDelete(t *testing.T) { clusterDao := newMockClusterDao() nodePoolDao := newMockNodePoolDao() adapterStatusDao := newMockAdapterStatusDao() - service := NewClusterService(clusterDao, nodePoolDao, adapterStatusDao, testAdapterConfig()) + service := NewClusterService(clusterDao, nodePoolDao, newMockNodePoolService(), adapterStatusDao, testAdapterConfig()) ctx := context.Background() // When: _, svcErr := service.SoftDelete(ctx, "nonexistent") @@ -1352,7 +1541,7 @@ func TestClusterSoftDelete(t *testing.T) { adapterStatusDao := newMockAdapterStatusDao() adapterConfig := testAdapterConfig() adapterConfig.Required.Cluster = []string{"validation"} - service := NewClusterService(clusterDao, nodePoolDao, adapterStatusDao, adapterConfig) + service := NewClusterService(clusterDao, nodePoolDao, newMockNodePoolService(), adapterStatusDao, adapterConfig) ctx := context.Background() clusterID := "ready-cluster" @@ -1443,7 +1632,7 @@ func TestReconciled_DuringDeletion_ChildResources(t *testing.T) { clusterDao := newMockClusterDao() nodePoolDao := newMockNodePoolDao() adapterStatusDao := newMockAdapterStatusDao() - service := NewClusterService(clusterDao, nodePoolDao, adapterStatusDao, adapterConfig) + service := NewClusterService(clusterDao, nodePoolDao, newMockNodePoolService(), adapterStatusDao, adapterConfig) cluster := makeClusterWithDeletion(clusterID, 2) clusterDao.clusters[clusterID] = cluster @@ -1491,7 +1680,7 @@ func TestReconciled_DuringDeletion_ChildResources(t *testing.T) { clusterDao := newMockClusterDao() nodePoolDao := newMockNodePoolDao() adapterStatusDao := newMockAdapterStatusDao() - service := NewClusterService(clusterDao, nodePoolDao, adapterStatusDao, adapterConfig) + service := NewClusterService(clusterDao, nodePoolDao, newMockNodePoolService(), adapterStatusDao, adapterConfig) cluster := makeClusterWithDeletion(clusterID, 2) clusterDao.clusters[clusterID] = cluster @@ -1529,3 +1718,97 @@ func TestReconciled_DuringDeletion_ChildResources(t *testing.T) { g.Expect(ready.Status).To(Equal(api.ConditionTrue)) }) } + +func TestClusterPatch(t *testing.T) { + t.Parallel() + t.Run("spec changed increments generation", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + clusterDao := newMockClusterDao() + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + adapterConfig := testAdapterConfig() + adapterConfig.Required.Cluster = []string{} + service := NewClusterService(clusterDao, nodePoolDao, newMockNodePoolService(), adapterStatusDao, adapterConfig) + ctx := context.Background() + + clusterDao.clusters["c1"] = &api.Cluster{ + Meta: api.Meta{ID: "c1"}, + Spec: []byte(`{"old":"spec"}`), + Labels: []byte(`{}`), + Generation: 1, + } + + newSpec := map[string]interface{}{"new": "spec"} + result, svcErr := service.Patch(ctx, "c1", &api.ClusterPatchRequest{Spec: &newSpec}) + + g.Expect(svcErr).To(BeNil()) + g.Expect(result.Generation).To(Equal(int32(2))) + }) + + t.Run("spec unchanged keeps generation", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + clusterDao := newMockClusterDao() + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + adapterConfig := testAdapterConfig() + adapterConfig.Required.Cluster = []string{} + service := NewClusterService(clusterDao, nodePoolDao, newMockNodePoolService(), adapterStatusDao, adapterConfig) + ctx := context.Background() + + clusterDao.clusters["c1"] = &api.Cluster{ + Meta: api.Meta{ID: "c1"}, + Spec: []byte(`{"key":"value"}`), + Labels: []byte(`{}`), + Generation: 3, + } + + sameSpec := map[string]interface{}{"key": "value"} + result, svcErr := service.Patch(ctx, "c1", &api.ClusterPatchRequest{Spec: &sameSpec}) + + g.Expect(svcErr).To(BeNil()) + g.Expect(result.Generation).To(Equal(int32(3))) + }) + + t.Run("labels changed increments generation", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + clusterDao := newMockClusterDao() + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + adapterConfig := testAdapterConfig() + adapterConfig.Required.Cluster = []string{} + service := NewClusterService(clusterDao, nodePoolDao, newMockNodePoolService(), adapterStatusDao, adapterConfig) + ctx := context.Background() + + clusterDao.clusters["c1"] = &api.Cluster{ + Meta: api.Meta{ID: "c1"}, + Spec: []byte(`{}`), + Labels: []byte(`{"env":"dev"}`), + Generation: 1, + } + + newLabels := map[string]string{"env": "prod"} + result, svcErr := service.Patch(ctx, "c1", &api.ClusterPatchRequest{Labels: &newLabels}) + + g.Expect(svcErr).To(BeNil()) + g.Expect(result.Generation).To(Equal(int32(2))) + }) + + t.Run("not found returns 404", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + clusterDao := newMockClusterDao() + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + service := NewClusterService(clusterDao, nodePoolDao, newMockNodePoolService(), adapterStatusDao, testAdapterConfig()) + ctx := context.Background() + + newSpec := map[string]interface{}{"a": "b"} + _, svcErr := service.Patch(ctx, "nonexistent", &api.ClusterPatchRequest{Spec: &newSpec}) + + g.Expect(svcErr).NotTo(BeNil()) + g.Expect(svcErr.HTTPCode).To(Equal(404)) + }) +} diff --git a/pkg/services/generic.go b/pkg/services/generic.go index 5a1b8e20..22843685 100755 --- a/pkg/services/generic.go +++ b/pkg/services/generic.go @@ -25,7 +25,7 @@ import ( type GenericService interface { List( - ctx context.Context, username string, args *ListArguments, resourceList interface{}, + ctx context.Context, args *ListArguments, resourceList interface{}, ) (*api.PagingMeta, *errors.ServiceError) } @@ -60,13 +60,12 @@ type listContext struct { disallowedFields *map[string]string joins map[string]dao.TableRelation set map[string]bool - username string resourceType string groupBy []string } func (s *sqlGenericService) newListContext( - ctx context.Context, username string, args *ListArguments, resourceList interface{}, + ctx context.Context, args *ListArguments, resourceList interface{}, ) (*listContext, interface{}, *errors.ServiceError) { resourceModel := reflect.TypeOf(resourceList).Elem().Elem() resourceTypeStr := resourceModel.Name() @@ -81,7 +80,6 @@ func (s *sqlGenericService) newListContext( return &listContext{ ctx: ctx, args: args, - username: username, pagingMeta: &api.PagingMeta{Page: args.Page}, resourceList: resourceList, disallowedFields: &disallowedFields, @@ -91,9 +89,9 @@ func (s *sqlGenericService) newListContext( // List resourceList must be a pointer to a slice of database resource objects func (s *sqlGenericService) List( - ctx context.Context, username string, args *ListArguments, resourceList interface{}, + ctx context.Context, args *ListArguments, resourceList interface{}, ) (*api.PagingMeta, *errors.ServiceError) { - listCtx, model, err := s.newListContext(ctx, username, args, resourceList) + listCtx, model, err := s.newListContext(ctx, args, resourceList) if err != nil { return nil, err } diff --git a/pkg/services/generic_test.go b/pkg/services/generic_test.go index bddf0258..dd66c139 100755 --- a/pkg/services/generic_test.go +++ b/pkg/services/generic_test.go @@ -41,7 +41,7 @@ func TestSQLTranslation(t *testing.T) { search := test["search"].(string) errorMsg := test["error"].(string) listCtx, model, serviceErr := genericService.newListContext( - context.Background(), "", &ListArguments{Search: search}, &list, + context.Background(), &ListArguments{Search: search}, &list, ) Expect(serviceErr).ToNot(HaveOccurred()) d := g.GetInstanceDao(context.Background(), model) @@ -78,7 +78,7 @@ func TestSQLTranslation(t *testing.T) { sqlReal := test["sql"].(string) valuesReal := test["values"].(types.GomegaMatcher) listCtx, _, serviceErr := genericService.newListContext( - context.Background(), "", &ListArguments{Search: search}, &list, + context.Background(), &ListArguments{Search: search}, &list, ) Expect(serviceErr).ToNot(HaveOccurred()) tslTree, err := tsl.ParseTSL(search) diff --git a/pkg/services/node_pool.go b/pkg/services/node_pool.go index 1173136d..c46e9f5d 100644 --- a/pkg/services/node_pool.go +++ b/pkg/services/node_pool.go @@ -1,8 +1,10 @@ package services import ( + "bytes" "context" "encoding/json" + "fmt" "time" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" @@ -17,8 +19,15 @@ import ( type NodePoolService interface { Get(ctx context.Context, id string) (*api.NodePool, *errors.ServiceError) Create(ctx context.Context, nodePool *api.NodePool) (*api.NodePool, *errors.ServiceError) - Replace(ctx context.Context, nodePool *api.NodePool) (*api.NodePool, *errors.ServiceError) + Patch(ctx context.Context, id string, patch *api.NodePoolPatchRequest) (*api.NodePool, *errors.ServiceError) + GetByIDAndOwner(ctx context.Context, id string, ownerID string) (*api.NodePool, *errors.ServiceError) + ListByCluster( + ctx context.Context, clusterID string, args *ListArguments, + ) (api.NodePoolList, *api.PagingMeta, *errors.ServiceError) SoftDelete(ctx context.Context, id string) (*api.NodePool, *errors.ServiceError) + CascadeSoftDelete( + ctx context.Context, nodePools api.NodePoolList, deletedBy string, deletedTime time.Time, + ) *errors.ServiceError Delete(ctx context.Context, id string) *errors.ServiceError All(ctx context.Context) (api.NodePoolList, *errors.ServiceError) @@ -36,13 +45,17 @@ type NodePoolService interface { func NewNodePoolService( nodePoolDao dao.NodePoolDao, + clusterDao dao.ClusterDao, adapterStatusDao dao.AdapterStatusDao, adapterConfig *config.AdapterRequirementsConfig, + generic GenericService, ) NodePoolService { return &sqlNodePoolService{ nodePoolDao: nodePoolDao, + clusterDao: clusterDao, adapterStatusDao: adapterStatusDao, adapterConfig: adapterConfig, + generic: generic, } } @@ -50,8 +63,10 @@ var _ NodePoolService = &sqlNodePoolService{} type sqlNodePoolService struct { nodePoolDao dao.NodePoolDao + clusterDao dao.ClusterDao adapterStatusDao dao.AdapterStatusDao adapterConfig *config.AdapterRequirementsConfig + generic GenericService } func (s *sqlNodePoolService) Get(ctx context.Context, id string) (*api.NodePool, *errors.ServiceError) { @@ -63,6 +78,12 @@ func (s *sqlNodePoolService) Get(ctx context.Context, id string) (*api.NodePool, } func (s *sqlNodePoolService) Create(ctx context.Context, nodePool *api.NodePool) (*api.NodePool, *errors.ServiceError) { + if nodePool.CreatedBy == "" { + nodePool.CreatedBy = defaultSystemUser + } + if nodePool.UpdatedBy == "" { + nodePool.UpdatedBy = defaultSystemUser + } if nodePool.Generation == 0 { nodePool.Generation = 1 } @@ -80,12 +101,33 @@ func (s *sqlNodePoolService) Create(ctx context.Context, nodePool *api.NodePool) return updatedNodePool, nil } -func (s *sqlNodePoolService) Replace( - ctx context.Context, nodePool *api.NodePool, +func (s *sqlNodePoolService) Patch( + ctx context.Context, nodePoolID string, patch *api.NodePoolPatchRequest, ) (*api.NodePool, *errors.ServiceError) { - nodePool, err := s.nodePoolDao.Replace(ctx, nodePool) + nodePool, err := s.nodePoolDao.GetForUpdate(ctx, nodePoolID) if err != nil { - return nil, handleUpdateError("NodePool", err) + return nil, handleGetError("NodePool", "id", nodePoolID, err) + } + + if nodePool.DeletedTime != nil { + return nil, errors.ConflictState("NodePool '%s' is marked for deletion", nodePoolID) + } + + oldSpec := nodePool.Spec + oldLabels := nodePool.Labels + + if applyErr := applyNodePoolPatch(nodePool, patch); applyErr != nil { + return nil, errors.Validation("Invalid patch data: %v", applyErr) + } + + if bytes.Equal(oldSpec, nodePool.Spec) && bytes.Equal(oldLabels, nodePool.Labels) { + return nodePool, nil + } + + nodePool.IncrementGeneration() + + if saveErr := s.nodePoolDao.Save(ctx, nodePool); saveErr != nil { + return nil, handleUpdateError("NodePool", saveErr) } updated, svcErr := s.UpdateNodePoolStatusFromAdapters(ctx, nodePool.ID) @@ -95,33 +137,95 @@ func (s *sqlNodePoolService) Replace( return updated, nil } -func (s *sqlNodePoolService) SoftDelete(ctx context.Context, id string) (*api.NodePool, *errors.ServiceError) { - nodePool, err := s.nodePoolDao.GetForUpdate(ctx, id) +func (s *sqlNodePoolService) GetByIDAndOwner( + ctx context.Context, nodePoolID string, clusterID string, +) (*api.NodePool, *errors.ServiceError) { + nodePool, err := s.nodePoolDao.GetByIDAndOwner(ctx, nodePoolID, clusterID) + if err != nil { + return nil, handleGetError("NodePool", "id", nodePoolID, err) + } + return nodePool, nil +} + +func (s *sqlNodePoolService) ListByCluster( + ctx context.Context, clusterID string, args *ListArguments, +) (api.NodePoolList, *api.PagingMeta, *errors.ServiceError) { + if _, err := s.clusterDao.Get(ctx, clusterID); err != nil { + return nil, nil, handleGetError("Cluster", "id", clusterID, err) + } + + if args.Search == "" { + args.Search = "owner_id = '" + clusterID + "'" + } else { + args.Search = "(" + args.Search + ") AND owner_id = '" + clusterID + "'" + } + + var nodePools []api.NodePool + paging, svcErr := s.generic.List(ctx, args, &nodePools) + if svcErr != nil { + return nil, nil, svcErr + } + + result := make(api.NodePoolList, len(nodePools)) + for i := range nodePools { + result[i] = &nodePools[i] + } + return result, paging, nil +} + +func (s *sqlNodePoolService) SoftDelete(ctx context.Context, nodePoolID string) (*api.NodePool, *errors.ServiceError) { + nodePool, err := s.nodePoolDao.GetForUpdate(ctx, nodePoolID) if err != nil { return nil, handleSoftDeleteError("NodePool", err) } - // Already marked for deletion — return as-is (idempotent). if nodePool.DeletedTime != nil { return nodePool, nil } t := time.Now().UTC().Truncate(time.Microsecond) - deletedBy := "system@hyperfleet.local" - nodePool.DeletedTime = &t - nodePool.DeletedBy = &deletedBy - nodePool.Generation++ + deletedBy := defaultSystemUser + nodePool.MarkDeleted(deletedBy, t) + nodePool.IncrementGeneration() if err := s.nodePoolDao.Save(ctx, nodePool); err != nil { return nil, handleSoftDeleteError("NodePool", err) } - nodePool, svcErr := s.UpdateNodePoolStatusFromAdapters(ctx, nodePool.ID) + updated, svcErr := s.UpdateNodePoolStatusFromAdapters(ctx, nodePool.ID) if svcErr != nil { return nil, svcErr } - return nodePool, nil + return updated, nil +} + +func (s *sqlNodePoolService) CascadeSoftDelete( + ctx context.Context, nodePools api.NodePoolList, deletedBy string, deletedTime time.Time, +) *errors.ServiceError { + if deletedBy == "" { + deletedBy = defaultSystemUser + } + if deletedTime.IsZero() { + deletedTime = time.Now().UTC().Truncate(time.Microsecond) + } + + for _, np := range nodePools { + if np.DeletedTime == nil { + np.MarkDeleted(deletedBy, deletedTime) + np.IncrementGeneration() + } + } + + if svcErr := recomputeNodePoolConditions(ctx, nodePools, s.adapterStatusDao, s.adapterConfig); svcErr != nil { + return svcErr + } + + if err := s.nodePoolDao.SaveAll(ctx, nodePools); err != nil { + return handleSoftDeleteError("NodePool", err) + } + + return nil } func (s *sqlNodePoolService) Delete(ctx context.Context, id string) *errors.ServiceError { @@ -165,6 +269,24 @@ func (s *sqlNodePoolService) OnDelete(ctx context.Context, id string) error { return nil } +func applyNodePoolPatch(nodePool *api.NodePool, patch *api.NodePoolPatchRequest) error { + if patch.Spec != nil { + specJSON, err := json.Marshal(*patch.Spec) + if err != nil { + return fmt.Errorf("failed to marshal nodepool spec: %w", err) + } + nodePool.Spec = specJSON + } + if patch.Labels != nil { + labelsJSON, err := json.Marshal(*patch.Labels) + if err != nil { + return fmt.Errorf("failed to marshal nodepool labels: %w", err) + } + nodePool.Labels = labelsJSON + } + return nil +} + func nodePoolRefTime(np *api.NodePool) time.Time { if np == nil { return time.Time{} @@ -176,7 +298,7 @@ func nodePoolRefTime(np *api.NodePool) time.Time { } // UpdateNodePoolStatusFromAdapters is the public entry point for callers outside -// ProcessAdapterStatus (e.g. Create, Replace, SoftDelete) that don't already hold the node +// ProcessAdapterStatus (e.g. Create, Patch, SoftDelete) that don't already hold the node // pool and adapter statuses. func (s *sqlNodePoolService) UpdateNodePoolStatusFromAdapters( ctx context.Context, nodePoolID string, diff --git a/pkg/services/node_pool_test.go b/pkg/services/node_pool_test.go index 6031aacf..86454bfc 100644 --- a/pkg/services/node_pool_test.go +++ b/pkg/services/node_pool_test.go @@ -13,6 +13,7 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/config" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/dao" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" ) const ( @@ -48,6 +49,17 @@ func (d *mockNodePoolDao) Get(ctx context.Context, id string) (*api.NodePool, er return nil, gorm.ErrRecordNotFound } +func (d *mockNodePoolDao) GetByIDAndOwner(ctx context.Context, id string, ownerID string) (*api.NodePool, error) { + np, err := d.Get(ctx, id) + if err != nil { + return nil, err + } + if np.OwnerID != ownerID { + return nil, gorm.ErrRecordNotFound + } + return np, nil +} + func (d *mockNodePoolDao) GetForUpdate(ctx context.Context, id string) (*api.NodePool, error) { return d.Get(ctx, id) } @@ -72,11 +84,6 @@ func (d *mockNodePoolDao) Create(ctx context.Context, nodePool *api.NodePool) (* return nodePool, nil } -func (d *mockNodePoolDao) Replace(ctx context.Context, nodePool *api.NodePool) (*api.NodePool, error) { - d.nodePools[nodePool.ID] = nodePool - return nodePool, nil -} - func (d *mockNodePoolDao) Save(ctx context.Context, nodePool *api.NodePool) error { d.nodePools[nodePool.ID] = nodePool return nil @@ -87,28 +94,6 @@ func (d *mockNodePoolDao) Delete(ctx context.Context, id string) error { return nil } -func (d *mockNodePoolDao) SoftDeleteByOwner(ctx context.Context, ownerID string, t time.Time, deletedBy string) error { - for id, np := range d.nodePools { - if np.OwnerID == ownerID && np.DeletedTime == nil { - np.DeletedTime = &t - np.DeletedBy = &deletedBy - np.Generation++ - d.nodePools[id] = np - } - } - return nil -} - -func (d *mockNodePoolDao) FindSoftDeletedByOwner(ctx context.Context, ownerID string) (api.NodePoolList, error) { - var result api.NodePoolList - for _, np := range d.nodePools { - if np.OwnerID == ownerID && np.DeletedTime != nil { - result = append(result, np) - } - } - return result, nil -} - func (d *mockNodePoolDao) FindByIDs(ctx context.Context, ids []string) (api.NodePoolList, error) { var result api.NodePoolList for _, id := range ids { @@ -129,12 +114,9 @@ func (d *mockNodePoolDao) FindByOwner(ctx context.Context, ownerID string) (api. return result, nil } -func (d *mockNodePoolDao) UpdateStatusConditionsByIDs(ctx context.Context, updates map[string][]byte) error { - for id, statusConditions := range updates { - if np, ok := d.nodePools[id]; ok { - np.StatusConditions = statusConditions - d.nodePools[id] = np - } +func (d *mockNodePoolDao) SaveAll(ctx context.Context, nodePools api.NodePoolList) error { + for _, np := range nodePools { + d.nodePools[np.ID] = np } return nil } @@ -158,6 +140,24 @@ func (d *mockNodePoolDao) All(ctx context.Context) (api.NodePoolList, error) { var _ dao.NodePoolDao = &mockNodePoolDao{} +type mockGenericService struct { + err *errors.ServiceError + nodePools []api.NodePool +} + +func (m *mockGenericService) List( + _ context.Context, _ *ListArguments, resourceList interface{}, +) (*api.PagingMeta, *errors.ServiceError) { + if m.err != nil { + return nil, m.err + } + target := resourceList.(*[]api.NodePool) + *target = m.nodePools + return &api.PagingMeta{Page: 1, Size: int64(len(m.nodePools)), Total: int64(len(m.nodePools))}, nil +} + +var _ GenericService = &mockGenericService{} + // TestNodePoolProcessAdapterStatus_FirstUnknownCondition tests that the first Unknown Available condition is stored func TestNodePoolProcessAdapterStatus_FirstUnknownCondition(t *testing.T) { t.Parallel() @@ -167,7 +167,7 @@ func TestNodePoolProcessAdapterStatus_FirstUnknownCondition(t *testing.T) { adapterStatusDao := newMockAdapterStatusDao() config := testNodePoolAdapterConfig() - service := NewNodePoolService(nodePoolDao, adapterStatusDao, config) + service := NewNodePoolService(nodePoolDao, nil, adapterStatusDao, config, nil) ctx := context.Background() nodePoolID := testNodePoolID @@ -228,7 +228,7 @@ func TestNodePoolProcessAdapterStatus_SubsequentUnknownCondition(t *testing.T) { adapterStatusDao := newMockAdapterStatusDao() config := testNodePoolAdapterConfig() - service := NewNodePoolService(nodePoolDao, adapterStatusDao, config) + service := NewNodePoolService(nodePoolDao, nil, adapterStatusDao, config, nil) ctx := context.Background() nodePoolID := testNodePoolID @@ -284,7 +284,7 @@ func TestNodePoolProcessAdapterStatus_InvalidStatusReturnsValidationError(t *tes nodePoolDao := newMockNodePoolDao() adapterStatusDao := newMockAdapterStatusDao() config := testNodePoolAdapterConfig() - service := NewNodePoolService(nodePoolDao, adapterStatusDao, config) + service := NewNodePoolService(nodePoolDao, nil, adapterStatusDao, config, nil) ctx := context.Background() nodePoolID := testNodePoolID @@ -327,7 +327,7 @@ func TestNodePoolProcessAdapterStatus_EmptyStatusReturnsValidationError(t *testi nodePoolDao := newMockNodePoolDao() adapterStatusDao := newMockAdapterStatusDao() config := testNodePoolAdapterConfig() - service := NewNodePoolService(nodePoolDao, adapterStatusDao, config) + service := NewNodePoolService(nodePoolDao, nil, adapterStatusDao, config, nil) ctx := context.Background() nodePoolID := testNodePoolID @@ -370,7 +370,7 @@ func TestNodePoolProcessAdapterStatus_TrueCondition(t *testing.T) { adapterStatusDao := newMockAdapterStatusDao() config := testNodePoolAdapterConfig() - service := NewNodePoolService(nodePoolDao, adapterStatusDao, config) + service := NewNodePoolService(nodePoolDao, nil, adapterStatusDao, config, nil) ctx := context.Background() nodePoolID := testNodePoolID @@ -434,7 +434,7 @@ func TestNodePoolProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown(t adapterStatusDao := newMockAdapterStatusDao() config := testNodePoolAdapterConfig() - service := NewNodePoolService(nodePoolDao, adapterStatusDao, config) + service := NewNodePoolService(nodePoolDao, nil, adapterStatusDao, config, nil) ctx := context.Background() nodePoolID := testNodePoolID @@ -500,7 +500,7 @@ func TestNodePoolProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnkn adapterStatusDao := newMockAdapterStatusDao() config := testNodePoolAdapterConfig() - service := NewNodePoolService(nodePoolDao, adapterStatusDao, config) + service := NewNodePoolService(nodePoolDao, nil, adapterStatusDao, config, nil) ctx := context.Background() nodePoolID := testNodePoolID @@ -566,7 +566,7 @@ func TestNodePoolAvailableReadyTransitions(t *testing.T) { adapterConfig := testNodePoolAdapterConfig() adapterConfig.Required.Nodepool = []string{"validation", "hypershift"} - service := NewNodePoolService(nodePoolDao, adapterStatusDao, adapterConfig) + service := NewNodePoolService(nodePoolDao, nil, adapterStatusDao, adapterConfig, nil) ctx := context.Background() nodePoolID := testNodePoolID @@ -736,7 +736,7 @@ func TestNodePoolStaleAdapterStatusUpdatePolicy(t *testing.T) { adapterConfig := testNodePoolAdapterConfig() adapterConfig.Required.Nodepool = []string{"validation", "hypershift"} - service := NewNodePoolService(nodePoolDao, adapterStatusDao, adapterConfig) + service := NewNodePoolService(nodePoolDao, nil, adapterStatusDao, adapterConfig, nil) ctx := context.Background() nodePoolID := testNodePoolID @@ -814,7 +814,7 @@ func TestNodePoolSyntheticTimestampsStableWithoutAdapterStatus(t *testing.T) { adapterConfig := testNodePoolAdapterConfig() adapterConfig.Required.Nodepool = []string{"validation"} - service := NewNodePoolService(nodePoolDao, adapterStatusDao, adapterConfig) + service := NewNodePoolService(nodePoolDao, nil, adapterStatusDao, adapterConfig, nil) ctx := context.Background() nodePoolID := testNodePoolID @@ -916,7 +916,7 @@ func TestNodePoolSoftDelete(t *testing.T) { // Given: nodePoolDao := newMockNodePoolDao() adapterStatusDao := newMockAdapterStatusDao() - service := NewNodePoolService(nodePoolDao, adapterStatusDao, testNodePoolAdapterConfig()) + service := NewNodePoolService(nodePoolDao, nil, adapterStatusDao, testNodePoolAdapterConfig(), nil) ctx := context.Background() nodePoolID := testNodePoolID nodePoolDao.nodePools[nodePoolID] = &api.NodePool{ @@ -938,7 +938,7 @@ func TestNodePoolSoftDelete(t *testing.T) { // Given: nodePoolDao := newMockNodePoolDao() adapterStatusDao := newMockAdapterStatusDao() - service := NewNodePoolService(nodePoolDao, adapterStatusDao, testNodePoolAdapterConfig()) + service := NewNodePoolService(nodePoolDao, nil, adapterStatusDao, testNodePoolAdapterConfig(), nil) ctx := context.Background() nodePoolID := testNodePoolID originalTime := time.Now().Add(-time.Hour) @@ -960,7 +960,7 @@ func TestNodePoolSoftDelete(t *testing.T) { // Given: nodePoolDao := newMockNodePoolDao() adapterStatusDao := newMockAdapterStatusDao() - service := NewNodePoolService(nodePoolDao, adapterStatusDao, testNodePoolAdapterConfig()) + service := NewNodePoolService(nodePoolDao, nil, adapterStatusDao, testNodePoolAdapterConfig(), nil) ctx := context.Background() // When: _, svcErr := service.SoftDelete(ctx, "nonexistent") @@ -976,7 +976,7 @@ func TestNodePoolSoftDelete(t *testing.T) { adapterStatusDao := newMockAdapterStatusDao() adapterConfig := testNodePoolAdapterConfig() adapterConfig.Required.Nodepool = []string{"validation"} - service := NewNodePoolService(nodePoolDao, adapterStatusDao, adapterConfig) + service := NewNodePoolService(nodePoolDao, nil, adapterStatusDao, adapterConfig, nil) ctx := context.Background() nodePoolID := "ready-nodepool" @@ -1027,3 +1027,224 @@ func TestNodePoolSoftDelete(t *testing.T) { g.Expect(postReady.ObservedGeneration).To(Equal(int32(2))) }) } + +func TestNodePoolPatch(t *testing.T) { + t.Parallel() + t.Run("spec changed increments generation", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + adapterConfig := testNodePoolAdapterConfig() + adapterConfig.Required.Nodepool = []string{} + service := NewNodePoolService(nodePoolDao, nil, adapterStatusDao, adapterConfig, nil) + ctx := context.Background() + + nodePoolDao.nodePools["np1"] = &api.NodePool{ + Meta: api.Meta{ID: "np1"}, + Spec: []byte(`{"old":"spec"}`), + Labels: []byte(`{}`), + Generation: 1, + } + + newSpec := map[string]interface{}{"new": "spec"} + result, svcErr := service.Patch(ctx, "np1", &api.NodePoolPatchRequest{Spec: &newSpec}) + + g.Expect(svcErr).To(BeNil()) + g.Expect(result.Generation).To(Equal(int32(2))) + }) + + t.Run("spec unchanged keeps generation", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + adapterConfig := testNodePoolAdapterConfig() + adapterConfig.Required.Nodepool = []string{} + service := NewNodePoolService(nodePoolDao, nil, adapterStatusDao, adapterConfig, nil) + ctx := context.Background() + + nodePoolDao.nodePools["np1"] = &api.NodePool{ + Meta: api.Meta{ID: "np1"}, + Spec: []byte(`{"key":"value"}`), + Labels: []byte(`{}`), + Generation: 3, + } + + sameSpec := map[string]interface{}{"key": "value"} + result, svcErr := service.Patch(ctx, "np1", &api.NodePoolPatchRequest{Spec: &sameSpec}) + + g.Expect(svcErr).To(BeNil()) + g.Expect(result.Generation).To(Equal(int32(3))) + }) + + t.Run("labels changed increments generation", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + adapterConfig := testNodePoolAdapterConfig() + adapterConfig.Required.Nodepool = []string{} + service := NewNodePoolService(nodePoolDao, nil, adapterStatusDao, adapterConfig, nil) + ctx := context.Background() + + nodePoolDao.nodePools["np1"] = &api.NodePool{ + Meta: api.Meta{ID: "np1"}, + Spec: []byte(`{}`), + Labels: []byte(`{"env":"dev"}`), + Generation: 1, + } + + newLabels := map[string]string{"env": "prod"} + result, svcErr := service.Patch(ctx, "np1", &api.NodePoolPatchRequest{Labels: &newLabels}) + + g.Expect(svcErr).To(BeNil()) + g.Expect(result.Generation).To(Equal(int32(2))) + }) + + t.Run("not found returns 404", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + service := NewNodePoolService(nodePoolDao, nil, adapterStatusDao, testNodePoolAdapterConfig(), nil) + ctx := context.Background() + + newSpec := map[string]interface{}{"a": "b"} + _, svcErr := service.Patch(ctx, "nonexistent", &api.NodePoolPatchRequest{Spec: &newSpec}) + + g.Expect(svcErr).NotTo(BeNil()) + g.Expect(svcErr.HTTPCode).To(Equal(404)) + }) +} + +func TestGetByIDAndOwner(t *testing.T) { + t.Parallel() + t.Run("happy path returns nodepool", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + nodePoolDao := newMockNodePoolDao() + service := NewNodePoolService(nodePoolDao, nil, newMockAdapterStatusDao(), testNodePoolAdapterConfig(), nil) + ctx := context.Background() + + nodePoolDao.nodePools["np1"] = &api.NodePool{ + Meta: api.Meta{ID: "np1"}, + OwnerID: "cluster1", + } + + result, svcErr := service.GetByIDAndOwner(ctx, "np1", "cluster1") + g.Expect(svcErr).To(BeNil()) + g.Expect(result.ID).To(Equal("np1")) + }) + + t.Run("owner mismatch returns 404", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + nodePoolDao := newMockNodePoolDao() + service := NewNodePoolService(nodePoolDao, nil, newMockAdapterStatusDao(), testNodePoolAdapterConfig(), nil) + ctx := context.Background() + + nodePoolDao.nodePools["np1"] = &api.NodePool{ + Meta: api.Meta{ID: "np1"}, + OwnerID: "cluster1", + } + + _, svcErr := service.GetByIDAndOwner(ctx, "np1", "wrong-cluster") + g.Expect(svcErr).NotTo(BeNil()) + g.Expect(svcErr.HTTPCode).To(Equal(404)) + }) + + t.Run("nodepool not found returns 404", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + nodePoolDao := newMockNodePoolDao() + service := NewNodePoolService(nodePoolDao, nil, newMockAdapterStatusDao(), testNodePoolAdapterConfig(), nil) + ctx := context.Background() + + _, svcErr := service.GetByIDAndOwner(ctx, "nonexistent", "cluster1") + g.Expect(svcErr).NotTo(BeNil()) + g.Expect(svcErr.HTTPCode).To(Equal(404)) + }) +} + +func TestListByCluster(t *testing.T) { + t.Parallel() + testClusterUUID := "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11" + + t.Run("happy path returns nodepools for cluster", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + nodePoolDao := newMockNodePoolDao() + clusterDao := newMockClusterDao() + genericSvc := &mockGenericService{ + nodePools: []api.NodePool{ + {Meta: api.Meta{ID: "np1"}, OwnerID: testClusterUUID}, + {Meta: api.Meta{ID: "np2"}, OwnerID: testClusterUUID}, + }, + } + service := NewNodePoolService(nodePoolDao, + clusterDao, + newMockAdapterStatusDao(), + testNodePoolAdapterConfig(), + genericSvc, + ) + ctx := context.Background() + + clusterDao.clusters[testClusterUUID] = &api.Cluster{Meta: api.Meta{ID: testClusterUUID}} + + args := &ListArguments{Page: 1, Size: 100} + result, paging, svcErr := service.ListByCluster(ctx, testClusterUUID, args) + + g.Expect(svcErr).To(BeNil()) + g.Expect(result).To(HaveLen(2)) + g.Expect(result[0].ID).To(Equal("np1")) + g.Expect(result[1].ID).To(Equal("np2")) + g.Expect(paging.Total).To(Equal(int64(2))) + }) + + t.Run("cluster not found returns 404", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + nodePoolDao := newMockNodePoolDao() + clusterDao := newMockClusterDao() + genericSvc := &mockGenericService{} + service := NewNodePoolService(nodePoolDao, + clusterDao, + newMockAdapterStatusDao(), + testNodePoolAdapterConfig(), + genericSvc, + ) + ctx := context.Background() + + nonexistentUUID := "b1ffbc99-9c0b-4ef8-bb6d-6bb9bd380a22" + args := &ListArguments{Page: 1, Size: 100} + _, _, svcErr := service.ListByCluster(ctx, nonexistentUUID, args) + + g.Expect(svcErr).NotTo(BeNil()) + g.Expect(svcErr.HTTPCode).To(Equal(404)) + }) + + t.Run("existing search is preserved and ANDed with owner_id", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + nodePoolDao := newMockNodePoolDao() + clusterDao := newMockClusterDao() + genericSvc := &mockGenericService{} + service := NewNodePoolService(nodePoolDao, + clusterDao, + newMockAdapterStatusDao(), + testNodePoolAdapterConfig(), + genericSvc, + ) + ctx := context.Background() + + clusterDao.clusters[testClusterUUID] = &api.Cluster{Meta: api.Meta{ID: testClusterUUID}} + + args := &ListArguments{Page: 1, Size: 100, Search: "name = 'test'"} + _, _, svcErr := service.ListByCluster(ctx, testClusterUUID, args) + + g.Expect(svcErr).To(BeNil()) + g.Expect(args.Search).To(ContainSubstring("name = 'test'")) + g.Expect(args.Search).To(ContainSubstring("AND owner_id = '" + testClusterUUID + "'")) + }) +} diff --git a/pkg/services/status_helpers.go b/pkg/services/status_helpers.go index eda3d0a9..617d1441 100644 --- a/pkg/services/status_helpers.go +++ b/pkg/services/status_helpers.go @@ -49,7 +49,7 @@ func computeNodePoolConditionsJSON( } // updateNodePoolStatusFromAdapters fetches a single nodepool by ID, recomputes its status -// conditions from current adapter reports, and persists the result via Replace. +// conditions from current adapter reports, and persists the result via Save. // Returns the updated nodepool unchanged if conditions have not changed. func updateNodePoolStatusFromAdapters( ctx context.Context, @@ -81,21 +81,18 @@ func updateNodePoolStatusFromAdapters( } nodePool.StatusConditions = conditionsJSON - nodePool, err = nodePoolDao.Replace(ctx, nodePool) - if err != nil { + if err = nodePoolDao.Save(ctx, nodePool); err != nil { return nil, handleUpdateError("NodePool", err) } return nodePool, nil } -// updateNodePoolStatusesForCascadeDelete recomputes and persists status conditions for a set of -// cascade-soft-deleted nodepools. Fetches all their adapter statuses in one query and writes -// only changed rows via UpdateStatusConditionsByIDs. -func updateNodePoolStatusesForCascadeDelete( +// recomputeNodePoolConditions fetches adapter statuses and recomputes status conditions +// for each nodepool, setting StatusConditions directly on the models. +func recomputeNodePoolConditions( ctx context.Context, nodePools []*api.NodePool, - nodePoolDao dao.NodePoolDao, adapterStatusDao dao.AdapterStatusDao, adapterConfig *config.AdapterRequirementsConfig, ) *errors.ServiceError { @@ -119,7 +116,6 @@ func updateNodePoolStatusesForCascadeDelete( statusesByResource[s.ResourceID] = append(statusesByResource[s.ResourceID], s) } - updates := make(map[string][]byte) requiredAdapters := adapterConfig.RequiredNodePoolAdapters() for _, np := range nodePools { @@ -128,13 +124,7 @@ func updateNodePoolStatusesForCascadeDelete( return svcErr } if conditionsJSON != nil { - updates[np.ID] = conditionsJSON - } - } - - if len(updates) > 0 { - if err := nodePoolDao.UpdateStatusConditionsByIDs(ctx, updates); err != nil { - return handleUpdateError("NodePool", err) + np.StatusConditions = conditionsJSON } } diff --git a/plugins/clusters/plugin.go b/plugins/clusters/plugin.go index e61e105a..39f57124 100644 --- a/plugins/clusters/plugin.go +++ b/plugins/clusters/plugin.go @@ -26,6 +26,7 @@ func NewServiceLocator(env *environments.Env) ServiceLocator { return services.NewClusterService( dao.NewClusterDao(&env.Database.SessionFactory), dao.NewNodePoolDao(&env.Database.SessionFactory), + nodePools.Service(&env.Services), dao.NewAdapterStatusDao(&env.Database.SessionFactory), env.Config.Adapters, ) @@ -71,7 +72,6 @@ func init() { clusterNodePoolsHandler := handlers.NewClusterNodePoolsHandler( Service(envServices), nodePools.Service(envServices), - generic.Service(envServices), ) clustersRouter.HandleFunc("/{id}/nodepools", clusterNodePoolsHandler.List).Methods(http.MethodGet) clustersRouter.HandleFunc("/{id}/nodepools", clusterNodePoolsHandler.Create).Methods(http.MethodPost) diff --git a/plugins/nodePools/plugin.go b/plugins/nodePools/plugin.go index 1b6e46a2..17822a13 100644 --- a/plugins/nodePools/plugin.go +++ b/plugins/nodePools/plugin.go @@ -23,8 +23,10 @@ func NewServiceLocator(env *environments.Env) ServiceLocator { return func() services.NodePoolService { return services.NewNodePoolService( dao.NewNodePoolDao(&env.Database.SessionFactory), + dao.NewClusterDao(&env.Database.SessionFactory), dao.NewAdapterStatusDao(&env.Database.SessionFactory), env.Config.Adapters, + generic.Service(&env.Services), ) } } From 8561b0be9d395a42c7c489ed0b08f429a405f96d Mon Sep 17 00:00:00 2001 From: Dmitrii Andreev Date: Wed, 6 May 2026 11:14:40 -0500 Subject: [PATCH 2/3] HYPERFLEET-995 - fix: use semantic JSON comparison instead of bytes.Equal --- pkg/services/cluster.go | 5 +- pkg/services/cluster_test.go | 50 ++++++++++++++++++++ pkg/services/node_pool.go | 3 +- pkg/services/node_pool_test.go | 48 +++++++++++++++++++ pkg/services/status_helpers.go | 3 +- pkg/services/util.go | 13 ++++++ pkg/services/util_test.go | 85 ++++++++++++++++++++++++++++++++++ 7 files changed, 200 insertions(+), 7 deletions(-) create mode 100644 pkg/services/util_test.go diff --git a/pkg/services/cluster.go b/pkg/services/cluster.go index 9710ed47..041e9999 100644 --- a/pkg/services/cluster.go +++ b/pkg/services/cluster.go @@ -1,7 +1,6 @@ package services import ( - "bytes" "context" "encoding/json" "fmt" @@ -110,7 +109,7 @@ func (s *sqlClusterService) Patch( return nil, errors.Validation("Invalid patch data: %v", applyErr) } - if bytes.Equal(oldSpec, cluster.Spec) && bytes.Equal(oldLabels, cluster.Labels) { + if jsonEqual(oldSpec, cluster.Spec) && jsonEqual(oldLabels, cluster.Labels) { return cluster, nil } @@ -287,7 +286,7 @@ func (s *sqlClusterService) recomputeAndSaveClusterStatus( return nil, errors.GeneralError("Failed to marshal conditions: %s", err) } - if bytes.Equal(cluster.StatusConditions, conditionsJSON) { + if jsonEqual(cluster.StatusConditions, conditionsJSON) { return cluster, nil } diff --git a/pkg/services/cluster_test.go b/pkg/services/cluster_test.go index 338f694b..bad83c7b 100644 --- a/pkg/services/cluster_test.go +++ b/pkg/services/cluster_test.go @@ -1796,6 +1796,56 @@ func TestClusterPatch(t *testing.T) { g.Expect(result.Generation).To(Equal(int32(2))) }) + t.Run("spec unchanged with different key order keeps generation", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + clusterDao := newMockClusterDao() + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + adapterConfig := testAdapterConfig() + adapterConfig.Required.Cluster = []string{} + service := NewClusterService(clusterDao, nodePoolDao, newMockNodePoolService(), adapterStatusDao, adapterConfig) + ctx := context.Background() + + clusterDao.clusters["c1"] = &api.Cluster{ + Meta: api.Meta{ID: "c1"}, + Spec: []byte(`{"z":"last","a":"first","m":"middle"}`), + Labels: []byte(`{}`), + Generation: 5, + } + + sameSpec := map[string]interface{}{"z": "last", "a": "first", "m": "middle"} + result, svcErr := service.Patch(ctx, "c1", &api.ClusterPatchRequest{Spec: &sameSpec}) + + g.Expect(svcErr).To(BeNil()) + g.Expect(result.Generation).To(Equal(int32(5))) + }) + + t.Run("labels unchanged with different key order keeps generation", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + clusterDao := newMockClusterDao() + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + adapterConfig := testAdapterConfig() + adapterConfig.Required.Cluster = []string{} + service := NewClusterService(clusterDao, nodePoolDao, newMockNodePoolService(), adapterStatusDao, adapterConfig) + ctx := context.Background() + + clusterDao.clusters["c1"] = &api.Cluster{ + Meta: api.Meta{ID: "c1"}, + Spec: []byte(`{}`), + Labels: []byte(`{"z":"zulu","a":"alpha"}`), + Generation: 4, + } + + sameLabels := map[string]string{"z": "zulu", "a": "alpha"} + result, svcErr := service.Patch(ctx, "c1", &api.ClusterPatchRequest{Labels: &sameLabels}) + + g.Expect(svcErr).To(BeNil()) + g.Expect(result.Generation).To(Equal(int32(4))) + }) + t.Run("not found returns 404", func(t *testing.T) { t.Parallel() g := NewWithT(t) diff --git a/pkg/services/node_pool.go b/pkg/services/node_pool.go index c46e9f5d..9a21dfec 100644 --- a/pkg/services/node_pool.go +++ b/pkg/services/node_pool.go @@ -1,7 +1,6 @@ package services import ( - "bytes" "context" "encoding/json" "fmt" @@ -120,7 +119,7 @@ func (s *sqlNodePoolService) Patch( return nil, errors.Validation("Invalid patch data: %v", applyErr) } - if bytes.Equal(oldSpec, nodePool.Spec) && bytes.Equal(oldLabels, nodePool.Labels) { + if jsonEqual(oldSpec, nodePool.Spec) && jsonEqual(oldLabels, nodePool.Labels) { return nodePool, nil } diff --git a/pkg/services/node_pool_test.go b/pkg/services/node_pool_test.go index 86454bfc..909ce7eb 100644 --- a/pkg/services/node_pool_test.go +++ b/pkg/services/node_pool_test.go @@ -1102,6 +1102,54 @@ func TestNodePoolPatch(t *testing.T) { g.Expect(result.Generation).To(Equal(int32(2))) }) + t.Run("spec unchanged with different key order keeps generation", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + adapterConfig := testNodePoolAdapterConfig() + adapterConfig.Required.Nodepool = []string{} + service := NewNodePoolService(nodePoolDao, nil, adapterStatusDao, adapterConfig, nil) + ctx := context.Background() + + nodePoolDao.nodePools["np1"] = &api.NodePool{ + Meta: api.Meta{ID: "np1"}, + Spec: []byte(`{"z":"last","a":"first","m":"middle"}`), + Labels: []byte(`{}`), + Generation: 5, + } + + sameSpec := map[string]interface{}{"z": "last", "a": "first", "m": "middle"} + result, svcErr := service.Patch(ctx, "np1", &api.NodePoolPatchRequest{Spec: &sameSpec}) + + g.Expect(svcErr).To(BeNil()) + g.Expect(result.Generation).To(Equal(int32(5))) + }) + + t.Run("labels unchanged with different key order keeps generation", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + adapterConfig := testNodePoolAdapterConfig() + adapterConfig.Required.Nodepool = []string{} + service := NewNodePoolService(nodePoolDao, nil, adapterStatusDao, adapterConfig, nil) + ctx := context.Background() + + nodePoolDao.nodePools["np1"] = &api.NodePool{ + Meta: api.Meta{ID: "np1"}, + Spec: []byte(`{}`), + Labels: []byte(`{"z":"zulu","a":"alpha"}`), + Generation: 4, + } + + sameLabels := map[string]string{"z": "zulu", "a": "alpha"} + result, svcErr := service.Patch(ctx, "np1", &api.NodePoolPatchRequest{Labels: &sameLabels}) + + g.Expect(svcErr).To(BeNil()) + g.Expect(result.Generation).To(Equal(int32(4))) + }) + t.Run("not found returns 404", func(t *testing.T) { t.Parallel() g := NewWithT(t) diff --git a/pkg/services/status_helpers.go b/pkg/services/status_helpers.go index 617d1441..38eb19f0 100644 --- a/pkg/services/status_helpers.go +++ b/pkg/services/status_helpers.go @@ -1,7 +1,6 @@ package services import ( - "bytes" "context" "encoding/json" @@ -41,7 +40,7 @@ func computeNodePoolConditionsJSON( return nil, errors.GeneralError("Failed to marshal conditions: %s", err) } - if bytes.Equal(np.StatusConditions, conditionsJSON) { + if jsonEqual(np.StatusConditions, conditionsJSON) { return nil, nil } diff --git a/pkg/services/util.go b/pkg/services/util.go index 4e4dfa96..d36be128 100755 --- a/pkg/services/util.go +++ b/pkg/services/util.go @@ -1,7 +1,9 @@ package services import ( + "encoding/json" e "errors" + "reflect" "strings" "gorm.io/gorm" @@ -10,6 +12,17 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" ) +func jsonEqual(a, b []byte) bool { + var va, vb any + if err := json.Unmarshal(a, &va); err != nil { + return false + } + if err := json.Unmarshal(b, &vb); err != nil { + return false + } + return reflect.DeepEqual(va, vb) +} + // Field names suspected to contain personally identifiable information var piiFields = []string{ "username", diff --git a/pkg/services/util_test.go b/pkg/services/util_test.go new file mode 100644 index 00000000..609cab27 --- /dev/null +++ b/pkg/services/util_test.go @@ -0,0 +1,85 @@ +package services + +import ( + "testing" + + . "github.com/onsi/gomega" +) + +func TestJSONEqual(t *testing.T) { + RegisterTestingT(t) + + tests := []struct { + name string + a, b []byte + expected bool + }{ + { + name: "identical bytes", + a: []byte(`{"a":1,"b":2}`), + b: []byte(`{"a":1,"b":2}`), + expected: true, + }, + { + name: "different key order same values", + a: []byte(`{"b":2,"a":1}`), + b: []byte(`{"a":1,"b":2}`), + expected: true, + }, + { + name: "nested objects different key order", + a: []byte(`{"x":{"b":2,"a":1},"y":3}`), + b: []byte(`{"y":3,"x":{"a":1,"b":2}}`), + expected: true, + }, + { + name: "different values", + a: []byte(`{"a":1}`), + b: []byte(`{"a":2}`), + expected: false, + }, + { + name: "extra key", + a: []byte(`{"a":1}`), + b: []byte(`{"a":1,"b":2}`), + expected: false, + }, + { + name: "arrays preserve order", + a: []byte(`[1,2,3]`), + b: []byte(`[1,2,3]`), + expected: true, + }, + { + name: "arrays different order not equal", + a: []byte(`[1,2,3]`), + b: []byte(`[3,2,1]`), + expected: false, + }, + { + name: "invalid json a", + a: []byte(`not json`), + b: []byte(`{"a":1}`), + expected: false, + }, + { + name: "invalid json b", + a: []byte(`{"a":1}`), + b: []byte(`not json`), + expected: false, + }, + { + name: "whitespace differences", + a: []byte(`{ "a" : 1 }`), + b: []byte(`{"a":1}`), + expected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + RegisterTestingT(t) + Expect(jsonEqual(tt.a, tt.b)).To(Equal(tt.expected)) + }) + } +} From 8dc77ea780cd829fae2fef2d6cfe9f9a8aa11ce1 Mon Sep 17 00:00:00 2001 From: Dmitrii Andreev Date: Thu, 7 May 2026 08:56:35 -0500 Subject: [PATCH 3/3] HYPERFLEET-995 - refactor: remove pointer-to-interface on SessionFactory Go interfaces are already reference types. Storing *db.SessionFactory and dereferencing with (*d.sessionFactory).New(ctx) adds unnecessary indirection. Change all DAOs to store db.SessionFactory directly. --- pkg/dao/adapter_status.go | 24 ++++++++++++------------ pkg/dao/cluster.go | 20 ++++++++++---------- pkg/dao/generic.go | 6 +++--- pkg/dao/node_pool.go | 28 ++++++++++++++-------------- pkg/services/generic_test.go | 2 +- plugins/adapterStatus/plugin.go | 2 +- plugins/clusters/plugin.go | 6 +++--- plugins/generic/plugin.go | 2 +- plugins/nodePools/plugin.go | 6 +++--- 9 files changed, 48 insertions(+), 48 deletions(-) diff --git a/pkg/dao/adapter_status.go b/pkg/dao/adapter_status.go index 0e91125c..7fec2d00 100644 --- a/pkg/dao/adapter_status.go +++ b/pkg/dao/adapter_status.go @@ -29,15 +29,15 @@ type AdapterStatusDao interface { var _ AdapterStatusDao = &sqlAdapterStatusDao{} type sqlAdapterStatusDao struct { - sessionFactory *db.SessionFactory + sessionFactory db.SessionFactory } -func NewAdapterStatusDao(sessionFactory *db.SessionFactory) AdapterStatusDao { +func NewAdapterStatusDao(sessionFactory db.SessionFactory) AdapterStatusDao { return &sqlAdapterStatusDao{sessionFactory: sessionFactory} } func (d *sqlAdapterStatusDao) Get(ctx context.Context, id string) (*api.AdapterStatus, error) { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) var adapterStatus api.AdapterStatus if err := g2.Take(&adapterStatus, "id = ?", id).Error; err != nil { return nil, err @@ -48,7 +48,7 @@ func (d *sqlAdapterStatusDao) Get(ctx context.Context, id string) (*api.AdapterS func (d *sqlAdapterStatusDao) Create( ctx context.Context, adapterStatus *api.AdapterStatus, ) (*api.AdapterStatus, error) { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) if err := g2.Omit(clause.Associations).Create(adapterStatus).Error; err != nil { db.MarkForRollback(ctx, err) return nil, err @@ -59,7 +59,7 @@ func (d *sqlAdapterStatusDao) Create( func (d *sqlAdapterStatusDao) Upsert( ctx context.Context, adapterStatus *api.AdapterStatus, existing *api.AdapterStatus, ) (*api.AdapterStatus, error) { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) if existing != nil { updateResult := g2.Model(&api.AdapterStatus{}). @@ -104,7 +104,7 @@ func (d *sqlAdapterStatusDao) Upsert( // Delete permanently removes the adapter status row from the database. func (d *sqlAdapterStatusDao) Delete(ctx context.Context, id string) error { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) adapterStatus := &api.AdapterStatus{Meta: api.Meta{ID: id}} if err := g2.Omit(clause.Associations).Delete(adapterStatus).Error; err != nil { db.MarkForRollback(ctx, err) @@ -114,7 +114,7 @@ func (d *sqlAdapterStatusDao) Delete(ctx context.Context, id string) error { } func (d *sqlAdapterStatusDao) DeleteByResource(ctx context.Context, resourceType, resourceID string) error { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) if err := g2.Where("resource_type = ? AND resource_id = ?", resourceType, resourceID). Delete(&api.AdapterStatus{}).Error; err != nil { db.MarkForRollback(ctx, err) @@ -126,7 +126,7 @@ func (d *sqlAdapterStatusDao) DeleteByResource(ctx context.Context, resourceType func (d *sqlAdapterStatusDao) FindByResource( ctx context.Context, resourceType, resourceID string, ) (api.AdapterStatusList, error) { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) statuses := api.AdapterStatusList{} query := g2.Where("resource_type = ? AND resource_id = ?", resourceType, resourceID) if err := query.Find(&statuses).Error; err != nil { @@ -138,7 +138,7 @@ func (d *sqlAdapterStatusDao) FindByResource( func (d *sqlAdapterStatusDao) FindByResourceIDs( ctx context.Context, resourceType string, resourceIDs []string, ) (api.AdapterStatusList, error) { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) statuses := api.AdapterStatusList{} if len(resourceIDs) == 0 { return statuses, nil @@ -153,7 +153,7 @@ func (d *sqlAdapterStatusDao) FindByResourceIDs( func (d *sqlAdapterStatusDao) FindByResourcePaginated( ctx context.Context, resourceType, resourceID string, offset, limit int, ) (api.AdapterStatusList, int64, error) { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) statuses := api.AdapterStatusList{} var total int64 @@ -176,7 +176,7 @@ func (d *sqlAdapterStatusDao) FindByResourcePaginated( func (d *sqlAdapterStatusDao) FindByResourceAndAdapter( ctx context.Context, resourceType, resourceID, adapter string, ) (*api.AdapterStatus, error) { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) var adapterStatus api.AdapterStatus query := g2.Where("resource_type = ? AND resource_id = ? AND adapter = ?", resourceType, resourceID, adapter) if err := query.Take(&adapterStatus).Error; err != nil { @@ -186,7 +186,7 @@ func (d *sqlAdapterStatusDao) FindByResourceAndAdapter( } func (d *sqlAdapterStatusDao) All(ctx context.Context) (api.AdapterStatusList, error) { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) statuses := api.AdapterStatusList{} if err := g2.Find(&statuses).Error; err != nil { return nil, err diff --git a/pkg/dao/cluster.go b/pkg/dao/cluster.go index a0629f58..51d6b353 100644 --- a/pkg/dao/cluster.go +++ b/pkg/dao/cluster.go @@ -23,15 +23,15 @@ type ClusterDao interface { var _ ClusterDao = &sqlClusterDao{} type sqlClusterDao struct { - sessionFactory *db.SessionFactory + sessionFactory db.SessionFactory } -func NewClusterDao(sessionFactory *db.SessionFactory) ClusterDao { +func NewClusterDao(sessionFactory db.SessionFactory) ClusterDao { return &sqlClusterDao{sessionFactory: sessionFactory} } func (d *sqlClusterDao) Get(ctx context.Context, id string) (*api.Cluster, error) { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) var cluster api.Cluster if err := g2.Take(&cluster, "id = ?", id).Error; err != nil { return nil, err @@ -40,7 +40,7 @@ func (d *sqlClusterDao) Get(ctx context.Context, id string) (*api.Cluster, error } func (d *sqlClusterDao) GetForUpdate(ctx context.Context, id string) (*api.Cluster, error) { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) var cluster api.Cluster if err := g2.Clauses(clause.Locking{Strength: "UPDATE"}).Take(&cluster, "id = ?", id).Error; err != nil { return nil, err @@ -49,7 +49,7 @@ func (d *sqlClusterDao) GetForUpdate(ctx context.Context, id string) (*api.Clust } func (d *sqlClusterDao) Create(ctx context.Context, cluster *api.Cluster) (*api.Cluster, error) { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) if err := g2.Omit(clause.Associations).Create(cluster).Error; err != nil { db.MarkForRollback(ctx, err) return nil, err @@ -58,7 +58,7 @@ func (d *sqlClusterDao) Create(ctx context.Context, cluster *api.Cluster) (*api. } func (d *sqlClusterDao) Save(ctx context.Context, cluster *api.Cluster) error { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) if err := g2.Omit(clause.Associations).Save(cluster).Error; err != nil { db.MarkForRollback(ctx, err) return err @@ -67,7 +67,7 @@ func (d *sqlClusterDao) Save(ctx context.Context, cluster *api.Cluster) error { } func (d *sqlClusterDao) SaveStatusConditions(ctx context.Context, id string, statusConditions []byte) error { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) result := g2.Model(&api.Cluster{}).Where("id = ?", id).Update("status_conditions", statusConditions) if result.Error != nil { db.MarkForRollback(ctx, result.Error) @@ -77,7 +77,7 @@ func (d *sqlClusterDao) SaveStatusConditions(ctx context.Context, id string, sta } func (d *sqlClusterDao) Delete(ctx context.Context, id string) error { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) if err := g2.Omit(clause.Associations).Delete(&api.Cluster{Meta: api.Meta{ID: id}}).Error; err != nil { db.MarkForRollback(ctx, err) return err @@ -86,7 +86,7 @@ func (d *sqlClusterDao) Delete(ctx context.Context, id string) error { } func (d *sqlClusterDao) FindByIDs(ctx context.Context, ids []string) (api.ClusterList, error) { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) clusters := api.ClusterList{} if err := g2.Where("id in (?)", ids).Find(&clusters).Error; err != nil { return nil, err @@ -95,7 +95,7 @@ func (d *sqlClusterDao) FindByIDs(ctx context.Context, ids []string) (api.Cluste } func (d *sqlClusterDao) All(ctx context.Context) (api.ClusterList, error) { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) clusters := api.ClusterList{} if err := g2.Find(&clusters).Error; err != nil { return nil, err diff --git a/pkg/dao/generic.go b/pkg/dao/generic.go index 4a595164..0f0f250e 100755 --- a/pkg/dao/generic.go +++ b/pkg/dao/generic.go @@ -41,7 +41,7 @@ type GenericDao interface { var _ GenericDao = &sqlGenericDao{} type sqlGenericDao struct { - sessionFactory *db.SessionFactory + sessionFactory db.SessionFactory g2 *gorm.DB } @@ -54,14 +54,14 @@ type TableRelation struct { ForeignColumnName string } -func NewGenericDao(sessionFactory *db.SessionFactory) GenericDao { +func NewGenericDao(sessionFactory db.SessionFactory) GenericDao { return &sqlGenericDao{sessionFactory: sessionFactory} } func (d *sqlGenericDao) GetInstanceDao(ctx context.Context, model interface{}) GenericDao { return &sqlGenericDao{ sessionFactory: d.sessionFactory, - g2: (*d.sessionFactory).New(ctx).Model(model), + g2: d.sessionFactory.New(ctx).Model(model), } } diff --git a/pkg/dao/node_pool.go b/pkg/dao/node_pool.go index 51366bab..0f58ab71 100644 --- a/pkg/dao/node_pool.go +++ b/pkg/dao/node_pool.go @@ -27,15 +27,15 @@ type NodePoolDao interface { var _ NodePoolDao = &sqlNodePoolDao{} type sqlNodePoolDao struct { - sessionFactory *db.SessionFactory + sessionFactory db.SessionFactory } -func NewNodePoolDao(sessionFactory *db.SessionFactory) NodePoolDao { +func NewNodePoolDao(sessionFactory db.SessionFactory) NodePoolDao { return &sqlNodePoolDao{sessionFactory: sessionFactory} } func (d *sqlNodePoolDao) Get(ctx context.Context, id string) (*api.NodePool, error) { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) var nodePool api.NodePool if err := g2.Take(&nodePool, "id = ?", id).Error; err != nil { return nil, err @@ -44,7 +44,7 @@ func (d *sqlNodePoolDao) Get(ctx context.Context, id string) (*api.NodePool, err } func (d *sqlNodePoolDao) GetByIDAndOwner(ctx context.Context, id string, ownerID string) (*api.NodePool, error) { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) var nodePool api.NodePool if err := g2.Take(&nodePool, "id = ? AND owner_id = ?", id, ownerID).Error; err != nil { return nil, err @@ -53,7 +53,7 @@ func (d *sqlNodePoolDao) GetByIDAndOwner(ctx context.Context, id string, ownerID } func (d *sqlNodePoolDao) GetForUpdate(ctx context.Context, id string) (*api.NodePool, error) { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) var nodePool api.NodePool if err := g2.Clauses(clause.Locking{Strength: "UPDATE"}).Take(&nodePool, "id = ?", id).Error; err != nil { return nil, err @@ -62,7 +62,7 @@ func (d *sqlNodePoolDao) GetForUpdate(ctx context.Context, id string) (*api.Node } func (d *sqlNodePoolDao) SaveStatusConditions(ctx context.Context, id string, statusConditions []byte) error { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) result := g2.Model(&api.NodePool{}).Where("id = ?", id).Update("status_conditions", statusConditions) if result.Error != nil { db.MarkForRollback(ctx, result.Error) @@ -72,7 +72,7 @@ func (d *sqlNodePoolDao) SaveStatusConditions(ctx context.Context, id string, st } func (d *sqlNodePoolDao) Create(ctx context.Context, nodePool *api.NodePool) (*api.NodePool, error) { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) if err := g2.Omit(clause.Associations).Create(nodePool).Error; err != nil { db.MarkForRollback(ctx, err) return nil, err @@ -81,7 +81,7 @@ func (d *sqlNodePoolDao) Create(ctx context.Context, nodePool *api.NodePool) (*a } func (d *sqlNodePoolDao) Save(ctx context.Context, nodePool *api.NodePool) error { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) if err := g2.Omit(clause.Associations).Save(nodePool).Error; err != nil { db.MarkForRollback(ctx, err) return err @@ -90,7 +90,7 @@ func (d *sqlNodePoolDao) Save(ctx context.Context, nodePool *api.NodePool) error } func (d *sqlNodePoolDao) Delete(ctx context.Context, id string) error { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) if err := g2.Omit(clause.Associations).Delete(&api.NodePool{Meta: api.Meta{ID: id}}).Error; err != nil { db.MarkForRollback(ctx, err) return err @@ -99,7 +99,7 @@ func (d *sqlNodePoolDao) Delete(ctx context.Context, id string) error { } func (d *sqlNodePoolDao) FindByIDs(ctx context.Context, ids []string) (api.NodePoolList, error) { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) nodePools := api.NodePoolList{} if err := g2.Where("id in (?)", ids).Find(&nodePools).Error; err != nil { return nil, err @@ -108,7 +108,7 @@ func (d *sqlNodePoolDao) FindByIDs(ctx context.Context, ids []string) (api.NodeP } func (d *sqlNodePoolDao) FindByOwner(ctx context.Context, ownerID string) (api.NodePoolList, error) { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) var nodePools api.NodePoolList if err := g2.Where("owner_id = ?", ownerID).Find(&nodePools).Error; err != nil { return nil, err @@ -120,7 +120,7 @@ func (d *sqlNodePoolDao) SaveAll(ctx context.Context, nodePools api.NodePoolList if len(nodePools) == 0 { return nil } - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) if err := g2.Omit(clause.Associations).Save(nodePools).Error; err != nil { db.MarkForRollback(ctx, err) return err @@ -129,7 +129,7 @@ func (d *sqlNodePoolDao) SaveAll(ctx context.Context, nodePools api.NodePoolList } func (d *sqlNodePoolDao) ExistsByOwner(ctx context.Context, ownerID string) (bool, error) { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) var count int64 if err := g2.Model(&api.NodePool{}).Where("owner_id = ?", ownerID).Limit(1).Count(&count).Error; err != nil { return false, err @@ -138,7 +138,7 @@ func (d *sqlNodePoolDao) ExistsByOwner(ctx context.Context, ownerID string) (boo } func (d *sqlNodePoolDao) All(ctx context.Context) (api.NodePoolList, error) { - g2 := (*d.sessionFactory).New(ctx) + g2 := d.sessionFactory.New(ctx) nodePools := api.NodePoolList{} if err := g2.Find(&nodePools).Error; err != nil { return nil, err diff --git a/pkg/services/generic_test.go b/pkg/services/generic_test.go index dd66c139..607ab70c 100755 --- a/pkg/services/generic_test.go +++ b/pkg/services/generic_test.go @@ -22,7 +22,7 @@ func TestSQLTranslation(t *testing.T) { var dbFactory db.SessionFactory = dbmocks.NewMockSessionFactory() defer dbFactory.Close() //nolint:errcheck - g := dao.NewGenericDao(&dbFactory) + g := dao.NewGenericDao(dbFactory) genericService := sqlGenericService{genericDao: g} // ill-formatted search or disallowed fields should be rejected diff --git a/plugins/adapterStatus/plugin.go b/plugins/adapterStatus/plugin.go index 199777ab..dd700d92 100644 --- a/plugins/adapterStatus/plugin.go +++ b/plugins/adapterStatus/plugin.go @@ -13,7 +13,7 @@ type ServiceLocator func() services.AdapterStatusService func NewServiceLocator(env *environments.Env) ServiceLocator { return func() services.AdapterStatusService { return services.NewAdapterStatusService( - dao.NewAdapterStatusDao(&env.Database.SessionFactory), + dao.NewAdapterStatusDao(env.Database.SessionFactory), ) } } diff --git a/plugins/clusters/plugin.go b/plugins/clusters/plugin.go index 39f57124..04812d39 100644 --- a/plugins/clusters/plugin.go +++ b/plugins/clusters/plugin.go @@ -24,10 +24,10 @@ type ServiceLocator func() services.ClusterService func NewServiceLocator(env *environments.Env) ServiceLocator { return func() services.ClusterService { return services.NewClusterService( - dao.NewClusterDao(&env.Database.SessionFactory), - dao.NewNodePoolDao(&env.Database.SessionFactory), + dao.NewClusterDao(env.Database.SessionFactory), + dao.NewNodePoolDao(env.Database.SessionFactory), nodePools.Service(&env.Services), - dao.NewAdapterStatusDao(&env.Database.SessionFactory), + dao.NewAdapterStatusDao(env.Database.SessionFactory), env.Config.Adapters, ) } diff --git a/plugins/generic/plugin.go b/plugins/generic/plugin.go index 43b1b78b..d7c7ae6f 100755 --- a/plugins/generic/plugin.go +++ b/plugins/generic/plugin.go @@ -12,7 +12,7 @@ type ServiceLocator func() services.GenericService func NewServiceLocator(env *environments.Env) ServiceLocator { return func() services.GenericService { - return services.NewGenericService(dao.NewGenericDao(&env.Database.SessionFactory)) + return services.NewGenericService(dao.NewGenericDao(env.Database.SessionFactory)) } } diff --git a/plugins/nodePools/plugin.go b/plugins/nodePools/plugin.go index 17822a13..4bc20dfb 100644 --- a/plugins/nodePools/plugin.go +++ b/plugins/nodePools/plugin.go @@ -22,9 +22,9 @@ type ServiceLocator func() services.NodePoolService func NewServiceLocator(env *environments.Env) ServiceLocator { return func() services.NodePoolService { return services.NewNodePoolService( - dao.NewNodePoolDao(&env.Database.SessionFactory), - dao.NewClusterDao(&env.Database.SessionFactory), - dao.NewAdapterStatusDao(&env.Database.SessionFactory), + dao.NewNodePoolDao(env.Database.SessionFactory), + dao.NewClusterDao(env.Database.SessionFactory), + dao.NewAdapterStatusDao(env.Database.SessionFactory), env.Config.Adapters, generic.Service(&env.Services), )