Grails

Allow more than one Log4J configuration closure

Details

  • Type: Improvement Improvement
  • Status: Closed Closed
  • Priority: Blocker Blocker
  • Resolution: Fixed
  • Affects Version/s: None
  • Fix Version/s: 2.0-M1
  • Component/s: Configuration
  • Labels:
    None
  • Patch Submitted:
    Yes
  • Testcase included:
    yes

Description

One of my biggest complaints about the DSL-based approach to Log4J configuration is the way that it is impossible to define a main configuration in Config.groovy but then selectively override parts of the configuration in a grails.config.locations external configuration file. The log4j closure is a single configuration entry, so if you specify a value in an external configuration file it completely replaces the settings from Config.groovy rather than adding to them.

The attached patch allows grailsApplication.config.log4j to be a Collection or Map (whose values are Closures) as well as a single closure, and in this case the configuration mechanism calls each closure in the collection or map in sequence. Since ConfigObject extends LinkedHashMap this permits the following idiom:

Config.groovy
grails.config.locations = ['file:external-config.groovy']

log4j.main = {
  // main L4J configuration here
}
external-config.groovy
    
log4j.external = {
  // common configuration
}

environments {
  development {
    log4j.env = {
      // environment-specific config
    }
  }
  production {
    log4j.env = {
      // environment-specific config
    }
  }
}

The closures will be called in their order of definition, i.e. log4j.main first, then log4j.external and finally the appropriate log4j.env.

Activity

Ascending order - Click to sort in descending order
Graeme Rocher made changes -
Field Original Value New Value
Priority Major [ 3 ] Blocker [ 1 ]
Fix Version/s 1.4-M1 [ 16812 ]
Contegix Support made changes -
Project Import Thu Mar 24 21:22:24 CDT 2011 [ 1301019744151 ]
Jeff Brown made changes -
Assignee Jeff Brown [ brownj ]
Jeff Brown made changes -
Status Open [ 1 ] In Progress [ 3 ]
Graeme Rocher made changes -
Fix Version/s 1.4-M1 [ 11040 ]
Fix Version/s 1.4-M2 [ 12504 ]
Repository Branch Revision Date Author/Committer Message
Grails Core master a995ca2... 2011-06-08 06:25:04 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - add test from patch attached to the JIRA
Files Changed
MODIFY grails-test-suite-uber/src/test/groovy/org/codehaus/groovy/grails/plugins/logging/Log4jDslTests.groovy

Repository Branch Revision Date Author/Committer Message
Grails Core cli-improvements a995ca2... 2011-06-08 06:25:04 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - add test from patch attached to the JIRA
Files Changed
MODIFY grails-test-suite-uber/src/test/groovy/org/codehaus/groovy/grails/plugins/logging/Log4jDslTests.groovy

Repository Branch Revision Date Author/Committer Message
Grails Core 2.0.x a995ca2... 2011-06-08 06:25:04 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - add test from patch attached to the JIRA
Files Changed
MODIFY grails-test-suite-uber/src/test/groovy/org/codehaus/groovy/grails/plugins/logging/Log4jDslTests.groovy

Repository Branch Revision Date Author/Committer Message
Grails Core hibernate4 a995ca2... 2011-06-08 06:25:04 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - add test from patch attached to the JIRA
Files Changed
MODIFY grails-test-suite-uber/src/test/groovy/org/codehaus/groovy/grails/plugins/logging/Log4jDslTests.groovy

Repository Branch Revision Date Author/Committer Message
Grails Core 2.1.x a995ca2... 2011-06-08 06:25:04 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - add test from patch attached to the JIRA
Files Changed
MODIFY grails-test-suite-uber/src/test/groovy/org/codehaus/groovy/grails/plugins/logging/Log4jDslTests.groovy

Repository Branch Revision Date Author/Committer Message
Grails Core 2.2.x a995ca2... 2011-06-08 06:25:04 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - add test from patch attached to the JIRA
Files Changed
MODIFY grails-test-suite-uber/src/test/groovy/org/codehaus/groovy/grails/plugins/logging/Log4jDslTests.groovy

