codeconsole commented on code in PR #15949: URL: https://github.com/apache/grails-core/pull/15949#discussion_r3557222309
########## grails-test-examples/external-configuration/src/integration-test/groovy/test/app/RelaxedPropertyResolutionSpec.groovy: ########## @@ -0,0 +1,56 @@ +/* + * 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 test.app + +import grails.testing.mixin.integration.Integration +import org.springframework.beans.factory.annotation.Autowired +import org.springframework.boot.actuate.endpoint.Access +import org.springframework.boot.convert.ApplicationConversionService +import org.springframework.core.env.ConfigurableEnvironment +import spock.lang.Specification + +/** + * Regression tests for issue #15818. Actuator endpoint access is resolved directly through + * {@code environment.getProperty(name, Access)} rather than relaxed configuration-property + * binding, so the environment itself must be configured with the + * {@link ApplicationConversionService} for lenient values such as {@code unrestricted} or + * {@code read-only} to convert. Before the fix the application failed to start with the + * {@code management.endpoint.*.access} values declared in {@code application.yml}. + */ +@Integration +class RelaxedPropertyResolutionSpec extends Specification { + + @Autowired + ConfigurableEnvironment springEnvironment + + void 'the environment uses the ApplicationConversionService'() { + expect: 'the conversion service installed by Spring Boot is present' + springEnvironment.conversionService instanceof ApplicationConversionService Review Comment: Observation: the two enum-resolution features below already prove everything this feature asserts, so this one is technically redundant — but it's useful as a first-failure diagnostic that distinguishes "conversion service missing" from "converter behavior changed", and it documents intent. No change needed. ########## grails-core/src/main/groovy/grails/boot/GrailsApp.groovy: ########## @@ -133,7 +133,9 @@ class GrailsApp extends SpringApplication { @Override protected void configureEnvironment(ConfigurableEnvironment environment, String[] args) { - configurePropertySources(environment, args) + // Delegating to super installs the ApplicationConversionService so relaxed property + // resolution (e.g. lowercase enum values) works via environment.getProperty() + super.configureEnvironment(environment, args) Review Comment: Two side effects of this delegation worth noting (neither blocking): 1. `GrailsApplicationPostProcessor.loadApplicationConfig()` mutates `environment.getConversionService()` via `addConverter(...)` and then propagates it into `PropertySourcesConfig`. That still works only because Boot 3.5 installs a **new mutable** `ApplicationConversionService` here rather than the unmodifiable `ApplicationConversionService.getSharedInstance()`. If a future Boot version switches to the shared instance, those `addConverter` calls will throw at startup — worth remembering during Boot upgrades. 2. Because that same conversion service flows into `PropertySourcesConfig`, typed access through `grailsApplication.config.getProperty(name, type)` also becomes lenient (durations, lowercase/hyphenated enums, delimited collections, formatters), not just `environment.getProperty()`. It's strictly in the direction of Spring Boot alignment, so I consider it a plus — but the behavior change is a bit broader than the PR description states. Maybe add a sentence to the description so it's discoverable later. ########## grails-core/src/test/groovy/grails/boot/GrailsAppEnvironmentConversionSpec.groovy: ########## @@ -0,0 +1,67 @@ +/* + * 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 grails.boot + +import grails.util.Environment +import org.springframework.boot.WebApplicationType +import org.springframework.boot.convert.ApplicationConversionService +import org.springframework.context.ConfigurableApplicationContext +import org.springframework.context.annotation.Configuration +import spock.lang.Specification +import spock.util.environment.RestoreSystemProperties + +/** + * Verifies the application environment created by {@link GrailsApp} supports the same + * relaxed property resolution as a plain Spring Boot application, so values resolved + * directly through {@code environment.getProperty(name, Enum)} accept lenient formats + * such as lowercase or hyphenated enum names (see issue #15818). + */ +@RestoreSystemProperties +class GrailsAppEnvironmentConversionSpec extends Specification { + + void "environment resolves relaxed enum property values"() { + setup: + System.setProperty(Environment.KEY, Environment.TEST.getName()) + System.setProperty('test.access.lowercase', 'unrestricted') + System.setProperty('test.access.hyphenated', 'read-only') + GrailsApp app = new GrailsApp(EnvironmentConversionTestConfiguration) + app.webApplicationType = WebApplicationType.NONE + + when: + ConfigurableApplicationContext context = app.run() + + then: + context.environment.conversionService instanceof ApplicationConversionService Review Comment: Nit: this asserts a Boot implementation detail on top of the behavioral assertions below it, but since the installed conversion service *is* the contract being restored (and it would still pass if Boot ever reverted to `getSharedInstance()`, which is also an `ApplicationConversionService`), it works well as a diagnostic — when this line fails you know the service was never installed, vs. a converter behavior change. Fine as is. -- 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]
