From 30101c92e1f99867d93ac5f8d462bd8bbc68b9a7 Mon Sep 17 00:00:00 2001 From: Andrea Fernandez Buitrago <15234535+anferbui@users.noreply.github.com> Date: Thu, 30 Oct 2025 17:24:13 +0000 Subject: [PATCH] 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]: https://github.com/swiftlang/swift-docc/commit/65aaf926ec079ddbd40f29540d4180a70af99e5e --- .../ConvertActionConverter.swift | 33 ++-- .../Indexing/ExternalRenderNodeTests.swift | 146 ++++++++++++++++++ 2 files changed, 168 insertions(+), 11 deletions(-) diff --git a/Sources/SwiftDocC/Infrastructure/ConvertActionConverter.swift b/Sources/SwiftDocC/Infrastructure/ConvertActionConverter.swift index 1673f7b33d..c18d64004b 100644 --- a/Sources/SwiftDocC/Infrastructure/ConvertActionConverter.swift +++ b/Sources/SwiftDocC/Infrastructure/ConvertActionConverter.swift @@ -100,16 +100,7 @@ package enum ConvertActionConverter { let resultsSyncQueue = DispatchQueue(label: "Convert Serial Queue", qos: .unspecified, attributes: []) let resultsGroup = DispatchGroup() - - // 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) - } - + let renderSignpostHandle = signposter.beginInterval("Render", id: signposter.makeSignpostID(), "Render \(context.knownPages.count) pages") var conversionProblems: [Problem] = context.knownPages.concurrentPerform { identifier, results in @@ -172,7 +163,27 @@ package enum ConvertActionConverter { signposter.endInterval("Render", renderSignpostHandle) guard !Task.isCancelled else { return [] } - + + // Consumes all external links and adds them into the sidebar. + // This consumes all external links referenced across all content, and indexes them so they're available for reference in the navigator. + // This is not ideal as it means that links outside of the Topics section can impact the content of the navigator. + // TODO: It would be more correct to only index external links which have been curated as part of the Topics section. + // + // This has to run after all local nodes have been indexed because we're associating the external node with the **local** documentation's identifier, + // which makes it possible for there be clashes between local and external render nodes. + // When there are duplicate nodes, only the first one will be indexed, + // so in order to prefer local entities whenever there are any clashes, we have to index external nodes second. + // TODO: External render nodes should be associated with the correct documentation identifier. + try signposter.withIntervalSignpost("Index external links", id: signposter.makeSignpostID()) { + for externalLink in context.externalCache { + // Here we're associating the external node with the **local** documentation's identifier. + // This is needed because nodes are only considered children if the parent and child's identifier 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) + } + } + // Write various metadata if emitDigest { signposter.withIntervalSignpost("Emit digest", id: signposter.makeSignpostID()) { diff --git a/Tests/SwiftDocCTests/Indexing/ExternalRenderNodeTests.swift b/Tests/SwiftDocCTests/Indexing/ExternalRenderNodeTests.swift index 1d5ed2b1cc..588c979ebd 100644 --- a/Tests/SwiftDocCTests/Indexing/ExternalRenderNodeTests.swift +++ b/Tests/SwiftDocCTests/Indexing/ExternalRenderNodeTests.swift @@ -507,4 +507,150 @@ class ExternalRenderNodeTests: XCTestCase { XCTAssertEqual(objcNavigatorExternalRenderNode.metadata.title, objcTitle) XCTAssertTrue(objcNavigatorExternalRenderNode.metadata.isBeta) } + + func testExternalLinksInContentDontAffectNavigatorIndex() async throws { + let externalResolver = ExternalReferenceResolverTests.TestExternalReferenceResolver() + externalResolver.expectedReferencePath = "/documentation/testbundle/sampleclass" + + let catalog = Folder(name: "unit-test.docc", content: [ + TextFile(name: "Article.md", utf8Content: """ + # Article + + This is an internal article with an external link which clashes with the curated local link. + + External links in content should not affect the navigator. + + ## Topics + + - ``SampleClass`` + """), + TextFile(name: "SampleClass.md", utf8Content: """ + # ``SampleClass`` + + This extends the documentation for this symbol. + + ## Topics + + - + - + """), + TextFile(name: "ChildArticleA.md", utf8Content: """ + # ChildArticleA + + A child article. + """), + TextFile(name: "ChildArticleB.md", utf8Content: """ + # ChildArticleB + + A child article. + """), + // Symbol graph with a class that matches an external link path + JSONFile(name: "TestBundle.symbols.json", content: makeSymbolGraph(moduleName: "TestBundle", symbols: [ + makeSymbol(id: "some-symbol-id", language: .swift, kind: .class, pathComponents: ["SampleClass"]) + ])), + ]) + + var configuration = DocumentationContext.Configuration() + configuration.externalDocumentationConfiguration.sources[externalResolver.bundleID] = externalResolver + let (bundle, context) = try await loadBundle(catalog: catalog, configuration: configuration) + XCTAssert(context.problems.isEmpty, "Unexpectedly found problems: \(context.problems.map(\.diagnostic.summary))") + + let renderIndexFolder = try createTemporaryDirectory() + let indexBuilder = NavigatorIndex.Builder(outputURL: renderIndexFolder, bundleIdentifier: bundle.id.rawValue, sortRootChildrenByName: true, groupByLanguage: true) + indexBuilder.setup() + let outputConsumer = TestExternalRenderNodeOutputConsumer(indexBuilder: indexBuilder) + + let problems = try ConvertActionConverter.convert( + context: context, + outputConsumer: outputConsumer, + sourceRepository: nil, + emitDigest: false, + documentationCoverageOptions: .noCoverage + ) + XCTAssert(problems.isEmpty, "Unexpectedly found problems: \(DiagnosticConsoleWriter.formattedDescription(for: problems))") + indexBuilder.finalize(emitJSONRepresentation: true, emitLMDBRepresentation: false) + + XCTAssertEqual( + try RenderIndex.fromURL(renderIndexFolder.appendingPathComponent("index.json", isDirectory: false)), + try RenderIndex.fromString( + """ + { + "includedArchiveIdentifiers": [ + "unit-test" + ], + "interfaceLanguages": { + "swift": [ + { + "children": [ + { + "title": "Articles", + "type": "groupMarker" + }, + { + "children": [ + { + "children": [ + { + "path": "/documentation/unit-test/childarticlea", + "title": "ChildArticleA", + "type": "article" + }, + { + "path": "/documentation/unit-test/childarticleb", + "title": "ChildArticleB", + "type": "article" + } + ], + "path": "/documentation/testbundle/sampleclass", + "title": "SampleClass", + "type": "class" + } + ], + "path": "/documentation/unit-test/article", + "title": "Article", + "type": "symbol" + } + ], + "path": "/documentation/testbundle", + "title": "TestBundle", + "type": "module" + } + ] + }, + "schemaVersion": { + "major": 0, + "minor": 1, + "patch": 2 + } + } + """ + ) + ) + } +} + +private class TestExternalRenderNodeOutputConsumer: ConvertOutputConsumer, ExternalNodeConsumer { + let indexBuilder: Synchronized! + + init(indexBuilder: NavigatorIndex.Builder) { + self.indexBuilder = Synchronized(indexBuilder) + } + + func consume(externalRenderNode: ExternalRenderNode) throws { + try self.indexBuilder.sync { try $0.index(renderNode: externalRenderNode) } + } + + func consume(renderNode: RenderNode) throws { + try self.indexBuilder.sync { try $0.index(renderNode: renderNode) } + } + + func consume(assetsInBundle bundle: DocumentationBundle) throws { } + func consume(linkableElementSummaries: [LinkDestinationSummary]) throws { } + func consume(indexingRecords: [IndexingRecord]) throws { } + func consume(assets: [RenderReferenceType: [any RenderReference]]) throws { } + func consume(benchmarks: Benchmark) throws { } + func consume(documentationCoverageInfo: [CoverageDataEntry]) throws { } + func consume(renderReferenceStore: RenderReferenceStore) throws { } + func consume(buildMetadata: BuildMetadata) throws { } + func consume(linkResolutionInformation: SerializableLinkResolutionInformation) throws { } }