jdaugherty commented on code in PR #16148:
URL: https://github.com/apache/grails-core/pull/16148#discussion_r3784760092
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/multitenancy/transform/TenantTransform.groovy:
##########
@@ -136,7 +138,11 @@ class TenantTransform extends
AbstractDatastoreMethodDecoratingTransformation {
return makeDelegatingClosureCall(tenantServiceVar, 'withId',
args(tenantIdVar), params(param(serializableClassNode, VAR_TENANT_ID)),
originalMethodCallExpr, variableScope)
}
else {
- addError('@Tenant value should be a closure', annotationNode)
+ sourceUnit.getErrorCollector().addErrorAndContinue(
Review Comment:
This is exactly `AstUtils.error(SourceUnit, ASTNode, String)`, which already
exists
(`grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/reflect/AstUtils.groovy:818`)
and builds the same `SyntaxErrorMessage`/`SyntaxException` pair from the
node's positions. Please call it instead of hand-rolling it here:
```groovy
AstUtils.error(sourceUnit, annotationNode, '@Tenant value should be a
closure')
```
That also drops the two new imports.
##########
grails-datamapping-core/src/main/groovy/org/grails/compiler/gorm/GormEntityTransformation.groovy:
##########
@@ -114,7 +114,6 @@ class GormEntityTransformation extends
AbstractASTTransformation implements Comp
private static MethodNode REMOVE_FROM_METHOD_NODE =
GORM_ENTITY_CLASS_NODE.getMethods('removeFrom').get(0)
private static MethodNode GET_ASSOCIATION_ID_METHOD_NODE =
GORM_ENTITY_CLASS_NODE.getMethods('getAssociationId').get(0)
public static final Parameter[] ADD_TO_PARAMETERS = [new
Parameter(AstUtils.OBJECT_CLASS_NODE, 'obj')] as Parameter[]
- public static final ClassNode SERIALIZABLE_CLASS_NODE =
ClassHelper.make(Serializable).getPlainNodeReference()
Review Comment:
`SERIALIZABLE_CLASS_NODE` is `public static final` on a shipped class, so
removing it is a source- and binary-compatibility break for anything outside
this repo (there are no in-repo callers). Same category of change in
`DirtyCheckingTransformer`: `weaveIntoExistingSetter`,
`createMarkDirtyMethodCall`, `getGetterAndSetterForPropertyName` and
`isAnnotatedWithJavaValidationApi` go from `protected` to `protected static`
(subclasses that override them stop compiling), and `GetterAndSetter` goes from
an inner class to a static nested class (its constructor signature changes).
If that is intentional for 8.1.x, fine — but it is a behaviour-neutral
cleanup commit carrying an API break, so please say so in the description
rather than leaving it in a "fix IntelliJ warnings" commit.
##########
grails-datamapping-core/src/test/groovy/org/grails/compiler/gorm/LocalTransformationSupportSpec.groovy:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.compiler.gorm
+
+import org.codehaus.groovy.ast.ASTNode
+import org.codehaus.groovy.ast.AnnotationNode
+import org.codehaus.groovy.ast.ClassHelper
+import org.codehaus.groovy.ast.ClassNode
+import org.codehaus.groovy.ast.FieldNode
+
+import spock.lang.Specification
+
+/**
+ * {@code LocalTransformationSupport.resolveAnnotatedClassOrNull} is the
shared guard extracted
+ * from {@link DirtyCheckTransformation} and {@link
JpaGormEntityTransformation}'s
+ * {@code visit(ASTNode[], SourceUnit)} entry points.
+ */
+class LocalTransformationSupportSpec extends Specification {
+
+ private static final ClassNode ANNOTATION_TYPE =
ClassHelper.make(Deprecated)
+ private static final ClassNode OTHER_ANNOTATION_TYPE =
ClassHelper.make(SuppressWarnings)
+
+ void "resolves the annotated class when the annotation type matches and
the node is a class"() {
+ given:
+ ClassNode targetClass = new ClassNode('com.example.Target', 0,
ClassHelper.OBJECT_TYPE)
+ AnnotationNode annotationNode = new AnnotationNode(ANNOTATION_TYPE)
+
+ expect:
+
LocalTransformationSupport.resolveAnnotatedClassOrNull([annotationNode,
targetClass] as ASTNode[], ANNOTATION_TYPE) == targetClass
+ }
+
+ void "returns null when the annotation type does not match"() {
+ given:
+ ClassNode targetClass = new ClassNode('com.example.Target', 0,
ClassHelper.OBJECT_TYPE)
+ AnnotationNode annotationNode = new
AnnotationNode(OTHER_ANNOTATION_TYPE)
+
+ expect:
+
LocalTransformationSupport.resolveAnnotatedClassOrNull([annotationNode,
targetClass] as ASTNode[], ANNOTATION_TYPE) == null
+ }
+
+ void "returns null when the annotated node is not a class"() {
Review Comment:
Two things here.
This branch cannot occur in production: `@DirtyCheck`, `@Entity` and
`@JpaEntity` are all declared `@Target([ElementType.TYPE])`, so Groovy never
dispatches these local transforms on a `FieldNode`. That sits oddly beside
commit `53e1767`, which removed the malformed-`astNodes` guard from this same
class precisely because it was structurally unreachable — the same argument
applies to the `!(parent instanceof ClassNode)` half of the check this spec
exists to cover.
More generally, the whole spec drives an extracted internal helper directly,
which is what CLAUDE.md rule 9 asks us to avoid; the guard is already exercised
end-to-end whenever a source annotated with `@DirtyCheck` / `@Entity` /
`@JpaEntity` is compiled. Also worth fixing the class javadoc: it names
`DirtyCheckTransformation` and `JpaGormEntityTransformation`, but
`GormEntityTransformation` uses the helper too.
##########
grails-datamapping-core/src/test/groovy/grails/gorm/annotation/multitenancy/TenantTransformSpec.groovy:
##########
@@ -0,0 +1,168 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package grails.gorm.annotation.multitenancy
+
+import org.codehaus.groovy.ast.ClassNode
+import org.codehaus.groovy.ast.MethodNode
+import org.codehaus.groovy.ast.ModuleNode
+import org.codehaus.groovy.control.CompilationUnit
+import org.codehaus.groovy.control.MultipleCompilationErrorsException
+import org.codehaus.groovy.control.Phases
+
+import spock.lang.Specification
+
+import org.grails.datastore.gorm.multitenancy.transform.TenantTransform
+
+/**
+ * {@link CurrentTenantTransformSpec} only exercises {@code @CurrentTenant} and
+ * {@code @WithoutTenant}, which both take {@code
TenantTransform#buildDelegatingMethodCall}'s first
+ * two branches. The third branch - the actual {@code @Tenant} annotation,
which resolves the tenant
+ * id from a closure supplied as the annotation value - and its own error path
(a non-closure value)
+ * were unexercised. These specs cover that branch directly, plus the small
standalone
+ * {@code getAnnotationType()}/{@code hasTenantAnnotation} surface not reached
by any existing spec.
+ */
+class TenantTransformSpec extends Specification {
+
+ void "test @Tenant with a closure value transforms a method to resolve the
tenant id from the closure"() {
+ given: 'a service with @Tenant applied at the method level with a
closure tenant resolver'
+ def bookService = new GroovyShell().evaluate('''
+import grails.gorm.multitenancy.Tenant
+
+class BookService {
+ @Tenant({ "someTenant" })
+ List listBooks() {
+ return ["The Stand"]
+ }
+}
+new BookService()
+
+''')
+ when: 'the list books method is invoked'
+ bookService.listBooks()
+
+ then: 'an exception was thrown because GORM is not setup, proving the
delegating call was generated and reached'
+ thrown(IllegalStateException)
Review Comment:
This assertion does not prove what the `then:` label claims.
`buildDelegatingMethodCall` emits the
`ServiceRegistry.targetDatastore(...).getService(TenantService)` declaration as
the *first* statement of the rewritten body on every branch, so the
`IllegalStateException` is raised before the closure is ever cloned or called.
The spec would pass identically if the `@Tenant` closure branch were never
taken — including on the non-closure error path this PR just changed.
Assert something that only the closure branch produces: inspect the
rewritten `MethodNode` (you already have `compileAndFindMethod` in this spec)
for the `$tenantResolver` / `$tenantId` declarations, or check the generated
method's parameter list. Same for the class-level test at line 81.
##########
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/WhereQueryAssociationPathSpec.groovy:
##########
@@ -0,0 +1,181 @@
+/*
+ * 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.datastore.gorm.query.transform
+
+import org.codehaus.groovy.control.MultipleCompilationErrorsException
+
+import spock.lang.Specification
+
+/**
+ * A property-path expression such as {@code author.name} or a deeper {@code
author.publisher.name} is
+ * rewritten by {@code
DetachedCriteriaTransformer#handleAssociationQueryViaPropertyExpression} into
+ * nested {@code delegate.<association> { ... }} calls, one per path segment.
Those delegate calls are
+ * dynamic (routed through {@code AbstractDetachedCriteria#methodMissing}) and
need the target class to be
+ * GORM-enhanced against a live datastore to resolve - something this module
deliberately has none of - so
+ * these associations are verified structurally: the source compiles (or fails
to, for the invalid-property
+ * cases) and a nested closure is synthesized per association segment walked.
+ */
+class WhereQueryAssociationPathSpec extends Specification {
+
+ // The domain class names must be unique across the test JVM because
+ // AstPropertyResolveUtils caches resolved properties statically by class
name
+ private static final String SINGLE_LEVEL_SOURCE = '''
+import grails.gorm.DetachedCriteria
+import grails.gorm.annotation.Entity
+
+class AssocPathSingleQueryService {
+ protected DetachedCriteria<AssocPathSingleBook> findByAuthorName(String
name) {
+ AssocPathSingleBook.where {
+ author.name == name
+ }
+ }
+}
+
+@Entity
+class AssocPathSingleBook {
+ String title
+ AssocPathSingleAuthor author
+}
+
+@Entity
+class AssocPathSingleAuthor {
+ String name
+}
+'''
+
+ private static final String MULTI_LEVEL_SOURCE = '''
+import grails.gorm.DetachedCriteria
+import grails.gorm.annotation.Entity
+
+class AssocPathMultiQueryService {
+ protected DetachedCriteria<AssocPathMultiBook> findByPublisherName(String
name) {
+ AssocPathMultiBook.where {
+ author.publisher.name == name
+ }
+ }
+}
+
+@Entity
+class AssocPathMultiBook {
+ String title
+ AssocPathMultiAuthor author
+}
+
+@Entity
+class AssocPathMultiAuthor {
+ String name
+ AssocPathMultiPublisher publisher
+}
+
+@Entity
+class AssocPathMultiPublisher {
+ String name
+}
+'''
+
+ private static List<Class<?>> findQueryClosures(GroovyClassLoader gcl,
String methodName) {
+ gcl.loadedClasses.findAll { it.name.contains("_${methodName}_") }
+ }
+
+ void "a single-level association property path compiles and generates one
nested association closure"() {
+ given:
+ GroovyClassLoader gcl = new GroovyClassLoader()
+
+ when:
+ gcl.parseClass(SINGLE_LEVEL_SOURCE)
+
+ then:
+ noExceptionThrown()
+
+ and: 'one closure for the outer where-block and one nested closure for
the association segment walked'
+ List<Class<?>> queryClosures = findQueryClosures(gcl,
'findByAuthorName').sort { it.name.count('$_closure') }
Review Comment:
These assertions are pinned to Groovy's generated closure class-naming
scheme — the `_<methodName>_` substring and the number of `$_closure`
occurrences in the class name. That is compiler-internal naming, not output of
`DetachedCriteriaTransformer`: a change to how Groovy names nested closures
silently breaks these tests, and a change to the transform that stops
synthesizing association closures could still leave the counts intact.
The other specs in this PR (e.g. `WhereQueryEmbeddedPropertyPathSpec`)
inspect the transformed AST at canonicalization instead — please use that
approach here so the assertion is about the nested `delegate.<association> {
... }` calls actually generated.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/OrderedGormTransformation.groovy:
##########
@@ -60,6 +60,8 @@ class OrderedGormTransformation extends
AbstractASTTransformation implements Com
throw new RuntimeException("Internal error: wrong types:
${astNodes[0].getClass()} / ${astNodes[1].getClass()}")
}
+ init(astNodes, source)
Review Comment:
`init(...)` re-checks the exact condition the hand-rolled guard three lines
above already rejected — `AbstractASTTransformation.init` throws
`GroovyBugError` when `nodes[0]` is not an `AnnotationNode` or `nodes[1]` is
not an `AnnotatedNode`. Now that `init` is being called, the manual
`instanceof` check and its `RuntimeException` are dead weight; move
`init(astNodes, source)` to the top of the method and delete the guard.
--
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]
