1. 13 Jul, 2019 1 commit
    • Nathan Harris's avatar
      Make `RedisConnection.sendCommandsImmediately` public · fae8eada
      Nathan Harris authored
      Motivation:
      
      During the refactor work (commit ea7c755d) 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
      fae8eada
  2. 09 Jul, 2019 4 commits
    • Nathan Harris's avatar
      Iterate on type safety for `zadd` · 0ecb3c1e
      Nathan Harris authored
      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.
      0ecb3c1e
    • Nathan Harris's avatar
      Rename `RedisCommand` properties to avoid overloading terms and being more specific. · 26057638
      Nathan Harris authored
      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.
      26057638
    • Nathan Harris's avatar
      7e7e3546
    • Nathan Harris's avatar
      Rename `RedisNIOError` to `RedisClientError` · 13432f0c
      Nathan Harris authored
      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.
      13432f0c
  3. 05 Jul, 2019 1 commit
    • Nathan Harris's avatar
      Refactor `RedisConnection` · ea7c755d
      Nathan Harris authored
      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.
      ea7c755d
  4. 04 Jul, 2019 3 commits
    • Nathan Harris's avatar
      b807af58
    • Nathan Harris's avatar
      60 -- Provide Strong Option Types in SortedSet Commands · 64232992
      Nathan Harris authored
      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.
      64232992
    • Nathan Harris's avatar
      59 -- Use `RESPValueConvertible` as Generic Constraint · fa227b0e
      Nathan Harris authored
      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.
      fa227b0e
  5. 03 Jul, 2019 7 commits
    • Nathan Harris's avatar
      Revisit `RESPValue` and `RESPValueConvertible` implementations. · cd9bd04f
      Nathan Harris authored
      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.
      cd9bd04f
    • Nathan Harris's avatar
      Rename `RedisCommandContext` to `RedisCommand`, improve `RedisCommandHandler`... · 5fb0bfca
      Nathan Harris authored
      Rename `RedisCommandContext` to `RedisCommand`, improve `RedisCommandHandler` usage semantics, and cleanup documentation.
      
      Motivation:
      
      During proposal review, it was noted that `RedisCommandContext` was a bit misleading, and the hidden reference semantics worrisome.
      
      In addition, several of parts of the documentation around `RedisCommandHandler` were weak or also misleading.
      
      Modifications:
      
      - Rename `RedisCommandContext` to just `RedisCommand`
      - Update documentation to be more explicit about the module who owns the types being referenced
      - Update documentation to call out explicit usage semantics and behavior
      - Change `RedisCommandHandler` to close the socket connection on error thrown
      - Rename the "base" Redis Channel Handlers to be more explicitly named
      
      Result:
      
      Users should have clearer documentation on what happens when using `RedisCommandHandler` and `RedisCommand` without hidden semantics.
      
      This contributes to issue #47.
      5fb0bfca
    • Nathan Harris's avatar
      Update `RedisClient.expire` to no longer use `deadline` terminology. · b96f64c7
      Nathan Harris authored
      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
      b96f64c7
    • Nathan Harris's avatar
      Change `Redis.makeDefaultClientBootstrap` to `ClientBootstrap.makeRedisTCPClient` · 5d232ad0
      Nathan Harris authored
      Motivation:
      
      During proposal review, feedback was provided that the discoverability of the factory method for building a standard RESP `ChannelPipeline` was poor outside of documentation.
      
      Modifications:
      
      - Move `Redis.makeDefaultClientBootstrap` to `ClientBootstrap.makeRedisTCPClient`.
      - Move the `channelInitializer` implementation into a new `Channel.addBaseRedisHandlers()` instance method.
      
      Result:
      
      Users should have an easier time discovering how to easily create baseline RESP `ChannelPipelines`.
      
      This contributes to #47.
      5d232ad0
    • Nathan Harris's avatar
      Refactor `RESPTranslator` to mutate the passed `ByteBuffer` directly. · b8c19488
      Nathan Harris authored
      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
      b8c19488
    • Nathan Harris's avatar
      Add `fromRESP` label to `RESPValueConvertible.init` · 60d5c4ce
      Nathan Harris authored
      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
      60d5c4ce
    • Nathan Harris's avatar
      Make `RedisCommandHandler` final · 1281724a
      Nathan Harris authored
      Motivation:
      
      During SSWG review, feedback was provided that forced a re-evaluation of early design feedback interpretation on composability of RedisNIO.
      
      First understanding was that the desire was to have "customization points" to gain benefits of implementation of RedisCommandHandler, while new understanding is that it just needs to be public in order for users to include it in their own custom ChannelPipeline schemes.
      
      Modifications:
      
      `RedisCommandHandler` is now a final class.
      
      Result:
      
      Users will no longer be able to subclass `RedisCommandHandler`, but gain a super slight performance increase.
      
      This contributes to #47.
      1281724a
  6. 25 Jun, 2019 3 commits
  7. 22 Jun, 2019 3 commits
  8. 12 Jun, 2019 1 commit
    • Nathan Harris's avatar
      Add static property for default RedisConnection port · 73b3f5df
      Nathan Harris authored
      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.
      73b3f5df
  9. 11 Jun, 2019 3 commits
    • Nathan Harris's avatar
      Fix `Data` Conversion to `RESPValue` · 34647e14
      Nathan Harris authored
      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.
      34647e14
    • Nathan Harris's avatar
    • Nathan Harris's avatar
      Improve debugging of `RESPValue` · d4584924
      Nathan Harris authored
      Motivation:
      
      Reading output from `RESPValue` existentials is entirely verbose and does not provide a human-readable description of what is being represented.
      
      Modifications:
      
      - Add conformance to `CustomStringConvertible` for `RESPValue`
      - Remove redundant logging of arguments in `RedisConnection.send`
      
      Result:
      
      String representations of `RESPValue` should now be more readable for human to understand.
      d4584924
  10. 06 Jun, 2019 1 commit
    • Nathan Harris's avatar
      Rename `NIORedis` to `RedisNIO` · e81f9546
      Nathan Harris authored
      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.
      e81f9546
  11. 05 Jun, 2019 1 commit
    • Nathan Harris's avatar
      Implement Basic Metrics (#40) · 9df65396
      Nathan Harris authored
      Motivation:
      
      End users will want to capture some baseline performance metrics that can only be accurately gathered at this level of the stack.
      
      Modifications:
      
      Added new `RedisMetrics` struct that retains all SwiftMetrics objects used throughout the system lifecycle, and appropriate code to track desired metrics.
      
      Result:
      
      Users now have a way to peek into the system's performance to understand what NIORedis is being asked to do from their usage.
      9df65396
  12. 27 May, 2019 3 commits
    • Nathan Harris's avatar
      Add Blocking List Pop Commands · eaa9d2bf
      Nathan Harris authored
      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.
      eaa9d2bf
    • Nathan Harris's avatar
      Add Blocking Sorted Set Pop Commands · 73922325
      Nathan Harris authored
      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.
      73922325
    • Nathan Harris's avatar
      Improve Redis Channel Pipeline Debugging · d0da3e76
      Nathan Harris authored
      Motivation:
      
      Debugging and properly handling errors was difficult as the `ChannelHandler` names were the default generic names, in addition the `RedisCommandHandler` did not pipe errors further up the pipeline.
      
      Modifications:
      
      `RedisCommandHandler` now calls `context.fireErrorCaught(error)` when receiving errors, and the default Redis `ChannelPipeline` handlers are added with debugging names.
      
      Result:
      
      It should be easier for debugging purposes to work with any Redis `ChannelPipeline`.
      d0da3e76
  13. 02 May, 2019 2 commits
    • Nathan Harris's avatar
      Minor comment fixes · d9d61baf
      Nathan Harris authored
      d9d61baf
    • Nathan Harris's avatar
      Update Readme and Tweak `Redis.makeConnection(...)` · 0131fe43
      Nathan Harris authored
      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.
      0131fe43
  14. 01 May, 2019 4 commits
    • Nathan Harris's avatar
    • Nathan Harris's avatar
      Resolve `RedisPipeline` Proposal Discussion Topic · 066a5c3d
      Nathan Harris authored
      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`.
      066a5c3d
    • Nathan Harris's avatar
      Simplify RESP Parsing and Redis Channel Handlers · 477668e6
      Nathan Harris authored
      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.
      477668e6
    • Nathan Harris's avatar
      Namespace global static factory methods under `Redis` · e446a834
      Nathan Harris authored
      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(...)`
      e446a834
  15. 30 Apr, 2019 1 commit
    • Nathan Harris's avatar
      Standardize Error Handling · 46e96bd9
      Nathan Harris authored
      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`.
      46e96bd9
  16. 24 Apr, 2019 2 commits