Skip to content

Implements MemcachedValue Protocol and Conformances #22

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

Merged
merged 7 commits into from
Jul 19, 2023

Conversation

dkz2
Copy link
Contributor

@dkz2 dkz2 commented Jul 19, 2023

This PR addresses the need to enhance our type safety by implementing a MemcachedValue protocol that manages the conversion of generic types to ByteBuffer. We have provided conformance for standard Swift types (Int, UInt, String, etc.) to MemcachedValue. This PR will close #21.

Motivation:
To improve the robustness and versatility of our interactions with Memcached servers, we need to ensure that our API can handle a variety of data types safely and efficiently. This is achieved by the introduction of a MemcachedValue Protocol, which allows for a streamlined conversion of generic types to and from ByteBuffer.

Modifications:

  • Implemented a MemcachedValue protocol to handle the conversion of generic types to ByteBuffer.
  • Provided conformances for standard Swift types (Int, UInt, String, etc.) to MemcachedValue.
  • Updated the get and set method in theConnection Actor to use our new MemcachedValue protocol.
  • Added Integration test for Connection Actor and Unit test for MemcachedValue.

Result:
Our API can now handle a wider array of Swift types when interacting with Memcahed servers, via our new MemcachedValue protocol.

Comment on lines 140 to 141
/// - Returns: A `Value` containing the fetched value, or `nil` if no value was found.
public func get<Value: MemcachedValue>(_ key: String) async throws -> Value? {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for this PR, but we probably want to create something like a MemcacheItem<Value: MemcacheValue> in the future that we return here. This way we could attach metadata such as the TTL or other opaque flags that the user passed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also change this method slightly to make it a bit more ergonomic to spell

Suggested change
/// - Returns: A `Value` containing the fetched value, or `nil` if no value was found.
public func get<Value: MemcachedValue>(_ key: String) async throws -> Value? {
/// - Returns: A `Value` containing the fetched value, or `nil` if no value was found.
public func get<Value: MemcachedValue>(_ key: String, as valueType: Value.Type = Value.self) async throws -> Value? {

@@ -159,6 +159,7 @@ public actor MemcachedConnection {
}
}.value

return Value.readFromBuffer(&response!)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't force unwrap here. In what cases can we get nil here?

///
/// - Parameter buffer: The ByteBuffer to which the integer should be written.
public func writeToBuffer(_ buffer: inout ByteBuffer) {
buffer.reserveCapacity(MemoryLayout<Self>.size)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we have to do this. writeInterger is making sure that there is enough capacity before writing.

///
/// - Parameter buffer: The ByteBuffer to which the string should be written.
public func writeToBuffer(_ buffer: inout ByteBuffer) {
buffer.reserveCapacity(buffer.writerIndex + self.count)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

///
/// - Parameter buffer: The ByteBuffer from which the value should be read.
public static func readFromBuffer(_ buffer: inout ByteBuffer) -> Self? {
guard let string = buffer.readString(length: buffer.readableBytes) else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can just return buffer.readString(length: buffer.readableBytes) in this method

Comment on lines 21 to 24
// Gracefully shutdown the event loop group when it's no longer needed
defer {
try! group.syncShutdownGracefully()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have to do this if we use a static let

defer {
try! group.syncShutdownGracefully()
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please use the @main entry point instead of top level code. This could look like the following

@main
struct Program {
    static func main() async throws {
        // Your code goes here
    }
}

Importantly you have to rename your main.swift file to something like Program.swift

}

// Instantiate a new MemcachedConnection actor with host, port, and event loop group
let connectionActor = MemcachedConnection(host: "127.0.0.1", port: 11211, eventLoopGroup: group)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove the actor from the name here. Just call it memcachedConnection

// Instantiate a new MemcachedConnection actor with host, port, and event loop group
let connectionActor = MemcachedConnection(host: "127.0.0.1", port: 11211, eventLoopGroup: group)

// Use of Swift new structured concurrency model to run the connection and perform operations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove that comment

Suggested change
// Use of Swift new structured concurrency model to run the connection and perform operations

// This opens the connection and handles requests until the task is cancelled or the connection is closed
group.addTask { try await connectionActor.run() }

// Set a value for a key. This is an async operation so we use "await".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can drop the comments around async here and in the next comments

/// - Returns: A `ByteBuffer` containing the server's response to the set request.
public func set(_ key: String, value: String) async throws -> ByteBuffer? {
public func set<Value: MemcachedValue>(_ key: String, value: Value) async throws -> ByteBuffer? {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we are at it we should probably remove the ByteBuffer return here. I don't think that makes much sense right now.

@FranzBusch FranzBusch merged commit 67477dd into swift-server:main Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce MemcachedValue Protocol and Conformances
2 participants