From 8d4afb7ad97fe1f8a4afec7977daf6ea098a6525 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20R=C3=B6nnqvist?= Date: Wed, 4 Jun 2025 16:34:18 +0200 Subject: [PATCH 1/4] Fix false positive curation for article links with extra path components rdar://150874238 --- .../Infrastructure/DocumentationCurator.swift | 20 +++- .../DocumentationCuratorTests.swift | 102 +++++++++++++++++- 2 files changed, 117 insertions(+), 5 deletions(-) diff --git a/Sources/SwiftDocC/Infrastructure/DocumentationCurator.swift b/Sources/SwiftDocC/Infrastructure/DocumentationCurator.swift index b310940a5..6c181c30f 100644 --- a/Sources/SwiftDocC/Infrastructure/DocumentationCurator.swift +++ b/Sources/SwiftDocC/Infrastructure/DocumentationCurator.swift @@ -90,9 +90,22 @@ struct DocumentationCurator { } // Try extracting an article from the cache - let articleFilename = unresolved.topicURL.components.path.components(separatedBy: "/").last! - let sourceArticlePath = NodeURLGenerator.Path.article(bundleName: bundle.displayName, articleName: articleFilename).stringValue - + let sourceArticlePath: String = { + let path = unresolved.topicURL.components.path.removingLeadingSlash + + // The article path can either be written as + // - "ArticleName" + // - "CatalogName/ArticleName" + // - "documentation/CatalogName/ArticleName" + switch path.components(separatedBy: "/").count { + case 0,1: + return NodeURLGenerator.Path.article(bundleName: bundle.displayName, articleName: path).stringValue + case 2: + return NodeURLGenerator.Path.documentationFolder + path + default: + return path.prependingLeadingSlash + } + }() let reference = ResolvedTopicReference( bundleID: resolved.bundleID, path: sourceArticlePath, @@ -115,6 +128,7 @@ struct DocumentationCurator { context.topicGraph.addNode(curatedNode) // Move the article from the article cache to the documentation + let articleFilename = reference.url.pathComponents.last! context.linkResolver.localResolver.addArticle(filename: articleFilename, reference: reference, anchorSections: documentationNode.anchorSections) context.documentationCache[reference] = documentationNode diff --git a/Tests/SwiftDocCTests/Infrastructure/DocumentationCuratorTests.swift b/Tests/SwiftDocCTests/Infrastructure/DocumentationCuratorTests.swift index fea832171..6ffd402a2 100644 --- a/Tests/SwiftDocCTests/Infrastructure/DocumentationCuratorTests.swift +++ b/Tests/SwiftDocCTests/Infrastructure/DocumentationCuratorTests.swift @@ -1,7 +1,7 @@ /* This source file is part of the Swift.org open source project - Copyright (c) 2021-2024 Apple Inc. and the Swift project authors + Copyright (c) 2021-2025 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 @@ -232,7 +232,105 @@ class DocumentationCuratorTests: XCTestCase { XCTAssertEqual(root.path, "/documentation/Root") XCTAssertEqual(crawler.problems.count, 0) - + } + + func testCuratorDoesNotRelateNodesWhenArticleLinksContainExtraPathComponents() throws { + let (bundle, context) = try loadBundle(catalog: + Folder(name: "CatalogName.docc", content: [ + TextFile(name: "Root.md", utf8Content: """ + # Root + + @Metadata { + @TechnologyRoot + } + + Add an API Collection of indirection to more easily detect the failed curation. + + ## Topics + - + """), + + TextFile(name: "API-Collection.md", utf8Content: """ + # Some API Collection + + Fail to curate all 4 articles because of extra incorrect path components. + + ## Topics + + ### No links will resolve in this section + + - + - + - + - + """), + + TextFile(name: "First.md", utf8Content: "# First"), + TextFile(name: "Second.md", utf8Content: "# Second"), + TextFile(name: "Third.md", utf8Content: "# Third"), + TextFile(name: "Forth.md", utf8Content: "# Forth"), + ]) + ) + let (linkResolutionProblems, otherProblems) = context.problems.categorize(where: { $0.diagnostic.identifier == "org.swift.docc.unresolvedTopicReference" }) + XCTAssert(otherProblems.isEmpty, "Unexpected problems: \(otherProblems.map(\.diagnostic.summary).sorted())") + + XCTAssertEqual( + linkResolutionProblems.map(\.diagnostic.source?.lastPathComponent), + ["API-Collection.md", "API-Collection.md", "API-Collection.md", "API-Collection.md"], + "Every unresolved link is in the API collection" + ) + XCTAssertEqual( + linkResolutionProblems.map({ $0.diagnostic.range?.lowerBound.line }), [9, 10, 11, 12], + "There should be one warning about an unresolved reference for each link in the API collection's top" + ) + + let rootReference = try XCTUnwrap(context.soleRootModuleReference) + + for articleName in ["First", "Second", "Third", "Forth"] { + let reference = try XCTUnwrap(context.documentationCache.allReferences.first(where: { $0.lastPathComponent == articleName })) + XCTAssertEqual( + context.topicGraph.nodeWithReference(reference)?.shouldAutoCurateInCanonicalLocation, true, + "Article '\(articleName)' isn't (successfully) manually curated and should therefore automatically curate." + ) + XCTAssertEqual( + context.topicGraph.reverseEdges[reference]?.map(\.path), [rootReference.path], + "Article '\(articleName)' should only have a reverse edge to the root page where it will be automatically curated." + ) + } + + let apiCollectionReference = try XCTUnwrap(context.documentationCache.allReferences.first(where: { $0.lastPathComponent == "API-Collection" })) + let apiCollectionSemantic = try XCTUnwrap(try context.entity(with: apiCollectionReference).semantic as? Article) + XCTAssertEqual(apiCollectionSemantic.topics?.taskGroups.count, 1, "The API Collection has one topic section") + let topicSection = try XCTUnwrap(apiCollectionSemantic.topics?.taskGroups.first) + XCTAssertEqual(topicSection.links.map(\.destination), [ + // All these links are the same as they were authored which means that they didn't resolve. + "doc:WrongModuleName/First", + "doc:documentation/WrongModuleName/Second", + "doc:documentation/CatalogName/ExtraPathComponent/Third", + "doc:CatalogName/ExtraPathComponent/Forth", + ]) + + let rootPage = try context.entity(with: rootReference) + let renderer = DocumentationNodeConverter(bundle: bundle, context: context) + let renderNode = renderer.convert(rootPage) + + XCTAssertEqual(renderNode.topicSections.map(\.title), [ + nil, // An unnamed topic section + "Articles", // The automatic topic section + ]) + XCTAssertEqual(renderNode.topicSections.map { $0.identifiers.sorted() }, [ + // The unnamed topic section curates the API collection + [ + "doc://CatalogName/documentation/CatalogName/API-Collection" + ], + // The automatic "Articles" section curates all 4 articles + [ + "doc://CatalogName/documentation/CatalogName/First", + "doc://CatalogName/documentation/CatalogName/Forth", + "doc://CatalogName/documentation/CatalogName/Second", + "doc://CatalogName/documentation/CatalogName/Third", + ], + ]) } func testModuleUnderAncestorOfTechnologyRoot() throws { From 9a5a2c111bf64604721202f763d79799272189ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20R=C3=B6nnqvist?= Date: Wed, 4 Jun 2025 16:35:18 +0200 Subject: [PATCH 2/4] Remove explicit `.init` in test file --- .../Infrastructure/DocumentationCuratorTests.swift | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Tests/SwiftDocCTests/Infrastructure/DocumentationCuratorTests.swift b/Tests/SwiftDocCTests/Infrastructure/DocumentationCuratorTests.swift index 6ffd402a2..627c4de89 100644 --- a/Tests/SwiftDocCTests/Infrastructure/DocumentationCuratorTests.swift +++ b/Tests/SwiftDocCTests/Infrastructure/DocumentationCuratorTests.swift @@ -30,7 +30,7 @@ class DocumentationCuratorTests: XCTestCase { func testCrawl() throws { let (bundle, context) = try testBundleAndContext(named: "LegacyBundle_DoNotUseInNewTests") - var crawler = DocumentationCurator.init(in: context, bundle: bundle) + var crawler = DocumentationCurator(in: context, bundle: bundle) let mykit = try context.entity(with: ResolvedTopicReference(bundleID: "org.swift.docc.example", path: "/documentation/MyKit", sourceLanguage: .swift)) var symbolsWithCustomCuration = [ResolvedTopicReference]() @@ -219,7 +219,7 @@ class DocumentationCuratorTests: XCTestCase { """.write(to: url.appendingPathComponent("Root.md"), atomically: true, encoding: .utf8) } - let crawler = DocumentationCurator.init(in: context, bundle: bundle) + let crawler = DocumentationCurator(in: context, bundle: bundle) XCTAssert(context.problems.isEmpty, "Expected no problems. Found: \(context.problems.map(\.diagnostic.summary))") guard let moduleNode = context.documentationCache["SourceLocations"], @@ -361,7 +361,7 @@ class DocumentationCuratorTests: XCTestCase { """.write(to: url.appendingPathComponent("Ancestor.md"), atomically: true, encoding: .utf8) } - let _ = DocumentationCurator.init(in: context, bundle: bundle) + let _ = DocumentationCurator(in: context, bundle: bundle) XCTAssert(context.problems.isEmpty, "Expected no problems. Found: \(context.problems.map(\.diagnostic.summary))") guard let moduleNode = context.documentationCache["SourceLocations"], @@ -378,7 +378,7 @@ class DocumentationCuratorTests: XCTestCase { func testSymbolLinkResolving() throws { let (bundle, context) = try testBundleAndContext(named: "LegacyBundle_DoNotUseInNewTests") - let crawler = DocumentationCurator.init(in: context, bundle: bundle) + let crawler = DocumentationCurator(in: context, bundle: bundle) // Resolve top-level symbol in module parent do { @@ -431,7 +431,7 @@ class DocumentationCuratorTests: XCTestCase { func testLinkResolving() throws { let (sourceRoot, bundle, context) = try testBundleAndContext(named: "LegacyBundle_DoNotUseInNewTests") - var crawler = DocumentationCurator.init(in: context, bundle: bundle) + var crawler = DocumentationCurator(in: context, bundle: bundle) // Resolve and curate an article in module root (absolute link) do { @@ -524,7 +524,7 @@ class DocumentationCuratorTests: XCTestCase { """.write(to: root.appendingPathComponent("documentation").appendingPathComponent("api-collection.md"), atomically: true, encoding: .utf8) } - var crawler = DocumentationCurator.init(in: context, bundle: bundle) + var crawler = DocumentationCurator(in: context, bundle: bundle) let reference = ResolvedTopicReference(bundleID: "org.swift.docc.example", path: "/documentation/SideKit", sourceLanguage: .swift) try crawler.crawlChildren(of: reference, prepareForCuration: {_ in }) { (_, _) in } From 0372cca0e2f2e1ced5400f9a4766f86309002da8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20R=C3=B6nnqvist?= Date: Wed, 4 Jun 2025 16:35:38 +0200 Subject: [PATCH 3/4] Remove unused variable in test --- .../Infrastructure/DocumentationCuratorTests.swift | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Tests/SwiftDocCTests/Infrastructure/DocumentationCuratorTests.swift b/Tests/SwiftDocCTests/Infrastructure/DocumentationCuratorTests.swift index 627c4de89..fe7cd9813 100644 --- a/Tests/SwiftDocCTests/Infrastructure/DocumentationCuratorTests.swift +++ b/Tests/SwiftDocCTests/Infrastructure/DocumentationCuratorTests.swift @@ -334,7 +334,7 @@ class DocumentationCuratorTests: XCTestCase { } func testModuleUnderAncestorOfTechnologyRoot() throws { - let (_, bundle, context) = try testBundleAndContext(copying: "SourceLocations") { url in + let (_, _, context) = try testBundleAndContext(copying: "SourceLocations") { url in try """ # Root with ancestor curating a module @@ -361,7 +361,6 @@ class DocumentationCuratorTests: XCTestCase { """.write(to: url.appendingPathComponent("Ancestor.md"), atomically: true, encoding: .utf8) } - let _ = DocumentationCurator(in: context, bundle: bundle) XCTAssert(context.problems.isEmpty, "Expected no problems. Found: \(context.problems.map(\.diagnostic.summary))") guard let moduleNode = context.documentationCache["SourceLocations"], From 7ddb33c5b5a586be391c4a0f009205bc76bd4d1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20R=C3=B6nnqvist?= Date: Thu, 12 Jun 2025 11:09:06 +0200 Subject: [PATCH 4/4] Add missing path separator in curator fallback code path --- Sources/SwiftDocC/Infrastructure/DocumentationCurator.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/SwiftDocC/Infrastructure/DocumentationCurator.swift b/Sources/SwiftDocC/Infrastructure/DocumentationCurator.swift index 6c181c30f..151996567 100644 --- a/Sources/SwiftDocC/Infrastructure/DocumentationCurator.swift +++ b/Sources/SwiftDocC/Infrastructure/DocumentationCurator.swift @@ -101,7 +101,7 @@ struct DocumentationCurator { case 0,1: return NodeURLGenerator.Path.article(bundleName: bundle.displayName, articleName: path).stringValue case 2: - return NodeURLGenerator.Path.documentationFolder + path + return "\(NodeURLGenerator.Path.documentationFolder)/\(path)" default: return path.prependingLeadingSlash }