Skip to content

Seed URL mappings index with runtime fallback - #15956

Open
jamesfredley wants to merge 7 commits into
8.0.xfrom
feat/url-mapping-precompute-seed
Open

Seed URL mappings index with runtime fallback#15956
jamesfredley wants to merge 7 commits into
8.0.xfrom
feat/url-mapping-precompute-seed

Conversation

@jamesfredley

@jamesfredley jamesfredley commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Description

What was found

Problem Impact
URL mappings are resolved at runtime with linear/holder matching Startup and request routing cost stays higher than needed
No build-time index SPI for mappings Future precomputation work has no safe landing path
Google Doc 4.2 listed this as 8.1+ architecture Needs a starter that preserves runtime fallback

What changed

Area Change
Index properties Add UrlMappingsIndexProperties for optional META-INF/grails/url-mappings-index.properties
Runtime holder DefaultUrlMappingsHolder only detects the descriptor (debug log); runtime mapping evaluation stays authoritative whether or not a descriptor is present
Fallback Missing/malformed/unreadable descriptor is a non-fatal soft miss; existing runtime mapping behavior is unchanged
Docs / tests Upgrade note + missing/malformed/unreadable/precedence/valid coverage

Review feedback addressed

Source Item Resolution
Copilot load() skipped the TCCL and did not wrap IllegalArgumentException like IOException Try the thread context classloader first, then the provided one; catch IOException/RuntimeException per loader so a throwing TCCL still falls through to the fallback loader and an optional/malformed descriptor never aborts startup (Error still propagates)
Review pass New .adoc missing ASF header; @since/docs said 8.1; wording implied conditional fallback Added ASF header, corrected to 8.0, and clarified that runtime evaluation stays authoritative regardless of descriptor presence

Out of scope / follow-up

Topic Status
Full trie/routing precomputation 8.1+
Reverse-routing table generation Follow-up
AST transform writing the index at compile time Follow-up

Related MD topics

Source Topic
Google Doc 4.2 URL mapping pre-computation
Status map Not covered / defer -> starter now

Contributor Checklist

Issue and Scope

  • Starter PR for future architecture; no acknowledged issue required for discussion.
  • Runtime behavior preserved without an index.
  • Single focused seed change.
  • Targets 8.0.x as an experimental starter for 8.1 work.

Code Quality

  • Tests cover the fallback path.
  • Focused module tests run locally.
  • No mass reformatting.
  • Generative AI starting point; labeled.

Licensing and Attribution

  • Apache License 2.0.
  • Contributor rights confirmed.
  • ai-generated-starting-point label applied.

Documentation

  • Upgrade note added.
  • PR description explains scope.

Assisted-by: Sisyphus:xai/grok-4.5 [gpt-coding]

Add UrlMappingsIndexProperties and load path with fallback to runtime UrlMappingsHolder behavior.

Assisted-by: Sisyphus:xai/grok-4.5 [gpt-coding]

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces an initial SPI “landing pad” for future build-time URL-mappings indexing by reserving a descriptor location and exposing its presence to the runtime URL mappings holder, while preserving the existing runtime URL-matching behavior. It also adds an upgrade-note doc page and a unit test validating the “no index present” fallback path.

Changes:

  • Added UrlMappingsIndexProperties to load optional META-INF/grails/url-mappings-index.properties from the classpath.
  • Wired DefaultUrlMappingsHolder to load/expose the descriptor and emit a debug message when it’s present.
  • Added upgrade documentation and a unit test for the missing-descriptor fallback behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/UrlMappingsIndexProperties.java New classpath descriptor loader for future URL mappings index metadata.
grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingsHolder.java Loads and exposes the descriptor; logs when present while keeping runtime routing behavior.
grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/UrlMappingsIndexPropertiesSpec.groovy Adds fallback-path coverage when the descriptor is absent.
grails-doc/src/en/guide/upgrading/urlMappingsPrecompute.adoc Documents the reserved descriptor location and current runtime behavior.
grails-doc/src/en/guide/toc.yml Adds the new upgrade note page to the guide TOC.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@bito-code-review

Copy link
Copy Markdown

The current implementation of UrlMappingsIndexProperties.load(ClassLoader) prioritizes the provided classloader over the thread context classloader (TCCL), which can prevent discovery of application-packaged resources. Additionally, Properties.load(InputStream) can throw IllegalArgumentException for malformed content, which is not currently handled.

To address these issues, it is recommended to:

  1. Update the load method to check the TCCL first, falling back to the provided classloader if necessary.
  2. Wrap both IOException and IllegalArgumentException in a consistent exception type (e.g., IllegalStateException) to ensure robust error handling.

grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/UrlMappingsIndexProperties.java

public static UrlMappingsIndexProperties load(ClassLoader classLoader) {
        ClassLoader loader = Thread.currentThread().getContextClassLoader();
        if (loader == null) {
            loader = classLoader;
        }
        if (loader == null) {
            return EMPTY;
        }
        try (InputStream inputStream = loader.getResourceAsStream(LOCATION)) {
            if (inputStream == null) {
                return EMPTY;
            }
            Properties properties = new Properties();
            properties.load(inputStream);
            return new UrlMappingsIndexProperties(true, properties);
        }
        catch (IOException | IllegalArgumentException e) {
            throw new IllegalStateException("Unable to load " + LOCATION, e);
        }
    }

@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.35294% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.5363%. Comparing base (b980413) to head (3034e47).
⚠️ Report is 258 commits behind head on 8.0.x.

