Skip to content

Commit 86de647

Browse files
yim-leeyim-lee
authored
[Collections] add should only add to config if fetch is successful (#3195)
Motivation: The code currently adds a collection source to the config even when initial fetch fails. Modifications: We should fetch collection first and make sure that's successful before adding to config. Result: Bad sources are not added to the config.
1 parent d9fefd7 commit 86de647

File tree

3 files changed

+102
-27
lines changed

3 files changed

+102
-27
lines changed

‎Sources/PackageCollections/PackageCollections.swift

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,22 @@ public struct PackageCollections: PackageCollectionsProtocol {
137137
callback(.failure(error))
138138
case .success:
139139
// next try to fetch the collection from the network and store it locally so future operations dont need to access the network
140-
self.refreshCollectionFromSource(source: source, trustConfirmationProvider: trustConfirmationProvider, callback: callback)
140+
self.refreshCollectionFromSource(source: source, trustConfirmationProvider: trustConfirmationProvider) { collectionResult in
141+
switch collectionResult {
142+
case .failure(let error):
143+
// Don't delete the source if we are either pending user confirmation or have recorded user's preference
144+
if let error = error as? PackageCollectionError, error == .trustConfirmationRequired || error == .untrusted {
145+
return callback(.failure(error))
146+
}
147+
// Otherwise remove source since it fails to be fetched
148+
self.storage.sources.remove(source: source) { _ in
149+
// Whether removal succeeds or not, return the refresh error
150+
callback(.failure(error))
151+
}
152+
case .success(let collection):
153+
callback(.success(collection))
154+
}
155+
}
141156
}
142157
}
143158
}

‎Tests/PackageCollectionsTests/PackageCollectionsModelTests.swift

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -88,12 +88,27 @@ final class PackageCollectionsModelTests: XCTestCase {
8888
XCTAssertNil(versions.latestPrerelease)
8989
}
9090

