Create MapSet[T] and ImmutableMapSet[T] types and change Entity.Parents to be a ImmutableMapSet[EntityUID] #40

patjakdev · 2024-09-20T20:29:14Z

Issue #, if available: None

Description of changes:

This PR adds a generic MapSet[T] and ImmutableMapSet[T] for all the places we want the semantics of a set but are annoyed by the inconvenience of using the map[T]struct{} idiom in Go.

In particular, the parents of an Entity have set semantics. With the current slice-based implementation, time complexity of the evaluation of in is linear. Utilizing a ImmutableMapSet[EntityUID] instead gives us constant time complexity on average, which is better. That being said, for small sets, that constant time is actually slightly greater than the time it takes to perform a linear search. I will consider addressing that in a future PR, but for now I'm happy to just have set semantics for Entity parents.

The ImmutableMapSet[EntityUID] is projected publicly through the types package as EntityUIDSet, with appropriate constructors also projected. For now, at least, I thought it prudent not to allow external users to import and use MapSet[T] directly.

I've also replaced any existing internal usages of map[T]struct{} in the module with MapSet[T] or ImmutableMapSet[T] as appropriate.

Important notes:

MapSet[T] has no ordering guarantees. This applies to MarshalJSON as well, meaning there's no predictable JSON encoding for a given Set. Instead of using comparable, we could invent an Item interface that embeds comparable and also has a Less() method, but then this set couldn't easily be used for native types like string, which feels like a pretty lame drawback.
While MapSet is implemented in internal and therefore can't be directly imported by external users, the Entity.Parents is now of type MapSet[EntityUID] and therefore changes to the MapSet[T] interface should be considered breaking changes. I'm open to feedback on a better way to structure this. I considered putting MapSet in types, but thought that might be confusing...especially because I have some desire to extract an ImmutableHashSet out of types.Set in the future.

patjakdev · 2024-09-20T20:33:56Z

types/entities.go

@@ -13,13 +13,6 @@ import (
 // the Entity (it must be the same as the UID within the Entity itself.)
 type Entities map[EntityUID]*Entity

-// An Entity defines the parents and attributes for an EntityUID.


I extracted this because I always have trouble remembering where the heck this struct is defined.

patjakdev · 2024-09-20T21:04:38Z

authorize_test.go

@@ -1,68 +1,68 @@
-package cedar
+package cedar_test


This is good hygiene. None of the tests in this file need to interact with any private bits in the package.

patjakdev · 2024-09-20T21:05:23Z

authorize_test.go

 					UID:     cuzco,
-					Parents: []EntityUID{types.NewEntityUID("parent", "bob")},
+					Parents: cedar.NewEntityUIDSetFromSlice([]cedar.EntityUID{cedar.NewEntityUID("parent", "bob")}),


Here's an example of the breaking change.

I'll admit that it's a bit of a mouthful...

Maybe receive a variadic, to cut out the []cedar.EntityUID{ ? Though TBH, may not matter too much since it's going to be usually handled via JSON marshalling.

Maybe the words FromSlice aren't really needed.

patjakdev · 2024-09-20T21:05:32Z

authorize_test.go

 					UID:     cuzco,
-					Parents: []EntityUID{types.NewEntityUID("team", "osiris")},
+					Parents: cedar.NewEntityUIDSetFromSlice([]cedar.EntityUID{cedar.NewEntityUID("team", "osiris")}),


Another example.

patjakdev · 2024-09-20T21:05:58Z

authorize_test.go

 					UID:     dropTable,
-					Parents: []EntityUID{types.NewEntityUID("scary", "stuff")},
+					Parents: cedar.NewEntityUIDSetFromSlice([]cedar.EntityUID{cedar.NewEntityUID("scary", "stuff")}),


A couple more examples here and on line 212.

patjakdev · 2024-09-20T21:06:23Z

authorize_test.go

+			Entities: cedar.Entities{
+				cedar.NewEntityUID("Resource", "table"): &cedar.Entity{
+					UID:     cedar.NewEntityUID("Resource", "table"),
+					Parents: cedar.NewEntityUIDSetFromSlice([]cedar.EntityUID{cedar.NewEntityUID("Parent", "id")}),


The last example in this file

…nvenience functions around a map[T]struct{} Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

…g]struct{} for annotation names Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

…} for record keys Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

…} for detecting unbound or unused variables Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

internal/eval/evalers.go

internal/eval/evalers_test.go

internal/eval/partial.go

internal/sets/mapset.go

philhassey

A lot of little nitpicks here, glad to discuss. Great improvement to a lot of stuff!

…ropriately, and return pointers to underscore risk of copy by value Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

…oolean Intersects Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

philhassey · 2024-09-26T16:55:37Z

Set has no ordering. This applies to MarshalJSON as well, meaning there's no predictable JSON encoding for a given Set. Instead of using comparable, we could invent an Item interface that embeds comparable and also has a Less() method, but then this set couldn't easily be used for native types like string, which feels like a pretty lame drawback.

I think having a consistent JSON marshaller is important because having things change randomly is really annoying and can cause unnecessary updates happen when a diff is detected by something somewhere.

In the case of JSON marshaling, I don't think performance is the key metric. So I think what you could do is get a Marshal of the bytes of all the values. Then sort those slices of bytes, then emit the "[a,b,c]" in that order.

While MapSet is implemented in internal and therefore can't be directly imported by external users, the Entity.Parents is now of type MapSet[EntityUID] and therefore changes to the MapSet[T] interface should be considered breaking changes. I'm open to feedback on a better way to structure this. I considered putting MapSet in types, but thought that might be confusing...especially because I have some desire to extract an ImmutableHashSet out of types.Set in the future.

I thought the Entity.Parents was going to be immutable?

I agree that having type X internal.Something isn't great, but I think we can defer fixing that for a while. We have similar problems in our public ast package.

internal/eval/evalers.go

philhassey · 2024-09-26T17:00:54Z

internal/eval/evalers.go

 			}