Files with missing lines Patch % Lines
...grails/web/mapping/UrlMappingsIndexProperties.java 86.6667% 2 Missing and 2 partials ⚠️
...g/grails/web/mapping/DefaultUrlMappingsHolder.java 50.0000% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff                @@
##             8.0.x     #15956         +/-   ##
================================================
+ Coverage         0   51.5363%   +51.5363%     
- Complexity       0      17800      +17800     
================================================
  Files            0       2040       +2040     
  Lines            0      95620      +95620     
  Branches         0      16597      +16597     
================================================
+ Hits             0      49279      +49279     
- Misses           0      39029      +39029     
- Partials         0       7312       +7312     
Files with missing lines Coverage Δ
...g/grails/web/mapping/DefaultUrlMappingsHolder.java 72.8238% <50.0000%> (ø)
...grails/web/mapping/UrlMappingsIndexProperties.java 86.6667% <86.6667%> (ø)

... and 2038 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Address review feedback on UrlMappingsIndexProperties.load:

- Try the thread context classloader first, then the provided
  classloader (null-guarded), so an application-packaged descriptor is
  discovered.
- A malformed (IllegalArgumentException) or unreadable (IOException)
  descriptor is now logged at debug and treated as absent (EMPTY)
  instead of throwing IllegalStateException, so an optional descriptor
  can never abort application startup.

Assisted-by: Sisyphus:openai/gpt-5.6-terra [gpt-coding]
Follow-up to review feedback on the optional descriptor:

- Broaden load() error handling to treat any IOException or non-fatal
  RuntimeException (e.g. SecurityException) during classloader discovery
  or descriptor parsing as an absent descriptor, without swallowing Error,
  so it can never abort application startup.
- Add the ASF license header to the upgrading guide page, correct the
  version to 8.0 (matching the target branch and @SInCE), and clarify
  that runtime URL-mapping evaluation stays authoritative whether or not
  a descriptor is present.
- Add tests for an unreadable (throwing) classloader and a valid
  descriptor.

Assisted-by: Sisyphus:openai/gpt-5.6-terra [gpt-coding]
A throwing or restricted thread context classloader (e.g. a
SecurityException) must not prevent descriptor discovery through the
provided classloader. Acquire the TCCL defensively and catch load
failures per loader iteration, only returning EMPTY after every
classloader has been tried, so the optional descriptor stays non-fatal
while still resolving through the fallback loader.

Assisted-by: Sisyphus:openai/gpt-5.6-terra [gpt-coding]
@jamesfredley

Copy link
Copy Markdown
Contributor Author

Review feedback addressed

Merged the latest 8.0.x and pushed follow-up commits.

Copilot review comment (classloader / exception handling on UrlMappingsIndexProperties.load):

  • The loader now tries the thread context classloader first, then the provided classloader, so an application-packaged META-INF/grails/url-mappings-index.properties is discovered.
  • Failures are isolated per loader and treated as a soft miss: an IOException or any non-fatal RuntimeException (including the malformed-properties IllegalArgumentException, and a restricted/throwing TCCL) is logged at debug and skips that loader, so a broken TCCL still falls through to the provided classloader. EMPTY is only returned after every loader has been tried, and Error is never swallowed - so an optional/missing/malformed descriptor can never abort application startup.

Review-pass fixes: added the ASF license header to the new urlMappingsPrecompute.adoc, corrected the version references from 8.1 to 8.0 (matching the target branch and @since), and clarified the wording so it is explicit that runtime URL-mapping evaluation stays authoritative whether or not a descriptor is present (the descriptor is only reserved/detected in this seed). Added tests for missing, malformed, unreadable/throwing, TCCL-precedence, and valid-descriptor paths.

Local verification: :grails-web-url-mappings:test --tests UrlMappingsIndexPropertiesSpec (5 tests) passes.

@jamesfredley jamesfredley moved this to Todo in Apache Grails Jul 24, 2026
Codecov flagged 70.59% patch coverage (5 missing, 3 partial lines
across UrlMappingsIndexProperties.java and DefaultUrlMappingsHolder.java).
Verified jamesfredley's prior review-feedback fix (TCCL-first classloader
order, per-loader IOException|RuntimeException isolation) against the
current code first - it's correct as claimed.

Closed the reachable gaps:
- propertyNames() was never called by any test.
- The null-thread-context-classloader fallback path (loader == null ->
  continue to the provided classloader) was untested; a real JVM's TCCL
  is essentially never null in practice, but it's legal to set explicitly
  and is a real defensive branch worth locking in.
- The LOG.debug(...) line reporting a discovered descriptor never ran
  because the module's tests default to INFO. Added a per-logger DEBUG
  override in simplelogger.properties (scoped to just these two loggers)
  rather than a fragile system-property timing dependency, verified the
  full module suite still passes with it enabled.

Left two gaps deliberately uncovered, consistent with judgment calls made
elsewhere in this PR-review pass today: the TCCL-retrieval RuntimeException
catch (Thread.getContextClassLoader() only throws under a SecurityManager,
which is disallowed by default since JDK 18 on this JDK 21 baseline - a
test would need to force-enable a deprecated-for-removal mechanism to
reach genuinely dead code on this baseline), and
DefaultUrlMappingsHolder's own debug-log line (only reachable when a real
META-INF/grails/url-mappings-index.properties exists on
DefaultUrlMappingsHolder's own hardcoded classloader - not injectable -
so covering it needs a classpath-crafted forked-process test for two
lines of pure logging with no behavioral significance).

Result: UrlMappingsIndexProperties.java goes from 5 missing/3 partial to
2 missing/2 partial lines; full module test suite remains green.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@borinquenkid borinquenkid added this to the grails:8.0.0-RC1 milestone Jul 27, 2026
@testlens-app

testlens-app Bot commented Jul 27, 2026

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: 3034e47
▶️ Tests: 37404 executed
⚪️ Checks: 59/59 completed


Learn more about TestLens at testlens.app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

3 participants