-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose the paging token API from the C/C++ driver #22
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,8 @@ import Foundation | |
import Logging | ||
import NIO | ||
|
||
public protocol PagingStateToken: ContiguousBytes {} | ||
|
||
extension CassandraClient { | ||
/// Resulting row(s) of a Cassandra query. Data are returned all at once. | ||
public final class Rows: Sequence { | ||
|
@@ -46,6 +48,35 @@ extension CassandraClient { | |
Iterator(rows: self) | ||
} | ||
|
||
/// Returns a reusable paging token. | ||
/// | ||
/// - Warning: This token is not suitable or safe for sharing externally. | ||
public func opaquePagingStateToken() throws -> OpaquePagingStateToken { | ||
try OpaquePagingStateToken(token: self.rawPagingStateToken()) | ||
} | ||
|
||
private func rawPagingStateToken() throws -> [UInt8] { | ||
var buffer: UnsafePointer<CChar>? | ||
var length = 0 | ||
|
||
// The underlying memory is freed with the Rows result | ||
let result = cass_result_paging_state_token(self.rawPointer, &buffer, &length) | ||
guard result == CASS_OK, let bytesPointer = buffer else { | ||
throw CassandraClient.Error(result) | ||
} | ||
|
||
let tokenBytes: [UInt8] = bytesPointer.withMemoryRebound(to: UInt8.self, capacity: length) { | ||
let bufferPointer = UnsafeBufferPointer(start: $0, count: length) | ||
return Array(unsafeUninitializedCapacity: length) { storagePointer, storageCount in | ||
var (unwritten, endIndex) = storagePointer.initialize(from: bufferPointer) | ||
precondition(unwritten.next() == nil) | ||
storageCount = storagePointer.distance(from: storagePointer.startIndex, to: endIndex) | ||
} | ||
} | ||
|
||
return tokenBytes | ||
} | ||
|
||
public final class Iterator: IteratorProtocol { | ||
public typealias Element = Row | ||
|
||
|
@@ -283,6 +314,20 @@ extension CassandraClient { | |
cass_value_is_null(self.rawPointer) == cass_true | ||
} | ||
} | ||
|
||
/// A reusable page token that can be used by `Statement` to resume querying | ||
/// at a specific position. | ||
public struct OpaquePagingStateToken: PagingStateToken { | ||
let token: [UInt8] | ||
|
||
internal init(token: [UInt8]) { | ||
self.token = token | ||
} | ||
|
||
public func withUnsafeBytes<R>(_ body: (UnsafeRawBufferPointer) throws -> R) rethrows -> R { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how is this used by clients? ideally we reduce unsafe API surface area There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh I think I see, this is used as an "opaque" entity the users pass down to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually want to use this feature myself externally, but doing it safely like the Java implementation. So I need access to the guts. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately, building that safety into the library itself would have necessitated pulling in additional dependencies and increasing the footprint of the library considerably, so I wanted to keep it simple here and allow users to use the token in a way that makes sense for their application. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you shed more light on what that could look like? generally ew prefer not promote unsafe APIs and here the only public API is unsafe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What the Java implementation does is take the paging token, along with the query, and all the bound parameters, and hashes them together. It then also encodes the protocol version in the token. That way the token can only be used on the exact same query it came from, along with the same protocol it came from. I originally had implemented that, but I needed a hashing algorithm to use. There doesn't appear to be anything within the Swift standard library, so I brought in SwiftCrypto. Unfortunately that requires a higher platform target, so I would have to bump that as well. That seemed like a lot to change just to support this paging token. Maybe there's a better option I'm not thinking of? Otherwise we could just expose the raw byte array and get rid of the contiguous bytes implementation. What do you think? |
||
try self.token.withUnsafeBytes(body) | ||
} | ||
} | ||
} | ||
|
||
// MARK: - Int8 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we use precondition elsewhere? otherwise maybe guard and throe an error so we dont crash the program?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it is used quite extensively. I am always a bit apprehensive about using it for the reason you highlighted.
Someone suggested this in an earlier review. This isn't a normal error condition, so I don't think throwing is appropriate. For this to fail either means we passed differing lengths in to one of those calls (a really bad bug), or something has seriously gone wrong in the runtime internally.
As a guard against a programming error, it seems a little excessive, but were it to be a reality I think crashing is actually appropriate as things have totally gone off the rails.