jdaugherty commented on code in PR #16005:
URL: https://github.com/apache/grails-core/pull/16005#discussion_r3895767090
##########
gradle/docs-dependencies.gradle:
##########
@@ -54,14 +48,54 @@ def configureGroovyDoc =
tasks.register('configureGroovyDoc') {
if (testContainersVersion) {
links << [packages: 'org.testcontainers.', href:
"https://javadoc.io/doc/org.testcontainers/testcontainers/${testContainersVersion}/";]
}
- def springVersion = resolveProjectVersion('spring-core')
- if (springVersion) {
- links << [packages: 'org.springframework.core.', href:
"https://docs.spring.io/spring-framework/docs/${springVersion}/javadoc-api/";]
- }
+ // Groovydoc resolves 'links' first-match-wins in declaration order,
so the more
+ // specific org.springframework.boot. entry must come before the
broader
+ // org.springframework. entry or every Boot class link resolves
against the
+ // Framework javadocs instead, where the Boot classes don't exist.
def springBootVersion = resolveProjectVersion('spring-boot')
if (springBootVersion) {
links << [packages: 'org.springframework.boot.', href:
"https://docs.spring.io/spring-boot/docs/${springBootVersion}/api/";]
Review Comment:
This URL scheme was retired at Boot 3.4, so with the version this branch
resolves (3.5.16) it 404s:
| URL | status |
| --- | --- |
|
`docs.spring.io/spring-boot/docs/3.2.12/api/org/springframework/boot/SpringApplication.html`
| 200 |
|
`docs.spring.io/spring-boot/docs/3.4.0/api/org/springframework/boot/SpringApplication.html`
| 404 |
|
`docs.spring.io/spring-boot/3.5.16/api/java/org/springframework/boot/SpringApplication.html`
| 200 |
So the href wants to be
`https://docs.spring.io/spring-boot/${springBootVersion}/api/java/`.
The entry predates this PR, but this is the change that moves it above the
framework entry specifically so Boot types resolve here — worth having it point
somewhere that exists. (The `spring-framework/docs/${version}/javadoc-api/`
entry below is fine; verified 200 on 6.2.12.)
##########
gradle/docs-dependencies.gradle:
##########
@@ -54,14 +48,54 @@ def configureGroovyDoc =
tasks.register('configureGroovyDoc') {
if (testContainersVersion) {
links << [packages: 'org.testcontainers.', href:
"https://javadoc.io/doc/org.testcontainers/testcontainers/${testContainersVersion}/";]
}
- def springVersion = resolveProjectVersion('spring-core')
- if (springVersion) {
- links << [packages: 'org.springframework.core.', href:
"https://docs.spring.io/spring-framework/docs/${springVersion}/javadoc-api/";]
- }
+ // Groovydoc resolves 'links' first-match-wins in declaration order,
so the more
+ // specific org.springframework.boot. entry must come before the
broader
+ // org.springframework. entry or every Boot class link resolves
against the
+ // Framework javadocs instead, where the Boot classes don't exist.
def springBootVersion = resolveProjectVersion('spring-boot')
if (springBootVersion) {
links << [packages: 'org.springframework.boot.', href:
"https://docs.spring.io/spring-boot/docs/${springBootVersion}/api/";]
}
+ def springVersion = resolveProjectVersion('spring-core')
+ if (springVersion) {
+ links << [packages: 'org.springframework.', href:
"https://docs.spring.io/spring-framework/docs/${springVersion}/javadoc-api/";]
+ }
+ def hibernateVersion = resolveProjectVersion('hibernate-core')
+ if (hibernateVersion) {
+ def shortVersion = hibernateVersion.split('\\.').take(2).join('.')
+ links << [packages: 'org.hibernate.', href:
"https://docs.jboss.org/hibernate/orm/${shortVersion}/javadocs/";]
+ }
+ def jakartaValidationVersion =
resolveProjectVersion('jakarta.validation-api')
+ if (jakartaValidationVersion) {
+ def specVersion =
jakartaValidationVersion.split('\\.').take(2).join('.')
+ links << [packages: 'jakarta.validation.', href:
"https://jakarta.ee/specifications/bean-validation/${specVersion}/apidocs/";]
+ }
+ def jakartaPersistenceVersion =
resolveProjectVersion('jakarta.persistence-api')
+ if (jakartaPersistenceVersion) {
+ def specVersion =
jakartaPersistenceVersion.split('\\.').take(2).join('.')
+ links << [packages: 'jakarta.persistence.', href:
"https://jakarta.ee/specifications/persistence/${specVersion}/apidocs/";]
+ }
+ def jakartaServletVersion =
resolveProjectVersion('jakarta.servlet-api')
+ if (jakartaServletVersion) {
+ def servletSpecVersion =
jakartaServletVersion.split('\\.').take(2).join('.')
+ // The platform apidocs are versioned by Jakarta EE Platform
release, not by the
+ // Servlet spec version, so the two can't be derived from one
another directly.
+ def servletSpecToPlatformVersion = ['5.0': '9', '6.0': '10',
'6.1': '11']
+ def platformVersion =
servletSpecToPlatformVersion[servletSpecVersion]
+ if (platformVersion) {
+ links << [packages: 'jakarta.servlet.', href:
"https://jakarta.ee/specifications/platform/${platformVersion}/apidocs/";]
+ }
+ }
+ // No external link is registered for
org.grails.datastore./grails.gorm./org.grails.gorm.:
+ // since the data modules merged into grails-core in 7.0, those
classes are generated
+ // directly in this repo's own aggregate Groovydoc and resolve
locally. Mapping them to
+ // the older, standalone gorm.grails.org site would misdirect those
in-repo links.
+ def groovyVersion = resolveProjectVersion('groovy')
+ if (groovyVersion) {
+ links << [packages: 'groovy.', href:
"https://docs.groovy-lang.org/${groovyVersion}/html/gapi/";]
+ links << [packages: 'org.apache.groovy.', href:
"https://docs.groovy-lang.org/${groovyVersion}/html/gapi/";]
+ links << [packages: 'org.codehaus.groovy.', href:
"https://docs.groovy-lang.org/${groovyVersion}/html/gapi/";]
+ }
if (it.ext.has('groovydocLinks')) {
Review Comment:
The curated link list never reaches the Groovydoc tasks, so none of the
entries added or reordered above actually take effect.
`GroovydocEnhancerPlugin.resolveLinks(Groovydoc gdoc)` reads
`gdoc.ext.groovydocLinks` off the **Groovydoc** task, but inside this `doLast`
block `it` is the `configureGroovyDoc` task — so line 102 stores the list as an
extra property on `configureGroovyDoc` itself, and the `has()` check here only
ever sees what this same task set on an earlier run.
Reproduced on this branch:
```
./gradlew :grails-spring:groovydoc
grep -rho "href='https://[^']*'" grails-spring/build/docs/groovydoc | wc -l
# 0
```
Zero external links in the output. Every external reference comes out as a
dangling relative link instead — 20 distinct missing targets in that one small
module, e.g.
`href='../../../org.springframework.context.ApplicationContext.html'`. The
published docs show the same thing:
`org/grails/web/servlet/mvc/GrailsWebRequest.html` on
grails.apache.org/docs/latest/api links `HttpServletRequest` to
`../../../../../jakarta.servlet.http.HttpServletRequest.html`, which doesn't
exist.
Setting the same list on the Groovydoc task instead does produce external
links, which confirms the mechanism works and the wiring is the only thing
missing.
One thing worth knowing before restructuring this, because it changes what
this PR can claim: Groovy's `links` only feeds
`SimpleGroovyClassDoc.getDocUrl`, i.e. doc-comment references, `@see`, and
unresolved string types. Signature types (superclass, interfaces, parameters,
return types) go through `dolink` in groovydoc's `classDocName.html` template,
which builds `classDoc.relativeRootPath + t.fullPathName + '.html'`
unconditionally and never consults `links`. In the `:grails-spring` output only
1 of the 21 Spring references switched to an external URL once the wiring was
corrected. So fixing this recovers a real but small slice; the bulk of the
broken links needs a different fix.
##########
build-logic/docs-core/src/test/groovy/org/grails/doc/gradle/GroovydocLinkAuditorSpec.groovy:
##########
@@ -0,0 +1,69 @@
+/*
+ * 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.doc.gradle
+
+import spock.lang.Specification
+import spock.lang.TempDir
+
+class GroovydocLinkAuditorSpec extends Specification {
Review Comment:
Non-blocking, and pre-existing: nothing runs these. `build-logic` is an
included build, and no workflow invokes its `build` or `test` — a root
`./gradlew build` only pulls the plugin jars out of it. The three specs already
in this project have the same problem.
Verified they pass locally:
```
cd build-logic && ../gradlew :grails-docs-core:test --tests
'*GroovydocLinkAuditorSpec*'
# 3 passed
```
Just flagging that the coverage this adds won't gate anything until
build-logic's tests are wired into CI.
##########
gradle/docs-dependencies.gradle:
##########
@@ -54,14 +48,54 @@ def configureGroovyDoc =
tasks.register('configureGroovyDoc') {
if (testContainersVersion) {
links << [packages: 'org.testcontainers.', href:
"https://javadoc.io/doc/org.testcontainers/testcontainers/${testContainersVersion}/";]
}
- def springVersion = resolveProjectVersion('spring-core')
- if (springVersion) {
- links << [packages: 'org.springframework.core.', href:
"https://docs.spring.io/spring-framework/docs/${springVersion}/javadoc-api/";]
- }
+ // Groovydoc resolves 'links' first-match-wins in declaration order,
so the more
+ // specific org.springframework.boot. entry must come before the
broader
+ // org.springframework. entry or every Boot class link resolves
against the
+ // Framework javadocs instead, where the Boot classes don't exist.
def springBootVersion = resolveProjectVersion('spring-boot')
if (springBootVersion) {
links << [packages: 'org.springframework.boot.', href:
"https://docs.spring.io/spring-boot/docs/${springBootVersion}/api/";]
}
+ def springVersion = resolveProjectVersion('spring-core')
+ if (springVersion) {
+ links << [packages: 'org.springframework.', href:
"https://docs.spring.io/spring-framework/docs/${springVersion}/javadoc-api/";]
+ }
+ def hibernateVersion = resolveProjectVersion('hibernate-core')
+ if (hibernateVersion) {
+ def shortVersion = hibernateVersion.split('\\.').take(2).join('.')
+ links << [packages: 'org.hibernate.', href:
"https://docs.jboss.org/hibernate/orm/${shortVersion}/javadocs/";]
+ }
+ def jakartaValidationVersion =
resolveProjectVersion('jakarta.validation-api')
+ if (jakartaValidationVersion) {
+ def specVersion =
jakartaValidationVersion.split('\\.').take(2).join('.')
+ links << [packages: 'jakarta.validation.', href:
"https://jakarta.ee/specifications/bean-validation/${specVersion}/apidocs/";]
+ }
+ def jakartaPersistenceVersion =
resolveProjectVersion('jakarta.persistence-api')
+ if (jakartaPersistenceVersion) {
+ def specVersion =
jakartaPersistenceVersion.split('\\.').take(2).join('.')
+ links << [packages: 'jakarta.persistence.', href:
"https://jakarta.ee/specifications/persistence/${specVersion}/apidocs/";]
Review Comment:
The Jakarta Persistence apidocs are published as *modular* javadoc, so the
module name is a path segment. Groovydoc builds the href as `href +
type.replace('.', '/') + '.html'` (`SimpleGroovyClassDoc.buildUrl`), which
lands on:
- `.../persistence/3.1/apidocs/jakarta/persistence/Entity.html` → 404
-
`.../persistence/3.1/apidocs/jakarta.persistence/jakarta/persistence/Entity.html`
→ 200
Same for 3.2. Appending `jakarta.persistence/` to the href fixes it.
Checked the neighbours while I was there:
`bean-validation/3.1/apidocs/jakarta/validation/Validator.html` and
`platform/11/apidocs/jakarta/servlet/http/HttpServletRequest.html` are both
flat and both 200, so those two entries are correct as written. Hibernate
`orm/{5.6,6.6,7.1}/javadocs/org/hibernate/Session.html` and
`docs.groovy-lang.org/4.0.33/html/gapi/groovy/lang/MetaClass.html` are 200 as
well.
##########
build-logic/docs-core/src/main/groovy/org/grails/doc/gradle/GroovydocLinkAuditor.groovy:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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.doc.gradle
+
+import java.nio.file.Files
+import java.nio.file.Path
+import java.util.regex.Matcher
+import java.util.regex.Pattern
+
+/**
+ * Scans generated Groovydoc HTML for malformed inner-class links, where a
+ * slash-separated path segment (produced for an inner class, e.g.
+ * {@code Outer/Inner.html}) should have been the dotted Groovydoc naming
+ * convention ({@code Outer.Inner.html}). Kept free of the Gradle API so it
+ * can be unit tested directly - {@code build-logic/docs-core} deliberately
+ * keeps Gradle classes off the test compile classpath.
+ *
+ * <p>This intentionally does not audit the navigation links
(deprecated-list.html,
+ * help-doc.html, etc.): the bottom nav bar that {@code groovy-groovydoc}
emits on
+ * package-summary pages omits the relative-root prefix that every other
occurrence
+ * carries, which is a pre-existing quirk of that template rather than a
content
+ * issue this repository can fix - checking for it would flag hundreds of
instances
+ * of the same known, low-impact tool behavior on every doc build.</p>
+ */
+class GroovydocLinkAuditor {
+
+ // Inner class path issues like Query/Order.Direction.html ->
Query.Order.Direction.html.
+ private static final Pattern INNER_CLASS_LINK_PATTERN =
Review Comment:
Scope question on the audit as a whole.
"0 violations on a full `aggregateGroovydoc` run" isn't evidence the docs
are clean — this pattern only matches slash-separated inner-class paths, and
that shape doesn't appear to be emitted by Groovy 4.0.x. Published inner-class
links are correctly dotted (`org/grails/datastore/mapping/query/Query.html`
links its nested types as `Query.Criterion.html` etc.), and running this exact
regex over a locally generated `:grails-spring:groovydoc` tree matches nothing
— while that same tree contains 20 distinct hrefs pointing at files that were
never generated (`org.springframework.beans.MutablePropertyValues.html`,
`org.apache.commons.logging.Log.html`, …).
So the task as written can essentially only report clean, which makes it
weak insurance against exactly the class of breakage this PR is about.
`findViolationsInFile` already does the right thing structurally — resolve
the href relative to the page's own directory and check the filesystem.
Generalizing that to *every* relative `href` (flag when the resolved target
doesn't exist, skipping absolute/anchor-only hrefs) would catch the phantom
inner-class case, the dangling external-type case, and anything else, with less
code than the current special-casing. That would also give the task something
to say about whether the link config above is doing its job.
--
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]
