Grails 7: Accessing config key 'grails.sitemesh.default.layout' through dot notation is deprecated

[jamesfredley]

trafficstars

Issue description

There is an open discussion on which way to proceed.

Sample App: https://github.com/jamesfredley/grails-website-test/tree/7.0.0-SNAPSHOT

Start with: ./gradlew bootRun or ./grailsw run-app

2024-09-19T08:56:44.757-04:00  WARN 13688 --- [  restartedMain] org.grails.config.NavigableMap           : Accessing config key 'grails.sitemesh.default.layout' through dot notation is deprecated, and it will be removed in a future release. Use 'config.getProperty(key, targetClass)' instead.

[davydotcom]

@jamesfredley if I recall this config access method was deprecated mostly due to micronaut... is it still worth deprecating? I always thought it was silly we removed some of this magic traversal

[jamesfredley]

@davydotcom I wasn't around for that historical context, but it would be great to keep config key access via dot notation and get rid of these warnings.

[matrei]

I think the problem here might be faulty condition logic. This method in NavigableMap is what prints the warning:

public Object getProperty(String name) {
    if (!containsKey(name)) {
        return null
    }
    Object result = get(name)
    if (!(result instanceof NavigableMap)) {
        if (LOG.isWarnEnabled()) {
            LOG.warn("Accessing config key '{}' through dot notation is deprecated, and it will be removed in a future release. Use 'config.getProperty(key, targetClass)' instead.", name)
        }
    }
    return result
}

