Repository navigation
refactor: convert the grails-data-hibernate7 modules to Groovy - #16572
borinquenkid wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The conversion removes a used constructor and introduces Java-facing API compatibility regressions in public annotations and constants.
10 open findings
Restore HibernateAlias two-argument constructor · New Restore public visibility for projection constants · New Restore public visibility for deprecated constants · New Preserve public visibility of enum type constants · New Restore public visibility for BACKTICK · New Restore public visibility for deprecated event constants · New Preserve @Nullable callback return annotation · New Restore @Nullable on HibernateOperations.execute · New Restore public visibility for synchronization constant · New Restore public visibility for PARTICIPATE_SUFFIX · New
What changed in this PR
Converts the Hibernate 7 modules from Java to statically compiled Groovy while preserving their framework behavior and module structure.
Changes:
- Migrates production sources, tests, and fixtures to Groovy.
- Updates Gradle dependencies/plugins for Groovy compilation.
- Repoints RAT exclusions for vendored Liquibase sources.
| File | Description |
|---|---|
grails-data-hibernate7/spring-orm/** |
Converts Spring ORM integration and updates its build. |
grails-data-hibernate7/core/** |
Converts Hibernate query, mapping, proxy, and runtime infrastructure. |
grails-data-hibernate7/dbmigration-core/** |
Converts Liquibase integration, tests, and fixtures. |
grails-data-hibernate7/grails-plugin/** |
Converts plugin persistence-context support. |
gradle/rat-root-config.gradle |
Updates vendored-source exclusions to Groovy paths. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 10.0.x #16572 +/- ##
==================================================
+ Coverage 63.7632% 64.4127% +0.6495%
- Complexity 28262 30683 +2421
==================================================
Files 2260 2407 +147
Lines 112201 119124 +6923
Branches 20361 21741 +1380
==================================================
+ Hits 71543 76731 +5188
- Misses 31507 32537 +1030
- Partials 9151 9856 +705 🚀 New features to boost your workflow:
|
|
Besides the ten findings, the same sweep (javap -public of the 9.0.x build against this branch, for all 319 converted files) found about 130 dropped nullness annotations, a record that had stopped being one (CollectionHolder) and the checked exceptions of configureNamingStrategy; all restored in ee3d2b0 and pinned by JavaFacingApiSpec. The few differences that cannot be matched are Groovy artifacts (extra generated record constructors, two bridge methods, two classes that became public); the stack on top (#16568-#16570) has the fix merged in. |
|
This branch carries the hand port of the fix for #16665 (#16686 on 8.0.x) to the Groovy |
jamesfredley
left a comment
There was a problem hiding this comment.
Not reviewing the rest of the Java-to-Groovy series until this is fixed on the first pull request.
File history has to survive the extension change. After this merges, git log --follow on the .groovy file, and GitHub's blame and history, have to show the old .java history. GitHub and git log --follow only connect a rename when the similarity is at least 50%.
Most of this pull request does that: 299 .java to .groovy pairs are detected as renames at 50% or above. It is not finished. These conversions are still a delete plus a new file, so the Java history stops at the conversion commit:
GrailsDomainBinderGrailsPropertyBinderIndexBinderHibernatePersistentPropertyCollectionKeyColumnUpdaterNamingStrategyWrapperHibernateQueryExecutorHibernateGenericDialect(39%, still under the cutoff)HibernateSpringBeanDatabaseJpaPersistenceDatabaseHibernateChangedIndexChangeGeneratorExtendedSnapshotGenerator- the Lombok test fixtures whose getters and setters had to be expanded
git log --follow on GrailsDomainBinder.groovy at this head shows only the conversion commits. It does not show the Java file.
Do it in two steps, in this pull request, before the later slices copy the same shape:
git mvthe.javato the.groovypath and commit that alone. No rewrite in that commit. Similarity is 100%, so follow always works.- Convert the Groovy file in the next commit. Idiomatic cleanup stays in the later groovyism pull request, the same split the stage-2 commits already describe.
If a file cannot be moved first, the converted file still has to stay at R050 or higher against its .java original. Please re-run git diff -M --name-status <base> HEAD and confirm every converted class is an R line, not a D plus an A. I am not reviewing the other Java-to-Groovy pull requests until this one does that.
| @@ -0,0 +1,311 @@ | |||
| /* | |||
There was a problem hiding this comment.
This file is added, not renamed. At a 20% similarity threshold git still does not pair it with GrailsDomainBinder.java, and git log --follow on this path stops at the conversion commit.
Move the Java file to this path in its own commit, then convert it. The Java history has to be visible from this Groovy file before the rest of the series is reviewed.
There was a problem hiding this comment.
Done in the rebuild: 566d61db27 only moves the Java sources to their Groovy paths (324 R100 renames, GrailsDomainBinder.java → GrailsDomainBinder.groovy among them), and 68ff6d8967 converts them. git log --follow on GrailsDomainBinder.groovy now reaches the Java history back to the hibernate6 folder rename, and the same holds for the files whose combined diff is below the rename threshold. The PR description asks for a merge commit so that both commits survive.
The content of #16572 at 194cd43, on top of the move of the Java sources to their Groovy paths: the grails-data-hibernate7 core and dbmigration modules and their test fixtures converted to Groovy, with the semicolons kept under a suppression header as the stage-2 commits describe, plus the docs and build changes of that pull request. Idiomatic cleanup stays in the groovyism pull request that follows.
194cd43 to
68ff6d8
Compare
…commit): same tree, so this records the new history only
…commit): same tree, so this records the new history only
|
Done as the two steps, in this pull request: the branch is now the move commit and the convert commit, with the tree byte for byte what the previous head 194cd43 had.
The branches built on this one (#16696 and the native-binding stack) have recorded the rebuilt root with a merge that changes no tree, so their diffs against their bases are unchanged. Everything still carries |
jdaugherty
left a comment
There was a problem hiding this comment.
Head 68ff6d8967 on 10.0.x, merge-base 94fd84675f; the tree is byte for byte 194cd43cc2, and a trial merge into the current 10.0.x tip is clean (no duplicate class, no .java left under grails-data-hibernate7 apart from JavaCallerPin).
What I did:
- Checked the commit split:
566d61db27is exactly 324R100lines and nothing else;68ff6d8967is 337 modified and 9 added files.git log --followreaches the Java history on every converted file, including the 13 that the combined diff still reports below 50% (ExtendedSnapshotGeneratorat 2%,GrailsDomainBinderback to 2025). - Compared, hunk by hunk, the hand-ports of the six classes
10.0.xchanged after the last CI'd head4e0951067a(HibernateDatastore,CollectionBinder,ManyToOneValuesBinder,ToManyEntityMultiTenantFilterBinder,MultiTenantEventListener,MultiTenantCollectionEventListener). - Built the four modules from this head:
grails-data-hibernate7-core3880 tests / 0 failures / 22 skipped,dbmigration-core41 / 0,grails-plugin39 / 0;codenarcMaingreen on all four. javap -c -pover every compiled class of the converted sources, looking forScriptBytecodeAdapter,InvokerHelperand call-site use.- A sweep over all 333 files:
@CompileStaticpresence, semicolons and the suppressions that hide them,@author, comment text present in the Java and absent from the Groovy,$in double-quoted strings,assert, integer division, GroovyObject hook names, char literals, closure-to-SAM coercions, wildcard imports. - Checked the files changed outside the conversion against #16695 and #16686.
Must fix
1. The branch carries two open pull requests. CompositeUserTypeBinder, CompositeUserTypeMappingSpec, the CompositeUserType logic in GrailsDomainBinder, GrailsPropertyBinder and HibernatePersistentProperty, the changes to GrailsPropertyBinderSpec, ListSecondPassBinderSpec, MapSecondPassBinderSpec and GrailsHibernatePersistentPropertySpec, the customHibernateTypes.adoc, type.adoc and upgrading80x.adoc §26.11 changes and the hibernate-developer skill line are #16695 (open, base 8.0.x), at a revision behind its head (no CompositeUserTypeColumns). MultipleUnidirectionalHasManySpec, CollectionKeyColumnUpdaterSpec and the join-table logic in CollectionKeyColumnUpdater are #16686 (open, base 8.0.x). Neither is on 10.0.x. A conversion can only carry the conversion: when those PRs merge up, their .java files land next to these Groovy twins, and each feature is reviewed twice at different revisions. Please remove everything that comes from them; those classes are converted in the slice after they reach 10.0.x. That also makes the description's "No other build logic, settings, CI, docs or skills change" true again. Inline on CompositeUserTypeBinder, MultipleUnidirectionalHasManySpec and upgrading80x.adoc.
2. Every converted type is statically compiled. grep -L '@CompileStatic' over the moved files finds PersistentEntityNamingStrategy (an interface with default methods, so its bodies are code) and 14 converted tests and fixtures in dbmigration-core (the batch #16497 added; the older fixtures have it). The other hits are pure interfaces and ManagedEntity, which need nothing. The list is inline on PersistentEntityNamingStrategy.
3. The Java syntax kept for rename similarity has lost its reason. 93 files open with @SuppressWarnings('UnnecessarySemicolon') on the package statement and a TODO(groovyism-cleanup) comment saying the semicolons (in 59 of them also the double-quoted strings, in one the public modifiers) are "kept to preserve rename-similarity against the original .java". The move commit is what preserves the history now, so the reason is gone, and what is left is two styles across the 324 converted files, chosen by a git heuristic: 136 files keep 5,431 lines ending in a semicolon (1,423 of them imports, 119 package statements), 47 class-level suppressions hide UnnecessaryGString over 607 double-quoted strings without interpolation, and 188 files are already clean. These are the first two points of the Groovy style guide, and the suppressions are the only reason codenarcMain passes. Please strip the semicolons and the plain double quotes in the convert commit and delete the suppressions and the TODO comments, so CodeNarc enforces it from here on. The semicolons are mechanical (sed -E 's/;[[:space:]]*$//' over the converted files, or the semicolon step of the repository's codenarcFix task; that task's GString step also rewrites quotes inside comments and license headers, so its output can't be taken as is). codenarcTest is skipped in this build, so the 43 test and fixture files in dbmigration-core that have semicolons and no suppression need the same pass by hand. The deeper idioms (property access for the 1,326 getter and 482 setter calls, Elvis for the 106 != null ? ternaries, literals for the 103 new ArrayList/Arrays.asList/Collections.* calls, GStrings for the 83 concatenations) can stay for the groovyism PR, provided that PR sits in the stack before the next slice copies this shape.
4. Merge with a merge commit, never squash. The history argument only holds if both commits survive. The PR guidelines in CLAUDE.md say to squash, so please put a visible note in the description that this PR, and every slice of the series, must be merged with a merge commit. The description also still describes four commits on 9.0.x, "transplanted unchanged", with no docs or skills changes; please bring it up to date.
Should fix
5. Still open from the previous round. The records keep their public Map constructor (HqlQueryContext(Map), HibernateQueryExecutor(Map), HibernateAlias(Map), PropertyArithmetic(Map)), and javap shows those constructors are the only place in the converted classes where InvokerHelper.createAssertError appears; @TupleConstructor(defaults = false, namedVariant = false) removes them. SqlRestriction (public API) and GroovyProxyInterceptorLogic.InterceptorState have none of the record options. GrailsHibernateTemplate.hibernateFlushModeToConstant is private and its spec calls it anyway. HibernateDatabasePojoTest has no license header. The 32 @author tags in ASF-headed files are still there. Inline where there is a line.
6. OrderByClauseBuilder.parseSortEntries writes currentEntry.direction three times through ScriptBytecodeAdapter.setGroovyObjectProperty (a @PackageScope field of a private nested class, written from the outer class). It isn't in the description's list of remaining dynamic sites. With it, the javap sweep's list for the converted classes is: this, ChildHibernateDatastore.destroy, the HibernateSession.updateAll closure, the spread call in HibernateDatastore.prepareMultiTenantClosure, the enum $INIT boilerplate, and the record Map constructors. Please keep the description's list equal to what the sweep finds.
7. Parameter names. sqlRestriction(String sqlRestriction) became sqlRestriction(String sql) in HibernateCriteriaBuilder and HibernateDetachedCriteriaExtension, Javadoc included. Callers don't notice, but a conversion shouldn't rename public parameters.
Nits
- About 30
@SuppressWarnings('PMD.…')values came over from the Java. PMD doesn't run on Groovy sources, so they are dead; drop them. - Wildcard imports in
HibernateIntegrationTestandSpringPackageScanningIntegrationTest. - 22 comment lines of the Java are missing from the Groovy; most are rewordings that follow a real change (
Optional, streams). The ones simply dropped are "Public constructor." inSimpleValueColumnBinder, theignoreon a catch inHibernatePersistentPropertyand the table-setup comment inColumnRemarksIntegrationTest. Fine as they are; listed because the series should keep comment text intact and diff it. - Closures coerced with
as(HibernateCallback×3,Predicate×10,ResourceConfigurer×5,Runnable×2,BiFunction) allocate a proxy per call; Groovy lambdas under@CompileStaticcompile toinvokedynamic, as the previous round noted. - The twelve files that gained an ASF header (
HibernateChangedIndexChangeGenerator,IdentifierGeneratorSupportand the #16497 tests and fixtures) are Grails-authored, so the headers are right; their exclusions ingradle/rat-root-config.gradlecan go now, so RAT checks them. - After the conversion Checkstyle reports
NO-SOURCEon all four modules andcodenarcTestis skipped, so CodeNarc onmainis the only analyzer left on them. Worth a line in the description.
Verified
- The hand-ports of the six multi-tenancy classes match the
10.0.xJava hunks line for line (computeIfAbsentwith a closure,Tenants.isWithoutId,prependListeners(EventType.INIT_COLLECTION, …), theFetchMode.JOINtoSELECTfallback, the many-to-many owner warning). - No Java literal
$became a GString; noassertin main sources (Groovy's is always on); no/on integers and no bare decimal literals; the char comparisons inHqlQueryContextcast withas char; thegetProperty(String)family is@POJOand the test covers all six;invokeMethodinHibernateCriteriaBuilderis the builder's own hook, as in Java;this == ALLinCascadeBehavioris on an enum, so==is identity there. JavaCallerPinis the right tool: a Java class in the test source set that fails the build when a constant stops being a field, a constructor goes, or athrowsclause changes.- RAT: every repointed exclusion resolves, and no
grails-data-hibernate7/**/*.javapath is referenced anywhere else in the repository.
Before merge
The no ci label can stay on while the stack is being reworked. Once everyone has approved, remove it and let a full run confirm this tree before merging: the last green head is 4e0951067a, and the six hand-ported classes and seven specs above have only been run locally since.
Checklist for the rest of the series
History
- Two commits per slice: a
git mv-only commit (every line ofgit diff -M --name-status <base> <move>isR100), then the conversion. Merge with a merge commit, never squash. git log --followon the lowest-similarity file reaches the Java history; no.javaleft in the module; RAT exclusions repointed; no.javapath of the module referenced anywhere else.
Static compilation
@CompileStaticon every converted class, enum, record, trait, interface with default or static methods, test and fixture. No@CompileDynamicorTypeCheckingMode.SKIPwithout a comment saying why.javap -c -pover the converted classes: noScriptBytecodeAdapter,InvokerHelperor call-site use beyond$getStaticMetaClass, enum$INIT, and the sites the description lists.
Java-facing API
javap -protecteddiff of the old and new jars.public static finalkeepspublic(otherwise it is a property); package-private members get@PackageScope; constructors of private nested classes stay private; no modifier-less field (it becomes a property with a public setter).- Records:
@RecordOptions(getAt = false, toList = false, toMap = false, size = false)and@TupleConstructor(defaults = false, namedVariant = false); a component namedisX/getXhits GROOVY-12452/GROOVY-12451. @Nullable/@Nonnull,throwsclauses, generics, varargs and parameter names unchanged;@Deprecatedand@Overridekept.- A class with
getProperty(String),setProperty,invokeMethodorgetMetaClassgets@POJOor a rename; a method namedplus,minus,getAt,putAt,call,asBoolean,isCase,next,and,oris now an operator for Groovy callers. - A
JavaCallerPin.javain the test source set pins constants, constructors and checked exceptions at compile time.
Semantics
==isequals():this == oinequals()becomesis(), sentinels compare withis(),==on Comparables goes throughcompareTo.?:falls back on empty strings, zero andfalse;Optional.orElseonly on null, so write!= null ?where the Java did.Optional.mapcollapses null to absent.Stream.toList()is unmodifiable; try-with-resources stays try-with-resources;assertis always on;int / intand1.5areBigDecimal;'c'is aString(as char); a$in a double-quoted string interpolates;returnin a closure returns from the closure;this/supermean something else inside a closure; a closure coerced withasis a proxy, a lambda is not.- Inert annotations become live (
@Slf4jadds alogthat shadows the caller's in a delegate-first closure,@Canonical); interface default methods need@CompileStatic;in,as,def,traitare keywords;Closure.call(new Object[]{x})double-wraps.
Style guide (groovy-lang.org/style-guide.html)
- No semicolons; no
publicon classes and methods; single quotes unless interpolating; no.class; property access instead of getters and setters; GStrings instead of concatenation; list and map literals; Elvis and?.only where the semantics are identical;defonly where the type is unknown; explicit imports; PMD and SpotBugs suppressions dropped.
Comments and headers
- Diff the comment text of each pair; keep every Javadoc tag; drop
@authoron ASF-headed files, keep it on vendored code; headers and RAT exclusions follow provenance; no reviewer names or review-process notes in source.
Build, tests, CI, scope
groovyand the code-style plugin on the module; say whenapi groovyis new for consumers; Checkstyle goesNO-SOURCEandcodenarcTestis skipped, so CodeNarc's rule set is the only gate left.- Run the module suites; JUnit tests still discovered (count before and after); a spec for every behaviour fixed during the conversion; CI on the final tree.
- Conversion only: nothing from open PRs, no feature, no doc change; the description lists every intentional difference and every remaining dynamic site.
A skill for the conversion
Please add .agents/skills/java-to-groovy-conversion/SKILL.md in this PR: a framework-development skill, not published, with the front matter validateRepositoryConventions checks (name, description, license: Apache-2.0), a row in the skill table of CLAUDE.md and AGENTS.md, and a mention in the "intentionally not published" sentence of .agents/skills/README.md. It should hold the procedure (move commit, convert commit, merge-commit requirement, what to run: git diff -M --name-status, git log --follow, the javap sweep, JavaCallerPin, the module suites) and the checklist above, so every later slice and its review start from the same list instead of from this thread.
| * @since 8.0.1 | ||
| */ | ||
| @CompileStatic | ||
| class CompositeUserTypeBinder { |
There was a problem hiding this comment.
This class, CompositeUserTypeMappingSpec, the CompositeUserType logic in GrailsDomainBinder, GrailsPropertyBinder and HibernatePersistentProperty, the changes to GrailsPropertyBinderSpec, ListSecondPassBinderSpec, MapSecondPassBinderSpec and GrailsHibernatePersistentPropertySpec, the customHibernateTypes.adoc, type.adoc and upgrading80x.adoc §26.11 changes and the hibernate-developer skill line are #16695, which is open on 8.0.x, and this copy is behind its head (no CompositeUserTypeColumns). None of it is on 10.0.x. A conversion PR can't carry it: when #16695 merges up, its .java files land next to these Groovy twins, and the feature is reviewed twice at two revisions. Please drop everything that comes from #16695; those classes get converted in the slice after it reaches 10.0.x.
There was a problem hiding this comment.
Done in 9f74af0: CompositeUserTypeBinder, CompositeUserTypeMappingSpec and the CompositeUserType code in GrailsDomainBinder, GrailsPropertyBinder and HibernatePersistentProperty are out, with the matching changes to GrailsPropertyBinderSpec, ListSecondPassBinderSpec, MapSecondPassBinderSpec, GrailsHibernatePersistentPropertySpec, the two docs pages and the skill line. The affected files are the straightforward conversion of their 10.0.x Java again; a grep for CompositeUserType finds only the sentence in customHibernateTypes.adoc that is on 10.0.x. The classes get converted in the slice after #16695 reaches 10.0.x.
| * written to its own join table. A sibling to-many that is not bound as a collection, such as a serializable one, | ||
| * neither blocks the session factory nor counts as a sharer of the join table. | ||
| */ | ||
| class MultipleUnidirectionalHasManySpec extends HibernateGormDatastoreSpec { |
There was a problem hiding this comment.
Same for #16686, open on 8.0.x: this spec, the CollectionKeyColumnUpdaterSpec changes and the join-table logic in CollectionKeyColumnUpdater. Please take them out and convert CollectionKeyColumnUpdater as it is on 10.0.x.
There was a problem hiding this comment.
Done in 9f74af0: MultipleUnidirectionalHasManySpec is deleted, CollectionKeyColumnUpdaterSpec is the 10.0.x content with only the quote style changed, and CollectionKeyColumnUpdater is the conversion of its 10.0.x Java again, counting every unidirectional HibernateToManyProperty of the owner.
| * @since 5.0 | ||
| */ | ||
| public interface PersistentEntityNamingStrategy { | ||
| interface PersistentEntityNamingStrategy { |
There was a problem hiding this comment.
Default methods are code, so this interface needs @CompileStatic like every other converted type; without it their bodies compile dynamically. grep -L '@CompileStatic' over the moved files finds this one plus 14 converted tests and fixtures in dbmigration-core that have none either: ColumnRemarksIntegrationTest, HibernateIndexDifferenceSuppressionTest, CustomIdGeneratorTest, IdentifierGeneratorSnapshotTest, NativeGeneratorAutoIncrementTest, NativeGeneratorSequenceTest, SequenceGeneratorTest, columnremarks/Item, CustomIdEntity, NativeGenEntity, SnowflakeId, SnowflakeIdGenerator, ForcedTableEntity, QualifiedSequenceEntity (the batch #16497 added; the older fixtures have it). The remaining hits are pure interfaces and ManagedEntity, which need nothing.
There was a problem hiding this comment.
Done in 9f74af0: PersistentEntityNamingStrategy and the 14 tests and fixtures you listed have @CompileStatic, as has HibernateDatabasePojoTest. The annotation type SnowflakeId has no code and needs none. They compile as they are, and the dbmigration-core suite passes (41 tests).
| */ | ||
| @SuppressWarnings({"rawtypes", "PMD.DataflowAnomalyAnalysis", "PMD.AvoidDuplicateLiterals"}) | ||
| public class HibernateSession extends AbstractAttributeStoringSession implements QueryAliasAwareSession { | ||
| // TODO(groovyism-cleanup): semicolons + double-quoted strings kept to preserve rename-similarity against the original .java (see matrei's review on #16576). Removed by the stacked groovyism-cleanup PR. |
There was a problem hiding this comment.
This comment, and the @SuppressWarnings('UnnecessarySemicolon') on the package statement above, are in 93 files, and the reason they give is gone: the move commit preserves the history now, not the similarity of this diff. What is left is two styles across the 324 converted files, chosen by a git heuristic: 136 files keep 5,431 lines ending in a semicolon (1,423 imports and 119 package statements among them), 47 class-level suppressions hide UnnecessaryGString over 607 double-quoted strings without interpolation, and 188 files are already clean. Please strip the semicolons and the plain double quotes in the convert commit and delete the suppressions and these comments, so CodeNarc enforces it from here. The semicolons are a sed -E 's/;[[:space:]]*$//' over the converted files (or the semicolon step of the codenarcFix task; its GString step also rewrites quotes inside comments and the license headers, so that output can't be taken as is). codenarcTest is skipped in this build, so the 43 test and fixture files in dbmigration-core that have semicolons and no suppression need the same pass by hand.
There was a problem hiding this comment.
Done in 9f74af0, for the whole tree: the TODO(groovyism-cleanup) comments and the UnnecessarySemicolon, UnnecessaryGString and PMD suppressions are gone, CodeNarc found the 4,401 semicolons and 198 plain double quoted strings that the suppressions hid, and they are fixed in the 146 files that had them (a small lexer that leaves strings and comments alone, validated by CodeNarc itself: codenarcMain is clean on all five modules with no suppression). The 175 public modifiers of HibernateConnectionMetadata are gone too. codenarcTest is skipped by the build, so the tests got the same pass, checked by compiling and running the suites. The idioms beyond the first two points of the style guide stay for the groovyism pull request, which sits before the next slice.
| } | ||
| currentEntry.direction = token.toLowerCase(Locale.ROOT); | ||
| } else { | ||
| currentEntry.direction = token.toLowerCase(Locale.ROOT) |
There was a problem hiding this comment.
javap shows this write, and the two below it, compile to ScriptBytecodeAdapter.setGroovyObjectProperty: direction is a @PackageScope field of a private nested class, written from the outer class. It isn't in the description's list of remaining dynamic sites. currentEntry.@direction = … (or a setter on SortEntry) makes it a field write.
There was a problem hiding this comment.
Done in 9f74af0: currentEntry.@direction = ..., so the three writes are field writes. The javap sweep no longer lists OrderByClauseBuilder.
| @@ -34,100 +33,106 @@ | |||
| * @param values the values of the {@code ?} placeholders, in order | |||
| * @since 8.0.0 | |||
| */ | |||
| public record SqlRestriction(String sql, List<?> values) implements Query.Criterion { | |||
| @CompileStatic | |||
| record SqlRestriction(String sql, List<?> values) implements Query.Criterion { | |||
There was a problem hiding this comment.
Public API (@since 8.0.0) and still without the record options the other records got: SqlRestriction(), SqlRestriction(String), SqlRestriction(Map), getAt, toList, toMap and size are all generated. Same for GroovyProxyInterceptorLogic.InterceptorState.
There was a problem hiding this comment.
Done in 9f74af0, with a detail worth knowing: adding @TupleConstructor to SqlRestriction made the generated constructor skip the validation of its compact constructor (a null value is rejected failed with a NullPointerException), so its canonical constructor is written out and the options are on it. GroovyProxyInterceptorLogic.InterceptorState and the liquibase IdentifierGeneratorSupport.SequenceKey got the same options; CodeNarc reads the components of an annotated record as fields and reports their indentation, so the three suppress Indentation with a comment.
| * A Groovy class would take it for the {@code GroovyObject} hook behind every dynamic property access, | ||
| * so the whole family is compiled without {@code GroovyObject}. | ||
| */ | ||
| class HibernateDatabasePojoTest { |
There was a problem hiding this comment.
New Grails test, not fork code, so it needs the ASF header. RAT doesn't see it because database/** is excluded wholesale.
There was a problem hiding this comment.
Done in 9f74af0: the test has the ASF header.
| */ | ||
| public org.grails.datastore.mapping.query.api.Criteria sqlRestriction(String sqlRestriction) { | ||
| return sqlRestriction(sqlRestriction, Collections.emptyList()); | ||
| org.grails.datastore.mapping.query.api.Criteria sqlRestriction(String sql) { |
There was a problem hiding this comment.
The parameter was sqlRestriction in Java (here, in the two-argument overload and in HibernateDetachedCriteriaExtension), and the Javadoc @param moved with it. Callers don't notice, but the conversion shouldn't rename public parameters; please keep the Java names.
There was a problem hiding this comment.
Done in 9f74af0: the parameter is sqlRestriction in HibernateCriteriaBuilder (both overloads) and HibernateDetachedCriteriaExtension, with the Javadoc @param. In Groovy a parameter with the name of a method of the same class shadows it, which is probably why the conversion renamed it, so the overloads call each other with an explicit receiver (this.sqlRestriction(...), HibernateDetachedCriteriaExtension.sqlRestriction(...)).
| @@ -1449,6 +1449,12 @@ class SportsCar extends Car { | |||
| } | |||
| ---- | |||
|
|
|||
| ===== 26.11 Custom User Types | |||
| import org.junit.After; | ||
| import org.junit.Before; | ||
| import org.junit.Test; | ||
| import java.io.* |
There was a problem hiding this comment.
Wildcard imports, here and in SpringPackageScanningIntegrationTest; the repository rule is explicit imports, fork test or not.
There was a problem hiding this comment.
Done in 9f74af0: explicit imports in both tests; the java.io ones are dropped, since Groovy imports them anyway.
|
@jdaugherty, 10.0.x now carries #16695 (composite user types), so this branch conflicted. Following your checklist:
Checked locally: no
|
🔎 No tests executed 🔎🏷️ Commit: 0cfca82 Learn more about TestLens at testlens.app/docs. |
Rename 326 .java files to .groovy under grails-data-hibernate7 to establish 100% rename tracking prior to conversion, preserving file history under git log --follow.
Convert all moved Java sources in grails-data-hibernate7 (spring-orm, core, dbmigration-core, and grails-plugin) to statically-compiled Groovy. - Remove trailing semicolons, plain double quotes, and groovyism TODO comments. - Remove dead PMD, SpotBugs, and UnnecessarySemicolon suppressions. - Add @CompileStatic across all converted classes, traits, enums, records, tests, and fixtures. - Add @RecordOptions(getAt = false, toList = false, toMap = false, size = false) and @TupleConstructor(defaults = false, namedVariant = false) to records. - Preserve Java-facing binary compatibility, package-private visibility with @PackageScope, and explicit receivers where parameter names match methods. - Introduce JavaCallerPin to verify public Java API contracts at compile time. - Add the java-to-groovy-conversion agent skill documenting the two-commit protocol and review checklist.
94ba143 to
a3b6fa5
Compare
|
@jdaugherty @matrei @jamesfredley The branch history has been rebuilt directly on
|


What
First slice of the Java-to-Groovy migration tracked in #16351: the Java sources of the
grails-data-hibernate7Gradle projects (spring-orm,core,dbmigration-core, and the plugin project) become Groovy. It is the conversion and nothing else: no feature, no fix, no documentation change, and nothing from a pull request that has not reached10.0.x(#16695 is merged into10.0.xand included; #16686 is unmerged and purged).Commits, in order, on top of
b4cb2ae96d(10.0.x):92c41267a0git mvonly: 326.javafiles to their.groovypaths, every lineR100, no content changea3b6fa56d6publicon members),@CompileStaticon every type with code, records with@RecordOptionsand@TupleConstructor, no@authoron ASF-headed files, JavaCallerPin, and java-to-groovy-conversion skillgit log --followon any converted file reaches its Java history.Why two commits
A one-commit conversion drops under the similarity git needs to follow a rename, so the first revision of this series kept Java syntax (semicolons, double quotes) to hold the similarity up. The move commit makes that unnecessary, so the Java syntax is gone and CodeNarc enforces the Groovy style from here on.
.agents/skills/java-to-groovy-conversionrecords the procedure and the review checklist for the slices that follow.Java-facing API
@CompileStatic; a pure interface and an annotation type need none. The liquibase-hibernateHibernateDatabasefamily also carries@POJO, so its publicgetProperty(String)does not become theGroovyObjecthook.@RecordOptions(getAt = false, toList = false, toMap = false, size = false)and@TupleConstructor(defaults = false, namedVariant = false), so they have only their canonical constructor.SqlRestrictionvalidates in its explicit canonical constructor and suppresses the record indentation false-positive with@SuppressWarnings('Indentation').@PackageScope,protectedorprivate;public static finalstayspublic; public parameter names are the Java ones (a parameter named like a method of the same class is called with an explicit receiver).JavaCallerPinis a Java class in the test source set: a constant that stops being a field, a constructor that goes, or athrowsclause that changes fails the build.Intentional differences from the Java API:
HqlQueryContext's record components aremutatingandnativeQuery, with explicitisUpdate()andisNative(): under@CompileStatic(Groovy 6.0.0) a record component namedisXcannot be called asisX()from another class.spring-ormanddbmigration-coredeclareapi 'org.apache.groovy:groovy', so a consumer gets Groovy on its compile classpath.spring-ormwas plain Java.Remaining dynamic sites
javap -c -pover the converted main classes finds these uses of the Groovy runtime beyond operators and the generated enum and metaclass boilerplate:ChildHibernateDatastore.destroyand theHibernateSession.updateAllclosure: a property write throughsetGroovyObjectProperty;HibernateDatastore.prepareMultiTenantClosure: a spread call;HibernateQuery.clone: a field read throughgetGroovyObjectField;HibernateConnectionSourceSettingsand its five nested settings classes:clone()callssuper.clone()of a Groovy superclass;GroovyProxyInterceptorLogic.handleUninitializedandGroovyProxyMetaClassDispatcher.intercept:InvokerHelper.getMetaClass, which is what the proxy dispatch is for.Build and tooling
Checkstyle reports
NO-SOURCEon the four modules andcodenarcTestis skipped in this build, so CodeNarc onmainis the only analyzer left on them; the converted tests were checked by hand.gradle/rat-root-config.gradlerepoints the RAT exclusions of the liquibase-hibernate fork files from.javato.groovy.Verification
On this head, one Gradle process at a time:
:grails-data-hibernate7-core:test:grails-data-hibernate7-dbmigration-core:testcodenarcMainon the modulesUnnecessarySemicolonorUnnecessaryGStringsuppression leftcompileGroovyandcompileTestGroovy@CompileStaticcodeStyle,validateRepositoryConventions,ratA trial merge into
10.0.xis clean, and no.javafile is left undergrails-data-hibernate7exceptJavaCallerPin.