Disambiguate external and local links by bundle ID

External and local links with the same path were colliding when building the navigator due to using the same bundle identifier to represent both, regardless of the real bundle identifier of the external link. [1]

This commit updates the navigation indexing logic to respect the real bundle identifiers of the links, which should remove the chance of collisions between local and external links when building the navigator.

Fixes rdar://163018922.

[1]: https://github.com/swiftlang/swift-docc/blob/e15f4e10e60c1f554e1c27912804ab7272dbfaec/Sources/SwiftDocC/Infrastructure/ConvertActionConverter.swift#L106-L108

Author: anferbui

Sources/SwiftDocC/Indexing/Navigator/NavigatorIndex.swift

@@ -792,8 +792,8 @@ extension NavigatorIndex {
                 navigationItem.usrIdentifier =  language.name + "-" + ExternalIdentifier.usr(usr).hash // We pair the hash and the language name
             }
 
-            let navigatorNode = NavigatorTree.Node(item: navigationItem, bundleIdentifier: bundleIdentifier)
-            
+            let navigatorNode = NavigatorTree.Node(item: navigationItem, bundleIdentifier: renderNode.identifier.bundleID.rawValue)
+
             // Process the children
             var children = [Identifier]()
             for (index, child) in renderNode.navigatorChildren(for: traits).enumerated() {
@@ -820,8 +820,8 @@ extension NavigatorIndex {
 
                     groupItem.path = identifier.path + "#" + fragment
 
-                    let navigatorGroup = NavigatorTree.Node(item: groupItem, bundleIdentifier: bundleIdentifier)
-                    
+                    let navigatorGroup = NavigatorTree.Node(item: groupItem, bundleIdentifier: renderNode.identifier.bundleID.rawValue)
+
                     identifierToNode[identifier] = navigatorGroup
                     children.append(identifier)
 
@@ -831,8 +831,12 @@ extension NavigatorIndex {
                 }
 
                 let identifiers = child.references.map { reference in
+                    let bundleIdentifier = reference.identifier.identifier
+                        .dropFirst(6) // Drop the `doc://` prefix
+                        .prefix(while: { $0 != "/" }) // Grab just the bundle identifier (the host component)
+                        .lowercased()
                     return Identifier(
-                        bundleIdentifier: bundleIdentifier.lowercased(),
+                        bundleIdentifier: bundleIdentifier,
                         path: reference.url.lowercased(),
                         languageIdentifier: language.mask
                     )

Sources/SwiftDocC/Infrastructure/ConvertActionConverter.swift

@@ -166,11 +166,8 @@ package enum ConvertActionConverter {
 
         try signposter.withIntervalSignpost("Index external links", id: signposter.makeSignpostID()) {
             // Consume external links and add them into the sidebar.
-            for externalLink in context.externalCache {
-                // Here we're associating the external node with the **current** bundle's bundle ID.
-                // This is needed because nodes are only considered children if the parent and child's bundle ID match.
-                // Otherwise, the node will be considered as a separate root node and displayed separately.
-                let externalRenderNode = ExternalRenderNode(externalEntity: externalLink.value, bundleIdentifier: context.inputs.id)
+            for (_, externalLink) in context.externalCache {
+                let externalRenderNode = ExternalRenderNode(externalEntity: externalLink)
                 try outputConsumer.consume(externalRenderNode: externalRenderNode)
             }
         }

Sources/SwiftDocC/Infrastructure/Link Resolution/LinkResolver+NavigatorIndex.swift

@@ -15,25 +15,16 @@ import SymbolKit
 package struct ExternalRenderNode {
     private var entity: LinkResolver.ExternalEntity
     private var topicRenderReference:  TopicRenderReference
-    
-    /// The bundle identifier for this external node.
-    private var bundleIdentifier: DocumentationBundle.Identifier
 
-    // This type is designed to misrepresent external content as local content to fit in with the navigator.
-    // This spreads the issue to more code rather than fixing it, which adds technical debt and can be fragile.
-    //
-    // At the time of writing this comment, this type and the issues it comes with has spread to 6 files (+ 3 test files).
-    // Luckily, none of that code is public API so we can modify or even remove it without compatibility restrictions.
-    init(externalEntity: LinkResolver.ExternalEntity, bundleIdentifier: DocumentationBundle.Identifier) {
+    init(externalEntity: LinkResolver.ExternalEntity) {
         self.entity = externalEntity
-        self.bundleIdentifier = bundleIdentifier
         self.topicRenderReference = externalEntity.makeTopicRenderReference()
     }
 
     /// The identifier of the external render node.
     package var identifier: ResolvedTopicReference {
         ResolvedTopicReference(
-            bundleID: bundleIdentifier,
+            bundleID: .init(rawValue: entity.referenceURL.host!),
             path: entity.referenceURL.path,
             fragment: entity.referenceURL.fragment,
             sourceLanguages: entity.availableLanguages

Tests/SwiftDocCTests/Indexing/ExternalRenderNodeTests.swift

@@ -128,32 +128,32 @@ class ExternalRenderNodeTests: XCTestCase {
         }
 
         let externalRenderNodes = context.externalCache.valuesByReference.values.map {
-            ExternalRenderNode(externalEntity: $0, bundleIdentifier: bundle.id)
+            ExternalRenderNode(externalEntity: $0)
         }.sorted(by: \.titleVariants.defaultValue)
         XCTAssertEqual(externalRenderNodes.count, 4)
 
-        XCTAssertEqual(externalRenderNodes[0].identifier.absoluteString, "doc://org.swift.MixedLanguageFramework/path/to/external/objCArticle")
+        XCTAssertEqual(externalRenderNodes[0].identifier.absoluteString, "doc://com.test.external/path/to/external/objCArticle")
         XCTAssertEqual(externalRenderNodes[0].kind, .article)
         XCTAssertEqual(externalRenderNodes[0].symbolKind, nil)
         XCTAssertEqual(externalRenderNodes[0].role, "article")
         XCTAssertEqual(externalRenderNodes[0].externalIdentifier.identifier, "doc://com.test.external/path/to/external/objCArticle")
         XCTAssertTrue(externalRenderNodes[0].isBeta)
 
-        XCTAssertEqual(externalRenderNodes[1].identifier.absoluteString, "doc://org.swift.MixedLanguageFramework/path/to/external/objCSymbol")
+        XCTAssertEqual(externalRenderNodes[1].identifier.absoluteString, "doc://com.test.external/path/to/external/objCSymbol")
         XCTAssertEqual(externalRenderNodes[1].kind, .symbol)
         XCTAssertEqual(externalRenderNodes[1].symbolKind, .func)
         XCTAssertEqual(externalRenderNodes[1].role, "symbol")
         XCTAssertEqual(externalRenderNodes[1].externalIdentifier.identifier, "doc://com.test.external/path/to/external/objCSymbol")
         XCTAssertFalse(externalRenderNodes[1].isBeta)
 
-        XCTAssertEqual(externalRenderNodes[2].identifier.absoluteString, "doc://org.swift.MixedLanguageFramework/path/to/external/swiftArticle")
+        XCTAssertEqual(externalRenderNodes[2].identifier.absoluteString, "doc://com.test.external/path/to/external/swiftArticle")
         XCTAssertEqual(externalRenderNodes[2].kind, .article)
         XCTAssertEqual(externalRenderNodes[2].symbolKind, nil)
         XCTAssertEqual(externalRenderNodes[2].role, "article")
         XCTAssertEqual(externalRenderNodes[2].externalIdentifier.identifier, "doc://com.test.external/path/to/external/swiftArticle")
         XCTAssertFalse(externalRenderNodes[2].isBeta)
 
-        XCTAssertEqual(externalRenderNodes[3].identifier.absoluteString, "doc://org.swift.MixedLanguageFramework/path/to/external/swiftSymbol")
+        XCTAssertEqual(externalRenderNodes[3].identifier.absoluteString, "doc://com.test.external/path/to/external/swiftSymbol")
         XCTAssertEqual(externalRenderNodes[3].kind, .symbol)
         XCTAssertEqual(externalRenderNodes[3].symbolKind, .class)
         XCTAssertEqual(externalRenderNodes[3].role, "symbol")
@@ -190,10 +190,7 @@ class ExternalRenderNodeTests: XCTestCase {
                 )
             ]
         )
-        let externalRenderNode = ExternalRenderNode(
-            externalEntity: externalEntity,
-            bundleIdentifier: "com.test.external"
-        )
+        let externalRenderNode = ExternalRenderNode(externalEntity: externalEntity)
 
         let swiftNavigatorExternalRenderNode = try XCTUnwrap(
             NavigatorExternalRenderNode(renderNode: externalRenderNode)
@@ -252,8 +249,8 @@ class ExternalRenderNodeTests: XCTestCase {
         let targetURL = try createTemporaryDirectory()
         let builder = NavigatorIndex.Builder(outputURL: targetURL, bundleIdentifier: context.inputs.id.rawValue, sortRootChildrenByName: true, groupByLanguage: true)
         builder.setup()
-        for externalLink in context.externalCache {
-            let externalRenderNode = ExternalRenderNode(externalEntity: externalLink.value, bundleIdentifier: context.inputs.id)
+        for (_, externalLink) in context.externalCache {
+            let externalRenderNode = ExternalRenderNode(externalEntity: externalLink)
             try builder.index(renderNode: externalRenderNode)
         }
         for identifier in context.knownPages {
@@ -355,8 +352,8 @@ class ExternalRenderNodeTests: XCTestCase {
         let targetURL = try createTemporaryDirectory()
         let builder = NavigatorIndex.Builder(outputURL: targetURL, bundleIdentifier: context.inputs.id.rawValue, sortRootChildrenByName: true, groupByLanguage: true)
         builder.setup()
-        for externalLink in context.externalCache {
-            let externalRenderNode = ExternalRenderNode(externalEntity: externalLink.value, bundleIdentifier: context.inputs.id)
+        for (_, externalLink) in context.externalCache {
+            let externalRenderNode = ExternalRenderNode(externalEntity: externalLink)
             try builder.index(renderNode: externalRenderNode)
         }
         for identifier in context.knownPages {
@@ -433,8 +430,8 @@ class ExternalRenderNodeTests: XCTestCase {
         let targetURL = try createTemporaryDirectory()
         let builder = NavigatorIndex.Builder(outputURL: targetURL, bundleIdentifier: context.inputs.id.rawValue, sortRootChildrenByName: true, groupByLanguage: true)
         builder.setup()
-        for externalLink in context.externalCache {
-            let externalRenderNode = ExternalRenderNode(externalEntity: externalLink.value, bundleIdentifier: context.inputs.id)
+        for (_, externalLink) in context.externalCache {
+            let externalRenderNode = ExternalRenderNode(externalEntity: externalLink)
             try builder.index(renderNode: externalRenderNode)
         }
         for identifier in context.knownPages {
@@ -490,10 +487,7 @@ class ExternalRenderNodeTests: XCTestCase {
                 )
             ]
         )
-        let externalRenderNode = ExternalRenderNode(
-            externalEntity: externalEntity,
-            bundleIdentifier: "com.test.external"
-        )
+        let externalRenderNode = ExternalRenderNode(externalEntity: externalEntity)
 
         let swiftNavigatorExternalRenderNode = try XCTUnwrap(
             NavigatorExternalRenderNode(renderNode: externalRenderNode)

Tests/SwiftDocCUtilitiesTests/ConvertActionTests.swift

@@ -3300,21 +3300,10 @@ class ConvertActionTests: XCTestCase {
                           {
                             "children": [
                               {
-                                "children": [
-                                  {
-                                    "path": "/documentation/testbundle/childarticlea",
-                                    "title": "ChildArticleA",
-                                    "type": "article"
-                                  },
-                                  {
-                                    "path": "/documentation/testbundle/childarticleb",
-                                    "title": "ChildArticleB",
-                                    "type": "article"
-                                  }
-                                ],
                                 "path": "/documentation/testbundle/sampleclass",
                                 "title": "SampleClass",
-                                "type": "class"
+                                "type": "class",
+                                "external": true
                               }
                             ],
                             "path": "/documentation/testbundle/article",