FSA Add AclBuilderTest

FSA Add AclBuilderTest

[ejona, 2013-12-20 16:32:31]

https://codereview.appspot.com/35900047/diff/1/test/com/google/enterprise/ada...
File test/com/google/enterprise/adaptor/fs/AclBuilderTest.java (right):

https://codereview.appspot.com/35900047/diff/1/test/com/google/enterprise/ada...
test/com/google/enterprise/adaptor/fs/AclBuilderTest.java:230:
user("joe").type(ALLOW).perms(GENERIC_READ)
Do you need joe in this test, and the next three?

If you remove joe, then you can use equality checks instead of
Contains/NotContains. The NotContains turn into equal check with empty Acl and
Contains turns into a single-entry ACL.

https://codereview.appspot.com/35900047/diff/1/test/com/google/enterprise/ada...
test/com/google/enterprise/adaptor/fs/AclBuilderTest.java:321: private void
checkAcl(Acl acl, InheritanceType expectedType)
Unused.

https://codereview.appspot.com/35900047/diff/1/test/com/google/enterprise/ada...
test/com/google/enterprise/adaptor/fs/AclBuilderTest.java:329:
Sets.newHashSet("joe", "mary"), Sets.newHashSet("mike"),
This would make a good pre-created ACL defined statically or similar:

Acl simpleAcl = new Acl.Builder()
    .permitUsers(users("joe", "mary")).denyUsers(users("mike"))
    .permitGroups(groups("EVERYONE")).denyGroups(groups("sales"))
    .setEverythingCaseInsensitive().build();

And then you replace this:
  checkAcl(acl, InheritanceType.CHILD_OVERRIDES, fragment);
with:
  assertEquals(acl, new Acl.Builder(simpleAcl)
      .setInheritanceType(InheritanceType.CHILD_OVERRIDES)
      .setInheritFrom(inheritId, fragment));

Yes it is longer, but I think it is much more obvious then what is happening.
With checkAcl() as it stands, it isn't obvious it is using joe, mary, etc. Every
time I read the call it looks like it is only going to check inheritance type
and fragment. And the fact that there is no inheritFrom docId specified gets me
too.

users() would look like:
List<UserPrincipal> users(String... usernames) {
  List<UserPrincipal> principals = new ArrayList<UserPrincipal>;
  for (String username : usernames) {
    principals.add(user(username));
  }
  return principals;
}

This change isn't necessary, but would add more confidence in the tests behaving
as expected (the tests are more obviously correct) and not letting things slip
through the cracks.

https://codereview.appspot.com/35900047/diff/1/test/com/google/enterprise/ada...
File test/com/google/enterprise/adaptor/fs/AclView.java (right):

https://codereview.appspot.com/35900047/diff/1/test/com/google/enterprise/ada...
test/com/google/enterprise/adaptor/fs/AclView.java:73: private final
AclEntryPermission[] perms;
This is mutable in an enum. Using Arrays.asList() and
Collections.unmodifiableList() in the constructor would be nice. And it seems
that doesn't add too much burden because you are already doing asList()
elsewhere (and so that could go away).

https://codereview.appspot.com/35900047/diff/1/test/com/google/enterprise/ada...
test/com/google/enterprise/adaptor/fs/AclView.java:137:
this.permissions.addAll(Arrays.asList(permissions));
The 'set' behavior is much preferred over this 'append'. It appears you don't
need to append; set works just fine.

[Brett, 2014-01-03 19:29:54]

https://codereview.appspot.com/35900047/diff/1/test/com/google/enterprise/ada...
File test/com/google/enterprise/adaptor/fs/AclBuilderTest.java (right):

https://codereview.appspot.com/35900047/diff/1/test/com/google/enterprise/ada...
test/com/google/enterprise/adaptor/fs/AclBuilderTest.java:230:
user("joe").type(ALLOW).perms(GENERIC_READ)
On 2013/12/20 16:32:31, ejona wrote:
> Do you need joe in this test, and the next three?
> 
> If you remove joe, then you can use equality checks instead of
> Contains/NotContains. The NotContains turn into equal check with empty Acl and
> Contains turns into a single-entry ACL.

Although I *could* just have a single entry ACL, I try to have some ACEs that
pass through unmolested, just for a bit of sanity.  There was a FS Connector bug
where everything after a NO_PROPAGATE ACE got ignored.  Granted, I should have
more fencepost tests like that...

https://codereview.appspot.com/35900047/diff/1/test/com/google/enterprise/ada...
test/com/google/enterprise/adaptor/fs/AclBuilderTest.java:321: private void
checkAcl(Acl acl, InheritanceType expectedType)
On 2013/12/20 16:32:31, ejona wrote:
> Unused.

Removed

https://codereview.appspot.com/35900047/diff/1/test/com/google/enterprise/ada...
File test/com/google/enterprise/adaptor/fs/AclView.java (right):

https://codereview.appspot.com/35900047/diff/1/test/com/google/enterprise/ada...
test/com/google/enterprise/adaptor/fs/AclView.java:73: private final
AclEntryPermission[] perms;
On 2013/12/20 16:32:31, ejona wrote:
> This is mutable in an enum. Using Arrays.asList() and
> Collections.unmodifiableList() in the constructor would be nice. And it seems
> that doesn't add too much burden because you are already doing asList()
> elsewhere (and so that could go away).

Done.  But went with a Collections.unmodifiableSet(EnumSet) instead.

https://codereview.appspot.com/35900047/diff/1/test/com/google/enterprise/ada...
test/com/google/enterprise/adaptor/fs/AclView.java:137:
this.permissions.addAll(Arrays.asList(permissions));
On 2013/12/20 16:32:31, ejona wrote:
> The 'set' behavior is much preferred over this 'append'. It appears you don't
> need to append; set works just fine.

I will revert this change.  I initially had tests that mixed the generic and the
specific perms, but those were unnecessary.

[Brett, 2014-01-03 19:30:28]

Eric's Feedback. Use Acl.equals() for assertions.

[ejona, 2014-01-14 00:32:03]

LGTM

https://codereview.appspot.com/35900047/diff/20001/test/com/google/enterprise...
File test/com/google/enterprise/adaptor/fs/AclBuilderTest.java (right):

https://codereview.appspot.com/35900047/diff/20001/test/com/google/enterprise...
test/com/google/enterprise/adaptor/fs/AclBuilderTest.java:64: private final
Set<UserPrincipal> emptyUsers = ImmutableSet.of();
Unused.

[Brett, 2014-01-14 18:10:17]

Committed 13 January 2014 to Filesystem Adaptor.