Issue 5577067: ACL inheritance related implementation in connector manager (Closed)
|
Created:
14 years, 4 months ago by Srinivas Modified:
14 years, 4 months ago Reviewers:
JohnL, brett.michael.johnson CC:
connector-cr_google.com, plexi_google.com, connector-eng_google.com, gsa-connectors-dev-forum_googlegroups.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. This redesign used doc properties for ACL
Patch Set 1 #
Total comments: 52
Patch Set 2 : Modifications as per review #
Total comments: 28
MessagesTotal messages: 13
Please review changes
Sign in to reply to this message.
http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... File source/java/com/google/enterprise/connector/spi/SpiConstants.java (right): http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/SpiConstants.java:367: return (this == other || (this != CONTENT && other != CONTENT)); return (this == other || this == ACL || (this != CONTENT && other != CONTENT)); ACL is compatible with any existing feed type. The only weird thing would be if the connector feeds a bunch of ACLs before feeding any real documents. At that point, the XmlFeed will have a FeedType of ACL, which when it is sent to the GSA will have a feedtype of metadata-and-url (see toLegacyString below). If the first real document is a CONTENT feed, then we will close the ACL feed, and start a new CONTENT feed. This is the correct thing to do. If the first document is WEB or CONTENTURL, then we will append it to the accumulating ACL feed. That too is OK, as they would produce "metadata-and-url" feeds as well. The log messages coming out of DocPusher may be dodgy, tho. The log messages would be consistent if we did this: return (this == other || this == ACL || other != ACL || (this != CONTENT && other != CONTENT)); which would always wrap up a bunch of leading ACLs into a single feed once we got a real document, but still allow ACLs to be appended to an existing feed.
Sign in to reply to this message.
+gsa-connectors-dev-forum
Sign in to reply to this message.
I think this will be much better than our original design. I've marked with (6.15.8) things that have changed in the ACL design in that build. John L http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... File source/java/com/google/enterprise/connector/pusher/XmlFeed.java (right): http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:114: private static final String XML_TYPE = "type"; "type" => "inheritance-type" (6.15.8) http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:115: private static final String XML_INHERIT_FROM = "inherit_from"; "inherit_from" => "inherit-from" (6.15.8) http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:335: Delete extra blank line. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:359: String searchUrl = getRecordUrl( We can go back to passing the document to getRecordUrl. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:509: XmlUtils.xmlAppendAttr(XML_TYPE, inheritanceType, aclBuff); What happens if inheritance type is not specified? Wrap this in an if? http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:512: if (!Strings.isNullOrEmpty(inheritFrom)) { inherit-from is now an attribute on the acl element (a peer of inheritance-type), rather than a separate nested element. (6.15.8) http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:523: Delete extra blank line. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:527: * Generate the acl principal xml data "acl" => "ACL", "xml" => "XML" here and elsewhere as needed. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:531: Delete extra blank line. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:532: Set<String> propertyNames = acl.getPropertyNames(); Instead of all this, just call findProperty on each of the four properties (aclusers, aclgroups, acldenyusers, and acldenygroups), and if the result isn't null, pass the value to wrapAclPrincipal as you are doing. On top of being cleaner, a connector is not required to advertise these properties in getPropertyNames. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:574: private static void wrapAclPrinicpal( StringBuffer buff, Property property, Delete extra space after "(". http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:577: ValueImpl value = null; The compiler should allow "ValueImpl value;" here. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... File source/java/com/google/enterprise/connector/spi/SpiConstants.java (right): http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/SpiConstants.java:367: return (this == other || (this != CONTENT && other != CONTENT)); On 2012/01/28 02:02:50, Brett wrote: > return (this == other || this == ACL || (this != CONTENT && other != CONTENT)); > > ACL is compatible with any existing feed type. The only weird thing would be if > the connector feeds a bunch of ACLs before feeding any real documents. At that > point, the XmlFeed will have a FeedType of ACL, which when it is sent to the GSA > will have a feedtype of metadata-and-url (see toLegacyString below). If the > first real document is a CONTENT feed, then we will close the ACL feed, and > start a new CONTENT feed. This is the correct thing to do. If the first > document is WEB or CONTENTURL, then we will append it to the accumulating ACL > feed. That too is OK, as they would produce "metadata-and-url" feeds as well. > The log messages coming out of DocPusher may be dodgy, tho. > > The log messages would be consistent if we did this: > > return (this == other || this == ACL || other != ACL || (this != CONTENT && > other != CONTENT)); > > which would always wrap up a bunch of leading ACLs into a single feed once we > got a real document, but still allow ACLs to be appended to an existing feed. To say it's the "correct thing to do" is misleading. It's possibly OK to send leading ACLs in their own feeds. It is not OK, however, to send CONTENT, ACL, CONTENT in multiple feeds, which both of these implementations would do. I think what we need to do is never call isCompatible on or with ACL, and don't remember the ACL in the middle so that when looking at the second this=CONTENT, other=CONTENT as well. That will also correctly handle, e.g., CONTENT, ACL, CONTENTURL. I think just something like "xmlFeed != null && feedType != ACL && !feedType.isCompatible(..) in DocPusher might do the trick. To avoid complete failure in the presence of bugs in that handling, we could say here that ACL is compatible only with ACL. Horribly inefficient, but never wrong. Also, I think this boolean expression is complex enough now to warrant an outer switch (this) { ... }, possibly with an early return for this==other. We can either return an error if this or other is ACL, or we can say return (this == other || this == ACL || other == ACL || (this != CONTENT && other != CONTENT)); http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/SpiConstants.java:625: * Value: google:inheritancetype google:aclinheritancetype, and these properties should be up with the existing ACL properties, and have @since 3.0 tags in the doc comments. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/SpiConstants.java:627: public static final String PROPNAME_INHERITANCETYPE = "google:inheritancetype"; PROPNAME_ACLINHERITANCETYPE http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/SpiConstants.java:633: * Value: google:inheritfrom google:aclinheritfrom http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/SpiConstants.java:635: public static final String PROPNAME_INHERITFROM = "google:inheritfrom"; PROPNAME_ACLINHERITFROM http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/SpiConstants.java:644: public static final String PROPNAME_ACLDENYUSERS = "google:acldenyusers"; Add a TODO here to clarify the behavior of and support for roles in DENY ACEs. http://codereview.appspot.com/5577067/diff/1/source/javatests/com/google/ente... File source/javatests/com/google/enterprise/connector/pusher/DocPusherTest.java (right): http://codereview.appspot.com/5577067/diff/1/source/javatests/com/google/ente... source/javatests/com/google/enterprise/connector/pusher/DocPusherTest.java:2497: /** I think it's critical to have tests of sequences of feed type values. Not just pairs, but, e.g., CONTENT, ACL, CONTENT, or CONTENT, ACL, CONTENTURL. I would go crazy and have a table-driven test that checks all combinations. Worst case is you list the expected results, or maybe you could find some brute force, different algorithm from the code you're testing to get the answers. http://codereview.appspot.com/5577067/diff/1/source/javatests/com/google/ente... source/javatests/com/google/enterprise/connector/pusher/DocPusherTest.java:2498: * Test Acl document. "Test Acl" => "Tests ACL". http://codereview.appspot.com/5577067/diff/1/source/javatests/com/google/ente... source/javatests/com/google/enterprise/connector/pusher/DocPusherTest.java:2501: Delete extra blank line. http://codereview.appspot.com/5577067/diff/1/source/javatests/com/google/ente... source/javatests/com/google/enterprise/connector/pusher/DocPusherTest.java:2518: System.out.println(resultXML); Don't forget to remove this before committing. http://codereview.appspot.com/5577067/diff/1/source/javatests/com/google/ente... source/javatests/com/google/enterprise/connector/pusher/DocPusherTest.java:2530: "<principal scope=\"GROUP\" access=\"PERMIT\">Engineering</principal>", 80 cols (drag, I know). http://codereview.appspot.com/5577067/diff/1/source/javatests/com/google/ente... source/javatests/com/google/enterprise/connector/pusher/DocPusherTest.java:2533: } catch (Exception e) { Why are you catching Exception? It's usually much better to let it be thrown to get a stack trace when debugging failing tests. http://codereview.appspot.com/5577067/diff/1/source/javatests/com/google/ente... source/javatests/com/google/enterprise/connector/pusher/DocPusherTest.java:2536: Delete extra blank line.
Sign in to reply to this message.
Slight correction to one of my comments. John L http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... File source/java/com/google/enterprise/connector/pusher/XmlFeed.java (right): http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:532: Set<String> propertyNames = acl.getPropertyNames(); On 2012/01/28 04:42:13, John L wrote: > Instead of all this, just call findProperty on each of the four properties > (aclusers, aclgroups, acldenyusers, and acldenygroups), and if the result isn't > null, pass the value to wrapAclPrincipal as you are doing. On top of being > cleaner, a connector is not required to advertise these properties in > getPropertyNames. A connector is generally not required to advertise all google: properties in getPropertyNames, but I think for the ACL properties maybe it does. In any case, we should just call findProperty on the known constants when we know what property we want. No need to loop over getPropertyNames.
Sign in to reply to this message.
Please review changes http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... File source/java/com/google/enterprise/connector/pusher/XmlFeed.java (right): http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:114: private static final String XML_TYPE = "type"; On 2012/01/28 04:42:13, John L wrote: > "type" => "inheritance-type" (6.15.8) Done. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:115: private static final String XML_INHERIT_FROM = "inherit_from"; On 2012/01/28 04:42:13, John L wrote: > "inherit_from" => "inherit-from" (6.15.8) Done. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:335: On 2012/01/28 04:42:13, John L wrote: > Delete extra blank line. Done. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:359: String searchUrl = getRecordUrl( On 2012/01/28 04:42:13, John L wrote: > We can go back to passing the document to getRecordUrl. Done. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:509: XmlUtils.xmlAppendAttr(XML_TYPE, inheritanceType, aclBuff); On 2012/01/28 04:42:13, John L wrote: > What happens if inheritance type is not specified? Wrap this in an if? Done. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:512: if (!Strings.isNullOrEmpty(inheritFrom)) { On 2012/01/28 04:42:13, John L wrote: > inherit-from is now an attribute on the acl element (a peer of > inheritance-type), rather than a separate nested element. (6.15.8) Done. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:523: On 2012/01/28 04:42:13, John L wrote: > Delete extra blank line. Done. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:527: * Generate the acl principal xml data On 2012/01/28 04:42:13, John L wrote: > "acl" => "ACL", "xml" => "XML" here and elsewhere as needed. Done. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:531: On 2012/01/28 04:42:13, John L wrote: > Delete extra blank line. Done. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:532: Set<String> propertyNames = acl.getPropertyNames(); On 2012/01/28 20:41:27, John L wrote: > On 2012/01/28 04:42:13, John L wrote: > > Instead of all this, just call findProperty on each of the four properties > > (aclusers, aclgroups, acldenyusers, and acldenygroups), and if the result > isn't > > null, pass the value to wrapAclPrincipal as you are doing. On top of being > > cleaner, a connector is not required to advertise these properties in > > getPropertyNames. > > A connector is generally not required to advertise all google: properties in > getPropertyNames, but I think for the ACL properties maybe it does. In any case, > we should just call findProperty on the known constants when we know what > property we want. No need to loop over getPropertyNames. Done. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:532: Set<String> propertyNames = acl.getPropertyNames(); On 2012/01/28 04:42:13, John L wrote: > Instead of all this, just call findProperty on each of the four properties > (aclusers, aclgroups, acldenyusers, and acldenygroups), and if the result isn't > null, pass the value to wrapAclPrincipal as you are doing. On top of being > cleaner, a connector is not required to advertise these properties in > getPropertyNames. Done. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:574: private static void wrapAclPrinicpal( StringBuffer buff, Property property, On 2012/01/28 04:42:13, John L wrote: > Delete extra space after "(". Done. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:577: ValueImpl value = null; On 2012/01/28 04:42:13, John L wrote: > The compiler should allow "ValueImpl value;" here. Done. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... File source/java/com/google/enterprise/connector/spi/SpiConstants.java (right): http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/SpiConstants.java:367: return (this == other || (this != CONTENT && other != CONTENT)); On 2012/01/28 04:42:13, John L wrote: > On 2012/01/28 02:02:50, Brett wrote: > > return (this == other || this == ACL || (this != CONTENT && other != > CONTENT)); > > > > ACL is compatible with any existing feed type. The only weird thing would be > if > > the connector feeds a bunch of ACLs before feeding any real documents. At > that > > point, the XmlFeed will have a FeedType of ACL, which when it is sent to the > GSA > > will have a feedtype of metadata-and-url (see toLegacyString below). If the > > first real document is a CONTENT feed, then we will close the ACL feed, and > > start a new CONTENT feed. This is the correct thing to do. If the first > > document is WEB or CONTENTURL, then we will append it to the accumulating ACL > > feed. That too is OK, as they would produce "metadata-and-url" feeds as well. > > > The log messages coming out of DocPusher may be dodgy, tho. > > > > The log messages would be consistent if we did this: > > > > return (this == other || this == ACL || other != ACL || (this != CONTENT && > > other != CONTENT)); > > > > which would always wrap up a bunch of leading ACLs into a single feed once we > > got a real document, but still allow ACLs to be appended to an existing feed. > > To say it's the "correct thing to do" is misleading. It's possibly OK to send > leading ACLs in their own feeds. It is not OK, however, to send CONTENT, ACL, > CONTENT in multiple feeds, which both of these implementations would do. I think > what we need to do is never call isCompatible on or with ACL, and don't remember > the ACL in the middle so that when looking at the second this=CONTENT, > other=CONTENT as well. That will also correctly handle, e.g., CONTENT, ACL, > CONTENTURL. I think just something like "xmlFeed != null && feedType != ACL && > !feedType.isCompatible(..) in DocPusher might do the trick. > > To avoid complete failure in the presence of bugs in that handling, we could say > here that ACL is compatible only with ACL. Horribly inefficient, but never > wrong. > > Also, I think this boolean expression is complex enough now to warrant an outer > switch (this) { ... }, possibly with an early return for this==other. > We can either return an error if this or other is ACL, or we can say > return (this == other || this == ACL || other == ACL || (this != CONTENT && > other != CONTENT)); as per our discussion modified to return (this == other || this == ACL || (this != CONTENT && other != CONTENT)); Done. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/SpiConstants.java:625: * Value: google:inheritancetype On 2012/01/28 04:42:13, John L wrote: > google:aclinheritancetype, and these properties should be up with the existing > ACL properties, and have @since 3.0 tags in the doc comments. Done. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/SpiConstants.java:627: public static final String PROPNAME_INHERITANCETYPE = "google:inheritancetype"; On 2012/01/28 04:42:13, John L wrote: > PROPNAME_ACLINHERITANCETYPE Done. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/SpiConstants.java:633: * Value: google:inheritfrom On 2012/01/28 04:42:13, John L wrote: > google:aclinheritfrom Done. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/SpiConstants.java:635: public static final String PROPNAME_INHERITFROM = "google:inheritfrom"; On 2012/01/28 04:42:13, John L wrote: > PROPNAME_ACLINHERITFROM Done. http://codereview.appspot.com/5577067/diff/1/source/java/com/google/enterpris... source/java/com/google/enterprise/connector/spi/SpiConstants.java:644: public static final String PROPNAME_ACLDENYUSERS = "google:acldenyusers"; On 2012/01/28 04:42:13, John L wrote: > Add a TODO here to clarify the behavior of and support for roles in DENY ACEs. Done. http://codereview.appspot.com/5577067/diff/1/source/javatests/com/google/ente... File source/javatests/com/google/enterprise/connector/pusher/DocPusherTest.java (right): http://codereview.appspot.com/5577067/diff/1/source/javatests/com/google/ente... source/javatests/com/google/enterprise/connector/pusher/DocPusherTest.java:2498: * Test Acl document. On 2012/01/28 04:42:13, John L wrote: > "Test Acl" => "Tests ACL". Done. http://codereview.appspot.com/5577067/diff/1/source/javatests/com/google/ente... source/javatests/com/google/enterprise/connector/pusher/DocPusherTest.java:2501: On 2012/01/28 04:42:13, John L wrote: > Delete extra blank line. Done. http://codereview.appspot.com/5577067/diff/1/source/javatests/com/google/ente... source/javatests/com/google/enterprise/connector/pusher/DocPusherTest.java:2518: System.out.println(resultXML); On 2012/01/28 04:42:13, John L wrote: > Don't forget to remove this before committing. Done. http://codereview.appspot.com/5577067/diff/1/source/javatests/com/google/ente... source/javatests/com/google/enterprise/connector/pusher/DocPusherTest.java:2530: "<principal scope=\"GROUP\" access=\"PERMIT\">Engineering</principal>", On 2012/01/28 04:42:13, John L wrote: > 80 cols (drag, I know). Done. http://codereview.appspot.com/5577067/diff/1/source/javatests/com/google/ente... source/javatests/com/google/enterprise/connector/pusher/DocPusherTest.java:2533: } catch (Exception e) { On 2012/01/28 04:42:13, John L wrote: > Why are you catching Exception? It's usually much better to let it be thrown to > get a stack trace when debugging failing tests. Done. http://codereview.appspot.com/5577067/diff/1/source/javatests/com/google/ente... source/javatests/com/google/enterprise/connector/pusher/DocPusherTest.java:2536: On 2012/01/28 04:42:13, John L wrote: > Delete extra blank line. Done.
Sign in to reply to this message.
LGTM, with trivial comments. John L http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/pusher/XmlFeed.java (right): http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:508: if(!Strings.isNullOrEmpty(inheritanceType)) { Add a space after "if". http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:555: private static void wrapAclPrinicpal(StringBuffer buff, Property property, "wrapAclPrinicpal" => "wrapAclPrincipal" http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/spi/SpiConstants.java (right): http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SpiConstants.java:266: * Identifies a single valued InheritanceType property. This value is "single valued InheritanceType" => "single-valued String". You should add a note that the value must be the string form of one of the InheritanceType enum constants. http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SpiConstants.java:270: * Value: google:inheritancetype "aclinheritancetype" http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SpiConstants.java:274: public static final String PROPNAME_ACLINHERITANCETYPE = "google:aclinheritancetype"; 80 cols. http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SpiConstants.java:280: * Value: google:inheritfrom "aclinheritfrom" http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SpiConstants.java:407: return (this == other || this == ACL || (this != CONTENT && Break before the last || for a higher-level break. (Breaks should be before operators, and not after them, except assignment where both are allowed but we consistently break after.)
Sign in to reply to this message.
Changes as per review http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/pusher/XmlFeed.java (right): http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:508: if(!Strings.isNullOrEmpty(inheritanceType)) { On 2012/01/31 00:39:51, John L wrote: > Add a space after "if". Done. http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:555: private static void wrapAclPrinicpal(StringBuffer buff, Property property, On 2012/01/31 00:39:51, John L wrote: > "wrapAclPrinicpal" => "wrapAclPrincipal" Done. http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/spi/SpiConstants.java (right): http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SpiConstants.java:266: * Identifies a single valued InheritanceType property. This value is On 2012/01/31 00:39:51, John L wrote: > "single valued InheritanceType" => "single-valued String". You should add a note > that the value must be the string form of one of the InheritanceType enum > constants. Done. http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SpiConstants.java:270: * Value: google:inheritancetype On 2012/01/31 00:39:51, John L wrote: > "aclinheritancetype" Done. http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SpiConstants.java:274: public static final String PROPNAME_ACLINHERITANCETYPE = "google:aclinheritancetype"; On 2012/01/31 00:39:51, John L wrote: > 80 cols. Done. http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SpiConstants.java:280: * Value: google:inheritfrom On 2012/01/31 00:39:51, John L wrote: > "aclinheritfrom" Done. http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/spi/SpiConstants.java:407: return (this == other || this == ACL || (this != CONTENT && On 2012/01/31 00:39:51, John L wrote: > Break before the last || for a higher-level break. (Breaks should be before > operators, and not after them, except assignment where both are allowed but we > consistently break after.) Done.
Sign in to reply to this message.
LGTM, with some minor comments. http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/pusher/XmlFeed.java (right): http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:468: * Constructs the record URL for the given doc id, feed type and search url "search url" -> "search URL." (note . at end - comments should be full sentences.) http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:475: SpiConstants.PROPNAME_DOCID); DOCID is required, so use DocUtils.getRequiredString(...); http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:498: SpiConstants.PROPNAME_DOCID); DOCID is required. (Or is it in a strict ACL doc?) http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:553: * Wrap the ACL principal info as XML data . at end http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:561: if (valString != null && valString.length() > 0) { Use if (!Strings.isNullOrEmpty(valString)) { http://codereview.appspot.com/5577067/diff/6001/source/javatests/com/google/e... File source/javatests/com/google/enterprise/connector/pusher/DocPusherTest.java (right): http://codereview.appspot.com/5577067/diff/6001/source/javatests/com/google/e... source/javatests/com/google/enterprise/connector/pusher/DocPusherTest.java:2517: System.out.println(resultXML); If you are going to print it out, try to make it easier to see in the output. I typically do something like: System.out.println(getName() + " resultXML:\n" + resultXML);
Sign in to reply to this message.
http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/pusher/XmlFeed.java (right): http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:468: * Constructs the record URL for the given doc id, feed type and search url On 2012/02/01 21:13:26, Brett wrote: > "search url" -> "search URL." (note . at end - comments should be full > sentences.) Done. http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:475: SpiConstants.PROPNAME_DOCID); On 2012/02/01 21:13:26, Brett wrote: > DOCID is required, so use DocUtils.getRequiredString(...); Done. http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:498: SpiConstants.PROPNAME_DOCID); On 2012/02/01 21:13:26, Brett wrote: > DOCID is required. (Or is it in a strict ACL doc?) Done. http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:553: * Wrap the ACL principal info as XML data On 2012/02/01 21:13:26, Brett wrote: > . at end Done. http://codereview.appspot.com/5577067/diff/6001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/pusher/XmlFeed.java:561: if (valString != null && valString.length() > 0) { On 2012/02/01 21:13:26, Brett wrote: > Use if (!Strings.isNullOrEmpty(valString)) { Done. http://codereview.appspot.com/5577067/diff/6001/source/javatests/com/google/e... File source/javatests/com/google/enterprise/connector/pusher/DocPusherTest.java (right): http://codereview.appspot.com/5577067/diff/6001/source/javatests/com/google/e... source/javatests/com/google/enterprise/connector/pusher/DocPusherTest.java:2517: System.out.println(resultXML); On 2012/02/01 21:13:26, Brett wrote: > If you are going to print it out, try to make it easier to see in the output. I > typically do something like: > System.out.println(getName() + " resultXML:\n" + resultXML); Done. http://codereview.appspot.com/5577067/diff/6001/source/javatests/com/google/e... source/javatests/com/google/enterprise/connector/pusher/DocPusherTest.java:2517: System.out.println(resultXML); On 2012/02/01 21:13:26, Brett wrote: > If you are going to print it out, try to make it easier to see in the output. I > typically do something like: > System.out.println(getName() + " resultXML:\n" + resultXML); this line is to be removed, was using to look at the ACL snippet
Sign in to reply to this message.
http://codereview.appspot.com/5577067/diff/6001/source/javatests/com/google/e... File source/javatests/com/google/enterprise/connector/pusher/DocPusherTest.java (right): http://codereview.appspot.com/5577067/diff/6001/source/javatests/com/google/e... source/javatests/com/google/enterprise/connector/pusher/DocPusherTest.java:2517: System.out.println(resultXML); In that case, I often tag such things with a DEBUGGING comment: System.out.println(...); // DEBUGGING Then, I almost always remember to grep for them before checking in. It also makes it obvious to reviewers that it should be removed. Note that that is my personal style, not really a google policy. On 2012/02/02 18:27:56, Srinivas wrote: > On 2012/02/01 21:13:26, Brett wrote: > > If you are going to print it out, try to make it easier to see in the output. > I > > typically do something like: > > System.out.println(getName() + " resultXML:\n" + resultXML); > > this line is to be removed, was using to look at the ACL snippet
Sign in to reply to this message.
Committed as revisions 2926 and 2928: http://code.google.com/p/google-enterprise-connector-manager/source/detail?r=... http://code.google.com/p/google-enterprise-connector-manager/source/detail?r=... Srinivas, please close this review and its antecedent. John L
Sign in to reply to this message.
Done. Thanks. On Fri, Feb 3, 2012 at 12:16 AM, <jlacey@google.com> wrote: > Committed as revisions 2926 and 2928: > > http://code.google.com/p/**google-enterprise-connector-** > manager/source/detail?r=2926<http://code.google.com/p/google-enterprise-connector-manager/source/detail?r=2926> > http://code.google.com/p/**google-enterprise-connector-** > manager/source/detail?r=2928<http://code.google.com/p/google-enterprise-connector-manager/source/detail?r=2928> > > Srinivas, please close this review and its antecedent. > > John L > > > http://codereview.appspot.com/**5577067/<http://codereview.appspot.com/5577067/> >
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
