Add the extension types, datetime and duration by apg · Pull Request #35 · cedar-policy/cedar-go

apg · 2024-09-16T18:04:58Z

See merged RFC 80.

This PR adds:

The datetime and duration type specified in RFC 80
Associated methods for those types
An implementation of operator overloading for ComparableValues, such as Long, Datetime, Duration.

This does not extend ComparableValue to Decimal, as there's no RFC / specification to make that so (doing so would be straightforward, however).

Both datetime and duration are backed by an int64 and can be lifted from a Go time.Time and a time.Duration respectively. It would be possible to remove the struct wrapper around the int64, as no other state is stored. We aren't doing this to prevent accidental casting / type assertions that remove the semantic meaning of these values.

(Note: there was some previous discussion in the strongdm fork of this repo: strongdm#3)

philhassey

Hope those help!

internal/eval/evalers.go

philhassey · 2024-09-16T20:44:51Z

internal/eval/evalers.go

+	if err != nil {
+		return nil, err
+	}
+	l, ok := v.(types.ComparableValue)


Can you make ComparableValue be an interface that only exists within the evalers package? I don't think the general public needs to be aware of it.

philhassey · 2024-09-16T20:46:00Z

internal/eval/evalers.go

+	if err != nil {
+		return zeroValue(), err
+	}
+	return types.Long(lhs.Value / 1000 / 60 / 60), nil


Can all theses things end up as constants? lhs.Value / Hour or something

philhassey · 2024-09-16T20:47:25Z

types/datetime.go

+		year, month, day, hour, minute, second, milli int
+		i                                             int
+	)
+


Max nitpick - maybe less linebreaks would feel a bit nicer.

philhassey · 2024-09-16T20:49:49Z

types/datetime.go

+
+	minute = 10*int(rune(s[i])-'0') + int(rune(s[i+1])-'0')
+
+	i = i + 2


