-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
schemadiff
: multi-error in schema normalization
#12675
Changes from 44 commits
c4c213b
3dac907
e88d426
61de6b0
389872c
56c8421
11d6379
f4fd673
a4ae98d
ef16bdf
54fb73a
2d3e70b
b69ede6
bc34369
bc8799d
dd1bc82
7f6fd13
4939d01
89d6cba
2194291
38eab14
7eb83c1
f98729b
36075ec
26e9577
5718b58
fe90b31
db43534
3bf836f
95a963e
ebf8a12
1244c0b
8b07d21
017e10d
eac2860
4daa08b
1699955
c66b6e9
3004533
1586e03
05e2364
1098f65
8a03a3f
2bb6e71
8c49bd2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,45 @@ import ( | |
"vitess.io/vitess/go/sqlescape" | ||
) | ||
|
||
// Wrapped is used to unwrap an error created by errors.Join() in Go 1.20 | ||
type Wrapped interface { | ||
Unwrap() []error | ||
} | ||
|
||
// Unwrap unwraps an error created by errors.Join() in Go 1.20, into its components | ||
func Unwrap(err error) []error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method will not be called right. u.Unwrap on line 27 is called on object level. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now no one is using this function... I added it for completeness. |
||
if err == nil { | ||
return nil | ||
} | ||
if u, ok := err.(Wrapped); ok { | ||
return u.Unwrap() | ||
} | ||
return nil | ||
} | ||
|
||
// Unwrap unwraps an error created by errors.Join() in Go 1.20, into its components, recursively | ||
func UnwrapAll(err error) (errs []error) { | ||
if err == nil { | ||
return nil | ||
} | ||
if u, ok := err.(Wrapped); ok { | ||
for _, e := range u.Unwrap() { | ||
errs = append(errs, UnwrapAll(e)...) | ||
} | ||
return errs | ||
} | ||
return []error{err} | ||
} | ||
|
||
// Unwrap unwraps an error created by errors.Join() in Go 1.20, into its components, recursively, | ||
// and returns one (the first) unwrapped error | ||
func UnwrapFirst(err error) error { | ||
if err == nil { | ||
return nil | ||
} | ||
return UnwrapAll(err)[0] | ||
} | ||
|
||
var ( | ||
ErrEntityTypeMismatch = errors.New("mismatched entity type") | ||
ErrStrictIndexOrderingUnsupported = errors.New("strict index ordering is unsupported") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
/* | ||
Copyright 2022 The Vitess Authors. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package schemadiff | ||
|
||
import ( | ||
"errors" | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestUnwrap(t *testing.T) { | ||
err1 := errors.New("err1") | ||
err2 := errors.New("err2") | ||
err3 := errors.New("err3") | ||
err4 := errors.New("err4") | ||
|
||
tt := []struct { | ||
name string | ||
err error | ||
expectUnwrap []error | ||
expectUnwrapAll []error | ||
expectUnwrapFirst error | ||
}{ | ||
{ | ||
name: "nil", | ||
expectUnwrap: nil, | ||
expectUnwrapAll: nil, | ||
expectUnwrapFirst: nil, | ||
}, | ||
{ | ||
name: "single", | ||
err: err1, | ||
expectUnwrap: nil, | ||
expectUnwrapAll: []error{err1}, | ||
expectUnwrapFirst: err1, | ||
}, | ||
{ | ||
name: "wrapped nil", | ||
err: errors.Join(nil), | ||
expectUnwrap: nil, | ||
expectUnwrapAll: nil, | ||
expectUnwrapFirst: nil, | ||
}, | ||
{ | ||
name: "single wrapped", | ||
err: errors.Join(err1), | ||
expectUnwrap: []error{err1}, | ||
expectUnwrapAll: []error{err1}, | ||
expectUnwrapFirst: err1, | ||
}, | ||
{ | ||
name: "flat wrapped", | ||
err: errors.Join(err1, err2, err3, err4), | ||
expectUnwrap: []error{err1, err2, err3, err4}, | ||
expectUnwrapAll: []error{err1, err2, err3, err4}, | ||
expectUnwrapFirst: err1, | ||
}, | ||
{ | ||
name: "double wrapped", | ||
err: errors.Join(errors.Join(err1)), | ||
expectUnwrap: []error{errors.Join(err1)}, | ||
expectUnwrapAll: []error{err1}, | ||
expectUnwrapFirst: err1, | ||
}, | ||
{ | ||
name: "double nested wrapped", | ||
err: errors.Join(errors.Join(err1, err2), errors.Join(err3, err4)), | ||
expectUnwrap: []error{errors.Join(err1, err2), errors.Join(err3, err4)}, | ||
expectUnwrapAll: []error{err1, err2, err3, err4}, | ||
expectUnwrapFirst: err1, | ||
}, | ||
} | ||
for _, tc := range tt { | ||
t.Run(tc.name, func(t *testing.T) { | ||
unwrapped := Unwrap(tc.err) | ||
unwrappedAll := UnwrapAll(tc.err) | ||
unwrappedFirst := UnwrapFirst(tc.err) | ||
|
||
assert.Equal(t, tc.expectUnwrap, unwrapped) | ||
assert.Equal(t, tc.expectUnwrapAll, unwrappedAll) | ||
assert.Equal(t, tc.expectUnwrapFirst, unwrappedFirst) | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -152,7 +152,8 @@ func TestNewSchemaFromQueriesLoop(t *testing.T) { | |
"create view v8 as select * from t1, v7", | ||
) | ||
_, err := NewSchemaFromQueries(queries) | ||
assert.Error(t, err) | ||
require.Error(t, err) | ||
err = UnwrapFirst(err) | ||
assert.EqualError(t, err, (&ViewDependencyUnresolvedError{View: "v7"}).Error()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we assert err length here as well .. to assure the numbers of errors are as expected There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And yes! I'll add that assertion anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather, I'll create a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unit tests added |
||
} | ||
|
||
|
@@ -700,6 +701,8 @@ func TestViewReferences(t *testing.T) { | |
require.NoError(t, err) | ||
require.NotNil(t, schema) | ||
} else { | ||
require.Error(t, err) | ||
err = UnwrapFirst(err) | ||
require.Equal(t, ts.expectErr, err, "received error: %v", err) | ||
} | ||
}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is more a structural comment; i'd suggest either renaming or relocating these types. right now you import
schemadiff.Wrapped
which, somehow, is for errors? i would expect from that name alone it's some sort of, well, wrapped diff structure (ditto theUnwrap*
functions).so, i'd either advocate for
schemadiff.WrappedError
orvterrors.Wrapped
(or a new package!)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. The location is suboptimal. I initially wanted to do this in
vterrors
, butvterrors
has its ownUnwrap
. I'll create a newerrors
package.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, refactored!