codeconsole commented on code in PR #16798: URL: https://github.com/apache/grails-core/pull/16798#discussion_r4432345662
########## grails-doc/src/en/guide/conf/config/ymlPropertyOptions.adoc: ########## @@ -34,6 +34,20 @@ production: Similarly system arguments can be accessed. +A placeholder is resolved wherever it appears in a value, including in an element of a list or in a map inside one: Review Comment: Done in 9d12728ab7, with your wording. The page now says a placeholder in a list element is resolved when the list is read through `grailsApplication.config`, and the sentence about the whole list no longer says "as Spring's `Environment` does". The table in the PR description no longer says it either. ########## grails-doc/src/en/guide/conf/config/ymlPropertyOptions.adoc: ########## @@ -34,6 +34,20 @@ production: Similarly system arguments can be accessed. +A placeholder is resolved wherever it appears in a value, including in an element of a list or in a map inside one: + +[source,yaml] +---- +app: + origins: + - https://app.example.com + - '${EXTRA_ORIGIN:https://admin.example.com}' +---- + +`grailsApplication.config` gives the resolved element whether the list is read whole, with `config.getProperty('app.origins', List)`, or by index, with `config.getProperty('app.origins[1]')`, as Spring's `Environment` does. Review Comment: Taken in 9d12728ab7, with the workaround from #16550: the NOTE says that a list bound with `@ConfigurationProperties` still receives the placeholder as written through Grails' YAML loader, and that a comma-separated value is resolved when it is bound. "Merging up" in the description now says to drop the NOTE at 8.0.x, where binding resolves list elements. ########## grails-core/src/main/groovy/org/grails/config/NavigableMapConfig.java: ########## @@ -53,15 +57,116 @@ @Deprecated public abstract class NavigableMapConfig implements Config { protected static final Logger LOG = LoggerFactory.getLogger(NavigableMapConfig.class); + + /** + * A key in brackets that is not an index and holds no dot, {@code [x]} of {@code routes[x]}. + */ + private static final Pattern NAMED_SUBSCRIPT = Pattern.compile("\\[([^.\\[\\]]*[^.\\[\\]0-9][^.\\[\\]]*)]"); protected ClassLoader classLoader = Thread.currentThread().getContextClassLoader(); protected ConfigurableConversionService conversionService = new DefaultConversionService(); + + /** + * Whether the merge resolves the placeholders in the values it writes. It does not while it merges values that are + * resolved already, so a placeholder an escape kept as text is not resolved a second time. + */ + private boolean resolvingPlaceholders = true; + protected NavigableMap configMap = new NavigableMap() { + + /** + * The texts the merge in progress has resolved, by the full name of the entry and the text. The merge writes one + * entry under several keys: a dotted key under the whole key and in its nested map, and a list element in its + * list in the nested map, in its list under the flat key and under its indexed key. Every write takes one value, + * even for {@code ${random.uuid}}. + */ + private Map<List<String>, String> resolvedTexts; + @Override - protected Object mergeMapEntry(NavigableMap targetMap, String sourceKey, Object newValue) { - if (newValue instanceof CharSequence) { - newValue = resolvePlaceholders(newValue.toString()); + public void merge(Map sourceMap, boolean parseFlatKeys) { + resolvedTexts = new HashMap<>(); + try { + super.merge(sourceMap, parseFlatKeys); + } finally { + resolvedTexts = null; + } + } + + /** + * Resolves the placeholders in a value before it is written: in a string, in the strings of a list, through + * the maps and lists it holds, and in a map written under an indexed key, {@code rows[0]}, which is written into + * its list as it is. A map written under a plain key is merged entry by entry, through this method. + */ + @Override + protected void mergeMapEntry(NavigableMap rootMap, String path, NavigableMap targetMap, String sourceKey, + Object sourceValue, boolean parseFlatKeys, boolean isNestedSet) { + Object value = sourceValue; + if (resolvingPlaceholders && (sourceValue instanceof CharSequence || sourceValue instanceof List || sourceValue instanceof Map && isIndexed(sourceKey))) { Review Comment: Done in c76316bd9e: the `Map` check is in parentheses and the condition is wrapped. a7958efcdf puts the `||` at the end of the first line, where checkstyle wants it. ########## grails-core/src/test/groovy/org/grails/config/PropertySourcesConfigSpec.groovy: ########## @@ -203,6 +207,194 @@ class PropertySourcesConfigSpec extends Specification { !config.containsKey('gorm.url') } + void 'a placeholder in a list element from application.yml is resolved as the environment resolves it'() { + given: "application.yml with a placeholder in a list of values and in a list of maps" + def yml = """\ +app: + origins: + - https://a.example.com + - '\${EXAMPLE_ORIGIN:https://default.example.com}' + rows: + - url: '\${EXAMPLE_ORIGIN:https://default.example.com}' + name: first + db: + host: '\${EXAMPLE_ORIGIN:https://default.example.com}' + tags: + - '\${EXAMPLE_ORIGIN:https://default.example.com}' +""" + def source = new YamlPropertySourceLoader().load('application.yml', new ByteArrayResource(yml.bytes)).first() + def environment = new StandardEnvironment() + environment.propertySources.addFirst(source) + ConfigurationPropertySources.attach(environment) + def config = new PropertySourcesConfig(environment.propertySources) + + expect: "the environment resolves each element" + environment.getProperty('app.origins[1]') == 'https://default.example.com' + environment.getProperty('app.rows[0].url') == 'https://default.example.com' + + and: "the config answers the same, by index and in the whole list" + config.getProperty('app.origins[1]') == 'https://default.example.com' + config.getProperty('app.origins', List) == ['https://a.example.com', 'https://default.example.com'] + config.navigate('app', 'origins') == ['https://a.example.com', 'https://default.example.com'] + config.getProperty('app.rows[0].url') == 'https://default.example.com' + config.navigate('app', 'rows')[0].url == 'https://default.example.com' + config.navigate('app', 'rows')[0].name == 'first' + + and: "so does a map or a list inside an element" + environment.getProperty('app.rows[0].db.host') == 'https://default.example.com' + config.getProperty('app.rows[0].db.host') == 'https://default.example.com' + config.navigate('app', 'rows')[0].db.host == 'https://default.example.com' + config.navigate('app', 'rows')[0].tags == ['https://default.example.com'] + } + + void 'a placeholder in a list held whole is resolved'() { + given: "a list held whole, as application.groovy gives it, of values, of maps and of lists" + def map = new NavigableMap() + map.merge([app: [origins: ['https://a.example.com', '${EXAMPLE_ORIGIN:https://default.example.com}'], + rows : [[url: '${EXAMPLE_ORIGIN:https://default.example.com}', port: 8080]], + groups : [['${EXAMPLE_ORIGIN:https://default.example.com}']]]], true) + def config = new PropertySourcesConfig(new NavigableMapPropertySource('application.groovy', map)) + + expect: + config.getProperty('app.origins', List) == ['https://a.example.com', 'https://default.example.com'] + config.navigate('app', 'origins') == ['https://a.example.com', 'https://default.example.com'] + config.navigate('app', 'rows') == [[url: 'https://default.example.com', port: 8080]] + config.navigate('app', 'groups') == [['https://default.example.com']] + + and: "the property source keeps the list as written" Review Comment: Added in 6c7e7ba220: `a list element follows the value its placeholder names when the config is refreshed, and #description keeps the placeholder`, for application.yml, application.groovy and an ordinary property source. It changes the value, calls `refresh()`, checks the whole list and navigation, and then checks that the source still holds `http://${exampleHost}`. It leaves out the read by index, which finds nothing in a list held whole, on 7.0.x too. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
