jdaugherty commented on code in PR #15972: URL: https://github.com/apache/grails-core/pull/15972#discussion_r3791863236
########## gradle.properties: ########## @@ -42,6 +43,8 @@ jnrPosixVersion=3.1.20 joddWotVersion=3.3.8 joptSimpleVersion=5.0.4 jspApiVersion=4.0.0 +logbackClassicVersion=1.4.14 +neo4jVersion=3.5.35 Review Comment: Two leftovers here. **`logbackClassicVersion=1.4.14` is dead.** It's referenced nowhere in the tree. `grails-data-neo4j/core/build.gradle:76` already declares `testRuntimeOnly 'ch.qos.logback:logback-classic'` with no version, so the BOM resolves it — which is the right outcome. The property that's left behind still reads as a deliberate downgrade below what `spring-boot-dependencies` manages. Please drop the line. **`neo4jVersion` should move into `grails-neo4j-bom`.** Now that a Neo4j-specific BOM exists, this is the natural home for it, and it isn't build-internal the way I assumed when I first said a property was fine here: - `grails-data-neo4j/core/build.gradle:57` declares `compileOnly "org.neo4j.test:neo4j-harness:$neo4jVersion"`, and three published main-source classes compile against it — `EmbeddedNeo4jServer`, `Neo4jEmbeddedConnectionSource`, and `Neo4jConnectionSourceFactory`. Any application using `grails.neo4j.type=embedded` therefore needs `neo4j-harness` on its own classpath, with no managed version to inherit. - This already breaks generated applications. `Neo4jGorm.java:62-65` adds `org.neo4j.test:neo4j-harness` as a versionless `testRuntimeOnly` dependency, and nothing manages that coordinate: `spring-boot-dependencies` 4.1.0 manages `org.neo4j.driver:neo4j-java-driver-bom` but has no `org.neo4j.test` entry, and neither `grails-bom` nor `grails-neo4j-bom` declares it. A Forge-generated Neo4j application will fail to resolve it. `Neo4JGormSpec` only asserts the `grails-data-neo4j` line of the rendered template, so it doesn't catch this — worth asserting the harness line too. There's already precedent for BOM-managing a test-only harness: `liquibase-test-harness` is declared in `dependencies.gradle`'s hibernate5/hibernate7 `customBomVersions` blocks. Declaring `neo4j-harness` alongside `neo4j-driver` in the `grails-neo4j-bom` branch fixes the generated app and lets the nine build-script interpolations of `$neo4jVersion` drop their explicit versions. `grails-data-neo4j/docs/build.gradle:75` passes the value through as an asciidoc attribute, so that one still needs to read the version from wherever it lands. ########## grails-test-examples/neo4j/base/build.gradle: ########## @@ -0,0 +1,104 @@ +/* + * 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. + */ +plugins { + id 'org.apache.grails.buildsrc.properties' + id 'org.apache.grails.buildsrc.dependency-validator' + id 'org.apache.grails.buildsrc.compile' +} + +version = projectVersion +group = 'examples' + +apply plugin: 'org.apache.grails.gradle.grails-web' +apply plugin: 'cloud.wondrify.asset-pipeline' + +dependencies { + + implementation platform(project(':grails-neo4j-bom')) + + implementation 'org.apache.grails:grails-data-neo4j' + implementation 'org.apache.grails:grails-core' + implementation 'org.apache.grails:grails-rest-transforms' + implementation 'org.apache.grails:grails-web-boot' + implementation 'org.apache.grails:grails-gsp' + implementation 'org.apache.grails:grails-sitemesh3' + + testAndDevelopmentOnly platform(project(':grails-neo4j-bom')) + testAndDevelopmentOnly 'org.webjars.npm:jquery' + + runtimeOnly 'cloud.wondrify:asset-pipeline-grails' + runtimeOnly 'org.fusesource.jansi:jansi' + runtimeOnly 'org.apache.grails:grails-scaffolding' + runtimeOnly 'org.apache.grails:grails-fields' + runtimeOnly 'org.apache.grails:grails-url-mappings' + runtimeOnly 'org.springframework.boot:spring-boot-autoconfigure' + runtimeOnly 'org.springframework.boot:spring-boot-starter-logging' + runtimeOnly 'org.springframework.boot:spring-boot-starter-tomcat' + + testImplementation 'org.apache.grails.testing:grails-testing-support-core' + testImplementation 'org.apache.grails:grails-testing-support-datamapping' + testImplementation 'org.apache.grails:grails-testing-support-web' + testImplementation 'org.spockframework:spock-core' + + testRuntimeOnly "org.neo4j.test:neo4j-harness:$neo4jVersion" + + integrationTestImplementation testFixtures('org.apache.grails:grails-geb') +} + +// The Spring Boot BOM (pulled in transitively via grails-neo4j-bom) force-upgrades Jetty to a +// 12.x platform version, both binary-incompatible with the embedded Neo4j 3.5.x test harness +// this app's tests use (neo4j-java-driver itself is already pinned by grails-neo4j-bom's +// 'strictly' constraint). See grails-data-neo4j/core/build.gradle for the full explanation. +def neo4jHarnessJettyVersion = '9.4.43.v20210629' +[configurations.testCompileClasspath, configurations.testRuntimeClasspath, + configurations.integrationTestCompileClasspath, configurations.integrationTestRuntimeClasspath].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" + } +} +// Deliberately older than the BOM's Jetty version (see comment above) - exempt from +// validateDependencyVersions. neo4j-java-driver itself no longer needs an override here: it's +// already pinned to the same version by grails-neo4j-bom's 'strictly' constraint. +project.ext.allowedBomOverrides = [ + 'org.eclipse.jetty:jetty-server', + 'org.eclipse.jetty:jetty-http', + 'org.eclipse.jetty:jetty-io', + 'org.eclipse.jetty:jetty-security', + 'org.eclipse.jetty:jetty-util', + 'org.eclipse.jetty:jetty-util-ajax', + 'org.eclipse.jetty:jetty-xml' +] Review Comment: The `test{}`/Jetty-force block was extracted into `grails-data-neo4j/gradle/neo4j-harness-test-config.gradle` for core/grails-plugin/boot-plugin, which is what I was after — but the same block is still copy-pasted into all five example apps, and it has already drifted. This app, `hibernate5`, and `spring-boot` force nine Jetty coordinates down to 9.4 but list only seven in `allowedBomOverrides` — `jetty-servlet` and `jetty-webapp` are forced and not exempted. `neo4j-standalone`, `test-data-service`, and the shared script all list nine. That inconsistency is exactly what extracting the block prevents. Preference is to extend the shared script to cover the example apps' `integrationTestCompileClasspath`/`integrationTestRuntimeClasspath` configurations and `apply from:` it here as well, so there's one copy. Failing that, please at least bring the three short lists back in sync with what each app actually forces. -- 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]
