Skip to content

Commit

Permalink
Fix generated errors for some JSON APIs not including message (aws#3089)
Browse files Browse the repository at this point in the history
Fixes the SDK's generated errors to all include the `Message` member
regardless if it was modeled on the error shape. This fixes the bug
identified in aws#3088 where some JSON errors were not modeled with the
Message member. This caused the errors message to be dropped, and not
retrievable.

This change also updates the SDK's generated smoke test to expect the
Message member of an error unmarshaled not to be empty.

Fixes aws#3088
  • Loading branch information
jasdel authored Jan 21, 2020
1 parent 4c5dc88 commit 6ca8a54
Show file tree
Hide file tree
Showing 108 changed files with 3,490 additions and 16 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG_PENDING.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,5 @@
### SDK Enhancements

### SDK Bugs
* Fix generated errors for some JSON APIs not including a message ([#3089](https://github.com/aws/aws-sdk-go/issues/3089))
* Fixes the SDK's generated errors to all include the `Message` member regardless if it was modeled on the error shape. This fixes the bug identified in #3088 where some JSON errors were not modeled with the Message member.
15 changes: 15 additions & 0 deletions private/model/api/codegentest/service/restjsonservice/api.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions private/model/api/codegentest/service/restxmlservice/api.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions private/model/api/codegentest/service/rpcservice/api.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions private/model/api/load.go
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@ func (a *API) Setup() error {
a.setMetadataEndpointsKey()
a.writeShapeNames()
a.resolveReferences()
a.backfillErrorMembers()

if !a.NoRemoveUnusedShapes {
a.removeUnusedShapes()
Expand Down
41 changes: 41 additions & 0 deletions private/model/api/passes.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,47 @@ func (a *API) resolveReferences() {
}
}

func (a *API) backfillErrorMembers() {
stubShape := &Shape{
ShapeName: "string",
Type: "string",
}
var locName string
switch a.Metadata.Protocol {
case "ec2", "query", "rest-xml":
locName = "Message"
case "json", "rest-json":
locName = "message"
}

for _, s := range a.Shapes {
if !s.Exception {
continue
}

var haveMessage bool
for name := range s.MemberRefs {
if strings.EqualFold(name, "Message") {
haveMessage = true
break
}
}
if !haveMessage {
ref := &ShapeRef{
ShapeName: stubShape.ShapeName,
Shape: stubShape,
LocationName: locName,
}
s.MemberRefs["Message"] = ref
stubShape.refs = append(stubShape.refs, ref)
}
}

if len(stubShape.refs) != 0 {
a.Shapes["SDKStubErrorMessageShape"] = stubShape
}
}

// A referenceResolver provides a way to resolve shape references to
// shape definitions.
type referenceResolver struct {
Expand Down
3 changes: 3 additions & 0 deletions private/model/api/smoke.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,9 @@ var smokeTestTmpl = template.Must(template.New(`smokeTestTmpl`).Parse(`
if len(aerr.Code()) == 0 {
t.Errorf("expect non-empty error code")
}
if len(aerr.Message()) == 0 {
t.Errorf("expect non-empty error message")
}
if v := aerr.Code(); v == request.ErrCodeSerialization {
t.Errorf("expect API error code got serialization failure")
}
Expand Down
60 changes: 46 additions & 14 deletions private/protocol/json/jsonutil/unmarshal.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"fmt"
"io"
"reflect"
"strings"
"time"

"github.com/aws/aws-sdk-go/aws"
Expand Down Expand Up @@ -45,10 +46,31 @@ func UnmarshalJSON(v interface{}, stream io.Reader) error {
return err
}

return unmarshalAny(reflect.ValueOf(v), out, "")
return unmarshaler{}.unmarshalAny(reflect.ValueOf(v), out, "")
}

func unmarshalAny(value reflect.Value, data interface{}, tag reflect.StructTag) error {
// UnmarshalJSONCaseInsensitive reads a stream and unmarshals the result into the
// object v. Ignores casing for structure members.
func UnmarshalJSONCaseInsensitive(v interface{}, stream io.Reader) error {
var out interface{}

err := json.NewDecoder(stream).Decode(&out)
if err == io.EOF {
return nil
} else if err != nil {
return err
}

return unmarshaler{
caseInsensitive: true,
}.unmarshalAny(reflect.ValueOf(v), out, "")
}

type unmarshaler struct {
caseInsensitive bool
}

func (u unmarshaler) unmarshalAny(value reflect.Value, data interface{}, tag reflect.StructTag) error {
vtype := value.Type()
if vtype.Kind() == reflect.Ptr {
vtype = vtype.Elem() // check kind of actual element type
Expand Down Expand Up @@ -80,17 +102,17 @@ func unmarshalAny(value reflect.Value, data interface{}, tag reflect.StructTag)
if field, ok := vtype.FieldByName("_"); ok {
tag = field.Tag
}
return unmarshalStruct(value, data, tag)
return u.unmarshalStruct(value, data, tag)
case "list":
return unmarshalList(value, data, tag)
return u.unmarshalList(value, data, tag)
case "map":
return unmarshalMap(value, data, tag)
return u.unmarshalMap(value, data, tag)
default:
return unmarshalScalar(value, data, tag)
return u.unmarshalScalar(value, data, tag)
}
}

func unmarshalStruct(value reflect.Value, data interface{}, tag reflect.StructTag) error {
func (u unmarshaler) unmarshalStruct(value reflect.Value, data interface{}, tag reflect.StructTag) error {
if data == nil {
return nil
}
Expand All @@ -114,7 +136,7 @@ func unmarshalStruct(value reflect.Value, data interface{}, tag reflect.StructTa
// unwrap any payloads
if payload := tag.Get("payload"); payload != "" {
field, _ := t.FieldByName(payload)
return unmarshalAny(value.FieldByName(payload), data, field.Tag)
return u.unmarshalAny(value.FieldByName(payload), data, field.Tag)
}

for i := 0; i < t.NumField(); i++ {
Expand All @@ -128,17 +150,27 @@ func unmarshalStruct(value reflect.Value, data interface{}, tag reflect.StructTa
if locName := field.Tag.Get("locationName"); locName != "" {
name = locName
}
if u.caseInsensitive {
if _, ok := mapData[name]; !ok {
// Fallback to uncased name search if the exact name didn't match.
for kn, v := range mapData {
if strings.EqualFold(kn, name) {
mapData[name] = v
}
}
}
}

member := value.FieldByIndex(field.Index)
err := unmarshalAny(member, mapData[name], field.Tag)
err := u.unmarshalAny(member, mapData[name], field.Tag)
if err != nil {
return err
}
}
return nil
}

func unmarshalList(value reflect.Value, data interface{}, tag reflect.StructTag) error {
func (u unmarshaler) unmarshalList(value reflect.Value, data interface{}, tag reflect.StructTag) error {
if data == nil {
return nil
}
Expand All @@ -153,7 +185,7 @@ func unmarshalList(value reflect.Value, data interface{}, tag reflect.StructTag)
}

for i, c := range listData {
err := unmarshalAny(value.Index(i), c, "")
err := u.unmarshalAny(value.Index(i), c, "")
if err != nil {
return err
}
Expand All @@ -162,7 +194,7 @@ func unmarshalList(value reflect.Value, data interface{}, tag reflect.StructTag)
return nil
}

func unmarshalMap(value reflect.Value, data interface{}, tag reflect.StructTag) error {
func (u unmarshaler) unmarshalMap(value reflect.Value, data interface{}, tag reflect.StructTag) error {
if data == nil {
return nil
}
Expand All @@ -179,14 +211,14 @@ func unmarshalMap(value reflect.Value, data interface{}, tag reflect.StructTag)
kvalue := reflect.ValueOf(k)
vvalue := reflect.New(value.Type().Elem()).Elem()

unmarshalAny(vvalue, v, "")
u.unmarshalAny(vvalue, v, "")
value.SetMapIndex(kvalue, vvalue)
}

return nil
}

func unmarshalScalar(value reflect.Value, data interface{}, tag reflect.StructTag) error {
func (u unmarshaler) unmarshalScalar(value reflect.Value, data interface{}, tag reflect.StructTag) error {

switch d := data.(type) {
case nil:
Expand Down
2 changes: 1 addition & 1 deletion private/protocol/jsonrpc/unmarshal_error.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func (u *UnmarshalTypedError) UnmarshalError(
// If exception code is know, use associated constructor to get a value
// for the exception that the JSON body can be unmarshaled into.
v := fn(respMeta)
err := jsonutil.UnmarshalJSON(v, body)
err := jsonutil.UnmarshalJSONCaseInsensitive(v, body)
if err != nil {
return nil, err
}
Expand Down
11 changes: 11 additions & 0 deletions private/protocol/jsonrpc/unmarshal_error_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (

const unknownErrJSON = `{"__type":"UnknownError", "message":"error message", "something":123}`
const simpleErrJSON = `{"__type":"SimpleError", "message":"some message", "foo":123}`
const simpleCasedErrJSON = `{"__type":"SimpleError", "Message":"some message", "foo":123}`

type SimpleError struct {
_ struct{} `type:"structure"`
Expand Down Expand Up @@ -138,6 +139,16 @@ func TestUnmarshalTypedError(t *testing.T) {
},
Err: "failed decoding",
},
"mixed case fields": {
Response: &http.Response{
Header: http.Header{},
Body: ioutil.NopCloser(strings.NewReader(simpleCasedErrJSON)),
},
Expect: &SimpleError{
Message2: aws.String("some message"),
Foo: aws.Int64(123),
},
},
}

for name, c := range cases {
Expand Down
2 changes: 1 addition & 1 deletion private/protocol/restjson/unmarshal_error.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ func (u *UnmarshalTypedError) UnmarshalError(
// If exception code is know, use associated constructor to get a value
// for the exception that the JSON body can be unmarshaled into.
v := fn(respMeta)
if err := jsonutil.UnmarshalJSON(v, body); err != nil {
if err := jsonutil.UnmarshalJSONCaseInsensitive(v, body); err != nil {
return nil, err
}

Expand Down
3 changes: 3 additions & 0 deletions service/acm/integ_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions service/apigateway/integ_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit 6ca8a54

Please sign in to comment.