From 519f1b08727da894b3984b035ccaf67ae809c0e6 Mon Sep 17 00:00:00 2001 From: Olivier Duclos Date: Wed, 5 Apr 2023 17:41:37 +0200 Subject: [PATCH] REORG/MEDIUM: use readable configuration errors This breaks the API of configuration.ConfError! Replace the configuration error codes by error strings, easier to understand. This slightly modifies error messages, but the error variable names remain unchanged. The method ConfError.Code() has been removed. Use ConfError.Is() or errors.Is() the compare errors. --- configuration/backend_test.go | 2 +- configuration/cache_test.go | 2 +- configuration/configuration.go | 12 ++--- configuration/errors.go | 63 +++++++++++++++------------ configuration/frontend_test.go | 2 +- configuration/log_forward_test.go | 2 +- configuration/mailers_section_test.go | 2 +- configuration/peer_section_test.go | 2 +- configuration/resolver_test.go | 2 +- configuration/ring_test.go | 2 +- configuration/transaction-handling.go | 2 +- configuration/transaction.go | 4 +- 12 files changed, 52 insertions(+), 45 deletions(-) diff --git a/configuration/backend_test.go b/configuration/backend_test.go index 67036dbb..8e228a84 100644 --- a/configuration/backend_test.go +++ b/configuration/backend_test.go @@ -983,7 +983,7 @@ func TestCreateEditDeleteBackend(t *testing.T) { err = clientTest.DeleteBackend("created", "", 999999999) if err != nil { if confErr, ok := err.(*ConfError); ok { - if confErr.Code() != ErrVersionMismatch { + if !confErr.Is(ErrVersionMismatch) { t.Error("Should throw ErrVersionMismatch error") } } else { diff --git a/configuration/cache_test.go b/configuration/cache_test.go index 59748534..d23712a8 100644 --- a/configuration/cache_test.go +++ b/configuration/cache_test.go @@ -178,7 +178,7 @@ func TestCreateEditDeleteCache(t *testing.T) { err = clientTest.DeleteCache("created_cache", "", 999999) if err != nil { if confErr, ok := err.(*ConfError); ok { - if confErr.Code() != ErrVersionMismatch { + if !confErr.Is(ErrVersionMismatch) { t.Error("Should throw ErrVersionMismatch error") } } else { diff --git a/configuration/configuration.go b/configuration/configuration.go index e7fa4f1c..c48d9ca0 100644 --- a/configuration/configuration.go +++ b/configuration/configuration.go @@ -95,7 +95,7 @@ func (c *client) GetParser(transactionID string) (parser.Parser, error) { p, ok := c.parsers[transactionID] c.clientMu.Unlock() if !ok { - return nil, NewConfError(ErrTransactionDoesNotExist, fmt.Sprintf("Transaction %s does not exist", transactionID)) + return nil, NewConfError(ErrTransactionDoesNotExist, transactionID) } return p, nil } @@ -109,7 +109,7 @@ func (c *client) AddParser(transactionID string) error { _, ok := c.parsers[transactionID] c.clientMu.Unlock() if ok { - return NewConfError(ErrTransactionAlreadyExists, fmt.Sprintf("Transaction %s already exists", transactionID)) + return NewConfError(ErrTransactionAlreadyExists, transactionID) } parserOptions := []parser_options.ParserOption{} @@ -150,7 +150,7 @@ func (c *client) DeleteParser(transactionID string) error { _, ok := c.parsers[transactionID] c.clientMu.Unlock() if !ok { - return NewConfError(ErrTransactionDoesNotExist, fmt.Sprintf("Transaction %s does not exist", transactionID)) + return NewConfError(ErrTransactionDoesNotExist, transactionID) } delete(c.parsers, transactionID) return nil @@ -165,7 +165,7 @@ func (c *client) CommitParser(transactionID string) error { p, ok := c.parsers[transactionID] c.clientMu.Unlock() if !ok { - return NewConfError(ErrTransactionDoesNotExist, fmt.Sprintf("Transaction %s does not exist", transactionID)) + return NewConfError(ErrTransactionDoesNotExist, transactionID) } c.parser = p delete(c.parsers, transactionID) @@ -180,7 +180,7 @@ func (c *client) GetVersion(transactionID string) (int64, error) { func (c *client) getVersion(transactionID string) (int64, error) { p, err := c.GetParser(transactionID) if err != nil { - return 0, NewConfError(ErrCannotReadVersion, fmt.Sprintf("Cannot read version: %s", err.Error())) + return 0, NewConfError(ErrCannotReadVersion, err.Error()) } data, _ := p.Get(parser.Comments, parser.CommentsSectionName, "# _version", true) @@ -194,7 +194,7 @@ func (c *client) IncrementVersion() error { ver.Value++ if err := c.parser.Save(c.ConfigurationFile); err != nil { - return NewConfError(ErrCannotSetVersion, fmt.Sprintf("Cannot set version: %s", err.Error())) + return NewConfError(ErrCannotSetVersion, err.Error()) } return nil } diff --git a/configuration/errors.go b/configuration/errors.go index 87fed9c2..ae7a0bf0 100644 --- a/configuration/errors.go +++ b/configuration/errors.go @@ -16,60 +16,67 @@ package configuration import ( + "errors" "fmt" oaerrors "github.com/go-openapi/errors" ) -const ( +var ( // General error, unknown cause - ErrGeneralError = 0 + ErrGeneralError = errors.New("unknown error") // Errors regarding configurations - ErrNoParentSpecified = 10 - ErrParentDoesNotExist = 11 - ErrBothVersionTransaction = 12 - ErrNoVersionTransaction = 13 - ErrValidationError = 14 - ErrVersionMismatch = 15 + ErrNoParentSpecified = errors.New("unspecified parent") + ErrParentDoesNotExist = errors.New("missing parent") + ErrBothVersionTransaction = errors.New("both version and transaction specified, specify only one") + ErrNoVersionTransaction = errors.New("version or transaction not specified") + ErrValidationError = errors.New("validation error") + ErrVersionMismatch = errors.New("version mismatch") - ErrTransactionDoesNotExist = 20 - ErrTransactionAlreadyExists = 21 - ErrCannotParseTransaction = 22 + ErrTransactionDoesNotExist = errors.New("transaction does not exist") + ErrTransactionAlreadyExists = errors.New("transaction already exist") + ErrCannotParseTransaction = errors.New("failed to parse transaction") - ErrObjectDoesNotExist = 30 - ErrObjectAlreadyExists = 31 - ErrObjectIndexOutOfRange = 32 + ErrObjectDoesNotExist = errors.New("missing object") + ErrObjectAlreadyExists = errors.New("object already exists") + ErrObjectIndexOutOfRange = errors.New("index out of range") - ErrErrorChangingConfig = 40 - ErrCannotReadConfFile = 41 - ErrCannotReadVersion = 42 - ErrCannotSetVersion = 43 + ErrErrorChangingConfig = errors.New("invalid change") + ErrCannotReadConfFile = errors.New("failed to read configuration") + ErrCannotReadVersion = errors.New("cannot read version") + ErrCannotSetVersion = errors.New("cannot set version") - ErrCannotFindHAProxy = 50 + ErrCannotFindHAProxy = errors.New("failed to find HAProxy") - ErrClientDoesNotExists = 60 + ErrClientDoesNotExists = errors.New("client does not exist") ) // ConfError general configuration client error type ConfError struct { - code int - msg string + err error + reason string // optional +} + +func (e *ConfError) Err() error { + return e.err } // Error implementation for ConfError func (e *ConfError) Error() string { - return fmt.Sprintf("%v: %s", e.code, e.msg) + if e.reason == "" { + return e.err.Error() + } + return fmt.Sprintf("%s: %s", e.err.Error(), e.reason) } -// Code returns ConfError code, which is one of constants in this package -func (e *ConfError) Code() int { - return e.code +func (e *ConfError) Is(target error) bool { + return e.err == target } // NewConfError constructor for ConfError -func NewConfError(code int, msg string) *ConfError { - return &ConfError{code: code, msg: msg} +func NewConfError(err error, reason string) *ConfError { + return &ConfError{err: err, reason: reason} } // CompositeTransactionError helper function to aggregate multiple errors diff --git a/configuration/frontend_test.go b/configuration/frontend_test.go index c3896dce..2a474b36 100644 --- a/configuration/frontend_test.go +++ b/configuration/frontend_test.go @@ -655,7 +655,7 @@ func TestCreateEditDeleteFrontend(t *testing.T) { err = clientTest.DeleteFrontend("created", "", 999999) if err != nil { if confErr, ok := err.(*ConfError); ok { - if confErr.Code() != ErrVersionMismatch { + if !confErr.Is(ErrVersionMismatch) { t.Error("Should throw ErrVersionMismatch error") } } else { diff --git a/configuration/log_forward_test.go b/configuration/log_forward_test.go index c565be84..2ab07aa6 100644 --- a/configuration/log_forward_test.go +++ b/configuration/log_forward_test.go @@ -129,7 +129,7 @@ func TestCreateEditDeleteLogForward(t *testing.T) { err = clientTest.DeleteLogForward("created_log_forward", "", 999999) if err != nil { if confErr, ok := err.(*ConfError); ok { - if confErr.Code() != ErrVersionMismatch { + if !confErr.Is(ErrVersionMismatch) { t.Error("should throw ErrVersionMismatch error") } } else { diff --git a/configuration/mailers_section_test.go b/configuration/mailers_section_test.go index 66c5be90..0fa23727 100644 --- a/configuration/mailers_section_test.go +++ b/configuration/mailers_section_test.go @@ -145,7 +145,7 @@ func TestCreateEditDeleteMailersSection(t *testing.T) { err = clientTest.DeleteMailersSection("newMailer", "", 999999) if err != nil { if confErr, ok := err.(*ConfError); ok { - if confErr.Code() != ErrVersionMismatch { + if !confErr.Is(ErrVersionMismatch) { t.Error("Should throw ErrVersionMismatch error") } } else { diff --git a/configuration/peer_section_test.go b/configuration/peer_section_test.go index 10f57f3f..0be4eb0a 100644 --- a/configuration/peer_section_test.go +++ b/configuration/peer_section_test.go @@ -176,7 +176,7 @@ func TestCreateEditDeletePeerSection(t *testing.T) { err = clientTest.DeletePeerSection("testcluster", "", 999999) if err != nil { if confErr, ok := err.(*ConfError); ok { - if confErr.Code() != ErrVersionMismatch { + if !confErr.Is(ErrVersionMismatch) { t.Error("Should throw ErrVersionMismatch error") } } else { diff --git a/configuration/resolver_test.go b/configuration/resolver_test.go index 0880a65b..31ac1ad6 100644 --- a/configuration/resolver_test.go +++ b/configuration/resolver_test.go @@ -125,7 +125,7 @@ func TestCreateEditDeleteResolver(t *testing.T) { err = clientTest.DeleteResolver("created_resolver", "", 999999) if err != nil { if confErr, ok := err.(*ConfError); ok { - if confErr.Code() != ErrVersionMismatch { + if !confErr.Is(ErrVersionMismatch) { t.Error("Should throw ErrVersionMismatch error") } } else { diff --git a/configuration/ring_test.go b/configuration/ring_test.go index 128676ef..8f8ba78a 100644 --- a/configuration/ring_test.go +++ b/configuration/ring_test.go @@ -145,7 +145,7 @@ func TestCreateEditDeleteRing(t *testing.T) { err = clientTest.DeleteRing("created_ring", "", 999999) if err != nil { if confErr, ok := err.(*ConfError); ok { - if confErr.Code() != ErrVersionMismatch { + if !confErr.Is(ErrVersionMismatch) { t.Error("should throw ErrVersionMismatch error") } } else { diff --git a/configuration/transaction-handling.go b/configuration/transaction-handling.go index 73e46e24..0d4f30f2 100644 --- a/configuration/transaction-handling.go +++ b/configuration/transaction-handling.go @@ -85,7 +85,7 @@ func (c *client) GetFailedParserTransactionVersion(transactionID string) (int64, ver, ok := data.(*types.ConfigVersion) if !ok { - return 0, NewConfError(ErrCannotReadVersion, "cannot read version") + return 0, NewConfError(ErrCannotReadVersion, "") } return ver.Value, nil } diff --git a/configuration/transaction.go b/configuration/transaction.go index b9edc34a..19a4d8c2 100644 --- a/configuration/transaction.go +++ b/configuration/transaction.go @@ -316,7 +316,7 @@ func (t *Transaction) CheckTransactionOrVersion(transactionID string, version in // start an implicit transaction if transaction is not already given tID := "" if transactionID != "" && version != 0 { - return "", NewConfError(ErrBothVersionTransaction, "both version and transaction specified, specify only one") + return "", NewConfError(ErrBothVersionTransaction, "") } if transactionID == "" && version == 0 { return "", NewConfError(ErrNoVersionTransaction, "version or transaction not specified, specify only one") @@ -685,7 +685,7 @@ func (t *Transaction) getFailedTransactionVersion(transactionID string) (int64, ver, err := t.TransactionClient.GetFailedParserTransactionVersion(transactionID) if err != nil { - return 0, NewConfError(ErrCannotReadVersion, "cannot read version") + return 0, NewConfError(ErrCannotReadVersion, "") } return ver, nil }