jdaugherty commented on code in PR #15972:
URL: https://github.com/apache/grails-core/pull/15972#discussion_r3564513288
##########
gradle.properties:
##########
@@ -40,6 +41,9 @@ jnrPosixVersion=3.1.20
joddWotVersion=3.3.8
joptSimpleVersion=5.0.4
jspApiVersion=4.0.0
+logbackClassicVersion=1.4.14
+neo4jDriverVersion=4.4.13
Review Comment:
Per the mono-repo-integration skill (Principle 2 / Phase 2) and CLAUDE.md's
Dependency Management section, versions of dependencies that ship on a
published module's runtime classpath belong in `dependencies.gradle` so the BOM
manages them — a loose `gradle.properties` entry is build-internal and
invisible to applications consuming `grails-bom`.
`org.neo4j.driver:neo4j-java-driver` is an api/runtime dependency of the
published modules (and is `force`d in three build files), so consumers should
inherit its version from the BOM. `neo4jVersion` (the test-only embedded
harness) and `geantyref` (testRuntimeOnly) are fine as properties.
Separately, `logbackClassicVersion=1.4.14` pins a logback *older* than what
`spring-boot-dependencies` already manages (1.5.x on the Boot 4 baseline).
CLAUDE.md: "Prefer inheriting from the Spring Boot BOM... do not re-pin a
coordinate that spring-boot-dependencies already manages." If the Neo4j 3.5
harness genuinely requires 1.4.x on that test classpath, keep it but add an
inline comment saying so; otherwise drop the explicit version and let the BOM
resolve it.
##########
gradle/publish-root-config.gradle:
##########
@@ -138,6 +138,10 @@ def publishedProjects = [
// graphql
'grails-data-graphql',
'grails-data-graphql-core',
+ // neo4j
+ 'grails-data-neo4j',
+ 'grails-datastore-gorm-neo4j',
+ 'gorm-neo4j-spring-boot',
Review Comment:
Two things on the publishing side of the migration (skill Phase 2):
1. **The coordinate rename isn't registered.** These modules previously
published as `org.grails:grails-datastore-gorm-neo4j` /
`org.grails.plugins:neo4j` / `org.grails:gorm-neo4j-spring-boot` and now
publish under `org.apache.grails`. The skill requires recording that in both
`RENAME.md` (old→new mapping table) and a `neo4j_mappings` block in
`etc/bin/rename_gradle_artifacts.sh` (mirroring `redis_mappings`), so the
migration is documented and the rewrite script stays complete. Neither is
touched in this PR.
2. **`gorm-neo4j-spring-boot` enshrines a legacy artifact id in the new
namespace.** The migration doc says project names were kept to avoid touching
internal `project(...)` references, which is fair for internal names — but this
one is a *published coordinate*. Before the first ASF release is the cheapest
possible moment to rename it to the `grails-` convention (e.g.
`grails-data-neo4j-spring-boot`); after that it needs its own RENAME.md entry
forever. Worth a deliberate decision rather than a default.
3. The skill also calls for merging the imported repo's developer list into
`PublishPlugin.groovy` (deduped against founder/developer/contributor/emeritus,
classified by commit recency). If everyone on the standalone repo's list is
already present, a note in the PR is enough — but it should be checked, not
skipped.
##########
grails-data-neo4j/examples/test-data-service/build.gradle:
##########
@@ -1,37 +0,0 @@
-/*
Review Comment:
Two of the five imported example apps are deleted here without being
relocated: `test-data-service` (28 files — the data-services/GORM-services
coverage app) and `neo4j-standalone` (the no-Grails, plain-GORM usage example).
Only `grails3-neo4j`, `grails3-neo4j-hibernate`, and `neo4j-spring-boot` made
it to `grails-test-examples/neo4j/`.
The earlier `grails7-initial-neo4jUpdates` branch preserved all five (as
`grails-test-examples/neo4j/{base,hibernate5,spring-boot,test-data-service}`
plus `neo4j-standalone`), so this looks like silently dropped test coverage
rather than a deliberate cut. If dropping them is intentional (e.g. superseded
by the TCK's data-service specs), the PR description should say so; otherwise
they should move to `grails-test-examples/neo4j/` with the others.
##########
grails-data-neo4j/GORM_REGISTRY_MIGRATION.md:
##########
@@ -0,0 +1,181 @@
+# Neo4j → GormRegistry: migration plan
Review Comment:
This is a planning/history document — PR sequencing, deviations discovered
while folding, `codenarcFix` warnings. That content is genuinely valuable (the
codenarcFix-corrupts-Cypher-strings finding especially), but the repo doesn't
keep per-migration narrative docs in module folders, and this one will be stale
the moment the stack merges (it describes PRs by number and branch names).
Suggest: move the narrative to the PR description / an issue; preserve the
durable operational facts where they're actually discoverable — the codenarcFix
hazard as a comment next to the CodeNarc suppression blocks it justifies
(partially done already), and the force-block rationale in the build files
(already done). If the maintainers want migration history in-repo, `RENAME.md`
and the commit messages are the established homes.
##########
settings.gradle:
##########
@@ -397,6 +414,16 @@
project(':grails-test-examples-mongodb-test-data-service').projectDir = new File
include 'grails-test-examples-mongodb-gson-templates'
project(':grails-test-examples-mongodb-gson-templates').projectDir = new
File(settingsDir, 'grails-test-examples/mongodb/gson-templates')
+// functional tests - neo4j examples
+include 'grails-test-examples-neo4j-grails3-neo4j'
Review Comment:
The test-integration wiring from the skill's Phase 2 is absent: there are no
`onlyNeo4jTests`/`skipNeo4jTests` flags in
`gradle/test-config.gradle`/`functional-test-config.gradle`/`grails-data-tck-config.gradle`,
nothing in `DEVELOPMENT.md`, and no `.github/workflows/gradle.yml` change.
Consequences:
- The three example apps' tests join whatever default slice picks them up,
with no way to run just the Neo4j slice locally or in CI, and no way to *skip*
them (the embedded Neo4j 3.5 harness is slow to boot and needs `--add-opens
java.lang`/`sun.nio.ch` JVM args — worth confirming the shared
functional-test-config runners actually pass those, since the migration doc
notes they'd only ever been configured on the core module).
- Per the skill, every new test job must be added to the publish job's
`needs:` list and `if:` result guard. With no dedicated job, nothing gates
snapshot publishing on the Neo4j functional tests specifically — they exist but
only protect publishing to the extent the default matrix happens to run them.
Since Neo4j here is embedded (no service container), the Testcontainers
guidance doesn't apply — this is just the flags + CI-job + publish-gating
wiring.
##########
settings.gradle:
##########
@@ -397,6 +414,16 @@
project(':grails-test-examples-mongodb-test-data-service').projectDir = new File
include 'grails-test-examples-mongodb-gson-templates'
project(':grails-test-examples-mongodb-gson-templates').projectDir = new
File(settingsDir, 'grails-test-examples/mongodb/gson-templates')
+// functional tests - neo4j examples
+include 'grails-test-examples-neo4j-grails3-neo4j'
+project(':grails-test-examples-neo4j-grails3-neo4j').projectDir = new
File(settingsDir, 'grails-test-examples/neo4j/grails3-neo4j')
Review Comment:
Two items visible by comparison with the earlier
`grails7-initial-neo4jUpdates` branch:
1. **Grails Forge integration is missing entirely.** Neo4j was removed from
Forge in #15107 (`Remove Neo4j GORM support`), and the earlier branch restored
it (`Neo4jGorm` feature + `GormImpl` option + `Neo4JGormSpec`). This PR doesn't
touch `grails-forge/`, so the module gets published but users still can't
select GORM for Neo4j when generating an application — the reintegration isn't
user-reachable until that feature returns (with the new
`org.apache.grails:grails-data-neo4j` coordinates rather than the old
`org.grails.plugins:neo4j` the branch used).
2. **The intended module/app names already exist on that branch**:
`grails-data-neo4j/core` → `grails-data-neo4j-core`, planned
`grails-data-neo4j-spring-boot` and `grails-data-neo4j-docs`, and example apps
named `base`/`hibernate5`/`spring-boot` — matching the
`grails-data-mongodb-core`/`grails-data-hibernate5-core` convention. This PR
instead carries the legacy `grails-datastore-gorm-neo4j`,
`gorm-neo4j-spring-boot`, and `grails3-neo4j*` names into the monorepo (see the
naming comment on publish-root-config), so the naming question isn't open-ended
— the convention-conformant names were already chosen once.
##########
grails-data-neo4j/grails-datastore-gorm-neo4j/build.gradle:
##########
@@ -17,93 +17,122 @@
* under the License.
*/
+plugins {
+ id 'groovy'
+ id 'java-library'
+ id 'org.apache.grails.buildsrc.properties'
+ id 'org.apache.grails.buildsrc.compile'
+ id 'org.apache.grails.buildsrc.publish'
+ id 'org.apache.grails.buildsrc.sbom'
+ id 'org.apache.grails.gradle.grails-code-style'
+}
+
+version = projectVersion
+group = 'org.apache.grails.data'
+
+ext {
+ gormApiDocs = true
+ pomTitle = 'GORM for Neo4j'
+ pomDescription = 'Provides a GORM Object Mapping implementation for the
Neo4j Graph Database'
+}
+
sourceSets.main.java.srcDirs = []
sourceSets.main.groovy.srcDirs += ["src/main/java"]
dependencies {
+
+ implementation platform(project(':grails-bom'))
+
api "org.neo4j.driver:neo4j-java-driver:$neo4jDriverVersion"
- api
"org.apache.grails.data:grails-datamapping-validation:$datastoreVersion"
- api "org.apache.grails.data:grails-datamapping-core:$datastoreVersion"
+ api project(':grails-datamapping-validation')
+ api project(':grails-datamapping-core')
// only needed for web dependencies
- compileOnly "org.apache.grails.data:grails-datastore-web:$datastoreVersion"
+ compileOnly project(':grails-datastore-web')
compileOnly "org.neo4j.test:neo4j-harness:$neo4jVersion"
- implementation "org.javassist:javassist:$javassistVersion"
+ implementation 'org.javassist:javassist'
+
testImplementation "org.neo4j.test:neo4j-harness:$neo4jVersion"
- testImplementation
"org.apache.grails.data:grails-datamapping-core-test:$datastoreVersion"
- testImplementation
"org.apache.grails.data:grails-datamapping-tck-tests:$datastoreVersion"
- testImplementation
"org.hibernate:hibernate-validator:$hibernateValidatorVersion"
+ testImplementation project(':grails-datamapping-core-test')
+ testImplementation project(':grails-datamapping-tck')
+ testImplementation 'org.spockframework:spock-core'
+ testImplementation 'jakarta.validation:jakarta.validation-api'
+ testImplementation 'org.hibernate.validator:hibernate-validator'
testImplementation "org.codehaus.gpars:gpars:$gparsVersion"
- testImplementation "cglib:cglib-nodep:$cglibNodepVersion"
- testImplementation "org.objenesis:objenesis:${objenesisVersion}"
-
- testRuntimeOnly "org.springframework:spring-aop:$springVersion"
- testRuntimeOnly "ch.qos.logback:logback-classic:1.4.14"
- testRuntimeOnly "javax.el:javax.el-api:3.0.0"
- testRuntimeOnly "org.glassfish.web:el-impl:2.2.1-b05"
+ testImplementation 'org.objenesis:objenesis'
+
+ testRuntimeOnly 'org.junit.platform:junit-platform-launcher'
+
+ // Spock's Mock() support needs this at runtime but spock-core doesn't
declare it as a dependency
+ testRuntimeOnly "io.leangen.geantyref:geantyref:$geantyrefVersion"
+ testRuntimeOnly 'net.bytebuddy:byte-buddy' // Required by Spock's mocking
support (cglib doesn't work on JDK 21+)
+ testRuntimeOnly 'org.springframework:spring-aop'
+ testRuntimeOnly "ch.qos.logback:logback-classic:$logbackClassicVersion"
+ testRuntimeOnly "jakarta.el:jakarta.el-api:$elApiVersion"
+ testRuntimeOnly
"org.glassfish.expressly:expressly:$defaultElImplementationVersion"
+}
+// The Spring Boot BOM (pulled in transitively via grails-bom) force-upgrades
Jetty to a
+// 12.x platform version and neo4j-java-driver to 6.x.
+def neo4jHarnessJettyVersion = '9.4.43.v20210629'
+
+// The embedded Neo4j 3.5.x test harness (neo4j-harness, test-only) is
compiled against Jetty
+// 9.4 and is binary-incompatible with Jetty 12's restructured handler/server
APIs. Scoped to
+// the test classpaths only, since main code never touches Jetty directly.
+[configurations.testCompileClasspath,
configurations.testRuntimeClasspath].each {
+ it.resolutionStrategy {
+ force "org.eclipse.jetty:jetty-server:$neo4jHarnessJettyVersion",
+ "org.eclipse.jetty:jetty-servlet:$neo4jHarnessJettyVersion",
+ "org.eclipse.jetty:jetty-webapp:$neo4jHarnessJettyVersion",
+ "org.eclipse.jetty:jetty-security:$neo4jHarnessJettyVersion",
+ "org.eclipse.jetty:jetty-http:$neo4jHarnessJettyVersion",
+ "org.eclipse.jetty:jetty-io:$neo4jHarnessJettyVersion",
+ "org.eclipse.jetty:jetty-util:$neo4jHarnessJettyVersion",
+ "org.eclipse.jetty:jetty-util-ajax:$neo4jHarnessJettyVersion",
+ "org.eclipse.jetty:jetty-xml:$neo4jHarnessJettyVersion"
+ }
+}
+
+// This module's own main code (e.g. Neo4jQuery#executeQuery) calls
Driver#defaultTypeSystem(),
+// which driver 6.x removed - so this force applies to all configurations,
main and test alike.
+configurations.all {
+ resolutionStrategy {
+ force "org.neo4j.driver:neo4j-java-driver:$neo4jDriverVersion"
+ }
}
test {
useJUnitPlatform()
- maxParallelForks = configuredTestParallel
+ systemProperty('neo4j.gorm.suite', System.getProperty('neo4j.gorm.suite')
?: true)
+ maxParallelForks = (findProperty('maxTestParallel') as Integer) ?: 1
forkEvery = 10
- jvmArgs = ['-Xmx1028M']
- afterSuite {
- System.out.print('.')
- System.out.flush()
- }
+ // The embedded Neo4j 3.5.x test harness reflectively pokes JDK internals
(Throwable's
+ // message field, sun.nio.ch.FileChannelImpl's lock accessors) at startup;
JDK 9+ strong
+ // encapsulation blocks that without these opens.
+ jvmArgs = [
+ '-Xmx1028M',
+ '--add-opens', 'java.base/java.lang=ALL-UNNAMED',
+ '--add-opens', 'java.base/sun.nio.ch=ALL-UNNAMED',
+ ]
}
-test.doFirst {
- def toBaseClassRelativePathWithoutExtension = { String base, String
classFile ->
- if (classFile.startsWith(base)) {
- def sansClass = classFile[0 .. classFile.size() - ".class".size()
- 1]
- def dollarIndex = sansClass.indexOf('$')
- def baseClass = dollarIndex > 0 ? sansClass[0..dollarIndex - 1] :
sansClass
- def relative = baseClass - base - '/'
- relative
- }
- else {
- null
- }
- }
- def tckClassesFile = project
- .configurations
- .testCompileClasspath
- .resolvedConfiguration
- .getResolvedArtifacts()
- .find { resolved ->
- resolved.moduleVersion.id.name ==
'grails-datamapping-tck-tests'
- }.file
-
- def tckClassesDir = project.file("${project.buildDir}/tck")
- copy {
- from zipTree(tckClassesFile)
- into tckClassesDir
- }
- copy {
- from tckClassesDir
- into sourceSets.test.output.classesDirs.find {
it.path.contains('classes/groovy') }
- include "**/*.class"
- exclude { details ->
- // Do not copy across any TCK class (or nested classes of that
class)
- // If there is a corresponding source file in the particular
modules
- // test source tree. Allows a module to override a test/helper.
-
- if (!details.file.isFile()) {
- return false
- }
- def candidatePath = details.file.absolutePath
- def relativePath =
toBaseClassRelativePathWithoutExtension(tckClassesDir.absolutePath,
candidatePath)
-
- if (relativePath == null) {
- throw new IllegalStateException("$candidatePath does not
appear to be in the TCK")
- }
-
- project.file("src/test/groovy/${relativePath}.groovy").exists()
- }
- }
+apply {
+ from
rootProject.layout.projectDirectory.file('gradle/grails-data-tck-config.gradle')
+ from rootProject.layout.projectDirectory.file('gradle/docs-config.gradle')
+}
+
+// This module predates the repo's Checkstyle/CodeNarc gate entirely: it was a
standalone Gradle
+// build (never included in root settings.gradle) until this PR wired it in,
so its Grails 3-era
+// source has never been checked against these rules. ~1,400 pre-existing
violations across the
+// module are tracked as a follow-up cleanup PR rather than fixed here -
codenarcFix's automated
+// fixes for SpaceAroundMapEntryColon/UnnecessaryGString are unsafe on this
module specifically,
+// since they rewrite string *contents* and this module embeds Cypher queries
in string literals
+// throughout (e.g. "MATCH (n:Label)"). Reports still generate; only
build-breaking is suppressed.
+tasks.withType(Checkstyle).configureEach {
+ ignoreFailures = true
Review Comment:
`ignoreFailures = true` for Checkstyle and CodeNarc (here and in
boot-plugin/grails-plugin) conflicts with CLAUDE.md rule 12 — the aggregate
violation reports are required to be empty before commits — and with the
skill's Phase 5 ("fix mechanically until codenarcMain passes"). With this in
place the ~1,400 violations don't fail the build, but they *will* pollute
`aggregateViolations` output for everyone, making the "reports must be empty"
gate meaningless repo-wide, not just for these modules.
The migration doc's rationale (never-checked Grails 3-era code,
`codenarcFix` unsafe on Cypher string literals) is legitimate — but the
mitigation should be: (a) a tracking issue for the cleanup referenced from
these comments, (b) the violation-fixer skill run with
`SpaceAroundMapEntryColon`/`UnnecessaryGString` excluded rather than blanket
suppression, or at minimum (c) confirmation from the maintainers that temporary
suppression is acceptable on this feature branch before it merges toward 8.0.x.
As written, the exception is open-ended with no owner.
--
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]
