[Collections] listCollections always returns all configured collections (swiftlang#3565)

Motivation:
`listCollections` has optional `identifiers` that if specified, only those collections are supposed to be fetched. However, the current implementation always fetches all configured collections because the logic for determining "missing" collections is flawed.

Modification:
Fix logic such that `listCollections` returns all or a subset of collections depending on `identifiers` param.

Author: yim-lee

Sources/PackageCollections/PackageCollections.swift

@@ -81,34 +81,38 @@ public struct PackageCollections: PackageCollectionsProtocol {
             case .failure(let error):
                 callback(.failure(error))
             case .success(let sources):
-                let identifiers = sources.map { .init(from: $0) }.filter { identifiers?.contains($0) ?? true }
-                if identifiers.isEmpty {
-                    return callback(.success([]))
+                let identiferSource = sources.reduce(into: [PackageCollectionsModel.CollectionIdentifier: PackageCollectionsModel.CollectionSource]()) { result, source in
+                    result[.init(from: source)] = source
                 }
-                let collectionOrder = identifiers.enumerated().reduce([Model.CollectionIdentifier: Int]()) { partial, element in
-                    var dictionary = partial
-                    dictionary[element.element] = element.offset
-                    return dictionary
+                let identifiersToFetch = identiferSource.keys.filter { identifiers?.contains($0) ?? true }
+
+                if identifiersToFetch.isEmpty {
+                    return callback(.success([]))
                 }
-                self.storage.collections.list(identifiers: identifiers) { result in
+
+                self.storage.collections.list(identifiers: identifiersToFetch) { result in
                     switch result {
                     case .failure(let error):
                         callback(.failure(error))
                     case .success(var collections):
+                        let sourceOrder = sources.enumerated().reduce(into: [Model.CollectionIdentifier: Int]()) { result, item in
+                            result[.init(from: item.element)] = item.offset
+                        }
+
                         // re-order by profile order which reflects the user's election
                         let sort = { (lhs: PackageCollectionsModel.Collection, rhs: PackageCollectionsModel.Collection) -> Bool in
-                            collectionOrder[lhs.identifier] ?? 0 < collectionOrder[rhs.identifier] ?? 0
+                            sourceOrder[lhs.identifier] ?? 0 < sourceOrder[rhs.identifier] ?? 0
                         }
 
-                        // We've fetched all the configured collections and we're done
-                        if collections.count == sources.count {
+                        // We've fetched all the wanted collections and we're done
+                        if collections.count == identifiersToFetch.count {
                             collections.sort(by: sort)
                             return callback(.success(collections))
                         }
 
                         // Some of the results are missing. This happens when deserialization of stored collections fail,
                         // so we will try refreshing the missing collections to update data in storage.
-                        let missingSources = Set(sources).subtracting(Set(collections.map { $0.source }))
+                        let missingSources = Set(identifiersToFetch.compactMap { identiferSource[$0] }).subtracting(Set(collections.map { $0.source }))
                         let refreshResults = ThreadSafeArrayStore<Result<Model.Collection, Error>>()
                         missingSources.forEach { source in
                             self.refreshCollectionFromSource(source: source, trustConfirmationProvider: nil) { refreshResult in

Tests/PackageCollectionsTests/PackageCollectionsTests.swift

@@ -567,6 +567,51 @@ final class PackageCollectionsTests: XCTestCase {
         }
     }
 
+    func testList() throws {
+        try skipIfUnsupportedPlatform()
+
+        let configuration = PackageCollections.Configuration()
+        let storage = makeMockStorage()
+        defer { XCTAssertNoThrow(try storage.close()) }
+
+        let mockCollections = makeMockCollections(count: 10)
+        let mockPackage = mockCollections.last!.packages.last!
+        let mockMetadata = makeMockPackageBasicMetadata()
+        let collectionProviders = [PackageCollectionsModel.CollectionSourceType.json: MockCollectionsProvider(mockCollections)]
+        let metadataProvider = MockMetadataProvider([mockPackage.reference: mockMetadata])
+        let packageCollections = PackageCollections(configuration: configuration, storage: storage, collectionProviders: collectionProviders, metadataProvider: metadataProvider)
+
+        try mockCollections.forEach { collection in
+            _ = try tsc_await { callback in packageCollections.addCollection(collection.source, trustConfirmationProvider: { _, cb in cb(true) }, callback: callback) }
+        }
+
+        let list = try tsc_await { callback in packageCollections.listCollections(callback: callback) }
+        XCTAssertEqual(list.count, mockCollections.count, "list count should match")
+    }
+
+    func testListSubset() throws {
+        try skipIfUnsupportedPlatform()
+
+        let configuration = PackageCollections.Configuration()
+        let storage = makeMockStorage()
+        defer { XCTAssertNoThrow(try storage.close()) }
+
+        let mockCollections = makeMockCollections(count: 10)
+        let mockPackage = mockCollections.last!.packages.last!
+        let mockMetadata = makeMockPackageBasicMetadata()
+        let collectionProviders = [PackageCollectionsModel.CollectionSourceType.json: MockCollectionsProvider(mockCollections)]
+        let metadataProvider = MockMetadataProvider([mockPackage.reference: mockMetadata])
+        let packageCollections = PackageCollections(configuration: configuration, storage: storage, collectionProviders: collectionProviders, metadataProvider: metadataProvider)
+
+        try mockCollections.forEach { collection in
+            _ = try tsc_await { callback in packageCollections.addCollection(collection.source, trustConfirmationProvider: { _, cb in cb(true) }, callback: callback) }
+        }
+
+        let expectedCollections = Set([mockCollections.first!.identifier, mockCollections.last!.identifier])
+        let list = try tsc_await { callback in packageCollections.listCollections(identifiers: expectedCollections, callback: callback) }
+        XCTAssertEqual(list.count, expectedCollections.count, "list count should match")
+    }
+
     func testListPerformance() throws {
         #if ENABLE_COLLECTION_PERF_TESTS
         #else