Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
168 changes: 163 additions & 5 deletions runs/service/converter/literal_json_converter.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"encoding/base64"
"fmt"
"math"
"reflect"
"regexp"
"strconv"
Expand All @@ -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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we usea library instead?

channelmeter/iso8601duration

Maybe...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From me looking:

Short answer: we could, but channelmeter/iso8601duration is a poor fit and would actually be a regression. It's not currently a dependency either, so we'd be adding a new (unmaintained) module.

The two dealbreakers:

  • It doesn't cover the formatting direction. String() only formats the library's own Duration struct (Years/Weeks/Days/Hours/Minutes/Seconds), not a Go time.Duration. Our literalToJsonValues path needs time.Duration → ISO string, so we'd still hand-roll the decomposition — the library buys us nothing there. By its own docstring it's a "partial implementation of ISO8601 durations (no months)."
  • It drops cases we explicitly support. It rejects months, can't represent fractional seconds, and has no negatives. Our converter handles all three. Swapping it in would silently narrow behavior and break frontend round-tripping (dayjs can emit P1M).

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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use a library for this

https://github.com/dustin/go-humanize

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
Expand Down Expand Up @@ -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"))
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down
105 changes: 104 additions & 1 deletion runs/service/converter/literal_json_converter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
})
}
Loading