jdaugherty commented on code in PR #15956: URL: https://github.com/apache/grails-core/pull/15956#discussion_r4094597132
########## grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/UrlMappingsIndexProperties.java: ########## @@ -0,0 +1,98 @@ +/* + * 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.web.mapping; + +import java.io.IOException; +import java.io.InputStream; +import java.util.Collections; +import java.util.Properties; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; + +/** + * Descriptor for a future build-time URL mappings index. + * + * @since 8.0.x + */ +public final class UrlMappingsIndexProperties { + + public static final String LOCATION = "META-INF/grails/url-mappings-index.properties"; + + private static final Log LOG = LogFactory.getLog(UrlMappingsIndexProperties.class); + private static final UrlMappingsIndexProperties EMPTY = new UrlMappingsIndexProperties(false, new Properties()); + + private final boolean present; + private final Properties properties; + + private UrlMappingsIndexProperties(boolean present, Properties properties) { + this.present = present; + this.properties = properties; + } + + public static UrlMappingsIndexProperties load(ClassLoader classLoader) { + ClassLoader threadContextClassLoader; + try { + threadContextClassLoader = Thread.currentThread().getContextClassLoader(); + } + catch (RuntimeException e) { + threadContextClassLoader = null; + } + for (ClassLoader loader : new ClassLoader[] {threadContextClassLoader, classLoader}) { + if (loader == null) { + continue; + } + try (InputStream inputStream = loader.getResourceAsStream(LOCATION)) { + if (inputStream == null) { + continue; + } + Properties properties = new Properties(); + properties.load(inputStream); + return new UrlMappingsIndexProperties(true, properties); + } + catch (IOException | RuntimeException e) { Review Comment: Two things on this catch: - `Properties.load` signals bad input with `IOException` or `IllegalArgumentException`. Catching every `RuntimeException` also turns real bugs (an NPE, say) into a debug-level soft miss. - The per-loader isolation this enables isn't tested. In the malformed and unreadable specs the failing loader is always the *last* one tried, so changing this block to `return EMPTY;` leaves all seven specs passing. A spec with a throwing TCCL and a valid fallback loader would pin it down. I tried that locally with a TCCL whose `getResourceAsStream` throws only for `LOCATION` and delegates everything else, and it finds the fallback copy on this branch. (Throwing for every resource breaks slf4j-simple, which loads `simplelogger.properties` through the TCCL.) ########## grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/UrlMappingsIndexProperties.java: ########## @@ -0,0 +1,98 @@ +/* + * 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.web.mapping; + +import java.io.IOException; +import java.io.InputStream; +import java.util.Collections; +import java.util.Properties; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; + +/** + * Descriptor for a future build-time URL mappings index. + * + * @since 8.0.x + */ +public final class UrlMappingsIndexProperties { + + public static final String LOCATION = "META-INF/grails/url-mappings-index.properties"; Review Comment: `getResourceAsStream` returns the first match on the classpath, so a single well-known location only works while exactly one jar provides it. URL mappings come from the application *and* from every plugin (`UrlMappingsHolderFactoryBean` merges each plugin's `UrlMappings` with its `pluginIndex`), and a build-time generator would naturally emit one index per compiled jar. At that point classpath order silently decides which index wins. `grails-plugin.xml` handles this by enumerating every copy with `getResources` (`PluginUtils`). #16000 handles it by resolving relative to the application's code-source root (`IOUtils.findRootResource`). Either one, chosen with the generator in mind, would fit better than a single classpath-wide lookup. The format is worth deciding at the same time. `Properties.load(InputStream)` reads ISO-8859-1 into a flat string map, which is an awkward fit for a trie or reverse-routing table, and non-Latin-1 URL segments would need `\u` escaping. ########## grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingsHolder.java: ########## @@ -233,6 +238,10 @@ public List getExcludePatterns() { return excludePatterns; } + public UrlMappingsIndexProperties getPrecomputedIndexProperties() { Review Comment: This getter and the public `UrlMappingsIndexProperties` class become API we have to keep once released, and nothing outside the spec calls them. #16000 keeps its reader package-private. That works here too, since the spec is already in `org.grails.web.mapping`. ########## grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/UrlMappingsIndexProperties.java: ########## @@ -0,0 +1,98 @@ +/* + * 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.web.mapping; + +import java.io.IOException; +import java.io.InputStream; +import java.util.Collections; +import java.util.Properties; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; + +/** + * Descriptor for a future build-time URL mappings index. + * + * @since 8.0.x + */ +public final class UrlMappingsIndexProperties { + + public static final String LOCATION = "META-INF/grails/url-mappings-index.properties"; + + private static final Log LOG = LogFactory.getLog(UrlMappingsIndexProperties.class); + private static final UrlMappingsIndexProperties EMPTY = new UrlMappingsIndexProperties(false, new Properties()); + + private final boolean present; + private final Properties properties; + + private UrlMappingsIndexProperties(boolean present, Properties properties) { + this.present = present; + this.properties = properties; + } + + public static UrlMappingsIndexProperties load(ClassLoader classLoader) { + ClassLoader threadContextClassLoader; + try { + threadContextClassLoader = Thread.currentThread().getContextClassLoader(); + } + catch (RuntimeException e) { + threadContextClassLoader = null; + } + for (ClassLoader loader : new ClassLoader[] {threadContextClassLoader, classLoader}) { Review Comment: With the TCCL tried first, `load(classLoader)` ignores its argument whenever the TCCL can see a descriptor, so a caller that passes an explicit loader gets the TCCL's copy instead. If `DefaultUrlMappingsHolder` needs TCCL-first, resolving it at the call site keeps the parameter meaningful. For example, `ClassUtils.getDefaultClassLoader()` tries the TCCL and then the class's loader. Falling through after a failure has a similar problem. If the TCCL's copy is malformed, the second loader usually finds the same resource again through parent delegation, or picks up a different jar's copy. Neither seems like what we'd want once something actually reads the index. The `RuntimeException` catch around `getContextClassLoader()` above can't be reached. That call only throws under a SecurityManager, which is off by default on our JDK 21 baseline and permanently disabled from JDK 24. I'd drop it. ########## grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingsHolder.java: ########## @@ -113,6 +114,7 @@ public DefaultUrlMappingsHolder(List<UrlMapping> mappings, List excludePatterns) public DefaultUrlMappingsHolder(List<UrlMapping> mappings, List excludePatterns, boolean doNotCallInit) { urlMappings = mappings; this.excludePatterns = excludePatterns; + this.precomputedIndexProperties = UrlMappingsIndexProperties.load(DefaultUrlMappingsHolder.class.getClassLoader()); Review Comment: This does two classpath lookups on every `new DefaultUrlMappingsHolder(...)`: the factory bean, `UrlMappingsFactory`, `GspAutoConfiguration`, and unit tests all construct one. It runs even when `doNotCallInit` is true, and today the result only feeds a debug log. Since the goal is lower startup cost, I'd leave the holder alone until something consumes the index. Then load it once (e.g. in `UrlMappingsHolderFactoryBean`) rather than per instance. ########## grails-web-url-mappings/src/test/resources/simplelogger.properties: ########## @@ -17,4 +17,9 @@ # under the License. # -org.slf4j.simpleLogger.defaultLogLevel=info \ No newline at end of file +org.slf4j.simpleLogger.defaultLogLevel=info + +# Enabled at DEBUG so UrlMappingsIndexPropertiesSpec/DefaultUrlMappingsHolderSpec can +# exercise the (non-behavioral) debug-log lines guarded by LOG.isDebugEnabled(). +org.slf4j.simpleLogger.log.org.grails.web.mapping.UrlMappingsIndexProperties=debug +org.slf4j.simpleLogger.log.org.grails.web.mapping.DefaultUrlMappingsHolder=debug Review Comment: This DEBUG override applies to every spec in the module, not just the new ones, and `DefaultUrlMappingsHolder` logs once per mapping on every match. Running `:grails-web-url-mappings:test` on this branch produced 5,571 DEBUG lines from it: about 1 MB of the module's ~1.03 MB of captured test output. If the debug line needs verifying, I'd capture it inside the spec rather than raise the level module-wide. Also, the `DefaultUrlMappingsHolderSpec` named in the comment above doesn't exist. ########## grails-doc/src/en/guide/toc.yml: ########## @@ -37,6 +37,7 @@ gettingStarted: upgrading: title: Upgrading from the previous versions upgrading80x: Upgrading from Grails 7 to Grails 8 + urlMappingsPrecompute: URL mapping precomputation seed Review Comment: This sits alongside the version upgrade guides, but an upgrading user has nothing to act on: no descriptor is generated or read yet. "Seed" is also internal terminology. Advertising a reserved location now invites apps and plugins to put files there before the format is defined. I'd drop `urlMappingsPrecompute.adoc` until the generator exists. At that point it belongs in the feature docs with a What's New entry, not in the upgrade guide. ########## grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/UrlMappingsIndexPropertiesSpec.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 org.grails.web.mapping + +import java.io.ByteArrayInputStream +import java.io.InputStream + +import grails.web.mapping.UrlMapping +import spock.lang.Specification + +class UrlMappingsIndexPropertiesSpec extends Specification { + + void 'missing build-time URL mapping index keeps runtime fallback active'() { Review Comment: This passes no matter what the index code does. There's no descriptor on the test classpath and the mapping list is empty, so `matchAll('/books').length == 0` holds either way. The behavior the PR promises, that runtime matching stays authoritative *when a descriptor is present*, isn't tested. That case can be reached through the TCCL, the same way the precedence spec does it. Build a holder with a descriptor on the TCCL and a real `"/books"(controller: 'book', action: 'list')` mapping (e.g. via `AbstractUrlMappingsSpec`). Then assert `precomputedIndexProperties.present` and that `/books` still matches `book`/`list`. I tried this locally and it passes on this branch, so it only needs adding. -- 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]