Repository Branch Revision Date Author/Committer Message
Grails Core master 472e96d... 2011-06-08 06:26:10 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - fix assertions in test which were backwards
Files Changed
MODIFY grails-test-suite-uber/src/test/groovy/org/codehaus/groovy/grails/plugins/logging/Log4jDslTests.groovy

Repository Branch Revision Date Author/Committer Message
Grails Core cli-improvements 472e96d... 2011-06-08 06:26:10 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - fix assertions in test which were backwards
Files Changed
MODIFY grails-test-suite-uber/src/test/groovy/org/codehaus/groovy/grails/plugins/logging/Log4jDslTests.groovy

Repository Branch Revision Date Author/Committer Message
Grails Core 2.0.x 472e96d... 2011-06-08 06:26:10 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - fix assertions in test which were backwards
Files Changed
MODIFY grails-test-suite-uber/src/test/groovy/org/codehaus/groovy/grails/plugins/logging/Log4jDslTests.groovy

Repository Branch Revision Date Author/Committer Message
Grails Core hibernate4 472e96d... 2011-06-08 06:26:10 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - fix assertions in test which were backwards
Files Changed
MODIFY grails-test-suite-uber/src/test/groovy/org/codehaus/groovy/grails/plugins/logging/Log4jDslTests.groovy

Repository Branch Revision Date Author/Committer Message
Grails Core 2.1.x 472e96d... 2011-06-08 06:26:10 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - fix assertions in test which were backwards
Files Changed
MODIFY grails-test-suite-uber/src/test/groovy/org/codehaus/groovy/grails/plugins/logging/Log4jDslTests.groovy

Repository Branch Revision Date Author/Committer Message
Grails Core 2.2.x 472e96d... 2011-06-08 06:26:10 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - fix assertions in test which were backwards
Files Changed
MODIFY grails-test-suite-uber/src/test/groovy/org/codehaus/groovy/grails/plugins/logging/Log4jDslTests.groovy

Repository Branch Revision Date Author/Committer Message
Grails Core master 4e73595... 2011-06-08 06:28:18 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - allow more than 1 log4j configuration closure
Files Changed
MODIFY grails-plugin-logging/src/main/groovy/org/codehaus/groovy/grails/plugins/logging/Log4jConfig.groovy
MODIFY grails-plugin-logging/src/main/groovy/org/codehaus/groovy/grails/web/util/Log4jConfigListener.java

Repository Branch Revision Date Author/Committer Message
Grails Core cli-improvements 4e73595... 2011-06-08 06:28:18 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - allow more than 1 log4j configuration closure
Files Changed
MODIFY grails-plugin-logging/src/main/groovy/org/codehaus/groovy/grails/plugins/logging/Log4jConfig.groovy
MODIFY grails-plugin-logging/src/main/groovy/org/codehaus/groovy/grails/web/util/Log4jConfigListener.java

Repository Branch Revision Date Author/Committer Message
Grails Core 2.0.x 4e73595... 2011-06-08 06:28:18 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - allow more than 1 log4j configuration closure
Files Changed
MODIFY grails-plugin-logging/src/main/groovy/org/codehaus/groovy/grails/plugins/logging/Log4jConfig.groovy
MODIFY grails-plugin-logging/src/main/groovy/org/codehaus/groovy/grails/web/util/Log4jConfigListener.java

Repository Branch Revision Date Author/Committer Message
Grails Core hibernate4 4e73595... 2011-06-08 06:28:18 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - allow more than 1 log4j configuration closure
Files Changed
MODIFY grails-plugin-logging/src/main/groovy/org/codehaus/groovy/grails/plugins/logging/Log4jConfig.groovy
MODIFY grails-plugin-logging/src/main/groovy/org/codehaus/groovy/grails/web/util/Log4jConfigListener.java

