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

[Guillaume Déflache]

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.
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...).


> @@ -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?

Cheers,
    Guillaume