cedar-policy / cedar-java

Java bindings for the Cedar language
https://www.cedarpolicy.com
Apache License 2.0
42 stars 19 forks source link

Fix build by reinstating Guava dependency #83

Closed john-h-kastner-aws closed 8 months ago

john-h-kastner-aws commented 8 months ago

This fixes an error caused by conflicting merges. A PR added new code which depended on Guava while another deleted the dependency.

The commit adds the dependency rather than updating the code to not require the dependency because my understanding is that Guava immutable sets provide better guarantees than Java unmodifiable collections. Specifically, unmodifiable sets hold a reference to the set used in their construction. If a reference is kept to the set used to construct an unmodifiable set, then any mutation to that set is reflected in the unmodifiable set. The Guava immutable set creates a copy.

In any case where we store the set in a field of an object, we should use guava immutable types. When returning a collection, we can use the builtin unmodifiable methods to avoid copying the data.

exceptionfactory commented 8 months ago

Just to provide some additional perspective, Guava is a 3 MB JAR, which is rather large considering the minimal use within the Cedar Java library.

It is correct that Collections.unmodifiableList() returns a read-only view of the underlying List, so any changes to the input List would be reflected, whereas Guava ImmutableList.copyOf() creates a new copy.

I did not notice this feature when submitting the pull request to remove Guava, so thanks for highlighting the differences. Under the hood, the Guava copyOf() method uses a few different approaches such as object cloning and java.util.Arrays.copyOf(). Perhaps it would be worth evaluating an internal approach in the Cedar library, as opposed to pulling in Guava?

Thanks for the consideration.