Closed bmorton closed 4 years ago
Looks like I also have some tests to fix for other versions. I’ll take a look at that too.
Hmm, I'm going to have to dig deeper into that failure. I'm not sure I understand why it's happening yet.
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!
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
Thanks for the amazing work, 0.2.0
is on its way! ✨
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.