Issue 5532099: ACL inheritance related implementation in connector manager (Closed)
|
Created:
14 years, 4 months ago by Srinivas Modified:
14 years, 4 months ago CC:
connector-cr_google.com, plexi_google.com, connector-eng_google.com Base URL:
http://google-enterprise-connector-manager.googlecode.com/svn/trunk/projects/connector-manager/ Visibility:
Public. |
DescriptionThis CL includes the files for ACL inheritance related implementation in connector manager. Please review the changes
Patch Set 1 #
Total comments: 16
Patch Set 2 : Changes as per review #
Total comments: 64
Patch Set 3 : Changes as per review #
Total comments: 3
MessagesTotal messages: 13
please review the changes
Sign in to reply to this message.
http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... File source/java/com/google/enterprise/connector/pusher/XmlFeed.java (right): http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:679: principalBuff.append(pInfo.getName()); getName() seems like it can contain special characters. http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... File source/java/com/google/enterprise/connector/spi/Acl.java (right): http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/Acl.java:2: The rest of the header looks missing. http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/Acl.java:15: Acl(String documentId, String inheritFromId, InheritanceType inheritType, FeedType feedType) { Should this be public? http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... File source/java/com/google/enterprise/connector/spi/SecureDocument.java (right): http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/SecureDocument.java:2: The rest of the header looks missing. http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/SecureDocument.java:29: public String toString() { It would seem better to add a getTag() method and use that instead of toString() in XmlFeed. http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/SecureDocument.java:50: SecureDocument(String documentId, String inheritFromId, InheritanceType inheritType, FeedType feedType) { Should this be public or protected? http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... File source/java/com/google/enterprise/connector/spiimpl/PrincipalInfo.java (right): http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spiimpl/PrincipalInfo.java:2: The rest of the header looks missing.
Sign in to reply to this message.
Please review changes http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... File source/java/com/google/enterprise/connector/pusher/XmlFeed.java (right): http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:679: principalBuff.append(pInfo.getName()); On 2012/01/19 19:45:30, ejona wrote: > getName() seems like it can contain special characters. Thanks for pointing it out. Modified to use XmlUtil.xmlAppendAttrValue() http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... File source/java/com/google/enterprise/connector/spi/Acl.java (right): http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/Acl.java:2: On 2012/01/19 19:45:30, ejona wrote: > The rest of the header looks missing. Done. http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/Acl.java:15: Acl(String documentId, String inheritFromId, InheritanceType inheritType, FeedType feedType) { On 2012/01/19 19:45:30, ejona wrote: > Should this be public? Yes, Done http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/Acl.java:15: Acl(String documentId, String inheritFromId, InheritanceType inheritType, FeedType feedType) { On 2012/01/19 19:45:30, ejona wrote: > Should this be public? Done. http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... File source/java/com/google/enterprise/connector/spi/SecureDocument.java (right): http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/SecureDocument.java:2: On 2012/01/19 19:45:30, ejona wrote: > The rest of the header looks missing. Done. http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/SecureDocument.java:29: public String toString() { On 2012/01/19 19:45:30, ejona wrote: > It would seem better to add a getTag() method and use that instead of toString() > in XmlFeed. There are similar pattern used in spiconstants. Example: RoleType, ActionType etc. Followed this. Can change it to getTag(), if it is preferred http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/SecureDocument.java:50: SecureDocument(String documentId, String inheritFromId, InheritanceType inheritType, FeedType feedType) { On 2012/01/19 19:45:30, ejona wrote: > Should this be public or protected? Done. http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... File source/java/com/google/enterprise/connector/spiimpl/PrincipalInfo.java (right): http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spiimpl/PrincipalInfo.java:2: On 2012/01/19 19:45:30, ejona wrote: > The rest of the header looks missing. Done.
Sign in to reply to this message.
Just FYI: I've only dipped my toe in the water with the CM at this point and don't know all the conventions. My suggestions are also not authoritative as John and Brett still have a much better understanding of this code. I mainly reviewed it because it related to some work happening in the Adaptor project. I also plan to take a look at some of these code reviews to help gain knowledge of the CM. http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... File source/java/com/google/enterprise/connector/spi/SecureDocument.java (right): http://codereview.appspot.com/5532099/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/SecureDocument.java:29: public String toString() { On 2012/01/19 20:51:33, Srinivas wrote: > On 2012/01/19 19:45:30, ejona wrote: > > It would seem better to add a getTag() method and use that instead of > toString() > > in XmlFeed. > There are similar pattern used in spiconstants. Example: RoleType, ActionType > etc. Followed this. Can change it to getTag(), if it is preferred Okay, sweet, that's fine. Keeping it as-is and mimicking the other code works for me.
Sign in to reply to this message.
http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/pusher/XmlFeed.java (right): http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:326: private void xmlWrapRecord(Document document, InputStream contentStream, String contentEncoding) 80 columns. Here and all the places below. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:657: aclBuff.append("</").append(XML_ACL).append(">\n"); You might as well use XmlUtils.xmlAppendEndTag(XML_ACL, aclBuff) here. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:670: StringBuffer principalBuff = new StringBuffer(); Why don't you just pass the aclBuff from xmlWrapAclRecord to here, rather than creating another stringbuffer. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:680: principalBuff.append("</").append(XML_PRINCIPAL).append(">\n"); You might as well use XmlUtils.xmlAppendEndTag(XML_PRINCIPAL, aclBuff) here. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/spi/Acl.java (right): http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/Acl.java:3: //Licensed under the Apache License, Version 2.0 (the "License"); Add a space after //: // Licensed ... http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/Acl.java:23: * How about a block description describing the difference between this and SecureDocument (and SimpleDocument). The classes in spi and util are the ones where the javadoc is actually published. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/Acl.java:35: */ We've been scrubbing the non-Javadoc comments as we find them. I would just let the @Override suffice here. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/Acl.java:38: throw new UnsupportedOperationException(); I don't think this will work. Won't XmlFeed.getRecordUrl() call this? http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/spi/SecureDocument.java (right): http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:3: //Licensed under the Apache License, Version 2.0 (the "License"); Add space after //. Where did you copy these from? http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:26: * block comment describing SecureDocument wrt Document. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:28: public abstract class SecureDocument implements Document { I don't actually see it implementing Document. Add abstract methods for findProperty & getPropertyNames. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:31: PARENT_OVERRIDES("parent_override"), CHILD_OVERRIDES("child_override)"), AND_BOTH_PERMIT( 80 cols. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:61: ditto. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:62: public SecureDocument(String documentId, String inheritFromId, InheritanceType inheritType, FeedType feedType) { ditto. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:140: * Adds pricipal info for the secude document / acl "secude" -> "secure" http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:151: * @return Returns PrincipalInfo as List don't need "Returns" http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/spiimpl/PrincipalInfo.java (right): http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spiimpl/PrincipalInfo.java:21: * @author sveldurthi@google.com (Srinivas Veldurthi) Meaningful block comment. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spiimpl/PrincipalInfo.java:34: * @param access Here descriptions are certainly in order. Even examples. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spiimpl/PrincipalInfo.java:44: * @return Returns principal name del "Returns" http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spiimpl/PrincipalInfo.java:52: * @return Returns principal name copy pasto? @return the scope http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spiimpl/PrincipalInfo.java:60: * @return Returns principal name @return the access
Sign in to reply to this message.
http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/spi/SecureDocument.java (right): http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:141: * pricipal -> principal Also block comments should form complete sentences/paragraphs, including punctuation.
Sign in to reply to this message.
Some fixes and other comments. John L http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/pusher/XmlFeed.java (right): http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:117: //private static final String XML_URL = "url"; Delete this or add a comment explaining why it's commented out. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:320: * @param document I think Eclipse generates this stuff, yes? We delete it. It's nice to include doc comments (and required for public stuff as Brett mentioned), but not to include empty boilerplate. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:465: * Constructs the record url for the document "url" => URL here and elsewhere. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:649: if (acl.getInheritFromId() != null && !(acl.getInheritFromId().isEmpty())) { Use Strings.isNullOrEmpty (from Guava). Also, you can't use String.isEmpty anywhere, which requires Java 6. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/spi/SecureDocument.java (right): http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:28: public abstract class SecureDocument implements Document { On 2012/01/19 21:54:43, Brett wrote: > I don't actually see it implementing Document. Add abstract methods for > findProperty & getPropertyNames. I think that's redundant. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:31: PARENT_OVERRIDES("parent_override"), CHILD_OVERRIDES("child_override)"), AND_BOTH_PERMIT( The string values should (must?) match the real values: parent-overrides (hyphen + 's'), child-overrides, and-both-permit. Anton didn't implement and-neither-deny, so we can/should leave that one out. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:140: * Adds pricipal info for the secude document / acl "/ acl" => "or ACL." (including the period) http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:153: public List<PrincipalInfo> getPrincipalInfo() { PrincipalInfo should not be visible here. I feel like we have a similar case where we document that it should be ignored. Or, we can take the Value/ValueImpl approach. Make Acl an abstract class with a static factory method that creates an AclImpl from the spiimpl package, and the AclImpl can have a public getPrincipalInfo method. I think I prefer the latter.
Sign in to reply to this message.
http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/spi/Acl.java (right): http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/Acl.java:38: throw new UnsupportedOperationException(); On 2012/01/19 21:54:43, Brett wrote: > I don't think this will work. Won't XmlFeed.getRecordUrl() call this? True that. What about changing the signature to getRecordUrl(String docId, String feedType, String searchUrl), where one call site passes getOptionalString(document, ...) and the other passes Acl.getDocId, etc?
Sign in to reply to this message.
More comments from our conversation. John L http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/pusher/XmlFeed.java (right): http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:471: public String getRecordUrl(Document document) throws RepositoryException { private http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/spi/SecureDocument.java (right): http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:54: private InheritanceType inheritType; Make these final. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:153: public List<PrincipalInfo> getPrincipalInfo() { On 2012/01/20 01:25:16, John L wrote: > PrincipalInfo should not be visible here. I feel like we have a similar case > where we document that it should be ignored. Or, we can take the Value/ValueImpl > approach. Make Acl an abstract class with a static factory method that creates > an AclImpl from the spiimpl package, and the AclImpl can have a public > getPrincipalInfo method. I think I prefer the latter. Two mistakes on my part: Acl would not have needed to be abstract, but we're in SecureDocument rather than Acl, and SecureDocument cannot have a concrete subclass in spiimpl, because it's for connectors to extend with a concrete implementation. We can move PrincipalInfo back to the SPI as a top-level class, and just leave this. We should return an Collections.unmodifiableList(principalList) so that connectors can't modify our internal list.
Sign in to reply to this message.
Please review changes http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/pusher/XmlFeed.java (right): http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:117: //private static final String XML_URL = "url"; On 2012/01/20 01:25:16, John L wrote: > Delete this or add a comment explaining why it's commented out. Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:320: * @param document On 2012/01/20 01:25:16, John L wrote: > I think Eclipse generates this stuff, yes? We delete it. It's nice to include > doc comments (and required for public stuff as Brett mentioned), but not to > include empty boilerplate. Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:326: private void xmlWrapRecord(Document document, InputStream contentStream, String contentEncoding) On 2012/01/19 21:54:43, Brett wrote: > 80 columns. Here and all the places below. Thought it is 100 columns http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:326: private void xmlWrapRecord(Document document, InputStream contentStream, String contentEncoding) On 2012/01/19 21:54:43, Brett wrote: > 80 columns. Here and all the places below. Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:465: * Constructs the record url for the document On 2012/01/20 01:25:16, John L wrote: > "url" => URL here and elsewhere. Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:471: public String getRecordUrl(Document document) throws RepositoryException { On 2012/01/20 21:56:47, John L wrote: > private Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:649: if (acl.getInheritFromId() != null && !(acl.getInheritFromId().isEmpty())) { On 2012/01/20 01:25:16, John L wrote: > Use Strings.isNullOrEmpty (from Guava). Also, you can't use String.isEmpty > anywhere, which requires Java 6. Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:657: aclBuff.append("</").append(XML_ACL).append(">\n"); On 2012/01/19 21:54:43, Brett wrote: > You might as well use XmlUtils.xmlAppendEndTag(XML_ACL, aclBuff) here. Yes, done http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:657: aclBuff.append("</").append(XML_ACL).append(">\n"); On 2012/01/19 21:54:43, Brett wrote: > You might as well use XmlUtils.xmlAppendEndTag(XML_ACL, aclBuff) here. Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:670: StringBuffer principalBuff = new StringBuffer(); On 2012/01/19 21:54:43, Brett wrote: > Why don't you just pass the aclBuff from xmlWrapAclRecord to here, rather than > creating another stringbuffer. Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:680: principalBuff.append("</").append(XML_PRINCIPAL).append(">\n"); On 2012/01/19 21:54:43, Brett wrote: > You might as well use XmlUtils.xmlAppendEndTag(XML_PRINCIPAL, aclBuff) here. Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/spi/Acl.java (right): http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/Acl.java:3: //Licensed under the Apache License, Version 2.0 (the "License"); On 2012/01/19 21:54:43, Brett wrote: > Add a space after //: > // Licensed ... Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/Acl.java:35: */ On 2012/01/19 21:54:43, Brett wrote: > We've been scrubbing the non-Javadoc comments as we find them. I would just let > the @Override suffice here. Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/Acl.java:38: throw new UnsupportedOperationException(); On 2012/01/19 21:54:43, Brett wrote: > I don't think this will work. Won't XmlFeed.getRecordUrl() call this? XmlFeed.getRecordUrl() doesn't call this. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/Acl.java:38: throw new UnsupportedOperationException(); On 2012/01/20 21:38:52, John L wrote: > On 2012/01/19 21:54:43, Brett wrote: > > I don't think this will work. Won't XmlFeed.getRecordUrl() call this? > > True that. What about changing the signature to getRecordUrl(String docId, > String feedType, String searchUrl), where one call site passes > getOptionalString(document, ...) and the other passes Acl.getDocId, etc? Yes, getOptionalSring() in getRecordUrl() calls finProperty. Modified getRecordUrl signature and it's implementation. Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/Acl.java:38: throw new UnsupportedOperationException(); On 2012/01/20 21:38:52, John L wrote: > On 2012/01/19 21:54:43, Brett wrote: > > I don't think this will work. Won't XmlFeed.getRecordUrl() call this? > > True that. What about changing the signature to getRecordUrl(String docId, > String feedType, String searchUrl), where one call site passes > getOptionalString(document, ...) and the other passes Acl.getDocId, etc? Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/spi/SecureDocument.java (right): http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:3: //Licensed under the Apache License, Version 2.0 (the "License"); On 2012/01/19 21:54:43, Brett wrote: > Add space after //. Where did you copy these from? Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:31: PARENT_OVERRIDES("parent_override"), CHILD_OVERRIDES("child_override)"), AND_BOTH_PERMIT( On 2012/01/19 21:54:43, Brett wrote: > 80 cols. Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:31: PARENT_OVERRIDES("parent_override"), CHILD_OVERRIDES("child_override)"), AND_BOTH_PERMIT( On 2012/01/20 01:25:16, John L wrote: > The string values should (must?) match the real values: parent-overrides (hyphen > + 's'), child-overrides, and-both-permit. Anton didn't implement > and-neither-deny, so we can/should leave that one out. Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:54: private InheritanceType inheritType; On 2012/01/20 21:56:47, John L wrote: > Make these final. Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:61: On 2012/01/19 21:54:43, Brett wrote: > ditto. Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:62: public SecureDocument(String documentId, String inheritFromId, InheritanceType inheritType, FeedType feedType) { On 2012/01/19 21:54:43, Brett wrote: > ditto. Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:140: * Adds pricipal info for the secude document / acl On 2012/01/19 21:54:43, Brett wrote: > "secude" -> "secure" Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:140: * Adds pricipal info for the secude document / acl On 2012/01/20 01:25:16, John L wrote: > "/ acl" => "or ACL." (including the period) Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:141: * On 2012/01/19 22:23:01, Brett wrote: > pricipal -> principal > > Also block comments should form complete sentences/paragraphs, including > punctuation. Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:151: * @return Returns PrincipalInfo as List On 2012/01/19 21:54:43, Brett wrote: > don't need "Returns" Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:153: public List<PrincipalInfo> getPrincipalInfo() { On 2012/01/20 21:56:47, John L wrote: > On 2012/01/20 01:25:16, John L wrote: > > PrincipalInfo should not be visible here. I feel like we have a similar case > > where we document that it should be ignored. Or, we can take the > Value/ValueImpl > > approach. Make Acl an abstract class with a static factory method that creates > > an AclImpl from the spiimpl package, and the AclImpl can have a public > > getPrincipalInfo method. I think I prefer the latter. > > Two mistakes on my part: Acl would not have needed to be abstract, but we're in > SecureDocument rather than Acl, and SecureDocument cannot have a concrete > subclass in spiimpl, because it's for connectors to extend with a concrete > implementation. We can move PrincipalInfo back to the SPI as a top-level class, > and just leave this. We should return an > Collections.unmodifiableList(principalList) so that connectors can't modify our > internal list. Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/spiimpl/PrincipalInfo.java (right): http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spiimpl/PrincipalInfo.java:44: * @return Returns principal name On 2012/01/19 21:54:43, Brett wrote: > del "Returns" Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spiimpl/PrincipalInfo.java:52: * @return Returns principal name On 2012/01/19 21:54:43, Brett wrote: > copy pasto? @return the scope Done. http://codereview.appspot.com/5532099/diff/5002/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spiimpl/PrincipalInfo.java:60: * @return Returns principal name On 2012/01/19 21:54:43, Brett wrote: > @return the access Done.
Sign in to reply to this message.
http://codereview.appspot.com/5532099/diff/7003/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/spi/SecureDocument.java (right): http://codereview.appspot.com/5532099/diff/7003/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SecureDocument.java:64: I really wish it was easier to wrap an existing Document, like the Filters do. How about a constructor that takes a Document, then gets the other fields from it? public SecureDocument(Document doc, String inheritFromId, InheritanceType inheritType) { this(DocUtils.getRequiredString(doc, SpiConstants.PROPNAME_DOCID, inheritFromId, inheritType, DocUtils.getOptionalString(doc, SpiConstants.PROPNAME_FEEDTYPE, DocUtils.getOptionalString(doc, SpiConstants.PROPNAME_SEARCHURL); this.sourceDoc = doc; } Then implement findProperty and getPropertyNames that throw unsupported op if sourceDoc is null.
Sign in to reply to this message.
Publishing some draft comments, but it seems based on our previous conversation that the non-properties implementation of Acl was not holding together, and Srinivas was going to explore some TDD of what we want to work, and then come back with possibly a very different implementation, most likely properties-based. John L http://codereview.appspot.com/5532099/diff/7003/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/pusher/XmlFeed.java (right): http://codereview.appspot.com/5532099/diff/7003/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:351: //String searchUrl = getRecordUrl(document); Delete this line. http://codereview.appspot.com/5532099/diff/7003/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:470: private String getRecordUrl(String docId, String feedType, String searchUrl) Sorry, I think this was my misdirection. The feedType is available, I don't think we need to pass it in. But the feedType is expected to be in a property.
Sign in to reply to this message.
This code review has been superseded by http://codereview.appspot.com/5577067/ John L
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
