Skip to content

Commit 01fb4cc

Browse files
dimitribounioldimitribouniol
committed
Fixed an issue where secondary index cursor lookups could fail if the contents (the ID) ended split on a page
1 parent 44162be commit 01fb4cc

File tree

4 files changed

+101
-17
lines changed

4 files changed

+101
-17
lines changed

‎Sources/CodableDatastore/Persistence/Disk Persistence/Datastore/DatastoreIndex.swift‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -289,12 +289,14 @@ extension DiskPersistence.Datastore.Index {
289289
/// - Parameters:
290290
/// - proposedEntry: The entry to use in comparison with other persisted entries.
291291
/// - pages: A collection of pages to check against.
292+
/// - requiresCompleteEntries: Set to `true` if the comparator requires a complete entry to operate with.
292293
/// - pageBuilder: A closure that provides a cached Page object for the loaded page.
293294
/// - comparator: A comparator to determine order and equality between the proposed entry and a persisted one.
294295
/// - Returns: The index within the pages collection where the entry would reside.
295296
func pageIndex<T>(
296297
for proposedEntry: T,
297298
in pages: [DatastoreIndexManifest.PageInfo],
299+
requiresCompleteEntries: Bool,
298300
pageBuilder: @Sendable (_ pageID: DatastorePageIdentifier) async -> DiskPersistence.Datastore.Page,
299301
comparator: @Sendable (_ lhs: T, _ rhs: DatastorePageEntry) throws -> SortOrder
300302
) async throws -> Int? {
@@ -348,11 +350,13 @@ extension DiskPersistence.Datastore.Index {
348350

349351
/// If we have some bytes, attempt to decode them into an entry.
350352
if let bytesForFirstEntry {
351-
firstEntryOfPage = try? DatastorePageEntry(bytes: bytesForFirstEntry, isPartial: false)
353+
firstEntryOfPage = try? DatastorePageEntry(bytes: bytesForFirstEntry, isPartial: true)
352354
}
353355

354-
/// If we have an entry, stop scanning as we can go ahead and operate on it.
355-
if firstEntryOfPage != nil { break pageIterator }
356+
/// If we have an entry, stop scanning as we can go ahead and operate on it. Also make sure that we have a complete entry if one is required by rejecting partial entries when the flag is set.
357+
if let firstEntryOfPage, !(requiresCompleteEntries && firstEntryOfPage.isPartial) {
358+
break pageIterator
359+
}
356360
}
357361
}
358362

@@ -388,6 +392,7 @@ extension DiskPersistence.Datastore.Index {
388392

389393
func entry<T>(
390394
for proposedEntry: T,
395+
requiresCompleteEntries: Bool,
391396
comparator: @Sendable (_ lhs: T, _ rhs: DatastorePageEntry) throws -> SortOrder
392397
) async throws -> (
393398
cursor: DiskPersistence.InstanceCursor,
@@ -396,6 +401,7 @@ extension DiskPersistence.Datastore.Index {
396401
try await entry(
397402
for: proposedEntry,
398403
in: try await manifest.orderedPages,
404+
requiresCompleteEntries: requiresCompleteEntries,
399405
pageBuilder: { await datastore.page(for: .init(index: self.id, page: $0)) },
400406
comparator: comparator
401407
)
@@ -404,6 +410,7 @@ extension DiskPersistence.Datastore.Index {
404410
func entry<T>(
405411
for proposedEntry: T,
406412
in pages: [DatastoreIndexManifest.PageInfo],
413+
requiresCompleteEntries: Bool,
407414
pageBuilder: @Sendable (_ pageID: DatastorePageIdentifier) async -> DiskPersistence.Datastore.Page,
408415
comparator: @Sendable (_ lhs: T, _ rhs: DatastorePageEntry) throws -> SortOrder
409416
) async throws -> (
@@ -415,6 +422,7 @@ extension DiskPersistence.Datastore.Index {
415422
let startingPageIndex = try await pageIndex(
416423
for: proposedEntry,
417424
in: pages,
425+
requiresCompleteEntries: requiresCompleteEntries,
418426
pageBuilder: pageBuilder,
419427
comparator: comparator
420428
)
@@ -549,11 +557,13 @@ extension DiskPersistence.Datastore.Index {
549557

550558
func insertionCursor<T>(
551559
for proposedEntry: T,
560+
requiresCompleteEntries: Bool,
552561
comparator: @Sendable (_ lhs: T, _ rhs: DatastorePageEntry) throws -> SortOrder
553562
) async throws -> DiskPersistence.InsertionCursor {
554563
try await insertionCursor(
555564
for: proposedEntry,
556565
in: try await manifest.orderedPages,
566+
requiresCompleteEntries: requiresCompleteEntries,
557567
pageBuilder: { await datastore.page(for: .init(index: self.id, page: $0)) },
558568
comparator: comparator
559569
)
@@ -562,6 +572,7 @@ extension DiskPersistence.Datastore.Index {
562572
func insertionCursor<T>(
563573
for proposedEntry: T,
564574
in pages: [DatastoreIndexManifest.PageInfo],
575+
requiresCompleteEntries: Bool,
565576
pageBuilder: @Sendable (_ pageID: DatastorePageIdentifier) async -> DiskPersistence.Datastore.Page,
566577
comparator: @Sendable (_ lhs: T, _ rhs: DatastorePageEntry) throws -> SortOrder
567578
) async throws -> DiskPersistence.InsertionCursor {
@@ -570,6 +581,7 @@ extension DiskPersistence.Datastore.Index {
570581
let startingPageIndex = try await pageIndex(
571582
for: proposedEntry,
572583
in: pages,
584+
requiresCompleteEntries: requiresCompleteEntries,
573585
pageBuilder: pageBuilder,
574586
comparator: comparator
575587
)

‎Sources/CodableDatastore/Persistence/Disk Persistence/Datastore/DatastorePageEntry.swift‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@ import Bytes
1313
struct DatastorePageEntry: Hashable {
1414
var headers: [Bytes]
1515
var content: Bytes
16+
17+
/// Whether the entry contains a complete header, but a partial content.
1618
var isPartial: Bool = false
1719
}
1820

‎Sources/CodableDatastore/Persistence/Disk Persistence/Transaction/Transaction.swift‎

Lines changed: 36 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -529,7 +529,11 @@ extension DiskPersistence.Transaction {
529529

530530
let index = try await rootObject.primaryIndex
531531

532-
let (cursor, entry) = try await index.entry(for: identifier, comparator: primaryIndexComparator)
532+
let (cursor, entry) = try await index.entry(
533+
for: identifier,
534+
requiresCompleteEntries: false,
535+
comparator: primaryIndexComparator
536+
)
533537
guard entry.headers.count == 2
534538
else { throw DiskPersistenceError.invalidEntryFormat }
535539

@@ -551,7 +555,11 @@ extension DiskPersistence.Transaction {
551555

552556
let index = try await rootObject.primaryIndex
553557

554-
return try await index.insertionCursor(for: identifier, comparator: primaryIndexComparator)
558+
return try await index.insertionCursor(
559+
for: identifier,
560+
requiresCompleteEntries: false,
561+
comparator: primaryIndexComparator
562+
)
555563
}
556564

557565
func directIndexCursor<IndexType: Indexable, IdentifierType: Indexable>(
@@ -572,7 +580,11 @@ extension DiskPersistence.Transaction {
572580
guard let index = try await rootObject.directIndexes[indexName]
573581
else { throw DatastoreInterfaceError.indexNotFound }
574582

575-
let (cursor, entry) = try await index.entry(for: (indexValue, identifier), comparator: directIndexComparator)
583+
let (cursor, entry) = try await index.entry(
584+
for: (indexValue, identifier),
585+
requiresCompleteEntries: false,
586+
comparator: directIndexComparator
587+
)
576588
guard entry.headers.count == 3
577589
else { throw DiskPersistenceError.invalidEntryFormat }
578590

@@ -597,7 +609,11 @@ extension DiskPersistence.Transaction {
597609
guard let index = try await rootObject.directIndexes[indexName]
598610
else { throw DatastoreInterfaceError.indexNotFound }
599611

600-
return try await index.insertionCursor(for: (indexValue, identifier), comparator: directIndexComparator)
612+
return try await index.insertionCursor(
613+
for: (indexValue, identifier),
614+
requiresCompleteEntries: false,
615+
comparator: directIndexComparator
616+
)
601617
}
602618

603619
func secondaryIndexCursor<IndexType: Indexable, IdentifierType: Indexable>(
@@ -614,7 +630,11 @@ extension DiskPersistence.Transaction {
614630
guard let index = try await rootObject.secondaryIndexes[indexName]
615631
else { throw DatastoreInterfaceError.indexNotFound }
616632

617-
let (cursor, _) = try await index.entry(for: (indexValue, identifier), comparator: secondaryIndexComparator)
633+
let (cursor, _) = try await index.entry(
634+
for: (indexValue, identifier),
635+
requiresCompleteEntries: true,
636+
comparator: secondaryIndexComparator
637+
)
618638

619639
return cursor
620640
}
@@ -633,7 +653,11 @@ extension DiskPersistence.Transaction {
633653
guard let index = try await rootObject.secondaryIndexes[indexName]
634654
else { throw DatastoreInterfaceError.indexNotFound }
635655

636-
return try await index.insertionCursor(for: (indexValue, identifier), comparator: secondaryIndexComparator)
656+
return try await index.insertionCursor(
657+
for: (indexValue, identifier),
658+
requiresCompleteEntries: true,
659+
comparator: secondaryIndexComparator
660+
)
637661
}
638662
}
639663

@@ -698,6 +722,7 @@ extension DiskPersistence.Transaction {
698722
} else {
699723
try await index.insertionCursor(
700724
for: (range.lowerBoundExpression, .ascending),
725+
requiresCompleteEntries: false,
701726
comparator: primaryIndexBoundComparator
702727
)
703728
}
@@ -723,6 +748,7 @@ extension DiskPersistence.Transaction {
723748
} else {
724749
try await index.insertionCursor(
725750
for: (range.upperBoundExpression, .descending),
751+
requiresCompleteEntries: false,
726752
comparator: primaryIndexBoundComparator
727753
)
728754
}
@@ -766,6 +792,7 @@ extension DiskPersistence.Transaction {
766792
} else {
767793
try await index.insertionCursor(
768794
for: (range.lowerBoundExpression, .ascending),
795+
requiresCompleteEntries: false,
769796
comparator: directIndexBoundComparator
770797
)
771798
}
@@ -791,6 +818,7 @@ extension DiskPersistence.Transaction {
791818
} else {
792819
try await index.insertionCursor(
793820
for: (range.upperBoundExpression, .descending),
821+
requiresCompleteEntries: false,
794822
comparator: directIndexBoundComparator
795823
)
796824
}
@@ -834,6 +862,7 @@ extension DiskPersistence.Transaction {
834862
} else {
835863
try await index.insertionCursor(
836864
for: (range.lowerBoundExpression, .ascending),
865+
requiresCompleteEntries: false,
837866
comparator: secondaryIndexBoundComparator
838867
)
839868
}
@@ -857,6 +886,7 @@ extension DiskPersistence.Transaction {
857886
} else {
858887
try await index.insertionCursor(
859888
for: (range.upperBoundExpression, .descending),
889+
requiresCompleteEntries: false,
860890
comparator: secondaryIndexBoundComparator
861891
)
862892
}

‎Tests/CodableDatastoreTests/DiskPersistenceDatastoreIndexTests.swift‎

Lines changed: 48 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@
1212
import XCTest
1313
@testable import CodableDatastore
1414

15+
fileprivate struct SortError: Error, Equatable {}
16+
1517
final class DiskPersistenceDatastoreIndexTests: XCTestCase {
1618
var temporaryStoreURL: URL = FileManager.default.temporaryDirectory
1719

@@ -26,7 +28,10 @@ final class DiskPersistenceDatastoreIndexTests: XCTestCase {
2628
func assertPageSearch(
2729
proposedEntry: UInt8,
2830
pages: [[DatastorePageEntryBlock]],
31+
requiredContentLength: Int? = nil,
32+
requiresCompleteEntries: Bool = false,
2933
expectedIndex: Int?,
34+
expectedSearchFailure: Bool = false,
3035
file: StaticString = #filePath,
3136
line: UInt = #line
3237
) async throws {
@@ -67,13 +72,25 @@ final class DiskPersistenceDatastoreIndexTests: XCTestCase {
6772
pageInfos.append(.existing(pageID))
6873
}
6974

70-
let result = try await index.pageIndex(for: proposedEntry, in: pageInfos) { [pageLookup] pageID in
71-
pageLookup[pageID]!
72-
} comparator: { lhs, rhs in
73-
lhs.sortOrder(comparedTo: rhs.headers[0][0])
75+
do {
76+
let result = try await index.pageIndex(
77+
for: proposedEntry,
78+
in: pageInfos,
79+
requiresCompleteEntries: requiresCompleteEntries
80+
) { [pageLookup] pageID in
81+
pageLookup[pageID]!
82+
} comparator: { lhs, rhs in
83+
if let requiredContentLength, rhs.content.count != requiredContentLength {
84+
throw SortError()
85+
}
86+
return lhs.sortOrder(comparedTo: rhs.headers[0][0])
87+
}
88+
89+
XCTAssertEqual(result, expectedIndex, file: file, line: line)
90+
XCTAssertFalse(expectedSearchFailure, file: file, line: line)
91+
} catch is SortError {
92+
XCTAssertTrue(expectedSearchFailure, "Encountered unexpected error", file: file, line: line)
7493
}
75-
76-
XCTAssertEqual(result, expectedIndex, file: file, line: line)
7794
}
7895

7996
func assertInsertionCursor(
@@ -121,7 +138,7 @@ final class DiskPersistenceDatastoreIndexTests: XCTestCase {
121138
pageInfos.append(.existing(pageID))
122139
}
123140

124-
let result = try await index.insertionCursor(for: RangeBoundExpression.including(proposedEntry), in: pageInfos) { [pageLookup] pageID in
141+
let result = try await index.insertionCursor(for: RangeBoundExpression.including(proposedEntry), in: pageInfos, requiresCompleteEntries: false) { [pageLookup] pageID in
125142
pageLookup[pageID]!
126143
} comparator: { lhs, rhs in
127144
lhs.sortOrder(comparedTo: rhs.headers[0][0], order: .ascending)
@@ -208,6 +225,29 @@ final class DiskPersistenceDatastoreIndexTests: XCTestCase {
208225
try await assertPageSearch(proposedEntry: 4, pages: pages, expectedIndex: 1)
209226
}
210227

228+
func testSplitContentBlockSearch() async throws {
229+
let entry1 = DatastorePageEntry(headers: [[1]], content: Array(repeating: 1, count: 100)).blocks(remainingPageSpace: 20, maxPageSpace: 1024)
230+
let entry3 = DatastorePageEntry(headers: [[3]], content: Array(repeating: 3, count: 100)).blocks(remainingPageSpace: 20, maxPageSpace: 1024)
231+
232+
let pages = [
233+
[entry1[0]],
234+
[entry1[1], entry3[0]],
235+
[entry3[1]]
236+
]
237+
238+
try await assertPageSearch(proposedEntry: 0, pages: pages, requiredContentLength: 100, requiresCompleteEntries: false, expectedIndex: 0, expectedSearchFailure: true)
239+
try await assertPageSearch(proposedEntry: 1, pages: pages, requiredContentLength: 100, requiresCompleteEntries: false, expectedIndex: 0, expectedSearchFailure: true)
240+
try await assertPageSearch(proposedEntry: 2, pages: pages, requiredContentLength: 100, requiresCompleteEntries: false, expectedIndex: 0, expectedSearchFailure: true)
241+
try await assertPageSearch(proposedEntry: 3, pages: pages, requiredContentLength: 100, requiresCompleteEntries: false, expectedIndex: 1, expectedSearchFailure: true)
242+
try await assertPageSearch(proposedEntry: 4, pages: pages, requiredContentLength: 100, requiresCompleteEntries: false, expectedIndex: 1, expectedSearchFailure: true)
243+
244+
try await assertPageSearch(proposedEntry: 0, pages: pages, requiredContentLength: 100, requiresCompleteEntries: true, expectedIndex: 0, expectedSearchFailure: false)
245+
try await assertPageSearch(proposedEntry: 1, pages: pages, requiredContentLength: 100, requiresCompleteEntries: true, expectedIndex: 0, expectedSearchFailure: false)
246+
try await assertPageSearch(proposedEntry: 2, pages: pages, requiredContentLength: 100, requiresCompleteEntries: true, expectedIndex: 0, expectedSearchFailure: false)
247+
try await assertPageSearch(proposedEntry: 3, pages: pages, requiredContentLength: 100, requiresCompleteEntries: true, expectedIndex: 1, expectedSearchFailure: false)
248+
try await assertPageSearch(proposedEntry: 4, pages: pages, requiredContentLength: 100, requiresCompleteEntries: true, expectedIndex: 1, expectedSearchFailure: false)
249+
}
250+
211251
func testTwoPageBackwardsBleedingBlockSearch() async throws {
212252
let entry1 = DatastorePageEntry(headers: [[1]], content: [1]).blocks(remainingPageSpace: 1024, maxPageSpace: 1024)
213253
let entry3 = DatastorePageEntry(headers: [[3]], content: [3]).blocks(remainingPageSpace: 7, maxPageSpace: 1024)
@@ -381,7 +421,7 @@ final class DiskPersistenceDatastoreIndexTests: XCTestCase {
381421
let exp = expectation(description: "Finished")
382422
Task { [pageInfos, pageLookup] in
383423
for _ in 0..<1000 {
384-
_ = try await index.pageIndex(for: UInt64.random(in: 0..<1000000), in: pageInfos) { pageID in
424+
_ = try await index.pageIndex(for: UInt64.random(in: 0..<1000000), in: pageInfos, requiresCompleteEntries: false) { pageID in
385425
pageLookup[pageID]!
386426
} comparator: { lhs, rhs in
387427
lhs.sortOrder(comparedTo: try UInt64(bigEndianBytes: rhs.headers[0]))

0 commit comments

Comments
 (0)