Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

all-modules-page: Validate cross-module links during resolution #2901

Closed
wants to merge 1 commit into from
Closed

all-modules-page: Validate cross-module links during resolution #2901

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions plugins/all-modules-page/src/main/kotlin/ExternalModuleLinkResolver.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import org.jetbrains.dokka.base.DokkaBase
import org.jetbrains.dokka.base.resolvers.shared.ExternalDocumentation
import org.jetbrains.dokka.base.resolvers.shared.PackageList
import org.jetbrains.dokka.base.resolvers.shared.PackageList.Companion.PACKAGE_LIST_NAME
import org.jetbrains.dokka.base.resolvers.shared.RecognizedLinkFormat
import org.jetbrains.dokka.links.DRI
import org.jetbrains.dokka.plugability.DokkaContext
import org.jetbrains.dokka.plugability.plugin
Expand Down Expand Up @@ -52,15 +51,19 @@ class DefaultExternalModuleLinkResolver(val context: DokkaContext) : ExternalMod
}

override fun resolve(dri: DRI, fileContext: File): String? {
val absoluteLink = elps.mapNotNull { it.resolve(dri) }.firstOrNull() ?: return null
val modulePath = context.configuration.outputDir.absolutePath.split(File.separator)
val contextPath = fileContext.absolutePath.split(File.separator)
val commonPathElements = modulePath.zip(contextPath)
val resolvedLinks = elps.mapNotNull { it.resolve(dri)?.removePrefix("file:") }
val modulePath = context.configuration.outputDir.absolutePath
val validLink = resolvedLinks.firstOrNull {
val absolutePath = File(modulePath + it)
absolutePath.isFile
Copy link

@pbajurko pbajurko Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This checks assumes that dependee submodules have been already processed but that is upto process order
in all-modules-page plugin. For our project it lead to situation where same DRI was sometimes resolved and sometimes not.
I workaround it by checking paths against Dokka's output in submodules since partial tasks will be done before multimodule starts but that is not clean solution.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. Thank you for noticing it. The workaround is correct, but it should not depend on particular paths like build/dokka/htmlMultiModule or build/dokka/htmlPartial/. It would be great to use module.sourceOutputDirectory

} ?: return null
Copy link
Member

Choose a reason for hiding this comment

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

Will it not work for the non-local external package-list at all? See the example with the serialization library from External documentation links configuration. Can you fix it, please?

It is nice to fix the issue at least for cross-module links.
A check of existing external URLs online can be excessive and Dokka can be run without access to the Internet.

Also, I think the fix should check links only if resolvedLinks has more than one link. What do you think? It can help avoid unexpected behaviour with resolving cross-module links.
This fix is local and temporary. We have a long-term plan #3368.

val modulePathParts = modulePath.split(File.separator)
val contextPathParts = fileContext.absolutePath.split(File.separator)
val commonPathElements = modulePathParts.zip(contextPathParts)
.takeWhile { (a, b) -> a == b }.count()

return (List(contextPath.size - commonPathElements - 1) { ".." } + modulePath.drop(commonPathElements)).joinToString(
"/"
) + absoluteLink.removePrefix("file:")
return (List(contextPathParts.size - commonPathElements - 1) { ".." } + modulePathParts.drop(commonPathElements))
.joinToString("/") + validLink
}

override fun resolveLinkToModuleIndex(moduleName: String): String? =
Expand Down
10 changes: 8 additions & 2 deletions plugins/all-modules-page/src/test/kotlin/templates/ResolveLinkCommandResolutionTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class ResolveLinkCommandResolutionTest : MultiModuleAbstractTest() {
}
}

val contentFile = setup(link)
val contentFile = setup(link, testedDri)
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a test case for the case from #2272?

val configuration = configuration()

testFromData(configuration, useOutputLocationFromConfig = true) {
Expand Down Expand Up @@ -95,10 +95,16 @@ class ResolveLinkCommandResolutionTest : MultiModuleAbstractTest() {
}
}

fun setup(content: String): File {
private fun setup(content: String, resolutionTarget: DRI? = null): File {
folder.create()
val innerModule1 = folder.newFolder("module1")
val innerModule2 = folder.newFolder("module2")
if (resolutionTarget != null) {
val resolvedFile = innerModule2
.resolve("${resolutionTarget.packageName}/-${resolutionTarget.classNames?.toLowerCase()}/index.html")
resolvedFile.parentFile.mkdirs()
resolvedFile.createNewFile()
}
val packageList = innerModule2.resolve("package-list")
packageList.writeText(mockedPackageListForPackages(RecognizedLinkFormat.DokkaHtml, "package2"))
val contentFile = innerModule1.resolve("index.html")
Expand Down
14 changes: 10 additions & 4 deletions plugins/all-modules-page/src/test/kotlin/templates/ResolveLinkGfmCommandResolutionTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class ResolveLinkGfmCommandResolutionTest : MultiModuleAbstractTest() {

val expected = "[Sample text inside](../module2/package2/-sample/index.md)"

val content = setup(link)
val content = setup(link, testedDri)
val configuration = configuration()

testFromData(configuration, pluginOverrides = listOf(GfmTemplateProcessingPlugin(), GfmPlugin()), useOutputLocationFromConfig = true) {
Expand All @@ -61,10 +61,16 @@ class ResolveLinkGfmCommandResolutionTest : MultiModuleAbstractTest() {
}
}

private fun setup(content: String): File {
private fun setup(content: String, resolutionTarget: DRI? = null): File {
folder.create()
val innerModule1 = folder.newFolder( "module1")
val innerModule2 = folder.newFolder( "module2")
val innerModule1 = folder.newFolder("module1")
val innerModule2 = folder.newFolder("module2")
if (resolutionTarget != null) {
val resolvedFile = innerModule2
.resolve("${resolutionTarget.packageName}/-${resolutionTarget.classNames?.toLowerCase()}/index.md")
resolvedFile.parentFile.mkdirs()
resolvedFile.createNewFile()
}
val packageList = innerModule2.resolve("package-list")
packageList.writeText(mockedPackageListForPackages(RecognizedLinkFormat.DokkaGFM, "package2"))
val contentFile = innerModule1.resolve("index.md")
Expand Down