Replace dynamically generated errors with constants #711

iamnotacake · 2020-09-01T15:32:04Z

Only errors that contain static text message were replaced since they remain constant until the end of runtime.
Packages cell, compare, keys, message, session now contain more error constants, one may use them to compare with error returned by some function from the same package.
ErrOverflow will be deprecated starting from 0.14.0, ErrOutOfMemory is recommended instead due to more understandable name.

Checklist

The coding guidelines are followed
Changelog is updated

Only errors that contain static text message were replaced.

Replace errors created with `errors.NewCallbackError()`.

gothemis/compare/compare.go

gothemis/cell/cell.go

gothemis/compare/compare.go

gothemis/keys/keypair.go

gothemis/session/session.go

Lagovas · 2020-09-01T15:54:29Z

gothemis/session/session.go

+	ErrMissingClientID           = errors.NewWithCode(errors.InvalidParameter, "empty client ID for Secure Session")
+	ErrMissingPrivateKey         = errors.NewWithCode(errors.InvalidParameter, "empty client private key for Secure Session")
+	ErrMissingMessage            = errors.NewWithCode(errors.InvalidParameter, "empty message for Secure Session")
+	ErrOverflow                  = errors.NewWithCode(errors.NoMemory, "Secure Session cannot allocate enough memory")


Not clear what exactly overflow. Memory, int capacity, buffer capacity, amount of requests.... Let's give the name which enough just to read and it will describe what exactly overflowed?)

What about ErrOverflow -> ErrOutOfMemory? Quite common name for such types of errors.