-			for _, k := range fe.Parents {
+			fe.Parents.Iterate(func(k types.EntityUID) bool {


This is fine, but as a point of interest, I wonder if using a closure causes an allocation.

internal/mapset/immutable_test.go

internal/mapset/mapset.go

internal/mapset/mapset_test.go

policy.go

types/entity.go

types/set.go

philhassey

Looks good, a few minor tweaks would be nice.

Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

patjakdev marked this pull request as ready for review September 20, 2024 20:32

patjakdev requested review from philhassey and apg September 20, 2024 20:32

patjakdev force-pushed the entity-parents-as-set branch from 3740e46 to 88889b6 Compare September 20, 2024 20:33

patjakdev commented Sep 20, 2024

View reviewed changes

patjakdev marked this pull request as draft September 20, 2024 20:34

patjakdev force-pushed the entity-parents-as-set branch from 88889b6 to 68fb91c Compare September 20, 2024 21:02

patjakdev commented Sep 20, 2024

View reviewed changes

patjakdev marked this pull request as ready for review September 20, 2024 21:13

patjakdev force-pushed the entity-parents-as-set branch 2 times, most recently from 88d6d96 to 4cf812d Compare September 23, 2024 17:43

patjakdev added 5 commits September 23, 2024 14:54

cedar-go/internal: add a MapSet type, which is basically just some co…

fd1c22a

…nvenience functions around a map[T]struct{} Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

cedar-go/types: convert Entity.Parents to a MapSet[EntityUID]

4fc3a74

Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

cedar-go/internal/parser: use a MapSet[string] instead of a map[strin…

c2ef430

…g]struct{} for annotation names Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

internal/parser: use a MapSet[string] instead of a map[string]struct{…

58729be

…} for record keys Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

x/exp/batch: use a MapSet[types.String] instead of map[types.String]{…

32380ba

…} for detecting unbound or unused variables Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

patjakdev force-pushed the entity-parents-as-set branch from 4cf812d to 32380ba Compare September 23, 2024 21:54

philhassey reviewed Sep 24, 2024

View reviewed changes

internal/eval/evalers.go Show resolved Hide resolved

philhassey reviewed Sep 24, 2024

View reviewed changes

internal/eval/evalers_test.go Outdated Show resolved Hide resolved

philhassey reviewed Sep 24, 2024

View reviewed changes

internal/eval/partial.go Outdated Show resolved Hide resolved

philhassey reviewed Sep 24, 2024

View reviewed changes

internal/sets/mapset.go Outdated Show resolved Hide resolved

philhassey reviewed Sep 24, 2024

View reviewed changes

internal/sets/mapset.go Outdated Show resolved Hide resolved

philhassey reviewed Sep 24, 2024

View reviewed changes

internal/sets/mapset.go Outdated Show resolved Hide resolved

philhassey reviewed Sep 24, 2024

View reviewed changes

internal/sets/mapset.go Outdated Show resolved Hide resolved

philhassey reviewed Sep 24, 2024

View reviewed changes

internal/sets/mapset.go Outdated Show resolved Hide resolved

philhassey reviewed Sep 24, 2024

View reviewed changes

internal/sets/mapset.go Outdated Show resolved Hide resolved

philhassey approved these changes Sep 24, 2024

View reviewed changes

patjakdev added 9 commits September 24, 2024 15:57

internal/mapset: move mapset to its own package, rename functions app…

2e445e2

…ropriately, and return pointers to underscore risk of copy by value Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

internal/mapset: create new immutable mapset type

d479334

Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

types: make EntityUIDSet an ImmutableHashSet

a81d260

Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

internal/mapset: rename mapset.New to mapset.Make

68386de

Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

internal/mapset: remove some unused methods on MapSet

7b923a3

Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

internal/mapset: rename FromSlice to FromItems and use variadic args

6b1e959

Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

internal/mapset: reduce the functionality of Intersection to just a b…

efe617a

…oolean Intersects Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

internal/mapset: add some documentation around the zero value of MapSet

05c78eb

Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

types: fix some tiny typos

c0c3981

Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>

patjakdev force-pushed the entity-parents-as-set branch from 3db1c9a to c0c3981 Compare September 24, 2024 22:58

patjakdev changed the title ~~Create a MapSet[T] type and change Entity.Parents to be a MapSet[EntityUID]~~ Create MapSet[T] and ImmutableMapSet[T] types and change Entity.Parents to be a ImmutableMapSet[EntityUID] Sep 25, 2024