Skip to content

Remove guava threadsafe #330

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Remove guava threadsafe #330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf77e13
Changes, but using ConcurrentHashMap for thread safety
hildo Feb 8, 2025
6dc221f
Fix up after rebase
hildo Feb 21, 2025
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
1 change: 0 additions & 1 deletion CedarJava/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,6 @@ dependencies {
implementation 'com.fasterxml.jackson.core:jackson-databind:2.18.2'
implementation 'com.fasterxml.jackson.datatype:jackson-datatype-jdk8:2.18.2'
implementation 'com.fizzed:jne:4.3.0'
implementation 'com.google.guava:guava:33.4.0-jre'
compileOnly 'com.github.spotbugs:spotbugs-annotations:4.8.6'
compileOnly 'org.projectlombok:lombok:1.18.30'
annotationProcessor 'org.projectlombok:lombok:1.18.30'
Expand Down
12 changes: 6 additions & 6 deletions CedarJava/src/main/java/com/cedarpolicy/model/AuthorizationResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.collect.ImmutableList;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;

/**
Expand All @@ -34,10 +34,10 @@ public final class AuthorizationResponse {
public final Optional<AuthorizationSuccessResponse> success;
/** This will be present if and only if `type` is `Failure`. */
@JsonProperty("errors")
public final Optional<ImmutableList<DetailedError>> errors;
public final Optional<List<DetailedError>> errors;
/** Warnings can be produced regardless of whether we have a `Success` or `Failure`. */
@JsonProperty("warnings")
public final ImmutableList<String> warnings;
public final List<String> warnings;

/**
* If `type` is `Success`, `success` should be present and `errors` empty.
Expand All @@ -52,11 +52,11 @@ public AuthorizationResponse(
) {
this.type = type;
this.success = success;
this.errors = errors.map((list) -> ImmutableList.copyOf(list));
this.errors = errors.map((list) -> List.copyOf(list));
if (warnings == null) {
this.warnings = ImmutableList.of(); // empty
this.warnings = List.of(); // empty
} else {
this.warnings = ImmutableList.copyOf(warnings);
this.warnings = List.copyOf(warnings);
}
}

Expand Down
10 changes: 4 additions & 6 deletions CedarJava/src/main/java/com/cedarpolicy/model/AuthorizationSuccessResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import java.util.List;
import java.util.Set;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;

/**
* Successful authorization response
Expand All @@ -44,10 +42,10 @@ public static class Diagnostics {
* Set of policyID's that caused the decision. For example, when a policy evaluates to Deny,
* all forbid policies that evaluated to True will appear in `reason`.
*/
private ImmutableSet<String> reason;
private Set<String> reason;

/** Set of errors and warnings returned by Cedar. */
private ImmutableList<AuthorizationError> errors;
private List<AuthorizationError> errors;

/**
* Read the reasons and errors from a JSON object.
Expand All @@ -59,8 +57,8 @@ public static class Diagnostics {
public Diagnostics(
@JsonProperty("reason") Set<String> reason,
@JsonProperty("errors") List<AuthorizationError> errors) {
this.errors = ImmutableList.copyOf(errors);
this.reason = ImmutableSet.copyOf(reason);
this.errors = List.copyOf(errors);
this.reason = Set.copyOf(reason);
}

/**
Expand Down
13 changes: 6 additions & 7 deletions CedarJava/src/main/java/com/cedarpolicy/model/DetailedError.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.collect.ImmutableList;
import java.util.List;
import java.util.Optional;

Expand All @@ -40,10 +39,10 @@ public class DetailedError {
public final Optional<Severity> severity;
/** Source labels (ranges) */
@JsonProperty("sourceLocations")
public final ImmutableList<SourceLabel> sourceLocations;
public final List<SourceLabel> sourceLocations;
/** Related errors */
@JsonProperty("related")
public final ImmutableList<DetailedError> related;
public final List<DetailedError> related;

@JsonCreator
public DetailedError(
Expand All @@ -61,14 +60,14 @@ public DetailedError(
this.url = url;
this.severity = severity;
if (sourceLocations.isPresent()) {
this.sourceLocations = ImmutableList.copyOf(sourceLocations.get());
this.sourceLocations = List.copyOf(sourceLocations.get());
} else {
this.sourceLocations = ImmutableList.of(); // empty
this.sourceLocations = List.of(); // empty
}
if (related.isPresent()) {
this.related = ImmutableList.copyOf(related.get());
this.related = List.copyOf(related.get());
} else {
this.related = ImmutableList.of(); // empty
this.related = List.of(); // empty
}
}

Expand Down
5 changes: 2 additions & 3 deletions CedarJava/src/main/java/com/cedarpolicy/model/PartialAuthorizationRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import com.cedarpolicy.value.EntityUID;
import com.cedarpolicy.value.Value;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.google.common.collect.ImmutableMap;

import java.util.Map;
import java.util.HashMap;
Expand Down Expand Up @@ -156,12 +155,12 @@ public Builder resource(EntityUID resourceEUID) {
* @return The builder.
*/
public Builder context(Map<String, Value> context) {
this.context = Optional.of(ImmutableMap.copyOf(context));
this.context = Optional.of(Map.copyOf(context));
return this;
}

public Builder context(Context context) {
this.context = Optional.of(ImmutableMap.copyOf(context.getContext()));
this.context = Optional.of(Map.copyOf(context.getContext()));
return this;
}

Expand Down
12 changes: 6 additions & 6 deletions CedarJava/src/main/java/com/cedarpolicy/model/PartialAuthorizationResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@
import com.cedarpolicy.ExperimentalFeature;
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.collect.ImmutableList;

import java.util.ArrayList;
import java.util.List;
import java.util.Optional;

@Experimental(ExperimentalFeature.PARTIAL_EVALUATION)
Expand All @@ -25,12 +25,12 @@ public class PartialAuthorizationResponse {
* This will be present if and only if `type` is `Failure`.
*/
@JsonProperty("errors")
public final Optional<ImmutableList<DetailedError>> errors;
public final Optional<List<DetailedError>> errors;
/**
* Warnings can be produced regardless of whether we have a `Success` or `Failure`.
*/
@JsonProperty("warnings")
public final ImmutableList<String> warnings;
public final List<String> warnings;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this count as a breaking change (hence requiring a major version bump) because we are updating type for a public field?

Copy link
Contributor

Choose a reason for hiding this comment

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

In Rust SDK, PE is an experimental feature and hence does not obey SemVer. I'm not sure if CedarJava follows a similar rule given that there's no easy to specify an experimental feature. Just FYI.

Copy link
Author

Choose a reason for hiding this comment

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

@muditchaudhary that is a good pickup. It would be a breaking change, especially if the application was using cedar-java's transitive dependencies to include guava (ImmutableList would not be available anymore). If the code stored this in a variable of type java.util.List, it wouldn't. But that's not an assumption that can be made.

I am not familiar with how the version numbers are managed. If we are to change them, is the change included in this PR? Or is the library versioned using another process? Happy to help where I am able to.

Copy link
Author

Choose a reason for hiding this comment

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

I am assuming that if I am to make the version change, I should be setting the version at line 284 in CedarJava/build.gradle to 4.0.0? But I will wait for confirmation if that's the correct process

Copy link
Contributor

@muditchaudhary muditchaudhary Sep 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@hildo If needed, we'll bump the version on release. So, there shouldn't be any action required within this PR. However, I am checking with others on how we'd want to approach it. Ideally, we want our major versions to be in-sync with Cedar, which is at 4.x. If we bump the major version for CedarJava for this change i.e., 5.x we won't be in-sync with Cedar versions anymore (which might cause some confusion). I'll provide an update soon.

We are also making a public field type change for AuthorizationResponse (which is not experimental)

Copy link
Author

Choose a reason for hiding this comment

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

@muditchaudhary all good. Thanks for the update.


/**
* If `type` is `Success`, `success` should be present and `errors` empty.
Expand All @@ -45,11 +45,11 @@ public PartialAuthorizationResponse(
) {
this.type = type;
this.success = success;
this.errors = errors.map((list) -> ImmutableList.copyOf(list));
this.errors = errors.map((list) -> List.copyOf(list));
if (warnings == null) {
this.warnings = ImmutableList.of(); // empty
this.warnings = List.of(); // empty
} else {
this.warnings = ImmutableList.copyOf(warnings);
this.warnings = List.copyOf(warnings);
}
}

Expand Down
34 changes: 15 additions & 19 deletions CedarJava/src/main/java/com/cedarpolicy/model/PartialAuthorizationSuccessResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,40 +24,36 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.databind.JsonNode;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;

/**
* Successful partial authorization response
*/
@Experimental(ExperimentalFeature.PARTIAL_EVALUATION)
public final class PartialAuthorizationSuccessResponse {
private final AuthorizationSuccessResponse.Decision decision;
private final ImmutableSet<String> satisfied;
private final ImmutableSet<String> errored;
private final ImmutableSet<String> mayBeDetermining;
private final ImmutableSet<String> mustBeDetermining;
private final ImmutableMap<String, JsonNode> residuals;
private final ImmutableSet<String> nontrivialResiduals;
private final ImmutableSet<String> warnings;
private final Set<String> satisfied;
private final Set<String> errored;
private final Set<String> mayBeDetermining;
private final Set<String> mustBeDetermining;
private final Map<String, JsonNode> residuals;
private final Set<String> nontrivialResiduals;
private final Set<String> warnings;

public PartialAuthorizationSuccessResponse(
AuthorizationSuccessResponse.Decision decision, Set<String> satisfied, Set<String> errored,
Set<String> mayBeDetermining, Set<String> mustBeDetermining, Map<String, JsonNode> residuals,
Set<String> nontrivialResiduals, Set<String> warnings) {
this.decision = decision;
// note that ImmutableSet.copyOf() attempts to avoid a full copy when possible
// see https://github.com/google/guava/wiki/ImmutableCollectionsExplained
this.satisfied = ImmutableSet.copyOf(satisfied);
this.errored = ImmutableSet.copyOf(errored);
this.mayBeDetermining = ImmutableSet.copyOf(mayBeDetermining);
this.mustBeDetermining = ImmutableSet.copyOf(mustBeDetermining);
this.residuals = ImmutableMap.copyOf(residuals);
this.nontrivialResiduals = ImmutableSet.copyOf(nontrivialResiduals);
this.satisfied = Set.copyOf(satisfied);
this.errored = Set.copyOf(errored);
this.mayBeDetermining = Set.copyOf(mayBeDetermining);
this.mustBeDetermining = Set.copyOf(mustBeDetermining);
this.residuals = Map.copyOf(residuals);
this.nontrivialResiduals = Set.copyOf(nontrivialResiduals);
if (warnings == null) {
this.warnings = ImmutableSet.of(); // empty
this.warnings = Set.of(); // empty
} else {
this.warnings = ImmutableSet.copyOf(warnings);
this.warnings = Set.copyOf(warnings);
}
}

Expand Down
25 changes: 11 additions & 14 deletions CedarJava/src/main/java/com/cedarpolicy/model/ValidationResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import com.fasterxml.jackson.annotation.JsonAlias;
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.collect.ImmutableList;

import java.util.List;
import java.util.Objects;
Expand All @@ -39,38 +38,36 @@ public final class ValidationResponse {
* reported in `success`), but rather higher-level errors, like
* a failure to parse or to call the validator.
*/
public final Optional<ImmutableList<DetailedError>> errors;
public final Optional<List<DetailedError>> errors;
/**
* Other warnings not associated with particular policies.
* For instance, warnings about your schema itself.
* These warnings can be produced regardless of whether `type` is
* `Success` or `Failure`.
*/
public final ImmutableList<DetailedError> warnings;
public final List<DetailedError> warnings;

public static final class ValidationSuccessResponse {
/** Validation errors associated with particular policies. */
@JsonProperty("validationErrors")
public final ImmutableList<ValidationError> validationErrors;
public final List<ValidationError> validationErrors;
/** Validation warnings associated with particular policies. */
@JsonProperty("validationWarnings")
public final ImmutableList<ValidationError> validationWarnings;
public final List<ValidationError> validationWarnings;

@JsonCreator
public ValidationSuccessResponse(
@JsonProperty("validationErrors") Optional<List<ValidationError>> validationErrors,
@JsonProperty("validationWarnings") Optional<List<ValidationError>> validationWarnings) {
// note that ImmutableSet.copyOf() attempts to avoid a full copy when possible
// see https://github.com/google/guava/wiki/ImmutableCollectionsExplained
if (validationErrors.isPresent()) {
this.validationErrors = ImmutableList.copyOf(validationErrors.get());
this.validationErrors = List.copyOf(validationErrors.get());
} else {
this.validationErrors = ImmutableList.of(); // empty
this.validationErrors = List.of(); // empty
}
if (validationWarnings.isPresent()) {
this.validationWarnings = ImmutableList.copyOf(validationWarnings.get());
this.validationWarnings = List.copyOf(validationWarnings.get());
} else {
this.validationWarnings = ImmutableList.of(); // empty
this.validationWarnings = List.of(); // empty
}
}
}
Expand All @@ -91,16 +88,16 @@ public ValidationResponse(
@JsonProperty("errors") Optional<List<DetailedError>> errors,
@JsonProperty("warnings") @JsonAlias("otherWarnings") Optional<List<DetailedError>> warnings) {
this.type = type;
this.errors = errors.map((list) -> ImmutableList.copyOf(list));
this.errors = errors.map((list) -> List.copyOf(list));
if (type == SuccessOrFailure.Success) {
this.success = Optional.of(new ValidationSuccessResponse(validationErrors, validationWarnings));
} else {
this.success = Optional.empty();
}
if (warnings.isPresent()) {
this.warnings = ImmutableList.copyOf(warnings.get());
this.warnings = List.copyOf(warnings.get());
} else {
this.warnings = ImmutableList.of(); // empty
this.warnings = List.of(); // empty
}
}

Expand Down
3 changes: 1 addition & 2 deletions CedarJava/src/main/java/com/cedarpolicy/model/policy/TemplateLink.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import java.util.Map;
import java.util.stream.Collectors;

import com.google.common.collect.ImmutableList;

/** Template-linked policy. */
public class TemplateLink {
Expand All @@ -42,7 +41,7 @@ public class TemplateLink {
public TemplateLink(String templateId, String resultPolicyId, List<LinkValue> linkValues) {
this.templateId = templateId;
this.resultPolicyId = resultPolicyId;
this.linkValues = ImmutableList.copyOf(linkValues);
this.linkValues = List.copyOf(linkValues);
}

/** Get the template ID. */
Expand Down
12 changes: 10 additions & 2 deletions CedarJava/src/main/java/com/cedarpolicy/value/EntityTypeName.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@
package com.cedarpolicy.value;

import com.cedarpolicy.loader.LibraryLoader;
import com.google.common.base.Suppliers;

import java.util.List;
import java.util.Optional;
import java.util.concurrent.ConcurrentHashMap;
import java.util.Objects;
import java.util.function.Supplier;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -50,7 +50,15 @@ public final class EntityTypeName {
protected EntityTypeName(List<String> namespace, String basename) {
this.namespace = namespace;
this.basename = basename;
this.entityTypeNameRepr = Suppliers.memoize(() -> getEntityTypeNameRepr(this));
this.entityTypeNameRepr = new Supplier<String>() {

private ConcurrentHashMap<String, String> localMap = new ConcurrentHashMap<>();

@Override
public String get() {
return localMap.computeIfAbsent("entityTypeNameRepr", k -> EntityTypeName.getEntityTypeNameRepr(EntityTypeName.this));
}
};
}

/**
Expand Down
13 changes: 11 additions & 2 deletions CedarJava/src/main/java/com/cedarpolicy/value/EntityUID.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@
package com.cedarpolicy.value;

import java.util.Optional;
import java.util.concurrent.ConcurrentHashMap;
import java.util.Objects;
import java.util.function.Supplier;

import com.cedarpolicy.loader.LibraryLoader;
import com.cedarpolicy.serializer.JsonEUID;
import com.google.common.base.Suppliers;

/**
* Represents a Cedar Entity UID. An entity UID contains both the entity type and a unique
Expand All @@ -45,7 +45,16 @@ public final class EntityUID extends Value {
public EntityUID(EntityTypeName type, EntityIdentifier id) {
this.type = type;
this.id = id;
this.euidRepr = Suppliers.memoize(() -> getEUIDRepr(type, id));
this.euidRepr = new Supplier<String>() {

private ConcurrentHashMap<String, String> localMap = new ConcurrentHashMap<>();

@Override
public String get() {
return localMap.computeIfAbsent("euidRepr", k -> EntityUID.getEUIDRepr(type, id));
}

};
}

/**
Expand Down