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:
@/private/tmp/claude-502/-Users-walterduquedeestrada-IdeaProjects-grails-core/33d6ddff-34c7-4bf1-bdc6-769351571bbd/scratchpad/pr16034-replies/leak.md
##########
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:
@/private/tmp/claude-502/-Users-walterduquedeestrada-IdeaProjects-grails-core/33d6ddff-34c7-4bf1-bdc6-769351571bbd/scratchpad/pr16034-replies/breaking-field.md
##########
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:
@/private/tmp/claude-502/-Users-walterduquedeestrada-IdeaProjects-grails-core/33d6ddff-34c7-4bf1-bdc6-769351571bbd/scratchpad/pr16034-replies/javadoc-rationale.md
##########
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:
@/private/tmp/claude-502/-Users-walterduquedeestrada-IdeaProjects-grails-core/33d6ddff-34c7-4bf1-bdc6-769351571bbd/scratchpad/pr16034-replies/atomic-publish.md
##########
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:
@/private/tmp/claude-502/-Users-walterduquedeestrada-IdeaProjects-grails-core/33d6ddff-34c7-4bf1-bdc6-769351571bbd/scratchpad/pr16034-replies/root-cause.md
##########
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:
@/private/tmp/claude-502/-Users-walterduquedeestrada-IdeaProjects-grails-core/33d6ddff-34c7-4bf1-bdc6-769351571bbd/scratchpad/pr16034-replies/test-coverage.md
##########
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:
@/private/tmp/claude-502/-Users-walterduquedeestrada-IdeaProjects-grails-core/33d6ddff-34c7-4bf1-bdc6-769351571bbd/scratchpad/pr16034-replies/weak-cache-test.md
##########
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:
@/private/tmp/claude-502/-Users-walterduquedeestrada-IdeaProjects-grails-core/33d6ddff-34c7-4bf1-bdc6-769351571bbd/scratchpad/pr16034-replies/equals-hashcode.md
--
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]
