jdaugherty commented on code in PR #16042: URL: https://github.com/apache/grails-core/pull/16042#discussion_r3669978532
########## grails-core/src/test/groovy/org/grails/testing/support/LogCapture.groovy: ########## @@ -0,0 +1,86 @@ +/* + * 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.testing.support + +import ch.qos.logback.classic.Level +import ch.qos.logback.classic.Logger +import ch.qos.logback.classic.spi.ILoggingEvent +import ch.qos.logback.core.read.ListAppender +import org.slf4j.LoggerFactory + +/** + * Captures log events emitted by a specific logger during a test. + * + * <p>Temporarily sets the logger to the given level (defaults to {@link Level#TRACE}) so that + * all events are captured, and restores the original level when {@link LogCapture#stop()} is called.</p> + * + * <p>Typical Spock usage:</p> + * <pre> + * given: + * def logCapture = new LogCapture(MyClass) + * // or by logger name: + * def logCapture = new LogCapture('org.some.Logger') + * + * then: + * logCapture.events.any { it.level == Level.WARN } + * + * cleanup: + * logCapture.stop() + * </pre> + */ +class LogCapture { Review Comment: Nice extraction — asserting on structured `ILoggingEvent`s instead of scraping `System.err` is a real improvement, and restoring the previous level in `stop()` is the part people usually forget. Two things, given the commit message calls this "reusable": 1. It lives in `src/test`, so it is reusable only within `grails-core`. If the intent is for other modules to adopt it, `java-test-fixtures` + `testFixturesApi 'ch.qos.logback:logback-classic'` would make it actually consumable. If the intent is grails-core-only for now, the commit message is overselling it and the class javadoc should say it is module-local. 2. `stop()` is not idempotent and there is no `Closeable`/`AutoCloseable`, so every caller needs a `cleanup:` block and a forgotten one leaks an appender plus a mutated level into every subsequent test in the fork. Implementing `AutoCloseable` (with `close()` delegating to `stop()`) would at least let callers use `withCloseable`, and makes the leak-on-forgetting harder. Minor: `LogCapture(Class loggerClass, ...)` is raw — `Class<?>` avoids the rawtypes warning. ########## grails-core/src/test/groovy/grails/plugins/DefaultGrailsPluginManagerSpec.groovy: ########## @@ -87,83 +90,93 @@ class DefaultGrailsPluginManagerSpec extends Specification { where: grailsVersion | pluginGrailsVersion || expectedCompatible - "1.0" | "3.3.1 > *" || false - "2.5" | "3.0.1" || false - "3.0.0" | "3.3.10 > *" || false - "3.3.10" | "4.0.0 > *" || false - "4.0.1" | "3.0.0.BUILD-SNAPSHOT > *" || true - "4.0.1" | "4.0.1" || true - "4.0.1" | "3.0.1" || false - "4.0.1" | "3.3.1 > *" || true - "4.0.1" | "3.3.10 > *" || true + '1.0' | '3.3.1 > *' || false + '2.5' | '3.0.1' || false + '3.0.0' | '3.3.10 > *' || false + '3.3.10' | '4.0.0 > *' || false + '4.0.1' | '3.0.0.BUILD-SNAPSHOT > *' || true + '4.0.1' | '4.0.1' || true + '4.0.1' | '3.0.1' || false + '4.0.1' | '3.3.1 > *' || true + '4.0.1' | '3.3.10 > *' || true // Milestone, release candidate and snapshot versions on both the application and the plugin (#14058) - "7.0.0-M2" | "7.0.0-M1 > *" || true - "7.0.0-M1" | "7.0.0-M2 > *" || false - "7.0.0-RC1" | "7.0.0-M1 > *" || true - "7.0.0-M1" | "7.0.0-RC1 > *" || false - "7.0.0" | "7.0.0-RC1 > *" || true - "7.0.0-RC1" | "7.0.0 > *" || false - "7.0.0-SNAPSHOT" | "7.0.0-SNAPSHOT > *" || true - "7.0.5-M1" | "7.0.3 > *" || true - "7.0.0-M1" | "7.0.0-M1" || true - "7.0.0-M2" | "7.0.0-M1" || false + '7.0.0-M2' | '7.0.0-M1 > *' || true + '7.0.0-M1' | '7.0.0-M2 > *' || false + '7.0.0-RC1' | '7.0.0-M1 > *' || true + '7.0.0-M1' | '7.0.0-RC1 > *' || false + '7.0.0' | '7.0.0-RC1 > *' || true + '7.0.0-RC1' | '7.0.0 > *' || false + '7.0.0-SNAPSHOT' | '7.0.0-SNAPSHOT > *' || true + '7.0.5-M1' | '7.0.3 > *' || true + '7.0.0-M1' | '7.0.0-M1' || true + '7.0.0-M2' | '7.0.0-M1' || false } def "per-plugin loaded messages are DEBUG and a single INFO summary reports the load order"() { given: 'a discovery bean with two plugins registered in reverse of their load order' def gcl = new GroovyClassLoader() def alphaClass = gcl.parseClass(''' -class AlphaProbeGrailsPlugin { - def version = "1.0.0" -} -''') + class AlphaProbeGrailsPlugin { + def version = '1.0.0' + } + ''') def betaClass = gcl.parseClass(''' -class BetaProbeGrailsPlugin { - def version = "2.0.0" - def loadAfter = ['alphaProbe'] -} -''') - def application = new DefaultGrailsApplication() - application.mainContext = new GenericApplicationContext() - def discovery = new DefaultPluginDiscovery(new Class<?>[]{betaClass, alphaClass}) - discovery.loadPluginsFromClasspath = false - - and: 'standard error is captured to observe slf4j-simple output' - def originalErr = System.err - def captured = new ByteArrayOutputStream() - System.setErr(new PrintStream(captured, true)) + class BetaProbeGrailsPlugin { + def version = '2.0.0' + def loadAfter = ['alphaProbe'] + } + ''') + def application = new DefaultGrailsApplication(mainContext: new GenericApplicationContext()) + def discovery = new DefaultPluginDiscovery([betaClass, alphaClass] as Class<?>[]).tap { + loadPluginsFromClasspath = false + } + + and: 'configure a logback appender to capture log messages' + def logCapture = new LogCapture(DefaultGrailsPluginManager) when: discovery.init(new StandardEnvironment()) - def manager = new DefaultGrailsPluginManager(application, discovery) - manager.loadPlugins() + def manager = new DefaultGrailsPluginManager(application, discovery).tap { + loadPlugins() + } then: 'both plugins are loaded' - manager.getGrailsPlugin('alphaProbe') != null - manager.getGrailsPlugin('betaProbe') != null + with(manager) { + getGrailsPlugin('alphaProbe') != null + getGrailsPlugin('betaProbe') != null + } and: 'the per-plugin loaded-successfully messages are not emitted at INFO' - captured.toString().readLines() - .findAll { it.contains('loaded successfully') } - .every { !it.contains('INFO') } + logCapture.events + .findAll { it.formattedMessage.contains('loaded successfully') } + .every { it.level.toString() != 'INFO' } Review Comment: Comparing `it.level.toString()` against a string literal gives up the type safety that moving to logback just bought. `ILoggingEvent.getLevel()` returns `ch.qos.logback.classic.Level`, and `LogCapture`'s own javadoc example and `GlobalGrailsClassInjectorTransformationSpec:466` both use the enum: ```groovy .every { it.level != Level.INFO } ``` Same at line 161 and at `PluginDiscoverySpec:375`. Worth making consistent across all three specs in this PR. Separately: `findAll { ... }.every { ... }` is vacuously true if nothing matched, so this assertion cannot distinguish "the messages are at DEBUG" from "the messages were never logged". Since `LogCapture` defaults to `TRACE`, the DEBUG events genuinely are captured now — which means this is finally checkable, unlike the old slf4j-simple version. Worth spending one line to say so: ```groovy def loadedMessages = logCapture.events.findAll { it.formattedMessage.contains('loaded successfully') } loadedMessages.size() == 2 loadedMessages.every { it.level == Level.DEBUG } ``` ########## grails-core/build.gradle: ########## @@ -95,21 +95,20 @@ dependencies { api 'org.slf4j:jcl-over-slf4j' // Testing - testImplementation 'org.slf4j:slf4j-simple' + testImplementation 'ch.qos.logback:logback-classic' // Used for checking log output in tests Review Comment: This makes `grails-core` the only module in the repo on logback for tests — 18 others still declare `testImplementation 'org.slf4j:slf4j-simple'`. That is justified here (logback is what makes `ListAppender` assertions possible), but it is worth a word in the PR description so the next person adding a module does not copy the wrong pattern, and so the eventual "standardise on one test logging backend" decision is a conscious one rather than drift. There is no `logback-test.xml` under `grails-core/src/test/resources`, so logback falls back to its default configuration (root at DEBUG, console appender). I measured the actual impact on `:grails-core:test` and it is small — 2 DEBUG lines across 62 test classes — so this is not urgent. But it is unpinned: the noise level of the whole module's test output now depends on whatever the classes under test happen to log at DEBUG. A three-line `logback-test.xml` with root at `WARN` makes it deterministic, and `LogCapture` still works because it raises the level on the specific logger it captures. ########## grails-core/src/main/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformation.groovy: ########## @@ -56,140 +58,158 @@ import org.grails.io.support.GrailsResourceUtils import org.grails.io.support.UrlResource /** - * A global transformation that applies Grails' transformations to classes within a Grails project + * Global AST transformation that applies Grails compiler injection to Grails project sources, + * including applications and plugins. + * + * <p>It identifies Grails artefacts, applies the relevant {@link ClassInjector} and + * {@link grails.compiler.traits.TraitInjector} implementations, and registers artefact handlers + * and injector implementations. When compiling a plugin descriptor, it also creates or updates + * the {@code META-INF/grails-plugin.xml} descriptor and records transformed plugin resources.</p> * - * @author Graeme Rocher * @since 3.0 */ -@GroovyASTTransformation(phase = CompilePhase.CANONICALIZATION) +@Slf4j @CompileStatic +@GroovyASTTransformation class GlobalGrailsClassInjectorTransformation implements ASTTransformation, CompilationUnitAware, TransformWithPriority { + /** + * The system property signalling that a multi-project build compiles each project into its own + * isolated output directory. When set, the transform must never fall back to a shared or guessed + * location, which could leak one module's generated metadata into another. + */ + public static final String ISOLATED_BUILD_PROPERTY = 'grails.isolated.build' + + public static final ClassNode ARTEFACT_CLASS_NODE = new ClassNode(Artefact) public static final ClassNode ARTEFACT_HANDLER_CLASS = ClassHelper.make('grails.core.ArtefactHandler') public static final ClassNode TRAIT_INJECTOR_CLASS = ClassHelper.make('grails.compiler.traits.TraitInjector') + private static final AntPathMatcher ANT_PATH_MATCHER = new AntPathMatcher() + + private final LinkedHashSet<String> pendingPluginClassNames = [] + private final Collection<String> pluginExcludePatterns = [] Review Comment: Good change, and I wanted to confirm the assumption it rests on rather than guess at it. Groovy's `ASTTransformationVisitor` creates **one instance of a global transform per `CompilationUnit`** and drives `visit()` on that instance for each source unit in order — I verified with a throwaway global transform registered via `META-INF/services`: ``` total instances created: 3 instance=2 visit#1 source=SrcA.groovy instance=2 visit#2 source=SrcB.groovy instance=2 visit#3 source=SrcC.groovy instance=3 visit#1 source=SrcA.groovy <- second CompilationUnit -> new instance instance=3 visit#2 source=SrcB.groovy ``` So instance scope == compilation scope, which is precisely the lifetime `pendingPluginClassNames` and `pluginExcludePatterns` want: carried across source units within one compile task, isolated between compile tasks. It also answers the Copilot concurrency thread — the phase operation is applied sequentially over source units, so the unsynchronised `LinkedHashSet` is fine and a `ThreadLocal`/`CompilationUnit`-keyed map would be strictly worse. The corollary is that the `clear()` at line 286 no longer has a job to do; see that comment. ########## grails-core/src/main/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformation.groovy: ########## @@ -211,153 +231,419 @@ class GlobalGrailsClassInjectorTransformation implements ASTTransformation, Comp return targetDirectory } + /** + * Adds the compiled class to the {@code META-INF/grails.factories} entry for the supplied type + * when it is a concrete subtype of that type. Existing generated entries and matching + * project-source entries are preserved, and the resulting factory file is written to the + * compilation target directory. + * + * @param classNode the class being compiled + * @param superType the factory interface or superclass whose implementations are registered + * @param compilationTargetDirectory the compilation output directory containing the factory file + * @return {@code true} when {@code classNode} is a non-abstract subtype of {@code superType} and + * was registered; {@code false} otherwise + */ static boolean updateGrailsFactoriesWithType(ClassNode classNode, ClassNode superType, File compilationTargetDirectory) { - FactoriesFileWriter.updateFactoriesWithType(classNode, superType, compilationTargetDirectory, - 'META-INF/grails.factories', ['src/main/resources/META-INF/grails.factories']) + FactoriesFileWriter.updateFactoriesWithType( + classNode, + superType, + compilationTargetDirectory, + 'META-INF/grails.factories', + ['src/main/resources/META-INF/grails.factories'] + ) } - static LinkedHashSet<String> pendingPluginClasses = [] - static Collection<String> pluginExcludes = [] + private static boolean updateGrailsFactoriesWithTypes(ClassNode classNode, Collection<ClassNode> superTypes, File compilationTargetDirectory) { + superTypes.any { + updateGrailsFactoriesWithType(classNode, it, compilationTargetDirectory) + } + } - protected static void generatePluginXml(ClassNode pluginClassNode, String pluginVersion, Set<String> transformedClasses, File pluginXmlFile) { + /** + * Creates or updates the generated {@code META-INF/grails-plugin.xml} descriptor and carries + * forward artefact classes collected during compilation. + * + * @param pluginClassNode the compiled plugin descriptor class, or {@code null} when none was found + * @param pluginVersion the plugin version, or {@code null} when no concrete plugin descriptor + * is being generated + * @param transformedClassNames the artefact classes transformed in the current source unit + * @param pluginXmlFile the generated plugin descriptor file + */ + protected void generatePluginXml( + @Nullable ClassNode pluginClassNode, + @Nullable String pluginVersion, + Set<String> transformedClassNames, + File pluginXmlFile + ) { + // first check if plugin.xml exists + pluginXmlFile.parentFile.mkdirs() def pluginXmlExists = pluginXmlFile.exists() - LinkedHashSet<String> pluginClasses = [] - pluginClasses.addAll(transformedClasses) - pluginClasses.addAll(pendingPluginClasses) - - // if the class being transformed is a *GrailsPlugin class then if it doesn't exist create it - if (pluginClassNode && !pluginClassNode.isAbstract()) { + def pluginClasses = [] as LinkedHashSet<String> + pluginClasses.addAll(transformedClassNames) + pluginClasses.addAll(pendingPluginClassNames) + // Reset excludes from a previous source unit so that patterns declared by one plugin + // do not leak into a subsequent compilation within the same Gradle worker. + pluginExcludePatterns.clear() + + // Create or update grails-plugin.xml when a concrete plugin class is present; otherwise, + // update an existing descriptor or defer resource names until the descriptor is compiled. + if (pluginClassNode && !pluginClassNode.abstract) { + if (!pluginVersion) { + throw new IllegalStateException( + "Unable to generate '${pluginXmlFile}' because plugin class '${pluginClassNode.name}' " + + 'does not define a plugin version.' + ) + } Review Comment: Failing fast here is the right instinct, but throwing out of `visit()` produces a diagnostic that blames Groovy for a plugin misconfiguration. Compiling `class UnversionedGrailsPlugin {}` on this branch gives the author: ``` org.codehaus.groovy.GroovyBugError: BUG! exception in phase 'canonicalization' in source unit '.../UnversionedGrailsPlugin.groovy' Unable to generate '.../META-INF/grails-plugin.xml' because plugin class 'UnversionedGrailsPlugin' does not define a plugin version. ``` "BUG!" is Groovy's wrapper for an unexpected `RuntimeException` escaping a phase operation; it invites a bug report against the wrong project, and it carries no source location. `GrailsASTUtils.error(SourceUnit, ASTNode, String, boolean)` already exists for this, in this package (`GrailsASTUtils.java:181`). Routing the failure through the error collector reports it as an ordinary compilation error against the descriptor's line/column: ```groovy GrailsASTUtils.error(source, pluginClassNode, "Unable to generate '${pluginXmlFile}' because plugin class '${pluginClassNode.name}' " + 'does not define a plugin version.', true) return ``` That needs `generatePluginXml` to take the `SourceUnit` (or the check to move up into `visit()`, which has it) — either is fine. Same applies to the `IllegalStateException` in `resolveCompilationTargetDirectory`, though that one predates this PR. ########## grails-core/src/test/groovy/org/grails/exception/reporting/StackTraceFiltererSpec.groovy: ########## @@ -64,55 +65,48 @@ class StackTraceFiltererSpec extends Specification { } def 'filter emits a StackTrace log entry for a single throwable by default'() { - given: 'captured System.err' - def originalErr = System.err - def baos = new ByteArrayOutputStream() - System.setErr(new PrintStream(baos, true)) + given: 'a configured log appender to capture the StackTrace log entry' + def logCapture = new LogCapture('StackTrace') and: 'an exception whose stack trace mixes application and internal frames' def exception = new RuntimeException('boom') exception.stackTrace = [ - new StackTraceElement('test.FooController', 'show', 'FooController.groovy', 6), - new StackTraceElement('java.lang.reflect.Method', 'invoke', 'Method.java', 580) + ['test.FooController', 'show', 'FooController.groovy', 6], + ['java.lang.reflect.Method', 'invoke', 'Method.java', 580] ] as StackTraceElement[] when: 'the exception is filtered' filterer.filter(exception) then: "a 'Full Stack Trace:' entry is emitted by the filterer for backwards compatibility" - System.err.flush() - baos.toString().contains('Full Stack Trace:') + logCapture.events.any { it.formattedMessage.contains('Full Stack Trace:') } cleanup: - System.setErr(originalErr) + logCapture.stop() } def 'filter does not emit a StackTrace log entry when logFullStackTraceOnFilter is disabled'() { - given: 'captured System.err' - def originalErr = System.err - def baos = new ByteArrayOutputStream() - System.setErr(new PrintStream(baos, true)) + given: 'a configured log appender to capture the StackTrace log entry' + def logCapture = new LogCapture('StackTrace') and: 'a filterer with the side-effect emission disabled' - def quietFilterer = new DefaultStackTraceFilterer() - quietFilterer.logFullStackTraceOnFilter = false + def quietFilterer = new DefaultStackTraceFilterer(logFullStackTraceOnFilter: false) and: 'an exception whose stack trace mixes application and internal frames' def exception = new RuntimeException('boom') exception.stackTrace = [ - new StackTraceElement('test.FooController', 'show', 'FooController.groovy', 6), - new StackTraceElement('java.lang.reflect.Method', 'invoke', 'Method.java', 580) + ['test.FooController', 'show', 'FooController.groovy', 6], + ['java.lang.reflect.Method', 'invoke', 'Method.java', 580] ] as StackTraceElement[] when: 'the exception is filtered' quietFilterer.filter(exception) then: "no 'Full Stack Trace:' entry is emitted by the filterer" - System.err.flush() - !baos.toString().contains('Full Stack Trace:') + logCapture.events.every { !it.formattedMessage.contains('Full Stack Trace:') } Review Comment: `every { !... }` on an empty list is `true`, so this passes whether the filterer stayed quiet or the logger was never wired up at all. Same at line 210. The old `!baos.toString().contains(...)` had the same hole, so this is not a regression — but the migration is the moment to close it, since the positive counterpart at line 82/180 already proves events *are* captured for this logger. Either assert emptiness directly: ```groovy logCapture.events.empty ``` or, if other events on the `StackTrace` logger are expected, `logCapture.events.count { it.formattedMessage.contains('Full Stack Trace:') } == 0`, which mirrors the `== 2` assertion at line 180 and reads as a deliberate zero rather than an accident. ########## grails-core/src/main/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformation.groovy: ########## @@ -211,153 +231,419 @@ class GlobalGrailsClassInjectorTransformation implements ASTTransformation, Comp return targetDirectory } + /** + * Adds the compiled class to the {@code META-INF/grails.factories} entry for the supplied type + * when it is a concrete subtype of that type. Existing generated entries and matching + * project-source entries are preserved, and the resulting factory file is written to the + * compilation target directory. + * + * @param classNode the class being compiled + * @param superType the factory interface or superclass whose implementations are registered + * @param compilationTargetDirectory the compilation output directory containing the factory file + * @return {@code true} when {@code classNode} is a non-abstract subtype of {@code superType} and + * was registered; {@code false} otherwise + */ static boolean updateGrailsFactoriesWithType(ClassNode classNode, ClassNode superType, File compilationTargetDirectory) { - FactoriesFileWriter.updateFactoriesWithType(classNode, superType, compilationTargetDirectory, - 'META-INF/grails.factories', ['src/main/resources/META-INF/grails.factories']) + FactoriesFileWriter.updateFactoriesWithType( + classNode, + superType, + compilationTargetDirectory, + 'META-INF/grails.factories', + ['src/main/resources/META-INF/grails.factories'] + ) } - static LinkedHashSet<String> pendingPluginClasses = [] - static Collection<String> pluginExcludes = [] + private static boolean updateGrailsFactoriesWithTypes(ClassNode classNode, Collection<ClassNode> superTypes, File compilationTargetDirectory) { + superTypes.any { + updateGrailsFactoriesWithType(classNode, it, compilationTargetDirectory) + } + } - protected static void generatePluginXml(ClassNode pluginClassNode, String pluginVersion, Set<String> transformedClasses, File pluginXmlFile) { + /** + * Creates or updates the generated {@code META-INF/grails-plugin.xml} descriptor and carries + * forward artefact classes collected during compilation. + * + * @param pluginClassNode the compiled plugin descriptor class, or {@code null} when none was found + * @param pluginVersion the plugin version, or {@code null} when no concrete plugin descriptor + * is being generated + * @param transformedClassNames the artefact classes transformed in the current source unit + * @param pluginXmlFile the generated plugin descriptor file + */ + protected void generatePluginXml( + @Nullable ClassNode pluginClassNode, + @Nullable String pluginVersion, + Set<String> transformedClassNames, + File pluginXmlFile + ) { + // first check if plugin.xml exists + pluginXmlFile.parentFile.mkdirs() def pluginXmlExists = pluginXmlFile.exists() - LinkedHashSet<String> pluginClasses = [] - pluginClasses.addAll(transformedClasses) - pluginClasses.addAll(pendingPluginClasses) - - // if the class being transformed is a *GrailsPlugin class then if it doesn't exist create it - if (pluginClassNode && !pluginClassNode.isAbstract()) { + def pluginClasses = [] as LinkedHashSet<String> + pluginClasses.addAll(transformedClassNames) + pluginClasses.addAll(pendingPluginClassNames) + // Reset excludes from a previous source unit so that patterns declared by one plugin + // do not leak into a subsequent compilation within the same Gradle worker. + pluginExcludePatterns.clear() Review Comment: This is a regression, and it silently turns `pluginExcludes` off for most of a plugin build. The comment's rationale no longer applies. `pluginExcludePatterns` is now an instance field, and a global transform gets a fresh instance per `CompilationUnit` (verified — see my note on line 89), so patterns cannot leak into "a subsequent compilation within the same Gradle worker". There is nothing left for this `clear()` to guard against. What it *does* do is discard the descriptor's excludes for every **later source unit in the same compilation**: - SU1 = `FooGrailsPlugin.groovy` → `writePluginXml`/`updatePluginXml` records `pluginExcludePatterns`. - SU2 = any other artefact → `generatePluginXml` clears the patterns, then `updatePluginXml(null, ...)` runs with `pluginClassNode == null`, so line 412 never repopulates them, and `handleExcludes` at line 421 is a no-op. Excluded resources therefore survive into `META-INF/grails-plugin.xml`. That is not cosmetic: `PluginXmlHandler` collects `<resource>` into `providedClasses`, which `BinaryGrailsPlugin.initializeProvidedArtefacts` registers as the plugin's artefacts — so classes an author deliberately excluded get loaded in consuming applications. Here is a spec that **passes on `8.0.x` and fails on this branch**: ```groovy void "excludes recorded by the descriptor still apply to a later source unit"() { given: 'an existing descriptor listing a resource the plugin excludes' def pluginXml = new File(tempDir, 'carry-over-plugin.xml') def seed = ''' <plugin name="carryOver" version="1.0" grailsVersion="1.0 > *"> <type>CarryOverGrailsPlugin</type> <resources> <resource>ExcludedThing</resource> <resource>KeptThing</resource> </resources> </plugin> ''' pluginXml.text = seed def classNode = compilePlugin(''' class CarryOverGrailsPlugin { def pluginExcludes = ['Excluded*'] } ''') when: 'source unit 1 is the plugin descriptor' transformation.generatePluginXml(classNode, '1.0', ['KeptThing'] as Set, pluginXml) then: new XmlSlurper().parse(pluginXml).resources.resource*.text() == ['KeptThing'] when: 'source unit 2 is an ordinary artefact source' pluginXml.text = seed transformation.generatePluginXml(null, null, ['AnotherThing'] as Set, pluginXml) then: 'the excludes recorded in source unit 1 are still honoured' new XmlSlurper().parse(pluginXml).resources.resource*.text() == ['KeptThing', 'AnotherThing'] } ``` On this branch the second `then:` yields `[ExcludedThing, KeptThing, AnotherThing]`. The fix is to delete the three lines. Worth adding the test above regardless — no existing spec drives two successive `generatePluginXml` calls, which is why the whole static-to-instance change is currently uncovered in the direction that matters. ########## grails-core/src/main/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformation.groovy: ########## @@ -211,153 +231,419 @@ class GlobalGrailsClassInjectorTransformation implements ASTTransformation, Comp return targetDirectory } + /** + * Adds the compiled class to the {@code META-INF/grails.factories} entry for the supplied type + * when it is a concrete subtype of that type. Existing generated entries and matching + * project-source entries are preserved, and the resulting factory file is written to the + * compilation target directory. + * + * @param classNode the class being compiled + * @param superType the factory interface or superclass whose implementations are registered + * @param compilationTargetDirectory the compilation output directory containing the factory file + * @return {@code true} when {@code classNode} is a non-abstract subtype of {@code superType} and + * was registered; {@code false} otherwise + */ static boolean updateGrailsFactoriesWithType(ClassNode classNode, ClassNode superType, File compilationTargetDirectory) { - FactoriesFileWriter.updateFactoriesWithType(classNode, superType, compilationTargetDirectory, - 'META-INF/grails.factories', ['src/main/resources/META-INF/grails.factories']) + FactoriesFileWriter.updateFactoriesWithType( + classNode, + superType, + compilationTargetDirectory, + 'META-INF/grails.factories', + ['src/main/resources/META-INF/grails.factories'] + ) } - static LinkedHashSet<String> pendingPluginClasses = [] - static Collection<String> pluginExcludes = [] + private static boolean updateGrailsFactoriesWithTypes(ClassNode classNode, Collection<ClassNode> superTypes, File compilationTargetDirectory) { + superTypes.any { + updateGrailsFactoriesWithType(classNode, it, compilationTargetDirectory) + } + } - protected static void generatePluginXml(ClassNode pluginClassNode, String pluginVersion, Set<String> transformedClasses, File pluginXmlFile) { + /** + * Creates or updates the generated {@code META-INF/grails-plugin.xml} descriptor and carries + * forward artefact classes collected during compilation. + * + * @param pluginClassNode the compiled plugin descriptor class, or {@code null} when none was found + * @param pluginVersion the plugin version, or {@code null} when no concrete plugin descriptor + * is being generated + * @param transformedClassNames the artefact classes transformed in the current source unit + * @param pluginXmlFile the generated plugin descriptor file + */ + protected void generatePluginXml( + @Nullable ClassNode pluginClassNode, + @Nullable String pluginVersion, + Set<String> transformedClassNames, + File pluginXmlFile + ) { + // first check if plugin.xml exists + pluginXmlFile.parentFile.mkdirs() def pluginXmlExists = pluginXmlFile.exists() - LinkedHashSet<String> pluginClasses = [] - pluginClasses.addAll(transformedClasses) - pluginClasses.addAll(pendingPluginClasses) - - // if the class being transformed is a *GrailsPlugin class then if it doesn't exist create it - if (pluginClassNode && !pluginClassNode.isAbstract()) { + def pluginClasses = [] as LinkedHashSet<String> + pluginClasses.addAll(transformedClassNames) + pluginClasses.addAll(pendingPluginClassNames) + // Reset excludes from a previous source unit so that patterns declared by one plugin + // do not leak into a subsequent compilation within the same Gradle worker. + pluginExcludePatterns.clear() + + // Create or update grails-plugin.xml when a concrete plugin class is present; otherwise, + // update an existing descriptor or defer resource names until the descriptor is compiled. + if (pluginClassNode && !pluginClassNode.abstract) { + if (!pluginVersion) { + throw new IllegalStateException( + "Unable to generate '${pluginXmlFile}' because plugin class '${pluginClassNode.name}' " + + 'does not define a plugin version.' + ) + } if (!pluginXmlExists) { + // The plugin descriptor is being compiled for the first time. writePluginXml(pluginClassNode, pluginVersion, pluginXmlFile, pluginClasses) } else { - // otherwise if the file does exist, update it with the plugin name + // Refresh the existing descriptor with the current plugin metadata and resources. updatePluginXml(pluginClassNode, pluginVersion, pluginXmlFile, pluginClasses) } } else if (pluginXmlExists) { - // if the class isn't the *GrailsPlugin class then only update the plugin.xml if it already exists + // Add resources from this source unit to the existing descriptor. updatePluginXml(null, pluginVersion, pluginXmlFile, pluginClasses) } else { - // otherwise add it to a list of pending classes to populated when the plugin.xml is created - pendingPluginClasses.addAll(transformedClasses) + // Defer these resource names until a source unit compiles the plugin descriptor. + pendingPluginClassNames.addAll(transformedClassNames) } } - @CompileDynamic - static void writePluginXml(ClassNode pluginClassNode, String pluginVersion, File pluginXml, Collection<String> artefactClasses) { + /** + * Writes a new plugin descriptor from the plugin class metadata and supplied artefact classes. + * + * @param pluginClassNode the plugin descriptor class + * @param pluginVersion the required plugin version when {@code pluginClassNode} is present + * @param pluginXml the output descriptor file + * @param artefactClassNames artefact class names to include as resources + */ + void writePluginXml( + @Nullable ClassNode pluginClassNode, + String pluginVersion, + File pluginXml, + Collection<String> artefactClassNames + ) { + pluginXml.parentFile.mkdirs() if (pluginClassNode) { - PluginAstReader pluginAstReader = new PluginAstReader() - def info = pluginAstReader.readPluginInfo(pluginClassNode) - - pluginXml.withWriter(StandardCharsets.UTF_8.name()) { Writer writer -> - def mkp = new MarkupBuilder(writer) - def pluginName = GrailsNameUtils.getLogicalPropertyName(pluginClassNode.name, 'GrailsPlugin') - - def pluginProperties = info.getProperties() - def excludes = pluginProperties.get('pluginExcludes') - if (excludes instanceof List) { - pluginExcludes.clear() - pluginExcludes.addAll(excludes) - } + writePluginXmlWithDescriptor(pluginClassNode, pluginVersion, pluginXml, artefactClassNames) + } else { + writePluginXmlWithoutDescriptor(pluginXml, artefactClassNames) + } + pendingPluginClassNames.clear() + } - def grailsVersion = pluginProperties['grailsVersion'] ?: getClass().getPackage().getImplementationVersion() + ' > *' - mkp.plugin(name: pluginName, version: pluginVersion, grailsVersion: grailsVersion) { - type(pluginClassNode.name) + @CompileDynamic + private void writePluginXmlWithDescriptor( + ClassNode pluginClassNode, + String pluginVersion, + File pluginXml, + Collection<String> artefactClassNames + ) { + def pluginInfo = new PluginAstReader().readPluginInfo(pluginClassNode) + pluginXml.withWriter(StandardCharsets.UTF_8.name()) { Writer writer -> + def markupBuilder = new MarkupBuilder(writer) + def pluginName = GrailsNameUtils.getLogicalPropertyName(pluginClassNode.name, 'GrailsPlugin') + def pluginProperties = pluginInfo.properties + def pluginExcludes = pluginProperties.get('pluginExcludes') + if (pluginExcludes instanceof List) { + pluginExcludePatterns.clear() + pluginExcludePatterns.addAll(pluginExcludes) + } - for (entry in pluginProperties) { - delegate."$entry.key"(entry.value) - } + // if the plugin class doesn't define a grailsVersion, use the version of the grails-core jar + def grailsVersion = pluginProperties['grailsVersion'] ?: + GlobalGrailsClassInjectorTransformation.package.implementationVersion + ' > *' Review Comment: Two notes on this expression, which is duplicated verbatim at line 620. First, the `getClass()` fix has a side effect worth calling out in the PR description. On `8.0.x` this resolved against `java.lang.Class`, so a plugin that did not declare `grailsVersion` got `grailsVersion="null > *"` written into its descriptor. Now it gets the real value — `CompilePlugin.groovy:83` sets `Implementation-Version` to `grailsVersion` — so published descriptors will start carrying `grailsVersion="8.0.0 > *"`. As far as I can tell nothing reads that attribute back (`PluginXmlHandler` only collects `<type>` and `<resource>`; `BinaryGrailsPlugin` only uses `getProvidedClasses()` and the descriptor `Resource`), so the blast radius looks like zero — but it is a change in generated output that is currently untested and unmentioned. Second, when the implementation version *is* unavailable — grails-core on the classpath as class directories, i.e. IDE and this project's own test run — the string concatenation still produces the literal `"null > *"`. I hit it in a probe: ``` <plugin name='probeTwo' version='1.0' grailsVersion='null > *'> ``` Given this PR now hard-fails on a missing *plugin* version, silently emitting `null > *` for a missing *grails* version is inconsistent. Since the expression is needed twice, extracting it is the natural place to decide: ```groovy private static String resolveGrailsVersionRange(Map pluginProperties) { def declared = pluginProperties['grailsVersion'] if (declared) return declared.toString() def frameworkVersion = GlobalGrailsClassInjectorTransformation.package.implementationVersion frameworkVersion ? "${frameworkVersion} > *" : null } ``` and then only write the attribute when it is non-null. ########## grails-core/src/test/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformationSpec.groovy: ########## @@ -184,41 +169,475 @@ class FooGrailsPlugin { GlobalGrailsClassInjectorTransformation.resolveCompilationTargetDirectory(source, true) then: "the build fails loudly rather than writing to a shared location" - IllegalStateException e = thrown() - e.message.contains(GlobalGrailsClassInjectorTransformation.ISOLATED_BUILD_PROPERTY) + def e = thrown(IllegalStateException) + e.message.contains('grails.isolated.build') } @RestoreSystemProperties void "findSourceDirectory prefers the per-project base.dir system property when set"() { given: "base.dir points at an existing directory" - File baseDir = File.createTempDir() - System.setProperty('base.dir', baseDir.absolutePath) - File target = new File(baseDir, 'build/classes/groovy/main') + System.setProperty('base.dir', tempDir.absolutePath) + def targetDir = new File(tempDir, 'build/classes/groovy/main') when: - File resolved = FactoriesFileWriter.findSourceDirectory(target) + def resolvedDir = FactoriesFileWriter.findSourceDirectory(targetDir) then: "the build-tool supplied base.dir wins" - resolved == baseDir - - cleanup: - baseDir.deleteDir() + resolvedDir == tempDir } @RestoreSystemProperties void "findSourceDirectory walks up to the project directory when base.dir is not set"() { given: "no base.dir and a standard per-project compile target" System.clearProperty('base.dir') - File projectDir = File.createTempDir() - File target = new File(projectDir, 'build/classes/groovy/main') + def targetDir = new File(tempDir, 'build/classes/groovy/main') when: - File resolved = FactoriesFileWriter.findSourceDirectory(target) + def resolvedDir = FactoriesFileWriter.findSourceDirectory(targetDir) then: "it resolves to the parent of the build directory" - resolved == projectDir + resolvedDir == tempDir + } + + void "priority returns the global grails transform order"() { + expect: + new GlobalGrailsClassInjectorTransformation().priority() == GroovyTransformOrder.GLOBAL_GRAILS_TRANSFORM_ORDER + } + + void "the global transform ignores a source without a resolvable URL"() { + when: + new GlobalGrailsClassInjectorTransformation().visit([] as ASTNode[], Stub(SourceUnit) { + getName() >> null + }) + + then: + noExceptionThrown() + } + + void "the global transform stamps a GrailsPlugin descriptor class with a version property"() { + given: "a *GrailsPlugin class with no explicit version property, and nowhere for plugin.xml to exist yet" + def sourceFile = new File(tempDir, 'PlainGrailsPlugin.groovy') + def targetDir = new File(tempDir, 'build/classes/groovy/main') + + when: "the source is compiled, exercising the registered global transform" + def classNode = compileToFile( + sourceFile, + 'class PlainGrailsPlugin {}', + targetDir, [projectVersion: '1.5'] + ) + + then: "the plugin class is stamped with the resolved version" + classNode.getProperty('version') != null + + and: "the plugin.xml describing it is generated as a side effect" + new File(targetDir, 'META-INF/grails-plugin.xml').exists() + } + + void "the global transform resolves a plugin version declared on the plugin class"() { + given: "a plugin descriptor with a declared version and no compiler project metadata" + def sourceFile = new File(tempDir, 'DeclaredGrailsPlugin.groovy') + def targetDir = new File(tempDir, 'build/classes/groovy/main') + + when: + def classNode = compileToFile( + sourceFile, + ''' + class DeclaredGrailsPlugin { + def version = '3.0' + } + ''', + targetDir + ) + + then: + classNode.getProperty('version').initialExpression.text == '3.0' + new File(targetDir, 'META-INF/grails-plugin.xml').exists() + } + + void "the global transform fails when a plugin descriptor class has no version"() { + given: "a plugin descriptor class without a declared or compiler-provided version" + def sourceFile = new File(tempDir, 'UnversionedGrailsPlugin.groovy') + def targetDir = new File(tempDir, 'build/classes/groovy/main') + + when: + compileToFile(sourceFile, 'class UnversionedGrailsPlugin {}', targetDir) + + then: + def exception = thrown(GroovyBugError) + with(exception) { + cause instanceof IllegalStateException + cause.message.contains('does not define a plugin version') + } + } + + void "the global transform annotates a plain Grails resource class with @GrailsPlugin metadata"() { + given: "a class under grails-app that isn't matched by any registered ArtefactHandler" + def sourceFile = new File(tempDir, 'grails-app/services/FooWidget.groovy') + def targetDir = new File(tempDir, 'build/classes/groovy/main') + + when: "the source is compiled, exercising the registered global transform" + def classNode = compileToFile( + sourceFile, + 'class FooWidget {}', + targetDir, + [projectName: 'foowidget', projectVersion: '2.0'] + ) + + then: "the class is stamped with the project's @GrailsPlugin metadata" + def annotations = classNode.getAnnotations(ClassHelper.make(GrailsPlugin)) + annotations.size() == 1 + with(annotations.first()) { + getMember('name').text == GrailsNameUtils.getPropertyNameForLowerCaseHyphenSeparatedName('foowidget') + getMember('version').text == '2.0' + } + } + + void "the global transform processes a Grails service as an artefact"() { + given: "a service source under grails-app" + def sourceFile = new File(tempDir, 'grails-app/services/FooService.groovy') + def targetDir = new File(tempDir, 'build/classes/groovy/main') + TraitInjectionUtils.@traitInjectors = [] + + when: + def classNode = compileToFile( + sourceFile, + 'class FooService {}', + targetDir + ) + + then: + with(classNode.getAnnotations(ClassHelper.make(Artefact))) { + size() == 1 + first().getMember('value').text == 'Service' + } + + cleanup: + TraitInjectionUtils.@traitInjectors = null + } + + void "the global transform registers a concrete artefact handler in grails.factories"() { + given: "an artefact handler source" + def sourceFile = new File(tempDir, 'src/main/groovy/TestHandler.groovy') + def targetDir = new File(tempDir, 'build/classes/groovy/main') + + when: + compileToFile( + sourceFile, + ''' + class TestHandler extends grails.core.ArtefactHandlerAdapter { + TestHandler() { + super('Test', null, null, 'Handler') + } + } + ''', + targetDir + ) + + then: + def factories = new File(targetDir, 'META-INF/grails.factories') + factories.exists() + factories.text.contains('TestHandler') + } + + void "the global transform skips classes whose source falls outside the Grails resource patterns"() { + given: "a plain source under src/main/groovy, which is project source but not a Grails resource" + def sourceFile = new File(tempDir, 'src/main/groovy/PlainClass.groovy') + def targetDir = new File(tempDir, 'build/classes/groovy/main') + + when: "the source is compiled, exercising the registered global transform" + def classNode = compileToFile( + sourceFile, + 'class PlainClass {}', + targetDir, + [projectName: 'plain', projectVersion: '1.0'] + ) + + then: "no @GrailsPlugin metadata is stamped on the class" + classNode.getAnnotations(ClassHelper.make(GrailsPlugin)).empty + } + + void "plugin xml excludes are honoured and metadata refreshed when updating an existing file"() { + given: "an existing plugin.xml with a resource that the plugin now wants excluded" + def pluginXml = File.createTempFile('plugin-xml-excludes', 'test.xml', tempDir) + def classNode = null + def cu = new CompilationUnit(new GroovyClassLoader()) + cu.addSource('BazGrailsPlugin', ''' + class BazGrailsPlugin { + def pluginExcludes = ['Excluded*'] + def grailsVersion = '3.0 > *' + } + ''') + cu.addPhaseOperation({ SourceUnit source, GeneratorContext context, ClassNode cn -> + if (cn.name.endsWith('GrailsPlugin')) { + classNode = cn + } + } as CompilationUnit.IPrimaryClassNodeOperation, Phases.CONVERSION) + cu.compile(Phases.CONVERSION) + pluginXml.text = ''' + <plugin name="baz" version="1.0" grailsVersion="1.0 > *"> + <type>BazGrailsPlugin</type> + <resources> + <resource>ExcludedThing</resource> + <resource>ExistingThing</resource> + </resources> + </plugin> + ''' + + when: "the transformation updates the plugin.xml" + transformation.generatePluginXml( + classNode, + '2.0', + ['ExcludedThing', 'NewThing'] as Set, + pluginXml + ) + + then: "the file exists" + pluginXml.exists() + + when: "the xml is parsed" + def xml = new XmlSlurper().parse(pluginXml) + + then: "the excluded resource was removed, the kept resource was added, and metadata was refreshed" + [email protected]() == '2.0' + [email protected]() == '3.0 > *' + xml.resources.resource*.text() == ['ExistingThing', 'NewThing'] + } + + void "plugin xml excludes are applied when writing a new descriptor"() { + given: + def pluginXml = new File(tempDir, 'plugin-xml-write-excludes.xml') + def classNode = compilePlugin(''' + class WrittenExcludesGrailsPlugin { + def pluginExcludes = ['Excluded*'] + def grailsVersion = '4.0 > *' + } + ''') + + when: + transformation.generatePluginXml( + classNode, + '1.0', + ['ExcludedThing', 'KeptThing'] as Set, + pluginXml + ) + + then: + new XmlSlurper().parse(pluginXml).resources.resource*.text() == ['KeptThing'] + } + + void "plugin xml resources are updated when an existing descriptor has no plugin class"() { + given: + def pluginXml = new File(tempDir, 'existing-plugin.xml') + pluginXml.text = ''' + <plugin> + <resources> + <resource>ExistingThing</resource> + </resources> + </plugin> + ''' + + when: + transformation.generatePluginXml( + null, + null, + ['NewThing'] as Set, + pluginXml + ) + + then: + new XmlSlurper().parse(pluginXml).resources.resource*.text() == ['ExistingThing', 'NewThing'] + } Review Comment: This is the closest the spec gets to the "descriptor already compiled, ordinary artefact compiled later" flow, but it drives `generatePluginXml` exactly once, on a fresh `transformation`, so no state is ever carried from one source unit to the next. That gap is why the `pluginExcludePatterns.clear()` regression is invisible: every feature here is a single-source-unit scenario, and `pluginExcludes` only breaks on the *second* call. The `pendingPluginClassNames` direction is covered ("artefact class names are deferred..." at line 474), so it is specifically the excludes carry-over that has no test. Adding the two-call spec from my comment on `GlobalGrailsClassInjectorTransformation.groovy:286` closes it, and would also stand as the regression test for the static-to-instance change generally — right now nothing asserts that state *does* persist across `visit()` calls, only that it exists. ########## grails-core/src/test/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformationSpec.groovy: ########## @@ -184,41 +169,475 @@ class FooGrailsPlugin { GlobalGrailsClassInjectorTransformation.resolveCompilationTargetDirectory(source, true) then: "the build fails loudly rather than writing to a shared location" - IllegalStateException e = thrown() - e.message.contains(GlobalGrailsClassInjectorTransformation.ISOLATED_BUILD_PROPERTY) + def e = thrown(IllegalStateException) + e.message.contains('grails.isolated.build') } @RestoreSystemProperties void "findSourceDirectory prefers the per-project base.dir system property when set"() { given: "base.dir points at an existing directory" - File baseDir = File.createTempDir() - System.setProperty('base.dir', baseDir.absolutePath) - File target = new File(baseDir, 'build/classes/groovy/main') + System.setProperty('base.dir', tempDir.absolutePath) + def targetDir = new File(tempDir, 'build/classes/groovy/main') when: - File resolved = FactoriesFileWriter.findSourceDirectory(target) + def resolvedDir = FactoriesFileWriter.findSourceDirectory(targetDir) then: "the build-tool supplied base.dir wins" - resolved == baseDir - - cleanup: - baseDir.deleteDir() + resolvedDir == tempDir } @RestoreSystemProperties void "findSourceDirectory walks up to the project directory when base.dir is not set"() { given: "no base.dir and a standard per-project compile target" System.clearProperty('base.dir') - File projectDir = File.createTempDir() - File target = new File(projectDir, 'build/classes/groovy/main') + def targetDir = new File(tempDir, 'build/classes/groovy/main') when: - File resolved = FactoriesFileWriter.findSourceDirectory(target) + def resolvedDir = FactoriesFileWriter.findSourceDirectory(targetDir) then: "it resolves to the parent of the build directory" - resolved == projectDir + resolvedDir == tempDir + } + + void "priority returns the global grails transform order"() { + expect: + new GlobalGrailsClassInjectorTransformation().priority() == GroovyTransformOrder.GLOBAL_GRAILS_TRANSFORM_ORDER + } + + void "the global transform ignores a source without a resolvable URL"() { + when: + new GlobalGrailsClassInjectorTransformation().visit([] as ASTNode[], Stub(SourceUnit) { + getName() >> null + }) + + then: + noExceptionThrown() + } + + void "the global transform stamps a GrailsPlugin descriptor class with a version property"() { + given: "a *GrailsPlugin class with no explicit version property, and nowhere for plugin.xml to exist yet" + def sourceFile = new File(tempDir, 'PlainGrailsPlugin.groovy') + def targetDir = new File(tempDir, 'build/classes/groovy/main') + + when: "the source is compiled, exercising the registered global transform" + def classNode = compileToFile( + sourceFile, + 'class PlainGrailsPlugin {}', + targetDir, [projectVersion: '1.5'] + ) + + then: "the plugin class is stamped with the resolved version" + classNode.getProperty('version') != null + + and: "the plugin.xml describing it is generated as a side effect" + new File(targetDir, 'META-INF/grails-plugin.xml').exists() + } + + void "the global transform resolves a plugin version declared on the plugin class"() { + given: "a plugin descriptor with a declared version and no compiler project metadata" + def sourceFile = new File(tempDir, 'DeclaredGrailsPlugin.groovy') + def targetDir = new File(tempDir, 'build/classes/groovy/main') + + when: + def classNode = compileToFile( + sourceFile, + ''' + class DeclaredGrailsPlugin { + def version = '3.0' + } + ''', + targetDir + ) + + then: + classNode.getProperty('version').initialExpression.text == '3.0' + new File(targetDir, 'META-INF/grails-plugin.xml').exists() + } + + void "the global transform fails when a plugin descriptor class has no version"() { + given: "a plugin descriptor class without a declared or compiler-provided version" + def sourceFile = new File(tempDir, 'UnversionedGrailsPlugin.groovy') + def targetDir = new File(tempDir, 'build/classes/groovy/main') + + when: + compileToFile(sourceFile, 'class UnversionedGrailsPlugin {}', targetDir) + + then: + def exception = thrown(GroovyBugError) + with(exception) { + cause instanceof IllegalStateException + cause.message.contains('does not define a plugin version') + } + } + + void "the global transform annotates a plain Grails resource class with @GrailsPlugin metadata"() { + given: "a class under grails-app that isn't matched by any registered ArtefactHandler" + def sourceFile = new File(tempDir, 'grails-app/services/FooWidget.groovy') + def targetDir = new File(tempDir, 'build/classes/groovy/main') + + when: "the source is compiled, exercising the registered global transform" + def classNode = compileToFile( + sourceFile, + 'class FooWidget {}', + targetDir, + [projectName: 'foowidget', projectVersion: '2.0'] + ) + + then: "the class is stamped with the project's @GrailsPlugin metadata" + def annotations = classNode.getAnnotations(ClassHelper.make(GrailsPlugin)) + annotations.size() == 1 + with(annotations.first()) { + getMember('name').text == GrailsNameUtils.getPropertyNameForLowerCaseHyphenSeparatedName('foowidget') + getMember('version').text == '2.0' + } + } + + void "the global transform processes a Grails service as an artefact"() { + given: "a service source under grails-app" + def sourceFile = new File(tempDir, 'grails-app/services/FooService.groovy') + def targetDir = new File(tempDir, 'build/classes/groovy/main') + TraitInjectionUtils.@traitInjectors = [] Review Comment: `TraitInjectionUtils.traitInjectors` is a `private static` field in a Java class (`TraitInjectionUtils.java:48`), so `.@traitInjectors` is reaching past the public surface into implementation state — the thing AGENTS.md rule 9 asks tests not to do. It appears three times in this spec (299, 547, 571), each with a matching `cleanup:` reset, so it is at least disciplined, but it is global mutable state being toggled from a test and it will break silently the day that field is renamed or initialised eagerly. If real trait injection has to be suppressed for these tests to be fast/deterministic, that is worth an actual seam. Otherwise it may be simpler to let the real injectors load: the three affected features compile a trivial `FooService`/`AnnotatedService`, and I would expect the resolved injector list to be short. Either way, please add a comment explaining *why* it is being nulled out — right now the next reader has no way to tell whether it is a performance shortcut or a correctness requirement. ########## grails-core/src/main/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformation.groovy: ########## @@ -211,153 +231,419 @@ class GlobalGrailsClassInjectorTransformation implements ASTTransformation, Comp return targetDirectory } + /** + * Adds the compiled class to the {@code META-INF/grails.factories} entry for the supplied type + * when it is a concrete subtype of that type. Existing generated entries and matching + * project-source entries are preserved, and the resulting factory file is written to the + * compilation target directory. + * + * @param classNode the class being compiled + * @param superType the factory interface or superclass whose implementations are registered + * @param compilationTargetDirectory the compilation output directory containing the factory file + * @return {@code true} when {@code classNode} is a non-abstract subtype of {@code superType} and + * was registered; {@code false} otherwise + */ static boolean updateGrailsFactoriesWithType(ClassNode classNode, ClassNode superType, File compilationTargetDirectory) { - FactoriesFileWriter.updateFactoriesWithType(classNode, superType, compilationTargetDirectory, - 'META-INF/grails.factories', ['src/main/resources/META-INF/grails.factories']) + FactoriesFileWriter.updateFactoriesWithType( + classNode, + superType, + compilationTargetDirectory, + 'META-INF/grails.factories', + ['src/main/resources/META-INF/grails.factories'] + ) } - static LinkedHashSet<String> pendingPluginClasses = [] - static Collection<String> pluginExcludes = [] + private static boolean updateGrailsFactoriesWithTypes(ClassNode classNode, Collection<ClassNode> superTypes, File compilationTargetDirectory) { + superTypes.any { + updateGrailsFactoriesWithType(classNode, it, compilationTargetDirectory) + } + } - protected static void generatePluginXml(ClassNode pluginClassNode, String pluginVersion, Set<String> transformedClasses, File pluginXmlFile) { + /** + * Creates or updates the generated {@code META-INF/grails-plugin.xml} descriptor and carries + * forward artefact classes collected during compilation. + * + * @param pluginClassNode the compiled plugin descriptor class, or {@code null} when none was found + * @param pluginVersion the plugin version, or {@code null} when no concrete plugin descriptor + * is being generated + * @param transformedClassNames the artefact classes transformed in the current source unit + * @param pluginXmlFile the generated plugin descriptor file + */ + protected void generatePluginXml( + @Nullable ClassNode pluginClassNode, + @Nullable String pluginVersion, + Set<String> transformedClassNames, + File pluginXmlFile + ) { + // first check if plugin.xml exists + pluginXmlFile.parentFile.mkdirs() def pluginXmlExists = pluginXmlFile.exists() - LinkedHashSet<String> pluginClasses = [] - pluginClasses.addAll(transformedClasses) - pluginClasses.addAll(pendingPluginClasses) - - // if the class being transformed is a *GrailsPlugin class then if it doesn't exist create it - if (pluginClassNode && !pluginClassNode.isAbstract()) { + def pluginClasses = [] as LinkedHashSet<String> + pluginClasses.addAll(transformedClassNames) + pluginClasses.addAll(pendingPluginClassNames) + // Reset excludes from a previous source unit so that patterns declared by one plugin + // do not leak into a subsequent compilation within the same Gradle worker. + pluginExcludePatterns.clear() + + // Create or update grails-plugin.xml when a concrete plugin class is present; otherwise, + // update an existing descriptor or defer resource names until the descriptor is compiled. + if (pluginClassNode && !pluginClassNode.abstract) { + if (!pluginVersion) { + throw new IllegalStateException( + "Unable to generate '${pluginXmlFile}' because plugin class '${pluginClassNode.name}' " + + 'does not define a plugin version.' + ) + } if (!pluginXmlExists) { + // The plugin descriptor is being compiled for the first time. writePluginXml(pluginClassNode, pluginVersion, pluginXmlFile, pluginClasses) } else { - // otherwise if the file does exist, update it with the plugin name + // Refresh the existing descriptor with the current plugin metadata and resources. updatePluginXml(pluginClassNode, pluginVersion, pluginXmlFile, pluginClasses) } } else if (pluginXmlExists) { - // if the class isn't the *GrailsPlugin class then only update the plugin.xml if it already exists + // Add resources from this source unit to the existing descriptor. updatePluginXml(null, pluginVersion, pluginXmlFile, pluginClasses) } else { - // otherwise add it to a list of pending classes to populated when the plugin.xml is created - pendingPluginClasses.addAll(transformedClasses) + // Defer these resource names until a source unit compiles the plugin descriptor. + pendingPluginClassNames.addAll(transformedClassNames) } } - @CompileDynamic - static void writePluginXml(ClassNode pluginClassNode, String pluginVersion, File pluginXml, Collection<String> artefactClasses) { + /** + * Writes a new plugin descriptor from the plugin class metadata and supplied artefact classes. + * + * @param pluginClassNode the plugin descriptor class + * @param pluginVersion the required plugin version when {@code pluginClassNode} is present + * @param pluginXml the output descriptor file + * @param artefactClassNames artefact class names to include as resources + */ + void writePluginXml( + @Nullable ClassNode pluginClassNode, + String pluginVersion, + File pluginXml, + Collection<String> artefactClassNames + ) { + pluginXml.parentFile.mkdirs() if (pluginClassNode) { - PluginAstReader pluginAstReader = new PluginAstReader() - def info = pluginAstReader.readPluginInfo(pluginClassNode) - - pluginXml.withWriter(StandardCharsets.UTF_8.name()) { Writer writer -> - def mkp = new MarkupBuilder(writer) - def pluginName = GrailsNameUtils.getLogicalPropertyName(pluginClassNode.name, 'GrailsPlugin') - - def pluginProperties = info.getProperties() - def excludes = pluginProperties.get('pluginExcludes') - if (excludes instanceof List) { - pluginExcludes.clear() - pluginExcludes.addAll(excludes) - } + writePluginXmlWithDescriptor(pluginClassNode, pluginVersion, pluginXml, artefactClassNames) + } else { + writePluginXmlWithoutDescriptor(pluginXml, artefactClassNames) + } + pendingPluginClassNames.clear() Review Comment: Dropping the old `if (pluginClassNode)` guard means `writePluginXml(null, ...)` is no longer a no-op, and that changes what happens to a corrupt descriptor. I ran `updatePluginXml(null, null, file, ['Foo'])` against `<plugin><resources>` on both branches: - `8.0.x`: file left untouched — `<plugin><resources>` - this branch: file replaced with `<plugin><resources><resource>Foo</resource></resources></plugin>` It does answer the Copilot thread about `pendingPluginClassNames` never being cleared, but the replacement is a descriptor with no `name`, no `version` and no `<type>`. `PluginXmlHandler` will happily parse it and hand back `providedClasses` that belong to no plugin, so under incremental compilation — where the `*GrailsPlugin` source unit is not recompiled — the corrupt file is swapped for a well-formed but meaningless one, and the failure stops being visible. Two alternatives that both keep the pending-list fix without inventing a nameless descriptor: delete the unusable file and fall back to the `pendingPluginClassNames.addAll(...)` deferral path, or keep writing but log at WARN that the descriptor is being written without plugin identity. Whichever you pick, the "recreates safely when the existing descriptor is malformed" test should assert the resulting *content*, not just that no exception escaped — right now it would pass for either behaviour. ########## grails-core/src/test/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformationSpec.groovy: ########## @@ -184,41 +169,475 @@ class FooGrailsPlugin { GlobalGrailsClassInjectorTransformation.resolveCompilationTargetDirectory(source, true) then: "the build fails loudly rather than writing to a shared location" - IllegalStateException e = thrown() - e.message.contains(GlobalGrailsClassInjectorTransformation.ISOLATED_BUILD_PROPERTY) + def e = thrown(IllegalStateException) + e.message.contains('grails.isolated.build') } @RestoreSystemProperties void "findSourceDirectory prefers the per-project base.dir system property when set"() { given: "base.dir points at an existing directory" - File baseDir = File.createTempDir() - System.setProperty('base.dir', baseDir.absolutePath) - File target = new File(baseDir, 'build/classes/groovy/main') + System.setProperty('base.dir', tempDir.absolutePath) + def targetDir = new File(tempDir, 'build/classes/groovy/main') when: - File resolved = FactoriesFileWriter.findSourceDirectory(target) + def resolvedDir = FactoriesFileWriter.findSourceDirectory(targetDir) then: "the build-tool supplied base.dir wins" - resolved == baseDir - - cleanup: - baseDir.deleteDir() + resolvedDir == tempDir } @RestoreSystemProperties void "findSourceDirectory walks up to the project directory when base.dir is not set"() { given: "no base.dir and a standard per-project compile target" System.clearProperty('base.dir') - File projectDir = File.createTempDir() - File target = new File(projectDir, 'build/classes/groovy/main') + def targetDir = new File(tempDir, 'build/classes/groovy/main') when: - File resolved = FactoriesFileWriter.findSourceDirectory(target) + def resolvedDir = FactoriesFileWriter.findSourceDirectory(targetDir) then: "it resolves to the parent of the build directory" - resolved == projectDir + resolvedDir == tempDir + } + + void "priority returns the global grails transform order"() { + expect: + new GlobalGrailsClassInjectorTransformation().priority() == GroovyTransformOrder.GLOBAL_GRAILS_TRANSFORM_ORDER + } + + void "the global transform ignores a source without a resolvable URL"() { + when: + new GlobalGrailsClassInjectorTransformation().visit([] as ASTNode[], Stub(SourceUnit) { + getName() >> null + }) + + then: + noExceptionThrown() + } + + void "the global transform stamps a GrailsPlugin descriptor class with a version property"() { + given: "a *GrailsPlugin class with no explicit version property, and nowhere for plugin.xml to exist yet" + def sourceFile = new File(tempDir, 'PlainGrailsPlugin.groovy') + def targetDir = new File(tempDir, 'build/classes/groovy/main') + + when: "the source is compiled, exercising the registered global transform" + def classNode = compileToFile( + sourceFile, + 'class PlainGrailsPlugin {}', + targetDir, [projectVersion: '1.5'] + ) + + then: "the plugin class is stamped with the resolved version" + classNode.getProperty('version') != null + + and: "the plugin.xml describing it is generated as a side effect" + new File(targetDir, 'META-INF/grails-plugin.xml').exists() + } + + void "the global transform resolves a plugin version declared on the plugin class"() { + given: "a plugin descriptor with a declared version and no compiler project metadata" + def sourceFile = new File(tempDir, 'DeclaredGrailsPlugin.groovy') + def targetDir = new File(tempDir, 'build/classes/groovy/main') + + when: + def classNode = compileToFile( + sourceFile, + ''' + class DeclaredGrailsPlugin { + def version = '3.0' + } + ''', + targetDir + ) + + then: + classNode.getProperty('version').initialExpression.text == '3.0' + new File(targetDir, 'META-INF/grails-plugin.xml').exists() + } + + void "the global transform fails when a plugin descriptor class has no version"() { + given: "a plugin descriptor class without a declared or compiler-provided version" + def sourceFile = new File(tempDir, 'UnversionedGrailsPlugin.groovy') + def targetDir = new File(tempDir, 'build/classes/groovy/main') + + when: + compileToFile(sourceFile, 'class UnversionedGrailsPlugin {}', targetDir) + + then: + def exception = thrown(GroovyBugError) + with(exception) { + cause instanceof IllegalStateException + cause.message.contains('does not define a plugin version') + } Review Comment: This test locks in the diagnostic rather than questioning it. `GroovyBugError` is Groovy's wrapper for an unexpected `RuntimeException` escaping a phase operation, so what this asserts is "a plugin author with a misconfigured descriptor is told they hit a compiler bug" — see my comment on the `throw` site. If the failure is routed through `GrailsASTUtils.error(...)` instead, this becomes a much better test *and* a much better error: assert `MultipleCompilationErrorsException` and that the collected message names the descriptor class, which also pins the source location the error collector attaches. Reaching through `thrown(GroovyBugError)` to `cause.message` is also fragile against Groovy changing how it wraps phase-operation failures, which is a second reason not to depend on it. ########## grails-core/src/main/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformation.groovy: ########## @@ -211,153 +231,419 @@ class GlobalGrailsClassInjectorTransformation implements ASTTransformation, Comp return targetDirectory } + /** + * Adds the compiled class to the {@code META-INF/grails.factories} entry for the supplied type + * when it is a concrete subtype of that type. Existing generated entries and matching + * project-source entries are preserved, and the resulting factory file is written to the + * compilation target directory. + * + * @param classNode the class being compiled + * @param superType the factory interface or superclass whose implementations are registered + * @param compilationTargetDirectory the compilation output directory containing the factory file + * @return {@code true} when {@code classNode} is a non-abstract subtype of {@code superType} and + * was registered; {@code false} otherwise + */ static boolean updateGrailsFactoriesWithType(ClassNode classNode, ClassNode superType, File compilationTargetDirectory) { - FactoriesFileWriter.updateFactoriesWithType(classNode, superType, compilationTargetDirectory, - 'META-INF/grails.factories', ['src/main/resources/META-INF/grails.factories']) + FactoriesFileWriter.updateFactoriesWithType( + classNode, + superType, + compilationTargetDirectory, + 'META-INF/grails.factories', + ['src/main/resources/META-INF/grails.factories'] + ) } - static LinkedHashSet<String> pendingPluginClasses = [] - static Collection<String> pluginExcludes = [] + private static boolean updateGrailsFactoriesWithTypes(ClassNode classNode, Collection<ClassNode> superTypes, File compilationTargetDirectory) { + superTypes.any { + updateGrailsFactoriesWithType(classNode, it, compilationTargetDirectory) + } + } - protected static void generatePluginXml(ClassNode pluginClassNode, String pluginVersion, Set<String> transformedClasses, File pluginXmlFile) { + /** + * Creates or updates the generated {@code META-INF/grails-plugin.xml} descriptor and carries + * forward artefact classes collected during compilation. + * + * @param pluginClassNode the compiled plugin descriptor class, or {@code null} when none was found + * @param pluginVersion the plugin version, or {@code null} when no concrete plugin descriptor + * is being generated + * @param transformedClassNames the artefact classes transformed in the current source unit + * @param pluginXmlFile the generated plugin descriptor file + */ + protected void generatePluginXml( + @Nullable ClassNode pluginClassNode, + @Nullable String pluginVersion, + Set<String> transformedClassNames, + File pluginXmlFile + ) { + // first check if plugin.xml exists + pluginXmlFile.parentFile.mkdirs() def pluginXmlExists = pluginXmlFile.exists() - LinkedHashSet<String> pluginClasses = [] - pluginClasses.addAll(transformedClasses) - pluginClasses.addAll(pendingPluginClasses) - - // if the class being transformed is a *GrailsPlugin class then if it doesn't exist create it - if (pluginClassNode && !pluginClassNode.isAbstract()) { + def pluginClasses = [] as LinkedHashSet<String> + pluginClasses.addAll(transformedClassNames) + pluginClasses.addAll(pendingPluginClassNames) + // Reset excludes from a previous source unit so that patterns declared by one plugin + // do not leak into a subsequent compilation within the same Gradle worker. + pluginExcludePatterns.clear() + + // Create or update grails-plugin.xml when a concrete plugin class is present; otherwise, + // update an existing descriptor or defer resource names until the descriptor is compiled. + if (pluginClassNode && !pluginClassNode.abstract) { + if (!pluginVersion) { + throw new IllegalStateException( + "Unable to generate '${pluginXmlFile}' because plugin class '${pluginClassNode.name}' " + + 'does not define a plugin version.' + ) + } if (!pluginXmlExists) { + // The plugin descriptor is being compiled for the first time. writePluginXml(pluginClassNode, pluginVersion, pluginXmlFile, pluginClasses) } else { - // otherwise if the file does exist, update it with the plugin name + // Refresh the existing descriptor with the current plugin metadata and resources. updatePluginXml(pluginClassNode, pluginVersion, pluginXmlFile, pluginClasses) } } else if (pluginXmlExists) { - // if the class isn't the *GrailsPlugin class then only update the plugin.xml if it already exists + // Add resources from this source unit to the existing descriptor. updatePluginXml(null, pluginVersion, pluginXmlFile, pluginClasses) } else { - // otherwise add it to a list of pending classes to populated when the plugin.xml is created - pendingPluginClasses.addAll(transformedClasses) + // Defer these resource names until a source unit compiles the plugin descriptor. + pendingPluginClassNames.addAll(transformedClassNames) } } - @CompileDynamic - static void writePluginXml(ClassNode pluginClassNode, String pluginVersion, File pluginXml, Collection<String> artefactClasses) { + /** + * Writes a new plugin descriptor from the plugin class metadata and supplied artefact classes. + * + * @param pluginClassNode the plugin descriptor class + * @param pluginVersion the required plugin version when {@code pluginClassNode} is present + * @param pluginXml the output descriptor file + * @param artefactClassNames artefact class names to include as resources + */ + void writePluginXml( + @Nullable ClassNode pluginClassNode, + String pluginVersion, + File pluginXml, + Collection<String> artefactClassNames + ) { + pluginXml.parentFile.mkdirs() if (pluginClassNode) { - PluginAstReader pluginAstReader = new PluginAstReader() - def info = pluginAstReader.readPluginInfo(pluginClassNode) - - pluginXml.withWriter(StandardCharsets.UTF_8.name()) { Writer writer -> - def mkp = new MarkupBuilder(writer) - def pluginName = GrailsNameUtils.getLogicalPropertyName(pluginClassNode.name, 'GrailsPlugin') - - def pluginProperties = info.getProperties() - def excludes = pluginProperties.get('pluginExcludes') - if (excludes instanceof List) { - pluginExcludes.clear() - pluginExcludes.addAll(excludes) - } + writePluginXmlWithDescriptor(pluginClassNode, pluginVersion, pluginXml, artefactClassNames) + } else { + writePluginXmlWithoutDescriptor(pluginXml, artefactClassNames) + } + pendingPluginClassNames.clear() + } - def grailsVersion = pluginProperties['grailsVersion'] ?: getClass().getPackage().getImplementationVersion() + ' > *' - mkp.plugin(name: pluginName, version: pluginVersion, grailsVersion: grailsVersion) { - type(pluginClassNode.name) + @CompileDynamic + private void writePluginXmlWithDescriptor( + ClassNode pluginClassNode, + String pluginVersion, + File pluginXml, + Collection<String> artefactClassNames + ) { + def pluginInfo = new PluginAstReader().readPluginInfo(pluginClassNode) + pluginXml.withWriter(StandardCharsets.UTF_8.name()) { Writer writer -> + def markupBuilder = new MarkupBuilder(writer) + def pluginName = GrailsNameUtils.getLogicalPropertyName(pluginClassNode.name, 'GrailsPlugin') + def pluginProperties = pluginInfo.properties + def pluginExcludes = pluginProperties.get('pluginExcludes') + if (pluginExcludes instanceof List) { + pluginExcludePatterns.clear() + pluginExcludePatterns.addAll(pluginExcludes) + } - for (entry in pluginProperties) { - delegate."$entry.key"(entry.value) - } + // if the plugin class doesn't define a grailsVersion, use the version of the grails-core jar + def grailsVersion = pluginProperties['grailsVersion'] ?: + GlobalGrailsClassInjectorTransformation.package.implementationVersion + ' > *' - // if there are pending classes to add to the plugin.xml add those - if (artefactClasses) { - def antPathMatcher = new AntPathMatcher() - resources { - for (String cn in artefactClasses) { - if (!pluginExcludes.any() { String exc -> antPathMatcher.match(exc, cn.replace('.', '/')) }) { - resource(cn) - } + markupBuilder.plugin(name: pluginName, version: pluginVersion, grailsVersion: grailsVersion) { + type(pluginClassNode.name) + + for (def entry : pluginProperties) { + delegate."$entry.key"(entry.value) + } + + // if there are pending class names to add to the plugin.xml - add them as resources + if (artefactClassNames) { + resources { + for (def artefactClassName : artefactClassNames) { + if (!isResourceExcludedByPlugin(artefactClassName)) { + resource(artefactClassName) } } } } } - - pendingPluginClasses.clear() } } @CompileDynamic - static void updatePluginXml(ClassNode pluginClassNode, String pluginVersion, File pluginXmlFile, Collection<String> artefactClasses) { - if (!artefactClasses) return + private void writePluginXmlWithoutDescriptor(File pluginXml, Collection<String> artefactClassNames) { + pluginXml.withWriter(StandardCharsets.UTF_8.name()) { Writer writer -> + new MarkupBuilder(writer).plugin { + resources { + for (def artefactClassName : artefactClassNames) { + if (!isResourceExcludedByPlugin(artefactClassName)) { + resource(artefactClassName) + } + } + } + } + } + } + /** + * Updates an existing plugin descriptor with plugin metadata and newly discovered artefact + * resources. If the descriptor cannot be read or written, it is recreated. Review Comment: "If the descriptor cannot be read or written, it is recreated" no longer matches the code. The catch on line 429 covers parse and I/O failures only; a `GroovyRuntimeException` from the `@CompileDynamic` GPath manipulation in `writePluginXmlProperties` now propagates and fails the build. That narrowing is the right call — it is what would have surfaced the `mkp` defect immediately — but the contract in the doc should say so, e.g. "If the descriptor cannot be parsed or written, it is recreated; any other failure aborts compilation." ########## grails-core/src/main/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformation.groovy: ########## @@ -211,153 +231,419 @@ class GlobalGrailsClassInjectorTransformation implements ASTTransformation, Comp return targetDirectory } + /** + * Adds the compiled class to the {@code META-INF/grails.factories} entry for the supplied type + * when it is a concrete subtype of that type. Existing generated entries and matching + * project-source entries are preserved, and the resulting factory file is written to the + * compilation target directory. + * + * @param classNode the class being compiled + * @param superType the factory interface or superclass whose implementations are registered + * @param compilationTargetDirectory the compilation output directory containing the factory file + * @return {@code true} when {@code classNode} is a non-abstract subtype of {@code superType} and + * was registered; {@code false} otherwise + */ static boolean updateGrailsFactoriesWithType(ClassNode classNode, ClassNode superType, File compilationTargetDirectory) { - FactoriesFileWriter.updateFactoriesWithType(classNode, superType, compilationTargetDirectory, - 'META-INF/grails.factories', ['src/main/resources/META-INF/grails.factories']) + FactoriesFileWriter.updateFactoriesWithType( + classNode, + superType, + compilationTargetDirectory, + 'META-INF/grails.factories', + ['src/main/resources/META-INF/grails.factories'] + ) } - static LinkedHashSet<String> pendingPluginClasses = [] - static Collection<String> pluginExcludes = [] + private static boolean updateGrailsFactoriesWithTypes(ClassNode classNode, Collection<ClassNode> superTypes, File compilationTargetDirectory) { + superTypes.any { + updateGrailsFactoriesWithType(classNode, it, compilationTargetDirectory) + } + } - protected static void generatePluginXml(ClassNode pluginClassNode, String pluginVersion, Set<String> transformedClasses, File pluginXmlFile) { + /** + * Creates or updates the generated {@code META-INF/grails-plugin.xml} descriptor and carries + * forward artefact classes collected during compilation. + * + * @param pluginClassNode the compiled plugin descriptor class, or {@code null} when none was found + * @param pluginVersion the plugin version, or {@code null} when no concrete plugin descriptor + * is being generated + * @param transformedClassNames the artefact classes transformed in the current source unit + * @param pluginXmlFile the generated plugin descriptor file + */ + protected void generatePluginXml( + @Nullable ClassNode pluginClassNode, + @Nullable String pluginVersion, + Set<String> transformedClassNames, + File pluginXmlFile + ) { + // first check if plugin.xml exists + pluginXmlFile.parentFile.mkdirs() def pluginXmlExists = pluginXmlFile.exists() - LinkedHashSet<String> pluginClasses = [] - pluginClasses.addAll(transformedClasses) - pluginClasses.addAll(pendingPluginClasses) - - // if the class being transformed is a *GrailsPlugin class then if it doesn't exist create it - if (pluginClassNode && !pluginClassNode.isAbstract()) { + def pluginClasses = [] as LinkedHashSet<String> + pluginClasses.addAll(transformedClassNames) + pluginClasses.addAll(pendingPluginClassNames) + // Reset excludes from a previous source unit so that patterns declared by one plugin + // do not leak into a subsequent compilation within the same Gradle worker. + pluginExcludePatterns.clear() + + // Create or update grails-plugin.xml when a concrete plugin class is present; otherwise, + // update an existing descriptor or defer resource names until the descriptor is compiled. + if (pluginClassNode && !pluginClassNode.abstract) { + if (!pluginVersion) { + throw new IllegalStateException( + "Unable to generate '${pluginXmlFile}' because plugin class '${pluginClassNode.name}' " + + 'does not define a plugin version.' + ) + } if (!pluginXmlExists) { + // The plugin descriptor is being compiled for the first time. writePluginXml(pluginClassNode, pluginVersion, pluginXmlFile, pluginClasses) } else { - // otherwise if the file does exist, update it with the plugin name + // Refresh the existing descriptor with the current plugin metadata and resources. updatePluginXml(pluginClassNode, pluginVersion, pluginXmlFile, pluginClasses) } } else if (pluginXmlExists) { - // if the class isn't the *GrailsPlugin class then only update the plugin.xml if it already exists + // Add resources from this source unit to the existing descriptor. updatePluginXml(null, pluginVersion, pluginXmlFile, pluginClasses) } else { - // otherwise add it to a list of pending classes to populated when the plugin.xml is created - pendingPluginClasses.addAll(transformedClasses) + // Defer these resource names until a source unit compiles the plugin descriptor. + pendingPluginClassNames.addAll(transformedClassNames) } } - @CompileDynamic - static void writePluginXml(ClassNode pluginClassNode, String pluginVersion, File pluginXml, Collection<String> artefactClasses) { + /** + * Writes a new plugin descriptor from the plugin class metadata and supplied artefact classes. + * + * @param pluginClassNode the plugin descriptor class + * @param pluginVersion the required plugin version when {@code pluginClassNode} is present + * @param pluginXml the output descriptor file + * @param artefactClassNames artefact class names to include as resources + */ + void writePluginXml( + @Nullable ClassNode pluginClassNode, + String pluginVersion, + File pluginXml, + Collection<String> artefactClassNames + ) { + pluginXml.parentFile.mkdirs() if (pluginClassNode) { - PluginAstReader pluginAstReader = new PluginAstReader() - def info = pluginAstReader.readPluginInfo(pluginClassNode) - - pluginXml.withWriter(StandardCharsets.UTF_8.name()) { Writer writer -> - def mkp = new MarkupBuilder(writer) - def pluginName = GrailsNameUtils.getLogicalPropertyName(pluginClassNode.name, 'GrailsPlugin') - - def pluginProperties = info.getProperties() - def excludes = pluginProperties.get('pluginExcludes') - if (excludes instanceof List) { - pluginExcludes.clear() - pluginExcludes.addAll(excludes) - } + writePluginXmlWithDescriptor(pluginClassNode, pluginVersion, pluginXml, artefactClassNames) + } else { + writePluginXmlWithoutDescriptor(pluginXml, artefactClassNames) + } + pendingPluginClassNames.clear() + } - def grailsVersion = pluginProperties['grailsVersion'] ?: getClass().getPackage().getImplementationVersion() + ' > *' - mkp.plugin(name: pluginName, version: pluginVersion, grailsVersion: grailsVersion) { - type(pluginClassNode.name) + @CompileDynamic + private void writePluginXmlWithDescriptor( + ClassNode pluginClassNode, + String pluginVersion, + File pluginXml, + Collection<String> artefactClassNames + ) { + def pluginInfo = new PluginAstReader().readPluginInfo(pluginClassNode) + pluginXml.withWriter(StandardCharsets.UTF_8.name()) { Writer writer -> + def markupBuilder = new MarkupBuilder(writer) + def pluginName = GrailsNameUtils.getLogicalPropertyName(pluginClassNode.name, 'GrailsPlugin') + def pluginProperties = pluginInfo.properties + def pluginExcludes = pluginProperties.get('pluginExcludes') + if (pluginExcludes instanceof List) { + pluginExcludePatterns.clear() + pluginExcludePatterns.addAll(pluginExcludes) + } - for (entry in pluginProperties) { - delegate."$entry.key"(entry.value) - } + // if the plugin class doesn't define a grailsVersion, use the version of the grails-core jar + def grailsVersion = pluginProperties['grailsVersion'] ?: + GlobalGrailsClassInjectorTransformation.package.implementationVersion + ' > *' - // if there are pending classes to add to the plugin.xml add those - if (artefactClasses) { - def antPathMatcher = new AntPathMatcher() - resources { - for (String cn in artefactClasses) { - if (!pluginExcludes.any() { String exc -> antPathMatcher.match(exc, cn.replace('.', '/')) }) { - resource(cn) - } + markupBuilder.plugin(name: pluginName, version: pluginVersion, grailsVersion: grailsVersion) { + type(pluginClassNode.name) + + for (def entry : pluginProperties) { + delegate."$entry.key"(entry.value) + } + + // if there are pending class names to add to the plugin.xml - add them as resources + if (artefactClassNames) { + resources { + for (def artefactClassName : artefactClassNames) { + if (!isResourceExcludedByPlugin(artefactClassName)) { + resource(artefactClassName) } } } } } - - pendingPluginClasses.clear() } } @CompileDynamic - static void updatePluginXml(ClassNode pluginClassNode, String pluginVersion, File pluginXmlFile, Collection<String> artefactClasses) { - if (!artefactClasses) return + private void writePluginXmlWithoutDescriptor(File pluginXml, Collection<String> artefactClassNames) { + pluginXml.withWriter(StandardCharsets.UTF_8.name()) { Writer writer -> + new MarkupBuilder(writer).plugin { + resources { + for (def artefactClassName : artefactClassNames) { + if (!isResourceExcludedByPlugin(artefactClassName)) { + resource(artefactClassName) + } + } + } + } + } + } + /** + * Updates an existing plugin descriptor with plugin metadata and newly discovered artefact + * resources. If the descriptor cannot be read or written, it is recreated. + * + * @param pluginClassNode the plugin descriptor class, or {@code null} when only resources are updated + * @param pluginVersion the plugin version, or {@code null} when only resources are updated + * @param pluginXmlFile the existing plugin descriptor file + * @param artefactClassNames artefact class names to add as resources + */ + void updatePluginXml( + @Nullable ClassNode pluginClassNode, + @Nullable String pluginVersion, + File pluginXmlFile, + Collection<String> artefactClassNames + ) { + if (!artefactClassNames) return try { - XmlSlurper xmlSlurper = IOUtils.createXmlSlurper() - - def pluginXml = xmlSlurper.parse(pluginXmlFile) + def pluginXml = IOUtils.createXmlSlurper().parse(pluginXmlFile) if (pluginClassNode) { - def pluginName = GrailsNameUtils.getLogicalPropertyName(pluginClassNode.name, 'GrailsPlugin') - pluginXml.@name = pluginName - pluginXml.@version = pluginVersion - pluginXml.type = pluginClassNode.name - - PluginAstReader pluginAstReader = new PluginAstReader() - def info = pluginAstReader.readPluginInfo(pluginClassNode) - - def pluginProperties = info.getProperties() - def grailsVersion = pluginProperties['grailsVersion'] ?: getClass().getPackage().getImplementationVersion() + ' > *' - pluginXml.@grailsVersion = grailsVersion - for (entry in pluginProperties) { - pluginXml."$entry.key" = entry.value + def pluginProperties = writePluginXmlProperties(pluginClassNode, pluginVersion, pluginXml) + def pluginExcludes = pluginProperties.get('pluginExcludes') + if (pluginExcludes instanceof List) { + pluginExcludePatterns.clear() + pluginExcludePatterns.addAll(pluginExcludes as List<String>) } + } + writePluginXmlResources(pluginXml, artefactClassNames) + handleExcludes(pluginXml) - def excludes = pluginProperties.get('pluginExcludes') - if (excludes instanceof List) { - pluginExcludes.clear() - pluginExcludes.addAll(excludes) - } + pluginXmlFile.withWriter(StandardCharsets.UTF_8.name()) { + createMarkup(pluginXml).writeTo(it) } - def resources = pluginXml.resources + pendingPluginClassNames.clear() - for (String cn in artefactClasses) { - if (!resources.resource.find { it.text() == cn }) { - resources.appendNode { - resource(cn) - } + } catch (IOException | ParserConfigurationException | SAXException e) { + // Invalid or unreadable descriptor; recreate it + log.warn('Failed to update existing file {}. Recreating it instead...', pluginXmlFile.absolutePath, e) + writePluginXml(pluginClassNode, pluginVersion, pluginXmlFile, artefactClassNames) + } + } + + /** + * Removes resources matching the configured plugin exclusion patterns from a parsed descriptor. + * + * @param pluginXml the parsed plugin descriptor + */ + @CompileDynamic + protected void handleExcludes(GPathResult pluginXml) { + if (pluginExcludePatterns) { + pluginXml.resources.resource.each { resourceNode -> + if (isResourceExcludedByPlugin((resourceNode as GPathResult).text())) { + resourceNode.replaceNode {} } } + } + } - handleExcludes(pluginXml) + /** + * Determines whether a resource name matches any configured plugin exclusion pattern. + * + * @param resourceName the resource name to test + * @return {@code true} when the resource should be excluded + */ + private boolean isResourceExcludedByPlugin(String resourceName) { + def resourcePath = resourceName.replace('.', '/') + pluginExcludePatterns.any { + ANT_PATH_MATCHER.match(it, resourcePath) + } + } - Writable writable = new StreamingMarkupBuilder().bind { - mkp.yield(pluginXml) - } + /** + * Creates a writable representation of a parsed plugin descriptor. + * + * @param node the parsed XML node + * @return a writable representation of the node + */ + private static Writable createMarkup(GPathResult node) { + (Writable) new StreamingMarkupBuilder().bindNode(node) + } - pluginXmlFile.withWriter(StandardCharsets.UTF_8.name()) { Writer writer -> - writable.writeTo(writer) - } + /** + * Resolves the project version recorded in compiler metadata, falling back to the Grails + * implementation version when no project version is available. + * + * @param classNode the class whose compiler metadata is inspected + * @return the resolved project version + */ + private static String resolveProjectVersion(ClassNode classNode) { + def projectVersion = classNode.getNodeMetaData('projectVersion')?.toString() + if (projectVersion == null) { + // fallback to the version of the grails-core jar if no project version is available + projectVersion = GlobalGrailsClassInjectorTransformation.package.implementationVersion + } + projectVersion + } + + /** + * Resolves the project name recorded in compiler metadata. + * + * @param classNode the class whose compiler metadata is inspected + * @return the project name, or {@code null} when it is not present + */ + private static @Nullable String resolveProjectName(ClassNode classNode) { + classNode.getNodeMetaData('projectName')?.toString() + } - pendingPluginClasses.clear() + /** + * Determines whether a source belongs to a project that should be processed by this + * transformation. + * + * @param url the source URL + * @return {@code true} when the URL identifies project source + */ + private static boolean shouldVisit(@Nullable URL url) { + url != null && GrailsResourceUtils.isProjectSource(new UrlResource(url)) + } + + /** + * Determines whether a class is a concrete Grails plugin descriptor class. + * + * @param classNode the class to inspect + * @return {@code true} when the class name ends with {@code GrailsPlugin} and is not abstract + */ + private static boolean isGrailsPluginDescriptorClass(ClassNode classNode) { + classNode.name.endsWith('GrailsPlugin') && !classNode.abstract + } - } catch (e) { - // corrupt, recreate - writePluginXml(pluginClassNode, pluginVersion, pluginXmlFile, artefactClasses) + /** + * Resolves the plugin version from compiler metadata or from the plugin class's declared + * version property. + * + * @param classNode the plugin descriptor class + * @param projectVersion the version recorded in compiler metadata + * @return the resolved plugin version, or {@code null} when neither source defines one + */ + private static @Nullable String resolvePluginVersion(ClassNode classNode, @Nullable String projectVersion) { + if (projectVersion) { + return projectVersion } + def versionField = classNode.getDeclaredField('version') + def initialExpression = versionField?.initialExpression + initialExpression instanceof ConstantExpression ? initialExpression.text : null } + /** + * Adds the generated version property to a plugin descriptor class when it does not already + * declare one. + * + * @param classNode the plugin descriptor class + * @param pluginVersion the plugin version + */ + private static void addPluginVersionProperty(ClassNode classNode, String pluginVersion) { + if (!classNode.hasProperty('version')) { + classNode.addProperty( + new PropertyNode( + 'version', + Modifier.PUBLIC, + ClassHelper.make(Object), + classNode, + new ConstantExpression(pluginVersion), + null, + null + ) + ) + } + } + + /** + * Adds the Grails plugin annotation containing the project name and version to a class. + * + * @param classNode the class to annotate + * @param projectName the project name + * @param projectVersion the project version + */ + private static void addPluginAnnotation(ClassNode classNode, String projectName, String projectVersion) { + GrailsASTUtils.addAnnotationOrGetExisting( + classNode, + GrailsPlugin, + [ + name: GrailsNameUtils.getPropertyNameForLowerCaseHyphenSeparatedName(projectName), + version: projectVersion + ] as Map<String, Object> + ) + } + + /** + * Adds an import to the module containing the class. + * + * @param classNode the class whose module should receive the import + * @param className the fully qualified class name to import + */ + private static void addImport(ClassNode classNode, String className) { + classNode.module.addImport( + className.tokenize('.')[-1], + ClassHelper.make(className) + ) + } + + /** + * Adds an {@link Artefact} annotation identifying the artefact handler type. + * + * @param classNode the artefact class + * @param handlerType the artefact handler type + */ + private static void addArtefactAnnotation(ClassNode classNode, String handlerType) { + def annotationNode = new AnnotationNode(new ClassNode(Artefact)) + annotationNode.addMember('value', new ConstantExpression(handlerType)) + classNode.addAnnotation(annotationNode) + } + + /** + * Copies plugin metadata from a plugin class into an existing parsed descriptor. + * + * @param pluginClassNode the plugin descriptor class + * @param pluginVersion the plugin version + * @param pluginXml the parsed plugin descriptor + * @return the plugin properties extracted from the class + */ @CompileDynamic - protected static void handleExcludes(GPathResult pluginXml) { - if (pluginExcludes) { + private static Map writePluginXmlProperties( Review Comment: Re-raising this from the last round since it got more confusing rather than less. `writePluginXmlProperties` and `writePluginXmlResources` are helpers for **`updatePluginXml`** — they mutate a parsed `GPathResult` — but this PR also introduced `writePluginXmlWithDescriptor` / `writePluginXmlWithoutDescriptor`, which really are helpers for `writePluginXml`. Four `writePluginXml*` methods, split across two unrelated code paths, is the opposite of what the rest of this refactor achieves. `updatePluginXmlProperties` / `updatePluginXmlResources` removes the ambiguity for free. While you're in here: these two sit at the bottom of the class, ~200 lines from their only caller at line 413, with `handleExcludes`, `createMarkup`, `resolveProjectVersion` and the whole `addXxx` family in between. Moving them next to `updatePluginXml` would make the create-path/update-path split readable at a glance. -- 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]
