improvement of validating arguments #365
Conversation
Codecov Report
@@ Coverage Diff @@
## master #365 +/- ##
==========================================
+ Coverage 72.22% 72.59% +0.37%
==========================================
Files 19 19
Lines 738 748 +10
==========================================
+ Hits 533 543 +10
Misses 138 138
Partials 67 67
Continue to review full report at Codecov.
|
lambda/handler.go
Outdated
| return false, nil | ||
| } | ||
| if !contextType.Implements(argumentType) { | ||
| return false, errors.New("the first argument is an interface, but it is not a Context") |
There was a problem hiding this comment.
nit pick: consider following the existing error pattern of handler.... Maybe handler argument is an interface that context.Context does not implement? Or, if we exposed valuer it could be handler argument does not implement Valuer.
| if !contextType.Implements(argumentType) { | ||
| return false, errors.New("the first argument is an interface, but it is not a Context") | ||
| } | ||
| if argumentType.NumMethod() == 0 { |
There was a problem hiding this comment.
I don't quite understand what this line is doing. Can you provide a bit more detail?
There was a problem hiding this comment.
In this case, the handler has the following signature.
func handler(v interface{}) {}
func main() { lambda.Start(handler) }We have two choices to call the handler.
First is:
ctx := context.TODO()
handler(ctx)and, second is:
v, _ := json.Unmarshal(data)
handler(v)Calling the handler succeeds in either case, but we need to choose one of them.
Current implementation chooses the second.
So, to avoid breaking backward compatibility, we should the second here.
There was a problem hiding this comment.
Assuming the first argument does implement valuer (ie contextType.implements(argumentType) == true) wouldn't it always have at least 1 exported method? And if it did wouldn't this check always return false?
There was a problem hiding this comment.
And if it did wouldn't this check always return false?
No.
contextType.implements(argumentType) == true means the argumentType is a subset of context.Context.
The subsets of context.Context includes not only valuer but also interface{}.
Here is an example code: https://play.golang.org/p/qZMYLva7Rzo
package main
import (
"context"
"fmt"
)
type valuer interface {
Value(key interface{}) interface{}
}
func main() {
var ok bool
ctx := context.TODO()
_, ok = ctx.(valuer)
fmt.Printf("ctx implements valuer?: %t\n", ok)
_, ok = ctx.(interface{})
fmt.Printf("ctx implements interface{}?: %t\n", ok)
// Output:
// ctx implements valuer?: true
// ctx implements interface{}?: true
}| contextType := reflect.TypeOf((*context.Context)(nil)).Elem() | ||
| argumentType := handler.In(0) |
There was a problem hiding this comment.
Not a big deal but it would be nice if we didn't have to duplicate these lines. I wonder if it would make sense to combine case 1 and case 2 together somehow? I'd have to see what it looked like to understand if it impacts readability/maintainability.
There was a problem hiding this comment.
case 1 has a lot of corner cases. it is too difficult to combine them.
| } | ||
| for i, testCase := range testCases { | ||
| testCase := testCase | ||
| t.Run(fmt.Sprintf("testCase[%d] %s", i, testCase.name), func(t *testing.T) { | ||
| lambdaHandler := NewHandler(testCase.handler) | ||
| _, err := lambdaHandler.Invoke(context.TODO(), make([]byte, 0)) | ||
| _, err := lambdaHandler.Invoke(context.TODO(), []byte("{}")) |
There was a problem hiding this comment.
Why was this line changed?
There was a problem hiding this comment.
because it should be valid JSON.
If it is invalid, the Invoke returns an error of parsing JSON.
There was a problem hiding this comment.
huh, interesting. Thanks for the fix.
| } | ||
| for i, testCase := range testCases { | ||
| testCase := testCase | ||
| t.Run(fmt.Sprintf("testCase[%d] %s", i, testCase.name), func(t *testing.T) { | ||
| lambdaHandler := NewHandler(testCase.handler) | ||
| _, err := lambdaHandler.Invoke(context.TODO(), make([]byte, 0)) | ||
| _, err := lambdaHandler.Invoke(context.TODO(), []byte("{}")) |
There was a problem hiding this comment.
huh, interesting. Thanks for the fix.
| if !contextType.Implements(argumentType) { | ||
| return false, errors.New("the first argument is an interface, but it is not a Context") | ||
| } | ||
| if argumentType.NumMethod() == 0 { |
There was a problem hiding this comment.
Assuming the first argument does implement valuer (ie contextType.implements(argumentType) == true) wouldn't it always have at least 1 exported method? And if it did wouldn't this check always return false?
|
|
||
| type customContext interface { | ||
| context.Context | ||
| MyCustomMethod() |
There was a problem hiding this comment.
You mention in the PR description that you wanted to be able to use this kind of interface with your handlers but if all aws-lambda-go is passing in is a valuer (ie, a context.Context) wouldn't this fail at runtime because context.Context does not satisfy customContext?
Looking at the test it must not fail at runtime but I don't understand exactly how.
There was a problem hiding this comment.
I want to use interfaces like valuer, but I don't need to use customContext.
wouldn't this fail at runtime because context.Context does not satisfy customContext?
Yes, actually it panics with "reflect: Call using *context.valueCtx as type main.customContext" in aws/aws-lambda-go.
Users can’t understand it is a bug of aws/aws-lambda-go or wrong usage of aws/aws-lambda-go.
so, aws/aws-lambda-go should return an error with more friendly message rather than panicking.
Defining a handler to take only a subset of context.Context seems weird to me, can you help me understand why this would be a useful feature? |
|
Actually, I don't have any idea for useful use case of this feature. For example, the package main
import (
"context"
)
// valuer is a subset of context.Context
type valuer interface {
Value(key interface{}) interface{}
}
func handler(ctx valuer) {
return
}
func main() {
handler(context.TODO())
}In other hand, the package main
import (
"context"
)
// customContext is a superset of context.Context
// In other words, `customContext` implements `context.Context`.
type customContext interface {
context.Context
MyCustomMethod()
}
func handler(ctx customContext) {
return
}
func main() {
// compile error:
// cannot use context.TODO() (type context.Context) as type customContext in argument to handler:
// context.Context does not implement customContext (missing MyCustomMethod method)
handler(context.TODO())
}In this case, the first argument of the |
|
Note that I'm using the word "subset" as a term of mathematics.
So, |
Codecov Report
@@ Coverage Diff @@
## master #365 +/- ##
==========================================
+ Coverage 72.22% 72.59% +0.37%
==========================================
Files 19 19
Lines 738 748 +10
==========================================
+ Hits 533 543 +10
Misses 138 138
Partials 67 67
Continue to review full report at Codecov.
|
|
Can we merge this? |
|
Hi, any progress? |
There was a problem hiding this comment.
I think the size of the changes to resolve #377 be reduced, by making the context type check an equality one.
eg: doing the following change
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 {
contextType := reflect.TypeOf((*context.Context)(nil)).Elem()
argumentType := handler.In(0)
- handlerTakesContext = argumentType.Implements(contextType)
+ handlerTakesContext = argumentType == contextType
if handler.NumIn() > 1 && !handlerTakesContext {
return false, fmt.Errorf("handler takes two arguments, but the first is not Context. got %s", argumentType.Kind())
}
}If there's no major back-compat issue with something like this, I'd rather make the smaller change. Adding the other logic from this PR to support sub interfaces of context.Context is additional surface area that I'm not comfortable with as a maintainer. (unless there's some usecase that could be enabled by that - but that may be better discussed in a standalone issue)
Your example may break compatibility. e.g. // myContext has same definition with context.Context.
type myContext interface {
Deadline() (deadline time.Time, ok bool)
Done() <-chan struct{}
Err() error
Value(key any) any
}
func handler(ctx myContext, v interface{}) (events.APIGatewayProxyResponse, error) {
return events.APIGatewayProxyResponse{
Body: "Hello",
StatusCode: 200,
}, nil
}
func main() {
lambda.Start(handler)
}but after fixing, validation will fail because package main
import (
"context"
"fmt"
"reflect"
"time"
)
// myContext has same definition with context.Context.
type myContext interface {
Deadline() (deadline time.Time, ok bool)
Done() <-chan struct{}
Err() error
Value(key any) any
}
func main() {
contextType := reflect.TypeOf((*context.Context)(nil)).Elem()
myContextType := reflect.TypeOf((*myContext)(nil)).Elem()
// but not equals.
fmt.Println(contextType == myContextType)
// Output:
// false
} |
|
I've just opened a new pull request #475. |
|
closing since #475 was merged |
Issue #, if available:
I passed
interface { Value(key interface{}) interface{} }to the first argument of the handler.Actual Behavior:
It fails with "handler takes two arguments, but the first is not Context. got interface".
Here is the result of running with SAM CLI.
Expected Behavior:
It should be same result as the first argument is
context.Context.Another example:
It panics with "reflect: Call using *context.valueCtx as type main.customContext".
aws/aws-lambda-go should return an error with more friendly message.
Description of changes:
The
validateArgumentsvalidates that the first argument implementscontext.Context.It is documented in the official document:
The
validateArgumentsshould validate that the first argument is an interface and a subset ofcontext.Context.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.