WIP: Record problem if external and local entities conflict

Author: anferbui

Sources/SwiftDocC/Indexing/Navigator/NavigatorIndex.swift

@@ -58,7 +58,9 @@ public class NavigatorIndex {
 
         /// The navigator index has not been initialized.
         case navigatorIndexIsNil
-        
+
+        case pageConflict(description: String)
+
         public var errorDescription: String {
             switch self {
             case .missingBundleIdentifier:
@@ -67,6 +69,8 @@ public class NavigatorIndex {
                 return "The page has no valid title available."
             case .navigatorIndexIsNil:
                 return "The NavigatorIndex is Nil and can't be processed."
+            case .pageConflict(let description):
+                return "The page conflicts with another page. \(description)"
             }
         }
     }
@@ -709,7 +713,18 @@ extension NavigatorIndex {
                 .normalizedNavigatorIndexIdentifier(forLanguage: language.mask)
 
             guard identifierToNode[normalizedIdentifier] == nil else {
-                return nil // skip as item exists already.
+                // Because we're associating the external node with the **current** bundle's bundle ID, rather than its real bundle ID, there can be identifier clashes with local nodes.
+                // Additionally, since the navigator index ultimately maps nodes to a relative path, paths across bundles may clash.
+                // In the long term, we should move towards using unique identifiers for pages so that there can be no clash across bundles.
+                //
+                // Here we prefer local nodes over external nodes in the index, and report these issues to the author so that they're aware of the impact on the navigator,
+                // with a suggestion of how they can workaround this issue.
+                if !identifierToNode[normalizedIdentifier]!.item.isExternal && external {
+                    let externalIdentifier = renderNode.metadata.externalID ?? renderNode.identifier.absoluteString
+                    throw Error.pageConflict(description: "External reference \(externalIdentifier.singleQuoted) resolves to the same path as a local page, \(normalizedIdentifier.path.singleQuoted), and so can't have a usable entry in the index. Whenever \(externalIdentifier.singleQuoted) is referenced in the navigator, the local page will be used instead. Replace external references to \(externalIdentifier.singleQuoted) with a local reference.")
+                } else {
+                    return nil // skip as item exists already.
+                }
             }
 
             guard let title = usePageTitle ? renderNode.metadata.title : renderNode.navigatorTitle() else {

Sources/SwiftDocC/Infrastructure/ConvertActionConverter.swift

@@ -164,14 +164,18 @@ package enum ConvertActionConverter {
 
         guard !Task.isCancelled else { return [] }
 
-        try signposter.withIntervalSignpost("Index external links", id: signposter.makeSignpostID()) {
+        // 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.
+        //
+        // This opens up possibilities of clashes between local and external links with the same navigation path.
+        // To be able to properly resolve any potential clashes, we index external links after local entities have been indexed.
+        // and record any problems caused by conflicts.
+        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)
-                try outputConsumer.consume(externalRenderNode: externalRenderNode)
+                outputConsumer.consume(externalRenderNode: externalRenderNode)
             }
         }
 

Sources/SwiftDocC/Infrastructure/ConvertOutputConsumer.swift

@@ -112,5 +112,5 @@ package struct _Deprecated<Consumer: ConvertOutputConsumer>: _DeprecatedConsumeP
 package protocol ExternalNodeConsumer {
     /// Consumes a external render node that was generated during a conversion.
     /// > Warning: This method might be called concurrently.
-    func consume(externalRenderNode: ExternalRenderNode) throws
+    func consume(externalRenderNode: ExternalRenderNode)
 }

Sources/SwiftDocCUtilities/Action/Actions/Convert/ConvertFileWritingConsumer.swift

@@ -68,7 +68,7 @@ struct ConvertFileWritingConsumer: ConvertOutputConsumer, ExternalNodeConsumer {
         indexer?.index(renderNode)
     }
 
-    func consume(externalRenderNode: ExternalRenderNode) throws {
+    func consume(externalRenderNode: ExternalRenderNode) {
         // Index the external node, if indexing is enabled.
         indexer?.index(externalRenderNode)
     }

Sources/SwiftDocCUtilities/Action/Actions/Convert/Indexer.swift

@@ -57,7 +57,7 @@ extension ConvertAction {
                 } catch {
                     self.problems.append(error.problem(source: renderNode.identifier.url,
                                                   severity: .warning,
-                                                  summaryPrefix: "RenderNode indexing process failed"))
+                                                  summaryPrefix: "Navigator indexing process for local pages failed:"))
                 }
             })
         }
@@ -73,7 +73,7 @@ extension ConvertAction {
                 } catch {
                     self.problems.append(error.problem(source: renderNode.identifier.url,
                                                   severity: .warning,
-                                                  summaryPrefix: "External render node indexing process failed"))
+                                                  summaryPrefix: "Navigator indexing process for external references failed:"))
                 }
             })
         }