[Yanel-dev] Re: [Yanel-commits] rev 42979 - public/yanel/trunk/src/webapp/src/java/org/wyona/yanel/servlet

[Michael Wechner]

Guillaume Déflache schrieb:
> Hi!
>
> michi at wyona.com schrieb:
>> Author: michi
>> Date: 2009-05-21 15:24:19 +0200 (Thu, 21 May 2009)
>> New Revision: 42979
>>
>> Modified:
>>    
>> public/yanel/trunk/src/webapp/src/java/org/wyona/yanel/servlet/YanelServlet.java 
>>
>> Log:
>> cancel checkout bug fixed
>>
>> Modified: 
>> public/yanel/trunk/src/webapp/src/java/org/wyona/yanel/servlet/YanelServlet.java 
>>
>> ===================================================================
>> --- 
>> public/yanel/trunk/src/webapp/src/java/org/wyona/yanel/servlet/YanelServlet.java    
>> 2009-05-21 13:23:47 UTC (rev 42978)
>> +++ 
>> public/yanel/trunk/src/webapp/src/java/org/wyona/yanel/servlet/YanelServlet.java    
>> 2009-05-21 13:24:19 UTC (rev 42979)
> [...]
>> +                        if (checkoutUserID.equals(userID)) {
>> +                            versionable.cancelCheckout();
>> +                        } else {
>> +                            String eMessage = "Releasing the lock of 
>> '" + resource.getPath() + "' failed because checkout user '" + 
>> checkoutUserID + "' and session user '" + userID + "' are not the 
>> same!";
>> +                            log.warn(eMessage);
>> +                            throw new ServletException(eMessage);
>> +                        }
>>                      } catch (Exception e) {
>>                          log.error(e.getMessage(), e);
>> -                        throw new ServletException("Releasing of 
>> lock failed because of: " + resource.getPath() 
>> -                                + " " + e.getMessage(), e);
>> +                        throw new ServletException("Releasing the 
>> lock of '" + resource.getPath() + "' failed because of: " + 
>> e.getMessage(), e);
>>                      }
>>                  }
>>                  return;
>
> This looks fishy, we throw an exception but it is to get caught 
> immediately after by our own code: mabe the catch block should be 
> defined narrower somehow.

done
> Using "return;" early in the "cancelCheckout" branch would probably help!
>
> Also logging and throwing an wrapped exception are both nice things to 
> do (depending on the context), but please do not do both at the same 
> time, else logs quickly get incomprehensible (and sometimes truncated 
> somewhere along the way because they are too long...).

I generally agree, but I think it depends on the situation. IIRC the 
problem with the ServletException is that it's transmitted to the 
client, but otherwise it doesn't show up within the log4j log, but it 
helps if it is also part of the log4j.
>
>
>> @@ -1050,9 +1058,9 @@
>>              String message = res.getClass().getName() + " is not 
>> modifiable (neither V1 nor V2)!";
>>              log.warn(message);
>>   -            StringBuffer sb = new StringBuffer();
>>  
>> -            // TODO: Differentiate between Neutron based and other 
>> clients ...
>> +            // TODO: Differentiate between Neutron based and other 
>> clients ... (Use method isClientSupportingNeutron())
>> +            StringBuilder sb = new StringBuilder();
>>              sb.append("<?xml version=\"1.0\"?>");
>>              sb.append("<exception 
>> xmlns=\"http://www.wyona.org/neutron/1.0\" type=\"neutron\">");
>>              sb.append("<message>" + message + "</message>");
>>
>
> Maybe we should also use XMLExceptionV1.getCheckoutException here?

agreed, but see my email re type="neutron" and that it is not clear to 
me why it is being used. XMLExceptionV1 does not support this type, 
because the
neutron spec does not mention it, but Yanel is using it for whatever 
reason. I have changed it now by using existing Neutron exception types 
and added a TODO.

Cheers

Michi
>
> Cheers,
>    Guillaume