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.fragment)
}
}

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

mutating func visitHeading(_ heading: Heading) -> [RenderContent] {
return [RenderBlockContent.heading(.init(level: heading.level, text: heading.plainText, anchor: urlReadableFragment(heading.plainText)))]
return [RenderBlockContent.heading(.init(level: heading.level, text: heading.plainText, anchor: urlReadableFragment(heading.plainText).addingPercentEncoding(withAllowedCharacters: .urlFragmentAllowed)))]
}

mutating func visitListItem(_ listItem: ListItem) -> [RenderContent] {
Expand Down
20 changes: 20 additions & 0 deletions Tests/SwiftDocCTests/Model/RenderContentMetadataTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -297,4 +297,24 @@ 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 = """
## テスト
"""
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", "The UTF-8 representation of テスト is E3 83 86 E3 82 B9 E3 83 88")
default: XCTFail("Unexpected element")
}
}
}
63 changes: 63 additions & 0 deletions Tests/SwiftDocCTests/Rendering/HeadingAnchorTests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*
This source file is part of the Swift.org open source project

Copyright (c) 2024 Apple Inc. and the Swift project authors
Licensed under Apache License v2.0 with Runtime Library Exception

See https://swift.org/LICENSE.txt for license information
See https://swift.org/CONTRIBUTORS.txt for Swift project authors
*/

import Foundation
import XCTest
@testable import SwiftDocC
import SwiftDocCTestUtilities

class HeadingAnchorTests: XCTestCase {
func testEncodeHeadingAnchor() throws {
let catalogURL = try createTempFolder(content: [
Folder(name: "unit-test.docc", content: [
TextFile(name: "Root.md", utf8Content: """
# My root page

This single article defines two headings and links to them

@Metadata {
@TechnologyRoot
}

### テスト
- <doc:#テスト>

### Some heading
- <doc:#Some-heading>
"""),
])
])
let (_, bundle, context) = try loadBundle(from: catalogURL)

let reference = try XCTUnwrap(context.soleRootModuleReference)
let node = try context.entity(with: reference)
let renderContext = RenderContext(documentationContext: context, bundle: bundle)
let converter = DocumentationContextConverter(bundle: bundle, context: context, renderContext: renderContext)
let renderNode = try XCTUnwrap(converter.renderNode(for: node, at: nil))

// Check heading anchors are encoded
let contentSection = try XCTUnwrap(renderNode.primaryContentSections.first as? ContentRenderSection)
let headings: [RenderBlockContent.Heading] = contentSection.content.compactMap {
if case .heading(let heading) = $0 {
return heading
} else {
return nil
}
}
XCTAssertEqual(headings[0].anchor, "%E3%83%86%E3%82%B9%E3%83%88")
XCTAssertEqual(headings[1].anchor, "Some-heading")

// Check links to them
let testTopic0 = try XCTUnwrap(renderNode.references["doc://unit-test/documentation/Root#%E3%83%86%E3%82%B9%E3%83%88"] as? TopicRenderReference)
Copy link
Contributor

Choose a reason for hiding this comment

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

This line fails in the CI

Tests/SwiftDocCTests/Rendering/HeadingAnchorTests.swift:54: 
error: HeadingAnchorTests.testEncodeHeadingAnchor : 
XCTUnwrap failed: expected non-nil value of type "TopicRenderReference"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I have no idea. The test passes on my local Mac environment. But the test in the CI fails on both Mac and Linux.
The failed line is based on that you told me here. Can the key of renderNode.references have different values depending on the environment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reproduced this on linux docker image.
Currently all what I know is that the link which has doc:#テスト in its destination, is dropped by this guard. But it is not dropped on my local macOS.

// Before attempting to resolve the link, we confirm that is has a ResolvedTopicReference urlScheme
guard ResolvedTopicReference.urlHasResolvedTopicScheme(URL(string: destination)) else {

Because URL(string: "doc:#テスト")) returns nil on linux docker image and returns an object on local macOS.
But the result of the CI suggests it can also happen on macOS under certain conditions.

How do you think we should do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to fix that.
To my understanding, we should use ValidatedURL(parsingAuthoredLink:) rather than URL(string:) to get a url of the link destination because it is author provided documentation one.
ValidatedURL(parsingAuthoredLink:) is that already used by resolveTopicReference(_:) which is called in visitLink(_:).

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm.. I would have expected that by the time the link is rendered it would have already been "resolved" which should replace the authored destination with the url value from the ResolvedTopicReference. 🤔

In other words; I would expect the destination to have be "doc://unit-test/documentation/Root#%E3%83%86%E3%82%B9%E3%83%88" by this point instead of "doc:#テスト".

I wonder if there's a code path that doesn't do that. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it will be resolved here in same visitLink(_:)

guard let destination = link.destination, let resolved = resolveTopicReference(destination) else {

However before reaching that, the destination is checked here (28 lines above that point) and URL(string:) fails.

// Before attempting to resolve the link, we confirm that is has a ResolvedTopicReference urlScheme
guard ResolvedTopicReference.urlHasResolvedTopicScheme(URL(string: destination)) else {

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for doing that investigative work.

It looks like you've found another, unrelated, bug where links in synthesized root pages don't resolve their links in the intended manner. Unless you really want to, I don't think it's necessary to fix both bugs in the same PR.

This other bug is rather specific to links in synthesized root pages so it has a smaller impact and could be addressed separately. Anchors would still be encoded correctly—with your changes—in all other sources of documentation.

To get the test to pass you can explicitly mark it as a technology root (instead of relying on DocC to infer it as a root). To do this you'd add a @TechnologyRoot directive in the markup:

  TextFile(name: "Root.md", utf8Content: """
  # My root page

  This single article defines two headings and links to them

+ @Metadata {
+   @TechnologyRoot
+ }

  ### テスト
  - <doc:#テスト>

  ### Some heading
  - <doc:#Some-heading>
  """),

If you do this you can revert the change to treat link destinations as authored during rendering. Links are intended to always be resolved before rendering happens and I would prefer to not add any more code that relies on resolving links during rendering.

If you're curious I can go into more detail on Friday or next week.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got it! I did as you told me. 😃

XCTAssertEqual(testTopic0.url, "/documentation/root#%E3%83%86%E3%82%B9%E3%83%88")
let testTopic1 = try XCTUnwrap(renderNode.references["doc://unit-test/documentation/Root#Some-heading"] as? TopicRenderReference)
XCTAssertEqual(testTopic1.url, "/documentation/root#Some-heading")
}
}