docs: fix broken external and internal links in Groovydocs - #16005
docs: fix broken external and internal links in Groovydocs#16005borinquenkid wants to merge 4 commits into
Conversation
… coverage jdaugherty flagged that wiring auditGroovydocLinks as an unconditional dependency of the docs task would fail the build the moment any violation is found - and the author's own investigation on this PR noted ~30 residual "phantom" link reports that aren't real errors, so merging as-is would have broken every future docs build. Keep the task registered and runnable on demand (./gradlew :grails-doc:auditGroovydocLinks) without wiring it into docsTask.dependsOn. Also extract the violation-detection logic into a Gradle-API-free GroovydocLinkAuditor so it's unit testable - build-logic/docs-core deliberately keeps Gradle classes off the test compile classpath (see docs-core/build.gradle), so the previous DefaultTask-only implementation had no path to test coverage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR continues the effort to stabilize and validate generated Groovydocs by (1) improving external link mapping used during Groovydoc generation and (2) introducing a dedicated, testable Groovydoc link audit implementation that can be run on demand rather than as part of every docs build.
Changes:
- Registers a new
:grails-doc:auditGroovydocLinkstask (backed byAuditGroovydocLinksTask) to audit generated API docs. - Updates Groovydoc external link configuration/resolution to use Gradle’s
resolutionResultand expands mapped external Javadoc roots (Spring, Hibernate, Jakarta, GORM, Groovy). - Adds a Gradle-API-free
GroovydocLinkAuditorwith Spock unit tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| grails-doc/build.gradle | Adds registration for the auditGroovydocLinks task wired to combinedGroovydoc. |
| gradle/docs-dependencies.gradle | Updates dependency version resolution and expands external Javadoc link roots used by Groovydoc. |
| build-logic/docs-core/src/test/groovy/org/grails/doc/gradle/GroovydocLinkAuditorSpec.groovy | Adds unit tests for link violation detection. |
| build-logic/docs-core/src/main/groovy/org/grails/doc/gradle/GroovydocLinkAuditor.groovy | Introduces the Gradle-API-free auditor implementation. |
| build-logic/docs-core/src/main/groovy/org/grails/doc/gradle/AuditGroovydocLinksTask.groovy | Adds the Gradle task wrapper that runs the auditor and fails on violations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def auditGroovydocLinks = tasks.register('auditGroovydocLinks', AuditGroovydocLinksTask) { | ||
| dependsOn combinedGroovydoc | ||
| apiDocsDir = combinedGroovydoc.get().destinationDir | ||
| } |
| NAV_LINK_PATTERNS.each { Pattern pattern, String replacement -> | ||
| Matcher matcher = pattern.matcher(content) | ||
| if (matcher.find()) { | ||
| violations << "Malformed nav link in ${file.name}: ${matcher.group(0)}".toString() | ||
| } | ||
| } |
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
I think there's more work to do on this one.
| def springVersion = resolveProjectVersion('spring-core') | ||
| if (springVersion) { | ||
| links << [packages: 'org.springframework.core.', href: "https://docs.spring.io/spring-framework/docs/${springVersion}/javadoc-api/"] | ||
| links << [packages: 'org.springframework.', href: "https://docs.spring.io/spring-framework/docs/${springVersion}/javadoc-api/"] |
There was a problem hiding this comment.
Groovydoc resolves links first-match-wins in list order (SimpleGroovyClassDoc.getDocUrl iterates the links and returns on the first type.startsWith(prefix) hit). Broadening this entry to org.springframework. while it sits before the org.springframework.boot. entry means every Spring Boot class reference now matches here first and links into docs.spring.io/spring-framework/..., where the Boot packages don't exist — the Boot entry below becomes dead code and all Boot links 404.
Most-specific-first fixes it: move the org.springframework.boot. entry above this one.
| } | ||
| def jakartaValidationVersion = resolveProjectVersion('jakarta.validation-api') | ||
| if (jakartaValidationVersion) { | ||
| links << [packages: 'jakarta.validation.', href: "https://jakarta.ee/specifications/bean-validation/3.0/apidocs/"] |
There was a problem hiding this comment.
Minor: these three Jakarta links gate on the resolved artifact version but hardcode the spec version in the URL (3.0, 3.1, platform/10), so they'll silently go stale on the next spec bump. Deriving the URL segment from the resolved version (as done for Hibernate above) would keep them consistent.
| if (jakartaServletVersion) { | ||
| links << [packages: 'jakarta.servlet.', href: "https://jakarta.ee/specifications/platform/10/apidocs/"] | ||
| } | ||
| links << [packages: 'org.grails.datastore.', href: "https://gorm.grails.org/latest/api/"] |
There was a problem hiding this comment.
Question on these three: since the data modules merged into this repo in 7.0, org.grails.datastore.* / grails.gorm.* classes are generated in this repo's own aggregate groovydoc. gorm.grails.org/latest/api/ is the standalone pre-merge GORM site — is that the intended target, and are we sure these prefixes don't hijack references that should resolve locally within the combined docs?
There was a problem hiding this comment.
Actually, we host these directly now on our website, so we shoudlnt' refer to the older docs link
| links << [packages: 'org.grails.datastore.', href: "https://gorm.grails.org/latest/api/"] | ||
| links << [packages: 'grails.gorm.', href: "https://gorm.grails.org/latest/api/"] | ||
| links << [packages: 'org.grails.gorm.', href: "https://gorm.grails.org/latest/api/"] | ||
| links << [packages: 'groovy.', href: "https://docs.groovy-lang.org/latest/html/gapi/"] |
There was a problem hiding this comment.
docs.groovy-lang.org/latest is Groovy 5 while 7.0.x ships Groovy 4, so these links will point at the wrong major version's API docs. The rest of this block pins resolved versions — GroovySystem.version is available here to build the versioned URL (https://docs.groovy-lang.org/docs/groovy-${GroovySystem.version}/html/gapi/).
| class GroovydocLinkAuditor { | ||
|
|
||
| // Patterns common to Groovydoc navigation failures | ||
| private static final Map<Pattern, String> NAV_LINK_PATTERNS = [ |
There was a problem hiding this comment.
These nav-link patterns can never match real Groovydoc output. The templates in groovy-groovydoc (checked 4.0.24, what 7.0.x uses) emit nav links double-quoted with a legitimate relative prefix: href="${classDoc.relativeRootPath}deprecated-list.html". So the single-quoted patterns are dead code — and if the quoting were "fixed", they'd flag every nested class page's perfectly valid nav links, since the prefix is correct there. That also means a "0 violations" run partly reflects this vacuity rather than clean docs.
The inner-class check below is the genuinely valid part: class cross-reference links are single-quoted in the templates, which is where the Outer/Inner.html vs Outer.Inner.html phantom-link bug lives, and the only-flag-if-the-dotted-file-exists guard is a smart false-positive suppressor.
Suggest dropping these patterns (or reworking them to detect links whose resolved target doesn't exist, like the inner-class check does). Two related smells that go away with them: the map values (the replacement strings) are never read anywhere, and findViolationsInFile only records the first match per pattern per file, so the "Found N malformed links" total in the task's exception would undercount.
| @TaskAction | ||
| void auditLinks() { | ||
| File apiDir = apiDocsDir.get().asFile | ||
| if (!apiDir.exists()) { |
There was a problem hiding this comment.
This guard is unreachable: with @InputDirectory, Gradle fails input validation before the @TaskAction runs when the directory is missing.
| if (!violations.isEmpty()) { | ||
| violations.take(10).each { logger.error(it) } | ||
| if (violations.size() > 10) logger.error("... and ${violations.size() - 10} more") | ||
| throw new org.gradle.api.GradleException("Found ${violations.size()} malformed links in Groovydoc. Please fix the source issue rather than patching. See logs for details.") |
There was a problem hiding this comment.
Import org.gradle.api.GradleException instead of the inline FQCN — it's already on the compile classpath here, and the project convention is explicit imports.
|
|
||
| def auditGroovydocLinks = tasks.register('auditGroovydocLinks', AuditGroovydocLinksTask) { | ||
| dependsOn combinedGroovydoc | ||
| apiDocsDir = combinedGroovydoc.get().destinationDir |
There was a problem hiding this comment.
combinedGroovydoc.get() realizes the task eagerly and requires the explicit dependsOn above. Provider wiring keeps it lazy and carries the dependency implicitly:
apiDocsDir = project.layout.dir(combinedGroovydoc.map { it.destinationDir })(and then the dependsOn line can go.)
Fixes the CHANGES_REQUESTED review comments left unaddressed on this PR:
- Wire auditGroovydocLinks.apiDocsDir via Provider (project.layout.dir(
combinedGroovydoc.map { it.destinationDir })) instead of an eager
combinedGroovydoc.get(), so the dependency is implicit and the task
isn't realized during configuration.
- Drop the NAV_LINK_PATTERNS check entirely rather than reworking it:
those patterns never matched real (double-quoted) Groovydoc output,
so "0 violations" reflected dead code, not clean docs. Reworking it
to flag unresolved targets was tried and reverted after verifying
against real generated output - groovy-groovydoc's package-summary
template omits the relative-root prefix on its bottom nav bar for
every package, which would flag ~330 instances of that one known
tool quirk per module. The inner-class check (the genuinely valid
part, per review) is unaffected and still the sole detector.
- Remove the AuditGroovydocLinksTask directory-exists guard: it's dead
code, since @InputDirectory already fails task validation before
@TaskAction runs if the directory is missing.
- Import GradleException instead of using the inline FQCN.
- Fix the org.springframework[.boot]. link ordering: Groovydoc resolves
links first-match-wins, so the broader entry sitting before the more
specific one was making every Spring Boot link resolve against the
Framework javadocs, where the Boot classes don't exist.
- Derive the Jakarta spec/platform versions from the resolved artifact
versions instead of hardcoding them - the servlet link was already
stale on this branch (hardcoded platform/10, but 7.0.x resolves
servlet-api 6.1.0, i.e. platform 11).
- Derive the Groovy javadoc link from the resolved groovy version
instead of "latest" (which serves Groovy 5 docs; 7.0.x ships 4.0.32).
- Remove the org.grails.datastore./grails.gorm./org.grails.gorm.
external link mappings: those classes are generated directly in this
repo's own aggregate Groovydoc since the data modules merged in 7.0,
so mapping them externally to gorm.grails.org misdirects local links.
Verified the corrected GroovydocLinkAuditor against real generated
Groovydoc output (grails-core module and the full grails-doc aggregate
combinedGroovydoc): 0 violations, with the fixed link-order/version
values confirmed present in the resolved links list.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
✅ All tests passed ✅🏷️ Commit: 45a938c Learn more about TestLens at testlens.app. |
Continues #15327 by @sanjana2505006 — rebases onto current 7.0.x and addresses @jdaugherty's outstanding review feedback:
TEST_FAILURES.mdthat was accidentally committed (flagged in docs: fix broken external and internal links in Groovydocs #15327, never addressed).auditGroovydocLinksfrom the defaultdocsbuild so it can't fail every future docs build on a false positive. jdaugherty's last comment on docs: fix broken external and internal links in Groovydocs #15327: "The audit should be a task we run, but it shouldn't be run by default unless we're going to capture it in the workflow summary of the build." It remains available on demand:./gradlew :grails-doc:auditGroovydocLinks.GroovydocLinkAuditorclass with unit tests (build-logic/docs-coredeliberately keeps Gradle classes off the test compile classpath, so the previousDefaultTask-only implementation had no path to test coverage).Verified against a full
aggregateGroovydocrun on current7.0.x(all 60+ modules): 0 violations — the ~30 phantom-link false positives Sanjana observed earlier in the PR's life (from the old Python-script-based approach) no longer reproduce with the current native-Groovy audit logic.Closes #15327.
Co-Authored-By: Sanjana sanju250506@gmail.com