Motivation:
The `CNIOSHA1` module from SwiftNIO is an implementation detail. The fact that we were able to import and build the project with it is a bug/feature of Swift Package Manager.
Should the Swift ecosystem standardize on an implementation, that will replace the one we have chosen later.
Modifications:
Use the SHA1 native Swift implementation from CryptoSwift by reproducing the necessary code in our project.
Updates to the appropriate files to provide attribution to CryptoSwift have been included.
Result:
A reliable and pure Swift implementation of the SHA1 digest algorithm in the project for the sake of Lua script cache hashing.
### General usage
Most users should only need to use the `evalScript` method. We take the approach recommended by the [redis EVAL docs](https://redis.io/commands/eval):
>>>
The client library implementation can always optimistically send EVALSHA under the hood even when the client actually calls EVAL, in the hope the script was already seen by the server. If the NOSCRIPT error is returned EVAL will be used instead.
>>>
To facilitate this, we introduce a RedisScript struct, which calculates the sha1 hash on the client and stores it alongside the script source. I took inspiration for this approach from the [radix golang library](https://github.com/mediocregopher/radix). The struct makes using scripts a bit easier/nicer, and guides the developer toward the highest performance approach, but I'm curious whether you think it's an unnecessary abstraction.
One disadvantage of the optimistic approach using `EVALSHA` is that the `evalScript` may not be atomic if the script is not cached yet, which will be a problem if we are using it during an explicit pipeline or MULTI/EXEC operation. The redis docs recommend using `SCRIPT LOAD` before a multi/exec operation to avoid this. I think we could leave this up to the application developer.
### Should we implement the basic commands?
The approach above forces the application developer to use the evalScript method instead of exposing either `EVAL` or `EVALSHA` directly. This is to avoid the behavior of only using `EVAL` without caching, which might otherwise seem like the natural choice. If you think this is too opinionated, we can also add basic `eval` and `evalSha` methods.
### SHA1 calculation
I've copied the approach used by the SwiftNIO developers - RediStack already includes the `CNIOSHA1` module required.
There's a comment in [their private implementation](https://github.com/apple/swift-nio/blob/master/Sources/NIOWebSocket/SHA1.swift#L17) which indicates they're not entirely happy with this, but consider it the best approach for server-side Swift for now. I think we could follow their lead and migrate to a standard library method if/when it becomes available.
The `sha1` method is added as an extension property on String. Because it can theoretically fail, the `sha1` property is optional and thus the RedisScript initializer is failable, which is a bit unpleasant. I don't think the sha1 calculation should never actually fail given a String input. A force unwrap in the sha1 property would remove several code smells [like this](https://gitlab.com/liamdon/swift-redi-stack/blob/scripting/Sources/RediStack/Commands/ScriptingCommands.swift#L92), but I left it out in case you have a strong opposition to force unwraps - let me know what you think.
### Not implemented
I have not implemented `SCRIPT DEBUG` or `SCRIPT KILL`, because these seem more like sysadmin tasks that should not be in a client library. But they can easily be added if we want full coverage of the scripting commands.
Motivation:
The SortedSet and List range commands (LTRIM, LRANGE, ZRANGE, etc.) are stringly-based and not flexible with Swift syntax.
Modifications:
- Add overloads of LTRIM that support the gambit of Range Standard Library types
- Rework LRANGE to mirror LTRIM method signatures
- Rework ZScore Range based commands to be more type-safe with `RedisZScoreBound` enum
- Rework ZLex Range based commands to be more type-safe with `RedisZLexBound` enum
- Rework ZCOUNT, ZLEXCOUNT, ZRANGE, ZREVRANGE, ZREMRANGEBYLEX, ZREMRANGEBYRANK, ZREMRANGEBYSCORE methods to be more type-safe and support Swift Range syntax
Result:
Working with SortedSet ranges should be much more type safe, and expressive with Swift's Range syntax.
Motivation:
While reviewing the API, the current design does not read well, and still has room for misunderstanding the actual end result of a ZADD operation.
Modifications:
- Rename `RedisSortedSetAddOption` to `RedisZaddInsertBehavior` and update cases to match desired use site syntax.
- Add `RedisZaddReturnBehavior` enum to define how `zadd` should calculate the return value.
- Update `zadd` and its overloads to support the two new enums in the form of `zadd(_:to:inserting:returning:)`
Result:
The more "Swifty" API will make it much more clear to developers at the call site what the actual behavior of the ZADD command will be.
Motivation:
The goal is to have a strong-typed API for type-safety in arbitrary values, such as trying to use
Int to represent time - as '3' could mean any unit of time, leaving many places for errors and bugs.
Modifications:
Switch all current APIs that accept a `timeout` argument to use `NIO.TimeAmount` instead of a plain `Int`.
Result:
Developers will have an easier time reasoning about their own code as to what values might mean when working with
timeouts in Redis APIs.
Motivation:
Inspired by Swift by Sundell's article on type-safe identifers, the goal of this commit is to have the compiler
assist in preventing incorrect Redis key values from being used in API calls.
See https://www.swiftbysundell.com/articles/type-safe-identifiers-in-swift/ for the inspiration.
Modifications:
- Add new `RedisKey` struct that wraps around a single `String` value that conforms to several expected protocols
(Hashable, Comparable, Codable, etc.)
- Change all command APIs to require `RedisKey` rather than plain strings
Result:
When encountering an API requiring a RedisKey, it should be much more apparant at the use site what form a value should take.
Motivation:
Logging is more dynamic in real world usage than the current static heavy API allows.
Users generally want to be capable of updating connection logger metadata to attach dynamic properties such as an HTTP request ID for log tracing.
Modifications:
- Move all logs to `RedisConnection`
- Add `id: UUID` property to `RedisConnection`
- Add `logging` property and `setLogging(to:)` method requirements to `RedisClient`
- Add chainable `logging(to:)` method extension to `RedisClient`
- Add additional `trace` log statements to `RedisConnection`
- Change when `RedisConnection.init` logging and metric calls are made
- Change some `debug` log statements to `trace in `RedisConnection`
Result:
Users should have infinitely more flexibility in how RedisConnection, and RedisClient implementations in general, behave in regards to logging.
Motivation:
For ergonomics, users sometimes want to provide arguments as a variadic list rather than an array.
Modifications:
- Add variadic overloads for almost all methods that accept lists of homogenous types
Result:
Users should have more flexibility in the way arguments are passed to command methods
Motivation:
It is wrong to always assume that a GET operation is expecting a String response type, as users may be storing other types of data.
Modifications:
- Add `get` generic method with a constraint for types of `RESPValueConvertible` to convert values to the user desired type
- Change existing `get` method to specialize the generic overload
- Fix incorrect doc block regarding the ELF failure condition
Result:
Users should now be able to specialize the return type of a "GET" command
Motivation:
While working to add more test coverage with `RESPTranslator`, it was made apparent that a `.bulkString(.none)` is impossible to create directly with the `RESPValue` initializers, even though it is a reasonable possibility.
Additionally, forcing all integer types to have to be stored in an `Int` is unnecessarily restrictive.
Modifications:
- Change `RESPValue.init(bulk:)` initializers to accept `Optional` instances
- Change `RESPValue.init(bulk:)` for `Int` initializer to be generic on `FixedWidthInteger`
Result:
Converting types to and from `RESPValue` should be more bi-directional and seamless.
Motivation:
Diagnostics for why `.bulkString` parses might fail were weak, and edge cases fell through gaps in coverage were found.
Modifications:
Added new cases to `RESPTranslator.ParsingError` for `.bulkString` parsing with additional test coverage.
Result:
Users should have better diagnostics for bogus data or failed parsing state.
Motivation:
While working on issue #56, it was forgotten to add the new test cases to the linux manifest file.
Modifications:
Update the linux manifests to include all current unit tests
Result:
All written unit tests should be ran on Linux
Motivation:
After working with RedisKit with RediStackTestUtils as a dependency, it was realized how opinionated the module is in how RedisConnections can be created in test environments.
Modifications:
Require more information, with reasonable defaults for `RedisConnection.init()`. Provide subclass hooks for `RedisIntegrationTestCase` for implementors to make decisions for themselves at how to connect to Redis.
Result:
Users should have more freedom in how they connect to Redis in their units tests.
Motivation:
For users looking to contribute, and for those looking to validate the library, it was unclear what tests require an actual connection to a Redis instance in order to run.
Modifications:
Add a `RediStackIntegrationTests` that takes all tests that require a Redis instance in order to run.
Result:
Those looking to run just unit tests, or contribute new tests, can now directly point to a specific testTarget as defined in the Package manifest.
Motivation:
There is a reasonable way to compare if two `RedisErrors` are equal, which was seen as needed in the `Equatable` conformance for `RESPValue`.
Modifications:
Added `Equatable` conformance for `RedisError` by comparing the messages.
Result:
Two `RedisError` instances are now equatable.
Motivation:
While working on unit tests the need for conformance to `Equatable` for `RESPValue` has been needed a few times and it was decided to make it public.
Modifications:
Added conformance to `Equatable` for `RESPValue` with unit test.
Result:
Users should now be able to compare two `RESPValue` instances for equality.
Motivation:
A handful of times, unit tests were forgotten to be added to the `allTests` extension, or were incorrectly copy/pasted.
Modifications:
Remove manual entries of `allTest` and use generated result from `swift test --generate-linuxmain`
Result:
There should be proper test parity between macOS and Linux.
Motivation:
Issue #60 called for improving the type safety of the options available for the `zadd` command, and MR !70 made some great headway, but attempted to cram too much into a single enum.
Modifications:
- Break the `RedisSortedSetAddOption.returnChangedCount` value into an additional boolean param
Result:
Using `zadd` should now be more straight forward, while being type safe.
Motivation:
To make it a little more generic, and to avoid turnover during renames (such as the planned rebranding in issue #61), `RedisClientError` more accurately reflects the source of the errors, as well as the responsibility of causing the bug.
Modifications:
- Rename `RedisNIOError` to `RedisClientError`
- Rename `RedisError` file to `RedisErrors`
- Add documentation of `RedisClientError`
- Remove no longer used `.unsupportedOperation(method:message:)` value
- Rename `.responseConversion(to:)` to `.failedRESPConversion(to:)`
Result:
Names of `RedisClientError` should be more descriptive, less prone to turnover, and more documented for users to understand the issues related to these thrown errors.
Motivation:
During proposal review, and while working within the codebase, several issues were identified with how `RedisConnection` was architectured.
Modifications:
- Change implementation of `RedisConnection` in some areas for new logic of internal `ConnectionState`
- Change behavior of logging in a few places
- The initializer for `RedisConnection` is now **internal**
- How users can override the default `ClientBootstrap` for a connection is by passing an instance to the `.connect` static method
- Change unit tests to inherit from a common XCTestCase class that handles creation and cleanup of `RedisConnection` in tests
- Remove Redis namespace enum
Result:
The API for `RedisConnection` should be much simpler, with the implementation being less buggy.
This resolves issues #49, #54, and #57.
Motivation:
While working through issue #59, it was noticed just how "stringly" the SortedSet command options for `zadd`, `zinterstore`, and `zunionstore` were, and Swift provides ways of having strong type safety for these options.
Modifications:
- Add `RedisSortedSetAddOption` and `RedisSortedSetAggregateMethod` to replace the String API in `zadd`, `zinterstore`, and `zunionstore`
- Fix an implication of how `overestimatedCountBeingAdded` documentation for `Array where Element == RESPValue` for `add(contentsOf:overestimatedCountBeingAdded:_:)`
Result:
Users should have a more discoverable and straightforward way that isn't error prone for calling `zadd`, `zinterstore`, and `zunionstore` with Redis supported options.
Motivation:
Johannes continues to provide great insight, and correctly pointed out that `RESPValueConvertible` was being used as an "existential" in all cases.
This can cause unexpected type-erasure and introduce unnecessary cost overhead with dynamic dispatch when in most cases we know the exact value we want for `RESPValue` to execute commands.
Modifications:
- Add new extensions to `Array where Element == RESPValue` for appending and adding elements into them
- Change `RedisClient.send(command:with:)` to require `[RESPValue]` instead of `[RESPValueConvertible]` as the `with` argument type
- Change all instances of `RESPValueConvertible` being an "existential" type for method arguments to instead be a generic constraint
Result:
The library should be safeguarded from a class of bugs, with the use of `send` being a bit more straight forward, with some new convenience methods for `[RESPValue]` types.
Motivation:
Johannes provided a fair code review of the project and summarized his findings in issue #48, and one of the prime offenders was all of the `unsafe*` APIs (pointers, buffers, bytes)
that were used with `RESPValue` and `RESPValueConvertible`.
He also provided great feedback and pointed out good points of confusion with the API design of `RESPValue` and `RESPValueConvertible`.
Modifications:
- Return to using `Array` instead of `ContiguousArray` for `RESPValue.array` storage
- Update all documentation to be more thorough in explaining how the types should be used and conformed to.
- Remove all uses of `unsafe*` APIs where possible
- Change implementations to be a lot more type and memory safe, double checking assumptions
- Remove conformance to `ExpressibleBy*Literal` as it is too easy for users to shoot themselves in the foot and saves only a few characters over `.init(bulk:)`
- Create new `RedisNIOTestUtils` target for common test extensions, making them public
- Move most almost all implementations of `RESPValue` computed properties into the `RESPValueConvertible` conformances
Result:
Users should be more safeguarded by the API against unknowingly getting incorrect `RESPValue` representations, the API design of `RESPValue` and `RESPValueConvertible` should be much clearer,
and memory safety should be at a higher bar from these changes.
This resolves issues #55 & #48, and contributes to issue #47.
Motivation:
During proposal review, it was appropriately pointed out that `RedisClient.expire` incorrectly mixes 'deadline' and 'timeout' terminology.
Modifications:
- Change references of 'deadline' to 'timeout' to follow Redis' established semantics for 'EXPIRE'
- Add additional unit test for `RedisClient.expire`
Result:
`RedisClient.expire` should now be more clear as to its semantics and not mix terminology incorrectly.
This contributes to issue #47
Motivation:
During proposal review, it was pointed out that the code with a position index was redundant and error prone over relying on `ByteBuffer`'s `readerIndex`.
Modifications:
Refactored `RESPTranslator` to rely on `ByteBuffer.readerIndex` for position of current parsing cursor,
and eliminated `ParsingResult` enum to instead return `RESPValue?`.
The implementation for writing out `RESPValue` has been expanded to `RESPValueConvertible` and moved to an extension of `ByteBuffer`.
Result:
Parsing `RESPValue` into and out of `ByteBuffer` should be less error-prone and more straight forward.
This contributes to issues #47 and #55
Motivation:
During SSWG review, feedback was provided on the API design of the `RESPValueConvertible.init` signature and how it should appropriately follow Swift Design Guidelines regarding labels.
Modifications:
`RESPValueConvertible.init(_:` is now `RESPValueConvertible.init(fromRESP:)`.
Result:
There should be more clarity at the call site when initializing a type from a `RESPValue`.
This contributes to #47
Motivation:
The default port for Redis is well published to be 6379, and it is common to want to pass this value around or use it as a static default value in methods and initializers.
Modifications:
Add `RedisConnection.defaultPort` static property for all users to use, and update references of the `6379` literals to use new the new property.
Result:
Users should have a reliable default defined to use everywhere to avoid bugs.
Motivation:
`Foundation.Data` was unexpectedly receiving `RESPValueConvertible` conformance through the `Collection` extension which lead to incorrect encoding.
Modifications:
Add explicit conformance to `RESPValueConvertible` for `Data`.
Result:
Users should see their `Data` encoding to RESP format as expected.
Motivation:
Now that the project has migrated from GitHub, it can now fully use GitLab CI.
Modifications:
Removed `circle.yml` and added `.gitlab-ci.yml` files
Result:
GitLab CI should be fully supported
Motivation:
The SSWG has identified a fast approaching reality of namespace clashes in SPM within the ecosystem and has proposed a rule on names that `NIORedis` no longer complies with.
Modifications:
All references to `NIORedis` have been switched to `RedisNIO` as this module name is unique (at least within GitHub's public repositories).
The goals for this name are as follows:
1. To indicate that this is a Redis client library that is built with SwiftNIO
2. That it is a lower level library, as it directly exposes SwiftNIO as an implementation detail
2a. The idea being that a higher level library (`Redis`) will be used, and to "go one level deeper" in the stack, you append the "deeper" `NIO` postfix
3. It follows a naming pattern adopted by Vapor who has expressed their desire to adopt this library as their Redis implementation
Result:
A repository, package name, and module name that are unique across GitHub's public repositories that achives the goals outlined above.
Motivation:
To be a comprehensive library, all commands should be implemented, even if they are highly discouraged. List's collection of commands were missing `brpop`, `blpop`, and `brpoplpush`.
Modifications:
`brpop`, `blpop` and `brpoplpush` are supported with defaults and overloads for an easier API.
Result:
Users now have access to `brpop`, `blpop` and `brpoplpush` commands.
Motivation:
To be a comprehensive library, all commands should be implemented, even if they are highly discouraged. Sorted Set's collection of commands were missing `bzpopmin` and `bzpopmax`.
Modifications:
`bzpopmin` and `bzpopmax` are supported with defaults and overloads for an easier API.
`RedisClient.channel` is now `internal` to have access during testing for bypassing normal guards for closing connections.
Result:
Users now have access to `bzpopmin` and `bzpopmax` commands.
Motivation:
The goal of the `Redis.makeConnection` factory method is to provide end users with a quick way to jump in and get started with Redis in Swift.
Right now, users have to provide an `EventLoopGroup` instance, when a reasonable default is available for us to define.
Modifications:
- Add: `MultiThreadedEventLoopGroup` for 1 thread as a default argument to the `using:` label in `Redis.makeConnection`
- Remove: The `with:` label for the password in `Redis.makeConnection`
- Change: The project README to reflect the current state of the project
Results:
Users should be able to create `RedisConnections` by just defining an IP Address & Port to connect to - and possibly a password.
In addition, the README should now properly direct users on how to use the latest version of the library.
Motivation:
During the discussion thread of the NIORedis SSWG proposal, there were concerns expressed over the implementation
of `RedisPipeline` that covered the following areas:
1. Clashes with SwiftNIO concepts such as "Pipeline"
2. Misleading users on library behavior with using a Pipeline vs. a single `RedisConnection`
3. The implementation was "too clever" in that it allowed users to easily find themselves in "Undefined Behavior"
due to lack of enough type safety in the API
However, the value in being able to control how frequently "flush" happens on a socket was discussed and considered
high - so something still needs to be in place to force flushes by users.
It was decided to leave the implementation of batching commands and their responses to library users, perhaps in
higher level frameworks while the library will provide said mechanism for controlling writing and flushing of commands.
Modifications:
- Add: `sendCommandsImmediately` bool property to `RedisConnection` to handle choice of flushing after each command
- Remove: `RedisPipeline`
Results:
Users should now have a more clear understanding on what type of control they have over the timing of when commands
are actually sent over the wire, with the greater emphasis placed on `RedisConnection`.
Motivation:
As it stands, the parsing / encoding implementations for RESP was directly tied to the NIO Channel Handlers.
Unit tests were sloppily spread across multiple files and it made it difficult to explicitly separate out
the Channel Handler behavior from the RESP specification implementation.
Modifications:
- Add: `RESPTranslator` enum helper object with static methods that only handles RESP parsing / encoding
- Rename: `RESPEncoder` to `RedisMessageEncoder`
- Rename: `RESPDecoder` to `RedisByteDecoder`
Results:
It should be easier to understand what type is intended to be used as part of a NIO channel pipeline while
still having a direct way of parsing / encoding between Swift types and the RESP specification.
Unit tests should be more maintainable as well.
Motivation:
The two provided factory methods for creating `RedisConnection` and `ClientBootstrap` were not readily discoverable or appropriately expressible.
Modifications:
- Add: `Redis` top-level namespace enum
- Move & Rename: `RedisConnection.connect` factory method to `Redis.makeConnection`
- Move & Rename: `ClientBootstrap.makeRedisDefault` factory method to `Redis.makeDefaultClientBootstrap`
- Rename: `EventLoopFuture` extension file to just `SwiftNIO` to have all framework extensions in a single file
Results:
Using NIORedis should be more discoverable and straight forward on how to create a connection with `Redis.makeConnection(...)` over `RedisConnection.connect(...)`
Motivation:
`RedisError` was being used in many places in an ad-hoc fashion that made it unclear as to what source of errors it exactly represents.
In addition, it doesn't follow community conventions of handling known "classes" of errors with enums rather than static struct instances.
Results:
All errors that are generated and thrown within the library are specified as either `RESPDecoder.Error` or `NIORedisError`.
`RedisError` is simplified to represent an error message sent by a Redis instance that conforms to `LocalizedError`.
Motivation:
Throughout the process of creating and using RESPValue, many unnecessary allocations happened translating ByteBuffers into [Int8] raw byte arrays.
Results:
RESPValue has a more conherent underlying data structure and computed properties, while decoding RESP is more straight forward and performant
Resolves#31
Motivation:
`Foundation.Data` is known to be more expensive than is necessary for this low level of a library, and has some quirks with its usage.
Results:
This library now works with byte arrays (`[UInt8]`) directly, and all references to `Foundation` now explicitly import the exact types they need.
Motivation:
`RedisDriver` has one of the weakest use cases for existence, as its entire purpose can be solved with a static function on `RedisConnection`.
Result:
`RedisDriver` no longer is a thing, and a simpler process of creating a `RedisConnection` is available through the static method `connect(to:with:on:logger:)`.
Also, the `ClientBootstrap.makeForRedis(using:)` method has been cleaned up and renamed to `ClientBootstrap.makeRedisDefault(using:)`.
Motivation:
`RedisPipeline` has a weak use case as a protocol, as it is more than likely 98% of use cases will end up using the `NIORedisPipeline` implementation.
Result:
`RedisPipeline` is more direct in its intended usage, with comment blocks being more clear.
Motivation:
`RedisCommandExecutor` was a complex and "wordy" name that was not 100% clear as to how it relates to other types.
`RedisConnection` also has not had a strong use case shown for it to exists as a separate protocol - using up a great name for the "out of the box" implementation.
Result:
`RedisClient` instead of `RedisCommandExecutor` is more clear as to what it is, in Redis terminology, a communication client.
`RedisConnection` as a concrete class provides an identifiable basic block for making connections to Redis.
`RedisConnection` also saw some fixes to `close()` while having some names and comment blocks tweaked for updated naming.
Nathan Harris
Liam Don
Nathan Harris
Ondrej Rafaj