Issue 70090044: Use List root folder named resource for List ACL instead of List default view URL (Closed)
|
Created:
10 years, 1 month ago by Tanmay Vartak Modified:
10 years, 1 month ago CC:
connector-cr_google.com Visibility:
Public. |
DescriptionUse List root folder named resource for List ACL instead of List default view URL.
SharePoint users \ admin can change list default view url easily via SharePoint UI as per business needs. If default view URL is changed for List, ACL inheritance will be broken for child documents until GSA recrawls child items.
List root folder can not be modified via UI and it remains same once list is created. with this CL, adaptor will send named resource for List Root folder. All child items will inherit permissions from this named resource. Also List home page (list default view url) will inherit its permission from named resource.
Patch Set 1 #
Total comments: 17
Patch Set 2 : with code review comments implemented #
Total comments: 1
MessagesTotal messages: 10
https://codereview.appspot.com/70090044/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java (left): https://codereview.appspot.com/70090044/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java:1288: .put(1, new UserPrincipal("SOMEHOST\\administrator", DEFAULT_NAMESPACE)) this was unused code (note missing @Test) which I replaced with new test "testGetDocContentListItemWithListAsParent"
Sign in to reply to this message.
https://codereview.appspot.com/70090044/diff/1/src/com/google/enterprise/adap... File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java (right): https://codereview.appspot.com/70090044/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1468: final Map<DocId, Acl> map = new TreeMap<DocId, Acl>(); You can use Collections.singletonMap() if you would like. https://codereview.appspot.com/70090044/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1894: DocId parentDocId = parentIsList ? rootFolderDocId : folderDocId; If parentIsList == true, then rootFolderDocId == folderDocId. So this logic isn't necessary any more. It would be good to keep some of the comments around in some capacity and explain what is going on, but it seems parentDocId is always folderDocId at this point. https://codereview.appspot.com/70090044/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java (right): https://codereview.appspot.com/70090044/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java:803: new DocId("http://localhost:1/sites/SiteCollection/Lists/Custom List")) This hsould be indented a bit more since it is a continuation of the previous line. https://codereview.appspot.com/70090044/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java:811: .setEverythingCaseInsensitive() These should be indented further as well. https://codereview.appspot.com/70090044/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java:1149: final String golden I don't think we need to check the contents and metadata. Those are handled by other tests and are just noise here.
Sign in to reply to this message.
On 2014/03/06 18:43:00, ejona wrote: > https://codereview.appspot.com/70090044/diff/1/src/com/google/enterprise/adap... > File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java > (right): > > https://codereview.appspot.com/70090044/diff/1/src/com/google/enterprise/adap... > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1468: final > Map<DocId, Acl> map = new TreeMap<DocId, Acl>(); > You can use Collections.singletonMap() if you would like. > > https://codereview.appspot.com/70090044/diff/1/src/com/google/enterprise/adap... > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1894: DocId > parentDocId = parentIsList ? rootFolderDocId : folderDocId; > If parentIsList == true, then rootFolderDocId == folderDocId. So this logic > isn't necessary any more. It would be good to keep some of the comments around > in some capacity and explain what is going on, but it seems parentDocId is > always folderDocId at this point. > > https://codereview.appspot.com/70090044/diff/1/test/com/google/enterprise/ada... > File test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java > (right): > > https://codereview.appspot.com/70090044/diff/1/test/com/google/enterprise/ada... > test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java:803: > new DocId("http://localhost:1/sites/SiteCollection/Lists/Custom List")) > This hsould be indented a bit more since it is a continuation of the previous > line. > > https://codereview.appspot.com/70090044/diff/1/test/com/google/enterprise/ada... > test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java:811: > .setEverythingCaseInsensitive() > These should be indented further as well. > > https://codereview.appspot.com/70090044/diff/1/test/com/google/enterprise/ada... > test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java:1149: > final String golden > I don't think we need to check the contents and metadata. Those are handled by > other tests and are just noise here. Ping for PJ. I will work on review comments.
Sign in to reply to this message.
Thank you. https://codereview.appspot.com/70090044/diff/1/src/com/google/enterprise/adap... File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java (right): https://codereview.appspot.com/70090044/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1454: = encodeDocId(l.getMetadata().getRootFolder()); metadata could be null? https://codereview.appspot.com/70090044/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1465: response.setAcl(new Acl.Builder().setInheritFrom(rootFolderDocId) no need for fragment? https://codereview.appspot.com/70090044/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1480: }); I think it makes sense to sit on this CL until Miguel has finished making async available. I am thinking I may well be wrong about that.
Sign in to reply to this message.
https://codereview.appspot.com/70090044/diff/1/src/com/google/enterprise/adap... File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java (right): https://codereview.appspot.com/70090044/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1454: = encodeDocId(l.getMetadata().getRootFolder()); On 2014/03/07 22:17:43, pjo wrote: > metadata could be null? No. This metadata is guaranteed. We are hosed if we don't have it. https://codereview.appspot.com/70090044/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1465: response.setAcl(new Acl.Builder().setInheritFrom(rootFolderDocId) On 2014/03/07 22:17:43, pjo wrote: > no need for fragment? No. We actually are specifying a named resource that has no fragment as it is a URL we can predict. The current DocId for the List can change easily over time, so we don't want to use a fragment on the current document. https://codereview.appspot.com/70090044/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1480: }); On 2014/03/07 22:17:43, pjo wrote: > I think it makes sense to sit on this CL > until Miguel has finished making async > available. I am thinking I may well be > wrong about that. The plan was to submit it as-is, and then do a super-easy followup CL once that other piece goes in.
Sign in to reply to this message.
For this one-at-a-time version, do we know what the performance is and are we happy with it? Thank you, PJ - technology's compounding interest - On Fri, Mar 7, 2014 at 2:26 PM, <ejona@google.com> wrote: > > https://codereview.appspot.com/70090044/diff/1/src/com/ > google/enterprise/adaptor/sharepoint/SharePointAdaptor.java > File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java > (right): > > https://codereview.appspot.com/70090044/diff/1/src/com/ > google/enterprise/adaptor/sharepoint/SharePointAdaptor.java#newcode1454 > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1454: > = encodeDocId(l.getMetadata().getRootFolder()); > On 2014/03/07 22:17:43, pjo wrote: > >> metadata could be null? >> > > No. This metadata is guaranteed. We are hosed if we don't have it. > > > https://codereview.appspot.com/70090044/diff/1/src/com/ > google/enterprise/adaptor/sharepoint/SharePointAdaptor.java#newcode1465 > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1465: > response.setAcl(new Acl.Builder().setInheritFrom(rootFolderDocId) > On 2014/03/07 22:17:43, pjo wrote: > >> no need for fragment? >> > > No. We actually are specifying a named resource that has no fragment as > it is a URL we can predict. The current DocId for the List can change > easily over time, so we don't want to use a fragment on the current > document. > > > https://codereview.appspot.com/70090044/diff/1/src/com/ > google/enterprise/adaptor/sharepoint/SharePointAdaptor.java#newcode1480 > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1480: > }); > On 2014/03/07 22:17:43, pjo wrote: > >> I think it makes sense to sit on this CL >> until Miguel has finished making async >> available. I am thinking I may well be >> wrong about that. >> > > The plan was to submit it as-is, and then do a super-easy followup CL > once that other piece goes in. > > https://codereview.appspot.com/70090044/ >
Sign in to reply to this message.
https://codereview.appspot.com/70090044/diff/1/src/com/google/enterprise/adap... File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java (right): https://codereview.appspot.com/70090044/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1468: final Map<DocId, Acl> map = new TreeMap<DocId, Acl>(); On 2014/03/06 18:43:00, ejona wrote: > You can use Collections.singletonMap() if you would like. Done. https://codereview.appspot.com/70090044/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1894: DocId parentDocId = parentIsList ? rootFolderDocId : folderDocId; On 2014/03/06 18:43:00, ejona wrote: > If parentIsList == true, then rootFolderDocId == folderDocId. So this logic > isn't necessary any more. It would be good to keep some of the comments around > in some capacity and explain what is going on, but it seems parentDocId is > always folderDocId at this point. Done. https://codereview.appspot.com/70090044/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java (right): https://codereview.appspot.com/70090044/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java:803: new DocId("http://localhost:1/sites/SiteCollection/Lists/Custom List")) On 2014/03/06 18:43:00, ejona wrote: > This hsould be indented a bit more since it is a continuation of the previous > line. Done. https://codereview.appspot.com/70090044/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java:811: .setEverythingCaseInsensitive() On 2014/03/06 18:43:00, ejona wrote: > These should be indented further as well. Done. https://codereview.appspot.com/70090044/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java:1149: final String golden On 2014/03/06 18:43:00, ejona wrote: > I don't think we need to check the contents and metadata. Those are handled by > other tests and are just noise here. Done.
Sign in to reply to this message.
LGTM. Thank you. https://codereview.appspot.com/70090044/diff/20001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java (right): https://codereview.appspot.com/70090044/diff/20001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1892: // If parent is a folder,item will inherit its permissions from parent s/,/s /
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
