lisa/fix-workflow-grace-period-enforce

internal/service/relational/workflows/workflow_definition_service.go

@@ -2,6 +2,7 @@ package workflows
 
 import (
 	"errors"
+	"fmt"
 
 	"github.com/google/uuid"
 	"gorm.io/gorm"
@@ -24,10 +25,7 @@ func NewWorkflowDefinitionService(db *gorm.DB) *WorkflowDefinitionService {
 // Create creates a new workflow definition
 func (s *WorkflowDefinitionService) Create(definition *WorkflowDefinition) error {
 	return s.base.ValidateAndCreate(definition, "workflow definition", func() error {
-		if definition != nil && definition.Name == "" {
-			return errors.New("workflow definition name is required")
-		}
-		return nil
+		return s.ValidateDefinition(definition)
 	})
 }
 
@@ -72,6 +70,12 @@ func (s *WorkflowDefinitionService) Update(id *uuid.UUID, updates *WorkflowDefin
 		return err
 	}
 
+	updatesForValidation := *updates
+	updatesForValidation.ID = id
+	if err := s.ValidateDefinition(&updatesForValidation); err != nil {
+		return err
+	}
+
 	var existing WorkflowDefinition
 	// Don't modify the updates object, just pass it to UpdateEntity
 	return s.base.UpdateEntity(&existing, updates, id, "workflow definition")
@@ -112,6 +116,9 @@ func (s *WorkflowDefinitionService) ValidateDefinition(definition *WorkflowDefin
 	if definition.GracePeriodDays != nil && *definition.GracePeriodDays < 0 {
 		return errors.New("grace period days must be non-negative")
 	}
+	if err := s.validateGracePeriodHierarchy(definition); err != nil {
+		return err
+	}
 
 	return CombineErrors(
 		ValidateStringRequired(definition.Name, "workflow definition name"),
@@ -120,6 +127,36 @@ func (s *WorkflowDefinitionService) ValidateDefinition(definition *WorkflowDefin
 	)
 }
 
+func (s *WorkflowDefinitionService) validateGracePeriodHierarchy(definition *WorkflowDefinition) error {
+	if definition.ID == nil || definition.GracePeriodDays == nil {
+		return nil
+	}
+
+	var instance WorkflowInstance
+	err := s.db.Select("id").
+		Where("workflow_definition_id = ? AND grace_period_days IS NOT NULL AND grace_period_days > ?", definition.ID, *definition.GracePeriodDays).
+		First(&instance).Error
+	if err == nil {
+		return fmt.Errorf("workflow definition grace period days must be greater than or equal to explicit workflow instance grace period days")
+	}
+	if !errors.Is(err, gorm.ErrRecordNotFound) {
+		return err
+	}
+
+	var step WorkflowStepDefinition
+	err = s.db.Select("id").
+		Where("workflow_definition_id = ? AND grace_period_days IS NOT NULL AND grace_period_days > ?", definition.ID, *definition.GracePeriodDays).
+		First(&step).Error
+	if err == nil {
+		return fmt.Errorf("workflow definition grace period days must be greater than or equal to explicit workflow step grace period days")
+	}
+	if !errors.Is(err, gorm.ErrRecordNotFound) {
+		return err
+	}
+
+	return nil
+}
+
 // CountInstances counts the number of instances for a workflow definition
 func (s *WorkflowDefinitionService) CountInstances(id *uuid.UUID) (int64, error) {
 	return s.base.CountWhere(&WorkflowInstance{}, "workflow_definition_id = ?", id)

internal/service/relational/workflows/workflow_definition_service_test.go

@@ -146,6 +146,33 @@ func TestWorkflowDefinitionService_Update(t *testing.T) {
 	assert.Contains(t, err.Error(), "not found")
 }
 
+func TestWorkflowDefinitionService_UpdateValidatesGracePeriodHierarchyWithRouteID(t *testing.T) {
+	db := setupTestDB(t)
+	service := NewWorkflowDefinitionService(db)
+
+	definition := createTestWorkflowDefinition()
+	grace := 10
+	definition.GracePeriodDays = &grace
+	require.NoError(t, db.Create(definition).Error)
+
+	stepGrace := 7
+	step := createTestWorkflowStepDefinition(definition.ID)
+	step.GracePeriodDays = &stepGrace
+	require.NoError(t, db.Create(step).Error)
+
+	invalidGrace := 5
+	updates := &WorkflowDefinition{
+		Name:             definition.Name,
+		Version:          definition.Version,
+		SuggestedCadence: definition.SuggestedCadence,
+		GracePeriodDays:  &invalidGrace,
+	}
+
+	err := service.Update(definition.ID, updates)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "workflow definition grace period days must be greater than or equal")
+}
+
 // TestWorkflowDefinitionService_Delete tests the Delete method
 func TestWorkflowDefinitionService_Delete(t *testing.T) {
 	db := setupTestDB(t)
@@ -293,6 +320,55 @@ func TestWorkflowDefinitionService_ValidateDefinition(t *testing.T) {
 	assert.Contains(t, err.Error(), "invalid cadence")
 }
 
+func TestWorkflowDefinitionService_ValidateDefinitionGracePeriodHierarchy(t *testing.T) {
+	db := setupTestDB(t)
+	service := NewWorkflowDefinitionService(db)
+
+	validDefinitionGrace := 10
+	validDefinition := createTestWorkflowDefinition()
+	validDefinition.GracePeriodDays = &validDefinitionGrace
+	require.NoError(t, db.Create(validDefinition).Error)
+
+	validInstanceGrace := 7
+	validInstance := createTestWorkflowInstance(validDefinition.ID)
+	validInstance.GracePeriodDays = &validInstanceGrace
+	require.NoError(t, db.Create(validInstance).Error)
+
+	validStepGrace := 6
+	validStep := createTestWorkflowStepDefinition(validDefinition.ID)
+	validStep.GracePeriodDays = &validStepGrace
+	require.NoError(t, db.Create(validStep).Error)
+
+	err := service.ValidateDefinition(validDefinition)
+	require.NoError(t, err)
+
+	definition := createTestWorkflowDefinition()
+	grace := 5
+	definition.GracePeriodDays = &grace
+	require.NoError(t, db.Create(definition).Error)
+
+	instanceGrace := 7
+	instance := createTestWorkflowInstance(definition.ID)
+	instance.GracePeriodDays = &instanceGrace
+	require.NoError(t, db.Create(instance).Error)
+
+	err = service.ValidateDefinition(definition)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "workflow definition grace period days must be greater than or equal")
+
+	instance.GracePeriodDays = nil
+	require.NoError(t, db.Save(instance).Error)
+
+	stepGrace := 6
+	step := createTestWorkflowStepDefinition(definition.ID)
+	step.GracePeriodDays = &stepGrace
+	require.NoError(t, db.Create(step).Error)
+
+	err = service.ValidateDefinition(definition)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "workflow definition grace period days must be greater than or equal")
+}
+
 // TestWorkflowDefinitionService_CountInstances tests the CountInstances method
 func TestWorkflowDefinitionService_CountInstances(t *testing.T) {
 	db := setupTestDB(t)

internal/service/relational/workflows/workflow_instance_service.go

@@ -3,6 +3,7 @@ package workflows
 import (
 	"context"
 	"errors"
+	"fmt"
 	"time"
 
 	"github.com/google/uuid"
@@ -209,6 +210,9 @@ func (s *WorkflowInstanceService) ValidateInstance(instance *WorkflowInstance) e
 	if instance.GracePeriodDays != nil && *instance.GracePeriodDays < 0 {
 		return errors.New("grace period days must be non-negative")
 	}
+	if err := s.validateGracePeriodHierarchy(instance); err != nil {
+		return err
+	}
 
 	return CombineErrors(
 		ValidateStringRequired(instance.Name, "instance name"),
@@ -219,6 +223,36 @@ func (s *WorkflowInstanceService) ValidateInstance(instance *WorkflowInstance) e
 	)
 }
 
+func (s *WorkflowInstanceService) validateGracePeriodHierarchy(instance *WorkflowInstance) error {
+	if instance.WorkflowDefinitionID == nil || instance.GracePeriodDays == nil {
+		return nil
+	}
+
+	var definition WorkflowDefinition
+	if err := s.db.Select("id", "grace_period_days").First(&definition, instance.WorkflowDefinitionID).Error; err != nil {
+		if errors.Is(err, gorm.ErrRecordNotFound) {
+			return fmt.Errorf("workflow definition with id %s not found", instance.WorkflowDefinitionID.String())
+		}
+		return err
+	}
+	if definition.GracePeriodDays != nil && *instance.GracePeriodDays > *definition.GracePeriodDays {
+		return fmt.Errorf("workflow instance grace period days must be less than or equal to workflow definition grace period days")
+	}
+
+	var step WorkflowStepDefinition
+	err := s.db.Select("id").
+		Where("workflow_definition_id = ? AND grace_period_days IS NOT NULL AND grace_period_days > ?", instance.WorkflowDefinitionID, *instance.GracePeriodDays).
+		First(&step).Error
+	if err == nil {
+		return fmt.Errorf("workflow instance grace period days must be greater than or equal to explicit workflow step grace period days")
+	}
+	if !errors.Is(err, gorm.ErrRecordNotFound) {
+		return err
+	}
+
+	return nil
+}
+
 // CalculateNextSchedule calculates the next scheduled time based on cadence
 func (s *WorkflowInstanceService) CalculateNextSchedule(from time.Time, cadence string) time.Time {
 	cadenceType := CadenceType(cadence)

internal/service/relational/workflows/workflow_instance_service_test.go

@@ -579,6 +579,51 @@ func TestWorkflowInstanceService_ValidateInstance(t *testing.T) {
 	assert.Contains(t, err.Error(), "invalid cadence")
 }
 
+func TestWorkflowInstanceService_ValidateInstanceGracePeriodHierarchy(t *testing.T) {
+	db := setupTestDB(t)
+	service := NewWorkflowInstanceService(db)
+
+	missingDefinitionID := uuid.New()
+	missingDefinitionGrace := 1
+	missingDefinitionInstance := createTestWorkflowInstance(&missingDefinitionID)
+	missingDefinitionInstance.GracePeriodDays = &missingDefinitionGrace
+
+	err := service.ValidateInstance(missingDefinitionInstance)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "workflow definition with id")
+	assert.Contains(t, err.Error(), missingDefinitionID.String())
+
+	definitionGrace := 5
+	definition := createTestWorkflowDefinition()
+	definition.GracePeriodDays = &definitionGrace
+	require.NoError(t, db.Create(definition).Error)
+
+	validInstanceGrace := 5
+	validInstance := createTestWorkflowInstance(definition.ID)
+	validInstance.GracePeriodDays = &validInstanceGrace
+	err = service.ValidateInstance(validInstance)
+	require.NoError(t, err)
+
+	instanceGrace := 6
+	instance := createTestWorkflowInstance(definition.ID)
+	instance.GracePeriodDays = &instanceGrace
+
+	err = service.ValidateInstance(instance)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "workflow instance grace period days must be less than or equal")
+
+	instanceGrace = 4
+	instance.GracePeriodDays = &instanceGrace
+	stepGrace := 5
+	step := createTestWorkflowStepDefinition(definition.ID)
+	step.GracePeriodDays = &stepGrace
+	require.NoError(t, db.Create(step).Error)
+
+	err = service.ValidateInstance(instance)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "workflow instance grace period days must be greater than or equal")
+}
+
 // TestWorkflowInstanceService_calculateNextSchedule tests the calculateNextSchedule method
 func TestWorkflowInstanceService_calculateNextSchedule(t *testing.T) {
 	db := setupTestDB(t)

internal/service/relational/workflows/workflow_step_definition_service.go

@@ -64,7 +64,9 @@ func (s *WorkflowStepDefinitionService) GetByWorkflowDefinitionID(workflowDefID
 // Update updates an existing workflow step definition
 func (s *WorkflowStepDefinitionService) Update(id *uuid.UUID, updates *WorkflowStepDefinition) error {
 	var existing WorkflowStepDefinition
-	return s.base.ValidateAndUpdate(&existing, updates, id, "workflow step definition", nil)
+	return s.base.ValidateAndUpdate(&existing, updates, id, "workflow step definition", func() error {
+		return s.ValidateStep(updates)
+	})
 }
 
 // Delete soft deletes a workflow step definition
@@ -191,6 +193,39 @@ func (s *WorkflowStepDefinitionService) ValidateStep(step *WorkflowStepDefinitio
 	if step.GracePeriodDays != nil && *step.GracePeriodDays < 0 {
 		return errors.New("grace period days must be non-negative")
 	}
+	if err := s.validateGracePeriodHierarchy(step); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func (s *WorkflowStepDefinitionService) validateGracePeriodHierarchy(step *WorkflowStepDefinition) error {
+	if step.WorkflowDefinitionID == nil || step.GracePeriodDays == nil {
+		return nil
+	}
+
+	var definition WorkflowDefinition
+	if err := s.db.Select("id", "grace_period_days").First(&definition, step.WorkflowDefinitionID).Error; err != nil {
+		if errors.Is(err, gorm.ErrRecordNotFound) {
+			return fmt.Errorf("workflow definition with id %s not found", step.WorkflowDefinitionID.String())
+		}
+		return err
+	}
+	if definition.GracePeriodDays != nil && *step.GracePeriodDays > *definition.GracePeriodDays {
+		return fmt.Errorf("workflow step grace period days must be less than or equal to workflow definition grace period days")
+	}
+
+	var instance WorkflowInstance
+	err := s.db.Select("id").
+		Where("workflow_definition_id = ? AND grace_period_days IS NOT NULL AND grace_period_days < ?", step.WorkflowDefinitionID, *step.GracePeriodDays).
+		First(&instance).Error
+	if err == nil {
+		return fmt.Errorf("workflow step grace period days must be less than or equal to explicit workflow instance grace period days")
+	}
+	if !errors.Is(err, gorm.ErrRecordNotFound) {
+		return err
+	}
 
 	return nil
 }

internal/service/relational/workflows/workflow_step_definition_service_test.go

@@ -174,6 +174,7 @@ func TestWorkflowStepDefinitionService_Update(t *testing.T) {
 		UUIDModel:            relational.UUIDModel{ID: &nonExistentID},
 		WorkflowDefinitionID: workflowDef.ID, // Include the workflow definition ID
 		Name:                 "Updated Step Definition",
+		ResponsibleRole:      "updated_role",
 	}
 	err = service.Update(&nonExistentID, updatesWithNonExistentID)
 	assert.Error(t, err)
@@ -546,6 +547,51 @@ func TestWorkflowStepDefinitionService_ValidateStep(t *testing.T) {
 	assert.Contains(t, err.Error(), "workflow definition ID is required")
 }
 
+func TestWorkflowStepDefinitionService_ValidateStepGracePeriodHierarchy(t *testing.T) {
+	db := setupTestDB(t)
+	service := NewWorkflowStepDefinitionService(db)
+
+	missingDefinitionID := uuid.New()
+	missingDefinitionGrace := 1
+	missingDefinitionStep := createTestWorkflowStepDefinition(&missingDefinitionID)
+	missingDefinitionStep.GracePeriodDays = &missingDefinitionGrace
+
+	err := service.ValidateStep(missingDefinitionStep)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "workflow definition with id")
+	assert.Contains(t, err.Error(), missingDefinitionID.String())
+
+	definitionGrace := 5
+	definition := createTestWorkflowDefinition()
+	definition.GracePeriodDays = &definitionGrace
+	require.NoError(t, db.Create(definition).Error)
+
+	validStepGrace := 5
+	validStep := createTestWorkflowStepDefinition(definition.ID)
+	validStep.GracePeriodDays = &validStepGrace
+	err = service.ValidateStep(validStep)
+	require.NoError(t, err)
+
+	stepGrace := 6
+	step := createTestWorkflowStepDefinition(definition.ID)
+	step.GracePeriodDays = &stepGrace
+
+	err = service.ValidateStep(step)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "workflow step grace period days must be less than or equal")
+
+	stepGrace = 4
+	step.GracePeriodDays = &stepGrace
+	instanceGrace := 3
+	instance := createTestWorkflowInstance(definition.ID)
+	instance.GracePeriodDays = &instanceGrace
+	require.NoError(t, db.Create(instance).Error)
+
+	err = service.ValidateStep(step)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "workflow step grace period days must be less than or equal")
+}
+
 // TestWorkflowStepDefinitionService_Integration tests integration scenarios
 func TestWorkflowStepDefinitionService_Integration(t *testing.T) {
 	db := setupTestDB(t)