Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that bundle identifiers are valid URL host components #1069

Merged
merged 21 commits into from
Nov 21, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
28b9522
Add bundle identifier type that ensures a valid URL host component
d-ronnqvist Oct 22, 2024
0e62ffc
Use new Identifier type in `DocumentationBundle/Info`
d-ronnqvist Oct 22, 2024
b46b5d3
Update code to not use deprecated `Info/identifier` property
d-ronnqvist Oct 22, 2024
bfa673b
Use new Identifier type in `DocumentationBundle`
d-ronnqvist Oct 22, 2024
98c6a58
Update code to not use deprecated `identifier` property
d-ronnqvist Oct 22, 2024
399ed9a
Use new Identifier type in `[Unresolved|Resolved]TopicReference`
d-ronnqvist Oct 22, 2024
a7310b7
Update code to not use deprecated topic reference `bundleIdentifier` …
d-ronnqvist Oct 22, 2024
221891a
Deprecate `BundleIdentifier` in favor of `DocumentationBundle/Identif…
d-ronnqvist Oct 22, 2024
890e323
Use new Identifier type in `BuildMetadata`
d-ronnqvist Oct 22, 2024
3c56a2d
Use new Identifier type in `AssetReference`
d-ronnqvist Oct 22, 2024
13b11fb
Use new Identifier type in `ResourceReference`
d-ronnqvist Oct 22, 2024
414ef9c
Use new Identifier type in `ConvertServiceFallbackResolver`
d-ronnqvist Oct 22, 2024
61cc5e2
Use new Identifier type in `SerializableLinkResolutionInformation`
d-ronnqvist Oct 22, 2024
5369b1c
Use new Identifier type in `ConvertAction/Indexer`
d-ronnqvist Oct 22, 2024
4805a05
Prefer `bundleID` over `id` for types that scoped inside bundle
d-ronnqvist Oct 22, 2024
0f2f8ef
Merge branch 'main' into validate-bundle-identifier
d-ronnqvist Oct 31, 2024
c2442bc
Merge branch 'main' into validate-bundle-identifier
d-ronnqvist Nov 11, 2024
71b27b6
Merge branch 'main' into validate-bundle-identifier
d-ronnqvist Nov 19, 2024
3d063ef
Merge branch 'main' into validate-bundle-identifier
d-ronnqvist Nov 21, 2024
f97e6d3
Use `bundleID` instead of `id` for property names outside the bundle
d-ronnqvist Nov 21, 2024
9478d47
Use "bundle id" in local variable in fallback resolver
d-ronnqvist Nov 21, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ public struct ConvertService: DocumentationService {

if let linkResolvingServer {
let resolver = try OutOfProcessReferenceResolver(
bundleIdentifier: request.bundleInfo.identifier,
id: request.bundleInfo.id,
server: linkResolvingServer,
convertRequestIdentifier: messageIdentifier
)
Expand Down
8 changes: 4 additions & 4 deletions Sources/SwiftDocC/Infrastructure/DocumentationBundle.swift
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public struct DocumentationBundle {
An identifier string that specifies the app type of the bundle. The string should be in reverse DNS format using only the Roman alphabet in upper and lower case (A–Z, a–z), the dot (“.”), and the hyphen (“-”).
*/
public var identifier: String {
info.identifier
info.id.rawValue
}

/**
Expand Down Expand Up @@ -126,9 +126,9 @@ public struct DocumentationBundle {
self.customHeader = customHeader
self.customFooter = customFooter
self.themeSettings = themeSettings
self.rootReference = ResolvedTopicReference(bundleIdentifier: info.identifier, path: "/", sourceLanguage: .swift)
self.documentationRootReference = ResolvedTopicReference(bundleIdentifier: info.identifier, path: NodeURLGenerator.Path.documentationFolder, sourceLanguage: .swift)
self.tutorialTableOfContentsContainer = ResolvedTopicReference(bundleIdentifier: info.identifier, path: NodeURLGenerator.Path.tutorialsFolder, sourceLanguage: .swift)
self.rootReference = ResolvedTopicReference(bundleIdentifier: info.id.rawValue, path: "/", sourceLanguage: .swift)
self.documentationRootReference = ResolvedTopicReference(bundleIdentifier: info.id.rawValue, path: NodeURLGenerator.Path.documentationFolder, sourceLanguage: .swift)
self.tutorialTableOfContentsContainer = ResolvedTopicReference(bundleIdentifier: info.id.rawValue, path: NodeURLGenerator.Path.tutorialsFolder, sourceLanguage: .swift)
self.tutorialsContainerReference = tutorialTableOfContentsContainer.appendingPath(urlReadablePath(info.displayName))
self.articlesDocumentationRootReference = documentationRootReference.appendingPath(urlReadablePath(info.displayName))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,13 @@ import SymbolKit
public class OutOfProcessReferenceResolver: ExternalDocumentationSource, GlobalExternalSymbolResolver {
private let externalLinkResolvingClient: ExternalLinkResolving

@available(*, deprecated, renamed: "id", message: "Use 'id' instead. This deprecated API will be removed after 6.2 is released")
public var bundleIdentifier: String {
id.rawValue
}

/// The bundle identifier for the reference resolver in the other process.
public let bundleIdentifier: String
public let id: DocumentationBundle.Identifier
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: rename this to `bundleIdentifier.

