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:
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:
The assertions in `errorCaught(context:error:)` and `channelRead(context:data:)` are holdovers from when SwiftLog was not integrated into the package. The conditions where they were triggered are safely handled in other ways, so the asserts are not guarding against undefined behavior or bad app state.
Modifications:
Remove the two asserts regarding the `RedisCommandHandler.commandResponseQueue` to rely on Logging to bubble up errors to the user.
Result:
Debug builds using RediStack should not encounter unexpected assertions and should rely on other, better, error handling.
This contributes to issue #65
Motivation:
After enabling the thread sanitizer during testing, a data race with the `activeConnectionCount` in `RedisMetrics` was caught due to changing a primitive `Int` across threads.
Modifications:
Add a specialized class `ActiveConnectionGauge` to wrap the semantics of the `activeConnectionCount` with an `Atomic<Int>` property.
Result:
No data races should occur with the `RedisMetrics` subsystem.
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:
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:
During the refactor work (commit ea7c755) that was merged with MRs !71 and !53 - `sendCommandsImmediately` was accidentally lowered to `internal`
Modifications:
Properly mark `RedisConnection.sendCommandsImmediately` as `public`
Result:
Developers should now have proper access to the `sendCommandsImmediately` property
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:
There are several cases where "command" could refer to a command keyword, or an entire message (keyword + args). This made working with `RedisCommand` and it's documentation ambiguous.
Modifications:
- Rename `RedisCommand.command` to `message`
- Rename initializer labels to `message` and `responsePromise`
Result:
When encountering a `RedisCommand` everyone should know that they are dealing with a message that should be sent to Redis as soon as possible.
Nathan Harris
Liam Don