91-
func test_sourceOK() throws {
92-
do {
93-
let source = PackageCollectionsModel.CollectionSource(type: .json, url: URL(string: "https://package-collection-tests.com/test.json")!)
94-
XCTAssertNil(source.validate())
95-
}
91+
func testSourceValidation() throws {
92+
let httpsSource = PackageCollectionsModel.CollectionSource(type: .json, url: URL(string: "https://feed.mock.io")!)
93+
XCTAssertNil(httpsSource.validate(), "not expecting errors")
94+
95+
let httpsSource2 = PackageCollectionsModel.CollectionSource(type: .json, url: URL(string: "HTTPS://feed.mock.io")!)
96+
XCTAssertNil(httpsSource2.validate(), "not expecting errors")
97+
98+
let httpsSource3 = PackageCollectionsModel.CollectionSource(type: .json, url: URL(string: "HttpS://feed.mock.io")!)
99+
XCTAssertNil(httpsSource3.validate(), "not expecting errors")
100+
101+
let httpSource = PackageCollectionsModel.CollectionSource(type: .json, url: URL(string: "http://feed.mock.io")!)
102+
XCTAssertEqual(httpSource.validate()?.count, 1, "expecting errors")
103+
104+
let otherProtocolSource = PackageCollectionsModel.CollectionSource(type: .json, url: URL(string: "ftp://feed.mock.io")!)
105+
XCTAssertEqual(otherProtocolSource.validate()?.count, 1, "expecting errors")
106+
107+
let brokenUrlSource = PackageCollectionsModel.CollectionSource(type: .json, url: URL(string: "blah")!)
108+
XCTAssertEqual(brokenUrlSource.validate()?.count, 1, "expecting errors")
109+
}
96110

111+
func testSourceValidation_localFile() throws {
97112
do {
98113
fixture(name: "Collections") { directoryPath in
99114
// File must exist in local FS
@@ -105,7 +120,7 @@ final class PackageCollectionsModelTests: XCTestCase {
105120
}
106121
}
107122

108-
func test_source_localFileDoesNotExist() throws {
123+
func testSourceValidation_localFileDoesNotExist() throws {
109124
do {
110125
let source = PackageCollectionsModel.CollectionSource(type: .json, url: URL(fileURLWithPath: "/foo/bar"))
111126

‎Tests/PackageCollectionsTests/PackageCollectionsTests.swift

Lines changed: 65 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,71 @@ final class PackageCollectionsTests: XCTestCase {
110110
}
111111
}
112112

113+
func testInvalidCollectionNotAdded() throws {
114+
let configuration = PackageCollections.Configuration()
115+
let storage = makeMockStorage()
116+
defer { XCTAssertNoThrow(try storage.close()) }
117+
118+
let mockCollection = makeMockCollections(count: 1).first!
119+
120+
let collectionProviders = [PackageCollectionsModel.CollectionSourceType.json: MockCollectionsProvider([])]
121+
let metadataProvider = MockMetadataProvider([:])
122+
let packageCollections = PackageCollections(configuration: configuration, storage: storage, collectionProviders: collectionProviders, metadataProvider: metadataProvider)
123+
124+
do {
125+
let list = try tsc_await { callback in packageCollections.listCollections(callback: callback) }
126+
XCTAssertEqual(list.count, 0, "list should be empty")
127+
128+
let sources = try tsc_await { callback in storage.sources.list(callback: callback) }
129+
XCTAssertEqual(sources.count, 0, "sources should be empty")
130+
}
131+
132+
// add fails because collection is not found
133+
guard case .failure = tsc_await({ callback in packageCollections.addCollection(mockCollection.source, order: nil, callback: callback) }) else {
134+
return XCTFail("expected error")
135+
}
136+
137+
do {
138+
let list = try tsc_await { callback in packageCollections.listCollections(callback: callback) }
139+
XCTAssertEqual(list.count, 0, "list count should match")
140+
141+
let sources = try tsc_await { callback in storage.sources.list(callback: callback) }
142+
XCTAssertEqual(sources.count, 0, "sources should be empty")
143+
}
144+
}
145+
146+
func testCollectionPendingTrustConfirmIsKeptOnAdd() throws {
147+
let configuration = PackageCollections.Configuration()
148+
let storage = makeMockStorage()
149+
defer { XCTAssertNoThrow(try storage.close()) }
150+
151+
let mockCollection = makeMockCollections(count: 1, signed: false).first!
152+
153+
let collectionProviders = [PackageCollectionsModel.CollectionSourceType.json: MockCollectionsProvider([mockCollection])]
154+
let metadataProvider = MockMetadataProvider([:])
155+
let packageCollections = PackageCollections(configuration: configuration, storage: storage, collectionProviders: collectionProviders, metadataProvider: metadataProvider)
156+
157+
do {
158+
let list = try tsc_await { callback in packageCollections.listCollections(callback: callback) }
159+
XCTAssertEqual(list.count, 0, "list should be empty")
160+
161+
let sources = try tsc_await { callback in storage.sources.list(callback: callback) }
162+
XCTAssertEqual(sources.count, 0, "sources should be empty")
163+
}
164+
165+
guard case .failure = tsc_await({ callback in packageCollections.addCollection(mockCollection.source, order: nil, callback: callback) }) else {
166+
return XCTFail("expected error")
167+
}
168+
169+
do {
170+
let list = try tsc_await { callback in packageCollections.listCollections(callback: callback) }
171+
XCTAssertEqual(list.count, 0, "list count should match")
172+
173+
let sources = try tsc_await { callback in storage.sources.list(callback: callback) }
174+
XCTAssertEqual(sources.count, 1, "sources should match")
175+
}
176+
}
177+
113178
func testDelete() throws {
114179
let configuration = PackageCollections.Configuration()
115180
let storage = makeMockStorage()
@@ -1115,24 +1180,4 @@ final class PackageCollectionsTests: XCTestCase {
11151180
let delta = Date().timeIntervalSince(start)
11161181
XCTAssert(delta < 1.0, "should fetch quickly, took \(delta)")
11171182
}
1118-
1119-
func testSourceValidation() throws {
1120-
let httpsSource = PackageCollectionsModel.CollectionSource(type: .json, url: URL(string: "https://feed.mock.io")!)
1121-
XCTAssertNil(httpsSource.validate(), "not expecting errors")
1122-
1123-
let httpsSource2 = PackageCollectionsModel.CollectionSource(type: .json, url: URL(string: "HTTPS://feed.mock.io")!)
1124-
XCTAssertNil(httpsSource2.validate(), "not expecting errors")
1125-
1126-
let httpsSource3 = PackageCollectionsModel.CollectionSource(type: .json, url: URL(string: "HttpS://feed.mock.io")!)
1127-
XCTAssertNil(httpsSource3.validate(), "not expecting errors")
1128-
1129-
let httpSource = PackageCollectionsModel.CollectionSource(type: .json, url: URL(string: "http://feed.mock.io")!)
1130-
XCTAssertEqual(httpSource.validate()?.count, 1, "expecting errors")
1131-
1132-
let otherProtocolSource = PackageCollectionsModel.CollectionSource(type: .json, url: URL(string: "ftp://feed.mock.io")!)
1133-
XCTAssertEqual(otherProtocolSource.validate()?.count, 1, "expecting errors")
1134-
1135-
let brokenUrlSource = PackageCollectionsModel.CollectionSource(type: .json, url: URL(string: "blah")!)
1136-
XCTAssertEqual(brokenUrlSource.validate()?.count, 1, "expecting errors")
1137-
}
11381183
}

0 commit comments

Comments
 (0)