Naming this id can be confusing when invoking the property like outOfProcessReferenceResolver.id since it might convey that this is calling the identifier of the reference resolver process.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't name it bundleIdentifier because there's already a property with that name but I can name it bundleID.

Like I said in this comment I'm planning on deprecating the term "bundle" because it's causing confusion and I was trying to avoid needing to introduce and deprecate a public API in the same release but I don't think there's would be too many places like this, so perhaps that's not too bad.


/// Creates a new reference resolver that interacts with another executable.
///
Expand All @@ -77,20 +82,27 @@ public class OutOfProcessReferenceResolver: ExternalDocumentationSource, GlobalE
throw Error.invalidBundleIdentifierOutputFromExecutable(processLocation)
}

self.bundleIdentifier = decodedBundleIdentifier
self.id = .init(rawValue: decodedBundleIdentifier)
self.externalLinkResolvingClient = longRunningProcess
}

@available(*, deprecated, renamed: "init(id:server:convertRequestIdentifier:)", message: "Use 'init(id:server:convertRequestIdentifier:)' instead. This deprecated API will be removed after 6.2 is released")
public init(bundleIdentifier: String, server: DocumentationServer, convertRequestIdentifier: String?) throws {
self.id = .init(rawValue: bundleIdentifier)
self.externalLinkResolvingClient = LongRunningService(
server: server, convertRequestIdentifier: convertRequestIdentifier)
}

