Issue 6203047: SharePoint large ACL Processing (Closed)
|
Created:
14 years, 1 month ago by Tanmay Vartak Modified:
14 years ago CC:
connector-cr_google.com Base URL:
http://google-enterprise-connector-sharepoint.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionSharePoint large ACL Processing
1. SharePoint Group resolution in batches once per web state
2. Processing of large ACLs individually
Patch Set 1 #Patch Set 2 : Patch without pseudo groups for large ACLs #
Total comments: 6
Patch Set 3 : Patch 2 - With javadoc and advanced config properties #
Total comments: 134
Patch Set 4 : Patch with Code Review Comments Implemented #
Total comments: 3
Patch Set 5 : same patch with -x -w #Patch Set 6 : Support for 6.X GSA #Patch Set 7 : support for 6.x GSA with correct version of GssAclWs.java #
Total comments: 14
MessagesTotal messages: 20
On 2012/05/07 20:40:15, tvartak wrote: additional changes required to have configurable batch limits
Sign in to reply to this message.
On 2012/05/16 23:53:53, tvartak wrote: 1. SharePoint Group resolution in batches once per web state 2. Processing of large ACLs individually 3. Users which are assigned direct permissions will not be maintained as pseudo group in local database.
Sign in to reply to this message.
Can you please share with me the high level design changes document (with possible solution approaches, pros and cons) for the large ACL processing. Please create one if not available and also please mention all the use cases that you handled in the code review. It helps us a lot during code reviews. I think when we do some design changes to the connector it would be a good idea to capture all the details in a single documents for the review, so that review would be very easy for everyone. Below are some of the queries. why the group resolution should be once per web state ? How does group resolution can be done if web state has huge number of lists or document libraries with 100k documents ? What is the trigger point for the connector to perform batch processing ? Let me know if I mis-interpret something out here.
Sign in to reply to this message.
http://codereview.appspot.com/6203047/diff/3001/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/sharepoint/client/SharepointClient.java (right): http://codereview.appspot.com/6203047/diff/3001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClient.java:316: private boolean resolveSharePointGroups(WebState webState) { missing java doc ? http://codereview.appspot.com/6203047/diff/3001/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java (right): http://codereview.appspot.com/6203047/diff/3001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1083: result = stub.resolveSPGroupInBatch(groupIds, 5000); y 5000 is hard coded ? http://codereview.appspot.com/6203047/diff/3001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1109: public boolean resolveSharePointGroups(WebState webState) { missing java docs? It could be great if you can add inline comments for all the if & else conditions in this method. http://codereview.appspot.com/6203047/diff/3001/source_net/Acl/GssAcl.asmx File source_net/Acl/GssAcl.asmx (right): http://codereview.appspot.com/6203047/diff/3001/source_net/Acl/GssAcl.asmx#ne... source_net/Acl/GssAcl.asmx:1073: public GssResolveSPGroupResult ResolveSPGroupInBatch(string[] groupId,int batchSize) Just see the possibility to get rid of the nested if-else ladders with in the loops so that we can have better code readability. May be think of patterns that helps us.
Sign in to reply to this message.
I will add comments for new methods http://codereview.appspot.com/6203047/diff/3001/source/java/com/google/enterp... File source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java (right): http://codereview.appspot.com/6203047/diff/3001/source/java/com/google/enterp... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1083: result = stub.resolveSPGroupInBatch(groupIds, 5000); As indicated in initial patch description, this value will be fetched from advanced connector properties. On 2012/05/17 08:15:28, nageswaras wrote: > y 5000 is hard coded ? http://codereview.appspot.com/6203047/diff/3001/source_net/Acl/GssAcl.asmx File source_net/Acl/GssAcl.asmx (right): http://codereview.appspot.com/6203047/diff/3001/source_net/Acl/GssAcl.asmx#ne... source_net/Acl/GssAcl.asmx:1073: public GssResolveSPGroupResult ResolveSPGroupInBatch(string[] groupId,int batchSize) Not sure exactly what is expected here. This is an existing code. Can you please elaborate more? On 2012/05/17 08:15:28, nageswaras wrote: > Just see the possibility to get rid of the nested if-else ladders with in the > loops so that we can have better code readability. May be think of patterns that > helps us.
Sign in to reply to this message.
On 2012/05/21 20:52:53, tvartak wrote: Added 2 advanced configuration properties for SharePoint Connector. 1. largeACLThreshold = Threshold Value to identify ACL as large ACL. 2. groupResolutionBatchSize = groupResolutionBatchSize is used to arrive at an appropriate batch size for SharePoint Group Resolution inorder to avoid large SOAP response and web service timeout error
Sign in to reply to this message.
http://codereview.appspot.com/6203047/diff/10001/config/config/connectorDefau... File config/config/connectorDefaults.xml (right): http://codereview.appspot.com/6203047/diff/10001/config/config/connectorDefau... config/config/connectorDefaults.xml:96: for SharePoint Group Resolution inorder to avoid large SOAP resposnse Spell Check. "response"
Sign in to reply to this message.
Lots and lots of minor and trivial comments. Buried in here is a request to upload a patch set using -- -x -w on the end up the upload.py command, to make a block of reindented code easier to review. Please do that and then look at the other comments. John L http://codereview.appspot.com/6203047/diff/10001/config/config/connectorDefau... File config/config/connectorDefaults.xml (right): http://codereview.appspot.com/6203047/diff/10001/config/config/connectorDefau... config/config/connectorDefaults.xml:77: <property name="fetchACLInBatches"><value>false</value></property> Do we still need fetchACLInBatches and aclBatchSizeFactor? We need to support the setters in SharepointConnector, but we might not need them here or in connectorInstance.xml (especially the latter). http://codereview.appspot.com/6203047/diff/10001/config/config/connectorDefau... config/config/connectorDefaults.xml:93: <property name="largeACLThreshold"><value>5000</value></property> This is very high, isn't it? I thought this was essentially the size causing problems. We had talked about 500? http://codereview.appspot.com/6203047/diff/10001/config/config/connectorDefau... config/config/connectorDefaults.xml:96: for SharePoint Group Resolution inorder to avoid large SOAP resposnse "inorder" => "in order" http://codereview.appspot.com/6203047/diff/10001/config/config/connectorDefau... config/config/connectorDefaults.xml:97: and web service timeout error. groupResolutionBatchSize = -1 will result Why not 0 for both? Or accept values < 1 for both but at least document the same value here for both? http://codereview.appspot.com/6203047/diff/10001/config/config/connectorInsta... File config/config/connectorInstance.xml (right): http://codereview.appspot.com/6203047/diff/10001/config/config/connectorInsta... config/config/connectorInstance.xml:88: for SharePoint Group Resolution inorder to avoid large SOAP resposnse All the same comments and typos apply here. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... File source/java/com/google/enterprise/connector/sharepoint/client/SharepointClient.java (right): http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClient.java:309: // If Group resolution failed then No documents will be sent for WebState. 80 cols, "failed" => "fails" maybe? http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClient.java:310: return resolveSharePointGroups(webState); This return is funky. I would switch the test above (always nice to have positive conditions), and keep this state as-is but return false in the other block, and remove the return below. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClient.java:317: if (webState.getSPGroupsToResolve() == null || webState.getSPGroupsToResolve().isEmpty()) { 80 cols http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClient.java:327: } catch (Exception ex) { Delete extra blank lines around catch. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClient.java:330: LOGGER.log(Level.WARNING, "Problem while resolving groups under WebState [ " 80 cols http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClient.java:331: + webState.getWebUrl() + " ] . ", ex); Delete space before "." http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... File source/java/com/google/enterprise/connector/sharepoint/client/SharepointClientContext.java (right): http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClientContext.java:103: // Batch Size for SP Group Resolution Use doc comments, including periods: /** Batch size for SP group resolution. */ http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClientContext.java:103: // Batch Size for SP Group Resolution Flip the order of these two fields for consistency and logic flow. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClientContext.java:105: // Threshold value to identify large ACLs Ditto http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClientContext.java:1153: * @return the groupResolutionBatchSize "@return the group resolution batch size" http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClientContext.java:1160: * @param groupResolutionBatchSize the groupResolutionBatchSize to set "@param groupResolutionBatchSize the group resolution batch size to set" http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClientContext.java:1167: * @return the largeACLThreshold Same doc comment style, and method ordering. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... File source/java/com/google/enterprise/connector/sharepoint/spiimpl/SharepointConnector.java (right): http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/spiimpl/SharepointConnector.java:93: // Batch Size for SP Group Resolution Same doc comment and declaration ordering issues. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/spiimpl/SharepointConnector.java:524: sharepointClientContext.setGroupResolutionBatchSize(this.groupResolutionBatchSize); 80 cols http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/spiimpl/SharepointConnector.java:536: } Delete added trailing spaces. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/spiimpl/SharepointConnector.java:573: public void setUseSPSearchVisibility(boolean useSPSerachVisibility) { Please fix this typo while you're here: "Serach" => "Search" http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/spiimpl/SharepointConnector.java:1007: * @return the groupResolutionBatchSize Same comments as in SharepointClientContext. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... File source/java/com/google/enterprise/connector/sharepoint/state/WebState.java (right): http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/state/WebState.java:771: * @return the spGroups Same doc comment style comment here and below. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... File source/java/com/google/enterprise/connector/sharepoint/wsclient/client/AclWS.java (right): http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/client/AclWS.java:70: * @return boolean flag indicating status of group resolution This was true if it worked and false if it failed? Please include that here. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... File source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java (right): http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:158: includePolicyAcls,sharepointClientContext.getLargeACLThreshold()); Add a space after comma. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:203: String[] urlToPass = new String[1]; String[] urlToPass = { documentUrl }; That's special initializer syntax. Also, new String[] { documentUrl } is allowed in all contexts (assignments, method arguments, etc.). You could just write ... = getAclForUrls(new String[] { documentUrl }, ...); http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:205: Map<String, SPDocument> docMapToPass = Maps.newHashMap(); Might as well do this inside the if, where we need it. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:241: Map<String, SPDocument> largeACLUrlToDocMap = Maps.newHashMap(); "ACL" => "Acl" in names. I'll try to flag these with just Acl below. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:242: List<String> largeACLUrlsToReprocess = Lists.newArrayList(); Acl http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:261: boolean largeACL = Boolean.parseBoolean(acl.getLargeACL()); Acl http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:264: + " ] needs to be reprocessed as Large ACL Document"); Add space after "+". http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:446: // If sharePoint group does not have any members "sharePoint" => "SharePoint" http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:448: if (webState.getSPGroupsToResolve() == null) { Both of these elements are awkward, initializing the set here and, and then fetching the set to add something to it. What about simply having an addSPGroupToResolve(int) method, instead of the setSPGroupsToResolve method. Then don't use getSPGroupsToResolve here, and let AddSPGroupToResolve create the set in WebState when it's needed? http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1083: result = stub.resolveSPGroupInBatch(groupIds, sharepointClientContext.getGroupResolutionBatchSize()); 80 cols http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1113: * @param webState WebState for which groups needs to be resolved. No trailing periods on @tag fragments, unless there are multiple sentences. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1114: * @return True if group resolution is successful. "." => "," http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1119: if (webState.getSPGroupsToResolve() == null || We can just fetch this set once here. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1129: LOGGER.log(Level.WARNING, LOGGER.warning(..., which saves a line, too, or maybe LOGGER.logp(Level.WARNING, "Group resolution failed for WebState [{0}]", webState.getWebUrl()). http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1130: "Group Resolution null for WebState[ " Add a space before "[" http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1131: + webState.getWebUrl() + " ]. Returning false"); Does the "Returning false" really add anything here? http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1134: GssPrincipal[] groups = result.getPrinicpals(); Can we fix the typo in getPrinicpals, or do we have to live with it for backward compatibility? Seems like this is all new and we can fix it. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1135: if (null != groups && groups.length > 0) { groups != null && ... http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1152: + " memberships in user data store. ", e); Delete space after "." http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1157: !result.getLastGroupResolved().isEmpty()) { This looks like a string, so you can't use isEmpty (requires Java 6) and should use Strings.isNullOrEmpty. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1171: webState.setSPGroupsToResolve(new TreeSet<String>()); I see, you want to reset the collection. Maybe webState.getSPGroupsToResolve().clear()? http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1175: return resolveSharePointGroups(webState); How many levels of recursion could occur here? Seems like a do-while loop might be better. http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx File source_net/Acl/GssAcl.asmx (right): http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:277: private string largeACL; Acl http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:279: public String LargeACL Acl http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:665: Delete extra blank line. http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:827: return GetAclForUrlsUsingInheritance(urls, false, true,0); Add space before "0". http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:838: public GssGetAclForUrlsResult GetAclForUrlsUsingInheritance(string[] urls, Boolean bUseInheritance, Boolean bIncludePolicyAcls, int largeACLThreshold) Acl http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:893: Boolean largeACL = secobj.RoleAssignments.Count > largeACLThreshold && largeACLThreshold > 0; Acl http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:894: if (largeACL && urls.Length > 1) Do you retrieve large ACLs in batches, which would suggest it could be beneficial to postpone the processing even for just one URL with a large ACL, or is it always the whole thing for one URL? http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:899: acl.AddLogMessage(String.Format("Large ACL for URL {0} with {1} role assignments",url,secobj.RoleAssignments.Count)); Add spaces after commas. http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:900: continue; I think it would be better to put the rest of this block in an else than use continue here. It's an overly large method anyway, and continue is harder to reason about. Could you extract some of it to a helper method? http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:1073: public GssResolveSPGroupResult ResolveSPGroupInBatch(string[] groupId,int batchSize) Add space after comma. http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:1080: GssResolveSPGroupResult result = new GssResolveSPGroupResult(); Can you upload a patch set using -- -x -w on the end up the upload.py command to try to make this change easier to read? http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:1083: if (null != groupId) groupId != null http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:1101: if (null == admin) admin == null http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:1114: { spGroup != null, etc. http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:1812: SPUser user = (SPUser)spPrincipal; Add a space after cast. http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:1820: gssPrincipal.Type = GssPrincipal.PrincipalType.USER; Is this not a default value, or do you just want to make it more clear? http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAclwsdl.aspx File source_net/Acl/GssAclwsdl.aspx (right): http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAclwsdl.as... source_net/Acl/GssAclwsdl.aspx:36: <s:element minOccurs="1" maxOccurs="1" name="largeACLThreshold" type="s:int" /> Acl http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAclwsdl.as... source_net/Acl/GssAclwsdl.aspx:94: <s:element minOccurs="0" maxOccurs="1" name="LargeACL" type="s:string" /> Acl http://codereview.appspot.com/6203047/diff/10001/wsdl/GssAcl.wsdl File wsdl/GssAcl.wsdl (right): http://codereview.appspot.com/6203047/diff/10001/wsdl/GssAcl.wsdl#newcode32 wsdl/GssAcl.wsdl:32: <s:element minOccurs="1" maxOccurs="1" name="bIncludePolicyAcls" type="s:boolean" /> Remove tabs. http://codereview.appspot.com/6203047/diff/10001/wsdl/GssAcl.wsdl#newcode33 wsdl/GssAcl.wsdl:33: <s:element minOccurs="1" maxOccurs="1" name="largeACLThreshold" type="s:int" /> Acl http://codereview.appspot.com/6203047/diff/10001/wsdl/GssAcl.wsdl#newcode91 wsdl/GssAcl.wsdl:91: <s:element minOccurs="0" maxOccurs="1" name="LargeACL" type="s:string" /> Acl
Sign in to reply to this message.
More broadly, it would be great to have some kind of tests for this code. Mocks might be hard from the soap package, but worth a look. We also don't really want all this logic in the soap package at all. John L http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... File source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java (right): http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:448: if (webState.getSPGroupsToResolve() == null) { Alternatively, do we have to put this in WebState? Can we just pass this set around, or create a separate wrapper object for it? On 2012/05/24 01:41:40, John L wrote: > Both of these elements are awkward, initializing the set here and, and then > fetching the set to add something to it. What about simply having an > addSPGroupToResolve(int) method, instead of the setSPGroupsToResolve method. > Then don't use getSPGroupsToResolve here, and let AddSPGroupToResolve create the > set in WebState when it's needed?
Sign in to reply to this message.
Trivial comment about "prinicpal" typos and backward compatibility. John L http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx File source_net/Acl/GssAcl.asmx (right): http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:652: private List<GssPrincipal> prinicpals; Which of these typos can we fix without breaking backward compatibility?
Sign in to reply to this message.
As discussed uploaded 2 patch with and without -w-x Added my comments where applicable http://codereview.appspot.com/6203047/diff/10001/config/config/connectorDefau... File config/config/connectorDefaults.xml (right): http://codereview.appspot.com/6203047/diff/10001/config/config/connectorDefau... config/config/connectorDefaults.xml:77: <property name="fetchACLInBatches"><value>false</value></property> I guess we can revisit this after 7.0 release. On 2012/05/24 01:41:40, John L wrote: > Do we still need fetchACLInBatches and aclBatchSizeFactor? We need to support > the setters in SharepointConnector, but we might not need them here or in > connectorInstance.xml (especially the latter). http://codereview.appspot.com/6203047/diff/10001/config/config/connectorDefau... config/config/connectorDefaults.xml:93: <property name="largeACLThreshold"><value>5000</value></property> large ACL threshold default value set to 500. On 2012/05/24 01:41:40, John L wrote: > This is very high, isn't it? I thought this was essentially the size causing > problems. We had talked about 500? http://codereview.appspot.com/6203047/diff/10001/config/config/connectorDefau... config/config/connectorDefaults.xml:96: for SharePoint Group Resolution inorder to avoid large SOAP resposnse Done. On 2012/05/21 21:00:32, tvartak wrote: > Spell Check. "response" http://codereview.appspot.com/6203047/diff/10001/config/config/connectorDefau... config/config/connectorDefaults.xml:97: and web service timeout error. groupResolutionBatchSize = -1 will result Done. On 2012/05/24 01:41:40, John L wrote: > Why not 0 for both? Or accept values < 1 for both but at least document the same > value here for both? http://codereview.appspot.com/6203047/diff/10001/config/config/connectorInsta... File config/config/connectorInstance.xml (right): http://codereview.appspot.com/6203047/diff/10001/config/config/connectorInsta... config/config/connectorInstance.xml:88: for SharePoint Group Resolution inorder to avoid large SOAP resposnse On 2012/05/24 01:41:40, John L wrote: > All the same comments and typos apply here. Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... File source/java/com/google/enterprise/connector/sharepoint/client/SharepointClient.java (right): http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClient.java:309: // If Group resolution failed then No documents will be sent for WebState. On 2012/05/24 01:41:40, John L wrote: > 80 cols, "failed" => "fails" maybe? Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClient.java:310: return resolveSharePointGroups(webState); On 2012/05/24 01:41:40, John L wrote: > This return is funky. I would switch the test above (always nice to have > positive conditions), and keep this state as-is but return false in the other > block, and remove the return below. Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClient.java:317: if (webState.getSPGroupsToResolve() == null || webState.getSPGroupsToResolve().isEmpty()) { On 2012/05/24 01:41:40, John L wrote: > 80 cols Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClient.java:327: } catch (Exception ex) { On 2012/05/24 01:41:40, John L wrote: > Delete extra blank lines around catch. Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClient.java:330: LOGGER.log(Level.WARNING, "Problem while resolving groups under WebState [ " On 2012/05/24 01:41:40, John L wrote: > 80 cols Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClient.java:331: + webState.getWebUrl() + " ] . ", ex); On 2012/05/24 01:41:40, John L wrote: > Delete space before "." Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... File source/java/com/google/enterprise/connector/sharepoint/client/SharepointClientContext.java (right): http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClientContext.java:103: // Batch Size for SP Group Resolution On 2012/05/24 01:41:40, John L wrote: > Use doc comments, including periods: /** Batch size for SP group resolution. */ Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClientContext.java:103: // Batch Size for SP Group Resolution On 2012/05/24 01:41:40, John L wrote: > Flip the order of these two fields for consistency and logic flow. Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClientContext.java:103: // Batch Size for SP Group Resolution On 2012/05/24 01:41:40, John L wrote: > Use doc comments, including periods: /** Batch size for SP group resolution. */ Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClientContext.java:105: // Threshold value to identify large ACLs On 2012/05/24 01:41:40, John L wrote: > Ditto Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClientContext.java:1153: * @return the groupResolutionBatchSize On 2012/05/24 01:41:40, John L wrote: > "@return the group resolution batch size" Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClientContext.java:1160: * @param groupResolutionBatchSize the groupResolutionBatchSize to set On 2012/05/24 01:41:40, John L wrote: > "@param groupResolutionBatchSize the group resolution batch size to set" Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClientContext.java:1167: * @return the largeACLThreshold On 2012/05/24 01:41:40, John L wrote: > Same doc comment style, and method ordering. Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... File source/java/com/google/enterprise/connector/sharepoint/spiimpl/SharepointConnector.java (right): http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/spiimpl/SharepointConnector.java:93: // Batch Size for SP Group Resolution On 2012/05/24 01:41:40, John L wrote: > Same doc comment and declaration ordering issues. Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/spiimpl/SharepointConnector.java:524: sharepointClientContext.setGroupResolutionBatchSize(this.groupResolutionBatchSize); On 2012/05/24 01:41:40, John L wrote: > 80 cols Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/spiimpl/SharepointConnector.java:536: } On 2012/05/24 01:41:40, John L wrote: > Delete added trailing spaces. Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/spiimpl/SharepointConnector.java:573: public void setUseSPSearchVisibility(boolean useSPSerachVisibility) { On 2012/05/24 01:41:40, John L wrote: > Please fix this typo while you're here: "Serach" => "Search" Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/spiimpl/SharepointConnector.java:1007: * @return the groupResolutionBatchSize On 2012/05/24 01:41:40, John L wrote: > Same comments as in SharepointClientContext. Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... File source/java/com/google/enterprise/connector/sharepoint/state/WebState.java (right): http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/state/WebState.java:771: * @return the spGroups On 2012/05/24 01:41:40, John L wrote: > Same doc comment style comment here and below. Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... File source/java/com/google/enterprise/connector/sharepoint/wsclient/client/AclWS.java (right): http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/client/AclWS.java:70: * @return boolean flag indicating status of group resolution On 2012/05/24 01:41:40, John L wrote: > This was true if it worked and false if it failed? Please include that here. Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... File source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java (right): http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:158: includePolicyAcls,sharepointClientContext.getLargeACLThreshold()); On 2012/05/24 01:41:40, John L wrote: > Add a space after comma. Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:203: String[] urlToPass = new String[1]; On 2012/05/24 01:41:40, John L wrote: > String[] urlToPass = { documentUrl }; > > That's special initializer syntax. Also, new String[] { documentUrl } is allowed > in all contexts (assignments, method arguments, etc.). You could just write > > ... = getAclForUrls(new String[] { documentUrl }, ...); Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:205: Map<String, SPDocument> docMapToPass = Maps.newHashMap(); On 2012/05/24 01:41:40, John L wrote: > Might as well do this inside the if, where we need it. Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:241: Map<String, SPDocument> largeACLUrlToDocMap = Maps.newHashMap(); On 2012/05/24 01:41:40, John L wrote: > "ACL" => "Acl" in names. I'll try to flag these with just Acl below. Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:242: List<String> largeACLUrlsToReprocess = Lists.newArrayList(); On 2012/05/24 01:41:40, John L wrote: > Acl Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:261: boolean largeACL = Boolean.parseBoolean(acl.getLargeACL()); On 2012/05/24 01:41:40, John L wrote: > Acl Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:264: + " ] needs to be reprocessed as Large ACL Document"); On 2012/05/24 01:41:40, John L wrote: > Add space after "+". Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:448: if (webState.getSPGroupsToResolve() == null) { On 2012/05/24 01:41:40, John L wrote: > Both of these elements are awkward, initializing the set here and, and then > fetching the set to add something to it. What about simply having an > addSPGroupToResolve(int) method, instead of the setSPGroupsToResolve method. > Then don't use getSPGroupsToResolve here, and let AddSPGroupToResolve create the > set in WebState when it's needed? Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:448: if (webState.getSPGroupsToResolve() == null) { Added method to WebState On 2012/05/24 01:41:40, John L wrote: > Both of these elements are awkward, initializing the set here and, and then > fetching the set to add something to it. What about simply having an > addSPGroupToResolve(int) method, instead of the setSPGroupsToResolve method. > Then don't use getSPGroupsToResolve here, and let AddSPGroupToResolve create the > set in WebState when it's needed? http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1083: result = stub.resolveSPGroupInBatch(groupIds, sharepointClientContext.getGroupResolutionBatchSize()); On 2012/05/24 01:41:40, John L wrote: > 80 cols Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1113: * @param webState WebState for which groups needs to be resolved. On 2012/05/24 01:41:40, John L wrote: > No trailing periods on @tag fragments, unless there are multiple sentences. Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1114: * @return True if group resolution is successful. On 2012/05/24 01:41:40, John L wrote: > "." => "," Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1119: if (webState.getSPGroupsToResolve() == null || On 2012/05/24 01:41:40, John L wrote: > We can just fetch this set once here. Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1130: "Group Resolution null for WebState[ " On 2012/05/24 01:41:40, John L wrote: > Add a space before "[" Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1135: if (null != groups && groups.length > 0) { On 2012/05/24 01:41:40, John L wrote: > groups != null && ... Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1152: + " memberships in user data store. ", e); On 2012/05/24 01:41:40, John L wrote: > Delete space after "." Done. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1157: !result.getLastGroupResolved().isEmpty()) { No longer used On 2012/05/24 01:41:40, John L wrote: > This looks like a string, so you can't use isEmpty (requires Java 6) and should > use Strings.isNullOrEmpty. http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1171: webState.setSPGroupsToResolve(new TreeSet<String>()); NA On 2012/05/24 01:41:40, John L wrote: > I see, you want to reset the collection. Maybe > webState.getSPGroupsToResolve().clear()? http://codereview.appspot.com/6203047/diff/10001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:1175: return resolveSharePointGroups(webState); On 2012/05/24 01:41:40, John L wrote: > How many levels of recursion could occur here? Seems like a do-while loop might > be better. Done. http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx File source_net/Acl/GssAcl.asmx (right): http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:277: private string largeACL; On 2012/05/24 01:41:40, John L wrote: > Acl Done. http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:279: public String LargeACL On 2012/05/24 01:41:40, John L wrote: > Acl Done. http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:665: On 2012/05/24 01:41:40, John L wrote: > Delete extra blank line. Done. http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:827: return GetAclForUrlsUsingInheritance(urls, false, true,0); On 2012/05/24 01:41:40, John L wrote: > Add space before "0". Done. http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:838: public GssGetAclForUrlsResult GetAclForUrlsUsingInheritance(string[] urls, Boolean bUseInheritance, Boolean bIncludePolicyAcls, int largeACLThreshold) On 2012/05/24 01:41:40, John L wrote: > Acl Done. http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:893: Boolean largeACL = secobj.RoleAssignments.Count > largeACLThreshold && largeACLThreshold > 0; On 2012/05/24 01:41:40, John L wrote: > Acl Done. http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:894: if (largeACL && urls.Length > 1) Always fetching full ACL for document. On 2012/05/24 01:41:40, John L wrote: > Do you retrieve large ACLs in batches, which would suggest it could be > beneficial to postpone the processing even for just one URL with a large ACL, or > is it always the whole thing for one URL? http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:899: acl.AddLogMessage(String.Format("Large ACL for URL {0} with {1} role assignments",url,secobj.RoleAssignments.Count)); On 2012/05/24 01:41:40, John L wrote: > Add spaces after commas. Done. http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:900: continue; On 2012/05/24 01:41:40, John L wrote: > I think it would be better to put the rest of this block in an else than use > continue here. It's an overly large method anyway, and continue is harder to > reason about. Could you extract some of it to a helper method? Done. http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:1073: public GssResolveSPGroupResult ResolveSPGroupInBatch(string[] groupId,int batchSize) On 2012/05/24 01:41:40, John L wrote: > Add space after comma. Done. http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:1083: if (null != groupId) On 2012/05/24 01:41:40, John L wrote: > groupId != null Done. http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:1083: if (null != groupId) This block was existing code, just moved to overridden method. On 2012/05/24 01:41:40, John L wrote: > groupId != null http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:1101: if (null == admin) On 2012/05/24 01:41:40, John L wrote: > admin == null Done. http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:1114: { On 2012/05/24 01:41:40, John L wrote: > spGroup != null, etc. Done. http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:1812: SPUser user = (SPUser)spPrincipal; On 2012/05/24 01:41:40, John L wrote: > Add a space after cast. Done. http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:1820: gssPrincipal.Type = GssPrincipal.PrincipalType.USER; Just to make it clear. Constructor for GssPrincipal is not explicitly setting any value for Type. On 2012/05/24 01:41:40, John L wrote: > Is this not a default value, or do you just want to make it more clear? http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAclwsdl.aspx File source_net/Acl/GssAclwsdl.aspx (right): http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAclwsdl.as... source_net/Acl/GssAclwsdl.aspx:36: <s:element minOccurs="1" maxOccurs="1" name="largeACLThreshold" type="s:int" /> On 2012/05/24 01:41:40, John L wrote: > Acl Done. http://codereview.appspot.com/6203047/diff/10001/source_net/Acl/GssAclwsdl.as... source_net/Acl/GssAclwsdl.aspx:94: <s:element minOccurs="0" maxOccurs="1" name="LargeACL" type="s:string" /> On 2012/05/24 01:41:40, John L wrote: > Acl Done. http://codereview.appspot.com/6203047/diff/10001/wsdl/GssAcl.wsdl File wsdl/GssAcl.wsdl (right): http://codereview.appspot.com/6203047/diff/10001/wsdl/GssAcl.wsdl#newcode33 wsdl/GssAcl.wsdl:33: <s:element minOccurs="1" maxOccurs="1" name="largeACLThreshold" type="s:int" /> On 2012/05/24 01:41:40, John L wrote: > Acl Done. http://codereview.appspot.com/6203047/diff/10001/wsdl/GssAcl.wsdl#newcode91 wsdl/GssAcl.wsdl:91: <s:element minOccurs="0" maxOccurs="1" name="LargeACL" type="s:string" /> On 2012/05/24 01:41:40, John L wrote: > Acl Done.
Sign in to reply to this message.
LGTM, with minor comments. John L http://codereview.appspot.com/6203047/diff/22001/source/java/com/google/enter... File source/java/com/google/enterprise/connector/sharepoint/client/ListsUtil.java (right): http://codereview.appspot.com/6203047/diff/22001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/ListsUtil.java:339: continue; This is probably just leakage while testing things, but I'd rather put this in another CL. http://codereview.appspot.com/6203047/diff/22001/source/java/com/google/enter... File source/java/com/google/enterprise/connector/sharepoint/client/SharepointClient.java (right): http://codereview.appspot.com/6203047/diff/22001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/client/SharepointClient.java:306: boolean spGroupResolutionResult = resolveSharePointGroups(webState); I generally prefer "return resolveSharePointGroups(webState);". Were you doing this for debugging or some other reason? http://codereview.appspot.com/6203047/diff/22001/wsdl/GssAcl.wsdl File wsdl/GssAcl.wsdl (right): http://codereview.appspot.com/6203047/diff/22001/wsdl/GssAcl.wsdl#newcode32 wsdl/GssAcl.wsdl:32: <s:element minOccurs="1" maxOccurs="1" name="bIncludePolicyAcls" type="s:boolean" /> This line is still indented with tabs, I think.
Sign in to reply to this message.
Patch uploaded with large ACL support for GSA 6.X On 2012/05/26 02:47:48, John L wrote: > LGTM, with minor comments. > > John L > > http://codereview.appspot.com/6203047/diff/22001/source/java/com/google/enter... > File > source/java/com/google/enterprise/connector/sharepoint/client/ListsUtil.java > (right): > > http://codereview.appspot.com/6203047/diff/22001/source/java/com/google/enter... > source/java/com/google/enterprise/connector/sharepoint/client/ListsUtil.java:339: > continue; > This is probably just leakage while testing things, but I'd rather put this in > another CL. > > http://codereview.appspot.com/6203047/diff/22001/source/java/com/google/enter... > File > source/java/com/google/enterprise/connector/sharepoint/client/SharepointClient.java > (right): > > http://codereview.appspot.com/6203047/diff/22001/source/java/com/google/enter... > source/java/com/google/enterprise/connector/sharepoint/client/SharepointClient.java:306: > boolean spGroupResolutionResult = resolveSharePointGroups(webState); > I generally prefer "return resolveSharePointGroups(webState);". Were you doing > this for debugging or some other reason? > > http://codereview.appspot.com/6203047/diff/22001/wsdl/GssAcl.wsdl > File wsdl/GssAcl.wsdl (right): > > http://codereview.appspot.com/6203047/diff/22001/wsdl/GssAcl.wsdl#newcode32 > wsdl/GssAcl.wsdl:32: <s:element minOccurs="1" maxOccurs="1" > name="bIncludePolicyAcls" type="s:boolean" /> > This line is still indented with tabs, I think.
Sign in to reply to this message.
LGTM, with minor comments. John L http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... File source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java (right): http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:243: Map<String,List<SPDocument>> parentUrlToDocMapToReProcess = Add a space after comma. Maybe a shorter name, like "reprocessDocs"? Certainly "re" is not a word by itself. http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:246: ACL: for (GssAcl acl : allAcls) { Please restore the outdent on the label. I can't find anything explicit, and would have guess two spaces, but for some reason the Google emacs style uses 3 spaces, which is what was done here. http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:271: if (parentUrl != null && !parentUrl.isEmpty()) { !Strings.isNullOrEmpty(parentUrl) That's better generally, and String.isEmpty is not allowed anyway (requires Java 6). http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:273: + " ] is idenified as LargeAcl but with inherit permissions," "idenified as LargeAcl" => "identified as a large ACL" Add a space after the comma in the string: permissions," => permissions, " http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:274: + "with Parent Url as " + parentUrl); "Url" => "URL" http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:275: List<SPDocument> childList = Using a multimap (e.g., com.google.common.collect.HashMultimap) would be more natural, so you could just say ...put(parentUrl, document) here. http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:409: List<SPDocument> childList = parentUrlToDocMapToReProcess.get(parentUrl); Note that Multimap behaves similarly here, except that get returns a Collection, and never returns null. http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:410: if (childList != null) { As coded, you never have childList == null here, either, so this can be deleted either way. http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:412: copyAcls(parent,child); Add space after comma. http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:446: * Utility method to copy ACLs Add a period. More interesting, please note that this makes a shallow copy. http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:457: Delete the two extra blank lines. http://codereview.appspot.com/6203047/diff/33001/source_net/Acl/GssAcl.asmx File source_net/Acl/GssAcl.asmx (right): http://codereview.appspot.com/6203047/diff/33001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:895: Delete extra blank line. http://codereview.appspot.com/6203047/diff/33001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:1554: using (SPWeb oWeb = (SPWeb)spObject) Add spaces after casts here and below. http://codereview.appspot.com/6203047/diff/33001/source_net/Acl/GssAcl.asmx#n... source_net/Acl/GssAcl.asmx:1571: Delete two extra blank lines.
Sign in to reply to this message.
Code Commit http://code.google.com/p/google-enterprise-connector-sharepoint/source/detail... On 2012/06/04 21:02:23, John L wrote: > LGTM, with minor comments. > > John L > > http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... > File > source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java > (right): > > http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... > source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:243: > Map<String,List<SPDocument>> parentUrlToDocMapToReProcess = > Add a space after comma. Maybe a shorter name, like "reprocessDocs"? Certainly > "re" is not a word by itself. > > http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... > source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:246: > ACL: for (GssAcl acl : allAcls) { > Please restore the outdent on the label. I can't find anything explicit, and > would have guess two spaces, but for some reason the Google emacs style uses 3 > spaces, which is what was done here. > > http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... > source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:271: > if (parentUrl != null && !parentUrl.isEmpty()) { > !Strings.isNullOrEmpty(parentUrl) > > That's better generally, and String.isEmpty is not allowed anyway (requires Java > 6). > > http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... > source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:273: > + " ] is idenified as LargeAcl but with inherit permissions," > "idenified as LargeAcl" => "identified as a large ACL" > Add a space after the comma in the string: permissions," => permissions, " > > http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... > source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:274: > + "with Parent Url as " + parentUrl); > "Url" => "URL" > > http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... > source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:275: > List<SPDocument> childList = > Using a multimap (e.g., com.google.common.collect.HashMultimap) would be more > natural, so you could just say ...put(parentUrl, document) here. > > http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... > source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:409: > List<SPDocument> childList = parentUrlToDocMapToReProcess.get(parentUrl); > Note that Multimap behaves similarly here, except that get returns a Collection, > and never returns null. > > http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... > source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:410: > if (childList != null) { > As coded, you never have childList == null here, either, so this can be deleted > either way. > > http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... > source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:412: > copyAcls(parent,child); > Add space after comma. > > http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... > source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:446: > * Utility method to copy ACLs > Add a period. More interesting, please note that this makes a shallow copy. > > http://codereview.appspot.com/6203047/diff/33001/source/java/com/google/enter... > source/java/com/google/enterprise/connector/sharepoint/wsclient/soap/GSAclWS.java:457: > > Delete the two extra blank lines. > > http://codereview.appspot.com/6203047/diff/33001/source_net/Acl/GssAcl.asmx > File source_net/Acl/GssAcl.asmx (right): > > http://codereview.appspot.com/6203047/diff/33001/source_net/Acl/GssAcl.asmx#n... > source_net/Acl/GssAcl.asmx:895: > Delete extra blank line. > > http://codereview.appspot.com/6203047/diff/33001/source_net/Acl/GssAcl.asmx#n... > source_net/Acl/GssAcl.asmx:1554: using (SPWeb oWeb = (SPWeb)spObject) > Add spaces after casts here and below. > > http://codereview.appspot.com/6203047/diff/33001/source_net/Acl/GssAcl.asmx#n... > source_net/Acl/GssAcl.asmx:1571: > Delete two extra blank lines.
Sign in to reply to this message.
|
