Skip to content

Conversation

@ndyakov
Copy link
Member

@ndyakov ndyakov commented Nov 18, 2025

This PR introduces typed errors for the library, utilising errors.As for error handling instead of the string checks that were used before. This way, errors can be wrapped and set to the command in hooks without breaking the library functionality.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces typed Redis errors to improve error handling when errors are wrapped by hooks or other middleware. The implementation replaces string-based error checking with type-based checking using Go's errors.As() function, allowing error type detection to work correctly even through multiple layers of error wrapping.

Key changes:

  • Added typed error structs for common Redis errors (LoadingError, ReadOnlyError, MovedError, AskError, ClusterDownError, TryAgainError, MasterDownError, MaxClientsError)
  • Implemented helper functions for type-safe error checking that work with wrapped errors
  • Updated error handling logic in shouldRetry() and isMovedError() to use typed checks
  • Added comprehensive test coverage for typed errors and error wrapping scenarios
  • Updated documentation with examples of error wrapping in hooks

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
internal/proto/redis_errors.go Defines typed error types and parsing logic for common Redis errors
internal/proto/redis_errors_test.go Unit tests for typed error creation, wrapping, and address extraction
internal/proto/reader.go Updates ParseErrorReply to use typed error parsing
error.go Updates retry logic to use typed error checks and exports public error checking functions
error_wrapping_test.go Integration tests demonstrating typed errors work with hook wrapping
README.md Documents typed error checking functions and error wrapping patterns in hooks

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ndyakov ndyakov requested a review from Copilot November 18, 2025 15:03
Copilot finished reviewing on behalf of ndyakov November 18, 2025 15:06
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

error.go:117

  • The string fallback for MASTERDOWN errors is missing. While there's a typed error check on line 91, the fallback string check for backward compatibility with plain errors is not present. This could cause retries to not work for wrapped non-typed MASTERDOWN errors.

Add the following after line 115:

if strings.HasPrefix(s, "MASTERDOWN ") {
    return true
}
	// Fallback to string checking for backward compatibility with plain errors
	s := err.Error()
	if strings.HasPrefix(s, "ERR max number of clients reached") {
		return true
	}
	if strings.HasPrefix(s, "LOADING ") {
		return true
	}
	if strings.HasPrefix(s, "READONLY ") {
		return true
	}
	if strings.HasPrefix(s, "CLUSTERDOWN ") {
		return true
	}
	if strings.HasPrefix(s, "TRYAGAIN ") {
		return true
	}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ndyakov ndyakov marked this pull request as ready for review November 18, 2025 16:56
@ndyakov ndyakov changed the title WIP!: [CAE-1784] typed errors [CAE-1784] typed errors Nov 18, 2025
@ndyakov ndyakov changed the title [CAE-1784] typed errors chore(errors): [CAE-1784] typed errors Nov 18, 2025
Copy link
Contributor

@htemelski-redis htemelski-redis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a great and much needed addition
Well done

@ndyakov ndyakov changed the title chore(errors): [CAE-1784] typed errors feat(errors): Introduce typed errors Nov 19, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ndyakov ndyakov force-pushed the ndyakov/CAE-1784-typed-errors branch from 67ca39b to 6b5a569 Compare November 19, 2025 14:37
@ndyakov ndyakov merged commit 6c24f60 into master Nov 19, 2025
21 checks passed
@ndyakov ndyakov deleted the ndyakov/CAE-1784-typed-errors branch November 19, 2025 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants