fix mirrors reverse mapping to be more deterministic (#5559) (#5562)

Author: tomerd

Sources/PackageGraph/DependencyMirrors.swift

@@ -11,12 +11,14 @@
 //===----------------------------------------------------------------------===//
 
 import Foundation
+import OrderedCollections
 import TSCBasic
 
 /// A collection of dependency mirrors.
 public final class DependencyMirrors: Equatable {
     private var index: [String: String]
-    private var reverseIndex: [String: String]
+    private var reverseIndex: [String: [String]]
+    private var visited: OrderedCollections.OrderedSet<String>
     private let lock = Lock()
 
     public var mapping: [String: String] {
@@ -27,7 +29,11 @@ public final class DependencyMirrors: Equatable {
 
     public init(_ mirrors: [String: String]) {
         self.index = mirrors
-        self.reverseIndex = Dictionary(mirrors.map({ ($0.1, $0.0) }), uniquingKeysWith: { first, _ in first })
+        self.reverseIndex = [String: [String]]()
+        for entry in mirrors {
+            self.reverseIndex[entry.value, default: []].append(entry.key)
+        }
+        self.visited = .init()
     }
 
     @available(*, deprecated)
@@ -46,7 +52,7 @@ public final class DependencyMirrors: Equatable {
     public func set(mirrorURL: String, forURL url: String) {
         self.lock.withLock {
             self.index[url] = mirrorURL
-            self.reverseIndex[mirrorURL] = url
+            self.reverseIndex[mirrorURL, default: []].append(url)
         }
     }
 
@@ -105,7 +111,12 @@ public final class DependencyMirrors: Equatable {
     /// - Returns: The mirrored URL, if one exists.
     public func mirrorURL(for url: String) -> String? {
         self.lock.withLock {
-            return self.index[url]
+            let value = self.index[url]
+            if value != nil {
+                // record visited mirrors for reverse index lookup sorting
+                self.visited.append(url)
+            }
+            return value
         }
     }
 
@@ -123,7 +134,23 @@ public final class DependencyMirrors: Equatable {
     /// - Returns: The original URL, if one exists.
     public func originalURL(for url: String) -> String? {
         self.lock.withLock {
-            return self.reverseIndex[url]
+            let alternatives = self.reverseIndex[url]
+            // since there are potentially multiple mapping, we need to sort them to produce deterministic results
+            let sorted = alternatives?.sorted(by: { lhs, rhs in
+                // check if it was visited (which means it used by the package)
+                switch (self.visited.firstIndex(of: lhs), self.visited.firstIndex(of: rhs)) {
+                case (.some(let lhsIndex), .some(let rhsIndex)):
+                    return lhsIndex < rhsIndex
+                case (.some, .none):
+                    return true
+                case (.none, .some):
+                    return false
+                case (.none, .none):
+                    // otherwise sort alphabetically
+                    return lhs < rhs
+                }
+            })
+            return sorted?.first
         }
     }
 }

Tests/WorkspaceTests/PinsStoreTests.swift

@@ -322,6 +322,10 @@ final class PinsStoreTests: XCTestCase {
         try store.saveState(toolsVersion: ToolsVersion.current)
         XCTAssert(fileSystem.exists(pinsFile))
 
+        let content: String = try fileSystem.readFileContents(pinsFile)
+        XCTAssertMatch(content, .contains(fooURL.absoluteString))
+        XCTAssertNoMatch(content, .contains(fooMirroredURL.absoluteString))
+
         // Load the store again from disk, with no mirrors
         let store2 = try PinsStore(pinsFile: pinsFile, workingDirectory: .root, fileSystem: fileSystem, mirrors: .init())
         XCTAssert(store2.pinsMap.count == 3)
@@ -334,4 +338,126 @@ final class PinsStoreTests: XCTestCase {
         XCTAssert(store3.pinsMap.count == 3)
         XCTAssertEqual(store3.pinsMap, store.pinsMap)
     }
+
+    func testPinsWithMirrorsDeterminism() throws {
+        let fooIdentity = PackageIdentity.plain("foo")
+        let fooURL1 = URL(string: "https://github.com/corporate/foo")!
+        let fooURL2 = URL(string: "https://github.com/corporate/foo.git")!
+        let fooURL3 = URL(string: "https://github.com/old-corporate/foo")!
+        let fooURL4 = URL(string: "https://github.com/old-corporate/foo.git")!
+        let fooMirroredURL = URL(string: "https://github.corporate.com/team/foo")!
+
+        let mirrors = DependencyMirrors()
+        mirrors.set(mirrorURL: fooMirroredURL.absoluteString, forURL: fooURL1.absoluteString)
+        mirrors.set(mirrorURL: fooMirroredURL.absoluteString, forURL: fooURL2.absoluteString)
+        mirrors.set(mirrorURL: fooMirroredURL.absoluteString, forURL: fooURL3.absoluteString)
+        mirrors.set(mirrorURL: fooMirroredURL.absoluteString, forURL: fooURL4.absoluteString)
+
+        let fileSystem = InMemoryFileSystem()
+        let pinsFile = AbsolutePath("/pins.txt")
+
+        let store1 = try PinsStore(pinsFile: pinsFile, workingDirectory: .root, fileSystem: fileSystem, mirrors: mirrors)
+        store1.pin(
+            packageRef: .remoteSourceControl(identity: fooIdentity, url: fooMirroredURL),
+            state: .version(v1, revision: "revision")
+        )
+
+        XCTAssert(store1.pinsMap.count == 1)
+        XCTAssertEqual(store1.pinsMap[fooIdentity]!.packageRef.kind, .remoteSourceControl(fooMirroredURL))
+
+        try store1.saveState(toolsVersion: ToolsVersion.current)
+        XCTAssert(fileSystem.exists(pinsFile))
+
+        let content: String = try fileSystem.readFileContents(pinsFile)
+        XCTAssertMatch(content, .contains(fooURL1.absoluteString))
+        XCTAssertNoMatch(content, .contains(fooURL2.absoluteString))
+        XCTAssertNoMatch(content, .contains(fooURL3.absoluteString))
+        XCTAssertNoMatch(content, .contains(fooURL4.absoluteString))
+        XCTAssertNoMatch(content, .contains(fooMirroredURL.absoluteString))
+
+        // Load the store again from disk, with no mirrors
+        let store2 = try PinsStore(pinsFile: pinsFile, workingDirectory: .root, fileSystem: fileSystem, mirrors: .init())
+        XCTAssert(store2.pinsMap.count == 1)
+        XCTAssertEqual(store2.pinsMap[fooIdentity]!.packageRef.kind, .remoteSourceControl(fooURL1))
+
+        // Load the store again from disk, with mirrors
+        let store3 = try PinsStore(pinsFile: pinsFile, workingDirectory: .root, fileSystem: fileSystem, mirrors: mirrors)
+        XCTAssert(store3.pinsMap.count == 1)
+        XCTAssertEqual(store3.pinsMap, store1.pinsMap)
+    }
+
+    func testMirrorsDeterminism() throws {
+        let URL1 = URL(string: "https://github.com/corporate/foo")!
+        let URL2 = URL(string: "https://github.com/corporate/foo.git")!
+        let URL3 = URL(string: "https://github.com/old-corporate/foo")!
+        let URL4 = URL(string: "https://github.com/old-corporate/foo.git")!
+        let mirroredURL = URL(string: "https://github.corporate.com/team/foo")!
+
+        do {
+            let mirrors = DependencyMirrors([
+                URL1.absoluteString: mirroredURL.absoluteString,
+                URL2.absoluteString: mirroredURL.absoluteString,
+                URL3.absoluteString: mirroredURL.absoluteString,
+                URL4.absoluteString: mirroredURL.absoluteString
+            ])
+
+            XCTAssertEqual(mirrors.mirrorURL(for: URL2.absoluteString), mirroredURL.absoluteString)
+            // reverse index is sorted by "visited", then alphabetically
+            XCTAssertEqual(mirrors.originalURL(for: mirroredURL.absoluteString), URL2.absoluteString)
+        }
+
+        do {
+            let mirrors = DependencyMirrors([
+                URL1.absoluteString: mirroredURL.absoluteString,
+                URL2.absoluteString: mirroredURL.absoluteString,
+                URL3.absoluteString: mirroredURL.absoluteString,
+                URL4.absoluteString: mirroredURL.absoluteString
+            ])
+
+            XCTAssertEqual(mirrors.mirrorURL(for: URL3.absoluteString), mirroredURL.absoluteString)
+            // reverse index is sorted by "visited", then alphabetically
+            XCTAssertEqual(mirrors.originalURL(for: mirroredURL.absoluteString), URL3.absoluteString)
+        }
+
+        do {
+            let mirrors = DependencyMirrors([
+                URL1.absoluteString: mirroredURL.absoluteString,
+                URL2.absoluteString: mirroredURL.absoluteString,
+                URL3.absoluteString: mirroredURL.absoluteString,
+                URL4.absoluteString: mirroredURL.absoluteString
+            ])
+
+            XCTAssertEqual(mirrors.mirrorURL(for: URL2.absoluteString), mirroredURL.absoluteString)
+            XCTAssertEqual(mirrors.mirrorURL(for: URL3.absoluteString), mirroredURL.absoluteString)
+            // reverse index is sorted by "visited", then alphabetically
+            XCTAssertEqual(mirrors.originalURL(for: mirroredURL.absoluteString), URL2.absoluteString)
+        }
+
+        do {
+            let mirrors = DependencyMirrors([
+                URL1.absoluteString: mirroredURL.absoluteString,
+                URL2.absoluteString: mirroredURL.absoluteString,
+                URL3.absoluteString: mirroredURL.absoluteString,
+                URL4.absoluteString: mirroredURL.absoluteString
+            ])
+
+            XCTAssertEqual(mirrors.mirrorURL(for: URL3.absoluteString), mirroredURL.absoluteString)
+            XCTAssertEqual(mirrors.mirrorURL(for: URL2.absoluteString), mirroredURL.absoluteString)
+            // reverse index is sorted by "visited", then alphabetically
+            XCTAssertEqual(mirrors.originalURL(for: mirroredURL.absoluteString), URL3.absoluteString)
+        }
+
+        do {
+            let mirrors = DependencyMirrors([
+                URL1.absoluteString: mirroredURL.absoluteString,
+                URL2.absoluteString: mirroredURL.absoluteString,
+                URL3.absoluteString: mirroredURL.absoluteString,
+                URL4.absoluteString: mirroredURL.absoluteString
+            ])
+
+            // reverse index is sorted by "visited", then alphabetically
+            XCTAssertEqual(mirrors.originalURL(for: mirroredURL.absoluteString), URL1.absoluteString)
+        }
+    }
+
 }