Maybe it would be nice if there was a few private constants
`
const dateOffset = 3
const hourOffset = 6

etc. The changing of i, then using offsets from i is a bit hard to read.

philhassey · 2024-09-16T20:51:53Z

types/datetime.go

+	return ok && a == b
+}
+
+func (a Datetime) Less(bi Value) bool {


Less and LessEqual should return errors in case the types are not the same.

Also, I'd call them LessThan and LessThanOrEqual to better match the cedar operators list:
https://docs.cedarpolicy.com/policies/syntax-operators.html

Alternatively it could just take in the exact type only Less(bi Datetime)

Then you'd just do all the type checking work within the evalers package.

Could go either way on this.

I think it's less desirable for the method to take a concrete type given there's an interface here. I'll do a type assertion in the implementation.

philhassey · 2024-09-16T20:53:12Z

types/datetime.go

+		// The following are not supported:
+		// "datetime(\"1970-01-01T00:00:00Z\")"
+		// {"fn":"datetime","arg":"1970-01-01T00:00:00Z"}
+		var res extValueJSON


As long as this is in parity with what we did for Decimal/IPAddr it's okay for now. But maybe add an issue somewhere about the deficit.

I'll just add it for duration/datetime, as they aren't hard to add.

philhassey · 2024-09-16T20:53:44Z

types/duration.go

+	"unicode"
+)
+
+const (


Ah, since I mentioned those constants elsewhere, maybe they can all live in the internal/consts package?

philhassey · 2024-09-16T20:54:54Z

types/duration.go

+}
+
+// UnsafeDuration creates a duration via unsafe conversion from int, int64, float64
+func UnsafeDuration[T int | int64 | float64](v T) Duration {


Maybe it should multiply by 1000 or something? And the comment should indicate what it takes in, e.g. seconds?

I'd probably just drop this function and only include FromStdDuration.

apg · 2024-09-17T22:55:20Z

@philhassey I think all of your comments have been addressed.

philhassey · 2024-09-18T16:43:46Z

internal/eval/comparable.go

+type ComparableValue interface {
+	types.Value
+	LessThan(types.Value) (bool, error)
+	LessThanEqual(types.Value) (bool, error)


Can we change LessThanEqual to LessThanOrEqual, that better matches the cedar docs:

https://docs.cedarpolicy.com/policies/syntax-operators.html#function-lessThanOrEqual

philhassey · 2024-09-18T16:47:16Z

types/duration.go

+	return Duration{Value: negative * total}, nil
+}
+
+func (a Duration) Equal(bi Value) bool {


All method and functions need a go-doc style docstring.

philhassey · 2024-09-18T16:47:39Z

types/datetime.go

+	Value int64
+}
+
+func UnsafeDatetime[T int | int64 | float64](v T) Datetime {


I still think this function isn't useful.

ha! I think I stopped using it and didn't delete it. I'll get rid of it.

Apparently I didn't stop using it. 🤔 I will stop using it.

philhassey

Hope that helps!

types/value.go

philhassey · 2024-09-18T16:55:40Z

I think this will be the next release, so maybe add in something to the README.md explaining the latest goodies in 0.3.2.

And also add some explanation as to how datetime is an accepted RFC, but it's a bit bleeding edge (as discussed in the meeting yesterday)

This PR does not add the duration type, nor the methods `toTime()`, `offset()`, or `durationSince()` that would require it. This PR adds operator overloading, by introducing a set of "Virtual" comparison Evaler. Types that can overload operators must implement the `Lesser` interface. Long and Datetime are implemented in this PR. (Note: It is expected that there will be a Decimal operator overload RFC at some later date, but that is not hooked into this) The now obsolete Evalers for Long comparisons have not been removed. A possible improvement to this PR would be to utilize them when it is known that Longs are on the right and left side. The datetime itself is backed by an int64, and can be lifted from a Go `time.Time`, integer or float "unsafely." Signed-off-by: Andrew Gwozdziewycz <andrew.gwozdziewycz@strongdm.com>

This builds on the Datetime PR. It adds the duration type, and all of the associated methods that require durations: - datetime.toTime - datetime.offset - datetime.durationSince - duration.toMilliseconds - duration.toSeconds - duration.toMinutes - duration.toHours - duration.toDays Signed-off-by: Andrew Gwozdziewycz <andrew.gwozdziewycz@strongdm.com>

philhassey · 2024-09-18T20:29:34Z

types/datetime.go

+	return Datetime{Value: t.UnixMilli()}, nil
+}
+
+// Equal returns true if the lfs represents the same timestamp as the rhs.


I think "lhs" instead of "lfs"

I feel like "lhs" and "rhs" is a bit odd. Maybe have it phrased "Equal returns true if the input represents the same timestamp."

philhassey

Looks good, just a tiny text edit. Can @patjakdev give this a quick review as well?

apg · 2024-09-18T22:03:33Z

OK. At 100% coverage for all changes. There's one line that's not covered in cedar_unmarshal that is unrelated to my changes. I tried to figure out a quick way to reach it, but failed. :)

patjakdev

Looks really good to me. Just a couple of nits.

patjakdev · 2024-09-18T22:05:49Z

internal/eval/evalers.go

+	if err != nil {
+		return zeroValue(), err
+	}
+	ok, err := lhs.LessThanOrEqual(rhs)


tiny nit: the ok here maybe should be isLessThanOrEqual? ok implies something else to me.

patjakdev · 2024-09-18T22:06:38Z

internal/consts/consts.go

+	MillisPerDay    = int64(1000 * 60 * 60 * 24)
+	MillisPerHour   = int64(1000 * 60 * 60)
+	MillisPerMinute = int64(1000 * 60)
+	MillisPerSecond = int64(1000)


tiny nit: seems like these could be composed from each other

patjakdev · 2024-09-18T22:10:46Z

types/duration.go

+
+// A Duration is a value representing a span of time in milliseconds.
+type Duration struct {
+	Value int64


I think we should strongly consider making Value private. The accessor methods with units seem like the right public interface for this.

Changed. Note that Decimal still exposes this.

patjakdev · 2024-09-18T22:11:47Z

types/datetime.go

+// Datetime represents a Cedar datetime value
+type Datetime struct {
+	// Value is a timestamp in milliseconds
+	Value int64


I think we should strongly consider keeping Value private and making an accessor method that gives you the time since the epoch in milliseconds.

Changed. Note that Decimal still exposes this.

I think Decimal will take a bit more effort, I've made an issue in the IDX project for us to handle that in a separate PR.

- Lesser becomes ComparableValue - Move ComparableValue to evalers only - Move all magic values to constants - TypeError when incompatible comparable types - Support more deserialization for duration/datetime - Make the datetime parser easier to follow - Drop UnsafeDatetime in favor of FromStdTime(time.UnixMilli(..)) - Document methods - Test Coverage to 100% Signed-off-by: Andrew Gwozdziewycz <andrew.gwozdziewycz@strongdm.com>

Signed-off-by: Andrew Gwozdziewycz <andrew.gwozdziewycz@strongdm.com>

apg force-pushed the apg/po-515-add-datetime branch from d92facc to 14e55f5 Compare September 16, 2024 18:07

apg marked this pull request as ready for review September 16, 2024 18:07

philhassey reviewed Sep 16, 2024

View reviewed changes

kjamieson-sdm changed the title ~~Add the extention types, datetime and duration~~ Add the extension types, datetime and duration Sep 17, 2024

apg force-pushed the apg/po-515-add-datetime branch 2 times, most recently from 948a208 to cf55f89 Compare September 17, 2024 22:52

philhassey reviewed Sep 18, 2024

View reviewed changes

types/value.go Show resolved Hide resolved

apg added 2 commits September 18, 2024 11:00

apg force-pushed the apg/po-515-add-datetime branch 2 times, most recently from 4adb50c to 47eb643 Compare September 18, 2024 18:03

philhassey reviewed Sep 18, 2024

View reviewed changes

philhassey approved these changes Sep 18, 2024

View reviewed changes

apg force-pushed the apg/po-515-add-datetime branch 2 times, most recently from 30daa0a to 78770a1 Compare September 18, 2024 22:02

patjakdev approved these changes Sep 18, 2024

View reviewed changes

apg added 2 commits September 18, 2024 16:18

Update the README indicating datetime / duration support

125cddb

Signed-off-by: Andrew Gwozdziewycz <andrew.gwozdziewycz@strongdm.com>

apg force-pushed the apg/po-515-add-datetime branch from 78770a1 to 125cddb Compare September 18, 2024 23:19

apg merged commit d1f59f4 into cedar-policy:main Sep 19, 2024

apg deleted the apg/po-515-add-datetime branch September 19, 2024 22:04


		minute = 10*int(rune(s[i])-'0') + int(rune(s[i+1])-'0')

		i = i + 2

Conversation

apg commented Sep 16, 2024

Uh oh!

philhassey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apg commented Sep 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philhassey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

philhassey commented Sep 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philhassey left a comment

Choose a reason for hiding this comment

Uh oh!

apg commented Sep 18, 2024

Uh oh!

patjakdev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!