jdaugherty commented on code in PR #16337: URL: https://github.com/apache/grails-core/pull/16337#discussion_r4018511577
########## grails-forge/grails-forge-core/src/main/java/org/grails/forge/ForgeContexts.java: ########## @@ -0,0 +1,48 @@ +/* + * 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. + */ +package org.grails.forge; + +import org.springframework.context.annotation.AnnotationConfigApplicationContext; +import org.springframework.core.env.ConfigurableEnvironment; +import org.springframework.core.env.MapPropertySource; +import org.springframework.core.env.StandardEnvironment; + +import java.util.Map; + +public final class ForgeContexts { + + private ForgeContexts() { + } + + public static AnnotationConfigApplicationContext create() { + return create(Map.of()); + } + + public static AnnotationConfigApplicationContext create(Map<String, Object> configuration) { Review Comment: Nothing on the main classpath consumes the `Environment` this overload populates: there is no `@Value` or `Environment#getProperty` in `grails-forge-core` or `grails-forge-cli`, and Boot's `@ConfigurationProperties` binder is not registered in this plain `AnnotationConfigApplicationContext`, so `GrailsForgeConfiguration` would not bind here either (it only lives in the hosted app, where auto-configuration handles it). The only caller is `ApplicationContextSpec`, which passes an empty map. Suggest removing this overload and the `configuration` hook in the spec rather than keeping an API that implies configuration takes effect. If a consumer is planned, the property source is only half of it; the reader has to be written as well. ########## grails-forge/grails-forge-cli/src/main/java/org/grails/forge/cli/Application.java: ########## @@ -91,31 +93,36 @@ public static void main(String[] args) { static CommandLine createCommandLine() { boolean noOpConsole = Application.interactiveShell; - try (BeanContext beanContext = ApplicationContext.builder().deduceEnvironment(false).start()) { + try (AnnotationConfigApplicationContext beanContext = ForgeContexts.create()) { return createCommandLine(beanContext, noOpConsole); } } static int execute(String[] args) { boolean noOpConsole = args.length > 0 && args[0].startsWith("update-cli-config"); - try (BeanContext beanContext = ApplicationContext.builder().deduceEnvironment(false).start()) { + try (AnnotationConfigApplicationContext beanContext = ForgeContexts.create()) { Review Comment: Every command still builds and tears down a full context here, and the interactive shell calls this once per line. With classpath scanning replacing Micronaut's compile-time DI that is noticeably more expensive. Measured locally on JDK 21, five runs each, against the `grails-forge-cli` runtime classpath (no shadow jar): | Command | 8.0.x | this PR | |---|---|---| | `--version` | ~295 ms | ~390 ms | | `create-app --help` | ~310 ms | ~415 ms | | `create-app temp --list-features` | ~320 ms | ~430 ms | Acceptable for one-shot `grails create-app`, but in the shell it is paid on every line. Suggest creating a single context in `main` for the interactive path and handing it to `execute`, so the scan runs once per session. That would also remove the arrangement where `createCommandLine()` closes the context and then gives the shell a `CommandLine` whose `GrailsPicocliFactory` still references it. It works today because picocli instantiates subcommands, converters and the version provider eagerly during construction (completion, usage and version rendering after close all check out), but `GrailsPicocliFactory` only catches `NoSuchBeanDefinitionException`, so anything picocli ever creates lazily would surface as an `IllegalStateException` from the closed context. ########## grails-forge/grails-forge-web/src/test/groovy/org/grails/forge/web/GrailsForgeConfigurationSpec.groovy: ########## @@ -17,25 +17,32 @@ * under the License. */ -package org.grails.forge.api +package org.grails.forge.web - -import io.micronaut.test.extensions.spock.annotation.MicronautTest -import jakarta.inject.Inject +import org.grails.forge.api.GrailsForgeConfiguration import spock.lang.Specification -@MicronautTest(startApplication = false) -class FeatureOperationsSpec extends Specification { +class GrailsForgeConfigurationSpec extends Specification { + + void "redirectUrl is exposed as a redirect URI without a redirectUri setter"() { + given: + GrailsForgeConfiguration config = new GrailsForgeConfiguration() + + when: + config.redirectUrl = 'https://start.grails.org/' - @Inject - FeatureOperations featureOperations + then: + config.redirectUrl == 'https://start.grails.org/' + config.redirectUri().get().toString() == 'https://start.grails.org/' + } - void "only visible features are exposed"() { + void "cors origins remain overridable by environment variables"() { Review Comment: This asserts the text of `application.yml` rather than the behaviour it stands in for, so it needs editing whenever the yml changes and would still pass if the placeholders stopped being resolved. The two mechanisms behind it are untested: Boot binding `grails.forge.redirect-url` into `GrailsForgeConfiguration` inside the Grails context (the POJO test above bypasses binding), and `corsConfigurationPlaceholderResolver` in `Application` rewriting `${...}` in `GrailsCorsConfiguration#allowedOrigins`. `ForgeApiIntegrationSpec` checks CORS for the literal origins but never that a placeholder was resolved, and the `text/html` branch of `ForgeApplicationController#home` is never taken. Suggest replacing this feature with integration coverage of those branches: `GET /` with `Accept: text/html` expecting a permanent redirect to the bound URL, and an assertion after startup that no `allowedOrigins` entry still contains `${`. ########## grails-forge/grails-forge-cli/src/main/java/org/grails/forge/cli/Application.java: ########## @@ -46,28 +50,28 @@ optionListHeading = "%n@|bold,underline Options:|@%n", commandListHeading = "%n@|bold,underline Commands:|@%n", subcommands = { - // Creation commands CreateAppCommand.class, CreateWebappCommand.class, CreatePluginCommand.class, CreateWebPluginCommand.class, CreateRestApiCommand.class }) -@Prototype -@TypeHint({ - Application.class, - GormImplCandidates.class, - GormImplConverter.class, - ServletImplCandidates.class, - ServletImplConverter.class, - CommonOptionsMixin.class, - DevelopmentReloadingCandidates.class, - DevelopmentReloadingConverter.class -}) +@Component +@Scope("prototype") public class Application extends BaseCommand implements Callable<Integer> { private static Boolean interactiveShell = false; + private static final List<Class<? extends CodeGenCommand>> CODE_GEN_COMMANDS = List.of( Review Comment: This replaces the previous `getBeanDefinitions(CodeGenCommand)` discovery with a static list, so a new code-gen command now has to be added here as well as written. Reasonable trade for a plain Spring context, since these commands take `CodeGenConfig` in their constructor and are not beans, but worth a one-line comment saying this list is the registration point. Otherwise the natural instinct is to annotate the new command and wonder why it never appears. ########## gradle/forge-test-config.gradle: ########## @@ -20,23 +20,37 @@ // Add JUnit Platform launcher dependency required by Gradle 9 for running tests // Also add ByteBuddy for Spock mocking on Java 17+ (CGLIB doesn't support Java 17 class files) dependencies { - testRuntimeOnly 'org.junit.platform:junit-platform-launcher:1.12.2' + testRuntimeOnly 'org.junit.platform:junit-platform-launcher' testImplementation "net.bytebuddy:byte-buddy" } -tasks.withType(Test).configureEach { +// Skipped Test tasks still execute their dependsOn. Only wire the local-Maven +// publish chain when Forge tests will actually run. +boolean forgeTestsEnabled = !['skipTests', 'skipForgeTests'].any { project.hasProperty(it) } && + (project.hasProperty('onlyForgeTests') || + !['onlyCoreTests', + 'onlyFunctionalTests', + 'onlyHibernate5Tests', + 'onlyHibernate7Tests', + 'onlyMongodbTests', + 'onlyNeo4jTests', + 'onlyRedisTests', + 'onlySpringSecurityTests'].any { project.hasProperty(it) }) + +tasks.withType(Test).configureEach { Test testTask -> + if (forgeTestsEnabled) { + testTask.dependsOn( Review Comment: This wires every forge `Test` task, including `grails-forge-core`, `grails-forge-api` and `grails-forge-web`, to publishing all root and grails-gradle publications into `build/local-maven` first. Only the specs that spawn a generated application's Gradle build need that repository: `grails-forge-cli` (`CommandSpec#executeGradleCommand`) and `grails-forge-test-core`. The `GRAILS_REPO_URL` specs in `grails-forge-core` only exercise URL parsing and never resolve anything. With the fold-in, a plain `./gradlew :grails-forge-core:test` now runs the full framework publish chain before a unit test can start, which the nested build did not do. Suggest scoping the `dependsOn` (and the `GRAILS_REPO_URL` environment) to the two modules that consume the repository, for example via a project `ext` flag those two build files set before applying this script. ########## grails-forge/grails-forge-core/build.gradle: ########## @@ -42,48 +47,38 @@ sourceSets { } dependencies { - annotationProcessor platform("io.micronaut.platform:micronaut-platform:$micronautVersion") - implementation platform("io.micronaut.platform:micronaut-platform:$micronautVersion") - annotationProcessor 'io.micronaut:micronaut-inject-java' - testAnnotationProcessor platform("io.micronaut.platform:micronaut-platform:$micronautVersion") - testAnnotationProcessor 'io.micronaut:micronaut-inject-java' + implementation platform(project(':grails-bom')) - api platform("io.micronaut.platform:micronaut-platform:$micronautVersion") // TODO: Should this be api? + api 'org.springframework:spring-context' + api 'jakarta.annotation:jakarta.annotation-api' + api 'jakarta.inject:jakarta.inject-api' api 'com.fasterxml.jackson.core:jackson-databind' api 'jakarta.validation:jakarta.validation-api' api "com.fizzed:rocker-runtime:$rockerVersion" api "io.github.java-diff-utils:java-diff-utils:$javaDiffUtils" - api 'io.micronaut:micronaut-http' - api 'io.micronaut:micronaut-http-client' - api 'io.micronaut:micronaut-inject' implementation "com.typesafe:config:$typesafeConfigVersion" implementation "org.apache.commons:commons-compress:$commonsCompressVersion" implementation "org.yaml:snakeyaml" - - compileOnly 'com.google.code.findbugs:jsr305' + implementation 'org.slf4j:slf4j-api' testImplementation 'org.apache.groovy:groovy-yaml' - - testCompileOnly 'io.micronaut:micronaut-inject-groovy' - testImplementation 'org.apache.groovy:groovy' testImplementation "org.spockframework:spock-core", { exclude group: 'org.apache.groovy', module: 'groovy-all' } testImplementation "ch.qos.logback:logback-classic" - testRuntimeOnly "org.objenesis:objenesis:$objenesisVersion" + testRuntimeOnly 'org.objenesis:objenesis' testImplementation 'org.apache.groovy:groovy-test' } def grailsVersionsPath = layout.buildDirectory.dir('version-info') def grailsVersionInfoTask = tasks.register('grailsVersionInfo', WriteGrailsVersionInfoTask) Review Comment: Anchoring here because the task class itself moved unchanged from the forge `buildSrc` (a pure rename, so GitHub offers no lines in it). Now that `WriteGrailsVersionInfoTask` lives in shared `build-logic`: at line 88 of the task the `GradleException` is constructed without `throw`, so a parse failure falls through with `pom == null` and line 93 fails with a `NullPointerException` instead of the intended message. Add `throw`. ########## grails-forge/README.md: ########## @@ -43,15 +43,13 @@ The user interface is [written in React](https://github.com/apache/grails-forge- ## API -API documentation for the production instance can be found at: Review Comment: I think this is important to preserve. We could take @codeconsole 's PR to auto generate this documentation like it was before. ########## grails-forge/README.md: ########## @@ -43,15 +43,13 @@ The user interface is [written in React](https://github.com/apache/grails-forge- ## API -API documentation for the production instance can be found at: +API usage for the production instance is available at: -* [Swagger / OpenAPI Doc](https://latest.grails.org/swagger-ui/index.html) -* [RAPI Doc](https://latest.grails.org/rapidoc/index.html) +* [Production API](https://latest.grails.org/) Review Comment: With the Micronaut OpenAPI endpoints gone these now point at the plain-text root, which lists the paths but not parameters or response shapes. Is a replacement API description planned (a checked-in OpenAPI document or springdoc), or is the plain-text response the API documentation going forward? Worth stating in the PR either way, since the start.grails.org UI and external tooling consume this API. -- 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]
