Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix build by reinstating Guava dependency #83

Merged
merged 3 commits into from
Feb 15, 2024

Conversation

john-h-kastner-aws
Copy link
Contributor

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.

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, 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.
@exceptionfactory
Copy link
Contributor

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.

Copy link
Contributor

@cdisselkoen cdisselkoen left a comment

Choose a reason for hiding this comment

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

Merging this now to unbreak CI is expedient, and then we can continue the discussion of the Guava dependency

@john-h-kastner-aws john-h-kastner-aws merged commit 5bc5393 into main Feb 15, 2024
1 check passed
@john-h-kastner-aws john-h-kastner-aws deleted the jkastner/fix-build branch February 20, 2024 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants