Repository navigation
refactor: convert grails-core to Groovy - #16586
borinquenkid wants to merge 7 commits into
Conversation
Public constants and fields declared without 'public' became Groovy properties (DefaultResourceLocator, PluginPathAwareFileSystemResourceLoader, AbstractResourceLocatorPostProcessor, DefaultArtefactInfo.handlerData); the MockFileResource test-fixture constructors lost their 'throws UnsupportedEncodingException'. Restore both. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…groovy-grails-core
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
sbglasius
left a comment
sbglasius
left a comment
There was a problem hiding this comment.
Review of the Java→Groovy conversion. The translation itself looks faithful: identity checks moved to .is(), Map-backed config classes use @POJO, and field writes use this.@. The comments below cover a hot-path performance regression, a couple of bugs that were carried over, test-isolation and convention issues, and two public-API shape changes.
|
|
||
| @Override | ||
| Object invoke(Object controller) throws Throwable { | ||
| return handle.invokeWithArguments(controller) |
There was a problem hiding this comment.
Efficiency: handle.invoke(controller) became handle.invokeWithArguments(controller). That is the generic varargs path: it allocates an array, boxes, and adapts the type on every call. In any non-development environment this runs on every controller action dispatch, so it undoes much of the gain MethodHandles had over the ReflectionInvoker. A cheaper option is to adapt the handle once (e.g. mh.asType(MethodType.methodType(Object, Object))) and use a typed invoker or LambdaMetafactory, or keep this invoker in Java.
| static void reset() { | ||
| setPluginManager(null) | ||
| setGrailsApplication(null) | ||
| setServletContext(null) |
There was a problem hiding this comment.
Correctness: reset() calls setServletContext(null) without a guard, so it throws a NullPointerException whenever servletContexts was never created, i.e. outside a servlet environment. It throws after the plugin manager and application are cleared but before setPluginManagerInCreation(false) and setConfig(null) run, so config is left behind. clear() already guards with servletContexts != null; reset() should do the same. The new HoldersSpec asserts the NPE instead of fixing it.
| def <T> T getProperty(String key, Class<T> targetType, T defaultValue, List<T> allowedValues) { | ||
| T v = getProperty(key, targetType, defaultValue) | ||
| if (!allowedValues.contains(v)) { | ||
| throw new GrailsConfigurationException('Invalid configuration value [\$value] for key [\${key}]. Possible values \$allowedValues') |
There was a problem hiding this comment.
Correctness: These messages are single-quoted with escaped \$value / \${key} / \$allowedValues, so they print the placeholders literally. There is also no value variable here; the local is v. The same issue is at lines 240 and 249 and at NavigableMapConfig.groovy:207. Now that this is Groovy, a GString such as "Invalid configuration value [${v}] for key [${key}]. Possible values ${allowedValues}" would fix it.
| // clear(), unlike reset(), guards its servlet-context write and is therefore safe to call | ||
| // outside a servlet environment (grails-core has no grails.web.context.WebRequestServletHolder | ||
| // on its classpath, so Holders.servletContexts is never populated here) | ||
| Holders.clear() |
There was a problem hiding this comment.
Test isolation: Holders.clear() also empties the static applicationDiscoveryStrategies list, which is loaded once from GrailsFactoriesLoader when the class initialises, and nothing restores it. Forks are reused across specs, so any spec that runs after this one in the same JVM loses every factory-registered strategy and can fail depending on test order. Consider saving and restoring the strategies, or resetting only the holders this spec sets.
| } | ||
|
|
||
| private static List<TypeFilter> includeFiltersOf(ClassPathBeanDefinitionScanner scanner) { | ||
| Field field = ClassPathScanningCandidateComponentProvider.getDeclaredField('includeFilters') |
There was a problem hiding this comment.
Conventions: This reads Spring's private includeFilters field by reflection, and GrailsContextNamespaceHandlerSpec:40 does the same with NamespaceHandlerSupport.parsers. The spec also calls the protected createScanner directly. CLAUDE.md rule 9 says: "Test via public APIs - ... never invoke internal implementations, package-private methods, or bypass the public surface directly". These tests will also break if Spring renames its private fields.
| */ | ||
| @POJO | ||
| @CompileStatic | ||
| static class GrailsPluginClass extends AbstractGrailsClass { |
There was a problem hiding this comment.
API compatibility: GrailsPluginClass was a public non-static inner class in Java and is now static, which changes its constructor signature and binary shape. Precompiled code that creates it through an outer instance would break. Was this intended? If not, it should keep its previous shape, in line with 80c8329, which aimed to keep the Java-facing API.
| @CompileStatic | ||
| class Pair<A, B> { | ||
|
|
||
| final A aValue |
There was a problem hiding this comment.
API compatibility: The Java package-private fields aValue / bValue (same in Triple) are now Groovy properties. The fields become private, and Groovy generates public getAValue() / getBValue() next to the hand-written getaValue() / getbValue(). @PackageScope final A aValue, as used in InstanceFactoryBean, would keep the original shape.
| def loader = new SimpleMapResourceLoader() | ||
|
|
||
| expect: | ||
| loader.getClassLoader() == SimpleMapResourceLoader.classLoader |
There was a problem hiding this comment.
Test coverage: This compares against the same expression the implementation returns (SimpleMapResourceLoader.classLoader), so the assertion can't fail. Comparing against an independent value such as loader.getClass().getClassLoader() would make it meaningful.
- DefaultGrailsControllerClass: adapt each action MethodHandle once to the (Object[])Object shape Groovy emits for invokeExact, so production dispatch is an exact direct invocation again instead of invokeWithArguments - Holders.reset(): guard the servlet-context write like clear() does, so it no longer throws outside a servlet environment and leaves config behind - CompositeConfig / NavigableMapConfig / ArtefactTypeAstTransformation: turn the messages that were literal "$value" placeholders in Java into GStrings - AbstractGrailsPlugin.GrailsPluginClass: back to a package-private, non-static inner class; DefaultGrailsPlugin passes the enclosing instance explicitly - Pair / Triple: keep the package-private fields (@PackageScope) instead of Groovy properties that would add getAValue()/getBValue() next to getaValue() - Specs: HoldersSpec restores the factory-loaded discovery strategies after clear() and asserts reset() works; the component-scan parser and namespace handler specs drive Spring through XML parsing and public APIs instead of reading private fields; SimpleMapResourceLoaderSpec compares against an independent value; new cases cover the message texts, inherited and non-String actions, and the package-private Pair/Triple shape
Faithfulness pass over the 191 converted files of grails-core: - identity comparisons that had become equals(): GrailsConversionServiceConverters (ApplicationContext), DefaultGrailsApplication (applicationClass) - field reads on another instance that had become getter calls: AbstractGrailsPlugin.equals, GrailsTransactionAttribute copy constructor - DefaultResourceLocator: plugin file-system name compared with equals() as in Java - package-private members that had become public: GrailsConversionServiceConverters, SynchronizationManager, SpringTransactionSynchronizationManager, the ChainedTransactionManager test constructor, GrailsClosureRuntimeHints.enclosingLoads, DefaultPluginDiscovery.filterPlugins/populatePlugins, the PluginInfo constructor, PluginMetadata.canRegisterPlugin/supportsEnvironment, PluginXmlHandler, AbstractGrailsPluginManager.getConfigSlurper, ClassRelativeClassLoader, DefaultResourceLocator.PluginResourceInfo, PluginAstReader, EnvironmentAwarePropertySource, GrailsPluginSorter.sortPlugins - GrailsEnvironmentPostProcessorSpec builds its PluginInfos through a same-package test factory now that the constructor is package-private again
| return Collections.emptyList() | ||
| } | ||
| if (paramValues.getClass().isArray()) { | ||
| return Arrays.asList((Object[]) paramValues) |
There was a problem hiding this comment.
(Object[]) paramValues behaves differently here than it did in Java. The static type of paramValues is Object, so Groovy routes the cast through its coercion rules rather than a plain checkcast: a primitive array such as int[] is boxed element by element into an Object[] and returned as a list, where the Java version threw ClassCastException. Verified with Groovy 5.1.3 and 6.0.0: toList(new int[] {1, 2}) returns [1, 2].
The more lenient result seems fine for a parameter-value helper, so no change requested; this is just to make the divergence a deliberate one. If the Java behaviour should be kept exactly, Object[].class.cast(paramValues) restores the ClassCastException.
| """ | ||
| XmlBeanDefinitionReader reader = new XmlBeanDefinitionReader(registry) | ||
| // the Grails schema is not on the classpath, so parse namespace-aware without XSD validation | ||
| reader.validating = false |
There was a problem hiding this comment.
Not introduced by this PR, but recorded here because the spec has to work around it: META-INF/spring.schemas maps http://grails.org/schema/context/grails-context.xsd to org/grails/spring/context/annotation/grails-context.xsd, and that file lives under src/main/groovy, which the build does not treat as a resource directory. It is missing from the published grails-core jar (checked grails-core-8.1.0-SNAPSHOT.jar: only spring.schemas is there), so an XSD-validating parse of <grails:component-scan> cannot resolve its schema. That is why this spec parses namespace-aware with validation off.
Packaging the XSD (for example processResources { from('src/main/groovy') { include '**/*.xsd' } }) is a separate fix and worth its own issue.
|
@jdaugherty feel free to approve , although I wont merge until the stack is ready |
…groovy-grails-core
What
Converts the remaining Java sources of
grails-coreto Groovy: 191.javafiles removed, 241.groovyfiles added (268 files changed, 16677 insertions(+), 12145 deletions(-)).Why
The maintainers asked on #16351 for the Java-to-Groovy migration to land one Gradle project at a time as a stack of PRs, bottom-up in dependency order. #16572 did
grails-data-hibernate7; this PR is 8 of 30 in the next stack (base:refactor/java-to-groovy-grails-shell-cli). The end state of every file is taken from the9.0.x-java-to-groovybranch, which already carries the regression fixes found by CI.Commits:
Changed outside the converted sources
M grails-core/build.gradleJava-facing API sweep
Every deleted
.javawas compared with its Groovy twin viajavap -v -protected(class declaration, public/protected fields, constructors, methods incl. modifiers/throws/generics, interface default methods, nested class modifiers, and the annotations callers see). Result:grails-core: 191 deleted .java, 9 difference(s) found and fixed in this PR (see thefix:commit), 0 leftSee also the notes below. Remaining informational entries (widened visibility, e.g. Groovy property accessors): 32.
Verification
classesandtestClasses(plusast/cli/testFixturessource sets where present) of the converted project(s) and of their direct dependents compile: ['grails-async', 'grails-beans-dsl', 'grails-cache', 'grails-console', 'grails-controllers', 'grails-converters', 'grails-core-cli-legacy', 'grails-data-mongodb', 'grails-databinding', 'grails-datamapping-core', 'grails-datamapping-core-test', 'grails-datamapping-support', 'grails-datamapping-tck', 'grails-datasource', 'grails-domain-class', 'grails-encoder', 'grails-events', 'grails-geb', 'grails-gsp', 'grails-gsp-core', 'grails-layout', 'grails-logging', 'grails-mail', 'grails-openapi', 'grails-quartz', 'grails-redis', 'grails-scaffolding', 'grails-services', 'grails-sitemesh3', 'grails-spring-security', 'grails-spring-security-acl', 'grails-spring-security-oauth2', 'grails-spring-security-rest', 'grails-spring-security-rest-gorm', 'grails-spring-security-rest-grailscache', 'grails-spring-security-rest-memcached', 'grails-spring-security-rest-redis', 'grails-spring-security-ui', 'grails-startup-progress', 'grails-taglib', 'grails-test-suite-base', 'grails-testing-support-datamapping', 'grails-testing-support-views-gson', 'grails-undertow', 'grails-validation', 'grails-views-core', 'grails-views-markup', 'grails-web-common']codeStyle,ratand a whole-repobuild -PskipTestsare run once on the tip of the stack.Notes
MIN_VALUE/MAX_VALUE/next()/previous(), property accessors appear for formerly package-private members, nested static classes with implicit constructors become public (they wereprotected), and generic type parameters that Groovy erases in constructors/methods (descriptors identical).static finalwithoutpubliccreates a property;==callsequals(); Closure/SAM coercions;@CompileStaticwith nested classes. All CI regression fixes found on the umbrella branch are included in the end state.🤖 Generated with Claude Code