Repository Branch Revision Date Author/Committer Message
Grails Core 2.1.x 4e73595... 2011-06-08 06:28:18 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - allow more than 1 log4j configuration closure
Files Changed
MODIFY grails-plugin-logging/src/main/groovy/org/codehaus/groovy/grails/plugins/logging/Log4jConfig.groovy
MODIFY grails-plugin-logging/src/main/groovy/org/codehaus/groovy/grails/web/util/Log4jConfigListener.java

Repository Branch Revision Date Author/Committer Message
Grails Core 2.2.x 4e73595... 2011-06-08 06:28:18 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - allow more than 1 log4j configuration closure
Files Changed
MODIFY grails-plugin-logging/src/main/groovy/org/codehaus/groovy/grails/plugins/logging/Log4jConfig.groovy
MODIFY grails-plugin-logging/src/main/groovy/org/codehaus/groovy/grails/web/util/Log4jConfigListener.java

Repository Branch Revision Date Author/Committer Message
Grails Core master f26a633... 2011-06-08 06:30:07 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - improve reloading support for defining more than 1 log4j configuration closure
Files Changed
MODIFY grails-plugin-logging/src/main/groovy/org/codehaus/groovy/grails/plugins/LoggingGrailsPlugin.groovy

Repository Branch Revision Date Author/Committer Message
Grails Core cli-improvements f26a633... 2011-06-08 06:30:07 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - improve reloading support for defining more than 1 log4j configuration closure
Files Changed
MODIFY grails-plugin-logging/src/main/groovy/org/codehaus/groovy/grails/plugins/LoggingGrailsPlugin.groovy

Repository Branch Revision Date Author/Committer Message
Grails Core 2.0.x f26a633... 2011-06-08 06:30:07 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - improve reloading support for defining more than 1 log4j configuration closure
Files Changed
MODIFY grails-plugin-logging/src/main/groovy/org/codehaus/groovy/grails/plugins/LoggingGrailsPlugin.groovy

Repository Branch Revision Date Author/Committer Message
Grails Core hibernate4 f26a633... 2011-06-08 06:30:07 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - improve reloading support for defining more than 1 log4j configuration closure
Files Changed
MODIFY grails-plugin-logging/src/main/groovy/org/codehaus/groovy/grails/plugins/LoggingGrailsPlugin.groovy

Repository Branch Revision Date Author/Committer Message
Grails Core 2.1.x f26a633... 2011-06-08 06:30:07 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - improve reloading support for defining more than 1 log4j configuration closure
Files Changed
MODIFY grails-plugin-logging/src/main/groovy/org/codehaus/groovy/grails/plugins/LoggingGrailsPlugin.groovy

Repository Branch Revision Date Author/Committer Message
Grails Core 2.2.x f26a633... 2011-06-08 06:30:07 -0700 Jeff Brown / Jeff Brown GRAILS-7314 - improve reloading support for defining more than 1 log4j configuration closure
Files Changed
MODIFY grails-plugin-logging/src/main/groovy/org/codehaus/groovy/grails/plugins/LoggingGrailsPlugin.groovy

Hide
Permalink
Jeff Brown added a comment -

Fixed:

https://github.com/grails/grails-core/commit/a995ca2608117f650fd3dab4bc9248f97f632e8c
https://github.com/grails/grails-core/commit/472e96d535aac986a744672cd01346a614a763ae
https://github.com/grails/grails-core/commit/4e735953811b4ea29852b82a53b47abb3ca0ad9f
https://github.com/grails/grails-core/commit/f26a633662707424a7b6ce43db213e95c3a85723

