tags:

views:

191

answers:

6

(Note that it's more of a Bash question than a Java question, see note below)

When configuring log4j in each class, we do the following:

public class Example {

  private static final Logger log = Logger.getLogger( Example.class );

The problem is that we now have a medium-sized codebase (200K LOC) containing a lot of Java classes and... Quite some misconfigured log4j loggers.

It's because people (including me, I admit), did silly cut'n'paste resulting sometimes in this:

public class Another {

  private static final Logger log = Logger.getLogger( Example.class );

And boom, instead of having Another.class, it's the old Example.class that is left and hence wrongly appearing in the logs (hence causing quite a few headaches).

I find it kind of weird that such kind of misconfiguration can happen but it can and our main issue now is not that it can happen but that we have to fix the misconfured loggers.

How can we go about automatically detecting these? (the fix can be manual but I'd like a way to find all the classes where log4j is misconfigured).

A Bash shell script, for example, would be very welcome.

  1. for every .java file
  2. find every "class XXX"
  3. parse the next 'x' lines (say 20)
  4. is there a Logger.getLogger(...) line?
  5. if yes, does it match the "class XXX"?
  6. if no report

False positive ain't an issue so it's not a problem if a few bogus "class XXX" are parsed etc.

NOTE: the problem is really that we now have 200 000 lines of code and we'd like to detect the violation automatically (the fix can be manual) so the question is not similar to:

[Is there a better way to get the current class variable in java?1

Actually it's probably more of a Bash question than a Java question :)

Any help on this most welcome.

A: 

Look for solutions in this posting: http://stackoverflow.com/questions/2151167/is-there-a-better-way-to-get-the-current-class-variable-in-java

Alexander Pogrebnyak
That's interesting but I specified this is not my issue: my issue is that we have 200 000 lines of code where the problem is there, and we're looking for an automated way to detect the issues (for example to then fix them using the suggestions you linked too :)
Webinator
A: 

you could try weaving Logger.getLogger with AspectJ, in order to determine if the parameter, Example.class in your case, equals the "current class" name.

Hint: you can get the "current class" name programmatically using something like:

String className = new Exception().getStackTrace()[0].getClassName();
dfa
Ah that's interesting: I've got no experience with AspectJ that said and was hoping for some "command line" way to quickly find the violation.
Webinator
A: 

Look into checkstyle. You can write a checkstyle custom rule that does this. It will be an interesting exercise in XPath.

However, if the code is very predictably structured, I'd offer that it could be done in sed. If you want to structure the computation in bash, then ...

  1. use exec to open a file descriptor to the file
  2. loop with while reading lines
  3. when you see the first 'class' statement, grab the class name.
  4. when you see the Logger construction, grab and check.
bmargulies
I think I'll add a new, more precisely formulated question then. A great deal of the power of a Un*x environment as always been the ability to quickly combine a few commands to get, pragmatically, the job done. You make it sound like I'm trying to parse a structured Java file to retrieve the AST but I'm really not. What I'm asking is very reasonable and probably not hard at all for someone more familiar than me with find/awk/grep.
Webinator
@OldEnthusiast - If your code is very, very, predictable in format, then I won't deny that sed and bash could do the job.
bmargulies
@bm - even if it is not very predictable, the script will be used with a human in the loop to check for false positives. It is clunky, but nowhere like as dangerous as regex-ing XML ... embedded in some production server.
Stephen C
@Stephen C -- thus my edit to sketch out the implementation.
bmargulies
+1  A: 

Untested:

find *.java | while read file
    do
        lines=$(grep -A 20 "public class .* {" "$file")
        class=$(echo "$lines" | sed -n '1 s/public class \(.*\) {/\1/p'
        log=$(echo "$lines" | grep "Logger.getLogger"
        log=$(echo "$log" | sed -n 's/.*( *\(.*\).class *).*')
        if [[ "$log" != "$class" ]]
        then
            echo "There's a mis-match in file $file, class $class, for logger $log"
        fi
    done
Dennis Williamson
Thanks a lot, I ended up writing something similar and it allowed me to find quite some classes where the logger was uncorrectly initialized.
Webinator
+1  A: 

I guess if you're looking for a one-liner, one-liner

find -name "*.java" -exec sed -i \
    -e 's/private static final Logger \([a-zA-Z_][a-zA-Z0-9_]*).*$/private static final Logger \1 = LoggerFactory.make()/g' \
    -e 's/import org\.apache\.log4j\.Logger;/&\nimport path.to.LoggerFactory;/g' \
    {} \;

I would back up your code before trying this. It's probably broken in a couple places, but with a few corrections will get you what you're looking for. If you're using svn or something, you'll have to tweak find to exclude the .svn directories, otherwise your commits will be really messed up.

The gist: don't even bother trying to capture class names. Incorporate the solution indirectly linked to by Alexander. But replace your initial Logger declarations with factory calls. The only thing you need to capture is the name of the local variable. Then you need to find where your imports are, which I'm assuming you can do pretty exactly because you're importing log4j (or java.util.logging). Find that import statement and import your factory right below it.

BTW, all the warnings you're getting about automating this are correct and equally apply to this solution. You need to be prepared to javac everything right after trying this at the very least. Really, you should have some test suites with monster code coverage to automatically run at this point.

David Berger
+1 Your approach is interesting. I didn't think about automagically changing the class name to use the Factory make approach. Don't worry about the code: Mercurial/hg all over the place, massive code coverage, unit tests etc. Meanwhile I wrote something pretty similar to what Denis posted and it did work. Now I'll probably add the factory and use your one liner. Once again, no worries it's Mercurial :)
Webinator
A: 

There may be a detector in FindBugs - if there isn't, it's definitely one to write...

Rich