jdaugherty commented on code in PR #15557:
URL: https://github.com/apache/grails-core/pull/15557#discussion_r3389327940
##########
grails-data-graphql/core/src/test/groovy/org/grails/gorm/graphql/response/errors/DefaultGraphQLErrorsResponseHandlerSpec.groovy:
##########
@@ -97,7 +97,10 @@ class DefaultGraphQLErrorsResponseHandlerSpec extends
Specification implements G
DataFetcher errorsFetcher =
codeRegistry.getDataFetcher(coordinates("MockValidateable", "errors"), field)
then:
- errorsFetcher.get(mockObjectEnv) instanceof List<FieldError>
+ // Groovy 5 disallows `instanceof` against a parameterized type (type
erasure makes
Review Comment:
The comment is redundant, let's remove it.
##########
dependencies.gradle:
##########
@@ -240,9 +243,12 @@ ext {
'liquibase-hibernate.version': '4.27.0',
'liquibase.version' : '4.27.0',
'hibernate.version' : '5.6.15.Final',
- 'groovy.version' : '5.0.5',
'spock.version' : '2.4-groovy-5.0',
'protobuf.version': '4.30.2',
+ // The Micronaut platform ships org.ow2.asm 9.10.1, above the
9.9.1 inherited via grails-base-bom.
Review Comment:
Remove the redundant comment
##########
grails-core/src/main/groovy/org/grails/compiler/injection/GrailsASTUtils.java:
##########
@@ -1510,8 +1510,20 @@ public static void processVariableScopes(SourceUnit
source, ClassNode classNode,
VariableScopeVisitor scopeVisitor = new VariableScopeVisitor(source);
if (methodNode == null) {
scopeVisitor.visitClass(classNode);
+ return;
+ }
+ scopeVisitor.prepareVisit(classNode);
+ if (methodNode.getExceptions() == null) {
+ // Groovy 5's VariableScopeVisitor reads the method's exceptions
array without a null check, and AST
Review Comment:
Is this comment up-todate? Wasn't this found to be a different issue?
##########
grails-data-hibernate5/core/src/test/groovy/grails/gorm/tests/HibernateEntityTraitGeneratedSpec.groovy:
##########
@@ -32,12 +32,11 @@ class HibernateEntityTraitGeneratedSpec extends
Specification {
@Shared @AutoCleanup HibernateDatastore datastore = new
HibernateDatastore(Club)
void "test that all HibernateEntity trait methods are marked as
Generated"() {
- // Unfortunately static methods have to check directly one by one
expect:
- Club.getMethod('findAllWithSql',
CharSequence).isAnnotationPresent(Generated)
- Club.getMethod('findWithSql',
CharSequence).isAnnotationPresent(Generated)
- Club.getMethod('findAllWithSql', CharSequence,
Map).isAnnotationPresent(Generated)
- Club.getMethod('findWithSql', CharSequence,
Map).isAnnotationPresent(Generated)
+ Club.getDeclaredMethod('findAllWithSql',
CharSequence).isAnnotationPresent(Generated)
+ Club.getDeclaredMethod('findAllWithSql', CharSequence,
Map).isAnnotationPresent(Generated)
+ Club.getDeclaredMethod('findWithSql',
CharSequence).isAnnotationPresent(Generated)
Review Comment:
Believe this is a bug. before it was findAllWithSql, which was changed to
findWithSql (duplicates the next line)
##########
grails-datastore-core/src/test/groovy/org/grails/datastore/mapping/reflect/ClassPropertyFetcherTests.groovy:
##########
@@ -114,8 +114,8 @@ class ClassPropertyFetcherTests {
}
}
-trait TestTrait<F extends Serializable> {
- F from
+trait TestTrait<T> {
Review Comment:
How is this not a groovy bug? from is a public value that should be copied
to the implementations of the trait. It clearly works without serializable, so
why doesn't it work with?
##########
dependencies.gradle:
##########
@@ -298,9 +306,12 @@ ext {
'liquibase-hibernate.version': '4.27.0',
'liquibase.version' : '4.27.0',
'hibernate.version' : '5.6.15.Final',
- 'groovy.version' : '5.0.5',
'spock.version' : '2.4-groovy-5.0',
'protobuf.version': '4.30.2',
+ // The Micronaut platform ships org.ow2.asm 9.10.1, above the
9.9.1 inherited via grails-base-bom.
Review Comment:
Remove the redundant comment
##########
grails-logging/src/main/groovy/org/grails/compiler/logging/LoggingTransformer.java:
##########
@@ -78,11 +80,24 @@ public void performInjectionOnAnnotatedClass(SourceUnit
source, ClassNode classN
return;
}
- AnnotationNode annotationNode = new
AnnotationNode(ClassHelper.make(Slf4j.class));
- LogASTTransformation logASTTransformation = new LogASTTransformation();
- logASTTransformation.setCompilationUnit(new CompilationUnit(new
GroovyClassLoader(getClass().getClassLoader())));
- logASTTransformation.visit(new ASTNode[]{ annotationNode, classNode},
source);
- classNode.putNodeMetaData(Slf4j.class, annotationNode);
+ // Groovy 5: an @Slf4j added during an AST transform is not processed,
so inject the log field manually.
Review Comment:
We should retest this assumption. We want to use the Slf4j annotation if
possible.
##########
grails-gsp/core/src/test/groovy/org/grails/gsp/GspCompileStaticSpec.groovy:
##########
@@ -146,6 +153,9 @@ out.print(messageClosure('World'))
t.metaInfo.compilationException.message.contains('Cannot find matching
method java.util.Date#getTimeTypo()')
}
+ // Note: In Groovy 5, the type checking extension behavior changed and
undeclared variables
+ // in GSP templates may not trigger compilation errors. This is a known
limitation.
Review Comment:
Yes, the whole point of CompileStatic is to fail on unresolveable
properties. This still seems like a regression. The taglib support is separate
from this.
##########
grails-test-suite-persistence/src/test/groovy/grails/web/databinding/GrailsWebDataBinderSpec.groovy:
##########
@@ -1806,12 +1804,21 @@ class Widget {
isNotBindable(bindable: false)
timeZone(nullable: true)
}
+
+ // Manual Comparable implementation (replaces @Sortable which conflicts
with @Entity in Groovy 5)
Review Comment:
If they aren't generating the same method, then this is a groovy bug. we
need to understand this failure.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/reflect/AstUtils.groovy:
##########
@@ -248,8 +248,20 @@ class AstUtils {
VariableScopeVisitor scopeVisitor = new VariableScopeVisitor(source)
if (methodNode == null) {
scopeVisitor.visitClass(classNode)
+ return
+ }
+ scopeVisitor.prepareVisit(classNode)
+ if (methodNode.exceptions == null) {
+ // Groovy 5's VariableScopeVisitor reads the method's exceptions
array without a null check, and AST
+ // transforms routinely create methods via
ClassNode.addMethod(..., null, ...). MethodNode.exceptions is
+ // final, so recompute scopes on a proxy that shares the same
parameters and code but carries an empty
+ // exceptions array, then copy the computed scope back onto the
real method.
+ MethodNode proxy = new MethodNode(methodNode.name,
methodNode.modifiers, methodNode.returnType,
Review Comment:
You're saying the Groovy API is validating it's not null, and you can set
null in the create? I was suggesting we update all of our clals to not pass
null, which then would fix these work arounds.
##########
grails-datamapping-validation/src/main/groovy/grails/gorm/validation/ConstrainedProperty.groovy:
##########
@@ -65,26 +66,32 @@ interface ConstrainedProperty extends Constrained {
String DEFAULT_INVALID_MIN_SIZE_MESSAGE =
MESSAGE_BUNDLE.getString(DEFAULT_INVALID_MIN_SIZE_MESSAGE_CODE)
String DEFAULT_NULL_MESSAGE =
MESSAGE_BUNDLE.getString(DEFAULT_NULL_MESSAGE_CODE)
String DEFAULT_INVALID_VALIDATOR_MESSAGE =
MESSAGE_BUNDLE.getString(DEFAULT_INVALID_VALIDATOR_MESSAGE_CODE)
-
- Map<String, String> DEFAULT_MESSAGES = new HashMap<String, String>() {
- {
- put(DEFAULT_BLANK_MESSAGE_CODE, DEFAULT_BLANK_MESSAGE)
- put(DEFAULT_DOESNT_MATCH_MESSAGE_CODE,
DEFAULT_DOESNT_MATCH_MESSAGE)
- put(DEFAULT_INVALID_CREDIT_CARD_MESSAGE_CODE,
DEFAULT_INVALID_CREDIT_CARD_MESSAGE)
- put(DEFAULT_INVALID_EMAIL_MESSAGE_CODE,
DEFAULT_INVALID_EMAIL_MESSAGE)
- put(DEFAULT_INVALID_MAX_MESSAGE_CODE, DEFAULT_INVALID_MAX_MESSAGE)
- put(DEFAULT_INVALID_MAX_SIZE_MESSAGE_CODE,
DEFAULT_INVALID_MAX_SIZE_MESSAGE)
- put(DEFAULT_INVALID_MIN_MESSAGE_CODE, DEFAULT_INVALID_MIN_MESSAGE)
- put(DEFAULT_INVALID_MIN_SIZE_MESSAGE_CODE,
DEFAULT_INVALID_MIN_SIZE_MESSAGE)
- put(DEFAULT_INVALID_RANGE_MESSAGE_CODE,
DEFAULT_INVALID_RANGE_MESSAGE)
- put(DEFAULT_INVALID_SIZE_MESSAGE_CODE,
DEFAULT_INVALID_SIZE_MESSAGE)
- put(DEFAULT_INVALID_URL_MESSAGE_CODE, DEFAULT_INVALID_URL_MESSAGE)
- put(DEFAULT_NOT_EQUAL_MESSAGE_CODE, DEFAULT_NOT_EQUAL_MESSAGE)
- put(DEFAULT_NOT_INLIST_MESSAGE_CODE, DEFAULT_NOT_IN_LIST_MESSAGE)
- put(DEFAULT_NULL_MESSAGE_CODE, DEFAULT_NULL_MESSAGE)
- put(DEFAULT_INVALID_VALIDATOR_MESSAGE_CODE,
DEFAULT_INVALID_VALIDATOR_MESSAGE)
- }
- }
+ String DEFAULT_NOT_UNIQUE_MESSAGE =
MESSAGE_BUNDLE.getString(DEFAULT_NOT_UNIQUE_MESSAGE_CODE)
+
+ // Built with a Groovy map literal rather than an
anonymous-HashMap-with-instance-initializer.
+ // On Groovy 5 (GROOVY-12063) the bareword references to the sibling
DEFAULT_* constants inside
+ // such an initializer compile to dynamic getProperty calls on `this` (the
HashMap), which the
+ // MOP resolves as map-key lookups on the still-empty map, so every entry
was captured as null.
+ // A map literal resolves the constants against the enclosing interface
scope and is correct on
+ // every Groovy version.
+ Map<String, String> DEFAULT_MESSAGES = [
Review Comment:
By default maps in Groovy are LinkHashMaps, no? This was a HashMap before
which can have performance issues. Is this map meant to be immutable?
--
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]
