Add optional artefact index reader for startup scanning - #16000
Add optional artefact index reader for startup scanning#16000jamesfredley wants to merge 3 commits into
Conversation
Assisted-by: opencode:gpt-5.6-sol
There was a problem hiding this comment.
Pull request overview
Introduces an internal, optional artefact index reader (META-INF/grails/artefacts.idx) to speed up application startup artefact discovery, while preserving the existing classpath-scan behavior via safe fallback when the index can’t be used.
Changes:
- Add
ArtefactIndexReaderto load application-rootartefacts.idx(ordered + deduped) and returnnullto trigger fallback scanning on failures. - Update
ApplicationArtefactScannerto prefer indexed classes when available and otherwise use the existingClassPathScanner, then append transformed classes. - Add a comprehensive
ApplicationArtefactScannerSpecplus small fixture artefacts to validate ordering, deduplication, package filtering, and fallback scenarios.
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-core/src/main/groovy/grails/boot/config/ArtefactIndexReader.java | New internal reader for the optional application artefact index with “all-or-nothing” validation behavior. |
| grails-core/src/main/groovy/grails/boot/config/ApplicationArtefactScanner.groovy | Prefer index-based loading (when valid) and use LinkedHashSet to preserve deterministic ordering/deduplication semantics. |
| grails-core/src/test/groovy/grails/boot/config/ApplicationArtefactScannerSpec.groovy | Adds coverage for index ordering, deduplication, fallback behavior, dependency index isolation, linkage failures, and transformed-class inclusion. |
| grails-core/src/test/groovy/grails/boot/config/indexed/IncludedArtefact.groovy | Test fixture used to validate packageName filtering for indexed entries. |
| grails-core/src/test/groovy/grails/boot/config/excluded/ExcludedArtefact.groovy | Test fixture used to validate packageName filtering exclusion for indexed entries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * <p>The index is UTF-8 text at {@value #RESOURCE_NAME}, with one fully qualified | ||
| * class name per nonempty line. Any unreadable or unresolvable entry rejects the | ||
| * complete index so callers can use their normal classpath scan.</p> | ||
| */ |
|
The Javadoc in grails-core/src/main/groovy/grails/boot/config/ArtefactIndexReader.java |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16000 +/- ##
================================================
+ Coverage 0 51.5133% +51.5133%
- Complexity 0 17800 +17800
================================================
Files 0 2040 +2040
Lines 0 95618 +95618
Branches 0 16597 +16597
================================================
+ Hits 0 49256 +49256
- Misses 0 39052 +39052
- Partials 0 7310 +7310
🚀 New features to boost your workflow:
|
|
Trying to understand the intent behind this one. Is it for startup speed and if so, how does it help there. PR description just needs a little more to help me understand intent. |
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
The initial review is below, but I think this needs discussed more in the weekly since it's an architectural shift. In general, I'm ok moving to more compile time based fixes for the performance benefit, but we need to agree as a team. Putting a PR out here prior to a discussion is concerning, since it could be merged without a wider architectural discussion.
| URL resource = new URL(IOUtils.findRootResource(applicationClass), RESOURCE_NAME); | ||
| Set<Class> classes = new LinkedHashSet<>(); | ||
| return readResource(resource, applicationClass.getClassLoader(), packageNames, classes) ? classes : null; | ||
| } catch (IOException ignored) { |
There was a problem hiding this comment.
IOUtils.findRootResource throws IllegalStateException, not IOException, when the class resource can't be resolved (targetClass.getResource(...) returning null — e.g. an application class from a classloader that doesn't expose .class resources). That escapes this catch and would fail startup, where today the same situation just scans. Since the whole contract of this reader is "never make things worse than the fallback," this should catch that too (e.g. catch (IOException | RuntimeException)).
| private ArtefactIndexReader() { | ||
| } | ||
|
|
||
| static Collection<Class> read(Class<?> applicationClass, Collection<String> packageNames) { |
There was a problem hiding this comment.
Two operational concerns for production startup code:
- Silence. Both outcomes are invisible — no log when the index is used, none when it's rejected. A corrupted index silently degrades to slow scanning and nobody finds out; a working index can't be confirmed either. Suggest debug-level logs for "index used (N entries)" and "index rejected, falling back".
- Stale-index hazard. A valid but incomplete index is the dangerous case: developer adds an artefact, index isn't regenerated, and the new class is silently absent from the application — no error, no fallback. The producer side will need a freshness guarantee (or the index should carry a hash/marker the reader can validate), and a kill-switch system property to force scanning would be a cheap escape hatch worth adding in the seed.
| } | ||
| } | ||
|
|
||
| private static boolean isInPackage(String className, Collection<String> packageNames) { |
There was a problem hiding this comment.
Worth documenting the semantic differences from the ClassPathScanner path this replaces, since the future index producer has to compensate for them:
- The scanner only returns classes carrying an annotation whose name starts with
grails.(defaultannotationFilter); the reader trusts every listed entry with no annotation check, so a hand-edited or buggy index can inject arbitrary classes into the artefact set. - The scanner skips
DEFAULT_IGNORED_ROOT_PACKAGES(com,org,net, …) even when explicitly passed as packageNames; the reader honors them.
Both are fine if the producer mirrors scan semantics exactly, but that contract currently lives nowhere — a sentence in the class javadoc would pin it.
| BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8))) { | ||
| String className; | ||
| while ((className = reader.readLine()) != null) { | ||
| if (className.isEmpty()) { |
There was a problem hiding this comment.
Rejecting the entire index on an empty line is stricter than it needs to be — a trailing blank line is the most common artifact of text-file generation and concatenation (the spec's own writeIndex has to .trim() to avoid it). Skipping blank lines (continue) keeps the strict-reject behavior for genuinely malformed content while tolerating the boring case. If strictness is intentional as a whole-file integrity signal, the javadoc should say the producer must not emit blank lines, including trailing ones.
…Reader isInPackage() had untested branches for the default-package match (packageName.isEmpty()) and for defensively skipping a null entry in packageNames, leaving ArtefactIndexReader.java's patch coverage at 80%. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Five review comments (Copilot, jdaugherty) on the artefact-index reader were left unaddressed by the prior commit, which only closed a coverage gap: - Widen read()'s catch to IOException | RuntimeException so a failure in IOUtils.findRootResource (e.g. IllegalStateException) or class loading (e.g. SecurityException) falls back to scanning instead of failing startup. - Add debug logging for index-used / index-rejected / index-disabled outcomes, since both paths were previously silent. - Add a grails.artefactIndex.disabled system property kill-switch to force classpath scanning. - Skip blank lines instead of rejecting the whole index on one; this also aligns the code with the class javadoc, which already documented blank lines as insignificant. - Document the semantic differences from ClassPathScanner (no annotation check, ignored-root-packages not applied) that an index producer must account for. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
✅ All tests passed ✅🏷️ Commit: 7662a26 Learn more about TestLens at testlens.app. |
Summary
META-INF/grails/artefacts.idxApplicationArtefactScanneruses a valid application-root index (honoringpackageNames) and safely falls back to classpath scanning when the index is absent, malformed, unresolvable, or linkage-invalidWhy
Today, artefact discovery (
ApplicationArtefactScanner→ClassPathScanner) runs at every application boot: it resource-pattern-matches the classpath per package, reads each candidate.classfile's metadata via ASM, and checks it for agrails.*-prefixed annotation. That's proportional to the number of classes on the classpath, not the number of actual artefacts, and it's pure runtime I/O + reflection-adjacent work that produces the same result on every single boot for a given build.An artefact index moves that discovery from "boot time, every time" to "build time, once": a producer (not part of this PR — see Scope note) would emit the flat list of artefact class names once, at build time, and boot then becomes a plain file read plus loading exactly those classes — no classpath scanning, no per-class metadata reads. That's the startup-speed win, and it scales with the number of artefacts instead of the size of the classpath.
This is consistent with a direction the team is already moving in elsewhere: shifting checks/work that don't need to happen at runtime to compile/build time (e.g. the compile-time SQL injection prevention work). This PR proposes applying that same idea to artefact discovery.
Scope note
This is a starter/seed: it adds only the reader and its safe fallback. The index producer is not included, so the optimization activates only once an
artefacts.idxis emitted into the application code-source root; until then behavior is unchanged (fallback scanning).Verification
:grails-core:test --tests ApplicationArtefactScannerSpec:grails-core:testgit diff --checkThis is an AI-generated starting point for build-time artefact indexing.