Error handlers

[bmorton]

Accidentally opened this before it was ready — I’ve updated the description below:

This PR adds error handlers similar to discussed in #16. I started implementing this at the RedisStoreAdapter level, but this seemed better because it can be used more broadly and doesn’t require implementation at each of the stores.

I tried following other patterns in the repo, but I’m happy to change things that seem overly complex. I’m undecided if I want to include something into the repo for handling failures more gracefully because there doesn’t seem to be a good way to hook it into the implementing application's instrumentation framework. Having an error handler that just silently swallows these things seemed like it would promote a bad practice. Instead, I think I will implement the error handler in my application and use this to integrate it. Thoughts?

I still need to add some documentation and update the change log if you’re happy with this approach.

[bmorton]

Looks like I also have some tests to fix for other versions. I’ll take a look at that too.

[bmorton]

Hmm, I'm going to have to dig deeper into that failure. I'm not sure I understand why it's happening yet.

[DmitryTsepelev]

So far so good! Added some comments, master is ready for rebasing

[DmitryTsepelev]

we probably could allow a callable object (like lambda) here

[bmorton]

Yep, good idea! Added.

[DmitryTsepelev]

Probably the time has come to make an util method and reuse it here and in the StoreAdapters module 🙂

[bmorton]

I took a swing at this. There's not too much to abstract away, but there's less duplication now. Let me know what you think or if I missed the point.

[bmorton]

Tests are passing now! I'm gonna get this wired up in my app and prove it out. If there are no other concerns with the approach, I'll write up docs and update the changelog. Thanks!

[bmorton]

Tested this in our app and it seems to work as intended. Mainly, I want to ensure that a network partition or latency between the app and Redis doesn't cause complete failure but instead degrades into a "QueryNotPersisted" failure followed by the client sending the query. It's definitely a degraded state, but better than complete failure I think.

Here's the error handler I'm using to accomplish this. It seems like this is a "bring your own" error handler, but it could go into this project if you think it'd be useful for others. I think an example would probably be good enough though.

class GracefulRedisErrorHandler < GraphQL::PersistedQueries::ErrorHandlers::BaseErrorHandler
  ERROR_DETAILS = "Redis failed while trying to retrieve or save a persisted query"

  def initialize(logger)
    @logger = logger
  end

  def call(error)
    case error
    when Redis::BaseError
      # Treat Redis errors as a cache miss, but ensure we still log the error for visibility
      logger.graphql_operation_error(message: ERROR_DETAILS, error: error)
    else
      raise error
    end

    # Return nothing to ensure handled errors are treated as cache misses
    return # rubocop:disable Style/RedundantReturn
  end

  private

  def logger
    if @logger.is_a?(Proc)
      @logger.call
    else
      @logger
    end
  end
end

[DmitryTsepelev]

Looks awesome! Please take a look at my proposal on camelize, and I'll be happy to merge it in as soon as you're done with the changelog and docs ✨

[DmitryTsepelev]

This looks like a perfect case for a String refinement. What do you think?

[bmorton]

Personally, I'm not a big fan of refinements. In this case, it would need to be on the Symbol class, so I'd expect camelize to return a symbol, but it needs to be a string. I'm happy to make the change if you think it'd improve the project, but I'm afraid it makes things less clear and adds too much magic here.

[DmitryTsepelev]

Sounds reasonable, it does not sound like an improvement

[DmitryTsepelev]

Thanks for the amazing work, 0.2.0 is on its way! ✨