-
Notifications
You must be signed in to change notification settings - Fork 548
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
Fix issue 377 #475
Fix issue 377 #475
Changes from all commits
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 |
---|---|---|
|
@@ -99,20 +99,36 @@ func WithEnableSIGTERM(callbacks ...func()) Option { | |
}) | ||
} | ||
|
||
func validateArguments(handler reflect.Type) (bool, error) { | ||
handlerTakesContext := false | ||
if handler.NumIn() > 2 { | ||
return false, fmt.Errorf("handlers may not take more than two arguments, but handler takes %d", handler.NumIn()) | ||
} else if handler.NumIn() > 0 { | ||
// handlerTakesContext returns whether the handler takes a context.Context as its first argument. | ||
func handlerTakesContext(handler reflect.Type) (bool, error) { | ||
switch handler.NumIn() { | ||
case 0: | ||
return false, nil | ||
case 1: | ||
contextType := reflect.TypeOf((*context.Context)(nil)).Elem() | ||
argumentType := handler.In(0) | ||
handlerTakesContext = argumentType.Implements(contextType) | ||
if handler.NumIn() > 1 && !handlerTakesContext { | ||
if argumentType.Kind() != reflect.Interface { | ||
return false, nil | ||
} | ||
|
||
// handlers like func(event any) are valid. | ||
if argumentType.NumMethod() == 0 { | ||
return false, nil | ||
} | ||
|
||
if !contextType.Implements(argumentType) || !argumentType.Implements(contextType) { | ||
return false, fmt.Errorf("handler takes an interface, but it is not context.Context: %q", argumentType.Name()) | ||
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.
|
||
} | ||
return true, nil | ||
case 2: | ||
contextType := reflect.TypeOf((*context.Context)(nil)).Elem() | ||
argumentType := handler.In(0) | ||
if argumentType.Kind() != reflect.Interface || !contextType.Implements(argumentType) || !argumentType.Implements(contextType) { | ||
return false, fmt.Errorf("handler takes two arguments, but the first is not Context. got %s", argumentType.Kind()) | ||
} | ||
return true, nil | ||
} | ||
|
||
return handlerTakesContext, nil | ||
return false, fmt.Errorf("handlers may not take more than two arguments, but handler takes %d", handler.NumIn()) | ||
} | ||
|
||
func validateReturns(handler reflect.Type) error { | ||
|
@@ -198,7 +214,7 @@ func reflectHandler(handlerFunc interface{}, h *handlerOptions) Handler { | |
return errorHandler(fmt.Errorf("handler kind %s is not %s", handlerType.Kind(), reflect.Func)) | ||
} | ||
|
||
takesContext, err := validateArguments(handlerType) | ||
takesContext, err := handlerTakesContext(handlerType) | ||
if err != nil { | ||
return errorHandler(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 check doesn't appear in the
case 2:
branch, can it be removed?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'm guessing this check is to keep handlers like
func(event any) {}
valid. If that's the case, it's worth adding a comment!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.
yes.
If the first argument is an interface, the handler may be
func(event any)
orfunc(ctx context.Context)
.We should accept both.