Show
Jeff Brown added a comment - Fixed: https://github.com/grails/grails-core/commit/a995ca2608117f650fd3dab4bc9248f97f632e8c https://github.com/grails/grails-core/commit/472e96d535aac986a744672cd01346a614a763ae https://github.com/grails/grails-core/commit/4e735953811b4ea29852b82a53b47abb3ca0ad9f https://github.com/grails/grails-core/commit/f26a633662707424a7b6ce43db213e95c3a85723
Jeff Brown made changes -
Status In Progress [ 3 ] Closed [ 6 ]
Resolution Fixed [ 1 ]
Burt Beckwith made changes -
Workflow jira [ 35252 ] Grails [ 46979 ]
Burt Beckwith made changes -
Workflow Grails [ 46979 ] Copy of Grails [ 54438 ]
Burt Beckwith made changes -
Workflow Copy of Grails [ 54438 ] Grails [ 61848 ]
Burt Beckwith made changes -
Workflow Grails [ 61848 ] Grails2 [ 69429 ]
Burt Beckwith made changes -
Workflow Grails2 [ 69429 ] jira [ 76636 ]
Burt Beckwith made changes -
Workflow jira [ 76636 ] Grails2 [ 79609 ]
Peter Ledbrook made changes -
Workflow Grails2 [ 79609 ] jira [ 87720 ]
Peter Ledbrook made changes -
Workflow jira [ 87720 ] Grails2 [ 95954 ]
Hide
Permalink
Peter Ledbrook added a comment -

Wow, only just discovered this feature existed! I'm glad it was added and thanks Ian for contributing it. I do have a comment about the implementation though: I think it's bad form to rely on ConfigObject extending LinkedHashMap. It really shouldn't extend that class anyway (have we learned nothing from Properties?) and it may very well not extend the class in future, although if it does change it would undoubtedly implement Map.

Of course, relying on insertion order of keys is fine if that's part of the contract that ConfigObject defines, but I'm not sure it does. As far as I understand, that behaviour is only implicit through the use of LinkedHashMap. Even if it were part of the contract, I'm not sure we should expose that to users for a couple of reasons.

First, by documenting log4j.main, log4j.external and log4j.env in the manner given, a user could be forgiven for thinking that log4j.env will override log4j.external, and log4j.external will override log4j.env, regardless of the order they are defined. Remember that this is a convention over configuration framework, and main, external and env look like conventions.

Second, I don't really understand the code be it seems from the comments that those closures defined later override the ones defined earlier. This is the opposite of ConfigObject merge behaviour, although it's consistent with the behaviour when defining the same property twice in the same file. This is less of an issue to be honest and I guess most people aren't aware of the merge behaviour anyway.

I guess it's probably too late to change the behaviour now, but I would prefer to see the order of overriding to be determined by the semantic meaning of the log key. Any others can come after, but I don't think the order of those should be defined.

Show
Peter Ledbrook added a comment - Wow, only just discovered this feature existed! I'm glad it was added and thanks Ian for contributing it. I do have a comment about the implementation though: I think it's bad form to rely on ConfigObject extending LinkedHashMap . It really shouldn't extend that class anyway (have we learned nothing from Properties ?) and it may very well not extend the class in future, although if it does change it would undoubtedly implement Map. Of course, relying on insertion order of keys is fine if that's part of the contract that ConfigObject defines, but I'm not sure it does. As far as I understand, that behaviour is only implicit through the use of LinkedHashMap . Even if it were part of the contract, I'm not sure we should expose that to users for a couple of reasons. First, by documenting log4j.main , log4j.external and log4j.env in the manner given, a user could be forgiven for thinking that log4j.env will override log4j.external , and log4j.external will override log4j.env , regardless of the order they are defined. Remember that this is a convention over configuration framework, and main , external and env look like conventions. Second, I don't really understand the code be it seems from the comments that those closures defined later override the ones defined earlier. This is the opposite of ConfigObject merge behaviour, although it's consistent with the behaviour when defining the same property twice in the same file. This is less of an issue to be honest and I guess most people aren't aware of the merge behaviour anyway. I guess it's probably too late to change the behaviour now, but I would prefer to see the order of overriding to be determined by the semantic meaning of the log key. Any others can come after, but I don't think the order of those should be defined.
Hide
Permalink
Peter Ledbrook added a comment -

As a concrete example, imagine that the log4j.external is defined in one external file and log4j.env is defined in another. Which closure would take precedence?

Show
Peter Ledbrook added a comment - As a concrete example, imagine that the log4j.external is defined in one external file and log4j.env is defined in another. Which closure would take precedence?

People

  • Assignee:
    Jeff Brown
    Reporter:
    Ian Roberts
Vote (2)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved: