jdaugherty commented on code in PR #16042:
URL: https://github.com/apache/grails-core/pull/16042#discussion_r3652174381
##########
grails-core/src/main/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformation.groovy:
##########
@@ -347,17 +303,104 @@ class GlobalGrailsClassInjectorTransformation implements
ASTTransformation, Comp
@CompileDynamic
protected static void handleExcludes(GPathResult pluginXml) {
if (pluginExcludes) {
-
def antPathMatcher = new AntPathMatcher()
- pluginXml.resources.resource.each { res ->
- if (pluginExcludes.any() { String exc ->
antPathMatcher.match(exc, res.text().replace('.', '/')) }) {
- res.replaceNode {}
+ pluginXml.resources.resource.each {
+ def resourceNode = it as GPathResult
+ if (pluginExcludes.any() { antPathMatcher.match(it,
resourceNode.text().replace('.', '/')) }) {
+ resourceNode.replaceNode {}
}
}
}
}
- public static final ClassNode ARTEFACT_CLASS_NODE = new ClassNode(Artefact)
+ @CompileDynamic
+ private static Writable createMarkup(GPathResult node) {
+ new StreamingMarkupBuilder().mkp.yield(node)
Review Comment:
This throws every time it is called. `mkp` is not a property of
`StreamingMarkupBuilder` — it is a pseudo-namespace that only resolves against
the builder's delegate *inside* a `bind {}` closure, so accessing it on the
instance fails:
```
groovy.lang.MissingPropertyException: No such property: mkp for class:
groovy.xml.StreamingMarkupBuilder
Possible solutions: qt
```
(verified against groovy-xml 5.0.7; `bind { mkp.yield(node) }` on the same
node returns the expected `<plugin name='foo'>...</plugin>`.)
Because `updatePluginXml` wraps everything in `catch (ignored)`, the failure
is swallowed and the method falls through to `writePluginXml`. Two consequences:
- For the `pluginClassNode == null` branch — `generatePluginXml` →
`updatePluginXml(null, ...)`, which is the path every non-descriptor source
unit takes — the fallback `writePluginXml(null, ...)` is a no-op because of its
`if (pluginClassNode)` guard. Artefact resources discovered in source units
compiled after the `*GrailsPlugin` class are therefore silently dropped from
`META-INF/grails-plugin.xml`, and `pendingPluginClasses` is never cleared.
- For the `pluginClassNode != null` branch the descriptor is fully
regenerated from the AST on every pass rather than merged, so anything already
in the file that is not reproducible from the AST is lost.
Keeping the `bind {}` form preserves the original behaviour and still gets
you the extracted helper:
```groovy
@CompileDynamic
private static Writable createMarkup(GPathResult node) {
new StreamingMarkupBuilder().bind { mkp.yield(node) }
}
```
(`bindNode(node)` — defined as `bind { out << node }` — may also fit, but it
is a different rendering call than `mkp.yield`, so it would need verifying
against the descriptor output.)
Worth a test that fails on the current code: put an element into an existing
`plugin.xml` that the AST cannot reproduce (or call `updatePluginXml(null,
'1.0', file, ['Foo'])`) and assert it survives the update.
##########
grails-core/src/main/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformation.groovy:
##########
@@ -347,17 +303,104 @@ class GlobalGrailsClassInjectorTransformation implements
ASTTransformation, Comp
@CompileDynamic
protected static void handleExcludes(GPathResult pluginXml) {
if (pluginExcludes) {
-
def antPathMatcher = new AntPathMatcher()
- pluginXml.resources.resource.each { res ->
- if (pluginExcludes.any() { String exc ->
antPathMatcher.match(exc, res.text().replace('.', '/')) }) {
- res.replaceNode {}
+ pluginXml.resources.resource.each {
+ def resourceNode = it as GPathResult
+ if (pluginExcludes.any() { antPathMatcher.match(it,
resourceNode.text().replace('.', '/')) }) {
+ resourceNode.replaceNode {}
}
}
}
}
- public static final ClassNode ARTEFACT_CLASS_NODE = new ClassNode(Artefact)
+ @CompileDynamic
+ private static Writable createMarkup(GPathResult node) {
+ new StreamingMarkupBuilder().mkp.yield(node)
+ }
- CompilationUnit compilationUnit
+ private static Object resolveProjectVersion(ClassNode classNode) {
+ def projectVersion = classNode.getNodeMetaData('projectVersion')
+ if (projectVersion == null) {
+ projectVersion = getClass().package.implementationVersion
Review Comment:
Moving this expression out of the instance method `visit()` and into a
`static` helper changes what `getClass()` resolves to. In a static Groovy
method the implicit receiver is the class object, so `getClass()` returns
`java.lang.Class` — not `GlobalGrailsClassInjectorTransformation`:
```groovy
@CompileStatic
class Probe {
static Object fromStatic() { getClass() } // -> class java.lang.Class
Object fromInstance() { getClass() } // -> class Probe
}
```
`Class.class.getPackage().getImplementationVersion()` is `null` (java.base
carries no manifest attributes), whereas the previous instance-context lookup
read `Implementation-Version` off the grails-core jar — which
`CompilePlugin.groovy` does set to `grailsVersion`. So this fallback now always
yields `null` instead of the Grails version.
That propagates: `pluginVersion` becomes `null`, and
`addPluginVersionProperty` calls `pluginVersion.toString()` with no null guard
(line 349), so a `*GrailsPlugin` class compiled without `projectVersion` node
metadata — plain `groovyc`, Groovy-Eclipse, anything that is not the Grails
Gradle plugin stamping the customizer — now NPEs during CANONICALIZATION rather
than getting the framework version.
Either keep the lookup anchored to the class explicitly:
```groovy
projectVersion =
GlobalGrailsClassInjectorTransformation.package.implementationVersion
```
or pass the resolved version in. Same latent issue exists in
`writePluginXml`/`writePluginXmlProperties`, but those were already static
before this PR, so it is pre-existing there rather than introduced.
##########
grails-core/src/test/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformationSpec.groovy:
##########
@@ -221,4 +228,160 @@ class FooGrailsPlugin {
cleanup:
projectDir.deleteDir()
}
+
+ void "priority returns the global grails transform order"() {
+ expect:
+ new GlobalGrailsClassInjectorTransformation().priority() ==
GroovyTransformOrder.GLOBAL_GRAILS_TRANSFORM_ORDER
+ }
+
+ /**
+ * Compiles the given source to a real file on disk (required so {@code
GrailsASTUtils.getSourceUrl}
+ * resolves a URL). Because {@code
GlobalGrailsClassInjectorTransformation} is itself registered as a
+ * global AST transformation (via {@code META-INF/services}) and
grails-core's own compiled classes are
+ * on this test's classpath, compiling all the way through {@code
CANONICALIZATION} exercises the real
+ * transformation exactly as production Grails builds do - no manual
{@code visit()} call is needed.
+ * {@code nodeMetaData} is stamped onto the class during {@code
CONVERSION}, before the transformation's
+ * own {@code CANONICALIZATION} pass runs, mirroring how the Grails Gradle
plugin stamps project
+ * name/version metadata via its own compiler customizer.
+ */
+ private static List compileToFile(File sourceFile, String source, File
targetDirectory, Map<String, String> nodeMetaData = [:]) {
+ sourceFile.parentFile.mkdirs()
+ sourceFile.text = source
+ def configuration = new CompilerConfiguration()
+ configuration.setTargetDirectory(targetDirectory)
+ if (nodeMetaData) {
+ configuration.addCompilationCustomizers(new
CompilationCustomizer(CompilePhase.CONVERSION) {
+ @Override
+ void call(SourceUnit source1, GeneratorContext context,
ClassNode cn) throws CompilationFailedException {
+ nodeMetaData.each { key, value -> cn.putNodeMetaData(key,
value) }
+ }
+ })
+ }
+ CompilationUnit cu = new CompilationUnit(configuration)
+ cu.addSource(sourceFile)
+ ClassNode capturedClassNode = null
+ SourceUnit capturedSourceUnit = null
+ cu.addPhaseOperation(new CompilationUnit.PrimaryClassNodeOperation() {
+ @Override
+ void call(SourceUnit source1, GeneratorContext context, ClassNode
cn) throws CompilationFailedException {
+ capturedClassNode = cn
+ capturedSourceUnit = source1
+ }
+ }, Phases.CANONICALIZATION)
+ cu.compile(Phases.CANONICALIZATION)
+ [capturedClassNode, capturedSourceUnit]
Review Comment:
`capturedSourceUnit` is returned but never used — all three callers
destructure it into a `sourceUnit` variable that is then dead. Dropping it lets
`compileToFile` return the `ClassNode` directly and removes the awkward
`source1` parameter naming in both inner classes.
Also worth adding the `pendingPluginClasses` / `pluginExcludes` reset to
these three specs' `cleanup:` blocks, as the excludes test below already does.
Compiling for real drives `generatePluginXml`, which mutates both static
collections, and with `maxParallelForks > 1` that state outlives the feature
method inside the fork.
##########
grails-core/src/test/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformationSpec.groovy:
##########
@@ -20,16 +20,23 @@ package org.grails.compiler.injection
import groovy.xml.MarkupBuilder
import groovy.xml.XmlSlurper
+import org.codehaus.groovy.ast.ClassHelper
import org.codehaus.groovy.ast.ClassNode
import org.codehaus.groovy.classgen.GeneratorContext
import org.codehaus.groovy.control.CompilationFailedException
import org.codehaus.groovy.control.CompilationUnit
+import org.codehaus.groovy.control.CompilePhase
import org.codehaus.groovy.control.CompilerConfiguration
import org.codehaus.groovy.control.Phases
import org.codehaus.groovy.control.SourceUnit
+import org.codehaus.groovy.control.customizers.CompilationCustomizer
import spock.lang.Specification
import spock.util.environment.RestoreSystemProperties
+import grails.plugins.metadata.GrailsPlugin
+import grails.util.GrailsNameUtils
+import org.apache.grails.common.compiler.GroovyTransformOrder
+
/**
* Created by graemerocher on 19/09/14.
Review Comment:
Might as well remove this since we're removing authors too.
##########
grails-core/src/main/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformation.groovy:
##########
@@ -285,60 +273,28 @@ class GlobalGrailsClassInjectorTransformation implements
ASTTransformation, Comp
}
}
- @CompileDynamic
- static void updatePluginXml(ClassNode pluginClassNode, String
pluginVersion, File pluginXmlFile, Collection<String> artefactClasses) {
+ static void updatePluginXml(ClassNode pluginClassNode, Object
pluginVersion, File pluginXmlFile, Collection<String> artefactClasses) {
if (!artefactClasses) 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.toString(), pluginXml)
def excludes = pluginProperties.get('pluginExcludes')
if (excludes instanceof List) {
pluginExcludes.clear()
- pluginExcludes.addAll(excludes)
- }
- }
-
- def resources = pluginXml.resources
-
- for (String cn in artefactClasses) {
- if (!resources.resource.find { it.text() == cn }) {
- resources.appendNode {
- resource(cn)
- }
+ pluginExcludes.addAll(excludes as List<String>)
}
}
-
+ writePluginXmlResources(pluginXml, artefactClasses)
handleExcludes(pluginXml)
- Writable writable = new StreamingMarkupBuilder().bind {
- mkp.yield(pluginXml)
- }
-
- pluginXmlFile.withWriter(StandardCharsets.UTF_8.name()) { Writer
writer ->
- writable.writeTo(writer)
+ pluginXmlFile.withWriter(StandardCharsets.UTF_8.name()) {
+ createMarkup(pluginXml).writeTo(it)
}
pendingPluginClasses.clear()
- } catch (e) {
+ } catch (ignored) {
Review Comment:
This catch-all is what turns the `createMarkup` defect above into a silent,
test-invisible fallback: it catches every `Exception`, not just the "XML is
corrupt" case the comment describes, and discards it without a trace.
Given the transform runs inside the compiler, a log line (or narrowing to
the parse/IO failures that actually mean "corrupt, recreate") would have
surfaced the `MissingPropertyException` immediately. Naming the parameter
`ignored` also asserts the exception is uninteresting, which is the opposite of
true here.
##########
grails-core/src/main/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformation.groovy:
##########
@@ -212,14 +196,18 @@ class GlobalGrailsClassInjectorTransformation implements
ASTTransformation, Comp
}
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 = []
-
- protected static void generatePluginXml(ClassNode pluginClassNode, String
pluginVersion, Set<String> transformedClasses, File pluginXmlFile) {
+ protected static void generatePluginXml(ClassNode pluginClassNode, Object
pluginVersion, Set<String> transformedClasses, File pluginXmlFile) {
Review Comment:
`pluginVersion` widened from `String` to `Object` here and on the two
`public static` methods below. Nothing in this repo calls them (only the spec
does), but they are public/protected static entry points on 8.0.x, so this is a
binary-incompatible signature change for any external plugin build compiled
against them, for no gain — the type is immediately narrowed again by
`.toString()` at five separate call sites.
The cleaner shape is to normalise once where the value originates, in
`visit()`, and leave these signatures as `String`:
```groovy
String pluginVersion = null
...
pluginVersion = projectVersion?.toString()
```
That also gives you the natural place to decide what a missing version
means, instead of deferring it to an unguarded `pluginVersion.toString()` in
`addPluginVersionProperty` (see the `resolveProjectVersion` comment).
##########
grails-core/src/test/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformationSpec.groovy:
##########
@@ -221,4 +228,160 @@ class FooGrailsPlugin {
cleanup:
projectDir.deleteDir()
}
+
+ void "priority returns the global grails transform order"() {
+ expect:
+ new GlobalGrailsClassInjectorTransformation().priority() ==
GroovyTransformOrder.GLOBAL_GRAILS_TRANSFORM_ORDER
+ }
+
+ /**
+ * Compiles the given source to a real file on disk (required so {@code
GrailsASTUtils.getSourceUrl}
+ * resolves a URL). Because {@code
GlobalGrailsClassInjectorTransformation} is itself registered as a
+ * global AST transformation (via {@code META-INF/services}) and
grails-core's own compiled classes are
+ * on this test's classpath, compiling all the way through {@code
CANONICALIZATION} exercises the real
+ * transformation exactly as production Grails builds do - no manual
{@code visit()} call is needed.
+ * {@code nodeMetaData} is stamped onto the class during {@code
CONVERSION}, before the transformation's
+ * own {@code CANONICALIZATION} pass runs, mirroring how the Grails Gradle
plugin stamps project
+ * name/version metadata via its own compiler customizer.
+ */
+ private static List compileToFile(File sourceFile, String source, File
targetDirectory, Map<String, String> nodeMetaData = [:]) {
+ sourceFile.parentFile.mkdirs()
+ sourceFile.text = source
+ def configuration = new CompilerConfiguration()
+ configuration.setTargetDirectory(targetDirectory)
+ if (nodeMetaData) {
+ configuration.addCompilationCustomizers(new
CompilationCustomizer(CompilePhase.CONVERSION) {
+ @Override
+ void call(SourceUnit source1, GeneratorContext context,
ClassNode cn) throws CompilationFailedException {
+ nodeMetaData.each { key, value -> cn.putNodeMetaData(key,
value) }
+ }
+ })
+ }
+ CompilationUnit cu = new CompilationUnit(configuration)
+ cu.addSource(sourceFile)
+ ClassNode capturedClassNode = null
+ SourceUnit capturedSourceUnit = null
+ cu.addPhaseOperation(new CompilationUnit.PrimaryClassNodeOperation() {
+ @Override
+ void call(SourceUnit source1, GeneratorContext context, ClassNode
cn) throws CompilationFailedException {
+ capturedClassNode = cn
+ capturedSourceUnit = source1
+ }
+ }, Phases.CANONICALIZATION)
+ cu.compile(Phases.CANONICALIZATION)
+ [capturedClassNode, capturedSourceUnit]
+ }
+
+ 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"
+ File projectDir = File.createTempDir()
+ File sourceFile = new File(projectDir, 'PlainGrailsPlugin.groovy')
+ File targetDirectory = new File(projectDir,
'build/classes/groovy/main')
+
+ when: "the source is compiled, exercising the registered global
transform"
+ def (ClassNode classNode, SourceUnit sourceUnit) =
compileToFile(sourceFile, '''
+class PlainGrailsPlugin {
+}
+''', targetDirectory, [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(targetDirectory, 'META-INF/grails-plugin.xml').exists()
+
+ cleanup:
+ projectDir.deleteDir()
+ }
+
+ 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"
+ File projectDir = File.createTempDir()
+ File sourceFile = new File(projectDir,
'grails-app/services/FooWidget.groovy')
+ File targetDirectory = new File(projectDir,
'build/classes/groovy/main')
+
+ when: "the source is compiled, exercising the registered global
transform"
+ def (ClassNode classNode, SourceUnit sourceUnit) =
compileToFile(sourceFile, '''
+class FooWidget {
+}
+''', targetDirectory, [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
+ annotations[0].getMember('name').text ==
GrailsNameUtils.getPropertyNameForLowerCaseHyphenSeparatedName('foowidget')
+ annotations[0].getMember('version').text == '2.0'
+
+ cleanup:
+ projectDir.deleteDir()
+ }
+
+ 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"
+ File projectDir = File.createTempDir()
+ File sourceFile = new File(projectDir,
'src/main/groovy/PlainClass.groovy')
+ File targetDirectory = new File(projectDir,
'build/classes/groovy/main')
+
+ when: "the source is compiled, exercising the registered global
transform"
+ def (ClassNode classNode, SourceUnit sourceUnit) =
compileToFile(sourceFile, '''
+class PlainClass {
+}
+''', targetDirectory, [projectName: 'plain', projectVersion: '1.0'])
+
+ then: "no @GrailsPlugin metadata is stamped on the class"
+ classNode.getAnnotations(ClassHelper.make(GrailsPlugin)).isEmpty()
+
+ cleanup:
+ projectDir.deleteDir()
+ }
+
+ void "Test that plugin dot xml excludes are honoured and metadata
refreshed when updating an existing file"() {
Review Comment:
This test does not exercise the path its name claims. Because `createMarkup`
throws (see the main-file comment), `updatePluginXml` aborts before writing and
`catch (ignored)` re-runs `writePluginXml`, which regenerates the descriptor
from the AST — and since `writePluginXml` applies `pluginExcludes` to
`artefactClasses` itself and reads `grailsVersion` from the same plugin
properties, it produces exactly the asserted `version == '2.0'`, `grailsVersion
== '3.0 > *'`, `resource*.text() == ['KeptThing']`. The assertions pass whether
the update path works or is entirely broken.
The pre-existing "plugin dot xml file is updated when the plugin dot xml
does exist" test has the same blind spot. To pin the update semantics, assert
on something only the merge path can produce — e.g. seed the existing
`plugin.xml` with a resource that is *not* in `artefactClasses` and require it
to survive, or drive `updatePluginXml(null, ...)` where the rewrite fallback is
a no-op.
##########
grails-core/src/main/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformation.groovy:
##########
@@ -347,17 +303,104 @@ class GlobalGrailsClassInjectorTransformation implements
ASTTransformation, Comp
@CompileDynamic
protected static void handleExcludes(GPathResult pluginXml) {
if (pluginExcludes) {
-
def antPathMatcher = new AntPathMatcher()
- pluginXml.resources.resource.each { res ->
- if (pluginExcludes.any() { String exc ->
antPathMatcher.match(exc, res.text().replace('.', '/')) }) {
- res.replaceNode {}
+ pluginXml.resources.resource.each {
+ def resourceNode = it as GPathResult
+ if (pluginExcludes.any() { antPathMatcher.match(it,
resourceNode.text().replace('.', '/')) }) {
Review Comment:
The inner closure's implicit `it` shadows the outer `each`'s `it`, so this
line reads as if the resource were being matched against itself. It is in fact
correct — I confirmed the inner `it` binds to the exclude pattern and
`resourceNode` still captures the resource — but the previous `{ String exc ->
antPathMatcher.match(exc, res.text()...) }` said so plainly, and this is a
readability step backwards in a refactoring PR. Keeping the explicit parameter
costs nothing:
```groovy
pluginXml.resources.resource.each { res ->
def resourceNode = res as GPathResult
if (pluginExcludes.any { String exc -> antPathMatcher.match(exc,
resourceNode.text().replace('.', '/')) }) {
resourceNode.replaceNode {}
}
}
```
--
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]
