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.

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

fix : URI detector/template loading from windows fs #110

Closed
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import com.vladsch.flexmark.util.sequence.Escaping.unescapeHtml
import org.asciidoctor.*
import org.asciidoctor.ast.Document
import java.nio.file.Path
import java.nio.file.Paths
import kotlin.io.path.Path
import kotlin.io.path.div

Expand All @@ -14,7 +15,7 @@ class AsciidocParser(
) {

companion object {
private val TEMPLATES_LOCATION = "/com/github/zeldigas/text2confl/asciidoc"
private val TEMPLATES_LOCATION = "com/github/zeldigas/text2confl/asciidoc"
}

private val ADOC: Asciidoctor by lazy {
Expand All @@ -27,9 +28,10 @@ class AsciidocParser(
}

private val templatesLocation: Path by lazy {
val templateResources = AsciidocParser::class.java.getResource(TEMPLATES_LOCATION)!!.toURI()
val templateResources = AsciidocParser::class.java.classLoader.getResource(TEMPLATES_LOCATION)!!.toURI()
Copy link
Owner

Choose a reason for hiding this comment

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

what is the reason for this change?

if (templateResources.scheme == "file") {
Path(templateResources.path)
val mainPath: String = Paths.get(templateResources).toString()
Path(mainPath)
} else {
val dest = config.workdir / "templates"

Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
package com.github.zeldigas.text2confl.convert.confluence

import com.github.zeldigas.text2confl.convert.PageHeader
import io.github.oshai.kotlinlogging.KotlinLogging
import java.net.URL
import java.nio.file.InvalidPathException
import java.nio.file.Path
import java.util.regex.Matcher
import java.util.regex.Pattern
import kotlin.io.path.relativeTo

interface ReferenceProvider {
Expand Down Expand Up @@ -39,24 +44,41 @@ class ReferenceProviderImpl(private val basePath: Path, documents: Map<Path, Pag
ReferenceProvider {

companion object {
private val URI_DETECTOR = "^[a-zA-Z][a-zA-Z0-9.+-]+:/{0,2}".toRegex()
private const val URI_DETECTOR = "^(https?|ftp)://[-a-zA-Z0-9+&@#/%?=~_|!:,.;'()]*[-a-zA-Z0-9+&@#/%=~_|]"
private const val MAILTO_DETECTOR = "mailto:"
private const val LOCALHOST_DETECTOR = "localhost:"

private val logger = KotlinLogging.logger {}
}
fun isValid(url: String): Boolean {
val pattern = Pattern.compile(URI_DETECTOR, Pattern.CASE_INSENSITIVE);
Copy link
Owner

Choose a reason for hiding this comment

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

please use regex as a field instead to avoid compiling regex string every time. Original code worked better, so please rollback to original approach.

val matcher = pattern.matcher(url.trim());
return matcher.matches();
}

private val normalizedDocs =
documents.map { (path, header) -> path.relativeTo(basePath).normalize() to header }.toMap()

override fun resolveReference(source: Path, refTo: String): Reference? {
if (URI_DETECTOR.matches(refTo)) return null

if (refTo.startsWith(MAILTO_DETECTOR)) return null
Copy link
Owner

Choose a reason for hiding this comment

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

I really don't get what's the reason of doing multiple matches - how about simply slightly adjusting original regex to catch url scheme and checking that it's http, https, ftp or mailto?

if (refTo.startsWith(LOCALHOST_DETECTOR)) return null
if (isValid(refTo)) return null
Copy link
Owner

Choose a reason for hiding this comment

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

method name is confusing - what does "valid" mean here? better to rename with isExternalUrl or something like this

if (refTo.startsWith("#")) return Anchor(refTo.substring(1))

val parts = refTo.split("#", limit = 2)
val ref = parts[0]
val anchor = parts.getOrNull(1)

val targetPath = source.resolveSibling(ref).relativeTo(basePath).normalize()
try {

val document = normalizedDocs[targetPath]?.title ?: return null
return Xref(document, anchor)
val targetPath = source.resolveSibling(ref).relativeTo(basePath).normalize()
val document = normalizedDocs[targetPath]?.title ?: return null
return Xref(document, anchor)
} catch (ex: InvalidPathException) {
logger.error { "Failed to resolve : $refTo from $source" }
Copy link
Owner

Choose a reason for hiding this comment

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

it's a bad pattern - log error and rethrow exception - because you'd get one more logging in another place. Either do not log or throw another exception with chained root cause.

throw ex
}
}

override fun pathFromDocsRoot(source: Path): Path {
Expand All @@ -83,4 +105,3 @@ class ReferenceProviderImpl(private val basePath: Path, documents: Map<Path, Pag


}

Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ class ConfluenceNodeRenderer(options: DataHolder) : PhasedNodeRenderer, Attribut
val resolvedLink = context.resolveLink(LinkType.IMAGE, node.url.unescape(), null, null)
var linkUrl: String = resolvedLink.url

if (!node.urlContent.isEmpty) {
if (node.urlContent.isNotEmpty) {
// reverse URL encoding of =, &
val content = Escaping.percentEncodeUrl(node.urlContent).replace("+", "%2B").replace("%3D", "=")
.replace("%26", "&amp;")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import org.junit.jupiter.api.io.TempDir
import org.junit.jupiter.params.ParameterizedTest
import org.junit.jupiter.params.provider.CsvSource
import java.nio.file.Path
import java.util.*
import kotlin.io.path.Path
import kotlin.io.path.writeBytes

Expand Down Expand Up @@ -70,6 +71,7 @@ class PageContentTest {

@Test
internal fun `Invalid result for unbalanced xml`() {
Locale.setDefault(Locale.ENGLISH);
Copy link
Owner

Choose a reason for hiding this comment

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

this is a bad idea - especially taking into account that you do it without any try/finally/extension way.

It's better to use something like this https://wesome.org/junit-5-pioneer-defaultlocale or create adhoc extension in tests to do something similar

val sampleXml = """
<table>
<p ac:parameter="hello">hello world</p>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package com.github.zeldigas.text2confl.convert.confluence
import assertk.assertThat
import assertk.assertions.isEqualTo
import assertk.assertions.isNotNull
import assertk.assertions.isNull
import com.github.zeldigas.text2confl.convert.PageHeader
import org.junit.jupiter.api.Test
import org.junit.jupiter.params.ParameterizedTest
Expand Down Expand Up @@ -68,4 +69,49 @@ internal class ReferenceProviderImplTest {

assertThat(result).isNotNull().isEqualTo(Xref("Sub Title One", "test"))
}


@Test
internal fun `Http resolution`() {
Copy link
Owner

Choose a reason for hiding this comment

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

PR title says that there is some fix for windows. What actually is failing here on windows?

val result = providerImpl.resolveReference(Path("docs/one.md"), "http://github.com")

assertThat(result).isNull()
}

@Test
internal fun `Https resolution`() {
val result = providerImpl.resolveReference(Path("docs/one.md"), "https://github.com")

assertThat(result).isNull()
}

@Test
internal fun `Mailto resolution`() {
val result = providerImpl.resolveReference(Path("docs/one.md"), "mailto:john.doe@github.com")

assertThat(result).isNull()
}


@Test
internal fun `French url resolution`() {
val result = providerImpl.resolveReference(Path("docs/one.md"), "http://github.com/handle'case")

assertThat(result).isNull()
}

@Test
internal fun `Parenthesis url resolution`() {
val result = providerImpl.resolveReference(Path("docs/one.md"), "http://github.com/handle()case")

assertThat(result).isNull()
}

@Test
internal fun `localhost url resolution`() {
val result = providerImpl.resolveReference(Path("docs/one.md"), "localhost:9000")

assertThat(result).isNull()
}

}