Issue 294620043: Correctly handle exceptions from siteusermapping
DescriptionCorrectly handle exceptions from siteusermapping
b/28948870 - sometimes siteusermapping can return errors, in such cases log the exception, but do not stop processing to prevent broken ACL inheritance chains.
Patch Set 1 #
Total comments: 13
Patch Set 2 : Correctly handle exceptions from siteusermapping #Patch Set 3 : #Patch Set 4 : Changing log level to fine, msg to less technical #
Total comments: 2
Patch Set 5 : Reverting set principal #
Total comments: 1
MessagesTotal messages: 16
Comments and questions. John L https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java (right): https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2522: } catch (IOException ex) { What does this IOException look like? The bug only specifies the IIS log message. https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2523: log.log(Level.INFO, "getSiteUserMapping failed with SOAP " Is INFO consistent with other skipped users? It seems like FINE is more appropriate. https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2523: log.log(Level.INFO, "getSiteUserMapping failed with SOAP " This message is overly technical, verbose, and redundant (since the SOAP exception itself is also logged). Something like "Site user mapping failed for " + id seems like enough. https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2529: // Try to refresh member id mapping and check again. Does the exception failure about mean that this retry is bound to fail also? Or does this specific exception mean that, but other SOAP or IOExceptions might not? https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2540: log.log(Level.INFO, "refreshSiteUserMapping failed with SOAP " Same message issues here.
Sign in to reply to this message.
Not a whole lot beyond JohnL's review comments - but then the change is rather a small one. :-) https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java (right): https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2523: log.log(Level.INFO, "getSiteUserMapping failed with SOAP " On 2016/05/25 19:43:16, JohnL wrote: > Is INFO consistent with other skipped users? It seems like FINE is more > appropriate. Acknowledged. https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2540: log.log(Level.INFO, "refreshSiteUserMapping failed with SOAP " On 2016/05/25 19:43:16, JohnL wrote: > Same message issues here. Acknowledged. https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2543: siteUserMappingRefreshed = true; Should this go into the try block (after the "principal =" line), so that it's only set to true if the two statements you put into the try block don't throw an IOException? (I guess this is similar to JohnL's question about "is retry bound to fail also?")
Sign in to reply to this message.
Please review. Thank you! https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java (right): https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2522: } catch (IOException ex) { On 2016/05/25 19:43:16, JohnL wrote: > What does this IOException look like? The bug only specifies the IIS log > message. I updated b/28948870 with the IOException and attached the log. https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2523: log.log(Level.INFO, "getSiteUserMapping failed with SOAP " On 2016/05/25 21:54:52, myk wrote: > On 2016/05/25 19:43:16, JohnL wrote: > > Is INFO consistent with other skipped users? It seems like FINE is more > > appropriate. > > Acknowledged. Done. https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2529: // Try to refresh member id mapping and check again. On 2016/05/25 19:43:16, JohnL wrote: > Does the exception failure about mean that this retry is bound to fail also? Or > does this specific exception mean that, but other SOAP or IOExceptions might > not? We assume SharePoint itself skips the SID it could not convert to string and returns a SOAP exception instead and we act the same way. Yes, the retry will fail too. Starting at line 1902 getSiteUserMapping() will either way throw IOException. If this is not what you meant please explain what you meant. https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2540: log.log(Level.INFO, "refreshSiteUserMapping failed with SOAP " On 2016/05/25 21:54:52, myk wrote: > On 2016/05/25 19:43:16, JohnL wrote: > > Same message issues here. > > Acknowledged. Done. https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2543: siteUserMappingRefreshed = true; On 2016/05/25 21:54:52, myk wrote: > Should this go into the try block (after the "principal =" line), so that it's > only set to true if the two statements you put into the try block don't throw an > IOException? > > (I guess this is similar to JohnL's question about "is retry bound to fail > also?") Actually it does not make sense to retry, I think it's OK here, we are ignoring the error like SharePoint.
Sign in to reply to this message.
Friendly ping on this. Thanks! On Thu, Jun 9, 2016 at 4:09 PM, <hpaku@google.com> wrote: > Please review. > > Thank you! > > > > https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... > File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java > (right): > > > https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2522: > } catch (IOException ex) { > On 2016/05/25 19:43:16, JohnL wrote: > >> What does this IOException look like? The bug only specifies the IIS >> > log > >> message. >> > > I updated b/28948870 <https://buganizer.corp.google.com/28948870> with > the IOException and attached the log. > > > https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2523: > log.log(Level.INFO, "getSiteUserMapping failed with SOAP " > On 2016/05/25 21:54:52, myk wrote: > >> On 2016/05/25 19:43:16, JohnL wrote: >> > Is INFO consistent with other skipped users? It seems like FINE is >> > more > >> > appropriate. >> > > Acknowledged. >> > > Done. > > > https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2529: > // Try to refresh member id mapping and check again. > On 2016/05/25 19:43:16, JohnL wrote: > >> Does the exception failure about mean that this retry is bound to fail >> > also? Or > >> does this specific exception mean that, but other SOAP or IOExceptions >> > might > >> not? >> > > We assume SharePoint itself skips the SID it could not convert to string > and returns a SOAP exception instead and we act the same way. > Yes, the retry will fail too. > Starting at line 1902 getSiteUserMapping() will either way throw > IOException. > > If this is not what you meant please explain what you meant. > > > https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2540: > log.log(Level.INFO, "refreshSiteUserMapping failed with SOAP " > On 2016/05/25 21:54:52, myk wrote: > >> On 2016/05/25 19:43:16, JohnL wrote: >> > Same message issues here. >> > > Acknowledged. >> > > Done. > > > https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2543: > siteUserMappingRefreshed = true; > On 2016/05/25 21:54:52, myk wrote: > >> Should this go into the try block (after the "principal =" line), so >> > that it's > >> only set to true if the two statements you put into the try block >> > don't throw an > >> IOException? >> > > (I guess this is similar to JohnL's question about "is retry bound to >> > fail > >> also?") >> > > Actually it does not make sense to retry, I think it's OK here, we are > ignoring the error like SharePoint. > > https://codereview.appspot.com/294620043/ > -- Huba Paku | Applications Engineer | hpaku@google.com | +41 44 668 13 50 Service Provided by <EPAM Systems>, on behalf of Google Brandschenkestrasse 110, 8002 Zurich, Switzerland Company Identifikationsnummer: CH-020.4.028.116-1 This email can contain confidential information.If you received this email by mistake, do not pass it to third parties and delete all copies and enclosures, and let us know that it has been delivered to wrong address. Thank you.
Sign in to reply to this message.
This is bit confusing for me if I directly look at patch 2 as all I see is log level change. I guess you have done local commit for patch 1 and made changes on top of it as per code review comments. You should have done git commit --amend instead. Can you please upload new patch with base as remote head instead of your local head commit. On 2016/06/15 13:41:17, hpaku wrote: > Friendly ping on this. > > Thanks! > > On Thu, Jun 9, 2016 at 4:09 PM, <mailto:hpaku@google.com> wrote: > > > Please review. > > > > Thank you! > > > > > > > > > https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... > > File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java > > (right): > > > > > > > https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... > > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2522: > > } catch (IOException ex) { > > On 2016/05/25 19:43:16, JohnL wrote: > > > >> What does this IOException look like? The bug only specifies the IIS > >> > > log > > > >> message. > >> > > > > I updated b/28948870 <https://buganizer.corp.google.com/28948870> with > > the IOException and attached the log. > > > > > > > https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... > > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2523: > > log.log(Level.INFO, "getSiteUserMapping failed with SOAP " > > On 2016/05/25 21:54:52, myk wrote: > > > >> On 2016/05/25 19:43:16, JohnL wrote: > >> > Is INFO consistent with other skipped users? It seems like FINE is > >> > > more > > > >> > appropriate. > >> > > > > Acknowledged. > >> > > > > Done. > > > > > > > https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... > > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2529: > > // Try to refresh member id mapping and check again. > > On 2016/05/25 19:43:16, JohnL wrote: > > > >> Does the exception failure about mean that this retry is bound to fail > >> > > also? Or > > > >> does this specific exception mean that, but other SOAP or IOExceptions > >> > > might > > > >> not? > >> > > > > We assume SharePoint itself skips the SID it could not convert to string > > and returns a SOAP exception instead and we act the same way. > > Yes, the retry will fail too. > > Starting at line 1902 getSiteUserMapping() will either way throw > > IOException. > > > > If this is not what you meant please explain what you meant. > > > > > > > https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... > > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2540: > > log.log(Level.INFO, "refreshSiteUserMapping failed with SOAP " > > On 2016/05/25 21:54:52, myk wrote: > > > >> On 2016/05/25 19:43:16, JohnL wrote: > >> > Same message issues here. > >> > > > > Acknowledged. > >> > > > > Done. > > > > > > > https://codereview.appspot.com/294620043/diff/1/src/com/google/enterprise/ada... > > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2543: > > siteUserMappingRefreshed = true; > > On 2016/05/25 21:54:52, myk wrote: > > > >> Should this go into the try block (after the "principal =" line), so > >> > > that it's > > > >> only set to true if the two statements you put into the try block > >> > > don't throw an > > > >> IOException? > >> > > > > (I guess this is similar to JohnL's question about "is retry bound to > >> > > fail > > > >> also?") > >> > > > > Actually it does not make sense to retry, I think it's OK here, we are > > ignoring the error like SharePoint. > > > > https://codereview.appspot.com/294620043/ > > > > > > -- > > Huba Paku | Applications Engineer | mailto:hpaku@google.com | +41 44 668 13 50 > Service Provided by <EPAM Systems>, on behalf of Google > > Brandschenkestrasse 110, 8002 Zurich, Switzerland > Company Identifikationsnummer: CH-020.4.028.116-1 > This email can contain confidential information.If you received this email > by mistake, do not pass it to third parties and delete all copies and > enclosures, and let us know that it has been delivered to wrong address. > Thank you.
Sign in to reply to this message.
On Wed, Jun 15, 2016 at 9:15 AM, <tvartak@google.com> wrote: > This is bit confusing for me if I directly look at patch 2 as all I see > is log level change. I guess you have done local commit for patch 1 and > made changes on top of it as per code review comments. You should have > done git commit --amend instead. Can you please upload new patch with > base as remote head instead of your local head commit. I noticed the same, but I have the feeling that those are the only changes made since Patch Set 1. Huba, is that correct? -- Marc
Sign in to reply to this message.
On 2016/06/15 21:23:42, myk wrote: > On Wed, Jun 15, 2016 at 9:15 AM, <mailto:tvartak@google.com> wrote: > > > This is bit confusing for me if I directly look at patch 2 as all I see > > is log level change. I guess you have done local commit for patch 1 and > > made changes on top of it as per code review comments. You should have > > done git commit --amend instead. Can you please upload new patch with > > base as remote head instead of your local head commit. > > > I noticed the same, but I have the feeling that those are the only changes > made since Patch Set 1. > Huba, is that correct? > > -- Marc Yes, correct. Sorry for the mistake. Please review now Patch set 3.
Sign in to reply to this message.
Friendly ping on this. Thank you! On Thu, Jun 16, 2016 at 7:55 AM, <hpaku@google.com> wrote: > On 2016/06/15 21:23:42, myk wrote: > >> On Wed, Jun 15, 2016 at 9:15 AM, <mailto:tvartak@google.com> wrote: >> > > > This is bit confusing for me if I directly look at patch 2 as all I >> > see > >> > is log level change. I guess you have done local commit for patch 1 >> > and > >> > made changes on top of it as per code review comments. You should >> > have > >> > done git commit --amend instead. Can you please upload new patch >> > with > >> > base as remote head instead of your local head commit. >> > > > I noticed the same, but I have the feeling that those are the only >> > changes > >> made since Patch Set 1. >> Huba, is that correct? >> > > -- Marc >> > > Yes, correct. Sorry for the mistake. > > Please review now Patch set 3. > > > > > https://codereview.appspot.com/294620043/ > -- Huba Paku | Applications Engineer | hpaku@google.com | +41 44 668 13 50 Service Provided by <EPAM Systems>, on behalf of Google Brandschenkestrasse 110, 8002 Zurich, Switzerland Company Identifikationsnummer: CH-020.4.028.116-1 This email can contain confidential information.If you received this email by mistake, do not pass it to third parties and delete all copies and enclosures, and let us know that it has been delivered to wrong address. Thank you.
Sign in to reply to this message.
> > Yes, correct. Sorry for the mistake.
> >
> > Please review now Patch set 3.
There is absolutely no diff between Patch Set 2 and Patch Set 3.
Please make sure you:
% git add SharePointAdaptor.java
% git commit --amend
% upload.py ... --rev remotes/origin/master:master
That should allow us to see your diff accurately.
Thank you!
-- Marc
Sign in to reply to this message.
On 2016/06/27 17:59:51, myk wrote: > > > Yes, correct. Sorry for the mistake. > > > > > > Please review now Patch set 3. > > There is absolutely no diff between Patch Set 2 and Patch Set 3. > Please make sure you: > % git add SharePointAdaptor.java > % git commit --amend > % upload.py ... --rev remotes/origin/master:master > > That should allow us to see your diff accurately. > Thank you! > -- Marc Hello Mark, Please check and review. Thanks!
Sign in to reply to this message.
https://codereview.appspot.com/294620043/diff/60001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java (right): https://codereview.appspot.com/294620043/diff/60001/src/com/google/enterprise... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2521: principal = siteUserMapping.getPrincipal(id); You're only setting principal if siteUserMapping is null, which doesn't seem right, and is certainly a change. Should this stay outside the try statement?
Sign in to reply to this message.
JohnL points out a very important diff. I think that, once it's corrected, this will be good to go. https://codereview.appspot.com/294620043/diff/60001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java (right): https://codereview.appspot.com/294620043/diff/60001/src/com/google/enterprise... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2521: principal = siteUserMapping.getPrincipal(id); On 2016/07/08 22:45:11, JohnL wrote: > You're only setting principal if siteUserMapping is null, which doesn't seem > right, and is certainly a change. Should this stay outside the try statement? Nice catch. I definitely think it should.
Sign in to reply to this message.
On Fri, Jul 8, 2016 at 5:50 PM, <myk@google.com> wrote: > JohnL points out a very important diff. I think that, once it's > corrected, this will be good to go. Also, please consider adding a new unit test.
Sign in to reply to this message.
Sorry for the delay. Uploading a patch set does not send email by default, so I didn't notice this CL had changed. John L https://codereview.appspot.com/294620043/diff/100001/src/com/google/enterpris... File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java (right): https://codereview.appspot.com/294620043/diff/100001/src/com/google/enterpris... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:2569: principal = getSiteUserMapping().getPrincipal(userId); This is another call to getSiteUserMapping, and you're not handling exceptions here. You could instead simply handle exceptions in getSiteUserMapping itself. The only gap would be logging the ID, which we could pass in for that purpose, but it will be logged just after these failures if the ID is not eventually resolved.
Sign in to reply to this message.
|
