codeconsole commented on code in PR #16292:
URL: https://github.com/apache/grails-core/pull/16292#discussion_r3929274419
##########
grails-beans-dsl/src/main/java/org/grails/compiler/beans/GrailsBeansASTTransformation.java:
##########
@@ -878,6 +890,118 @@ private void processBeanStatement(ClassNode classNode,
MethodCallExpression oute
classNode.addMethod(beanMethod);
}
+ // The methods this block just generated, in declaration order: everything
on the host that was
+ // not there before the two processing loops ran. MethodNode does not
override equals, so the
+ // removal is by identity and cannot drop a same-signature method the user
wrote.
+ private List<MethodNode> generatedMembers(ClassNode host, List<MethodNode>
preExisting) {
+ List<MethodNode> generated = new ArrayList<>(host.getMethods());
+ generated.removeAll(preExisting);
+ return generated;
+ }
+
+ /**
+ * Rejects a call from one generated method to another generated {@code
@Bean} method, on a host
+ * whose bean methods Spring does not proxy.
+ *
+ * <p>Calling a sibling {@code @Bean} method and getting the singleton
back is a CGLIB trick, and
+ * Spring only plays it for a full {@code @Configuration} class. On a
<i>lite</i> configuration
+ * source the same call is a plain Java call that constructs a second
instance - and lite is the
+ * common case for this DSL: {@code @AutoConfiguration} is
+ * {@code @Configuration(proxyBeanMethods = false)}, the sibling generated
for a plugin descriptor
+ * carries exactly that, and a Grails {@code Application} class is a
configuration source without
+ * being annotated {@code @Configuration} at all.</p>
+ *
+ * <p>Nothing about that failure is visible at runtime. The context
starts, every bean exists, and
+ * two objects live where the author meant one - so a listener registers
on the wrong instance, or
+ * configuration applied to one is missing from the other. It is also the
exact mistake a
+ * migration invites, since moving bean methods off a real {@code
@Configuration} class into this
+ * DSL silently changes what those calls mean.</p>
+ */
+ private void rejectUnproxiedSiblingBeanCalls(ClassNode host,
List<MethodNode> generated, SourceUnit source) {
+ Map<String, MethodNode> beanMethodsByName = new LinkedHashMap<>();
+ for (MethodNode method : generated) {
+ if
(!method.getAnnotations(ClassHelper.make(Bean.class)).isEmpty()) {
+ beanMethodsByName.put(method.getName(), method);
+ }
+ }
+ if (beanMethodsByName.isEmpty() || beanMethodsAreProxied(host)) {
Review Comment:
Valid, fixed in c2768dd.
Confirmed the premise behaviourally rather than taking it on trust: a
hand-written `@Configuration` with a static `@Bean` and a sibling call hands
out two distinct instances. That test is now in the spec, with no DSL involved
— it is the premise the whole rule rests on, so it should fail loudly if Spring
ever changes it.
Took your suggested shape: the bean-method map is built first and narrowed
rather than abandoned. On a proxied host it keeps only the static bean methods;
on any other host, all of them. A non-static sibling call on a full
`@Configuration` is still allowed, which is the exemption that host genuinely
earns.
The message now names the actual cause in the static case, since "is not a
proxied @Configuration class" would be untrue there:
> `"greeter(...)" is another bean declared in this block, and is declared
.staticMethod(). A static @Bean method is never intercepted by the container -
not even on a proxied @Configuration class like ProxiedStaticSiblingBeans -
because that interception is CGLIB subclassing, which cannot override a static
method, so this call does not return the bean Spring registered - it constructs
a second instance. …`
##########
grails-beans-dsl/src/main/java/org/grails/compiler/beans/GrailsBeansASTTransformation.java:
##########
@@ -878,6 +890,118 @@ private void processBeanStatement(ClassNode classNode,
MethodCallExpression oute
classNode.addMethod(beanMethod);
}
+ // The methods this block just generated, in declaration order: everything
on the host that was
+ // not there before the two processing loops ran. MethodNode does not
override equals, so the
+ // removal is by identity and cannot drop a same-signature method the user
wrote.
+ private List<MethodNode> generatedMembers(ClassNode host, List<MethodNode>
preExisting) {
+ List<MethodNode> generated = new ArrayList<>(host.getMethods());
+ generated.removeAll(preExisting);
+ return generated;
+ }
+
+ /**
+ * Rejects a call from one generated method to another generated {@code
@Bean} method, on a host
+ * whose bean methods Spring does not proxy.
+ *
+ * <p>Calling a sibling {@code @Bean} method and getting the singleton
back is a CGLIB trick, and
+ * Spring only plays it for a full {@code @Configuration} class. On a
<i>lite</i> configuration
+ * source the same call is a plain Java call that constructs a second
instance - and lite is the
+ * common case for this DSL: {@code @AutoConfiguration} is
+ * {@code @Configuration(proxyBeanMethods = false)}, the sibling generated
for a plugin descriptor
+ * carries exactly that, and a Grails {@code Application} class is a
configuration source without
+ * being annotated {@code @Configuration} at all.</p>
+ *
+ * <p>Nothing about that failure is visible at runtime. The context
starts, every bean exists, and
+ * two objects live where the author meant one - so a listener registers
on the wrong instance, or
+ * configuration applied to one is missing from the other. It is also the
exact mistake a
+ * migration invites, since moving bean methods off a real {@code
@Configuration} class into this
+ * DSL silently changes what those calls mean.</p>
+ */
+ private void rejectUnproxiedSiblingBeanCalls(ClassNode host,
List<MethodNode> generated, SourceUnit source) {
+ Map<String, MethodNode> beanMethodsByName = new LinkedHashMap<>();
+ for (MethodNode method : generated) {
+ if
(!method.getAnnotations(ClassHelper.make(Bean.class)).isEmpty()) {
+ beanMethodsByName.put(method.getName(), method);
+ }
+ }
+ if (beanMethodsByName.isEmpty() || beanMethodsAreProxied(host)) {
+ return;
+ }
+ for (MethodNode method : generated) {
+ if (method.getCode() == null) {
+ continue;
+ }
+ MethodNode caller = method;
+ method.getCode().visit(new CodeVisitorSupport() {
+ // Deliberately not descending. Inside a closure an
unqualified call is resolved
+ // against the delegate first, so `new Registry().tap {
initialize() }` calls the
+ // registry - not this class - even though the AST records
implicit-this either way.
+ // Reading that as a sibling bean call would reject working
code, which is a far
+ // worse trade than missing the rare bean call written inside
a nested closure.
+ @Override
+ public void visitClosureExpression(ClosureExpression
expression) {
+ }
+
+ @Override
+ public void visitMethodCallExpression(MethodCallExpression
call) {
+ super.visitMethodCallExpression(call);
+ if (!isSelfCall(call)) {
+ return;
+ }
+ MethodNode target =
beanMethodsByName.get(call.getMethodAsString());
+ if (target == null || target == caller) {
+ return;
+ }
+ addError(call, source, "\"" + call.getMethodAsString() +
"(...)\" is another bean declared " +
+ "in this block, and " +
host.getNameWithoutPackage() + " is not a proxied " +
+ "@Configuration class, so this call does not
return the bean Spring registered - it " +
+ "constructs a second instance. Inject it instead,
by declaring it as a parameter of " +
+ "this closure; if what you want is shared logic
rather than the bean, move it into a " +
+ "method(...) declaration.");
+ }
+ });
+ }
+ }
+
+ // An unqualified call, or one written against this. Anything with a real
receiver is somebody
+ // else's method that happens to share the name.
+ private boolean isSelfCall(MethodCallExpression call) {
+ return call.isImplicitThis() ||
+ (call.getObjectExpression() instanceof VariableExpression &&
+ ((VariableExpression)
call.getObjectExpression()).isThisExpression());
+ }
+
+ // Whether Spring will CGLIB-proxy this host's @Bean methods: true only
when @Configuration is
+ // reachable from the class's own annotations without passing through one
that sets
+ // proxyBeanMethods = false. @AutoConfiguration answers false through that
second clause - its
+ // meta-annotation is @Configuration(proxyBeanMethods = false) - and a
Grails Application class
+ // answers false by carrying no @Configuration at all.
+ private boolean beanMethodsAreProxied(ClassNode host) {
+ return proxiesBeanMethods(host.getAnnotations(), new HashSet<>());
+ }
+
+ private boolean proxiesBeanMethods(List<AnnotationNode> annotations,
Set<String> visited) {
+ for (AnnotationNode annotation : annotations) {
+ ClassNode type = annotation.getClassNode();
+ if (type.getName().startsWith("java.lang.annotation.") ||
!visited.add(type.getName())) {
Review Comment:
Valid, fixed in 6fbfd28.
You are right that it is wrong in principle regardless of how exotic the
triggering pair is: `visited` exists to stop a cycle in the meta-annotation
graph, and that only needs recording on the path that actually recurses.
Memoizing a branch that was pruned records an answer that was never computed.
Restructured exactly as you describe — the `@Configuration` test and the
`proxyBeanMethods = false` prune both happen before anything is recorded, and
only the descent records:
```java
if (type.getName().startsWith("java.lang.annotation.")) continue;
if (isFalseConstant(annotation.getMember(PROXY_BEAN_METHODS_MEMBER)))
continue;
if (Configuration.class.getName().equals(type.getName())) return true;
if (visited.add(type.getName()) && proxiesBeanMethods(type.getAnnotations(),
visited)) return true;
```
Regression test uses your `@AutoConfiguration @Configuration` pairing. I
reverted the walk to the old form and reran it to confirm it actually fails
without the fix, rather than assuming it does.
--
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]
