Skip to content

Commit a36812c

Browse files
committed
fix: address CodeRabbit review feedback on cron validation
- Rename parsScheduledTrigger -> parseScheduledTrigger (typo) - Return hasCron bool to distinguish scheduled:name from scheduled:name: so an explicit empty cron suffix is rejected with INVALID_CRON_EXPRESSION - Use fixed anchor and 10-sample minimum for interval check instead of sampling only 2 occurrences from now, avoiding time-dependent results
1 parent b6f74d6 commit a36812c

2 files changed

Lines changed: 50 additions & 12 deletions

File tree

services/control-plane/internal/validator/domain_validator.go

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -396,7 +396,7 @@ func (v *ManifestValidator) validateScheduledTriggers(
396396
scheduledCount++
397397

398398
remainder := strings.TrimPrefix(trigger, "scheduled:")
399-
name, cronExpr := parsScheduledTrigger(remainder)
399+
name, cronExpr, hasCron := parseScheduledTrigger(remainder)
400400

401401
if firstIdx, exists := seen[name]; exists {
402402
addError(result, ValidationError{
@@ -409,8 +409,19 @@ func (v *ManifestValidator) validateScheduledTriggers(
409409
seen[name] = i
410410
}
411411

412-
if cronExpr != "" {
413-
v.validateCronExpression(cronExpr, fmt.Sprintf("sagas[%d].trigger", i), saga.GetName(), result)
412+
if hasCron {
413+
if cronExpr == "" {
414+
addError(result, ValidationError{
415+
Severity: SeverityError,
416+
Path: fmt.Sprintf("sagas[%d].trigger", i),
417+
Code: "INVALID_CRON_EXPRESSION",
418+
Message: "cron expression cannot be empty; omit the trailing colon or provide a valid expression",
419+
ResourceType: "saga",
420+
ResourceID: saga.GetName(),
421+
})
422+
} else {
423+
v.validateCronExpression(cronExpr, fmt.Sprintf("sagas[%d].trigger", i), saga.GetName(), result)
424+
}
414425
}
415426
}
416427

@@ -424,14 +435,15 @@ func (v *ManifestValidator) validateScheduledTriggers(
424435
}
425436
}
426437

427-
// parsScheduledTrigger splits "name" or "name:cron expr" into (name, cronExpr).
438+
// parseScheduledTrigger splits "name" or "name:cron expr" into (name, cronExpr, hasCron).
439+
// hasCron is true when a colon separator was present, even if the expression is empty.
428440
// The cron expression may contain spaces, so only the first colon is used as separator.
429-
func parsScheduledTrigger(remainder string) (name, cronExpr string) {
441+
func parseScheduledTrigger(remainder string) (name, cronExpr string, hasCron bool) {
430442
idx := strings.Index(remainder, ":")
431443
if idx < 0 {
432-
return remainder, ""
444+
return remainder, "", false
433445
}
434-
return remainder[:idx], remainder[idx+1:]
446+
return remainder[:idx], remainder[idx+1:], true
435447
}
436448

437449
// validateCronExpression parses a cron expression and checks interval constraints.
@@ -449,11 +461,20 @@ func (v *ManifestValidator) validateCronExpression(expr, path, sagaName string,
449461
return
450462
}
451463

452-
// Compute next two occurrences to determine the interval.
453-
now := time.Now()
454-
t1 := schedule.Next(now)
455-
t2 := schedule.Next(t1)
456-
interval := t2.Sub(t1)
464+
// Sample 10 consecutive occurrences from a fixed anchor to find the minimum interval.
465+
// A fixed anchor avoids results that vary with the current time (e.g. daylight saving transitions).
466+
anchor := time.Date(2001, 1, 1, 0, 0, 0, 0, time.UTC)
467+
const sampleSize = 10
468+
prev := schedule.Next(anchor)
469+
minInterval := time.Duration(1<<63 - 1) // max duration
470+
for range sampleSize - 1 {
471+
next := schedule.Next(prev)
472+
if gap := next.Sub(prev); gap < minInterval {
473+
minInterval = gap
474+
}
475+
prev = next
476+
}
477+
interval := minInterval
457478

458479
if interval < minCronInterval {
459480
addError(result, ValidationError{

services/control-plane/internal/validator/domain_validator_test.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -419,6 +419,23 @@ func TestValidateScheduledTriggers_ValidCronExpression(t *testing.T) {
419419
assert.Empty(t, result.Warnings)
420420
}
421421

422+
func TestValidateScheduledTriggers_EmptyCronSuffix_Errors(t *testing.T) {
423+
v, err := New()
424+
require.NoError(t, err)
425+
426+
// scheduled:<name>: with trailing colon but empty cron is invalid
427+
manifest := &controlplanev1.Manifest{
428+
Sagas: []*controlplanev1.SagaDefinition{
429+
{Name: "billing", Trigger: "scheduled:billing:"},
430+
},
431+
}
432+
433+
result := &ValidationResult{Valid: true}
434+
v.validateScheduledTriggers(manifest, result)
435+
require.Len(t, result.Errors, 1)
436+
assert.Equal(t, "INVALID_CRON_EXPRESSION", result.Errors[0].Code)
437+
}
438+
422439
func TestValidateScheduledTriggers_NoCronExpression_Valid(t *testing.T) {
423440
v, err := New()
424441
require.NoError(t, err)

0 commit comments

Comments
 (0)