borinquenkid commented on code in PR #16034:
URL: https://github.com/apache/grails-core/pull/16034#discussion_r3692289709
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtils.java:
##########
@@ -47,7 +49,28 @@
* @since 6.1
*/
public class AstPropertyResolveUtils {
- protected static Map<String, Map<String, ClassNode>> cachedClassProperties
= new HashMap<>();
+
+ /**
+ * Cache of resolved properties per {@link ClassNode}.
+ * <p>
+ * Keyed by {@code ClassNode} identity rather than name. {@link
ClassNode#equals(Object)} and
+ * {@link ClassNode#hashCode()} compare by {@link ClassNode#getText()}
(essentially the class
+ * name), so a {@code Map} keyed by name - or even by {@code ClassNode}
itself as the map key -
+ * treats any two distinct {@code ClassNode} instances that happen to
share a name as the same
+ * cache entry. That collision is a real hazard for classes compiled
without a package (common
+ * in tests and dynamically generated sources), and for the same source
compiled more than once
+ * in separate {@code GroovyClassLoader}s: each compilation produces its
own {@code ClassNode}
+ * instance that must never share cached property data with another
compilation's instance of a
+ * same-named class. An {@link IdentityHashMap} avoids that collision
entirely by comparing keys
+ * with {@code ==} instead of {@code equals()}.
+ * <p>
+ * Wrapped in {@link Collections#synchronizedMap(Map)} because AST
transforms that populate and
+ * read this cache can run concurrently on multiple threads (e.g. parallel
test execution within
+ * one JVM/fork); a plain, unsynchronized {@link HashMap} is not safe for
concurrent structural
+ * modification and can corrupt its internal state under concurrent {@code
put()} calls.
+ */
+ protected static final Map<ClassNode, Map<String, ClassNode>>
cachedClassProperties =
+ Collections.synchronizedMap(new IdentityHashMap<>());
Review Comment:
Agreed, and went with your suggested direction: the static map is gone
entirely, and the cache is now stored as `ClassNode` metadata
(`classNode.redirect().getNodeMetaData(key, fn)`), so a cache entry is only
reachable through the node it describes and is collected with it. See the
updated javadoc on `cachedClassProperties`'s replacement
(`PROPERTIES_CACHE_KEY`) for the full writeup, including why keying on
`redirect()` is safe here (`setRedirect()` throws for a *primary* node - i.e.
every real caller of this utility, since they're all mid-compilation - so
`redirect()` is just `this` for the node's whole life in practice) and why
access ended up needing explicit per-node synchronization (some real callers
can resolve to interned singletons like `ClassHelper.OBJECT_TYPE` for a plain
`Object`/`def`-typed property, and those are shared across every compilation in
the JVM, not scoped to one thread the way a normal node is). Landed in
078ac9da53, tightened further in c3a4d9235b after a
n adversarial self-review turned up the synchronization gap.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtils.java:
##########
@@ -47,7 +49,28 @@
* @since 6.1
*/
public class AstPropertyResolveUtils {
- protected static Map<String, Map<String, ClassNode>> cachedClassProperties
= new HashMap<>();
+
+ /**
+ * Cache of resolved properties per {@link ClassNode}.
+ * <p>
+ * Keyed by {@code ClassNode} identity rather than name. {@link
ClassNode#equals(Object)} and
+ * {@link ClassNode#hashCode()} compare by {@link ClassNode#getText()}
(essentially the class
+ * name), so a {@code Map} keyed by name - or even by {@code ClassNode}
itself as the map key -
+ * treats any two distinct {@code ClassNode} instances that happen to
share a name as the same
+ * cache entry. That collision is a real hazard for classes compiled
without a package (common
+ * in tests and dynamically generated sources), and for the same source
compiled more than once
+ * in separate {@code GroovyClassLoader}s: each compilation produces its
own {@code ClassNode}
+ * instance that must never share cached property data with another
compilation's instance of a
+ * same-named class. An {@link IdentityHashMap} avoids that collision
entirely by comparing keys
+ * with {@code ==} instead of {@code equals()}.
+ * <p>
+ * Wrapped in {@link Collections#synchronizedMap(Map)} because AST
transforms that populate and
+ * read this cache can run concurrently on multiple threads (e.g. parallel
test execution within
+ * one JVM/fork); a plain, unsynchronized {@link HashMap} is not safe for
concurrent structural
+ * modification and can corrupt its internal state under concurrent {@code
put()} calls.
+ */
+ protected static final Map<ClassNode, Map<String, ClassNode>>
cachedClassProperties =
Review Comment:
Went further than private + `clearCache()` - the static map is gone entirely
(078ac9da53), so there's no field left to expose or protect. That's still
technically a breaking removal for anyone who referenced the old `protected`
field directly, just a cleaner one than a silent type change. Given it's an
internal implementation-detail field with no getter/documented extension use,
I'm inclined to leave it out of the upgrade guide, but flagging it in case
there's a reason to cover it there.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtils.java:
##########
@@ -47,7 +49,28 @@
* @since 6.1
*/
public class AstPropertyResolveUtils {
- protected static Map<String, Map<String, ClassNode>> cachedClassProperties
= new HashMap<>();
+
+ /**
+ * Cache of resolved properties per {@link ClassNode}.
+ * <p>
+ * Keyed by {@code ClassNode} identity rather than name. {@link
ClassNode#equals(Object)} and
+ * {@link ClassNode#hashCode()} compare by {@link ClassNode#getText()}
(essentially the class
+ * name), so a {@code Map} keyed by name - or even by {@code ClassNode}
itself as the map key -
+ * treats any two distinct {@code ClassNode} instances that happen to
share a name as the same
+ * cache entry. That collision is a real hazard for classes compiled
without a package (common
+ * in tests and dynamically generated sources), and for the same source
compiled more than once
+ * in separate {@code GroovyClassLoader}s: each compilation produces its
own {@code ClassNode}
+ * instance that must never share cached property data with another
compilation's instance of a
+ * same-named class. An {@link IdentityHashMap} avoids that collision
entirely by comparing keys
+ * with {@code ==} instead of {@code equals()}.
+ * <p>
+ * Wrapped in {@link Collections#synchronizedMap(Map)} because AST
transforms that populate and
+ * read this cache can run concurrently on multiple threads (e.g. parallel
test execution within
+ * one JVM/fork); a plain, unsynchronized {@link HashMap} is not safe for
concurrent structural
+ * modification and can corrupt its internal state under concurrent {@code
put()} calls.
Review Comment:
You're right, fixed (078ac9da53). The rationale no longer claims parallel
test forks race on this - confirmed against `gradle/test-config.gradle` that
`maxParallelForks` really does fork separate JVMs and this build never enables
JUnit's in-JVM parallel execution, so that was never a real race. Rewrote the
javadoc around the actual exposure instead: interned `ClassHelper` singletons
(`OBJECT_TYPE`, `STRING_TYPE`, etc.) that any concurrently-running compilation
in the same JVM could resolve to and touch through this cache - which is also
why access is now synchronized per-node (c3a4d9235b) rather than resting on
"only one thread ever touches a given node," which turned out not to be
universally true.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtils.java:
##########
@@ -94,22 +117,25 @@ public static List<String> getPropertyNames(ClassNode
classNode) {
}
private static Map<String, ClassNode> getPropertiesFromCache(ClassNode
classNode) {
- String className = classNode.getName();
- Map<String, ClassNode> cachedProperties =
cachedClassProperties.get(className);
+ Map<String, ClassNode> cachedProperties =
cachedClassProperties.get(classNode);
if (cachedProperties == null) {
- cachedProperties = new HashMap<>();
+ Map<String, ClassNode> newProperties = new HashMap<>();
boolean isDomainClass = AstUtils.isDomainClass(classNode);
if (isDomainClass) {
- cachedProperties.put(GormProperties.IDENTITY, new
ClassNode(Long.class));
- cachedProperties.put(GormProperties.VERSION, new
ClassNode(Long.class));
+ newProperties.put(GormProperties.IDENTITY, new
ClassNode(Long.class));
+ newProperties.put(GormProperties.VERSION, new
ClassNode(Long.class));
}
- cachedClassProperties.put(className, cachedProperties);
ClassNode currentNode = classNode;
while (currentNode != null &&
!currentNode.equals(ClassHelper.OBJECT_TYPE)) {
- populatePropertiesForClassNode(currentNode, cachedProperties,
isDomainClass, !isDomainClass);
+ populatePropertiesForClassNode(currentNode, newProperties,
isDomainClass, !isDomainClass);
currentNode = currentNode.getSuperClass();
}
- } return cachedProperties;
+ // Publish only once fully populated so a concurrent reader can
never observe a
+ // partially-populated entry for this ClassNode.
+ cachedProperties = newProperties;
+ cachedClassProperties.put(classNode, cachedProperties);
Review Comment:
`getNodeMetaData(key, fn)` uses `computeIfAbsent` internally (078ac9da53),
and I additionally wrapped the call in `synchronized (cacheHolder)`
(c3a4d9235b) after an adversarial self-review flagged that the backing
`ListHashMap` is explicitly documented as not thread-safe, and that some real
callers can land on interned singleton nodes (`ClassHelper.OBJECT_TYPE` etc.)
that more than one concurrent compilation could reach - the "only one thread
per node" assumption doesn't hold for those. See the javadoc for the precise
scope of what the synchronization does and doesn't cover.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtils.java:
##########
@@ -94,22 +117,25 @@ public static List<String> getPropertyNames(ClassNode
classNode) {
}
private static Map<String, ClassNode> getPropertiesFromCache(ClassNode
classNode) {
- String className = classNode.getName();
- Map<String, ClassNode> cachedProperties =
cachedClassProperties.get(className);
+ Map<String, ClassNode> cachedProperties =
cachedClassProperties.get(classNode);
if (cachedProperties == null) {
- cachedProperties = new HashMap<>();
+ Map<String, ClassNode> newProperties = new HashMap<>();
boolean isDomainClass = AstUtils.isDomainClass(classNode);
if (isDomainClass) {
- cachedProperties.put(GormProperties.IDENTITY, new
ClassNode(Long.class));
- cachedProperties.put(GormProperties.VERSION, new
ClassNode(Long.class));
+ newProperties.put(GormProperties.IDENTITY, new
ClassNode(Long.class));
+ newProperties.put(GormProperties.VERSION, new
ClassNode(Long.class));
}
- cachedClassProperties.put(className, cachedProperties);
ClassNode currentNode = classNode;
while (currentNode != null &&
!currentNode.equals(ClassHelper.OBJECT_TYPE)) {
- populatePropertiesForClassNode(currentNode, cachedProperties,
isDomainClass, !isDomainClass);
+ populatePropertiesForClassNode(currentNode, newProperties,
isDomainClass, !isDomainClass);
currentNode = currentNode.getSuperClass();
}
Review Comment:
Wanted to actually test this rather than argue about it. I reverted to the
pre-fix cache and ran the exact "same source compiled twice" scenario
`WhereQueryClosureCaptureSpec`'s second test exercises, 40 times sequentially
in one JVM (simulating many test classes sharing one fork's static state) - it
never diverged, even with the old buggy cache. So I can't point to a repro that
pins the ~1% CI flake specifically to the name/identity-collision mechanism,
and you may well be right that it isn't the actual trigger. I'm keeping the
identity-safety fix regardless, since it's an independently real, provable bug
on its own terms (the spec's second test shows two distinct, differently-shaped
`ClassNode`s that happen to share a name getting their properties conflated
under the old cache) - but I want to be upfront that I haven't confirmed it's
*the* flake's cause, only that it's *a* real bug. I'll be watching the
flaky-test dashboard (#16030) after this merges to see whether `WhereQueryCl
osureCaptureSpec` actually clears.
On the `isResolved()` snapshot mechanism specifically, I checked the Groovy
5.0.7 source directly: `ClassNode.clazz` has no setter anywhere outside the
`ClassNode(Class)` constructor, and `setRedirect()` throws a `GroovyBugError`
if called on a *primary* node - which is what every real caller of this utility
passes in, since they're all resolving a class mid-compilation. So for the
nodes this cache actually serves, `redirect()` is simply the node itself for
its entire life, and `isResolved()` can't flip after the node is first cached.
I don't think the staleness you're describing can occur for this code path as
it's actually used, though I agree the "cache once, forever" design would be
fragile if it could - documented the reasoning (and the primary-node guarantee
it leans on) in the class javadoc.
##########
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtilsSpec.groovy:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.grails.datastore.gorm.transform
+
+import java.lang.reflect.Modifier
+
+import org.codehaus.groovy.ast.ClassHelper
+import org.codehaus.groovy.ast.ClassNode
+import spock.lang.Specification
+
+/**
+ * {@link AstPropertyResolveUtils} caches resolved property metadata in a
static, process-wide
+ * map keyed by {@link ClassNode}. Two distinct compilations (e.g. the same
source parsed in two
+ * different {@code GroovyClassLoader}s, as happens for dynamically-generated
sources and in
+ * tests) produce distinct {@code ClassNode} instances that can legitimately
share the exact same
+ * name - {@code ClassNode#equals(Object)} compares by name, so a naive name-
or equals()-based
+ * cache key would conflate them, corrupting the resolved properties of one
class with those of
+ * an unrelated class that happens to share its name. This spec proves the
cache keys strictly by
+ * {@code ClassNode} identity, so same-named-but-distinct class nodes never
contaminate each
+ * other's cached property data.
+ */
+class AstPropertyResolveUtilsSpec extends Specification {
Review Comment:
Added both. Domain-class coverage now includes identity/version injection
and `hasMany`/`belongsTo`/`hasOne` resolved two ways: via AST initial
expressions (a hand-built `ClassNode` with a crafted `MapExpression`, matching
what `static hasMany = [...]` compiles to) and via reflection on an
already-resolved class (compiled through `GroovyClassLoader.parseClass` then
re-wrapped with `ClassHelper.make()`, since that's the only way to get
`isResolved() == true` for that branch).
Also added two concurrency tests: many threads each resolving their own
distinct, identically-named `ClassNode` (proves the identity-collision-freedom
property holds under concurrent load, not just sequentially), and - after an
adversarial self-review pointed out the first test can't exercise any real
race, since nothing is shared between the threads - a second test where 32
threads resolve the exact same shared `ClassNode` concurrently, which is what
the `synchronized` fix mentioned above actually protects. Worth being honest
about that second test's limits: I checked whether it fails without the
`synchronized` guard, and it didn't, in 8 runs - the cached computation is
deterministic and idempotent, so a black-box return-value test can't reliably
force the underlying race into an observably wrong result. Said that directly
in the test's comment rather than overclaiming what it proves; the
synchronization is justified by `ListHashMap`'s own "not thread-safe"
documentation, not by
this test having caught a live bug.
##########
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtilsSpec.groovy:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.grails.datastore.gorm.transform
+
+import java.lang.reflect.Modifier
+
+import org.codehaus.groovy.ast.ClassHelper
+import org.codehaus.groovy.ast.ClassNode
+import spock.lang.Specification
+
+/**
+ * {@link AstPropertyResolveUtils} caches resolved property metadata in a
static, process-wide
+ * map keyed by {@link ClassNode}. Two distinct compilations (e.g. the same
source parsed in two
+ * different {@code GroovyClassLoader}s, as happens for dynamically-generated
sources and in
+ * tests) produce distinct {@code ClassNode} instances that can legitimately
share the exact same
+ * name - {@code ClassNode#equals(Object)} compares by name, so a naive name-
or equals()-based
+ * cache key would conflate them, corrupting the resolved properties of one
class with those of
+ * an unrelated class that happens to share its name. This spec proves the
cache keys strictly by
+ * {@code ClassNode} identity, so same-named-but-distinct class nodes never
contaminate each
+ * other's cached property data.
+ */
+class AstPropertyResolveUtilsSpec extends Specification {
+
+ void "property lookups for two same-named ClassNodes in different packages
do not corrupt each other"() {
+ given: 'two distinct ClassNodes with the same simple name declared in
different packages'
+ ClassNode first = new ClassNode('org.example.one.Widget',
Modifier.PUBLIC, ClassHelper.OBJECT_TYPE)
+ first.addProperty('color', Modifier.PUBLIC, ClassHelper.STRING_TYPE,
null, null, null)
+
+ ClassNode second = new ClassNode('org.example.two.Widget',
Modifier.PUBLIC, ClassHelper.OBJECT_TYPE)
+ second.addProperty('weight', Modifier.PUBLIC,
ClassHelper.Integer_TYPE, null, null, null)
+
+ when: 'the first class node is resolved, populating its cache entry'
+ List<String> firstProperties =
AstPropertyResolveUtils.getPropertyNames(first)
+
+ then: 'only its own property is resolved'
+ firstProperties.contains('color')
+ !firstProperties.contains('weight')
+
+ when: 'the second, differently-packaged, same-simple-name class node
is resolved'
+ List<String> secondProperties =
AstPropertyResolveUtils.getPropertyNames(second)
+
+ then: 'its own property is resolved, not leaked from the first class
node'
+ secondProperties.contains('weight')
+ !secondProperties.contains('color')
+
+ and: 'the first class node cache entry remains unaffected by resolving
the second'
+ List<String> firstPropertiesAfter =
AstPropertyResolveUtils.getPropertyNames(first)
+ firstPropertiesAfter.contains('color')
+ !firstPropertiesAfter.contains('weight')
+ }
+
+ void "property lookups for two distinct ClassNode instances with the exact
same unqualified name do not corrupt each other"() {
+ given: 'two distinct ClassNode instances - as produced by two separate
compilations - sharing an identical unqualified name'
+ ClassNode first = new ClassNode('Widget', Modifier.PUBLIC,
ClassHelper.OBJECT_TYPE)
+ first.addProperty('color', Modifier.PUBLIC, ClassHelper.STRING_TYPE,
null, null, null)
+
+ ClassNode second = new ClassNode('Widget', Modifier.PUBLIC,
ClassHelper.OBJECT_TYPE)
+ second.addProperty('weight', Modifier.PUBLIC,
ClassHelper.Integer_TYPE, null, null, null)
+
+ expect: 'the two ClassNode instances compare equal by name - the exact
condition that would collide in a name-keyed or equals()-keyed cache'
+ first == second
+ first.hashCode() == second.hashCode()
+ !first.is(second)
+
+ when: 'both class nodes are resolved'
+ List<String> firstProperties =
AstPropertyResolveUtils.getPropertyNames(first)
+ List<String> secondProperties =
AstPropertyResolveUtils.getPropertyNames(second)
+
+ then: 'each keeps its own, independently-resolved properties despite
comparing equal'
+ firstProperties.contains('color')
+ !firstProperties.contains('weight')
+ secondProperties.contains('weight')
+ !secondProperties.contains('color')
+ }
+
+ void "getPropertyType resolves and caches the type of a declared
property"() {
+ given: 'a class node with a declared property'
+ ClassNode classNode = new ClassNode('org.example.PropertyTypeWidget',
Modifier.PUBLIC, ClassHelper.OBJECT_TYPE)
+ classNode.addProperty('label', Modifier.PUBLIC,
ClassHelper.STRING_TYPE, null, null, null)
+
+ expect: 'the resolved property type matches the declared type, both on
first (cache-populating) and second (cache-hit) lookup'
+ AstPropertyResolveUtils.getPropertyType(classNode, 'label') ==
ClassHelper.STRING_TYPE
+ AstPropertyResolveUtils.getPropertyType(classNode, 'label') ==
ClassHelper.STRING_TYPE
Review Comment:
Replaced it. The new test adds a second property to the `ClassNode` after
the first `getPropertyNames` lookup and asserts the second lookup doesn't see
it - `getPropertyNames` has no live-fallback path (unlike `getPropertyType`,
which falls through to a direct `classNode.getProperty()` lookup when the cache
doesn't contain the key), so this actually proves the result was cached rather
than recomputed. It also asserts, via a direct `classNode.getProperty('extra')
!= null` check, that the property really was added to the underlying node - so
the test demonstrates staleness specifically, not just a lookup that happens to
return nothing.
##########
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtilsSpec.groovy:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.grails.datastore.gorm.transform
+
+import java.lang.reflect.Modifier
+
+import org.codehaus.groovy.ast.ClassHelper
+import org.codehaus.groovy.ast.ClassNode
+import spock.lang.Specification
+
+/**
+ * {@link AstPropertyResolveUtils} caches resolved property metadata in a
static, process-wide
+ * map keyed by {@link ClassNode}. Two distinct compilations (e.g. the same
source parsed in two
+ * different {@code GroovyClassLoader}s, as happens for dynamically-generated
sources and in
+ * tests) produce distinct {@code ClassNode} instances that can legitimately
share the exact same
+ * name - {@code ClassNode#equals(Object)} compares by name, so a naive name-
or equals()-based
+ * cache key would conflate them, corrupting the resolved properties of one
class with those of
+ * an unrelated class that happens to share its name. This spec proves the
cache keys strictly by
+ * {@code ClassNode} identity, so same-named-but-distinct class nodes never
contaminate each
+ * other's cached property data.
+ */
+class AstPropertyResolveUtilsSpec extends Specification {
+
+ void "property lookups for two same-named ClassNodes in different packages
do not corrupt each other"() {
+ given: 'two distinct ClassNodes with the same simple name declared in
different packages'
+ ClassNode first = new ClassNode('org.example.one.Widget',
Modifier.PUBLIC, ClassHelper.OBJECT_TYPE)
+ first.addProperty('color', Modifier.PUBLIC, ClassHelper.STRING_TYPE,
null, null, null)
+
+ ClassNode second = new ClassNode('org.example.two.Widget',
Modifier.PUBLIC, ClassHelper.OBJECT_TYPE)
+ second.addProperty('weight', Modifier.PUBLIC,
ClassHelper.Integer_TYPE, null, null, null)
+
+ when: 'the first class node is resolved, populating its cache entry'
+ List<String> firstProperties =
AstPropertyResolveUtils.getPropertyNames(first)
+
+ then: 'only its own property is resolved'
+ firstProperties.contains('color')
+ !firstProperties.contains('weight')
+
+ when: 'the second, differently-packaged, same-simple-name class node
is resolved'
+ List<String> secondProperties =
AstPropertyResolveUtils.getPropertyNames(second)
+
+ then: 'its own property is resolved, not leaked from the first class
node'
+ secondProperties.contains('weight')
+ !secondProperties.contains('color')
+
+ and: 'the first class node cache entry remains unaffected by resolving
the second'
+ List<String> firstPropertiesAfter =
AstPropertyResolveUtils.getPropertyNames(first)
+ firstPropertiesAfter.contains('color')
+ !firstPropertiesAfter.contains('weight')
+ }
+
+ void "property lookups for two distinct ClassNode instances with the exact
same unqualified name do not corrupt each other"() {
+ given: 'two distinct ClassNode instances - as produced by two separate
compilations - sharing an identical unqualified name'
+ ClassNode first = new ClassNode('Widget', Modifier.PUBLIC,
ClassHelper.OBJECT_TYPE)
+ first.addProperty('color', Modifier.PUBLIC, ClassHelper.STRING_TYPE,
null, null, null)
+
+ ClassNode second = new ClassNode('Widget', Modifier.PUBLIC,
ClassHelper.OBJECT_TYPE)
+ second.addProperty('weight', Modifier.PUBLIC,
ClassHelper.Integer_TYPE, null, null, null)
+
+ expect: 'the two ClassNode instances compare equal by name - the exact
condition that would collide in a name-keyed or equals()-keyed cache'
+ first == second
+ first.hashCode() == second.hashCode()
Review Comment:
Done - trimmed to just `!first.is(second)`, with a comment explaining why
the `equals()`/`hashCode()` equality matters (it's the exact collision
condition a name- or `equals()`-keyed cache would hit) rather than asserting on
Groovy's own `ClassNode` equality contract.
--
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]
