diff --git a/runs/service/converter/literal_json_converter.go b/runs/service/converter/literal_json_converter.go index e525bfcdef..66ef821f29 100644 --- a/runs/service/converter/literal_json_converter.go +++ b/runs/service/converter/literal_json_converter.go @@ -4,6 +4,7 @@ import ( "context" "encoding/base64" "fmt" + "math" "reflect" "regexp" "strconv" @@ -23,6 +24,154 @@ import ( var digitsOnlyRegex = regexp.MustCompile(`^\d+$`) +// iso8601DurationRegex matches an ISO-8601 duration (e.g. "P2DT3H4M5S", "PT1.5S", "-PT30M"). +// The two "M" components are disambiguated by position: months precede "T", minutes follow it. +var iso8601DurationRegex = regexp.MustCompile(`^(-)?P(?:(\d+)Y)?(?:(\d+)M)?(?:(\d+)W)?(?:(\d+)D)?(?:T(?:(\d+)H)?(?:(\d+)M)?(?:(\d+(?:\.\d+)?)S)?)?$`) + +// Fixed component lengths used to convert ISO-8601 durations to/from seconds. Years and months +// are inherently ambiguous; these match the launch form frontend (dayjs: 365-day years, +// 30-day months) so values round-trip between the UI and the backend. +const ( + secondsPerMinute = 60 + secondsPerHour = 60 * secondsPerMinute + secondsPerDay = 24 * secondsPerHour + secondsPerWeek = 7 * secondsPerDay + secondsPerMonth = 30 * secondsPerDay + secondsPerYear = 365 * secondsPerDay +) + +// isISO8601Duration reports whether s looks like an ISO-8601 duration string. +func isISO8601Duration(s string) bool { + s = strings.TrimSpace(s) + return strings.HasPrefix(s, "P") || strings.HasPrefix(s, "-P") +} + +// parseISO8601Duration parses an ISO-8601 duration string (e.g. "P2DT3H4M5S") into a +// durationpb.Duration. Returns an error if the string is not a valid ISO-8601 duration. +func parseISO8601Duration(s string) (*durationpb.Duration, error) { + matches := iso8601DurationRegex.FindStringSubmatch(strings.TrimSpace(s)) + if matches == nil { + return nil, fmt.Errorf("invalid ISO-8601 duration: %q", s) + } + + // Require at least one component so bare "P"/"PT" are rejected. + hasComponent := false + for _, group := range matches[2:] { + if group != "" { + hasComponent = true + break + } + } + if !hasComponent { + return nil, fmt.Errorf("invalid ISO-8601 duration: %q", s) + } + + parseInt := func(v string) (int64, error) { + if v == "" { + return 0, nil + } + return strconv.ParseInt(v, 10, 64) + } + + years, err := parseInt(matches[2]) + if err != nil { + return nil, err + } + months, err := parseInt(matches[3]) + if err != nil { + return nil, err + } + weeks, err := parseInt(matches[4]) + if err != nil { + return nil, err + } + days, err := parseInt(matches[5]) + if err != nil { + return nil, err + } + hours, err := parseInt(matches[6]) + if err != nil { + return nil, err + } + minutes, err := parseInt(matches[7]) + if err != nil { + return nil, err + } + + totalSeconds := years*secondsPerYear + months*secondsPerMonth + weeks*secondsPerWeek + + days*secondsPerDay + hours*secondsPerHour + minutes*secondsPerMinute + + var nanos int32 + if matches[8] != "" { + secondsFloat, err := strconv.ParseFloat(matches[8], 64) + if err != nil { + return nil, err + } + wholeSeconds := int64(secondsFloat) + totalSeconds += wholeSeconds + nanos = int32(math.Round((secondsFloat - float64(wholeSeconds)) * 1e9)) + } + + duration := &durationpb.Duration{Seconds: totalSeconds, Nanos: nanos} + if matches[1] == "-" { + duration.Seconds = -duration.Seconds + duration.Nanos = -duration.Nanos + } + return duration, nil +} + +// formatISO8601Duration converts a time.Duration into an ISO-8601 duration string +// (e.g. "P2DT3H4M5S"). Only days/hours/minutes/seconds are emitted (never years/months) so the +// representation is unambiguous; the launch form frontend renders it back to a readable string. +func formatISO8601Duration(d time.Duration) string { + if d == 0 { + return "P0D" + } + negative := d < 0 + if negative { + d = -d + } + + const nanosPerSecond = int64(1e9) + totalNanos := d.Nanoseconds() + totalSeconds := totalNanos / nanosPerSecond + nanos := totalNanos % nanosPerSecond + + days := totalSeconds / secondsPerDay + rem := totalSeconds % secondsPerDay + hours := rem / secondsPerHour + rem %= secondsPerHour + minutes := rem / secondsPerMinute + seconds := rem % secondsPerMinute + + var sb strings.Builder + if negative { + sb.WriteString("-") + } + sb.WriteString("P") + if days > 0 { + fmt.Fprintf(&sb, "%dD", days) + } + if hours > 0 || minutes > 0 || seconds > 0 || nanos > 0 { + sb.WriteString("T") + if hours > 0 { + fmt.Fprintf(&sb, "%dH", hours) + } + if minutes > 0 { + fmt.Fprintf(&sb, "%dM", minutes) + } + if seconds > 0 || nanos > 0 { + if nanos > 0 { + frac := strings.TrimRight(fmt.Sprintf("%09d", nanos), "0") + fmt.Fprintf(&sb, "%d.%sS", seconds, frac) + } else { + fmt.Fprintf(&sb, "%dS", seconds) + } + } + } + return sb.String() +} + // This converter translates between JSON represntations and literals/variable maps/task specs. Here is an internal link // to documentation surrounding the RSJF spec we are targeting: // https://www.notion.so/Translating-Flyte-Literals-to-JSON-Schema-2098cc06513d80638f04e0a5920e6c4d?source=copy_link @@ -1062,7 +1211,7 @@ func literalToJsonValues(ctx context.Context, literal *core.Literal) (any, error return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("datetime cannot be nil")) case *core.Primitive_Duration: if p.Duration != nil { - return p.Duration.AsDuration().String(), nil + return formatISO8601Duration(p.Duration.AsDuration()), nil } logger.Errorf(ctx, "duration cannot be nil") return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("duration cannot be nil")) @@ -1568,8 +1717,17 @@ func jsonValueToLiteralWithFieldName(ctx context.Context, value any, literalType case core.SimpleType_DURATION: var dtn *durationpb.Duration if str, ok := value.(string); ok { - // Check if the string contains only digits (parse as seconds) - if digitsOnlyRegex.MatchString(str) { + switch { + case isISO8601Duration(str): + // Preferred wire format: ISO-8601 (e.g. "P2DT3H", "PT30M"). + parsed, err := parseISO8601Duration(str) + if err != nil { + logger.Errorf(ctx, "error parsing ISO-8601 duration when converting json to literal. String: [%s] Err: %v", str, err) + return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("error parsing ISO-8601 duration when converting json to literal")) + } + dtn = parsed + case digitsOnlyRegex.MatchString(str): + // Backward compatibility: a bare number is treated as seconds. seconds, err := strconv.ParseInt(str, 10, 64) if err != nil { logger.Errorf(ctx, "error parsing duration seconds when converting json to literal. String: [%s] Err: %v", str, err) @@ -1579,8 +1737,8 @@ func jsonValueToLiteralWithFieldName(ctx context.Context, value any, literalType dtn = &durationpb.Duration{ Seconds: seconds, } - } else { - // Parse the duration string into a time.Duration + default: + // Backward compatibility: Go-style duration strings (e.g. "2h30m"). duration, err := time.ParseDuration(str) if err != nil { logger.Errorf(ctx, "error parsing duration when converting json to literal. String duration: [%s] Err: %v", str, err) diff --git a/runs/service/converter/literal_json_converter_test.go b/runs/service/converter/literal_json_converter_test.go index 49de671bba..d653d82272 100644 --- a/runs/service/converter/literal_json_converter_test.go +++ b/runs/service/converter/literal_json_converter_test.go @@ -345,7 +345,7 @@ func TestLiteralsToJsonSchema(t *testing.T) { testDurationField := properties["test_duration"].(map[string]any) assert.Equal(t, "string", testDurationField["type"]) assert.Equal(t, "duration", testDurationField["format"]) - assert.Equal(t, duration.String(), testDurationField["default"]) + assert.Equal(t, "PT1H30M", testDurationField["default"]) assert.Equal(t, "A test duration", testDurationField["description"]) jsonToLiteralsResult, err := LaunchFormJsonToLiterals(context.Background(), literalsToJsonResult) @@ -595,3 +595,106 @@ func createSimpleDataclassLiteralSingleType() ([]*task.NamedLiteral, *core.Varia return literals, variableMap } + +func TestFormatISO8601Duration(t *testing.T) { + cases := []struct { + name string + duration time.Duration + expected string + }{ + {"zero", 0, "P0D"}, + {"seconds", 5 * time.Second, "PT5S"}, + {"minutes and seconds", 90 * time.Second, "PT1M30S"}, + {"hours and minutes", time.Hour + 30*time.Minute, "PT1H30M"}, + {"days", 48 * time.Hour, "P2D"}, + {"days hours minutes seconds", 49*time.Hour + 4*time.Minute + 5*time.Second, "P2DT1H4M5S"}, + {"fractional seconds", 1500 * time.Millisecond, "PT1.5S"}, + {"negative", -(time.Hour + 30*time.Minute), "-PT1H30M"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.expected, formatISO8601Duration(tc.duration)) + }) + } +} + +func TestParseISO8601Duration(t *testing.T) { + t.Run("valid", func(t *testing.T) { + cases := []struct { + input string + expected time.Duration + }{ + {"P0D", 0}, + {"PT5S", 5 * time.Second}, + {"PT1M30S", 90 * time.Second}, + {"PT1H30M", time.Hour + 30*time.Minute}, + {"P2D", 48 * time.Hour}, + {"P2DT1H4M5S", 49*time.Hour + 4*time.Minute + 5*time.Second}, + {"P1W", 7 * 24 * time.Hour}, + {"PT1.5S", 1500 * time.Millisecond}, + {"-PT1H30M", -(time.Hour + 30*time.Minute)}, + {"P1M", 30 * 24 * time.Hour}, + {"P1Y", 365 * 24 * time.Hour}, + } + for _, tc := range cases { + t.Run(tc.input, func(t *testing.T) { + parsed, err := parseISO8601Duration(tc.input) + require.NoError(t, err) + assert.Equal(t, tc.expected, parsed.AsDuration()) + }) + } + }) + + t.Run("invalid", func(t *testing.T) { + for _, input := range []string{"P", "PT", "", "2h30m", "not-a-duration", "P1H"} { + _, err := parseISO8601Duration(input) + assert.Error(t, err, "expected error for %q", input) + } + }) + + t.Run("round trips with formatISO8601Duration", func(t *testing.T) { + for _, d := range []time.Duration{0, 5 * time.Second, time.Hour + 30*time.Minute, 49*time.Hour + 4*time.Minute + 5*time.Second, 1500 * time.Millisecond} { + parsed, err := parseISO8601Duration(formatISO8601Duration(d)) + require.NoError(t, err) + assert.Equal(t, d, parsed.AsDuration()) + } + }) +} + +func TestJSONValuesToLiteralsDuration(t *testing.T) { + variableMap := makeVariableMap(map[string]*core.Variable{ + "duration": { + Type: &core.LiteralType{Type: &core.LiteralType_Simple{Simple: core.SimpleType_DURATION}}, + }, + }) + + cases := []struct { + name string + value string + expected time.Duration + }{ + {"iso-8601", "P2DT3H", 51 * time.Hour}, + {"iso-8601 minutes", "PT30M", 30 * time.Minute}, + {"bare seconds (backward compat)", "3600", time.Hour}, + {"go-style readable (backward compat)", "2h30m", 2*time.Hour + 30*time.Minute}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + values, err := structpb.NewStruct(map[string]any{"duration": tc.value}) + require.NoError(t, err) + + literals, err := JSONValuesToLiterals(context.Background(), variableMap, values) + require.NoError(t, err) + require.Len(t, literals, 1) + assert.Equal(t, tc.expected, literals[0].GetValue().GetScalar().GetPrimitive().GetDuration().AsDuration()) + }) + } + + t.Run("invalid iso-8601 errors", func(t *testing.T) { + values, err := structpb.NewStruct(map[string]any{"duration": "P1H"}) + require.NoError(t, err) + + _, err = JSONValuesToLiterals(context.Background(), variableMap, values) + assert.Error(t, err) + }) +}