Code review - Issue 291280043: Principal is domainFormat agnostic.https://codereview.appspot.com/2016-03-24T23:10:25+00:00rietveld
Message from unknown
2016-03-24T15:36:06+00:00afedorenchikurn:md5:75343f418e43ac4139865e29f9f42541
Message from afedorenchik@google.com
2016-03-24T15:36:12+00:00afedorenchikurn:md5:21a1905707a579be291bd0180be99bf4
Message from myk@google.com
2016-03-24T16:03:28+00:00mykurn:md5:d7741329bd52b99ee891956f9178d7d1
The changes to Principal.java look great -- but while you have made tiny changes to a couple of other test files, what is missing is changes to PrincipalTest.java to demonstrate that principals composed with different domain formats are (now) equal to each other (as well as having a zero value when calling compareTo() on each other). That's the only thing that's left to do.
https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/adaptor/Principal.java
File src/com/google/enterprise/adaptor/Principal.java (right):
https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/adaptor/Principal.java#newcode69
src/com/google/enterprise/adaptor/Principal.java:69: && p.parse().plainName.equals(parse().plainName)
easier to read than creating new variables to hold the parsed entities. Parsing doesn't take too long; it's OK to do it twice.
https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/adaptor/Principal.java#newcode77
src/com/google/enterprise/adaptor/Principal.java:77: return Arrays.hashCode(new Object[]{ isUser(), parse().domain, parse().plainName, namespace });
Line too long; please split after parse().domain, (and indent the next line 4 additional spaces).
https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/adaptor/Principal.java#newcode102
src/com/google/enterprise/adaptor/Principal.java:102: // Only for users
This comment doesn't explain why you are checking domains only if both principals are users.
Suggestion: replace with something along the lines of:
// Skip the domain comparison for groups because ____
(fill in the blank with the justification).
https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/adaptor/Principal.java#newcode103
src/com/google/enterprise/adaptor/Principal.java:103: if (isUser() && other.isUser()) {
lines 97-98 imply that isUser() and other.isUser() have the same value -- you could just check
if (isUser()) here.
https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/adaptor/Principal.java#newcode148
src/com/google/enterprise/adaptor/Principal.java:148:
It's ok to keep this change (removal of leading blanks) in.
https://codereview.appspot.com/291280043/diff/1/test/com/google/enterprise/adaptor/AclTransformTest.java
File test/com/google/enterprise/adaptor/AclTransformTest.java (right):
https://codereview.appspot.com/291280043/diff/1/test/com/google/enterprise/adaptor/AclTransformTest.java#newcode193
test/com/google/enterprise/adaptor/AclTransformTest.java:193: }
but for this file, it seems that the *only* change is whitespace. So I would suggest reverting this file (unless you really do have changes to make here).
Message from unknown
2016-03-24T18:49:07+00:00afedorenchikurn:md5:262e7a6c882311a0516de8900ea3ff78
Message from afedorenchik@google.com
2016-03-24T18:55:17+00:00afedorenchikurn:md5:3bcb609e1ab428f1d85bedfa0c048500
Hi,
I've done changes and all tests are good.
Please let me know if you have other comments.
https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/adaptor/Principal.java
File src/com/google/enterprise/adaptor/Principal.java (right):
https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/adaptor/Principal.java#newcode69
src/com/google/enterprise/adaptor/Principal.java:69: && p.parse().plainName.equals(parse().plainName)
On 2016/03/24 16:03:28, myk wrote:
> easier to read than creating new variables to hold the parsed entities. Parsing
> doesn't take too long; it's OK to do it twice.
Acknowledged. Thanks.
https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/adaptor/Principal.java#newcode77
src/com/google/enterprise/adaptor/Principal.java:77: return Arrays.hashCode(new Object[]{ isUser(), parse().domain, parse().plainName, namespace });
On 2016/03/24 16:03:28, myk wrote:
> Line too long; please split after parse().domain, (and indent the next line 4
> additional spaces).
Done. Not sure about proper intend.
https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/adaptor/Principal.java#newcode102
src/com/google/enterprise/adaptor/Principal.java:102: // Only for users
On 2016/03/24 16:03:28, myk wrote:
> This comment doesn't explain why you are checking domains only if both
> principals are users.
> Suggestion: replace with something along the lines of:
> // Skip the domain comparison for groups because ____
> (fill in the blank with the justification).
Done.
https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/adaptor/Principal.java#newcode103
src/com/google/enterprise/adaptor/Principal.java:103: if (isUser() && other.isUser()) {
On 2016/03/24 16:03:28, myk wrote:
> lines 97-98 imply that isUser() and other.isUser() have the same value -- you
> could just check
> if (isUser()) here.
Done.
Message from unknown
2016-03-24T18:58:50+00:00afedorenchikurn:md5:828faf31476875ba2621616171b3f7ad
Message from myk@google.com
2016-03-24T21:08:12+00:00mykurn:md5:fa65a4392f06b66218e644e9d5cb3b0b
Looks good to me. Thank you very much for your work!
https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/adaptor/Principal.java
File src/com/google/enterprise/adaptor/Principal.java (right):
https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/adaptor/Principal.java#newcode77
src/com/google/enterprise/adaptor/Principal.java:77: return Arrays.hashCode(new Object[]{ isUser(), parse().domain, parse().plainName, namespace });
On 2016/03/24 18:55:16, afedorenchik wrote:
> On 2016/03/24 16:03:28, myk wrote:
> > Line too long; please split after parse().domain, (and indent the next line
> 4
> > additional spaces).
>
> Done. Not sure about proper intend.
4 was correct here. In general, indent two for a new block:
if (condition) {
thenAction(); // indented 2
} else {
elseAction(); // indented 2
}
but indent 4 when the next line is split from the current line due to line length limits. If the second line isn't enough, the third line stays with the second line's indent - it does not indent an additional 4.
https://codereview.appspot.com/291280043/diff/40001/test/com/google/enterprise/adaptor/PrincipalTest.java
File test/com/google/enterprise/adaptor/PrincipalTest.java (right):
https://codereview.appspot.com/291280043/diff/40001/test/com/google/enterprise/adaptor/PrincipalTest.java#newcode146
test/com/google/enterprise/adaptor/PrincipalTest.java:146: assertFalse(p6.equals(g1));
Above new tests look good! I'm surprised that Eclipse let you put in a line with spaces only (lines 137, 141, 143). It's OK with me if you leave those spaces in (or if you decide to delete them). If you do delete them (but make no other change), feel free to submit.
Message from afedorenchik@google.com
2016-03-24T21:56:24+00:00afedorenchikurn:md5:12e89c060b9c3beb172fbb19c28c3fa3
https://codereview.appspot.com/291280043/diff/40001/test/com/google/enterprise/adaptor/PrincipalTest.java
File test/com/google/enterprise/adaptor/PrincipalTest.java (right):
https://codereview.appspot.com/291280043/diff/40001/test/com/google/enterprise/adaptor/PrincipalTest.java#newcode146
test/com/google/enterprise/adaptor/PrincipalTest.java:146: assertFalse(p6.equals(g1));
On 2016/03/24 21:08:12, myk wrote:
> Above new tests look good! I'm surprised that Eclipse let you put in a line
> with spaces only (lines 137, 141, 143). It's OK with me if you leave those
> spaces in (or if you decide to delete them). If you do delete them (but make no
> other change), feel free to submit.
Acknowledged. Not sure if it OK to leave as is. I can remove this spaces. Also confused why Eclipse does not removed them on save.
Message from unknown
2016-03-24T23:10:25+00:00afedorenchikurn:md5:1565d73695b6d185a43b710789471703