sbglasius commented on code in PR #16586: URL: https://github.com/apache/grails-core/pull/16586#discussion_r4257144748
########## grails-core/src/main/groovy/org/grails/core/DefaultGrailsControllerClass.groovy: ########## @@ -0,0 +1,230 @@ +/* + * 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.core + +import java.lang.invoke.MethodHandle +import java.lang.invoke.MethodHandles +import java.lang.invoke.MethodType +import java.lang.reflect.Method +import java.lang.reflect.Modifier + +import groovy.transform.CompileStatic +import groovy.transform.stc.POJO +import org.springframework.util.ReflectionUtils + +import grails.config.Settings +import grails.core.GrailsApplication +import grails.core.GrailsControllerClass +import grails.util.Environment +import grails.web.Action +import grails.web.UrlConverter + +/** + * Evaluates the conventions contained within controllers to perform auto-configuration. + * + * @author Graeme Rocher + * @author Steven Devijver + * @since 0.1 + */ +@SuppressWarnings('rawtypes') +@CompileStatic +@POJO +class DefaultGrailsControllerClass extends AbstractInjectableGrailsClass implements GrailsControllerClass { + + public static final String CONTROLLER = 'Controller' + + private static final String DEFAULT_CLOSURE_PROPERTY = 'defaultAction' + public static final String ALLOWED_HTTP_METHODS_PROPERTY = 'allowedMethods' + public static final Object[] EMPTY_ARGS = new Object[0] + public static final String SCOPE = 'scope' + public static final String SCOPE_SINGLETON = 'singleton' + private String scope + private Map<String, ActionInvoker> actions = new HashMap<>() + private String defaultActionName + private String namespace + protected Map<String, String> actionUriToViewName = new HashMap<>() + + DefaultGrailsControllerClass(Class<?> clazz) { + super(clazz, CONTROLLER) + namespace = getStaticPropertyValue(NAMESPACE_PROPERTY, String) + defaultActionName = getStaticPropertyValue(DEFAULT_CLOSURE_PROPERTY, String) + if (defaultActionName == null) { + defaultActionName = INDEX_ACTION + } + methodStrategy(actions) + this.scope = getStaticPropertyValue(SCOPE, String) + } + + void initialize() { + // no-op + } + + @Override + void setGrailsApplication(GrailsApplication grailsApplication) { + this.@grailsApplication = grailsApplication + if (this.scope == null) { + this.scope = grailsApplication.getConfig().getProperty(Settings.CONTROLLERS_DEFAULT_SCOPE, SCOPE_SINGLETON) + } + } + + @Override + Set<String> getActions() { + return actions.keySet() + } + + String getNamespace() { + return namespace + } + + @Override + String getScope() { + return scope + } + + @Override + boolean isSingleton() { + return SCOPE_SINGLETON.equalsIgnoreCase(getScope()) + } + + @Override + String getDefaultAction() { + return this.defaultActionName + } + + private void methodStrategy(Map<String, ActionInvoker> methodNames) { + + Class superClass = getClazz() + MethodHandles.Lookup lookup = MethodHandles.lookup() + while (superClass != Object && superClass != GroovyObject) { + for (Method method in superClass.getMethods()) { + if (Modifier.isPublic(method.getModifiers()) && method.getAnnotation(Action) != null) { + String methodName = method.getName() + if (Environment.isDevelopmentMode()) { + methodNames.put(methodName, new ReflectionInvoker(method)) + } + else { + MethodHandle mh + try { + mh = lookup.findVirtual(superClass, methodName, MethodType.methodType(method.getReturnType())) + methodNames.put(methodName, new MethodHandleInvoker(mh)) + } catch (NoSuchMethodException | IllegalAccessException e) { + methodNames.put(methodName, new ReflectionInvoker(method)) + } + } + + } + } + superClass = superClass.getSuperclass() + } + + if (!isActionMethod(defaultActionName) && methodNames.size() == 1 && !isReadableProperty('scaffold')) { + defaultActionName = methodNames.keySet().iterator().next() + } + } + + @Override + boolean mapsToURI(String uri) { + if (uri.startsWith('/')) { + String[] tokens = uri.substring(1).split('\\/') + if (tokens.length > 0) { + String controllerName = tokens[0] + if (getLogicalPropertyName().equals(controllerName)) { + if (tokens.length > 1) { + String actionName = tokens[1] + if (actions.containsKey(actionName) || defaultActionName.equals(actionName)) { + return true + } + } + else { + return true + } + } + } + } + return false + } + + /** + * Register a new {@link grails.web.UrlConverter} with the controller + * + * @param urlConverter The {@link grails.web.UrlConverter} to register + */ + @Override + void registerUrlConverter(UrlConverter urlConverter) { + for (String actionName in new ArrayList<>(actions.keySet())) { + actionUriToViewName.put(urlConverter.toUrlElement(actionName), actionName) + actions.put(urlConverter.toUrlElement(actionName), actions.remove(actionName)) + } + defaultActionName = urlConverter.toUrlElement(defaultActionName) + } + + /** + * Invokes the controller action for the given name on the given controller instance + * + * @param controller The controller instance + * @param action The action name + * @return The result of the action + * @throws Throwable + */ + @Override + Object invoke(Object controller, String action) throws Throwable { + if (action == null) action = this.defaultActionName + ActionInvoker handle = actions.get(action) + if (handle == null) throw new IllegalArgumentException('Invalid action name: ' + action) + return handle.invoke(controller) + } + + String actionUriToViewName(String actionUri) { + String actionName = actionUriToViewName.get(actionUri) + + return actionName != null ? actionName : actionUri + } + + private interface ActionInvoker { + Object invoke(Object controller) throws Throwable + } + + private class ReflectionInvoker implements ActionInvoker { + private final Method method + + ReflectionInvoker(Method method) { + this.method = method + ReflectionUtils.makeAccessible(method) + } + + @Override + Object invoke(Object controller) throws Throwable { + return method.invoke(controller) + } + } + + private class MethodHandleInvoker implements ActionInvoker { + private final MethodHandle handle + + MethodHandleInvoker(MethodHandle handle) { + this.handle = handle + } + + @Override + Object invoke(Object controller) throws Throwable { + return handle.invokeWithArguments(controller) Review Comment: **Efficiency:** `handle.invoke(controller)` became `handle.invokeWithArguments(controller)`. That is the generic varargs path: it allocates an array, boxes, and adapts the type on every call. In any non-development environment this runs on every controller action dispatch, so it undoes much of the gain MethodHandles had over the `ReflectionInvoker`. A cheaper option is to adapt the handle once (e.g. `mh.asType(MethodType.methodType(Object, Object))`) and use a typed invoker or `LambdaMetafactory`, or keep this invoker in Java. ########## grails-core/src/main/groovy/grails/util/Pair.groovy: ########## @@ -17,60 +17,65 @@ * under the License. */ -package grails.util; +package grails.util -public class Pair<A, B> { - final A aValue; - final B bValue; +import groovy.transform.CompileStatic - public Pair(A aValue, B bValue) { - this.aValue = aValue; - this.bValue = bValue; +@CompileStatic +class Pair<A, B> { + + final A aValue Review Comment: **API compatibility:** The Java package-private fields `aValue` / `bValue` (same in `Triple`) are now Groovy properties. The fields become private, and Groovy generates public `getAValue()` / `getBValue()` next to the hand-written `getaValue()` / `getbValue()`. `@PackageScope final A aValue`, as used in `InstanceFactoryBean`, would keep the original shape. ########## grails-core/src/test/groovy/org/grails/spring/context/annotation/ClosureClassIgnoringComponentScanBeanDefinitionParserSpec.groovy: ########## @@ -0,0 +1,103 @@ +/* + * 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.spring.context.annotation + +import java.lang.reflect.Field + +import org.springframework.beans.factory.support.DefaultListableBeanFactory +import org.springframework.beans.factory.xml.XmlBeanDefinitionReader +import org.springframework.beans.factory.xml.XmlReaderContext +import org.springframework.context.annotation.ClassPathBeanDefinitionScanner +import org.springframework.context.annotation.ClassPathScanningCandidateComponentProvider +import org.springframework.core.io.ByteArrayResource +import org.springframework.core.type.filter.TypeFilter +import spock.lang.Specification + +import grails.plugins.GrailsPluginManager + +/** + * {@link #configureScanner} and its resource-pattern $-exclusion logic are exercised + * end-to-end, using real Spring collaborators, via {@link GrailsComponentScanPostProcessorSpec}, + * whose {@code resourcePatternResolver()} builds the identical anonymous {@code AntPathMatcher}. + * Real {@code ParserContext}/{@code Element} XML-parsing collaborators are heavy to construct + * here without also driving a full namespace-handler parse, so this spec focuses on + * {@link #createScanner}, the plugin-manager type-filter wiring specific to this class. + */ +class ClosureClassIgnoringComponentScanBeanDefinitionParserSpec extends Specification { + + ClosureClassIgnoringComponentScanBeanDefinitionParser parser = new ClosureClassIgnoringComponentScanBeanDefinitionParser() + + private static XmlReaderContext readerContextFor(DefaultListableBeanFactory registry) { + new XmlBeanDefinitionReader(registry).createReaderContext(new ByteArrayResource(new byte[0])) + } + + private static List<TypeFilter> includeFiltersOf(ClassPathBeanDefinitionScanner scanner) { + Field field = ClassPathScanningCandidateComponentProvider.getDeclaredField('includeFilters') Review Comment: **Conventions:** This reads Spring's private `includeFilters` field by reflection, and `GrailsContextNamespaceHandlerSpec:40` does the same with `NamespaceHandlerSupport.parsers`. The spec also calls the protected `createScanner` directly. CLAUDE.md rule 9 says: "Test via public APIs - ... never invoke internal implementations, package-private methods, or bypass the public surface directly". These tests will also break if Spring renames its private fields. ########## grails-core/src/main/groovy/org/grails/config/CompositeConfig.groovy: ########## @@ -0,0 +1,269 @@ +/* + * 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.config + +import groovy.transform.CompileStatic +import groovy.transform.stc.POJO + +import grails.config.Config +import grails.util.GrailsStringUtils +import org.grails.core.exceptions.GrailsConfigurationException + +/** + * A {@link Config} composed of other Configs + * + * @author Graeme Rocher + * @since 3.0 + */ +@CompileStatic +@POJO +class CompositeConfig implements Config { + + protected Deque<Config> configs = new ArrayDeque<>() + + /** + * Adds a config at the highest level of precedence + * + * @param config + */ + void addFirst(Config config) { + configs.addFirst(config) + } + + /** + * Adds a config at the lowest level of precedence + * + * @param config + */ + void addLast(Config config) { + configs.addLast(config) + } + + @Override + @Deprecated + Map<String, Object> flatten() { + Map<String, Object> flattened = new LinkedHashMap<>() + for (Config c in configs) { + flattened.putAll(c.flatten()) + } + return flattened + } + + @Override + Properties toProperties() { + Properties properties = new Properties() + for (Config c in configs) { + properties.putAll(c.toProperties()) + } + return properties + } + + @Override + Config merge(Map<String, Object> toMerge) { + throw new UnsupportedOperationException('Config cannot be modified') + } + + @Override + def <T> T getProperty(String key, Class<T> targetType, T defaultValue, List<T> allowedValues) { + T v = getProperty(key, targetType, defaultValue) + if (!allowedValues.contains(v)) { + throw new GrailsConfigurationException('Invalid configuration value [\$value] for key [\${key}]. Possible values \$allowedValues') Review Comment: **Correctness:** These messages are single-quoted with escaped `\$value` / `\${key}` / `\$allowedValues`, so they print the placeholders literally. There is also no `value` variable here; the local is `v`. The same issue is at lines 240 and 249 and at `NavigableMapConfig.groovy:207`. Now that this is Groovy, a GString such as `"Invalid configuration value [${v}] for key [${key}]. Possible values ${allowedValues}"` would fix it. ########## grails-core/src/main/groovy/org/grails/plugins/AbstractGrailsPlugin.groovy: ########## @@ -0,0 +1,260 @@ +/* + * 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.plugins + +import groovy.transform.CompileStatic +import groovy.transform.stc.POJO +import org.slf4j.Logger +import org.slf4j.LoggerFactory +import org.springframework.beans.BeansException +import org.springframework.context.ApplicationContext +import org.springframework.core.env.ConfigurableEnvironment +import org.springframework.core.env.PropertySource +import org.springframework.util.Assert + +import grails.config.Config +import grails.core.GrailsApplication +import grails.plugins.GrailsPlugin +import grails.plugins.GrailsPluginManager +import grails.util.GrailsNameUtils +import org.apache.grails.core.plugins.PluginUtils +import org.grails.core.AbstractGrailsClass +import org.grails.plugins.support.WatchPattern + +/** + * Abstract implementation that provides some default behaviours + * + * @author Graeme Rocher + */ +@CompileStatic +abstract class AbstractGrailsPlugin extends GroovyObjectSupport implements GrailsPlugin { + + private static final Logger LOG = LoggerFactory.getLogger(AbstractGrailsPlugin) + + public static final String PLUGIN_YML = PluginUtils.PLUGIN_YML_CONFIG + public static final String PLUGIN_YML_PATH = PluginUtils.PLUGIN_YML_CONFIG_PATH + public static final String PLUGIN_GROOVY = PluginUtils.PLUGIN_GROOVY_CONFIG + public static final String PLUGIN_GROOVY_PATH = PluginUtils.PLUGIN_GROOVY_CONFIG_PATH + + protected GrailsApplication grailsApplication + protected boolean isBase = false + protected String version = '1.0' + protected Map<String, Object> dependencies = new HashMap<>() + protected String[] dependencyNames = [] as String[] + protected Class<?> pluginClass + protected ApplicationContext applicationContext + protected GrailsPluginManager manager + protected String[] evictionList = [] as String[] + protected Config config + + /** + * Wrapper Grails class for plugins. + * + * @author Graeme Rocher + */ + @POJO + @CompileStatic + static class GrailsPluginClass extends AbstractGrailsClass { Review Comment: **API compatibility:** `GrailsPluginClass` was a public non-static inner class in Java and is now `static`, which changes its constructor signature and binary shape. Precompiled code that creates it through an outer instance would break. Was this intended? If not, it should keep its previous shape, in line with 80c8329, which aimed to keep the Java-facing API. ########## grails-core/src/test/groovy/grails/util/HoldersSpec.groovy: ########## @@ -0,0 +1,130 @@ +/* + * 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.util + +import grails.config.Config +import grails.core.GrailsApplication +import grails.plugins.GrailsPluginManager +import org.grails.core.support.GrailsApplicationDiscoveryStrategy +import spock.lang.Specification + +class HoldersSpec extends Specification { + + void cleanup() { + // clear(), unlike reset(), guards its servlet-context write and is therefore safe to call + // outside a servlet environment (grails-core has no grails.web.context.WebRequestServletHolder + // on its classpath, so Holders.servletContexts is never populated here) + Holders.clear() Review Comment: **Test isolation:** `Holders.clear()` also empties the static `applicationDiscoveryStrategies` list, which is loaded once from `GrailsFactoriesLoader` when the class initialises, and nothing restores it. Forks are reused across specs, so any spec that runs after this one in the same JVM loses every factory-registered strategy and can fail depending on test order. Consider saving and restoring the strategies, or resetting only the holders this spec sets. ########## grails-core/src/test/groovy/org/grails/core/io/SimpleMapResourceLoaderSpec.groovy: ########## @@ -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.core.io + +import spock.lang.Specification + +import org.springframework.core.io.ByteArrayResource + +class SimpleMapResourceLoaderSpec extends Specification { + + void 'getResource returns a resource registered in the backing map'() { + given: + def loader = new SimpleMapResourceLoader() + def resource = new ByteArrayResource('content'.bytes) + + when: + loader.getResources().put('/foo.txt', resource) + + then: + loader.getResource('/foo.txt').is(resource) + loader.getResource('/missing.txt') == null + } + + void 'getClassLoader returns the class loader that loaded this class'() { + given: + def loader = new SimpleMapResourceLoader() + + expect: + loader.getClassLoader() == SimpleMapResourceLoader.classLoader Review Comment: **Test coverage:** This compares against the same expression the implementation returns (`SimpleMapResourceLoader.classLoader`), so the assertion can't fail. Comparing against an independent value such as `loader.getClass().getClassLoader()` would make it meaningful. ########## grails-core/src/main/groovy/grails/util/Holders.groovy: ########## @@ -0,0 +1,230 @@ +/* + * 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.util + +import java.lang.reflect.InvocationTargetException + +import groovy.transform.CompileStatic +import org.apache.commons.logging.Log +import org.apache.commons.logging.LogFactory +import org.springframework.context.ApplicationContext +import org.springframework.context.Lifecycle +import org.springframework.util.Assert + +import grails.config.Config +import grails.core.GrailsApplication +import grails.plugins.GrailsPluginManager +import org.grails.core.io.support.GrailsFactoriesLoader +import org.grails.core.support.GrailsApplicationDiscoveryStrategy + +/** + * Allows looking up key classes in a static context + * + * + * @author Burt Beckwith + * @author Graeme Rocher + * + * @since 2.0 + */ +@CompileStatic +class Holders { + + private static final Log LOG = LogFactory.getLog(Holders) + private static Holder<GrailsPluginManager> pluginManagers = new Holder<>('PluginManager') + private static Holder<Boolean> pluginManagersInCreation = new Holder<>('PluginManagers in creation') + private static Holder<Config> configs = new Holder<>('config') + private static Holder<Map<?, ?>> flatConfigs = new Holder<>('flat config') + + private static List<GrailsApplicationDiscoveryStrategy> applicationDiscoveryStrategies = GrailsFactoriesLoader.loadFactories(GrailsApplicationDiscoveryStrategy, Holders.getClassLoader()) + private static Holder servletContexts + + static { + + createServletContextsHolder() + } + + private static GrailsApplication applicationSingleton // TODO remove + + private Holders() { + // static only + } + + static void addApplicationDiscoveryStrategy(GrailsApplicationDiscoveryStrategy strategy) { + applicationDiscoveryStrategies.add(strategy) + } + + static void clear() { + pluginManagers.set(null) + pluginManagersInCreation.set(null) + configs.set(null) + flatConfigs.set(null) + if (servletContexts != null) { + servletContexts.set(null) + } + applicationDiscoveryStrategies.clear() + applicationSingleton = null + } + + static void setServletContext(final Object servletContext) { + servletContexts.set(servletContext) + } + + static Object getServletContext() { + return get(servletContexts, 'servletContext') + } + + static ApplicationContext getApplicationContext() { + for (GrailsApplicationDiscoveryStrategy strategy in applicationDiscoveryStrategies) { + ApplicationContext applicationContext = strategy.findApplicationContext() + if (applicationContext != null) { + boolean running = ((Lifecycle) applicationContext).isRunning() + if (running) { + return applicationContext + } + } + } + throw new IllegalStateException('Could not find ApplicationContext, configure Grails correctly first') + } + + /** + * + * @return The ApplicationContext or null if it doesn't exist + */ + static ApplicationContext findApplicationContext() { + for (GrailsApplicationDiscoveryStrategy strategy in applicationDiscoveryStrategies) { + ApplicationContext applicationContext = strategy.findApplicationContext() + if (applicationContext != null) { + return applicationContext + } + } + return null + } + + /** + * + * @return The ApplicationContext or null if it doesn't exist + */ + static GrailsApplication findApplication() { + for (GrailsApplicationDiscoveryStrategy strategy in applicationDiscoveryStrategies) { + GrailsApplication grailsApplication = strategy.findGrailsApplication() + if (grailsApplication != null) { + return grailsApplication + } + } + return applicationSingleton + } + + static GrailsApplication getGrailsApplication() { + GrailsApplication grailsApplication = findApplication() + Assert.notNull(grailsApplication, 'GrailsApplication not found') + return grailsApplication + } + + static void setGrailsApplication(GrailsApplication application) { + applicationSingleton = application + } + + static void setConfig(Config config) { + configs.set(config) + + // reset flat config + flatConfigs.set(config == null ? null : config) + } + + static Config getConfig() { + return get(configs, 'config') + } + + static Map<?, ?> getFlatConfig() { + Map<?, ?> flatConfig = get(flatConfigs, 'flatConfig') + return flatConfig == null ? Collections.emptyMap() : flatConfig + } + + static void setPluginManagerInCreation(boolean inCreation) { + pluginManagersInCreation.set(inCreation) + } + + static void setPluginManager(GrailsPluginManager pluginManager) { + if (pluginManager != null) { + pluginManagersInCreation.set(false) + } + pluginManagers.set(pluginManager) + } + + static GrailsPluginManager getPluginManager() { + return getPluginManager(false) + } + + static GrailsPluginManager getPluginManager(boolean mappedOnly) { + while (true) { + Boolean inCreation = get(pluginManagersInCreation, 'PluginManager in creation', mappedOnly) + if (inCreation == null) { + inCreation = false + } + if (!inCreation) { + break + } + + try { + Thread.sleep(100) + } + catch (InterruptedException e) { + break + } + } + + return get(pluginManagers, 'PluginManager', mappedOnly) + } + + static GrailsPluginManager currentPluginManager() { + GrailsPluginManager current = getPluginManager() + Assert.notNull(current, 'No PluginManager set') + return current + } + + static void reset() { + setPluginManager(null) + setGrailsApplication(null) + setServletContext(null) Review Comment: **Correctness:** `reset()` calls `setServletContext(null)` without a guard, so it throws a NullPointerException whenever `servletContexts` was never created, i.e. outside a servlet environment. It throws after the plugin manager and application are cleared but before `setPluginManagerInCreation(false)` and `setConfig(null)` run, so config is left behind. `clear()` already guards with `servletContexts != null`; `reset()` should do the same. The new `HoldersSpec` asserts the NPE instead of fixing it. -- 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]
