commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sebb (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (EL-17) Exceptions thrown in conditional logging blocks - looks totally wrong
Date Thu, 26 May 2011 21:43:47 GMT

    [ https://issues.apache.org/jira/browse/EL-17?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13039949#comment-13039949
] 

Sebb commented on EL-17:
------------------------

The original code in EL 1.0 is

{code}
if (pLogger.isLoggingError ()) {
  pLogger.logError
    (Constants.NO_PROPERTY_EDITOR,
     str,
     pClass.getName ());
}
return null;
{code}

At first sight, this appears to conditionally log a message and then return null. 
However, the logError() method actually throws the exception ELException - it does not log
an error at all. 
Furthermore, the method isLoggingError() always returns true, so the return statement is never
reached.

This means that the new code is equivalent, provided that isErrorEnabled() behaves the same
as isLoggingError().
However, that is not the case, as the latter is the logging level, whereas the former determined
whether or not to throw an Exception.

> Exceptions thrown in conditional logging blocks - looks totally wrong
> ---------------------------------------------------------------------
>
>                 Key: EL-17
>                 URL: https://issues.apache.org/jira/browse/EL-17
>             Project: Commons EL
>          Issue Type: Bug
>            Reporter: Sebb
>
> The commit:
> http://svn.apache.org/viewvc?view=revision&revision=133240
> says
> Minor cleanup. Patch provided by Ryan Lubke
> Sounds innocuous. However, some of the changes actually added throw statements within
conditional logging statements, for example:
> {code}
> --- jakarta/commons/proper/el/trunk/src/java/org/apache/commons/el/Coercions.java	2003/07/31
02:01:07	133239
> +++ jakarta/commons/proper/el/trunk/src/java/org/apache/commons/el/Coercions.java	2003/08/01
21:12:32	133240
> @@ -394,11 +394,13 @@
>        try {
>  	return pValue.toString ();
>        }
> -      catch (Exception exc) {
> +      catch (Exception exc) {          
>            if (log.isErrorEnabled()) {
> -              log.error(
> -                  MessageUtil.getMessageWithArgs(Constants.TOSTRING_EXCEPTION, 
> -                                                 pValue.getClass().getName()), exc);
> +              String message = MessageUtil.getMessageWithArgs(
> +                  Constants.TOSTRING_EXCEPTION,
> +                  pValue.getClass().getName());
> +              log.error(message, exc);
> +              throw new ELException(exc);
>            }
>            return "";	
>        }
> {code}
> The first part of the change does nothing except expose the message as a string, but
why is the throw statement added?
> That seems completely wrong.
> There are a few other places where throw statements have been added to the logging conditional
blocks.
> Either these should be removed, or they should be outside the block.

--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira

Mime
View raw message