Did we end up deciding to use a List instead of a Set for the list of members?

I updated it to List<> instead of Set<> to get the conversation moving. I can revert back to Set<>.

Do we want to allow users to modify the binding in-place?

I'd need to allow mutations outside of builders. That's okay but it does allow for mutation directly of the policy when it wasn't allowed beforehand.

This could use ImmutableList.Builder if we want to keep it immutable

is there a reason to use String instead of Role?

I'm trying to remove helpers from the new usage patterns. I could add them back in though.

Should we be using auto-value for these classes and builders if they are meant to be value classes?

It might be tricky if we're trying to enforce that the list of members is immutable on the actual value, but it's probably worthwhile to have these classes fully immutable.

good call I didn't think about using auto-value and I think you're right in that it can be helpful to be fully immutable reducing the number of builders.

Please correct me but I don't think I should update the Policy class but only Binding and Condition to use Auto-Value.

This should probably be public?

This should probably be public?

Assuming this class is public, we want to keep guava types off the surface of the classes. This should be a List<String> and we'd document that the implementation is immutable so they shouldn't try to modify it.

I missed this comment a while back. The reason I'm using ImmutableList<> is based on recommendation from AutoValue docs https://github.com/google/auto/blob/master/value/userguide/builders-howto.md#-use-a-collection-valued-property. I want getMembers() to support a builder helper as well in case it's easier for users.

WDYT?

Removed ImmutableList and now using List in the surface while still supporting helper methods addMembers and removeMembers.

This could use ImmutableList.Builder if we want to keep it immutable

nit: avoid * imports

I might call this isVersion. The check* methods in guava throw exceptions when conditions are not met, whereas this method is returning a boolean

sorry, missed this. Should this be returning something?

oh, I missed this as well. Will fix.

Why @Nullable instead of a non-nullable Optional<Condition>?

Either is okay, but it's interesting that you're using both patterns in the same class.

I picked Nullable because it made more sense to me in this case. I'm not well versed in Optional's. Both patterns? Not sure I understand.

Sorry, I thought for some reason that you were also using Optional. Don't know why I thought that.

This could even take Iterable<String> if you want to be most general.

Thanks, addressing.

Do you expect this to be called other than from addMembers(String...) and removeMembers(String...)? If not, make this have default access.

No, good point.

Is this used from anywhere? Do you need it? Generally, builders don't need getters.

Same for getCondition().

removed to reduce surface size. At the moment they were convenience but I'd like to get more developer input before adding unnecessary methods to the public surface.

Why are these nullable?

Good point, no reason other than my first attempt with AutoValue. removing.

You can use an ImmutableSetMultimap instead, regardless of whether you change the method return type. You can always get the map out with Multimaps.asMap().

This whole block can be just

if (!bindingsList.equals(other.getBindingsList())) {
  return false;
}

Can etag ever be null? If not, just call etag.equals(other.getEtag()).

version is an int, so you can just do version == other.getVersion(). Otherwise you're boxing the int.

You're calling a static method on an instance, and throwing the instance away. That's not what you want to do. (If the project used Error-Prone, this would have generated a warning.)

It looks like you want a variant of BINDINGS_NO_CONDITIONS where the viewer role has a condition. What about this?

ImmutableList.of(
    BINDINGS_NO_CONDITIONS.get(0).toBuilder().setCondition(...).build(),
    BINDINGS_NO_CONDITIONS.get(1))

Although it might be nice to have a constant for each element of BINDINGS_NO_CONDITIONS (VIEWER and EDITOR)?

Sorry, I thought for some reason that you were also using Optional. Don't know why I thought that.

You could just inline emptyMembers into setMembers(ImmutableList.of()).

You don't really need to repeat this everywhere. It's better to use @Nullable for the hopefully few cases where null is accepted.

I still think all of these should use checkState instead of checkArgument so they throw IllegalStateException.

Throw IllegalArgumentException when the caller should have known that the arguments to this method are invalid.
Throw IllegalStateException when the caller should have known that this method cannot be called given the state of the object.