[
https://issues.apache.org/jira/browse/GROOVY-11928?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18073145#comment-18073145
]
ASF GitHub Bot commented on GROOVY-11928:
-----------------------------------------
Copilot commented on code in PR #2467:
URL: https://github.com/apache/groovy/pull/2467#discussion_r3072638311
##########
src/main/java/org/codehaus/groovy/transform/DelegateASTTransformation.java:
##########
@@ -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, getterName,
delegate.excludes, delegate.includes, allNames)) {
Review Comment:
The `willHaveIsAccessor` branch generates `isserName`, but the skip-check
passes `getterName` into `shouldSkipPropertyMethod`. This means
excludes/includes for the `isXxx` method name won't be honored. Pass
`isserName` instead so filtering applies to the actual generated method.
```suggestion
&& !shouldSkipPropertyMethod(prop, isserName,
delegate.excludes, delegate.includes, allNames)) {
```
##########
src/main/java/groovy/lang/MetaClassImpl.java:
##########
@@ -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.getCachedField().isAnnotationPresent(Internal.class)) {
+ return true;
+ }
+ MetaMethod getter = mbp.getGetter();
+ if (getter instanceof CachedMethod cm &&
cm.getCachedMethod().isAnnotationPresent(Internal.class)) {
+ return true;
+ }
+ MetaMethod setter = mbp.getSetter();
+ if (setter instanceof CachedMethod cm &&
cm.getCachedMethod().isAnnotationPresent(Internal.class)) {
Review Comment:
`isMarkedInternal` uses `field.getCachedField()` / `cm.getCachedMethod()`
which both attempt to make members accessible. For methods you can avoid that
by using `CachedMethod#getAnnotation(Internal.class)` (doesn't call
`makeAccessible`); for fields consider a similar non-accessing annotation
lookup to keep `getProperties()` cheap and side-effect free.
```suggestion
if (field != null && field.getAnnotation(Internal.class) != null) {
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) {
```
##########
src/main/java/org/codehaus/groovy/transform/AbstractASTTransformation.java:
##########
@@ -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 "";
Review Comment:
`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).
```suggestion
throw new IllegalArgumentException("Unsupported AnnotatedNode type:
" + (node == null ? "null" : node.getClass().getName()));
```
##########
src/main/java/groovy/lang/MetaClassImpl.java:
##########
@@ -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.getCachedField().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))) {
Review Comment:
`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.
> Make MetaClassImpl.getProperties() respect @Internal
> ----------------------------------------------------
>
> Key: GROOVY-11928
> URL: https://issues.apache.org/jira/browse/GROOVY-11928
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> MetaClassImpl.getProperties() does not check for @Internal annotations on
> fields or getter/setter methods. This creates an inconsistency:
> * At compile time, BeanUtils.getAllProperties() already skips methods
> annotated with @Internal (since GROOVY-9081).
> * At runtime, MetaClassImpl.getProperties() does not check @Internal, so
> tools like JsonOutput.toJson(), println, and other
> property-introspection-based features expose internal properties.
> AST transforms that generate internal fields (often with $ in the name)
> currently rely on ad-hoc excludes logic or deemedInternalName() checks in
> each individual transform (@ToString, @EqualsAndHashCode, @Delegate, etc.).
> If getProperties() respected @Internal, these transforms could simply
> annotate generated fields/accessors with @Internal and the filtering would
> happen consistently everywhere — both at compile time and runtime.
> This supercedes GROOVY-11516 which looked for a simplistic solution for names
> containing "$" but never really solved all the cases.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