UPD: I guess it was called ErrOverflow because in ~~most~~ all cases this error was returned when sizeOverflow() returned false (which means passed size couldn't fit in 32-bit signed int). BTW, I noticed this func implementation is duplicated few times:

cell/cell.go 235:func sizeOverflow(n C.size_t) bool { message/message.go 108:func sizeOverflow(n C.size_t) bool { compare/compare.go 104:func sizeOverflow(n C.size_t) bool { session/session.go 80:func sizeOverflow(n C.size_t) bool { keys/keypair.go 129:func sizeOverflow(n C.size_t) bool {

so maybe it's better to implement it once and use some imports

Yeah, it is much better. Advice for the future. It's good to search similar cases in the other code of project or in similar projects to not introduce something new and use the most common names/patterns/etc. Probably we used some errors like that in other parts of project)

Yeah, all these checks are for size overflow, but from Go viewpoint this can be treated as out-of-memory error. Themis Core asks Go to allocate a ginormous buffer with a length that can't even be represented with Go types, so Go cannot allocate that much memory.

Give errors more verbose names

ilammy

All of these changes are introducing new public API or change existing API. I really want to get them straight as we'd have to live with the stupid constant names we choose now. The fewer we add or touch, the better. Here is some bikeshedding from my part. I normally do not use "request changes" unless the code is seriously broken, but in this case I'll keep it on until the naming is okay.

Also, as we do change the API, I guess this warrants a changelog entry.

gothemis/compare/compare.go

ilammy · 2020-09-01T17:51:20Z

gothemis/cell/cell.go

+	ErrGetOutputSize     = errors.New("Failed to get output size")
+	ErrEncryptData       = errors.New("Failed to protect data")
+	ErrDecryptData       = errors.New("Failed to unprotect data")


Go's convention seems to start error messages with lowercase. There are some legacy error messages which do not follow this convention. I would ask you to fix the messages to use lowercase while extracting them into constants. (For avoidance of doubt, "Secure Cell" is a proper name so it should be capitalized.)

still upper case error messages

gothemis/compare/compare.go

gothemis/keys/symmetric.go

gothemis/message/message.go

gothemis/cell/cell.go

ilammy · 2020-09-01T18:20:17Z

gothemis/session/session.go

+	ErrGetRequestSize            = errors.New("Failed to get request size")
+	ErrGenRequest                = errors.New("Failed to generate request")
+	ErrGetWrappedSize            = errors.New("Failed to get wrapped size")
+	ErrWrapData                  = errors.New("Failed to wrap data")
+	ErrGetUnwrappedSize          = errors.New("Failed to get unwrapped size")
+	ErrUnwrapData                = errors.New("Failed to unwrap data")
+	ErrCallbackGetUnwrappedSize  = errors.NewCallbackError("Failed to get unwraped size (get_public_key_by_id callback error)")
+	ErrCallbackUnwrapData        = errors.NewCallbackError("Failed to unwrap data (get_public_key_by_id callback error)")
+	ErrCallbackGetRemoteIDLen    = errors.NewCallbackError("Failed to get session remote id length")
+	ErrCallbackBadRemoteIDLen    = errors.NewCallbackError("Incorrect remote id length (0)")
+	ErrCallbackGetRemoteID       = errors.NewCallbackError("Failed to get session remote id")


Honestly, I'd just squash all those variations into just two:

ErrMessageSize — when we fail to get appropriate message size

ErrMessageData — when processing something goes wrong

There may be some interest in whether the first or the second call to Themis Core failed, but I don't think that we need 10 new API entries to express basically the same thing: your data seems to be broken.

What do you think, everyone? (Naming is a discussion point too.)

actually, we had some situations when we read logs and messages like that which include context where it occurs, could save our time) actually in Acra, where we can meet errors in keystore, which use secure cell, in transport package which uses secure session, in decryption module, which uses secure cell + secure message. And info about where it occurs, like could not process something in cell or message, or session, could help to understand part where was error.

In my opinion, there isn't really much use for having an error for each individual method.

Themis Core has a signle THEMIS_FAIL for all failures, not a bunch of

THEMIS_SECURE_CELL_SEAL_ENCRYPT_FAIL

THEMIS_SECURE_CELL_TOKEN_PROTECT_DECRYPT_FAIL

THEMIS_SECURE_MESSAGE_VERIFY_FAIL

unique for each method. The context should be provided by a higher level means.

Discriminating between Secure Cell and Secure Message failures may be useful for a system where Secure Cell is used in only one subsystem and Secure Message is used in a different one. Then by observing the messages you can guess where the failure is. However, if multiple subsystems use Secure Cell then you still need some context for disambiguation – context added by the application, not Themis. And if you have this context in the first place, you don't really need to rely on Cell/Message distinction specifically to pinpoint the faulty subsystem.

gothemis/session/session.go

* Update changelog * Deprecate ErrOverflow, use ErrOutOfMemory instead * Rename many errors, remove redundant ones

* Replace ErrGetRequestSize, ErrGetWrappedSize, ErrGetUnwrappedSize with ErrMessageSize * Replace ErrGenerateRequest, ErrWrapData, ErrUnwrapData with ErrMessageData

CHANGELOG.md

gothemis/cell/cell.go

ilammy

Well, I believe this is mergeable now. Thanks for tedious updates, @iamnotacake! 💪

iamnotacake · 2020-09-04T09:56:52Z

@Lagovas could you please approve the PR or tell what exactly is still missing?

Lagovas

missed to change error messages to lowercase. after that lgtm

Lagovas · 2020-09-04T10:57:44Z

gothemis/cell/cell.go

+	ErrGetOutputSize     = errors.New("Failed to get output size")
+	ErrEncryptData       = errors.New("Failed to protect data")
+	ErrDecryptData       = errors.New("Failed to unprotect data")


still upper case error messages

Lagovas · 2020-09-04T10:58:08Z

gothemis/compare/compare.go

-	ErrMissingSecret = errors.NewWithCode(errors.InvalidParameter, "empty secret for Secure Comparator")
-	ErrMissingData   = errors.NewWithCode(errors.InvalidParameter, "empty comparison message for Secure Comparator")
-	ErrOverflow      = errors.NewWithCode(errors.NoMemory, "Secure Comparator cannot allocate enough memory")
+	ErrAppendSecret             = errors.New("Failed to append secret")


iamnotacake added 2 commits September 1, 2020 12:49

Replace dymanically generated errors with const ones

1cf684b

Only errors that contain static text message were replaced.

Replace dymanically generated callback errors with const ones

de4f0d0

Replace errors created with `errors.NewCallbackError()`.

iamnotacake added the W-GoThemis 🐹 Wrapper: GoThemis, Go API label Sep 1, 2020

iamnotacake requested review from ilammy and Lagovas September 1, 2020 15:32

iamnotacake requested a review from vixentael as a code owner September 1, 2020 15:32

vixentael reviewed Sep 1, 2020

View reviewed changes

gothemis/compare/compare.go Outdated Show resolved Hide resolved

Lagovas requested changes Sep 1, 2020

View reviewed changes

iamnotacake added 2 commits September 1, 2020 19:18

Updates after review

bec889d

Give errors more verbose names

Rename ErrOverflow -> ErrOutOfMemory

2505434

ilammy changed the title ~~Replace dymanically generated errors with const ones~~ Replace dynamically generated errors with constants Sep 1, 2020

ilammy requested changes Sep 1, 2020

View reviewed changes

iamnotacake added 2 commits September 2, 2020 16:24

Updates after review

f204717

* Update changelog * Deprecate ErrOverflow, use ErrOutOfMemory instead * Rename many errors, remove redundant ones

Unify few session errors into ErrMessageSize and ErrMessageData

89d2876

* Replace ErrGetRequestSize, ErrGetWrappedSize, ErrGetUnwrappedSize with ErrMessageSize * Replace ErrGenerateRequest, ErrWrapData, ErrUnwrapData with ErrMessageData

ilammy reviewed Sep 2, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

gothemis/cell/cell.go Outdated Show resolved Hide resolved

Fix typo in changelog, change deprecation version

f4f0857

ilammy approved these changes Sep 2, 2020

View reviewed changes

vixentael approved these changes Sep 3, 2020

View reviewed changes

Lagovas reviewed Sep 4, 2020

View reviewed changes

Make error messages start with lowercase

2e5a258

Lagovas approved these changes Sep 4, 2020

View reviewed changes

iamnotacake merged commit 68a91e9 into cossacklabs:master Sep 5, 2020

iamnotacake deleted the T977-use-statically-created-errors branch September 5, 2020 13:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace dynamically generated errors with constants #711

Replace dynamically generated errors with constants #711

iamnotacake commented Sep 1, 2020 •

edited

Loading

Lagovas Sep 1, 2020

iamnotacake Sep 1, 2020 •

edited

Loading

Lagovas Sep 1, 2020

ilammy Sep 1, 2020 •

edited

Loading

ilammy left a comment

ilammy Sep 1, 2020

Lagovas Sep 4, 2020

ilammy Sep 1, 2020 •

edited

Loading

vixentael Sep 1, 2020

Lagovas Sep 2, 2020

ilammy Sep 2, 2020

ilammy left a comment

iamnotacake commented Sep 4, 2020

Lagovas left a comment

Lagovas Sep 4, 2020

Lagovas Sep 4, 2020

Replace dynamically generated errors with constants #711

Replace dynamically generated errors with constants #711

Conversation

iamnotacake commented Sep 1, 2020 • edited Loading

Checklist

Choose a reason for hiding this comment

iamnotacake Sep 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilammy Sep 1, 2020 • edited Loading

Choose a reason for hiding this comment

ilammy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilammy Sep 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilammy left a comment

Choose a reason for hiding this comment

iamnotacake commented Sep 4, 2020

Lagovas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamnotacake commented Sep 1, 2020 •

edited

Loading

iamnotacake Sep 1, 2020 •

edited

Loading

ilammy Sep 1, 2020 •

edited

Loading

ilammy Sep 1, 2020 •

edited

Loading