jdaugherty commented on code in PR #16130:
URL: https://github.com/apache/grails-core/pull/16130#discussion_r4348957914
##########
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 question rather than an ask. The description measures reading the flat key
before the walk at 84 ns and leaves it out because `config.a.put('b', x)`
updates only the nested value. On the description's harness here (126
variables) the walk costs about 32 ns per segment (substring, uncached hash,
map read), so the four-segment keys are most of the 160 ns; flat-first measures
91 ns, and with an allocation-free pre-filter (next comment) 55 ns, about six
times faster than base.
Is a raw `put` on a sub-map a write path this class supports? `NavigableMap`
is `@Deprecated`, `NullSafeNavigator.put` throws, and every documented path
(`merge`, dot-notation assignment, `setAt`) writes the flat key. If it is not
supported, flat-first looks like the right default. If it is, the description
should say which raw write wins, because the mirror case exists today:
`merge([a: [b: 'nested']])` then `config.put('a.b', 'flat')` writes only the
flat key, and `getProperty('a.b')` keeps returning `nested` on base and on this
branch.
##########
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:
Commit 9adc51487e changes what existing keys return. A key that ran past a
non-Map value was answered with that value and is now absent, and
`containsProperty` flips from true to false for it. It reaches more than
scalars:
- List parent: `a.b: [1, 2]`, `getProperty('a.b.0')` returned the list and
now returns null.
- Closure parent from `application.groovy`: `grails.gorm.default.constraints
= { ... }`, `getProperty('grails.gorm.default.constraints.nullable')` returned
the closure and `containsProperty` of it was true; now null and false (probed
through `GroovyConfigPropertySourceLoader`).
- The two representations also start to disagree differently:
`merge(['server.url.port': 'y', 'server.url': 'x'])` in that order returns `x`
today and `y` with this change; `put('a', 'scalar'); put('a.b', 1)` returns
`scalar` today and `1`.
Reading through a value like a closure may be relied on, and a change to the
answer for existing keys is one for a major release, not 8.1.x. Please drop
this commit from the PR and open it against 9.0.x, where it can carry the List
and closure cases, the flat-key states above, `navigateMap`/`CodeGenConfig`
(which still return the parent on this branch, so the two lookup paths would
otherwise disagree), and an upgrade note.
For 8.1.x the `indexOf` walk keeps the old semantics by leaving `value`
untouched when a segment lands on a non-Map: the `else` branch at
`NavigableMapConfig.java` lines 379-382 goes away, and this feature with it.
The `StringTokenizer` removal and the measured gain stay.
##########
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:
This is the whole cost of a miss, and it is not one hash lookup:
`toUpperCase` and `replace('.')` each allocate for a typical lowercase dotted
key, and `contains` then hashes the fresh string. On the description's harness,
head runs at 160 ns/op here; computing the canonical hash over the key's chars
without allocating (ASCII fast path, falling back to the
`toUpperCase(Locale.ROOT)` form for any char above 127) and keeping this set
lookup for the rare hit runs at 119 ns/op. A single-pass `char[]` build alone
is 151. The PR's specs pass with either.
The fallback is required: per-char `Character.toUpperCase` differs from
`String.toUpperCase(Locale.ROOT)` for 102 BMP chars (`ß` → `SS` and friends),
so a key `straße` would be rejected although `STRASSE` matches through
`resolvePropertyName`.
Smaller: `new HashSet<>()` at line 418 resizes four times for ~120 names;
`HashSet.newHashSet(variables.size())` is available on 21.
##########
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:
`PropertySourcesConfig.refresh()` re-runs `initializeFromPropertySources`,
which re-merges Spring's live `systemEnvironment` source into `configMap`, but
this index is built once here and never rebuilt. After a `refresh()` the raw
variable name is in the map while the dotted spelling still resolves against
the old snapshot. Probe: install `PROBE_STUBBED_AFTER` reflectively after
construction, call `refresh()`, read `probe.stubbed.after`: base returns
`from-env`, this branch `from-yml`.
Could `refresh()` re-capture the index (say a `protected void
refreshSystemEnvironment()` on this class, called from
`PropertySourcesConfig.refresh()`, with the field `volatile`)? Besides closing
the inconsistency, it gives a test that installs a variable after the context
exists (system-stubs, JUnit Pioneer) a public way to see it through
`grailsApplication.config`, which is the one observable thing the snapshot
takes away, without adding a config knob. One feature in
`SystemEnvironmentConfigSpec` ("...until `refresh()`") would pin it.
##########
grails-core/src/test/groovy/org/grails/config/SystemEnvironmentConfigSpec.groovy:
##########
@@ -32,25 +33,21 @@ import java.lang.reflect.Field
*/
class SystemEnvironmentConfigSpec extends Specification {
- @Issue('#10670')
- void 'configuration properties defined in SystemEnvironment take
precedence'() {
- given: 'default configuration in application-yml'
- Resource resource1 = new ByteArrayResource('''
+ private static final String YAML = '''
property.with.period: from-yml
property_with_underscore: from-yml
property-with-hyphen: from-yml
property-with_mixed.symbols: from-yml
-'''.bytes, 'test.yml')
-
- def propertySourceLoader = new YamlPropertySourceLoader()
- def yamlPropertiesSource =
propertySourceLoader.load('application.yml', resource1, null)
- def propertySources = new MutablePropertySources()
- propertySources.addFirst(yamlPropertiesSource.first())
- def config = new PropertySourcesConfig(propertySources)
+'''
- and: 'overriding value with system environment variable'
+ @Issue('#10670')
+ void 'configuration properties defined in SystemEnvironment take
precedence'() {
+ given: 'a value in the system environment'
modifiableSystemEnvironment.put(systemProperty, value)
+ and: 'default configuration in application-yml'
+ def config = configFor(YAML)
+
expect:
config.getProperty(propertyToFind) == value
Review Comment:
Two behaviour changes the description lists have no feature:
`getProperty(null)`, `getProperty(null, String, 'dflt')` and
`containsProperty(null)` returning null, `dflt` and false; and the
`Locale.ROOT` upper-casing. For the latter, `Locale.setDefault(new Locale('tr',
'TR'))` with `PROPERTY_WITH_ID=from-env` and `property.with.id: from-yml`
resolves to `from-env` on this branch and to `from-yml` on base (restore the
locale in `cleanup:`).
Nit on this table: every row uses `from-env`, so a row cannot tell its own
spelling from a sibling's, or from a variable leaked by an earlier feature. A
per-row value (`value: systemProperty` works) makes each row self-proving.
--
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]
