Copilot commented on code in PR #15957: URL: https://github.com/apache/grails-core/pull/15957#discussion_r3560648050
########## grails-spring/src/main/groovy/grails/spring/BeanRegistryAdapter.java: ########## @@ -0,0 +1,40 @@ +/* + * 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 grails.spring; + +import java.util.Map; + +import groovy.lang.Closure; + +import org.springframework.beans.factory.config.BeanDefinition; +import org.springframework.beans.factory.support.BeanDefinitionRegistry; + +/** + * Experimental adapter seam for the BeanBuilder to Spring BeanRegistrar transition. + * + * @since 8.1 + */ +public interface BeanRegistryAdapter { Review Comment: The new public type name `grails.spring.BeanRegistryAdapter` collides with Spring’s existing `org.springframework.beans.factory.support.BeanRegistryAdapter`, which Grails already imports in core bootstrapping code (e.g. `grails/boot/config/GrailsEarlyPluginRegistrationPostProcessor.java:33` and `grails/boot/config/GrailsApplicationPostProcessor.groovy:32`). This makes imports ambiguous/confusing for framework and plugin code that interacts with Spring’s BeanRegistrar APIs and this new Grails adapter in the same file. Consider renaming this interface (and implementation/docs/tests) to something Grails-specific (e.g. `GrailsBeanRegistryAdapter` / `BeanBuilderRegistryAdapter`). ########## grails-spring/src/main/groovy/grails/spring/BeanBuilderBeanRegistryAdapter.java: ########## @@ -0,0 +1,64 @@ +/* + * 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 grails.spring; + +import java.util.Map; + +import groovy.lang.Closure; + +import org.springframework.beans.factory.config.BeanDefinition; +import org.springframework.beans.factory.support.BeanDefinitionRegistry; + +/** + * BeanRegistryAdapter backed by the existing BeanBuilder implementation. + * + * @since 8.1 + */ +public class BeanBuilderBeanRegistryAdapter implements BeanRegistryAdapter { + + private final BeanBuilder beanBuilder; + + public BeanBuilderBeanRegistryAdapter() { + this(new BeanBuilder()); + } + + public BeanBuilderBeanRegistryAdapter(BeanBuilder beanBuilder) { + this.beanBuilder = beanBuilder; + } Review Comment: Constructor accepts a BeanBuilder but does not validate it. Passing null would lead to a later NullPointerException with little context; BeanBuilder in this module consistently uses Assert.notNull for public setters/DSL entry points (e.g., BeanBuilder#setResourcePatternResolver). ########## grails-doc/src/en/guide/upgrading/beanRegistryAdapter.adoc: ########## @@ -0,0 +1,7 @@ +=== BeanBuilder to BeanRegistry strategy + +Grails 8.1 introduces an experimental `grails.spring.BeanRegistryAdapter` seam as the first step toward the Spring 7 `BeanRegistrar` direction tracked in issue #15824. +The initial `BeanBuilderBeanRegistryAdapter` delegates to the existing `BeanBuilder`, so `resources.groovy`, plugin `doWithSpring`, and existing BeanBuilder APIs continue to work unchanged. + +The intended migration path is to move call sites behind the adapter first, then add a Spring 7 BeanRegistrar-backed implementation once the framework integration points are ready. +BeanBuilder is not removed or behaviorally changed by this seed. Review Comment: This new Asciidoc file is missing the standard ASF license header block that the other upgrading guide pages include (e.g. `upgrading80x.adoc`). Also, the sentence “tracked in issue #15824” is misleading here because #15824 is the related PR about BeanBuilder XML/namespace laziness, not the tracking issue for the BeanRegistrar transition—rephrase to a weaker “see PR … for related work” (or link to the actual tracking issue if different). ########## grails-spring/src/main/groovy/grails/spring/BeanRegistryAdapter.java: ########## @@ -0,0 +1,40 @@ +/* + * 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 grails.spring; + +import java.util.Map; + +import groovy.lang.Closure; + +import org.springframework.beans.factory.config.BeanDefinition; +import org.springframework.beans.factory.support.BeanDefinitionRegistry; + +/** + * Experimental adapter seam for the BeanBuilder to Spring BeanRegistrar transition. + * + * @since 8.1 + */ Review Comment: PR description/checklist says this targets `8.0.x`, but the new public API and docs are marked as `@since 8.1` (and the upgrade guide text starts with “Grails 8.1 introduces…”). Please align the intended target version: either update the `@since`/docs to match 8.0.x, or update the PR metadata/branch targeting to reflect an 8.1 introduction. -- 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]
