-
Notifications
You must be signed in to change notification settings - Fork 72
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
Added ability for deferred mutator execution #380
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
91bd826
Added ability for deferred mutator execution
andrewnester b1c0392
Do not use errors.Join
andrewnester cc40e56
Correctly format errors
andrewnester ed2683b
Fixed error wrapping
andrewnester b845736
fixed formatting
andrewnester 56514bc
Refactored code for aggregate error handling
andrewnester File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
package bundle | ||
|
||
import ( | ||
"context" | ||
|
||
"github.com/databricks/bricks/libs/errs" | ||
) | ||
|
||
type DeferredMutator struct { | ||
mutators []Mutator | ||
finally []Mutator | ||
} | ||
|
||
func (d *DeferredMutator) Name() string { | ||
return "deferred" | ||
} | ||
|
||
func Defer(mutators []Mutator, finally []Mutator) []Mutator { | ||
return []Mutator{ | ||
&DeferredMutator{ | ||
mutators: mutators, | ||
finally: finally, | ||
}, | ||
} | ||
} | ||
|
||
func (d *DeferredMutator) Apply(ctx context.Context, b *Bundle) ([]Mutator, error) { | ||
mainErr := Apply(ctx, b, d.mutators) | ||
errOnFinish := Apply(ctx, b, d.finally) | ||
if mainErr != nil || errOnFinish != nil { | ||
return nil, errs.FromMany(mainErr, errOnFinish) | ||
} | ||
|
||
return nil, nil | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,108 @@ | ||
package bundle | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
type mutatorWithError struct { | ||
applyCalled int | ||
errorMsg string | ||
} | ||
|
||
func (t *mutatorWithError) Name() string { | ||
return "mutatorWithError" | ||
} | ||
|
||
func (t *mutatorWithError) Apply(_ context.Context, b *Bundle) ([]Mutator, error) { | ||
t.applyCalled++ | ||
return nil, fmt.Errorf(t.errorMsg) | ||
} | ||
|
||
func TestDeferredMutatorWhenAllMutatorsSucceed(t *testing.T) { | ||
m1 := &testMutator{} | ||
m2 := &testMutator{} | ||
m3 := &testMutator{} | ||
cleanup := &testMutator{} | ||
deferredMutator := Defer([]Mutator{m1, m2, m3}, []Mutator{cleanup}) | ||
|
||
bundle := &Bundle{} | ||
err := Apply(context.Background(), bundle, deferredMutator) | ||
assert.NoError(t, err) | ||
|
||
assert.Equal(t, 1, m1.applyCalled) | ||
assert.Equal(t, 1, m2.applyCalled) | ||
assert.Equal(t, 1, m3.applyCalled) | ||
assert.Equal(t, 1, cleanup.applyCalled) | ||
} | ||
|
||
func TestDeferredMutatorWhenFirstFails(t *testing.T) { | ||
m1 := &testMutator{} | ||
m2 := &testMutator{} | ||
mErr := &mutatorWithError{errorMsg: "mutator error occurred"} | ||
cleanup := &testMutator{} | ||
deferredMutator := Defer([]Mutator{mErr, m1, m2}, []Mutator{cleanup}) | ||
|
||
bundle := &Bundle{} | ||
err := Apply(context.Background(), bundle, deferredMutator) | ||
assert.ErrorContains(t, err, "mutator error occurred") | ||
|
||
assert.Equal(t, 1, mErr.applyCalled) | ||
assert.Equal(t, 0, m1.applyCalled) | ||
assert.Equal(t, 0, m2.applyCalled) | ||
assert.Equal(t, 1, cleanup.applyCalled) | ||
} | ||
|
||
func TestDeferredMutatorWhenMiddleOneFails(t *testing.T) { | ||
m1 := &testMutator{} | ||
m2 := &testMutator{} | ||
mErr := &mutatorWithError{errorMsg: "mutator error occurred"} | ||
cleanup := &testMutator{} | ||
deferredMutator := Defer([]Mutator{m1, mErr, m2}, []Mutator{cleanup}) | ||
|
||
bundle := &Bundle{} | ||
err := Apply(context.Background(), bundle, deferredMutator) | ||
assert.ErrorContains(t, err, "mutator error occurred") | ||
|
||
assert.Equal(t, 1, m1.applyCalled) | ||
assert.Equal(t, 1, mErr.applyCalled) | ||
assert.Equal(t, 0, m2.applyCalled) | ||
assert.Equal(t, 1, cleanup.applyCalled) | ||
} | ||
|
||
func TestDeferredMutatorWhenLastOneFails(t *testing.T) { | ||
m1 := &testMutator{} | ||
m2 := &testMutator{} | ||
mErr := &mutatorWithError{errorMsg: "mutator error occurred"} | ||
cleanup := &testMutator{} | ||
deferredMutator := Defer([]Mutator{m1, m2, mErr}, []Mutator{cleanup}) | ||
|
||
bundle := &Bundle{} | ||
err := Apply(context.Background(), bundle, deferredMutator) | ||
assert.ErrorContains(t, err, "mutator error occurred") | ||
|
||
assert.Equal(t, 1, m1.applyCalled) | ||
assert.Equal(t, 1, m2.applyCalled) | ||
assert.Equal(t, 1, mErr.applyCalled) | ||
assert.Equal(t, 1, cleanup.applyCalled) | ||
} | ||
|
||
func TestDeferredMutatorCombinesErrorMessages(t *testing.T) { | ||
m1 := &testMutator{} | ||
m2 := &testMutator{} | ||
mErr := &mutatorWithError{errorMsg: "mutator error occurred"} | ||
cleanupErr := &mutatorWithError{errorMsg: "cleanup error occurred"} | ||
deferredMutator := Defer([]Mutator{m1, m2, mErr}, []Mutator{cleanupErr}) | ||
|
||
bundle := &Bundle{} | ||
err := Apply(context.Background(), bundle, deferredMutator) | ||
assert.ErrorContains(t, err, "mutator error occurred\ncleanup error occurred") | ||
|
||
assert.Equal(t, 1, m1.applyCalled) | ||
assert.Equal(t, 1, m2.applyCalled) | ||
assert.Equal(t, 1, mErr.applyCalled) | ||
assert.Equal(t, 1, cleanupErr.applyCalled) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
package errs | ||
|
||
import "errors" | ||
|
||
type aggregateError struct { | ||
errors []error | ||
} | ||
|
||
func FromMany(errors ...error) error { | ||
n := 0 | ||
for _, err := range errors { | ||
if err != nil { | ||
n++ | ||
} | ||
} | ||
if n == 0 { | ||
return nil | ||
} | ||
aggregateErr := &aggregateError{ | ||
errors: make([]error, 0, n), | ||
} | ||
for _, err := range errors { | ||
if err != nil { | ||
aggregateErr.errors = append(aggregateErr.errors, err) | ||
} | ||
} | ||
return aggregateErr | ||
} | ||
|
||
func (ce *aggregateError) Error() string { | ||
var b []byte | ||
for i, err := range ce.errors { | ||
if i > 0 { | ||
b = append(b, '\n') | ||
} | ||
b = append(b, err.Error()...) | ||
} | ||
return string(b) | ||
} | ||
|
||
func (ce *aggregateError) Unwrap() error { | ||
return errorChain(ce.errors) | ||
} | ||
|
||
// Represents chained list of errors. | ||
// Implements Error interface so that chain of errors | ||
// can correctly work with errors.Is/As method | ||
type errorChain []error | ||
|
||
func (ec errorChain) Error() string { | ||
return ec[0].Error() | ||
} | ||
|
||
func (ec errorChain) Unwrap() error { | ||
if len(ec) == 1 { | ||
return nil | ||
} | ||
|
||
return ec[1:] | ||
} | ||
|
||
func (ec errorChain) As(target interface{}) bool { | ||
return errors.As(ec[0], target) | ||
} | ||
|
||
func (ec errorChain) Is(target error) bool { | ||
return errors.Is(ec[0], target) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
package errs | ||
|
||
import ( | ||
"errors" | ||
"fmt" | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestFromManyErrors(t *testing.T) { | ||
e1 := fmt.Errorf("Error 1") | ||
e2 := fmt.Errorf("Error 2") | ||
e3 := fmt.Errorf("Error 3") | ||
err := FromMany(e1, e2, e3) | ||
|
||
assert.True(t, errors.Is(err, e1)) | ||
assert.True(t, errors.Is(err, e2)) | ||
assert.True(t, errors.Is(err, e3)) | ||
|
||
assert.Equal(t, err.Error(), `Error 1 | ||
Error 2 | ||
Error 3`) | ||
} | ||
|
||
func TestFromManyErrorsWihtNil(t *testing.T) { | ||
e1 := fmt.Errorf("Error 1") | ||
var e2 error = nil | ||
e3 := fmt.Errorf("Error 3") | ||
err := FromMany(e1, e2, e3) | ||
|
||
assert.True(t, errors.Is(err, e1)) | ||
assert.True(t, errors.Is(err, e3)) | ||
|
||
assert.Equal(t, err.Error(), `Error 1 | ||
Error 3`) | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Nice! This makes me think we also need (separate PR of course) a
bundle.Seq
to execute multiple mutators in sequence such that we can composelock.Acquire
with abundle.Defer
where the unlock happens. As it is here,lock.Release
would always be called even iflock.Acquire
fails.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.
@pietern just to clarify to make sure I understand correctly. You propose to have a new method
bundle.Seq
with API like thisbundle.Seq
will essentially transform list of mutators into Mutator object withApply
method so it can be added to the list of mutatorsIn such case
seq
will only be executed oncelock.Acquire
succeedsThere 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.
Yes to the proposed prototype, but the call site would be slightly different and call the
Seq
function to compose calllock.Acquire
and the defer block in sequence.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.
So something like this then?
Do we then really need
bundle.Seq
? What ifbundle.Defer
just returnsMutator
instead of[]Mutator
?In that case we would just have it used like
@pietern any thoughts?
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.
We don't if we leave everything else the same, so yeah you could leave as is for this PR.
The comment on
bundle.Seq
is because we have mixed usage of singularMutator
and[]Mutator
in a couple places and formalizing[]Mutator
into abundle.Seq
would get rid of that.