support wrapError encode and decode #113
Conversation
There was a problem hiding this comment.
Ok I have given this a deep review.
Overall the data structuring is sound. As I was reviewing, I started to feel that "ownsErrorString" doesn't exactly convey what the flag is doing. Maybe the word "override" needs to go in there.
Two main things remaining:
-
this is a public library with downstream users. You can't nilly-willy change the interface of functions. So the existing wrapper encoder API needs to remain, and you can only add the new stuff by adding new API names.
-
as we predicted during our initial convo, the printing stuff is quite hard to get right. I left many comments below -- the gist is that the code for formatting errors needs to work with any error type, not just those provided by this library. So for example if you pass a go wrapError instance to FormatError it should work. So you can't special-case
opaqueErrorthere.Ideally your PR should work with all the test cases here: Add more tests dhartunian/errors#2
I hope my comments will be enough for you to figure it out. If not, I have drafted what a better solution looks like here: 8d7aed9
Reviewed 29 of 29 files at r1, all commit messages.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @dhartunian)
contexttags/with_context.go line 78 at r1 (raw file):
} func encodeWithContext(_ context.Context, err error) (string, []string, proto.Message, bool) {
let's revert this one (see my other comments)
errbase/adapters.go line 79 at r1 (raw file):
func encodePkgWithStack( _ context.Context, err error, ) (msgPrefix string, safe []string, _ proto.Message, _ bool) {
let's revert this one (see my other comments)
errbase/adapters.go line 87 at r1 (raw file):
func encodePathError( _ context.Context, err error, ) (msgPrefix string, safe []string, details proto.Message, ownError bool) {
let's revert this one (see my other comments)
errbase/adapters.go line 116 at r1 (raw file):
func encodeLinkError( _ context.Context, err error, ) (msgPrefix string, safe []string, details proto.Message, ownError bool) {
let's revert this one (see my other comments)
errbase/adapters.go line 146 at r1 (raw file):
func encodeSyscallError( _ context.Context, err error, ) (msgPrefix string, safe []string, details proto.Message, ownError bool) {
let's revert this one (see my other comments)
errbase/adapters.go line 215 at r1 (raw file):
RegisterLeafEncoder(GetTypeKey(&OpaqueErrno{}), encodeOpaqueErrno) RegisterWrapperEncoder(GetTypeKey(fmt.Errorf("text %w text", baseErr)), encodeWrapError)
This can use the new API.
errbase/encode.go line 136 at r1 (raw file):
// In that case, we'll try to compute a message prefix // manually. msg = extractPrefix(err, cause)
You forgot to modify this -- the case where no known encoder function is known.
It's still possible to do something here: if extractPrefix doesn't see the cause as a suffix of the message, then it's an indication that ownError should be true.
errbase/encode.go line 311 at r1 (raw file):
// WrapperEncoder is to be provided (via RegisterWrapperEncoder above) // by additional wrapper types not yet known to this library. type WrapperEncoder = func(ctx context.Context, err error) (msgPrefix string, safeDetails []string, payload proto.Message, ownError bool)
This is a breaking API change. Existing code that uses the errors library would break if they import this.
We'll want to have 2 different encoder function types defined here, and provide two separate Register methods. One can be implemented using the other. Like this:
- type
WrapperEncoder- unchanged type definition - type
WrapperEncoderWithMessageOverride- the new one - new function
RegisterWrapperEncoderWithMessageOverride- new function - function
RegisterWrapperEncoder: calls the new one with a wrapper closure to adapt the behavior.
This way all the calls to RegisterWrapperEncoder can remain unchanged.
The package doc needs to be updated accordingly.
errbase/format_error.go line 150 at r1 (raw file):
// and then assemble the pieces ourselves. p.formatRecursive(err, true /* isOutermost */, false /* withDetail */) if ee, ok := err.(*opaqueWrapper); ok {
This function needs to work with any wrapper type that has a custom message, not just opaqueWrapper.
errbase/opaque.go line 110 at r1 (raw file):
} } return e.cause
This seems incorrect - returning non-nil here means the cause will be printed. I don't think we want that.
errbase/unknown_type_test.go line 157 at r1 (raw file):
} func TestUnknownWrapperTraversal(t *testing.T) {
Might be worth having a variant of this test that checks what happens when the message is overridden.
errbase/unknown_type_test.go line 165 at r1 (raw file):
// Register a temporary encoder. myEncode := func(_ context.Context, err error) (string, []string, proto.Message, bool) {
This can remain unchanged.
errutil/redactable.go line 88 at r1 (raw file):
} func encodeWithPrefix(_ context.Context, err error) (string, []string, proto.Message, bool) {
this does not need to change.
errutil/redactable.go line 141 at r1 (raw file):
} func encodeWithNewMessage(_ context.Context, err error) (string, []string, proto.Message, bool) {
this does not need to change.
extgrpc/ext_grpc.go line 109 at r1 (raw file):
// it's an encodable error. func encodeWithGrpcCode(_ context.Context, err error) (string, []string, proto.Message, bool) {
this does not need to change.
exthttp/ext_http.go line 79 at r1 (raw file):
// it's an encodable error. func encodeWithHTTPCode(_ context.Context, err error) (string, []string, proto.Message, bool) {
this does not need to change.
fmttests/format_error_test.go line 687 at r1 (raw file):
) (prefix string, _ []string, _ proto.Message, _ bool) { m := err.(*werrWithElidedCause) return m.msg, nil, nil, false
This one is a candidate for the new API and return true here.
hintdetail/with_detail.go line 50 at r1 (raw file):
} func encodeWithDetail(_ context.Context, err error) (string, []string, proto.Message, bool) {
this does not need to change.
hintdetail/with_hint.go line 50 at r1 (raw file):
} func encodeWithHint(_ context.Context, err error) (string, []string, proto.Message, bool) {
this does not need to change.
markers/markers.go line 283 at r1 (raw file):
} func encodeMark(_ context.Context, err error) (msg string, _ []string, payload proto.Message, ownError bool) {
this does not need to change.
secondary/with_secondary.go line 62 at r1 (raw file):
func (e *withSecondaryError) Unwrap() error { return e.cause } func encodeWithSecondaryError(ctx context.Context, err error) (string, []string, proto.Message, bool) {
this does not need to change.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @dhartunian)
errbase/adapters.go line 215 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
This can use the new API.
Actually, if you implement my other suggestions, you will discover you do not need this encoder at all. It will "just work".
There is a (separate, and optional) benefit to providing a custom decoder (see what is done at the top of this init function, and notice how the code only needs to register the decoder and not the encoder). However I suspect you will not be able to do so without some additional head-scratching (to reverse-engineer the necessary %w formatting string from the complete wrapped error message). I suggest to file an issue for that and only do it later.
dhartunian
force-pushed
the
fix-wrapError-encode-decode
branch
from
ac27789 to
bd8a6b6
Compare
There was a problem hiding this comment.
Thank you for all the detail. I arrived at your solution as well which feels satisfying. Added all the new datadriven tests.
Reviewable status: 9 of 38 files reviewed, 21 unresolved discussions (waiting on @knz)
errbase/adapters.go line 79 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
let's revert this one (see my other comments)
Done.
errbase/adapters.go line 87 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
let's revert this one (see my other comments)
Done.
errbase/adapters.go line 116 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
let's revert this one (see my other comments)
Done.
errbase/adapters.go line 146 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
let's revert this one (see my other comments)
Done.
errbase/adapters.go line 215 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
Actually, if you implement my other suggestions, you will discover you do not need this encoder at all. It will "just work".
There is a (separate, and optional) benefit to providing a custom decoder (see what is done at the top of this
initfunction, and notice how the code only needs to register the decoder and not the encoder). However I suspect you will not be able to do so without some additional head-scratching (to reverse-engineer the necessary%wformatting string from the complete wrapped error message). I suggest to file an issue for that and only do it later.
Correct. This works as you described.
errbase/encode.go line 136 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
You forgot to modify this -- the case where no known encoder function is known.
It's still possible to do something here: if
extractPrefixdoesn't see the cause as a suffix of the message, then it's an indication thatownErrorshould be true.
Done.
errbase/encode.go line 311 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
This is a breaking API change. Existing code that uses the errors library would break if they import this.
We'll want to have 2 different encoder function types defined here, and provide two separate Register methods. One can be implemented using the other. Like this:
- type
WrapperEncoder- unchanged type definition- type
WrapperEncoderWithMessageOverride- the new one- new function
RegisterWrapperEncoderWithMessageOverride- new function- function
RegisterWrapperEncoder: calls the new one with a wrapper closure to adapt the behavior.This way all the calls to
RegisterWrapperEncodercan remain unchanged.The package doc needs to be updated accordingly.
Done. Thx for the advice.
Should I perhaps make it a bit more generic and use an "Options" type as the new argument in case we end up adding yet another param?
errbase/format_error.go line 150 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
This function needs to work with any wrapper type that has a custom message, not just
opaqueWrapper.
This is removed. formatRecursive now takes care of this via changes to formatSimple and extractPrefix which tell it to set elideShort in cases where a prefix cannot be extracted. This is now compatible with any wrapper.
errbase/opaque.go line 110 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
This seems incorrect - returning non-nil here means the cause will be printed. I don't think we want that.
Correct. Added special case above in this method. Also added errorf case to the datadriven tests which exercise that branch.
fmttests/format_error_test.go line 687 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
This one is a candidate for the new API and return
truehere.
Done.
contexttags/with_context.go line 78 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
let's revert this one (see my other comments)
Done.
errbase/unknown_type_test.go line 157 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
Might be worth having a variant of this test that checks what happens when the message is overridden.
Added a test to opaque.go which simulates what happens when the wrapper's new field is dropped.
errbase/unknown_type_test.go line 165 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
This can remain unchanged.
Done.
errutil/redactable.go line 88 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
this does not need to change.
Done.
errutil/redactable.go line 141 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
this does not need to change.
Done.
extgrpc/ext_grpc.go line 109 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
this does not need to change.
Done.
exthttp/ext_http.go line 79 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
this does not need to change.
Done.
hintdetail/with_detail.go line 50 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
this does not need to change.
Done.
hintdetail/with_hint.go line 50 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
this does not need to change.
Done.
markers/markers.go line 283 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
this does not need to change.
Done.
secondary/with_secondary.go line 62 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
this does not need to change.
Done.
There was a problem hiding this comment.
This all checks out. Just a few details remaining.
Reviewed 29 of 29 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @dhartunian)
errbase_api.go line 150 at r2 (raw file):
// WrapperEncoder is to be provided (via RegisterWrapperEncoder above) // by additional wrapper types not yet known to this library. type WrapperEncoder = errbase.WrapperEncoder
You might want to alias the new type here as well.
errbase/encode.go line 121 at r2 (raw file):
if e, ok := err.(*opaqueWrapper); ok { msg, ownError = extractPrefix(err, cause)
I think for opaqueWrapper you can use the value of the field already stored inside opaqueWrapper.
errbase/encode.go line 184 at r2 (raw file):
} // If error msg matches exactly then this is a wrapper // with no message of its own
nit: period at end of comment.
Also maybe move this condition just above, to avoid the overhead of strings.HasSuffix in that case.
errbase/format_error.go line 393 at r2 (raw file):
} } else { elideChildren := s.formatSimple(err, cause)
nit: it's not "children" you are eliding here. (The term "children" doesn't exist elsewhere in the library.)
Did you mean "elideCauseMsg" or something similar?
errbase/format_error.go line 395 at r2 (raw file):
elideChildren := s.formatSimple(err, cause) if elideChildren { // The error wants to elide the short messages from inner
nit: extract that for loop into a helper function and call it here.
Also, if elideChildren := ...; elideChildren { is more idiomatic.
Ditto below
errbase/format_error.go line 518 at r2 (raw file):
// given level of wrapping when the error object does not implement // the Formatter interface. // Returns true if we want to elide errors from child entries.
ditto: "child entries" is not a thing. Maybe shift semantics to "causal chain" or "wrapped error".
errbase/format_simple.go line 1 at r2 (raw file):
// Copyright 2019 The Cockroach Authors.
You should double check - I believe this file can be removed altogether.
fmttests/datadriven_test.go line 499 at r2 (raw file):
if *generateTestFiles { generateFiles() return
Not 100% sure this return is desirable. Your call.
|
Also maybe mention the new api in the README. |
dhartunian
force-pushed
the
fix-wrapError-encode-decode
branch
from
bd8a6b6 to
69e17d2
Compare
There was a problem hiding this comment.
README updated as well.
Reviewable status: 33 of 39 files reviewed, 5 unresolved discussions (waiting on @knz)
errbase_api.go line 150 at r2 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
You might want to alias the new type here as well.
Done.
errbase/encode.go line 121 at r2 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
I think for
opaqueWrapperyou can use the value of the field already stored inside opaqueWrapper.
I changed this to take either the encoded one or the heuristic if true, since we might want to still rely on heuristic if we get an opaqueWrapper from an old version with the value unset.
errbase/encode.go line 184 at r2 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
nit: period at end of comment.
Also maybe move this condition just above, to avoid the overhead of strings.HasSuffix in that case.
Done.
errbase/format_error.go line 393 at r2 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
nit: it's not "children" you are eliding here. (The term "children" doesn't exist elsewhere in the library.)
Did you mean "elideCauseMsg" or something similar?
done.
errbase/format_error.go line 395 at r2 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
nit: extract that for loop into a helper function and call it here.
Also,if elideChildren := ...; elideChildren {is more idiomatic.Ditto below
Done.
errbase/format_error.go line 518 at r2 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
ditto: "child entries" is not a thing. Maybe shift semantics to "causal chain" or "wrapped error".
Done.
errbase/format_simple.go line 1 at r2 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
You should double check - I believe this file can be removed altogether.
hah! gone!
fmttests/datadriven_test.go line 499 at r2 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
Not 100% sure this
returnis desirable. Your call.
Removed. I think I didn't realize I could do -generate and -rewrite together and was annoyed that every -generate run just generated a ton of failures.
There was a problem hiding this comment.
Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dhartunian)
errbase/encode.go line 121 at r2 (raw file):
Previously, dhartunian (David Hartunian) wrote…
I changed this to take either the encoded one or the heuristic if true, since we might want to still rely on heuristic if we get an
opaqueWrapperfrom an old version with the value unset.
If we are receiving from an old version, could we detect this during the decode logic, instead of here? Then we'd store the inferred value in the .ownsErrorString bool and we could reuse the common logic.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dhartunian)
errbase/encode.go line 121 at r2 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
If we are receiving from an old version, could we detect this during the
decodelogic, instead of here? Then we'd store the inferred value in the .ownsErrorString bool and we could reuse the common logic.
Thinking about this some more: is there a way we could detect when we are receiving a payload from a previous-version, even when this boolean is false?
Here's an idea: store another boolean that says "true when this version of the errors lib supports this feature'.
dhartunian
force-pushed
the
fix-wrapError-encode-decode
branch
from
69e17d2 to
0543d30
Compare
There was a problem hiding this comment.
Reviewable status: 27 of 40 files reviewed, 2 unresolved discussions (waiting on @knz)
errbase/encode.go line 121 at r2 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
Thinking about this some more: is there a way we could detect when we are receiving a payload from a previous-version, even when this boolean is false?
Here's an idea: store another boolean that says "true when this version of the errors lib supports this feature'.
That idea worked well. Take a look at the latest diff. I no longer try to "infer" the ownership during encode if we're already dealing with an opaqueWrapper.
There was a problem hiding this comment.
Reviewed 13 of 13 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dhartunian)
errbase/format_simple.go line 1 at r2 (raw file):
Previously, dhartunian (David Hartunian) wrote…
hah! gone!
LGTM with a nit
errbase/opaque.go line 46 at r4 (raw file):
prefix string details errorspb.EncodedErrorDetails encoderKnowsAboutErrorStringOwnership bool
I did a double-take here, a naive reader could think "since the version that introduced ownsErrorString knows how to encode it, why do we need to carry the boolean inside the opaque wrapper?"
(No discussion it's needed in the proto - I'm only confused here)
I am ready to believe it is necessary, but I don't fully understand why. Maybe a few words of commentary would help.
dhartunian
force-pushed
the
fix-wrapError-encode-decode
branch
from
0543d30 to
9b99420
Compare
There was a problem hiding this comment.
Reviewable status: 39 of 40 files reviewed, 1 unresolved discussion (waiting on @knz)
errbase/opaque.go line 46 at r4 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
I did a double-take here, a naive reader could think "since the version that introduced
ownsErrorStringknows how to encode it, why do we need to carry the boolean inside the opaque wrapper?"(No discussion it's needed in the proto - I'm only confused here)
I am ready to believe it is necessary, but I don't fully understand why. Maybe a few words of commentary would help.
Done.
There was a problem hiding this comment.
Reviewable status: 39 of 40 files reviewed, 1 unresolved discussion (waiting on @knz)
errbase/format_simple.go line 1 at r2 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
LGTM with a nit
One more look at this prior to merge prompted me to revisit the extra flag decision. I think it's unnecessary. If you look at the usage of the encoderKnowsAboutErrorStringOwnership flag in r5 revision you'll see that it's never actually used to make any meaningful decisions in the code. I think the act of writing in the flag, caused me to fix the code in encode for opaqueWrapper which now works without the extra flag.
I think without encoderKnowsAboutErrorStringOwnership we end up with the same scenario because we preserve the value of ownsErrorString via opaqueWrapper.
- Old version: always omits
ownsErrorStringwhich results in it being set tofalseon new version decoders - New version: always includes
ownsErrorStringwhich may be set to true
If an old version encodes an error, it will use prior logic to set the prefix value on the wrapper, resulting in a wrapper that never owns the error string and always concatenates the causal chain to its prefix when generating the error message. This encoding will be preserved across encode/decode operations between old and new version because the flag is false and is preserved as-is in opaqueWrapper.
If a new version encodes an error, as long as the error never crosses into an older version's decode/encode path it will have ownsErrorString preserved as expected. If it does cross into an older version, the flag will be ignored and we'll end up with an error printed that repeats some portions of the error string due to the prefix containing the causal chain but not being marked as doing so. This is okay because it doesn't lose information, just causes a bit of redundancy and confusion.
The inclusion of an extra flag does not allow us to be more clever in either of the above scenarios. New errors passing through old versions will always have the flags stripped. Old errors passing through new versions can't intelligently add flag values because they cannot reconstruct the inputs to the computation of ownsErrorString.
dhartunian
force-pushed
the
fix-wrapError-encode-decode
branch
from
9b99420 to
549e460
Compare
dhartunian
force-pushed
the
fix-wrapError-encode-decode
branch
2 times, most recently
from
e3c1d21 to
6267f9e
Compare
There was a problem hiding this comment.
Reviewed 13 of 13 files at r6, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @dhartunian)
errbase/format_simple.go line 1 at r2 (raw file):
If it does cross into an older version, the flag will be ignored and we'll end up with an error printed that repeats some portions of the error string [...] This is okay because it doesn't lose information
Gotcha. And this, in turn, is true because the error mark is encoded separately in the details payload.
You got a good simplification here, but i'm pretty sure we will want an end-to-end cross-version check before we publish an actual release of the library with the new code.
dhartunian
force-pushed
the
fix-wrapError-encode-decode
branch
from
6267f9e to
125952d
Compare
There was a problem hiding this comment.
I am definitely missing a lot of details, but why can't we remember the format string (with %ws in it) and fill in the %ws when we format the wrapper?
Reviewable status: 39 of 40 files reviewed, 9 unresolved discussions (waiting on @dhartunian and @knz)
errbase/adapters.go line 216 at r6 (raw file):
} func encodeWrapError(_ context.Context, err error) (
Where is this used?
errbase/adapters_test.go line 60 at r6 (raw file):
func TestAdaptGoSingleWrapErr(t *testing.T) { origErr := fmt.Errorf("an error %w", goErr.New("hello")) t.Logf("start err: %# v", pretty.Formatter(origErr))
[nit] either remove this, or add a second Logf with the "after" err
errbase/encode.go line 118 at r6 (raw file):
var msg string var details errorspb.EncodedErrorDetails var ownError bool
We have three names for the same thing. overrideError int he encoder type, ownsErrorString in opaqueWrapper and ownError here. They should all have the same name (ownsErrorString)
errbase/encode.go line 322 at r6 (raw file):
// prior to RegisterWrapperEncoder(). func RegisterWrapperEncoder(theType TypeKey, encoder WrapperEncoder) { RegisterWrapperEncoderWithMessageOverride(theType, func(ctx context.Context, err error) (msgPrefix string, safeDetails []string, payload proto.Message, overrideError bool) {
[nit] very long line
errbase/encode.go line 353 at r6 (raw file):
type WrapperEncoder = func(ctx context.Context, err error) (msgPrefix string, safeDetails []string, payload proto.Message) // WrapperEncoderWithMessageOverride is to be provided (via RegisterWrapperEncoderWithMessageOverride above)
[nit] wrap lines consistently
errbase/encode.go line 355 at r6 (raw file):
// WrapperEncoderWithMessageOverride is to be provided (via RegisterWrapperEncoderWithMessageOverride above) // by additional wrapper types not yet known to this library. // This encoder accepts an additional boolean flag which determines whether the
This comment seems incorrect. Also we need to explain the overrideError return (is it named correctly?)
errbase/encode.go line 358 at r6 (raw file):
// wrapper owns the error message completely instead of simply being a prefix // with the error message of its causes appended to it. type WrapperEncoderWithMessageOverride = func(ctx context.Context, err error) (msgPrefix string, safeDetails []string, payload proto.Message, overrideError bool)
[nit] can remove the =
errbase/err_string_ownership_test.go line 1 at r6 (raw file):
package errbase_test
[nit] missing a file comment header
errorspb/errors.proto line 87 at r6 (raw file):
// compatibility since the new behavior is only enabled when // this flag is set to `true`. bool wrapper_owns_error_string = 4;
Would things be cleaner if this was instead a string full_message (and when it is set, message_prefix is empty?). It's not ideal to have a field named MessagePrefix which is only sometimes what it says it is.
By the time our errors library looks at an error object, that format string is long gone (it should work with error types not defined through it). |
dhartunian
force-pushed
the
fix-wrapError-encode-decode
branch
from
125952d to
8e06c77
Compare
There was a problem hiding this comment.
Reviewable status: 32 of 40 files reviewed, 4 unresolved discussions (waiting on @knz and @RaduBerinde)
errbase/adapters_test.go line 60 at r6 (raw file):
Previously, RaduBerinde wrote…
[nit] either remove this, or add a second Logf with the "after" err
The structure is a bit odd here, but t.LogF line shows you the error prior to encoding, and the implementation of network prints the encoded, and then decoded error prior to returning. Test output will contain all 3.
errbase/encode.go line 118 at r6 (raw file):
Previously, RaduBerinde wrote…
We have three names for the same thing.
overrideErrorint he encoder type,ownsErrorStringinopaqueWrapperandownErrorhere. They should all have the same name (ownsErrorString)
Done.
errbase/encode.go line 322 at r6 (raw file):
Previously, RaduBerinde wrote…
[nit] very long line
Done.
errbase/encode.go line 355 at r6 (raw file):
Previously, RaduBerinde wrote…
This comment seems incorrect. Also we need to explain the
overrideErrorreturn (is it named correctly?)
Fixed the name and wording that the encoder returns a flag value, not accepts.
errbase/encode.go line 358 at r6 (raw file):
Previously, RaduBerinde wrote…
[nit] can remove the =
Done.
errbase/err_string_ownership_test.go line 1 at r6 (raw file):
Previously, RaduBerinde wrote…
[nit] missing a file comment header
Done. also added to opaque_test.go
errorspb/errors.proto line 87 at r6 (raw file):
Previously, RaduBerinde wrote…
Would things be cleaner if this was instead a
string full_message(and when it is set,message_prefixis empty?). It's not ideal to have a field namedMessagePrefixwhich is only sometimes what it says it is.
In order to keep this encoding backwards compatible, we need to have something in message_prefix so that a prior version can decode the error into something coherent that can be printed out.
I could write to both and only read the new one when present and non-nil. Maybe there should just be a refactor of the naming of the fields which wouldn't break things. Perhaps once this new version is out we can clean that up while keeping the API identical.
errbase/adapters.go line 216 at r6 (raw file):
Previously, RaduBerinde wrote…
Where is this used?
Thanks. Removed.
There was a problem hiding this comment.
Reviewable status: 32 of 40 files reviewed, 4 unresolved discussions (waiting on @dhartunian and @knz)
errbase/encode.go line 369 at r8 (raw file):
// RegisterWrapperEncoderWithMessageOverride above) by additional // wrapper types not yet known to this library. This encoder returns an // additional boolean flag which determines whether the wrapper owns
[nit] s/determines/indicates ?
errorspb/errors.proto line 87 at r6 (raw file):
Previously, dhartunian (David Hartunian) wrote…
In order to keep this encoding backwards compatible, we need to have something in
message_prefixso that a prior version can decode the error into something coherent that can be printed out.I could write to both and only read the new one when present and non-nil. Maybe there should just be a refactor of the naming of the fields which wouldn't break things. Perhaps once this new version is out we can clean that up while keeping the API identical.
Ah, makes sense. I think this is ok, but we should add to the comment for message_prefix to explain what it means if owns_error_string is set.
Maybe consider renaming it to something more generic like just message and add some accessors MessagePrefix() and FullMessage() which assert that the flag is set accordingly.
Also owns_error_string could be a "message type" enum with "suffix" being the 0 value and "full message" being a new value. It might make code more explicit compared to just a bool.
errorspb/errors.proto line 70 at r8 (raw file):
// The wrapper message prefix (which may be empty). This // isbprinted before the cause's own message when
[nit] is printed
dhartunian
force-pushed
the
fix-wrapError-encode-decode
branch
from
8e06c77 to
774f530
Compare
There was a problem hiding this comment.
Reviewable status: 21 of 40 files reviewed, 2 unresolved discussions (waiting on @knz and @RaduBerinde)
errbase/encode.go line 369 at r8 (raw file):
Previously, RaduBerinde wrote…
[nit] s/determines/indicates ?
done.
errorspb/errors.proto line 87 at r6 (raw file):
Previously, RaduBerinde wrote…
Ah, makes sense. I think this is ok, but we should add to the comment for
message_prefixto explain what it means ifowns_error_stringis set.Maybe consider renaming it to something more generic like just
messageand add some accessorsMessagePrefix()andFullMessage()which assert that the flag is set accordingly.Also
owns_error_stringcould be a "message type" enum with "suffix" being the 0 value and "full message" being a new value. It might make code more explicit compared to just a bool.
I had this thought re: enum when I first got started as well. Done.
I added a second non-proto enum in encode.go because it seemed a bit messy to let the public API reference a proto type. This keeps it a bit more neat with minimal conversion necessary.
Current usages did not push me to add accessors.
errorspb/errors.proto line 70 at r8 (raw file):
Previously, RaduBerinde wrote…
[nit] is printed
done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r7, 1 of 7 files at r8, 17 of 17 files at r9, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dhartunian and @RaduBerinde)
errbase/encode.go line 366 at r9 (raw file):
) // MessageType is used to encode information about
nit: word missing.
dhartunian
force-pushed
the
fix-wrapError-encode-decode
branch
from
774f530 to
6fc7021
Compare
There was a problem hiding this comment.
Reviewable status: 39 of 40 files reviewed, 2 unresolved discussions (waiting on @knz and @RaduBerinde)
errbase/encode.go line 366 at r9 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
nit: word missing.
done.
There was a problem hiding this comment.
Reviewable status: 39 of 40 files reviewed, 4 unresolved discussions (waiting on @dhartunian and @knz)
errbase/encode.go line 369 at r10 (raw file):
// within a wrapper error type. This information is used to affect // display logic. type MessageType int32
why not alias it to errorspb.MessageType instead of a new type?
errbase/encode.go line 376 at r10 (raw file):
// Prefix denotes an error message that should be prepended to the // message of its cause. Prefix MessageType = iota
we should do = MessageType(errorspb.MessageType_PREFIX) etc instead of relying on the ordering.
errorspb/errors.proto line 87 at r6 (raw file):
Previously, dhartunian (David Hartunian) wrote…
I had this thought re: enum when I first got started as well. Done.
I added a second non-proto enum in
encode.gobecause it seemed a bit messy to let the public API reference a proto type. This keeps it a bit more neat with minimal conversion necessary.Current usages did not push me to add accessors.
Very nice, thanks! It's so much better to see errbase.MessageType throughout the code instead of a bool that you need to read comments to understand.
errorspb/errors.proto line 82 at r10 (raw file):
// The wrapper message. This could either be a full error message // that can be printed independently, or a prefix (which may be // empty). This is printed before the cause's own message when
[nit] "or a (potentially empty) prefix which is printed before the cause's own message to construct the full message"
Previously, support for the `wrapError` type generated by
`fmt.Errorf("%w xxxx")` was missing an understanding of the fact
that the format string wasn't limited to just prefixes. Hence the
`opaqueWrapper` struct assumed that all wrappers had prefix strings
that would precede their causal chains.
This change adds an enum to the `EncodedWrapper` which tracks whether
the wrapper's error string is a prefix, or the full error. In the
case of `fmt.Errorf` the `wrapError` struct created within the
stdlib contains a fully interpolated string as its message, not the
format string, or a prefix. This means that when we encode it into a
`EncodedWrapper` for transfer over the network, we need to remember
to just print the wrapper's string when `.Error()` is called on the
decoded object on the other end.
Paired with this change the `opaqueWrapper` that is generated on the
decode side now contains this field as well which is referenced when
calling `.Error()` on the wrapper, ensuring we don't re-concatenate
the causes. This field is also referenced when formatting the error
in `state.formatSingleLineOutput` where we use it to simply output
the final entry's message, instead of concatenating all the entries
together.
This change is backwards compatible since older versions will omit the
new field in the proto, defaulting it to `PREFIX` in newer versions,
which will preserve existing behavior when printing out errors. New
code will set the field when necessary and interpret it appropriately.
If an error encoded with a new version is decoded by an older version,
the information about the error string will be discarded, which may
lead to repeated portions of the error message when printed out, if
the wrapper contains a copy of the message of its cause.
dhartunian
force-pushed
the
fix-wrapError-encode-decode
branch
from
6fc7021 to
19be170
Compare
There was a problem hiding this comment.
Reviewable status: 36 of 40 files reviewed, 1 unresolved discussion (waiting on @knz and @RaduBerinde)
errbase/encode.go line 369 at r10 (raw file):
Previously, RaduBerinde wrote…
why not alias it to
errorspb.MessageTypeinstead of a new type?
Done. Also fixed the types in the public API (errbase_api.go) which I had overlooked previously.
errbase/encode.go line 376 at r10 (raw file):
Previously, RaduBerinde wrote…
we should do
= MessageType(errorspb.MessageType_PREFIX)etc instead of relying on the ordering.
Done.
errorspb/errors.proto line 87 at r6 (raw file):
Previously, RaduBerinde wrote…
Very nice, thanks! It's so much better to see
errbase.MessageTypethroughout the code instead of a bool that you need to read comments to understand.
Agreed. Thx for the nudge.
errorspb/errors.proto line 82 at r10 (raw file):
Previously, RaduBerinde wrote…
[nit] "or a (potentially empty) prefix which is printed before the cause's own message to construct the full message"
Done.
Previously, support for the
wrapErrortype generated byfmt.Errorf("%w xxxx")was missing an understanding of the factthat the format string wasn't limited to just prefixes. Hence the
opaqueWrapperstruct assumed that all wrappers had prefix stringsthat would precede their causal chains.
This change adds a flag to the
EncodedWrapperwhich tracks whetherthe wrapper "owns" the error string. In the case of
fmt.Errorfthe
wrapErrorstruct created within the stdlib contains a fullyinterpolated string as its message, not the format string, or a
prefix. This means that when we encode it into a
EncodedWrapperfor transfer over the network, we need to remember to just print the
wrapper's string when
.Error()is called on the decoded object onthe other end.
Paired with this change the
opaqueWrapperthat is generated on thedecode side now contains this field as well which is referenced when
calling
.Error()on the wrapper, ensuring we don't re-concatenatethe causes. This field is also referenced when formatting the error
in
state.formatSingleLineOutputwhere we use it to simply outputthe final entry's message, instead of concatenating all the entries
together.
This change is backwards compatible since older versions will omit the
new field in the proto, defaulting it to
falsein newer versions,which will preserve existing behavior when printing out errors. New
code will set the field when necessary and interpret it appropriately.
If an error encoded with a new version is decoded by an older version,
the information about the error string will be discarded, which may
lead to repeated portions of the error message when printed out, if
the wrapper contains a copy of the message of its cause.
This change is