[Collections] 'describe' package matching is too loose (#3543)

* [Collections] 'describe' package matching is too loose

Motivation:
rdar://79069839

To distinguish between package vs. collection URL, we first search for it as if it were a package URL. The package search, however, is done using `PackageIdentity` and not the actual URL itself. The problem with identity is that it only uses the last URL path component, so `bar/foo` and `baz/foo` would end up with the same identity `foo`.

Modifications:
Update `findPackage` to allow multiple results since a package identity (the way it is) can be associated with multiple repository URLs. Caller of `findPackage` should then filter out packages by repository URL if needed.

Sources/PackageCollections/PackageCollections+Storage.swift

@@ -11,19 +11,19 @@
 import TSCBasic
 
 extension PackageCollections {
-    public struct Storage: Closable {
+    struct Storage: Closable {
         let sources: PackageCollectionsSourcesStorage
         let collections: PackageCollectionsStorage
 
-        public init(sources: PackageCollectionsSourcesStorage, collections: PackageCollectionsStorage) {
+        init(sources: PackageCollectionsSourcesStorage, collections: PackageCollectionsStorage) {
             self.sources = sources
             self.collections = collections
         }
     }
 }
 
 extension PackageCollections.Storage {
-    public func close() throws {
+    func close() throws {
         var errors = [Error]()
 
         let tryClose = { (item: Any) in

Sources/PackageCollections/PackageCollections.swift

@@ -318,7 +318,7 @@ public struct PackageCollections: PackageCollectionsProtocol {
         }
 
         // first find in storage
-        self.findPackage(identifier: reference.identity, collections: collections) { result in
+        self.findPackage(reference: reference, collections: collections) { result in
             switch result {
             case .failure(let error):
                 callback(.failure(error))
@@ -479,7 +479,7 @@ public struct PackageCollections: PackageCollectionsProtocol {
         }
     }
 
-    func findPackage(identifier: PackageIdentity,
+    func findPackage(reference: PackageReference,
                      collections: Set<PackageCollectionsModel.CollectionIdentifier>?,
                      callback: @escaping (Result<PackageCollectionsModel.PackageSearchResult.Item, Error>) -> Void) {
         self.storage.sources.list { result in
@@ -492,9 +492,21 @@ public struct PackageCollections: PackageCollectionsProtocol {
                     collectionIdentifiers = collectionIdentifiers.filter { collections.contains($0) }
                 }
                 if collectionIdentifiers.isEmpty {
-                    return callback(.failure(NotFoundError("\(identifier)")))
+                    return callback(.failure(NotFoundError("\(reference)")))
+                }
+                self.storage.collections.findPackage(identifier: reference.identity, collectionIdentifiers: collectionIdentifiers) { findPackageResult in
+                    switch findPackageResult {
+                    case .failure(let error):
+                        callback(.failure(error))
+                    case .success(let packagesCollections):
+                        // A package identity can be associated with multiple repository URLs
+                        let matches = packagesCollections.packages.filter { $0.reference.canonicalLocation == reference.canonicalLocation }
+                        guard let package = matches.first else {
+                            return callback(.failure(NotFoundError("\(reference)")))
+                        }
+                        callback(.success(.init(package: package, collections: packagesCollections.collections)))
+                    }
                 }
-                self.storage.collections.findPackage(identifier: identifier, collectionIdentifiers: collectionIdentifiers, callback: callback)
             }
         }
     }
@@ -584,3 +596,9 @@ private struct UnknownProvider: Error {
         self.sourceType = sourceType
     }
 }
+
+private extension PackageReference {
+    var canonicalLocation: String {
+        (self.location.hasSuffix(".git") ? String(self.location.dropLast(4)) : self.location).lowercased()
+    }
+}

Sources/PackageCollections/Storage/PackageCollectionsStorage.swift

@@ -1,7 +1,7 @@
 /*
  This source file is part of the Swift.org open source project
 
- Copyright (c) 2020 Apple Inc. and the Swift project authors
+ Copyright (c) 2020-2021 Apple Inc. and the Swift project authors
  Licensed under Apache License v2.0 with Runtime Library Exception
 
  See http://swift.org/LICENSE.txt for license information
@@ -53,15 +53,17 @@ public protocol PackageCollectionsStorage {
                         query: String,
                         callback: @escaping (Result<PackageCollectionsModel.PackageSearchResult, Error>) -> Void)
 
-    /// Returns `PackageSearchResult.Item` for the given package identity.
+    /// Returns packages for the given package identity.
+    ///
+    /// Since a package identity can be associated with more than one repository URL, the result may contain multiple items.
     ///
     /// - Parameters:
     ///   - identifier: The package identifier
     ///   - collectionIdentifiers: Optional. The identifiers of the `PackageCollection`s
     ///   - callback: The closure to invoke when result becomes available
     func findPackage(identifier: PackageIdentity,
                      collectionIdentifiers: [PackageCollectionsModel.CollectionIdentifier]?,
-                     callback: @escaping (Result<PackageCollectionsModel.PackageSearchResult.Item, Error>) -> Void)
+                     callback: @escaping (Result<(packages: [PackageCollectionsModel.Package], collections: [PackageCollectionsModel.CollectionIdentifier]), Error>) -> Void)
 
     /// Returns `TargetSearchResult` for the given search criteria.
     ///

Sources/PackageCollections/Storage/SQLitePackageCollectionsStorage.swift

@@ -405,7 +405,7 @@ final class SQLitePackageCollectionsStorage: PackageCollectionsStorage, Closable
 
     func findPackage(identifier: PackageIdentity,
                      collectionIdentifiers: [Model.CollectionIdentifier]?,
-                     callback: @escaping (Result<Model.PackageSearchResult.Item, Error>) -> Void) {
+                     callback: @escaping (Result<(packages: [PackageCollectionsModel.Package], collections: [PackageCollectionsModel.CollectionIdentifier]), Error>) -> Void) {
         let useSearchIndices: Bool
         do {
             useSearchIndices = try self.shouldUseSearchIndices()
@@ -454,11 +454,17 @@ final class SQLitePackageCollectionsStorage: PackageCollectionsStorage, Closable
                         // Sort collections by processing date so the latest metadata is first
                         .sorted(by: { lhs, rhs in lhs.lastProcessedAt > rhs.lastProcessedAt })
 
-                    guard let package = collections.compactMap({ $0.packages.first { $0.reference.identity == identifier } }).first else {
+                    // rdar://79069839 - Package identities are not unique to repository URLs so there can be more than one result.
+                    // It's up to the caller to filter out the best-matched package(s). Results are sorted with the latest ones first.
+                    let packages = collections.flatMap { collection in
+                        collection.packages.filter { $0.reference.identity == identifier }
+                    }
+
+                    guard !packages.isEmpty else {
                         return callback(.failure(NotFoundError("\(identifier)")))
                     }
 
-                    callback(.success(.init(package: package, collections: collections.map { $0.identifier })))
+                    callback(.success((packages: packages, collections: collections.map { $0.identifier })))
                 }
             }
         } else {
@@ -473,12 +479,16 @@ final class SQLitePackageCollectionsStorage: PackageCollectionsStorage, Closable
                             .first(where: { $0.reference.identity == identifier })
                             .flatMap { (collection: collection.identifier, package: $0) }
                     }
-                    // first package should have latest processing date
-                    guard let package = collectionPackages.first?.package else {
+
+                    // rdar://79069839 - Package identities are not unique to repository URLs so there can be more than one result.
+                    // It's up to the caller to filter out the best-matched package(s). Results are sorted with the latest ones first.
+                    let packages = collectionPackages.map { $0.package }
+
+                    guard !packages.isEmpty else {
                         return callback(.failure(NotFoundError("\(identifier)")))
                     }
-                    let collections = collectionPackages.map { $0.collection }
-                    callback(.success(.init(package: package, collections: collections)))
+
+                    callback(.success((packages: packages, collections: collectionPackages.map { $0.collection })))
                 }
             }
         }

Sources/PackageCollectionsModel/Formats/v1.md

@@ -27,7 +27,7 @@ To begin, define the top-level metadata about the collection:
 
 Each item in the `packages` array is a package object with the following properties:
 
-* `url`: The URL of the package. Currently only Git repository URLs are supported.
+* `url`: The URL of the package. Currently only Git repository URLs are supported. URL should be HTTPS and may contain `.git` suffix.
 * `summary`: A description of the package. **Optional.**
 * `keywords`: An array of keywords that the package is associated with. **Optional.**
 * `readmeURL`: The URL of the package's README. **Optional.**