When Grails is starting up it passes the first condition (containsKey('grails.sitemesh.default.layout') == true) but then get('grails.sitemesh.default.layout') == null which the second condition (!(result instanceof NavigableMap) does not account for.

If we change the second condition to if(result && !(result instanceof NavigableMap)) this warning should go away.

[jamesfredley]

related to:

https://github.com/grails/grails-core/issues/10188 https://github.com/grails/grails-core/pull/10280

[codeconsole]

Dot notation currently exclusively broken in plugins on 7..0.x

gsp example Nullpointer exception - console == null

${grailsApplication.config.grails.plugin.console.layout}

XXGrailsPlugin.groovy example Nullpointer exception - console == null

    void doWithApplicationContext() {
        config.grails.assets.plugin.'console'.excludes = ['**/*']
    }

Config is org.grails.config.PropertySourcesConfig

[codeconsole]

https://github.com/grails/grails-core/blob/7.0.x/grails-core/src/main/groovy/org/grails/config/PropertySourcesConfig.java https://github.com/grails/grails-core/commits/7.0.x/grails-core/src/main/groovy/org/grails/config/PropertySourcesConfig.java

[codeconsole]

https://github.com/grails/grails-core/blob/7.0.x/grails-core/src/main/groovy/org/grails/config/NavigableMapConfig.java https://github.com/grails/grails-core/commits/7.0.x/grails-core/src/main/groovy/org/grails/config/NavigableMapConfig.java

[codeconsole]

https://github.com/grails/grails-core/compare/6.2.x...7.0.x

[codeconsole]

https://github.com/grails/grails-core/pull/13448

[codeconsole]

Remove deprecated type NavigableMap.NullSafeNavigator https://github.com/grails/grails-core/pull/13448/commits/1ee3954f9688761a88d375c779e42481197c56e1

[codeconsole]

Deprecated here: https://github.com/grails/grails-core/pull/11554 https://github.com/grails/grails-core/commit/bdd4bc86b7fde4da4aaec99bbb1e82eb9db7dee7

https://docs.grails.org/5.3.6/guide/single.html#whatsNew

Deprecating ‘dot’-Based Navigation The ‘dot’-based navigation to Grails config is deprecated and will be removed in the future.

We request that you update your plugins to use configuration beans @ConfigurationProperties or @Value, or access configuration settings using grailsApplication.config.getProperty(‘a.b.c’, String) instead of grailsApplication.config.a.b.c. For more information, read the documentation at Creating and Installing Plugins.

[codeconsole]

From my comments on slack:

I don't really care about losing dot notation to be honest if it incurs and performance impact The issue I have is with the loss of the setter ${grailsApplication.config['grails.plugin.console.layout']} is a viable alternative but I do not see an inherent way to set a Map as we saw in the web console plugin upgrade I think this could be just a matter of adding a setProperty method? setProperty could be inefficient, but getters should be as efficient and null safe as possible..

we just need a solution for

config.grails.assets.plugin.'console'.excludes = ['**/*']

The current workaround is

config.merge(['config.grails.assets.plugin.console.excludes': ['**/*']])

[codeconsole]

https://github.com/grails/grails-core/blob/ab1b7e9ec4312170b4e6172bab89105cd12f9c4a/grails-bootstrap/src/main/groovy/org/grails/config/NavigableMap.groovy#L27

https://github.com/grails/grails-core/blob/7.0.x/grails-core/src/main/groovy/grails/config/Config.groovy

[codeconsole]

Config extends ConfigMap which has a setAt(Object key, Object value) but that won't be navigable it will just set the full key I think instead of a TreeMap I think the only real lacking solution here is the loss of the namespace. For instance, you can no longer config['grails'] and get everything prefixed with grails (edited) If we can modify the existing code to set to namespaces and add a setAt function that uses merge, it could be a better solution IF we are truly suffering a performance hit at the current use of getters.

So the following questions need to be answered in the current state:

1. Why does config.grails.assets.plugin.'console'.excludes = ['**/*'] still work in application.groovy? I am guessing because it is parsed differently.
2. Is there still a solution to get all configurations for a namespace? e.g. config['grails.assets'] should return all configurations in that namespace.
3. Setting should set to a namespace. Currently config['grails.assets.plugin.console.excludes'] sets grails.assets.plugin.console.excludes as the key instead of a NavigableMap. Is there a workaround setter? Does application.groovy still work correctly? Always setting to namespace will resolve question 2.

[jeffscottbrown]

if I recall this config access method was deprecated mostly due to micronaut

Do you recall what it was about Micronaut integration that lead to this method being deprecated?

[jdaugherty]

Config extends ConfigMap which has a setAt(Object key, Object value) but that won't be navigable it will just set the full key I think instead of a TreeMap I think the only real lacking solution here is the loss of the namespace. For instance, you can no longer config['grails'] and get everything prefixed with grails (edited) If we can modify the existing code to set to namespaces and add a setAt function that uses merge, it could be a better solution IF we are truly suffering a performance hit at the current use of getters.

So the following questions need to be answered in the current state:

1. Why does `config.grails.assets.plugin.'console'.excludes = ['**/*']` still work in `application.groovy`?
   I am guessing because it is parsed differently.

2. Is there still a solution to get all configurations for a namespace? e.g. `config['grails.assets']` should return all configurations in that namespace.

3. Setting should set to a namespace.  Currently config['grails.assets.plugin.console.excludes'] sets `grails.assets.plugin.console.excludes` as the key instead of a NavigableMap.  Is there a workaround setter? Does `application.groovy` still work correctly?  Always setting to namespace will resolve question 2.

On 1., the last time I looked at this it's because application.groovy is wired to a ConfigObject and ConfigObjects return an empty configObject if the key doesn't exist.

[jdaugherty]

We decided in the stewards meeting to revert the removal of this access. We will open separate tickets to address the other issues. This ticket will simply revert the removal.

[jdaugherty]

https://github.com/grails/grails-core/pull/13869 will restore this behavior. Separately, I've started looking into making this a java class, but it's clone() and @EqualsAndHashcode implementations have lead to further questions that need answered before we can convert.

[xpusostomos]

I've got to comment on this... I suspect the dot notation was deprecated, because the code to make dot notation work is a mess and a performance problem ... BUT BUT BUT BUT BUT ... it can be implemented easily in an efficient manner, so this whole deprecation bothers me a lot. The whole fabric of Groovy is that it can make things that you are tempted to write your own language for (and make no mistake "foo.bar.baz" is a mini language), into a groovy syntax. For example, the grails query syntaxes. It can do it by synthesizing method names on the fly. By all means clean up the current performance hindered properties implementation, but dot notation can be implemented using groovy magic in a few lines of code and won't hurt performance one little bit... especially for those who don't even use it. DON'T DEPRECATE IT.