/// Creates a new reference resolver that interacts with a documentation service.
///
/// The documentation service is expected to be able to handle messages of kind "resolve-reference".
///
/// - Parameters:
/// - bundleIdentifier: The bundle identifier the server can resolve references for.
/// - id: The bundle identifier the server can resolve references for.
/// - server: The server to send link resolution requests to.
/// - convertRequestIdentifier: The identifier that the resolver will use for convert requests that it sends to the server.
public init(bundleIdentifier: String, server: DocumentationServer, convertRequestIdentifier: String?) throws {
self.bundleIdentifier = bundleIdentifier
public init(id: DocumentationBundle.Identifier, server: DocumentationServer, convertRequestIdentifier: String?) throws {
self.id = id
self.externalLinkResolvingClient = LongRunningService(
server: server, convertRequestIdentifier: convertRequestIdentifier)
}
Expand All @@ -103,7 +115,7 @@ public class OutOfProcessReferenceResolver: ExternalDocumentationSource, GlobalE
return resolved

case let .unresolved(unresolvedReference):
guard unresolvedReference.bundleIdentifier == bundleIdentifier else {
guard unresolvedReference.bundleIdentifier == id.rawValue else {
fatalError("""
Attempted to resolve a local reference externally: \(unresolvedReference.description.singleQuoted).
DocC should never pass a reference to an external resolver unless it matches that resolver's bundle identifier.
Expand Down Expand Up @@ -243,7 +255,7 @@ public class OutOfProcessReferenceResolver: ExternalDocumentationSource, GlobalE

private func resolvedReference(for resolvedInformation: ResolvedInformation) -> ResolvedTopicReference {
return ResolvedTopicReference(
bundleIdentifier: bundleIdentifier,
bundleIdentifier: id.rawValue,
path: resolvedInformation.url.path,
fragment: resolvedInformation.url.fragment,
sourceLanguages: sourceLanguages(for: resolvedInformation)
Expand Down Expand Up @@ -767,13 +779,13 @@ extension OutOfProcessReferenceResolver: ConvertServiceFallbackResolver {
}

func resolveInformationForAsset(named assetName: String) throws -> DataAsset {
let assetReference = AssetReference(assetName: assetName, bundleIdentifier: bundleIdentifier)
let assetReference = AssetReference(assetName: assetName, bundleIdentifier: id.rawValue)
if let asset = assetCache[assetReference] {
return asset
}

let response = try externalLinkResolvingClient.sendAndWait(
request: Request.asset(AssetReference(assetName: assetName, bundleIdentifier: bundleIdentifier))
request: Request.asset(AssetReference(assetName: assetName, bundleIdentifier: id.rawValue))
) as Response

switch response {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ public struct ConvertAction: AsyncAction {
}

if let outOfProcessResolver {
configuration.externalDocumentationConfiguration.sources[outOfProcessResolver.bundleIdentifier] = outOfProcessResolver
configuration.externalDocumentationConfiguration.sources[outOfProcessResolver.id.rawValue] = outOfProcessResolver
configuration.externalDocumentationConfiguration.globalSymbolResolver = outOfProcessResolver
}
configuration.externalDocumentationConfiguration.dependencyArchives = dependencies
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import SwiftDocCTestUtilities
class ConvertServiceTests: XCTestCase {
private let testBundleInfo = DocumentationBundle.Info(
displayName: "TestBundle",
identifier: "identifier"
id: "identifier"
)

func testConvertSinglePage() throws {
Expand Down Expand Up @@ -1730,7 +1730,7 @@ class ConvertServiceTests: XCTestCase {
let request = ConvertRequest(
bundleInfo: DocumentationBundle.Info(
displayName: "TestBundle",
identifier: "com.test.bundle"
id: "com.test.bundle"
),
externalIDsToConvert: ["s:5MyKit0A5ClassC10myFunctionyyF"],
documentPathsToConvert: [],
Expand Down Expand Up @@ -2019,7 +2019,7 @@ class ConvertServiceTests: XCTestCase {
let request = ConvertRequest(
bundleInfo: DocumentationBundle.Info(
displayName: "TestBundle",
identifier: "org.swift.example"
id: "org.swift.example"
),
externalIDsToConvert: ["s:32MyKit3FooV"],
documentPathsToConvert: [],
Expand Down Expand Up @@ -2128,7 +2128,7 @@ class ConvertServiceTests: XCTestCase {
let request = ConvertRequest(
bundleInfo: DocumentationBundle.Info(
displayName: "TestBundleDisplayName",
identifier: "com.test.bundle"
id: "com.test.bundle"
),
externalIDsToConvert: ["s:21SmallTestingFramework40EnumerationWithSingleUnresolvableDocLinkO"],
documentPathsToConvert: [],
Expand Down Expand Up @@ -2166,7 +2166,7 @@ class ConvertServiceTests: XCTestCase {
let request = ConvertRequest(
bundleInfo: DocumentationBundle.Info(
displayName: "TestBundleDisplayName",
identifier: "com.test.bundle"
id: "com.test.bundle"
),
externalIDsToConvert: ["s:21SmallTestingFramework15TestEnumerationO06NesteddE0O0D6StructV06deeplyfD31FunctionWithUnresolvableDocLinkyyF"],
documentPathsToConvert: [],
Expand Down Expand Up @@ -2205,7 +2205,7 @@ class ConvertServiceTests: XCTestCase {
let request = ConvertRequest(
bundleInfo: DocumentationBundle.Info(
displayName: "TestBundleDisplayName",
identifier: "com.test.bundle"
id: "com.test.bundle"
),
externalIDsToConvert: ["s:21SmallTestingFramework43EnumerationWithSingleUnresolvableSymbolLinkO"],
documentPathsToConvert: [],
Expand Down Expand Up @@ -2331,7 +2331,7 @@ class ConvertServiceTests: XCTestCase {
let bundleURL = tempURL.appendingPathComponent("unit-test.docc")

let requestWithDifferentBundleID = ConvertRequest(
bundleInfo: DocumentationBundle.Info(displayName: "DisplayName", identifier: "com.example.something-else"),
bundleInfo: DocumentationBundle.Info(displayName: "DisplayName", id: "com.example.something-else"),
externalIDsToConvert: [],
bundleLocation: bundleURL,
symbolGraphs: [],
Expand All @@ -2341,7 +2341,7 @@ class ConvertServiceTests: XCTestCase {
XCTAssertEqual(try linkResolutionRequestsForConvertRequest(requestWithDifferentBundleID), [], "Shouldn't make any link resolution requests because the bundle IDs are different.")

let requestWithSameBundleID = ConvertRequest(
bundleInfo: DocumentationBundle.Info(displayName: "DisplayName", identifier: "com.example.something"),
bundleInfo: DocumentationBundle.Info(displayName: "DisplayName", id: "com.example.something"),
externalIDsToConvert: [],
bundleLocation: bundleURL,
symbolGraphs: [],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ class DocumentationServer_DefaultTests: XCTestCase {
let request = ConvertRequest(
bundleInfo: DocumentationBundle.Info(
displayName: "TestBundle",
identifier: "identifier"
id: "identifier"
),
externalIDsToConvert: ["s:5MyKit0A5ClassC10myFunctionyyF"],
symbolGraphs: [symbolGraph],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class DocumentationBundleInfoTests: XCTestCase {
let info = try DocumentationBundle.Info(from: infoPlistData)

XCTAssertEqual(info.displayName, "Test Bundle")
XCTAssertEqual(info.identifier, "org.swift.docc.example")
XCTAssertEqual(info.id.rawValue, "org.swift.docc.example")
XCTAssertEqual(info.defaultCodeListingLanguage, "swift")
}

Expand Down Expand Up @@ -82,7 +82,7 @@ class DocumentationBundleInfoTests: XCTestCase {
),
DocumentationBundle.Info(
displayName: "Info Plist Display Name",
identifier: "com.info.Plist"
id: "com.info.Plist"
)
)

Expand All @@ -93,7 +93,7 @@ class DocumentationBundleInfoTests: XCTestCase {
),
DocumentationBundle.Info(
displayName: "Fallback Display Name",
identifier: "com.fallback.Identifier"
id: "com.fallback.Identifier"
)
)

Expand All @@ -104,7 +104,7 @@ class DocumentationBundleInfoTests: XCTestCase {
),
DocumentationBundle.Info(
displayName: "Fallback Display Name",
identifier: "com.info.Plist"
id: "com.info.Plist"
)
)

Expand All @@ -128,7 +128,7 @@ class DocumentationBundleInfoTests: XCTestCase {
),
DocumentationBundle.Info(
displayName: "Info Plist Display Name",
identifier: "com.info.Plist"
id: "com.info.Plist"
)
)
}
Expand Down Expand Up @@ -237,7 +237,7 @@ class DocumentationBundleInfoTests: XCTestCase {
info,
DocumentationBundle.Info(
displayName: "Display Name",
identifier: "swift.org.Identifier",
id: "swift.org.Identifier",
defaultCodeListingLanguage: "swift",
defaultModuleKind: "Executable",
defaultAvailability: DefaultAvailability(
Expand All @@ -257,7 +257,7 @@ class DocumentationBundleInfoTests: XCTestCase {
func testFallbackToInfoInBundleDiscoveryOptions() throws {
let info = DocumentationBundle.Info(
displayName: "Display Name",
identifier: "swift.org.Identifier",
id: "swift.org.Identifier",
defaultCodeListingLanguage: "swift",
defaultModuleKind: "Executable",
defaultAvailability: DefaultAvailability(
Expand Down Expand Up @@ -341,7 +341,7 @@ class DocumentationBundleInfoTests: XCTestCase {
),
DocumentationBundle.Info(
displayName: "Derived Display Name",
identifier: "Derived Display Name"
id: "Derived Display Name"
)
)
}
Expand All @@ -366,7 +366,7 @@ class DocumentationBundleInfoTests: XCTestCase {
),
DocumentationBundle.Info(
displayName: "Derived Display Name",
identifier: "org.swift.docc.example"
id: "org.swift.docc.example"
)
)
}
Expand All @@ -390,7 +390,7 @@ class DocumentationBundleInfoTests: XCTestCase {
),
DocumentationBundle.Info(
displayName: "Example",
identifier: "Example"
id: "Example"
)
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ class DocumentationWorkspaceTests: XCTestCase {
return DocumentationBundle(
info: DocumentationBundle.Info(
displayName: "Test" + suffix,
identifier: "com.example.test" + suffix
id: DocumentationBundle.Identifier(rawValue: "com.example.test" + suffix)
),
symbolGraphURLs: [testSymbolGraphFile],
markupURLs: [testMarkupFile],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ class SymbolDisambiguationTests: XCTestCase {
let bundle = DocumentationBundle(
info: DocumentationBundle.Info(
displayName: "SymbolDisambiguationTests",
identifier: "com.test.SymbolDisambiguationTests"),
id: "com.test.SymbolDisambiguationTests"),
symbolGraphURLs: [swiftSymbolGraphURL, objcSymbolGraphURL],
markupURLs: [],
miscResourceURLs: []
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1791,7 +1791,7 @@ class SymbolGraphLoaderTests: XCTestCase {
let bundle = DocumentationBundle(
info: DocumentationBundle.Info(
displayName: "Test",
identifier: "com.example.test"
id: "com.example.test"
),
baseURL: URL(string: "https://example.com/example")!,
symbolGraphURLs: symbolGraphURLs,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class DefaultCodeBlockSyntaxTests: XCTestCase {
testBundleWithoutLanguageDefault = DocumentationBundle(
info: DocumentationBundle.Info(
displayName: testBundleWithLanguageDefault.displayName,
identifier: testBundleWithLanguageDefault.identifier,
id: DocumentationBundle.Identifier(rawValue: testBundleWithLanguageDefault.identifier),
defaultCodeListingLanguage: nil
),
baseURL: testBundleWithLanguageDefault.baseURL,
Expand Down
2 changes: 1 addition & 1 deletion Tests/SwiftDocCTests/XCTestCase+LoadingTestData.swift
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ extension XCTestCase {
let bundle = DocumentationBundle(
info: DocumentationBundle.Info(
displayName: "Test",
identifier: "com.example.test"
id: "com.example.test"
),
baseURL: URL(string: "https://example.com/example")!,
symbolGraphURLs: [],
Expand Down