Skip to content

GROOVY-11928: Make MetaClassImpl.getProperties() respect @Internal #2467

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
paulk-asert wants to merge 1 commit into
base: master
Choose a base branch
from
+258 −24
Open

GROOVY-11928: Make MetaClassImpl.getProperties() respect @Internal #2467

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 22 additions & 3 deletions src/main/java/groovy/lang/MetaClassImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*/
package groovy.lang;

import groovy.transform.Internal;
import org.apache.groovy.internal.util.UncheckedThrow;
import org.apache.groovy.util.BeanUtils;
import org.apache.groovy.util.SystemUtil;
Expand Down Expand Up @@ -2253,13 +2254,15 @@ public List<MetaProperty> getProperties() {
// simply return the values of the metaproperty map as a List
List<MetaProperty> ret = new ArrayList<>(propertyMap.size());
for (MetaProperty mp : propertyMap.values()) {
if (mp instanceof CachedField) {
if (mp.isSynthetic()
if (mp instanceof CachedField cf) {
if (cf.isSynthetic()
|| cf.isAnnotationPresent(Internal.class)
// GROOVY-5169, GROOVY-9081, GROOVY-9103, GROOVY-10438, GROOVY-10555, et al.
|| (!permissivePropertyAccess && !checkAccessible(getClass(), ((CachedField) mp).getDeclaringClass(), mp.getModifiers(), false))) {
|| (!permissivePropertyAccess && !checkAccessible(getClass(), cf.getDeclaringClass(), cf.getModifiers(), false))) {
Comment on lines +2257 to +2261
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cf.getCachedField().isAnnotationPresent(Internal.class) forces CachedField#getCachedField() which calls ReflectionUtils.makeAccessibleInPrivilegedAction(field) (side-effect + overhead) even though we only need to read the annotation. Consider adding/using an annotation accessor on CachedField that does not call makeAccessible, and use that here to avoid unnecessary accessibility changes during property enumeration.

Copilot uses AI. Check for mistakes.
continue;
}
} else if (mp instanceof MetaBeanProperty mbp) {
if (isMarkedInternal(mbp)) continue;
// filter out extrinsic properties (DGM, ...)
boolean getter = true, setter = true;
MetaMethod getterMetaMethod = mbp.getGetter();
Expand Down Expand Up @@ -2291,6 +2294,22 @@ private static boolean canAccessLegally(final MetaMethod method) {
|| ((CachedMethod) method).canAccessLegally(MetaClassImpl.class);
}

private static boolean isMarkedInternal(final MetaBeanProperty mbp) {
CachedField field = mbp.getField();
if (field != null && field.isAnnotationPresent(Internal.class)) {
return true;
}
MetaMethod getter = mbp.getGetter();
if (getter instanceof CachedMethod cm && cm.getAnnotation(Internal.class) != null) {
return true;
}
MetaMethod setter = mbp.getSetter();
if (setter instanceof CachedMethod cm && cm.getAnnotation(Internal.class) != null) {
return true;
}
return false;
}

/**
* return null if nothing valid has been found, a MetaMethod (for getter always the case if not null) or
* a LinkedList&lt;MetaMethod&gt; if there are multiple setter
Expand Down
47 changes: 47 additions & 0 deletions src/main/java/org/apache/groovy/ast/tools/AnnotatedNodeUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,14 @@
package org.apache.groovy.ast.tools;

import groovy.transform.Generated;
import groovy.transform.Internal;
import org.codehaus.groovy.ast.AnnotatedNode;
import org.codehaus.groovy.ast.AnnotationNode;
import org.codehaus.groovy.ast.ClassHelper;
import org.codehaus.groovy.ast.ClassNode;
import org.codehaus.groovy.ast.FieldNode;
import org.codehaus.groovy.ast.MethodNode;
import org.codehaus.groovy.ast.PropertyNode;

import java.util.List;

Expand All @@ -31,6 +35,7 @@
*/
public class AnnotatedNodeUtils {
private static final ClassNode GENERATED_TYPE = ClassHelper.make(Generated.class);
private static final ClassNode INTERNAL_TYPE = ClassHelper.make(Internal.class);

private AnnotatedNodeUtils() { }

Expand All @@ -54,4 +59,46 @@ public static boolean hasAnnotation(final AnnotatedNode node, final ClassNode an
public static boolean isGenerated(final AnnotatedNode node) {
return hasAnnotation(node, GENERATED_TYPE);
}

/**
* Marks a node with the {@link Internal @Internal} annotation.
*
* @since 6.0.0
*/
public static <T extends AnnotatedNode> T markAsInternal(final T nodeToMark) {
if (!isInternal(nodeToMark)) {
nodeToMark.addAnnotation(new AnnotationNode(INTERNAL_TYPE));
}
return nodeToMark;
}

/**
* Checks whether a node is annotated with {@link Internal @Internal}.
*
* @since 6.0.0
*/
public static boolean isInternal(final AnnotatedNode node) {
return hasAnnotation(node, INTERNAL_TYPE);
}

/**
* Checks whether an AST node is deemed internal, either by name convention
* (contains {@code $}) or by being annotated with {@link Internal @Internal}.
*
* @since 6.0.0
*/
public static boolean deemedInternal(final AnnotatedNode node) {
if (isInternal(node)) return true;
String name;
if (node instanceof FieldNode fn) {
name = fn.getName();
} else if (node instanceof PropertyNode pn) {
name = pn.getName();
} else if (node instanceof MethodNode mn) {
name = mn.getName();
} else {
return false;
}
return name.contains("$");
}
}
10 changes: 10 additions & 0 deletions src/main/java/org/codehaus/groovy/reflection/CachedField.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,16 @@ public Class getDeclaringClass() {
return field.getDeclaringClass();
}

/**
* Checks whether the underlying field has the specified annotation.
* Unlike {@link #getCachedField()}, this does not trigger accessibility changes.
*
* @since 6.0.0
*/
public boolean isAnnotationPresent(Class<? extends java.lang.annotation.Annotation> annotationType) {
return field.isAnnotationPresent(annotationType);
}

/**
* {@inheritDoc}
*/
Expand Down
47 changes: 47 additions & 0 deletions src/main/java/org/codehaus/groovy/transform/AbstractASTTransformation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,10 +252,26 @@ public static List<String> tokenize(String rawExcludes) {
return rawExcludes == null ? new ArrayList<>() : StringGroovyMethods.tokenize(rawExcludes, ", ");
}

/**
* @see org.apache.groovy.ast.tools.AnnotatedNodeUtils#markAsInternal(AnnotatedNode)
* @since 6.0.0
*/
public static void markAsInternal(AnnotatedNode node) {
AnnotatedNodeUtils.markAsInternal(node);
}

public static boolean deemedInternalName(String name) {
return name.contains("$");
}

/**
* @see org.apache.groovy.ast.tools.AnnotatedNodeUtils#deemedInternal(AnnotatedNode)
* @since 6.0.0
*/
public static boolean deemedInternal(AnnotatedNode node) {
return AnnotatedNodeUtils.deemedInternal(node);
}

public static boolean shouldSkipUndefinedAware(String name, List<String> excludes, List<String> includes) {
return shouldSkipUndefinedAware(name, excludes, includes, false);
}
Expand All @@ -266,6 +282,18 @@ public static boolean shouldSkipUndefinedAware(String name, List<String> exclude
(includes != null && !includes.contains(name));
}

/**
* Variant that checks both the name and {@link groovy.transform.Internal @Internal} annotation.
*
* @since 6.0.0
*/
public static boolean shouldSkipUndefinedAware(AnnotatedNode node, List<String> excludes, List<String> includes, boolean allNames) {
String name = nodeName(node);
return (excludes != null && excludes.contains(name)) ||
(!allNames && deemedInternal(node)) ||
(includes != null && !includes.contains(name));
}

public static boolean shouldSkip(String name, List<String> excludes, List<String> includes) {
return shouldSkip(name, excludes, includes, false);
}
Expand All @@ -276,6 +304,25 @@ public static boolean shouldSkip(String name, List<String> excludes, List<String
(includes != null && !includes.isEmpty() && !includes.contains(name));
}

/**
* Variant that checks both the name and {@link groovy.transform.Internal @Internal} annotation.
*
* @since 6.0.0
*/
public static boolean shouldSkip(AnnotatedNode node, List<String> excludes, List<String> includes, boolean allNames) {
String name = nodeName(node);
return (excludes != null && excludes.contains(name)) ||
(!allNames && deemedInternal(node)) ||
(includes != null && !includes.isEmpty() && !includes.contains(name));
}

private static String nodeName(AnnotatedNode node) {
if (node instanceof FieldNode fn) return fn.getName();
if (node instanceof PropertyNode pn) return pn.getName();
if (node instanceof MethodNode mn) return mn.getName();
return "";
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nodeName returns an empty string for AnnotatedNode types other than FieldNode/PropertyNode/MethodNode, which can silently change skip behavior (e.g., includes/excludes checks on ""). Since shouldSkip*(AnnotatedNode, ...) are public APIs, consider throwing an IllegalArgumentException for unsupported node types (or explicitly handling the additional node types you intend to support).

Suggested change
return "";
throw new IllegalArgumentException("Unsupported AnnotatedNode type: " + (node == null ? "null" : node.getClass().getName()));

Copilot uses AI. Check for mistakes.
}

public static boolean shouldSkipOnDescriptorUndefinedAware(boolean checkReturn, Map genericsSpec, MethodNode mNode,
List<ClassNode> excludeTypes, List<ClassNode> includeTypes) {
String descriptor = mNode.getTypeDescriptor();
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/org/codehaus/groovy/transform/BuilderASTTransformation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ protected static List<PropertyInfo> getPropertyInfoFromClassNode(ClassNode cNode
protected static List<PropertyInfo> getPropertyInfoFromClassNode(ClassNode cNode, List<String> includes, List<String> excludes, boolean allNames) {
List<PropertyInfo> props = new ArrayList<>();
for (FieldNode fNode : getInstancePropertyFields(cNode)) {
if (shouldSkip(fNode.getName(), excludes, includes, allNames)) continue;
if (shouldSkip(fNode, excludes, includes, allNames)) continue;
props.add(new PropertyInfo(fNode.getName(), fNode.getType()));
}
return props;
Expand Down Expand Up @@ -223,12 +223,12 @@ protected List<PropertyInfo> getPropertyInfoFromClassNode(BuilderASTTransformati
List<PropertyInfo> props = new ArrayList<>();
List<String> seen = new ArrayList<>();
for (PropertyNode pNode : BeanUtils.getAllProperties(cNode, false, false, allProperties)) {
if (shouldSkip(pNode.getName(), excludes, includes, allNames)) continue;
if (shouldSkip(pNode, excludes, includes, allNames)) continue;
props.add(new PropertyInfo(pNode.getName(), pNode.getType()));
seen.add(pNode.getName());
}
for (FieldNode fNode : getFields(transform, anno, cNode)) {
if (seen.contains(fNode.getName()) || shouldSkip(fNode.getName(), excludes, includes, allNames)) continue;
if (seen.contains(fNode.getName()) || shouldSkip(fNode, excludes, includes, allNames)) continue;
props.add(new PropertyInfo(fNode.getName(), fNode.getType()));
}
return props;
Expand Down
13 changes: 7 additions & 6 deletions src/main/java/org/codehaus/groovy/transform/DelegateASTTransformation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ private static Collection<MethodNode> filterMethods(final Collection<MethodNode>
methods.removeIf(candidate -> {
if (!candidate.isPublic() || candidate.isStatic() || (candidate.getModifiers () & ACC_SYNTHETIC) != 0) return true;

if (shouldSkip(candidate.getName(), delegate.excludes, delegate.includes, allNames)) return true;
if (shouldSkip(candidate, delegate.excludes, delegate.includes, allNames)) return true;

if (!includeDeprecated && !candidate.getAnnotations(DEPRECATED_TYPE).isEmpty()) return true;

Expand Down Expand Up @@ -320,7 +320,7 @@ private static void addSetterIfNeeded(final DelegateDescription delegate, final
String setterName = getSetterName(name);
if (!prop.isFinal()
&& delegate.owner.getSetterMethod(setterName) == null && delegate.owner.getProperty(name) == null
&& !shouldSkipPropertyMethod(name, setterName, delegate.excludes, delegate.includes, allNames)) {
&& !shouldSkipPropertyMethod(prop, setterName, delegate.excludes, delegate.includes, allNames)) {
addGeneratedMethod(
delegate.owner,
setterName,
Expand Down Expand Up @@ -352,7 +352,7 @@ private static void addGetterIfNeeded(final DelegateDescription delegate, final
extractAccessorInfo(delegate.owner, name, ownerWillHaveGetAccessor, ownerWillHaveIsAccessor);

if (willHaveGetAccessor && !ownerWillHaveGetAccessor.get()
&& !shouldSkipPropertyMethod(name, getterName, delegate.excludes, delegate.includes, allNames)) {
&& !shouldSkipPropertyMethod(prop, getterName, delegate.excludes, delegate.includes, allNames)) {
addGeneratedMethod(
delegate.owner,
getterName,
Expand All @@ -365,7 +365,7 @@ private static void addGetterIfNeeded(final DelegateDescription delegate, final
}

if (willHaveIsAccessor && !ownerWillHaveIsAccessor.get()
&& !shouldSkipPropertyMethod(name, getterName, delegate.excludes, delegate.includes, allNames)) {
&& !shouldSkipPropertyMethod(prop, isserName, delegate.excludes, delegate.includes, allNames)) {
addGeneratedMethod(
delegate.owner,
isserName,
Expand All @@ -386,8 +386,9 @@ private static void extractAccessorInfo(final ClassNode owner, final String name
willHaveIsAccessor.set(hasIsAccessor || (prop != null && !hasGetAccessor && isPrimitiveBoolean(prop.getOriginType())));
}

private static boolean shouldSkipPropertyMethod(final String propertyName, final String methodName, final List<String> excludes, final List<String> includes, final boolean allNames) {
return ((!allNames && deemedInternalName(propertyName))
private static boolean shouldSkipPropertyMethod(final PropertyNode prop, final String methodName, final List<String> excludes, final List<String> includes, final boolean allNames) {
String propertyName = prop.getName();
return ((!allNames && deemedInternal(prop))
|| excludes != null && (excludes.contains(propertyName) || excludes.contains(methodName))
|| (includes != null && !includes.isEmpty() && !includes.contains(propertyName) && !includes.contains(methodName)));
}
Expand Down
14 changes: 8 additions & 6 deletions src/main/java/org/codehaus/groovy/transform/EqualsAndHashCodeASTTransformation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@
import static org.codehaus.groovy.ast.tools.GeneralUtils.varX;
import static org.codehaus.groovy.ast.tools.GenericsUtils.makeClassSafe;
import static org.codehaus.groovy.ast.tools.GenericsUtils.nonGeneric;
import static org.apache.groovy.ast.tools.AnnotatedNodeUtils.markAsInternal;
import static org.objectweb.asm.Opcodes.ACC_PRIVATE;
import static org.objectweb.asm.Opcodes.ACC_PUBLIC;
import static org.objectweb.asm.Opcodes.ACC_SYNTHETIC;
Expand Down Expand Up @@ -178,6 +179,7 @@ public static void createHashCode(ClassNode cNode, boolean cacheResult, boolean
// TODO use pList and fList
if (cacheResult) {
final FieldNode hashField = cNode.addField("$hash$code", ACC_PRIVATE | ACC_SYNTHETIC, ClassHelper.int_TYPE, null);
markAsInternal(hashField);
final Expression hash = varX(hashField);
body.addStatement(ifS(
isZeroX(hash),
Expand Down Expand Up @@ -217,7 +219,7 @@ private static Statement calculateHashStatementsDefault(ClassNode cNode, Express
body.addStatement(declS(result, callX(HASHUTIL_TYPE, "initHash")));

for (PropertyNode pNode : pList) {
if (shouldSkipUndefinedAware(pNode.getName(), excludes, includes, allNames)) continue;
if (shouldSkipUndefinedAware(pNode, excludes, includes, allNames)) continue;
// _result = HashCodeHelper.updateHash(_result, getProperty()) // plus self-reference checking
Expression prop = useGetter ? getterThisX(cNode, pNode) : propX(varX("this"), pNode.getName());
final Expression current = callX(HASHUTIL_TYPE, UPDATE_HASH, args(result, prop));
Expand All @@ -227,7 +229,7 @@ private static Statement calculateHashStatementsDefault(ClassNode cNode, Express

}
for (FieldNode fNode : fList) {
if (shouldSkipUndefinedAware(fNode.getName(), excludes, includes, allNames)) continue;
if (shouldSkipUndefinedAware(fNode, excludes, includes, allNames)) continue;
// _result = HashCodeHelper.updateHash(_result, field) // plus self-reference checking
final Expression fieldExpr = varX(fNode);
final Expression current = callX(HASHUTIL_TYPE, UPDATE_HASH, args(result, fieldExpr));
Expand Down Expand Up @@ -259,15 +261,15 @@ private static Statement calculateHashStatementsPOJO(ClassNode cNode, Expression
final BlockStatement body = new BlockStatement();
final ArgumentListExpression args = new ArgumentListExpression();
for (PropertyNode pNode : pList) {
if (shouldSkipUndefinedAware(pNode.getName(), excludes, includes, allNames)) continue;
if (shouldSkipUndefinedAware(pNode, excludes, includes, allNames)) continue;
if (useGetter) {
args.addExpression(getterThisX(cNode, pNode));
} else {
args.addExpression(propX(varX("this"), pNode.getName()));
}
}
for (FieldNode fNode : fList) {
if (shouldSkipUndefinedAware(fNode.getName(), excludes, includes, allNames)) continue;
if (shouldSkipUndefinedAware(fNode, excludes, includes, allNames)) continue;
args.addExpression(varX(fNode));
}
if (callSuper) {
Expand Down Expand Up @@ -357,7 +359,7 @@ public static void createEquals(ClassNode cNode, boolean includeFields, boolean
final Set<String> names = new HashSet<>();
final List<PropertyNode> pList = getAllProperties(names, cNode, true, includeFields, allProperties, false, false, false);
for (PropertyNode pNode : pList) {
if (shouldSkipUndefinedAware(pNode.getName(), excludes, includes, allNames)) continue;
if (shouldSkipUndefinedAware(pNode, excludes, includes, allNames)) continue;
boolean canBeSelf = StaticTypeCheckingSupport.implementsInterfaceOrIsSubclassOf(
pNode.getOriginType(), cNode
);
Expand Down Expand Up @@ -385,7 +387,7 @@ public static void createEquals(ClassNode cNode, boolean includeFields, boolean
fList.addAll(getInstanceNonPropertyFields(cNode));
}
for (FieldNode fNode : fList) {
if (shouldSkipUndefinedAware(fNode.getName(), excludes, includes, allNames)) continue;
if (shouldSkipUndefinedAware(fNode, excludes, includes, allNames)) continue;
Expression fieldsEqual = pojo
? callX(OBJECTS_TYPE, EQUALS, args(varX(fNode), propX(otherTyped, fNode.getName())))
: hasEqualFieldX(fNode, otherTyped);
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/codehaus/groovy/transform/ImmutableASTTransformation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import groovy.transform.CompilationUnitAware;
import groovy.transform.ImmutableBase;
import groovy.transform.options.PropertyHandler;
import org.apache.groovy.ast.tools.AnnotatedNodeUtils;
import org.apache.groovy.ast.tools.ImmutablePropertyUtils;
import org.codehaus.groovy.ast.ASTNode;
import org.codehaus.groovy.ast.AnnotatedNode;
Expand Down Expand Up @@ -185,8 +186,7 @@ static boolean isSpecialNamedArgCase(final List<PropertyNode> list, final boolea

private static void ensureNotPublic(final AbstractASTTransformation xform, final String cNode, final FieldNode fNode) {
String fName = fNode.getName();
// TODO: Do we need to lock down things like: $ownClass?
if (fNode.isPublic() && !fName.contains("$") && !(fNode.isStatic() && fNode.isFinal())) {
if (fNode.isPublic() && !AnnotatedNodeUtils.deemedInternal(fNode) && !(fNode.isStatic() && fNode.isFinal())) {
xform.addError("Public field '" + fName + "' not allowed for " + MY_TYPE_NAME + " class '" + cNode + "'.", fNode);
}
}
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/org/codehaus/groovy/transform/LazyASTTransformation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*/
package org.codehaus.groovy.transform;

import org.apache.groovy.ast.tools.AnnotatedNodeUtils;
import org.codehaus.groovy.ast.ASTNode;
import org.codehaus.groovy.ast.AnnotatedNode;
import org.codehaus.groovy.ast.AnnotationNode;
Expand Down Expand Up @@ -95,6 +96,7 @@ static void visitField(ErrorCollecting xform, AnnotationNode node, FieldNode fie
String backingFieldName = "$" + fieldNode.getName();
fieldNode.rename(backingFieldName);
fieldNode.setModifiers(ACC_PRIVATE | ACC_SYNTHETIC | (fieldNode.getModifiers() & (~(ACC_PUBLIC | ACC_PROTECTED))));
AnnotatedNodeUtils.markAsInternal(fieldNode);
PropertyNode pNode = fieldNode.getDeclaringClass().getProperty(backingFieldName);
if (pNode != null) {
fieldNode.getDeclaringClass().getProperties().remove(pNode);
Expand Down
Loading
Loading