Clojure-Contrib

Unknown severity level causes NPE

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None

Description

$ cat .clojure
/opt/clojure/clojure.jar:/opt/clojure-contrib/clojure-contrib.jar:lib/commons-logging-1.1.1.jar
$ clj
Clojure 1.1.0-alpha-SNAPSHOT
user=> (use 'clojure.contrib.logging)
nil
user=> (log :info "Hello, world.")   
Sep 21, 2009 3:44:03 PM clojure.contrib.logging$eval__92$impl_write_BANG___100 invoke
INFO: Hello, world.
nil
user=> (log :bar "Hello, world.") 
java.lang.NullPointerException (NO_SOURCE_FILE:0)

I hit this when using :severe as a log level, which should actually be :error.

Activity

Hide
Assembla Importer added a comment -

ataggart said: Handled at the protocol implementation level in new version of c.c.logging (since alternate implementations may choose to use non-standard levels).

Show
Assembla Importer added a comment - ataggart said: Handled at the protocol implementation level in new version of c.c.logging (since alternate implementations may choose to use non-standard levels).
Hide
Assembla Importer added a comment -

stuart.sierra said: Updating tickets (#28, #32, #45, #47, #51, #19)

Show
Assembla Importer added a comment - stuart.sierra said: Updating tickets (#28, #32, #45, #47, #51, #19)
Hide
Assembla Importer added a comment -

rnewman said: "If one uses an invalid value one should not expect it to work"

No, but one should expect something more useful than a NPE. The patch I attached throws an exception when an invalid value is used (indeed, at compile time if it's a constant keyword).

c.c.l already translates from one set of log levels to those in use by the underlying implementation (e.g., if you're using Java Logging it'll convert :debug to FINE). This patch ensures that you can use the other set of names, too, which seems perfectly reasonable, but I can understand if you disagree. In that case, simply change translate-log-level to something like

(defn translate-log-level [level]
(or (#{:trace :debug :info :warn :error :fatal} level)
(throw (new Exception (str "Unknown log level " (prn-str level))))))

and commit. I'm not particularly fussed either way, but the validation needs to be done.

Show
Assembla Importer added a comment - rnewman said: "If one uses an invalid value one should not expect it to work" No, but one should expect something more useful than a NPE. The patch I attached throws an exception when an invalid value is used (indeed, at compile time if it's a constant keyword). c.c.l already translates from one set of log levels to those in use by the underlying implementation (e.g., if you're using Java Logging it'll convert :debug to FINE). This patch ensures that you can use the other set of names, too, which seems perfectly reasonable, but I can understand if you disagree. In that case, simply change translate-log-level to something like (defn translate-log-level [level] (or (#{:trace :debug :info :warn :error :fatal} level) (throw (new Exception (str "Unknown log level " (prn-str level)))))) and commit. I'm not particularly fussed either way, but the validation needs to be done.
Hide
Assembla Importer added a comment -

ataggart said: The only valid levels are the ones specified in the documentation, those levels in common use across multiple logging implementations. If one uses an invalid value one should not expect it to work.

The "correct" solution would be to provide a better exception in this error case.

Show
Assembla Importer added a comment - ataggart said: The only valid levels are the ones specified in the documentation, those levels in common use across multiple logging implementations. If one uses an invalid value one should not expect it to work. The "correct" solution would be to provide a better exception in this error case.
Hide
Assembla Importer added a comment -

rnewman said: [file:aOuXXKPWqr3OVpeJe5afGb]: Correct quoting.

Show
Assembla Importer added a comment - rnewman said: [file:aOuXXKPWqr3OVpeJe5afGb]: Correct quoting.
Hide
Assembla Importer added a comment -

rnewman said: Added a patch. This does three things:

  • Examines the level in (log). If it's a keyword at compile-time, transform it now; otherwise, transform it later.
  • Transformation maps j.u.l terms to those used by c.c.l. This is handy, though the mapping (of course) does not round-trip perfectly.
  • Unrecognized terms cause an exception to be thrown.

The best test is

(require ['clojure.contrib.logging :as 'log])
(log/log ((fn []:severe)) "hi")   ; Test runtime level computation. 
(log/log :boo "ho")
(log/log :fine "ha")
(log/log :info "ha")
Show
Assembla Importer added a comment - rnewman said: Added a patch. This does three things:
  • Examines the level in (log). If it's a keyword at compile-time, transform it now; otherwise, transform it later.
  • Transformation maps j.u.l terms to those used by c.c.l. This is handy, though the mapping (of course) does not round-trip perfectly.
  • Unrecognized terms cause an exception to be thrown.
The best test is
(require ['clojure.contrib.logging :as 'log])
(log/log ((fn []:severe)) "hi")   ; Test runtime level computation. 
(log/log :boo "ho")
(log/log :fine "ha")
(log/log :info "ha")
Hide
Assembla Importer added a comment -

rnewman said: [file:bCd_BmPWmr3OVpeJe5afGb]: Patch.

Show
Assembla Importer added a comment - rnewman said: [file:bCd_BmPWmr3OVpeJe5afGb]: Patch.

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: