jdaugherty commented on code in PR #16094: URL: https://github.com/apache/grails-core/pull/16094#discussion_r3775962488
########## grails-common/src/main/groovy/org/grails/aot/RegistrableTypes.java: ########## @@ -0,0 +1,251 @@ +/* + * 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.aot; Review Comment: We should use `org.apache.grails.common.aot` so we are JPMS compatible on newly added code. ########## .sdkmanrc: ########## @@ -5,7 +5,7 @@ # $JAVA_VERSION_MICRONAUT in release.yml; for local verification, install that JDK 25 # alongside this one (sdk install java <version>-librca) and follow the dual-JDK # instructions in RELEASE.md "Manual Verification: Reproducible Jar Files". -java=21.0.7-librca +java=21.0.12-librca Review Comment: The JDK bump (here, `release.yml`, `release-publish-docs.yml`, `etc/bin/Dockerfile`) has nothing to do with AOT and changes the JDK the reproducible-build guarantee is pinned to. Please pull it into its own PR so it can be verified against `verify-reproducible.sh` independently — a release-toolchain change should not ride along in a feature branch. ########## grails-core/src/main/groovy/org/grails/plugins/CoreGrailsPlugin.groovy: ########## @@ -189,7 +237,12 @@ class CoreGrailsPlugin extends Plugin { } } - registry.registerBean('proxyHandler', DefaultProxyHandler) + // The GORM implementations register a proxy handler that knows how to unwrap their own + // proxies; this is the one for an application that has none. Registering it over theirs + // left a Hibernate application unwrapping Hibernate proxies with the general case. + if (!hasBeanDefinition(application, 'proxyHandler')) { Review Comment: This is a real bug fix (GORM's `proxyHandler` was being clobbered), but it is unrelated to AOT and it changes runtime behaviour for every Hibernate/Mongo/Neo4j application, not just AOT ones. It deserves its own PR and its own regression test asserting that a Hibernate context ends up with the Hibernate proxy handler. Two further concerns with the mechanism: - `hasBeanDefinition` consults `application.mainContext`, not the `BeanRegistry` being written to. The javadoc on `hasConfigurationClassPostProcessor` even describes the case where those differ ("a test slice registering this plugin's beans on a bare registry") — in that case the check answers about the wrong object. If `BeanRegistry` genuinely can't be queried, that limitation should be stated explicitly here rather than in the sibling method. - If `mainContext` is null or not a `ConfigurableApplicationContext` the method returns `false` and registers anyway, so the fix silently degrades to the old behaviour. Is `mainContext` guaranteed to be set by the time registrars run? ########## grails-core/src/main/groovy/org/grails/spring/beans/aot/AbstractBeanDefinitionExcludeFilter.java: ########## @@ -0,0 +1,48 @@ +/* + * 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.spring.beans.aot; Review Comment: Same point as my note on `RegistrableTypes`, and it applies to every new package in this PR (`org.grails.spring.beans.aot`, `org.grails.aot`, `org.grails.plugins.web.taglib.aot`, `org.grails.web.mapping.aot`, `org.grails.datastore.gorm.aot`, ...): new code should go under `org.apache.grails.*` so we stay JPMS-compatible. Worth fixing across the PR in one pass since these are all brand-new classes with no compatibility constraint. ########## grails-url-mappings/src/main/groovy/org/grails/plugins/web/mapping/UrlMappingsGrailsPlugin.groovy: ########## @@ -155,6 +156,25 @@ class UrlMappingsGrailsPlugin extends Plugin { } } + /** + * Whether the mappings are to be reloadable, which decides how the holder is defined. + * + * <p>Not while the code is being generated, whatever the machine generating it looks like. + * Reloading swaps the mappings behind a proxy, and the proxy produces its {@code UrlMappings} + * through a target source rather than declaring the type -- so Spring can only learn what it + * produces by building it, which is exactly what reading a generated definition avoids. + * Generated that way nothing could be autowired by that type, and the application did not + * start. An image cannot reload anything in any case.</p> + */ + protected boolean isReloadEnabled() { Review Comment: This makes `isReloadEnabled()` answer `false` whenever `AOT_PROCESSING` is set, which is exactly the condition the PR description says users must work around by setting `grails.env=production` on `processAot` — and `GrailsGradlePlugin.configureAheadOfTimeProcessing` now sets that automatically as well. So there are three overlapping mechanisms for the same problem. Which one is actually load-bearing? If this flag check is sufficient, the "Limitations" note in the description and the manual `systemProperty 'grails.env', 'production'` in `grails-test-examples/aot/build.gradle` are stale and should go. If it isn't sufficient, please document why. ########## grails-core/src/main/groovy/grails/boot/GrailsBanner.groovy: ########## @@ -363,10 +666,13 @@ class GrailsBanner implements Banner { /** * Enumeration of optional version options. + * + * <p>The container being run is shown by default under {@code container}, which is the one an + * application is on. These name a particular container instead, for an application that wants + * to be told about one whether or not it is the one serving.</p> */ @CompileStatic enum OptionalVersionOption { Review Comment: `SPRING_SECURITY` is removed from `OptionalVersionOption` and moved into `DefaultVersionOption`. `OptionalVersionOption` is a public enum on a public class — removing a constant is a source- and binary-breaking change for anything referencing it, and it isn't listed in `upgrading80x.adoc`. More generally: the whole banner rework in this file (ANSI colouring, the NATIVE/AOT CACHE mark, `CONTAINER`, the default-set changes) is a user-visible feature that is independent of making an application AOT-processable. Only `resolveMark()` needs the AOT work at all. Please split the banner into its own PR — it's the part most likely to attract bikeshedding, and holding the AOT fix behind it helps nobody. ########## grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/core/GrailsGradlePlugin.groovy: ########## @@ -1115,6 +1268,150 @@ ${importStatements} } } + /** + * Wires up the cache the JDK can write for an application, so the next start reads what a + * training run worked out rather than working it out again. + * + * <p>Three steps, because the cache is only usable against the layout it was trained on: the + * archive is extracted, the extracted application is run and asked for its pages, and what the + * run recorded is left beside it. An application asks for this with + * {@code grails.aotCache.enabled}, and says which of its pages matter.</p> + */ + protected void configureAotCache(Project project) { + AotCacheExtension extension = ((ExtensionAware) project.extensions.getByName('grails')) + .extensions.create('aotCache', AotCacheExtension) + extension.enabled.convention(false) + extension.paths.convention([]) + extension.jvmArguments.convention(['-Dspring.aot.enabled=true', '-Dgrails.env=production']) + extension.port.convention(TRAINING_PORT) + extension.startTimeoutSeconds.convention(TRAINING_START_TIMEOUT_SECONDS) + + project.pluginManager.withPlugin(SPRING_BOOT_PLUGIN) { + TaskProvider<?> bootJar = project.tasks.named('bootJar') + Provider<Directory> application = project.layout.buildDirectory.dir('aot-cache/application') + Provider<JavaLauncher> launcher = trainingLauncher(project) + + TaskProvider<Exec> extract = project.tasks.register('extractAotCacheApplication', Exec) { Exec task -> + task.group = BasePlugin.BUILD_GROUP + task.description = 'Extracts the application, which is the form the cache is read against' + task.onlyIf { extension.enabled.get() } + task.dependsOn(bootJar) + // Named so the extraction is skipped when the archive it came from has not moved, + // rather than repeated on every run because nothing said what it produced. + task.inputs.file(project.provider { archiveOf(bootJar) }) + task.outputs.dir(application) + task.doFirst { + File destination = application.get().asFile + project.delete(destination) Review Comment: `project.delete(...)` inside `doFirst` is `Project` access at execution time, which is a hard error under the Gradle configuration cache (Gradle 9). The surrounding `task.commandLine(...)` also captures `bootJar`/`project` state at execution time. Use `@Inject FileSystemOperations`/`ExecOperations` (or make this a proper task type like `TrainAotCacheTask` already is) and resolve the archive through a `Provider<RegularFile>` input rather than `archiveOf(bootJar)`, which calls `TaskProvider.get()` eagerly. Has this been run with `--configuration-cache`? A `Exec` task that can't be config-cached will fail builds that have it enabled, and the new test projects under `src/test/resources/test-projects` don't appear to exercise it. ########## dependencies.gradle: ########## @@ -126,8 +126,8 @@ ext { 'sitemesh.version' : '2.6.0', 'scribejava.version' : '8.3.3', 'spock.version' : '2.4-groovy-5.0', - 'starter-sitemesh.version' : '3.3.0-M3', - 'spring-webmvc-sitemesh.version': '3.3.0-M3', + 'starter-sitemesh.version' : '3.3.0-SNAPSHOT', Review Comment: Blocker for merge, as you note in the description. Beyond the pin itself, two things need resolving before this can land: 1. An ASF release cannot depend on a SNAPSHOT. This has to be a released `spring-webmvc-sitemesh`/`starter-sitemesh` before merge, not after. 2. `3.3.0-SNAPSHOT` is not resolvable from the repositories this build declares, so CI on this branch is not actually building what the PR claims. Anything green here should be treated as untested until the pin lands. Given the size of the rest of the change, I'd rather see the SiteMesh-dependent part (`GrailsSiteMeshViewResolver` + this bump) split out and merged after the upstream release, so the AOT work isn't held hostage to it. ########## grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/core/GrailsGradlePlugin.groovy: ########## @@ -836,6 +869,39 @@ ${importStatements} it.destinationDirectory = project.layout.buildDirectory.dir('assetCompile/assets') } } + configureAssetsOnTheClasspath(project) + } + + /** + * Packages the compiled assets where an executable jar can read them. + * + * <p>The asset pipeline plugin puts them at the root of whatever archive is built, which is + * where a war serves its web content from and is therefore right for a war. An executable jar + * has no web content: it serves assets by reading them off the classpath, and its classpath is + * {@code BOOT-INF/classes} -- so the same assets, at the same place, in a jar rather than a war, + * are packaged but unreachable, and every asset a page asks for is a 404 while the page itself + * renders. Adding them under the classpath directory is what makes them found.</p> + * + * <p>Only for {@code bootJar}. A war already serves them from the root, and putting them on its + * classpath as well would ship the same bytes twice.</p> + */ + private void configureAssetsOnTheClasspath(Project project) { + project.pluginManager.withPlugin(SPRING_BOOT_PLUGIN) { + // Read after the build script has run, and by the task the pipeline registers rather + // than by the plugin that registers it: the asset pipeline's plugin id has changed + // once already, and the task name has not. + project.afterEvaluate { Review Comment: `afterEvaluate` + `findByName` is the pattern this build has been trying to move away from. `project.tasks.withType(...)`/`named(...).configure { }` on the `assetCompile` task name, guarded by `pluginManager.withPlugin`, gets the same laziness without an ordering-sensitive callback. Also: this asset-packaging fix (`BOOT-INF/classes/assets`) is a genuine bug fix but is independent of AOT — please split it out. It has its own spec (`AssetClasspathPackagingSpec`) already, so it should be easy to land separately and quickly. ########## grails-gsp/plugin/src/main/groovy/org/grails/plugins/web/GroovyPagesGrailsPlugin.groovy: ########## @@ -166,27 +187,22 @@ class GroovyPagesGrailsPlugin extends Plugin { } } - def deployed = !Metadata.getCurrent().isDevelopmentEnvironmentAvailable() groovyPageLocator(CachingGrailsConventionGroovyPageLocator) { bean -> bean.lazyInit = true if (customResourceLoader) { resourceLoader = groovyPageResourceLoader } - if (deployed) { - Resource defaultViews = applicationContext?.getResource('gsp/views.properties') - - if (defaultViews != null) { - if (!defaultViews.exists()) { - defaultViews = applicationContext?.getResource('classpath:gsp/views.properties') - } - } - - if (defaultViews?.exists()) { - precompiledGspMap = { PropertiesFactoryBean pfb -> - ignoreResourceNotFound = true - locations = [defaultViews] as Resource[] - } - } + // Where the pages compiled at build time are listed. Attached whatever the + // surroundings, because whether to read from it is decided where a page is looked + // up, at run time, and only there is the answer knowable: deciding it here settles + // it while the definition is being generated, in the directory the application was + // built in, where a development environment is available -- so an image would be + // built believing it has to compile its pages, which is the one thing it cannot do. + // Named rather than resolved, so that what is written down is a location to look in + // and not a path on the machine that did the building. + precompiledGspMap = { PropertiesFactoryBean pfb -> Review Comment: This drops the `deployed` gate: `precompiledGspMap` is now always configured, in every environment. `DefaultGroovyPageLocator.isPrecompiledAvailable()` still guards on `!isDevelopmentMode()`, so the intended behaviour is preserved — but only as long as that guard holds. The practical risk is a stale `gsp/views.properties` on the classpath (left by a previous `compileGroovyPages`, or shipped inside a plugin jar) now being loaded in dev where it previously wasn't even resolved. Since the whole point of `AbstractAotProcessor.AOT_PROCESSING` in `isDevelopmentMode()` below is to answer this question at generation time, could the location list stay gated on `!isDevelopmentMode()` and get the same AOT override, rather than being made unconditional? At minimum this needs a spec asserting dev-mode page reloading still works when `gsp/views.properties` is present on the classpath — I don't see one in this PR. ########## grails-core/src/main/groovy/grails/boot/config/GrailsEnvironmentPostProcessor.java: ########## @@ -92,6 +101,43 @@ public void postProcessEnvironment(ConfigurableEnvironment environment, SpringAp } } + /** + * Colours the output of an image running at a terminal, which it otherwise cannot tell it has. + * + * <p>Spring Boot decides by asking for the console, and an image answers that it has none even + * when it is being watched at a terminal. So the same application whose start-up is coloured + * under {@code bootRun} arrives plain once it is built, for a reason that has nothing to do with + * the terminal it is running at.</p> + * + * <p>What the environment names as the terminal is read instead, which an image does carry. + * That does not distinguish output being watched from output being redirected -- nothing in an + * image does, which is the whole difficulty -- so a shell that redirects to a file still gets + * the escapes. It does distinguish a shell from the places that name no terminal at all: a + * build, a container, a service manager, where the output is only ever read later and stays + * plain. An application that has said either way is left alone.</p> + */ + private void colourTheOutputOfAnImageThatHasATerminal(ConfigurableEnvironment environment) { Review Comment: This turns ANSI escapes on by default in a native image whenever `TERM` is set, and the javadoc acknowledges it will write escapes into redirected output. That's a regression for the common `./app > app.log` and `./app | grep` cases, and it silently overrides a Boot default on the user's behalf. Defaulting to *not* colouring is the safe direction; someone who wants colour in an image can already set `spring.output.ansi.enabled=always`. If this stays, it should be opt-in (e.g. keyed off an explicit Grails property), not inferred from `TERM`. Separately: this is banner/console cosmetics in the middle of an AOT bean-definition PR — same split request as `GrailsBanner`. Also, `isImage()` and `terminal()` are `protected` purely so a Spock spec can override them. Per the repo guidance, tests should exercise the public surface; a package-private seam or an injected `Supplier` would be preferable to widening the API of a public `EnvironmentPostProcessor`. ########## grails-core/src/main/groovy/grails/boot/config/GrailsAutoConfiguration.groovy: ########## @@ -88,6 +96,29 @@ class GrailsAutoConfiguration implements GrailsApplicationClass, ApplicationCont return classes } + /** + * The artefacts written down while the application's code was generated, or {@code null} where + * nothing was written down and they are to be found the usual ways. + * + * <p>Both usual ways need something an image does not have: one walks the classpath, the other + * reads a list the compile-time transform builds as it goes, which is empty in anything the + * transform did not itself compile. So an image found no artefacts at all, and an application + * could only start by naming its own -- a list to keep in step with itself forever after.</p> + * + * <p>They were found while the code was generated, on an ordinary JVM where both ways work, and + * left here.</p> + */ + protected Collection<Class> artefactsWrittenDownAheadOfTime() { Review Comment: `classes()` is called by `GrailsApplicationPostProcessor` while it builds the `DefaultGrailsApplication`. Is `applicationContext` guaranteed to be injected on the `GrailsAutoConfiguration` instance by then? If it is null the method returns `null`, `classes()` falls back to classpath scanning, and in an image that yields *nothing* — i.e. the exact failure this is meant to fix, but silently and only in the packaged artifact. If that ordering is guaranteed, please say so in the javadoc and cover it with a spec that goes through the post-processor rather than calling `artefactsWrittenDownAheadOfTime()` directly. If it isn't, the singleton needs to be reachable without the context. ########## grails-core/src/main/groovy/grails/boot/GrailsBanner.groovy: ########## @@ -152,52 +368,136 @@ class GrailsBanner implements Banner { case VersionOption.SPRING_SECURITY: ['Spring Security': findVersion('org.springframework.security.core.SpringSecurityCoreVersion')] break + case VersionOption.CONTAINER: + findContainerVersion() + break case VersionOption.TOMCAT: - ['Tomcat': findVersion('org.apache.catalina.util.ServerInfo')] + ['Tomcat': findTomcatVersion()] break case VersionOption.JETTY: - ['Jetty': findVersion('org.eclipse.jetty.util.Jetty')] + ['Jetty': findJettyVersion()] break case VersionOption.UNDERTOW: - ['Undertow': findVersion('io.undertow.Undertow')] + ['Undertow': findUndertowVersion()] break default: null } } as Map<String, String> + versions.findAll { String label, String version -> version != null } + } + + /** + * The servlet container the application is running on, and the version it records. + * + * <p>An application runs on one container: choosing another is done by excluding the starter + * for this one, so two are not on the classpath together. They are therefore tried in the order + * they are commonly used and the first one found is the answer -- an application on Tomcat + * never goes looking for Jetty.</p> + * + * <p>On a container that records no version, or on none of these, this is empty and the banner + * leaves the line out rather than saying it does not know.</p> + */ + protected Map<String, String> findContainerVersion() { + String tomcat = findTomcatVersion() + if (tomcat != null) { + return ['Tomcat': tomcat] + } + String jetty = findJettyVersion() + if (jetty != null) { + return ['Jetty': jetty] + } + String undertow = findUndertowVersion() + if (undertow != null) { + return ['Undertow': undertow] + } + return [:] } /** - * Finds the implementation version of the specified class. + * Tomcat's version, read from the resource it ships rather than only from its manifest. * - * @param className the fully qualified class name - * @return the implementation version, or 'unknown' if not found + * <p>A resource survives being repackaged into an executable jar or built into an image, where + * the manifest's attributes are no longer attached to the package -- which is why the manifest + * route reads as nothing in exactly the two places a version is most worth having.</p> */ - private static String findVersion(String className) { + protected String findTomcatVersion() { + findVersionInResource('org/apache/catalina/util/ServerInfo.properties', 'server.number') + ?: findVersion('org.apache.catalina.util.ServerInfo') + } + + protected String findJettyVersion() { + findVersion('org.eclipse.jetty.util.Jetty') + } + + protected String findUndertowVersion() { + findVersion('io.undertow.Undertow') + } + + /** + * A version a library records in a resource it ships, read without loading any of its classes. + * + * <p>The manifest route only works while a jar is a plain entry on the classpath. Repackaged + * into an executable jar its attributes are no longer attached to the package, and an image has + * no jars at all -- which is why a container version read that way reads as nothing in exactly + * the two places it is most worth having. A resource is still a resource in both.</p> + * + * @param resource the classpath location of the resource to read + * @param key the property within it that carries the version + * @return the version, or {@code null} where the resource or the property is absent + */ + protected static String findVersionInResource(String resource, String key) { + InputStream stream = GrailsBanner.classLoader.getResourceAsStream(resource) + if (stream == null) { + return null + } try { - def pkg = Class.forName(className).package - return pkg?.implementationVersion ?: 'unknown' + Properties properties = new Properties() + stream.withCloseable { properties.load(it) } + return properties.getProperty(key) + } + catch (IOException ignored) { + return null + } + } + + /** + * The version a library records in the manifest of the jar it ships in. + * + * <p>Loaded without being initialised. A version is read <em>about</em> a library rather than + * <em>from</em> it, and running a static initialiser to find one lets the library do whatever it + * does on the way -- Spring Security logs a line of its own from there, which arrived in the + * middle of the banner, between the mark and the very versions it was being read for. The + * manifest is attached to the package when the class is loaded, and loading is all this + * needs.</p> + * + * @param className the fully qualified name of a class the library ships + * @return the version, or {@code null} where the class is absent or records none + */ + protected static String findVersion(String className) { Review Comment: Behaviour change that isn't called out: `findVersion` previously returned `'unknown'` and the line was printed; now it returns `null` and `createBannerVersions` filters the entry out entirely. An application that explicitly asked for `tomcat` in `grails.banner.versions.include` and got `Tomcat: unknown` now gets silence, which reads as "the option was ignored" — the very failure mode `warnAboutUnrecognisedVersions` was added to fix. Suggest keeping `unknown` for versions the application explicitly *asked* for, and omitting only the defaults. ########## grails-core/src/main/groovy/org/grails/spring/beans/AbstractResourceLocatorPostProcessor.java: ########## @@ -61,10 +63,25 @@ public void postProcessBeanDefinitionRegistry(BeanDefinitionRegistry registry) t } GenericBeanDefinition definition = new GenericBeanDefinition(); definition.setAbstract(true); - definition.getPropertyValues().add("searchLocations", this.searchLocations); + definition.getPropertyValues().add("searchLocations", searchLocationsToInherit()); registry.registerBeanDefinition(BEAN_NAME, definition); } + /** + * The locations to be inherited, which while code is being generated are none. + * + * <p>These are directories on the machine this runs on, and a child definition merges them in. + * Generating code for that child writes them into it, so an application would carry the + * directory it was built in and look for its resources there -- a path that says where it was + * built and, wherever it then runs, is not where its resources are. A generated application + * reads them from its own contents instead, which is what is left when there is nowhere named + * to look.</p> + */ + private List<String> searchLocationsToInherit() { Review Comment: `SpringProperties.getFlag(AbstractAotProcessor.AOT_PROCESSING)` now appears here, in `GroovyPagesGrailsPlugin.isDevelopmentMode()`, and in `UrlMappingsGrailsPlugin.isReloadEnabled()`, each with its own long-form javadoc explaining the same thing. Please extract a single supported helper (e.g. alongside the new `org.apache.grails...aot` code) so there is one place to change if Spring renames the flag, and one place documenting the invariant. Substantively: returning `List.of()` during generation means an AOT-generated `grailsResourceLocator` (and every third-party child, including asset-pipeline's `assetResourceLocator`) ships with *no* search locations. Is that verified to still resolve `classpath*:` resources at runtime, or does it depend on each child declaring its own defaults? A spec asserting an AOT-generated resource locator can still find a packaged resource would make this much less scary. ########## grails-core/src/main/groovy/org/grails/spring/beans/aot/VarargsBeanRegistrationAotProcessor.java: ########## @@ -0,0 +1,180 @@ +/* + * 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.spring.beans.aot; + +import java.lang.reflect.Array; +import java.lang.reflect.Executable; +import java.util.Collection; +import java.util.List; +import java.util.function.Predicate; + +import org.jspecify.annotations.Nullable; + +import org.springframework.aot.generate.GenerationContext; +import org.springframework.beans.factory.aot.BeanRegistrationAotContribution; +import org.springframework.beans.factory.aot.BeanRegistrationAotProcessor; +import org.springframework.beans.factory.aot.BeanRegistrationCode; +import org.springframework.beans.factory.aot.BeanRegistrationCodeFragments; +import org.springframework.beans.factory.aot.BeanRegistrationCodeFragmentsDecorator; +import org.springframework.beans.factory.config.ConstructorArgumentValues; +import org.springframework.beans.factory.config.ConstructorArgumentValues.ValueHolder; +import org.springframework.beans.factory.support.RegisteredBean; +import org.springframework.beans.factory.support.RootBeanDefinition; +import org.springframework.javapoet.CodeBlock; +import org.springframework.util.ClassUtils; + +/** + * Gathers a variable-argument constructor argument into the array it feeds, ahead of time. + * + * <p>A bean declared through the plugin DSL passes its arguments positionally, and a constructor + * that ends in a variable-argument parameter is called the way the language allows: one value where + * the parameter is an array, or a collection where it is an array of that element type. Building the + * bean, Spring adapts the argument to the parameter. Reading the definition to generate code for it, + * Spring does not: it looks the argument up by the parameter's type, and a lone {@code String} does + * not answer to {@code String[]}.</p> + * + * <p>The argument is then missed and resolved as a dependency instead, and an array of a type nobody + * publishes as a bean resolves to an empty array rather than failing. So the bean is built, and + * built wrong: a datastore that maps no classes, or a servlet registration with no URL mapping, + * which then falls back to mapping everything. Nothing is logged, and the bean that goes wrong is + * rarely the one that reports it -- the first symptom is a page that 404s or a domain class that + * says it is not one.</p> + * + * <p>Gathering the argument into an array here means the generator writes out {@code new String[] + * {"*.gsp"}}, which the lookup does find. Only an argument that is already usable as the array is + * left alone, and an argument that would need its elements converted is left to the resolution that + * exists today rather than guessed at here.</p> + * + * @since 8.0 + */ +public class VarargsBeanRegistrationAotProcessor implements BeanRegistrationAotProcessor { + + @Override + @Nullable + public BeanRegistrationAotContribution processAheadOfTime(RegisteredBean registeredBean) { + Executable executable = resolveExecutable(registeredBean); + if (executable == null || !executable.isVarArgs()) { + return null; + } + Class<?>[] parameterTypes = executable.getParameterTypes(); + RootBeanDefinition beanDefinition = registeredBean.getMergedBeanDefinition(); + Object gathered = gatherTrailingArgument(beanDefinition.getConstructorArgumentValues(), parameterTypes); + if (gathered == null) { + return null; + } + return BeanRegistrationAotContribution.withCustomCodeFragments( + codeFragments -> new VarargsCodeFragments(codeFragments, gathered)); + } + + /** + * The constructor or factory method the generator will write the call to. + * + * <p>Resolution reads the bean class and its members, so a bean whose class cannot be resolved + * fails here rather than at the point of use. It is not this processor's place to report that: + * generation carries on and fails where it means something.</p> + */ + @Nullable + private Executable resolveExecutable(RegisteredBean registeredBean) { + try { + return registeredBean.resolveConstructorOrFactoryMethod(); + } + catch (Throwable ignored) { Review Comment: `catch (Throwable ignored)` swallows `Error`s — `OutOfMemoryError`, `StackOverflowError`, `NoClassDefFoundError` from an unrelated cause — and turns them into "this bean has no varargs constructor". The stated intent is to tolerate a bean class that can't be resolved; catch that (`BeanCreationException` / `IllegalStateException`, or at most `Exception`) rather than `Throwable`, and log at debug so a genuinely broken bean leaves a trace. -- 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]
