Skip to content

Commit a92348d

Browse files
anferbuianferbui
committed
Index external entities after local entities
The navigation indexing code skips all further nodes after it has already indexed a node with a given identifier [1]. This can result in an odd interaction when there is an external link which is resolved to a path that the bundle serves itself (or more colloquially, a catalog has an external link to itself). Since external entities are indexed before local entities, they were always be preferred over local entities, resulting in local entities mistakenly being dropped from the navigator altogether. The resulting navigator is incorrect and missing nodes, due to external entities not having hierarchy information. This issue was introduced when support for external links in the navigator was introduced. This commit updates `ConvertActionConverter` to always index local entities first. If any external entities clash with local entities, they will be resolved as the local entity (including its hierarchy) in the navigator. Fixes rdar://163018922. [1]: https://github.com/swiftlang/swift-docc/blob/b27288dd99b0e2715ed1a2d5720cd0f23118c030/Sources/SwiftDocC/Indexing/Navigator/NavigatorIndex.swift#L711-L713 [2]: 65aaf92
1 parent 7b74f5c commit a92348d

File tree

2 files changed

+198
-11
lines changed

2 files changed

+198
-11
lines changed

‎Sources/SwiftDocC/Infrastructure/ConvertActionConverter.swift‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -100,16 +100,7 @@ package enum ConvertActionConverter {
100100

101101
let resultsSyncQueue = DispatchQueue(label: "Convert Serial Queue", qos: .unspecified, attributes: [])
102102
let resultsGroup = DispatchGroup()
103-
104-
// Consume external links and add them into the sidebar.
105-
for externalLink in context.externalCache {
106-
// Here we're associating the external node with the **current** bundle's bundle ID.
107-
// This is needed because nodes are only considered children if the parent and child's bundle ID match.
108-
// Otherwise, the node will be considered as a separate root node and displayed separately.
109-
let externalRenderNode = ExternalRenderNode(externalEntity: externalLink.value, bundleIdentifier: context.inputs.id)
110-
try outputConsumer.consume(externalRenderNode: externalRenderNode)
111-
}
112-
103+
113104
let renderSignpostHandle = signposter.beginInterval("Render", id: signposter.makeSignpostID(), "Render \(context.knownPages.count) pages")
114105

115106
var conversionProblems: [Problem] = context.knownPages.concurrentPerform { identifier, results in
@@ -172,7 +163,18 @@ package enum ConvertActionConverter {
172163
signposter.endInterval("Render", renderSignpostHandle)
173164

174165
guard !Task.isCancelled else { return [] }
175-
166+
167+
try signposter.withIntervalSignpost("Index external links", id: signposter.makeSignpostID()) {
168+
// Consume external links and add them into the sidebar.
169+
for externalLink in context.externalCache {
170+
// Here we're associating the external node with the **current** bundle's bundle ID.
171+
// This is needed because nodes are only considered children if the parent and child's bundle ID match.
172+
// Otherwise, the node will be considered as a separate root node and displayed separately.
173+
let externalRenderNode = ExternalRenderNode(externalEntity: externalLink.value, bundleIdentifier: context.inputs.id)
174+
try outputConsumer.consume(externalRenderNode: externalRenderNode)
175+
}
176+
}
177+
176178
// Write various metadata
177179
if emitDigest {
178180
signposter.withIntervalSignpost("Emit digest", id: signposter.makeSignpostID()) {

‎Tests/SwiftDocCUtilitiesTests/ConvertActionTests.swift‎

Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3186,6 +3186,179 @@ class ConvertActionTests: XCTestCase {
31863186
"/images/unit-test/image-name~dark@2x.png"
31873187
])
31883188
}
3189+
3190+
func testConflictingLocalAndExternalLinksInNavigatorIndex() async throws {
3191+
let temporaryFolder = try createTemporaryDirectory()
3192+
3193+
let resolver: OutOfProcessReferenceResolver
3194+
do {
3195+
let executableLocation = temporaryFolder.appendingPathComponent("link-resolver-executable")
3196+
3197+
let externalBundleID = "com.external.test"
3198+
let externalSummary = LinkDestinationSummary(
3199+
kind: .class,
3200+
language: .swift,
3201+
relativePresentationURL: URL(string: "/documentation/TestBundle/SampleClass")!,
3202+
referenceURL: URL(string: "doc://\(externalBundleID)/documentation/TestBundle/SampleClass")!,
3203+
title: "SampleClass",
3204+
abstract: [.text("External abstract")],
3205+
availableLanguages: [.swift],
3206+
references: [],
3207+
variants: []
3208+
)
3209+
let encodedResponse = try String(decoding: JSONEncoder().encode(OutOfProcessReferenceResolver.ResponseV2.resolved(externalSummary)), as: UTF8.self)
3210+
3211+
try """
3212+
#!/bin/bash
3213+
echo '{"bundleIdentifier":"\(externalBundleID)","capabilities":0}' # Write this resolver's bundle identifier
3214+
read # Wait for docc to send a symbol USR
3215+
echo '\(encodedResponse)' # Respond with the resolved link summary
3216+
""".write(to: executableLocation, atomically: true, encoding: .utf8)
3217+
3218+
// `0o0700` is `-rwx------` (read, write, & execute only for owner)
3219+
try FileManager.default.setAttributes([.posixPermissions: 0o0700], ofItemAtPath: executableLocation.path)
3220+
XCTAssert(FileManager.default.isExecutableFile(atPath: executableLocation.path))
3221+
3222+
resolver = try OutOfProcessReferenceResolver(processLocation: executableLocation, errorOutputHandler: { _ in })
3223+
}
3224+
3225+
let catalog = Folder(name: "unit-test.docc", content: [
3226+
InfoPlist(displayName: "TestBundle", identifier: "com.test.example"),
3227+
TextFile(name: "Article.md", utf8Content: """
3228+
# Article
3229+
3230+
This is an internal article with an external link which resolves to an internal symbol.
3231+
3232+
## Topics
3233+
3234+
- <doc://com.external.test/documentation/TestBundle/SampleClass>
3235+
"""),
3236+
TextFile(name: "SampleClass.md", utf8Content: """
3237+
# ``SampleClass``
3238+
3239+
This extends the documentation for this symbol.
3240+
3241+
## Topics
3242+
3243+
- <doc:ChildArticleA>
3244+
- <doc:ChildArticleB>
3245+
"""),
3246+
TextFile(name: "ChildArticleA.md", utf8Content: """
3247+
# ChildArticleA
3248+
3249+
A child article.
3250+
"""),
3251+
TextFile(name: "ChildArticleB.md", utf8Content: """
3252+
# ChildArticleB
3253+
3254+
A child article.
3255+
"""),
3256+
// Symbol graph with a class that matches an external link path
3257+
JSONFile(name: "TestBundle.symbols.json", content: makeSymbolGraph(moduleName: "TestBundle", symbols: [
3258+
makeSymbol(id: "some-symbol-id", language: .swift, kind: .class, pathComponents: ["SampleClass"])
3259+
])),
3260+
])
3261+
3262+
let catalogURL = try catalog.write(inside: temporaryFolder)
3263+
let targetURL = temporaryFolder.appendingPathComponent("Output.doccarchive", isDirectory: true)
3264+
3265+
let action = try ConvertAction(
3266+
documentationBundleURL: catalogURL,
3267+
outOfProcessResolver: resolver,
3268+
analyze: false,
3269+
targetDirectory: targetURL,
3270+
htmlTemplateDirectory: nil,
3271+
emitDigest: false,
3272+
currentPlatforms: nil,
3273+
fileManager: FileManager.default,
3274+
temporaryDirectory: createTemporaryDirectory()
3275+
)
3276+
3277+
let result = try await action.perform(logHandle: .none)
3278+
3279+
XCTAssertEqual(result.outputs, [targetURL])
3280+
3281+
let renderIndexURL = targetURL.appendingPathComponent("index", isDirectory: true)
3282+
.appendingPathComponent("index.json", isDirectory: false)
3283+
3284+
XCTAssertEqual(
3285+
try RenderIndex.fromURL(renderIndexURL),
3286+
try RenderIndex.fromString(
3287+
"""
3288+
{
3289+
"includedArchiveIdentifiers": [
3290+
"com.test.example"
3291+
],
3292+
"interfaceLanguages": {
3293+
"swift": [
3294+
{
3295+
"children": [
3296+
{
3297+
"title": "Articles",
3298+
"type": "groupMarker"
3299+
},
3300+
{
3301+
"children": [
3302+
{
3303+
"children": [
3304+
{
3305+
"path": "/documentation/testbundle/childarticlea",
3306+
"title": "ChildArticleA",
3307+
"type": "article"
3308+
},
3309+
{
3310+
"path": "/documentation/testbundle/childarticleb",
3311+
"title": "ChildArticleB",
3312+
"type": "article"
3313+
}
3314+
],
3315+
"path": "/documentation/testbundle/sampleclass",
3316+
"title": "SampleClass",
3317+
"type": "class"
3318+
}
3319+
],
3320+
"path": "/documentation/testbundle/article",
3321+
"title": "Article",
3322+
"type": "symbol"
3323+
},
3324+
{
3325+
"title": "Classes",
3326+
"type": "groupMarker"
3327+
},
3328+
{
3329+
"children": [
3330+
{
3331+
"path": "/documentation/testbundle/childarticlea",
3332+
"title": "ChildArticleA",
3333+
"type": "article"
3334+
},
3335+
{
3336+
"path": "/documentation/testbundle/childarticleb",
3337+
"title": "ChildArticleB",
3338+
"type": "article"
3339+
}
3340+
],
3341+
"path": "/documentation/testbundle/sampleclass",
3342+
"title": "SampleClass",
3343+
"type": "class"
3344+
}
3345+
],
3346+
"path": "/documentation/testbundle",
3347+
"title": "TestBundle",
3348+
"type": "module"
3349+
}
3350+
]
3351+
},
3352+
"schemaVersion": {
3353+
"major": 0,
3354+
"minor": 1,
3355+
"patch": 2
3356+
}
3357+
}
3358+
"""
3359+
)
3360+
)
3361+
}
31893362

31903363
#endif
31913364
}
@@ -3280,3 +3453,15 @@ private extension ConvertAction {
32803453
return try await perform(logHandle: &logHandle)
32813454
}
32823455
}
3456+
3457+
extension RenderIndex {
3458+
static func fromString(_ string: String) throws -> RenderIndex {
3459+
let decoder = JSONDecoder()
3460+
return try decoder.decode(RenderIndex.self, from: Data(string.utf8))
3461+
}
3462+
3463+
static func fromURL(_ url: URL) throws -> RenderIndex? {
3464+
let data = try Data(contentsOf: url)
3465+
return try JSONDecoder().decode(RenderIndex.self, from: data)
3466+
}
3467+
}

0 commit comments

Comments
 (0)