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 a doc anchor is not encoded correctly #867

Merged
merged 9 commits into from
May 10, 2024
2 changes: 1 addition & 1 deletion Sources/SwiftDocC/Infrastructure/NodeURLGenerator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ public struct NodeURLGenerator {
)
} else {
let url = baseURL.appendingPathComponent(safePath, isDirectory: false)
return url.withFragment(reference.url.fragment)
return url.withFragment(reference.url.fragment?.removingPercentEncoding)
Copy link
Contributor

Choose a reason for hiding this comment

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

The reference stores the unencoded fragment. We can pass that here.

Suggested change
return url.withFragment(reference.url.fragment?.removingPercentEncoding)
return url.withFragment(reference.fragment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I didn't notice that it has the unencoded fragment. I've fixed it.

}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,11 @@ struct RenderContentCompiler: MarkupVisitor {
}

mutating func visitHeading(_ heading: Heading) -> [RenderContent] {
return [RenderBlockContent.heading(.init(level: heading.level, text: heading.plainText, anchor: urlReadableFragment(heading.plainText)))]
let fragment = urlReadableFragment(heading.plainText)
var components = URLComponents()
components.fragment = fragment
let anchor = components.url?.fragment ?? fragment
return [RenderBlockContent.heading(.init(level: heading.level, text: heading.plainText, anchor: anchor))]
Copy link
Contributor

Choose a reason for hiding this comment

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

A shorter way of encoding the fragment, without needing to handle the optional url, would be to call .addingPercentEncoding(withAllowedCharacters: .urlFragmentAllowed) on the fragment

Suggested change
let fragment = urlReadableFragment(heading.plainText)
var components = URLComponents()
components.fragment = fragment
let anchor = components.url?.fragment ?? fragment
return [RenderBlockContent.heading(.init(level: heading.level, text: heading.plainText, anchor: anchor))]
return [RenderBlockContent.heading(.init(level: heading.level, text: heading.plainText, anchor: urlReadableFragment(heading.plainText).addingPercentEncoding(withAllowedCharacters: .urlFragmentAllowed)))]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. But the reference url is encoded by using URLComponents here.

var components = URLComponents()
components.scheme = ResolvedTopicReference.urlScheme
components.host = bundleIdentifier
components.path = path
components.fragment = fragment
self.url = components.url!

Is there no difference between URLComponents and addingPercentEncoding(withAllowedCharacters:.urlFragmentAllowed)? If there are any character that will be encoded on the one hand and not be on the other hand, the link will be broken. I'm afraid of that. How do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I'm aware they do the same thing and the documentation for URLComponents/percentEncodedFragment recommends that the caller uses .addingPercentEncoding(withAllowedCharacters: .urlFragmentAllowed) for compatibility if they want to assign an already encoded fragment:

[...] Although ‘;’ is a legal path character, it is recommended that it be percent-encoded for best compatibility with URL (String.addingPercentEncoding(withAllowedCharacters:) will percent-encode any ‘;’ characters if you pass CharacterSet.urlFragmentAllowed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I'll modify the code as per your suggestion.

}

mutating func visitListItem(_ listItem: ListItem) -> [RenderContent] {
Expand Down
13 changes: 13 additions & 0 deletions Tests/SwiftDocCTests/Infrastructure/NodeURLGeneratorTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,19 @@ class NodeURLGeneratorTests: XCTestCase {
XCTAssertEqual(generator.urlForReference(classIdentifier).absoluteString, "file:///path/to/bundle/_testbundle-ctlj/products/documentation.builtbundle/com.example.testbundle/data/folder/_privateclass/_privatesubclass")
}

func testsafeURLWithEncodableFragment() throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want this test because we can't verify that it's created the resolved topic reference in the same way that the context is. This means that this test could pass while the end-to-end experience is still broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I've removed it.

// This is a realist DerivedData folder.
let baseURL = URL(string: "file:///path/to/bundle/_testbundle-ctlj/products/documentation.builtbundle/com.example.testbundle/data/")!
let generator = NodeURLGenerator(baseURL: baseURL)

let basicIdentifier = ResolvedTopicReference(bundleIdentifier: "com.example.testbundle",
path: "/folder/class/symbol",
fragment: "テスト", // Non-ASCII; E3 83 86 E3 82 B9 E3 83 88 in UTF-8
sourceLanguage: .swift)

XCTAssertEqual(generator.urlForReference(basicIdentifier).absoluteString, "file:///path/to/bundle/_testbundle-ctlj/products/documentation.builtbundle/com.example.testbundle/data/folder/class/symbol#%E3%83%86%E3%82%B9%E3%83%88")
}

var inputLongPaths = [
// Paths within the limits
"/path/to/symbol.json",
Expand Down
22 changes: 22 additions & 0 deletions Tests/SwiftDocCTests/Model/RenderContentMetadataTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -297,4 +297,26 @@ class RenderContentMetadataTests: XCTestCase {
default: XCTFail("Unexpected element")
}
}

func testHeadingAnchorShouldBeEncoded() throws {
let (bundle, context) = try testBundleAndContext(named: "TestBundle")
var renderContentCompiler = RenderContentCompiler(context: context, bundle: bundle, identifier: ResolvedTopicReference(bundleIdentifier: bundle.identifier, path: "/path", fragment: nil, sourceLanguage: .swift))

let source = """
## テスト

Note that テスト consists of Non-ASCII characters of E3 83 86 E3 82 B9 E3 83 88 in UTF-8.
Copy link
Contributor

Choose a reason for hiding this comment

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

very minor: Rather than this content in the markup, this information could be more useful near the heading.anchor assertion.

XCTAssertEqual(heading.anchor, "%E3%83%86%E3%82%B9%E3%83%88", "The UTF-8 representation of テスト is E3 83 86 E3 82 B9 E3 83 88")

If could also help explain the test assertion if there was a second assertion that verified that it matched the percent escaped value:

XCTAssertEqual(heading.anchor, "テスト".addingPercentEncoding(withAllowedCharacters: .urlFragmentAllowed))

That way someone who reads this test can understand that %E3%83%86%E3%82%B9%E3%83%88 is the percent encoded fragment.

"""
let document = Document(parsing: source)

let result = try XCTUnwrap(renderContentCompiler.visit(document.child(at: 0)!))
let element = try XCTUnwrap(result.first as? RenderBlockContent)
switch element {
case .heading(let heading):
XCTAssertEqual(heading.level, 2)
XCTAssertEqual(heading.text, "テスト")
XCTAssertEqual(heading.anchor, "%E3%83%86%E3%82%B9%E3%83%88")
default: XCTFail("Unexpected element")
}
}
}