codeconsole commented on code in PR #16130:
URL: https://github.com/apache/grails-core/pull/16130#discussion_r4350713050
##########
grails-core/src/main/groovy/org/grails/config/NavigableMapConfig.java:
##########
@@ -55,6 +58,7 @@ public abstract class NavigableMapConfig implements Config {
protected static final Logger LOG =
LoggerFactory.getLogger(NavigableMapConfig.class);
protected ClassLoader classLoader =
Thread.currentThread().getContextClassLoader();
protected ConfigurableConversionService conversionService = new
DefaultConversionService();
+ private final SystemEnvironmentIndex systemEnvironment = new
SystemEnvironmentIndex(System.getenv());
Review Comment:
Done in dc3e5d19e8. The index is a `volatile` field that a `protected void
refreshSystemEnvironment()` on `NavigableMapConfig` re-captures, and
`PropertySourcesConfig.refresh()` calls it before it merges the property
sources again, so the raw name and the dotted spelling agree after a refresh.
Pinned by `a variable installed after the config exists is seen once the config
is refreshed` in `SystemEnvironmentConfigSpec`: the variable is installed after
construction, the config still answers `from-yml`, and after `refresh()` it
answers `from-env`. The 8.1 upgrade guide names
`grailsApplication.config.refresh()` as the way for a test that installs
variables after the context exists.
One thing to flag: `refresh()` is on `PropertySourcesConfig`, not on the
`Config` interface, so from a test it is a dynamic call. The guide says so
(e9fc57f940), and that `refresh()` merges the sources again and needs calling
again once the variables are removed, since the config keeps the environment it
last read. Putting `refresh()` on `Config` would be an API addition, so I left
it out; say if you want it there.
##########
grails-core/src/main/groovy/org/grails/config/NavigableMapConfig.java:
##########
@@ -219,7 +223,7 @@ public <T> T getProperty(String key, Class<T> targetType, T
defaultValue, List<T
@Override
public <T> T getProperty(String key, Class<T> targetType, T defaultValue) {
- Object value = findInSystemEnvironment(key);
+ Object value = systemEnvironment.get(key);
if (value == null) {
value = getValueWithDotNotatedKeySupport(configMap, key);
Review Comment:
A raw sub-map `put` turns out not to be the deciding case: flat-first also
changes answers for configs built from supported writes alone.
`merge(['server.url.port': 'y', 'server.url': 'x'])` is what layering a
higher-precedence scalar over a lower-precedence nested block produces at
startup, since `initializeFromPropertySources` merges the sources lowest
precedence first and `merge` leaves the stale descendant flat key behind; it
answers `server.url.port` with `x` today and `y` flat-first. `put('a',
'scalar'); put('a.b', 1)` answers `a.b` with `scalar` today and `1` flat-first.
Those are the two flat-key states you asked to move to 9.0.x, so this PR keeps
walk-first, and the description now says which raw write wins as it ships: a
raw sub-map `put` is visible to `getProperty`, while a root `put` or `setAt`
over a merged key changes only `config['a.b']`.
Flat-first goes into the 9.0.x PR with the absent-past-scalar change. There
the walk already answers those two cases with the flat value, so flat-first
only adds which raw single-representation write wins, and one upgrade note
covers both. With the pre-filter in, the gap on this harness is 104 ns
walk-first against the 55 ns you measured flat-first.
##########
grails-core/src/main/groovy/org/grails/config/NavigableMapConfig.java:
##########
@@ -404,27 +360,115 @@ private Object
getValueWithDotNotatedKeySupport(NavigableMap configMap, String k
return null;
}
- List<String> keys = convertTokensIntoArrayList(new
StringTokenizer(key, "."));
- if (keys.size() == 0) {
- return null;
- }
-
Object value = null;
- for (int i = 0; i < keys.size(); i++) {
- if (i == 0) {
- value = configMap.get(keys.get(i));
- } else if (value instanceof Map) {
- value = ((Map) value).get(keys.get(i));
+ boolean first = true;
+ int length = key.length();
+ int start = 0;
+ while (start < length) {
+ int end = key.indexOf('.', start);
+ if (end < 0) {
+ end = length;
+ }
+ if (end > start) {
+ String segment = key.substring(start, end);
+ if (first) {
+ value = configMap.get(segment);
+ first = false;
+ } else if (value instanceof Map) {
+ value = ((Map<?, ?>) value).get(segment);
+ } else {
+ // The key names a child of a scalar, which does not exist
whatever the scalar is.
+ return null;
+ }
}
+ start = end + 1;
}
return value;
}
- private List<String> convertTokensIntoArrayList(StringTokenizer st) {
- List<String> elements = new ArrayList<>();
- while (st.hasMoreTokens()) {
- elements.add(st.nextToken());
+ /**
+ * The process environment as it stood when this config was created,
indexed so that a key no
+ * variable can match is answered with one hash lookup.
+ * <p>
+ * A key matches a variable in any of the spellings Spring's {@code
SystemEnvironmentPropertySource}
+ * accepts: as written, with dots replaced by underscores, with hyphens
replaced by underscores,
+ * with both replaced, and the upper-cased form of each. Every one of
those spellings has the same
+ * canonical name, so a key whose canonical name belongs to no variable
cannot match and is
+ * rejected without probing. Only a key whose canonical name is present
runs the spelling-by-spelling
+ * resolution, which keeps the precedence between spellings as it was.
+ * <p>
+ * The environment of a running JVM cannot change, so one snapshot per
config instance is exact.
+ * The memory it takes is proportional to the number of environment
variables, not to the size of
+ * the configuration or to the number of keys looked up.
+ */
+ private static final class SystemEnvironmentIndex {
+
+ /**
+ * {@code System.getenv(String)} matches a variable name
case-insensitively on Windows, where
+ * the operating system does, and exactly everywhere else. The
snapshot keeps that contract.
+ */
+ private static final boolean IS_WINDOWS =
System.getProperty("os.name", "").startsWith("Windows");
+
+ private final Map<String, String> variables;
+ private final Set<String> canonicalNames;
+
+ SystemEnvironmentIndex(Map<String, String> environment) {
+ this.variables = IS_WINDOWS ? new
TreeMap<>(String.CASE_INSENSITIVE_ORDER) : new HashMap<>();
+ this.variables.putAll(environment);
+ this.canonicalNames = new HashSet<>();
+ for (String name : variables.keySet()) {
+ canonicalNames.add(canonicalName(name));
+ }
+ }
+
+ String get(String key) {
+ if (key == null || !canonicalNames.contains(canonicalName(key))) {
+ return null;
+ }
+ return variables.get(resolvePropertyName(key));
+ }
+
+ private static String canonicalName(String name) {
+ return name.toUpperCase(Locale.ROOT).replace('.',
'_').replace('-', '_');
Review Comment:
Done in c58e746a18. `mayMatch` folds each character of the key to its
canonical form (`a`-`z` to upper case, `.` and `-` to `_`) and accumulates
`String.hashCode` over them, then looks the hash up in the sorted `int[]` of
the variables' canonical-name hashes; only a hit builds the canonical name for
the exact set check before the spelling-by-spelling resolution. A character
above 127 takes the exact check directly, so `straße` still matches `STRASSE`.
Both collections are sized up front with `HashMap.newHashMap` and
`HashSet.newHashSet`. On the description's harness here: 141 → 104 ns per call
(60 variables).
The `ß` fallback has its own feature in 3c61a8142f: `straße.x` matches
`STRASSE_X`, which the per-character fold alone cannot do.
##########
grails-core/src/test/groovy/org/grails/config/PropertySourcesConfigSpec.groovy:
##########
@@ -116,4 +116,33 @@ class PropertySourcesConfigSpec extends Specification {
then: 'the second value is not merged with the first value'
cfg.foo.bar == ['two': '2']
}
+
+ void 'a dotted key walks plain nested maps and skips empty segments'() {
+ given: "a plain map value that was put without flattened keys"
+ def config = new PropertySourcesConfig()
+ config.put('plain', [nested: [value: 'found']])
+
+ expect: "each segment of the key is resolved against the map it
reaches"
+ config.getProperty('plain.nested.value') == 'found'
+ config.getProperty('plain.nested', Map) == [value: 'found']
+ config.getProperty('plain.missing.value') == null
+
+ and: "empty segments are ignored as they always were"
+ config.getProperty('plain..nested.value') == 'found'
+ config.getProperty('.plain.nested.value.') == 'found'
+ config.getProperty('...') == null
+ }
+
+ void 'a dotted key that runs past a scalar value is absent'() {
+ given: "a scalar value"
+ def config = new PropertySourcesConfig([server: [url:
'http://localhost']])
+
+ expect: "the scalar itself resolves"
+ config.getProperty('server.url') == 'http://localhost'
+
+ and: "a key that names a child of the scalar is absent rather than the
scalar"
+ config.getProperty('server.url.port') == null
Review Comment:
Reverted in 1e79f8f3fd, as a revert so the reviewed commits stay intact. The
`indexOf` walk stays and leaves the value untouched when a segment lands on a
non-Map, as the `StringTokenizer` walk did, and the feature went with it. The
9.0.x PR will carry the absent-past-scalar change with the List and closure
cases, the flat-key states, `navigateMap` and `CodeGenConfig`, flat-first (see
the other thread) and an upgrade note.
The restored answer is pinned in 66e9bb2abe (`plain.nested.value.deeper`
answers `found` and `containsProperty` is true), so the 9.0.x PR flips that
assertion deliberately rather than silently.
--
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]
