Aws-sdk-go: dynamodbattribute: Misleading omitempty Behavior

[seiffert]

We recently switched to the dynamodbatttribute package for marshalling DynamoDB records and stumbled upon the fact that the omitempty behavior is slightly different from the one in the json package. With this issue, I'd like to discuss whether this is intended or a bug.

Take a look at this code:

type Foo struct {
        Bar *bool `json:"bar,omitempty"`
}

func main() {
        v := false
        a := Foo{&v}

        av, _ := dynamodbattribute.Marshal(a)
        jsonV, _ := json.Marshal(a)

        log.Printf("%#v", av)
        log.Printf("%s", string(jsonV))

        var b, c Foo
        _ = dynamodbattribute.Unmarshal(av, &b)
        _ = json.Unmarshal(jsonV, &c)

        log.Printf("%#v", b)
        log.Printf("%#v", c)
}

which results in

$ go run main.go 
2016/07/12 09:18:22 {
  NULL: true
}
2016/07/12 09:18:22 {"bar":false}
2016/07/12 09:18:22 main.Foo{Bar:(*bool)(nil)}
2016/07/12 09:18:22 main.Foo{Bar:(*bool)(0xc820075158)}

There you can see that the json package marshals the pointer to the boolean value false even if omitempty is set. dynamodbattribute ignores it.

On the other hand providing a nil-pointer in Foo.Bar:

func main() {
        a := Foo{}

        av, _ := dynamodbattribute.Marshal(a)
        jsonV, _ := json.Marshal(a)

        log.Printf("%#v", av)
        log.Printf("%s", string(jsonV))

        var b, c Foo
        _ = dynamodbattribute.Unmarshal(av, &b)
        _ = json.Unmarshal(jsonV, &c)

        log.Printf("%#v", b)
        log.Printf("%#v", c)
}

leads to

$ go run main.go 
2016/07/12 09:25:00 {
  NULL: true
}
2016/07/12 09:25:00 {}
2016/07/12 09:25:00 main.Foo{Bar:(*bool)(nil)}
2016/07/12 09:25:00 main.Foo{Bar:(*bool)(nil)}

The result is the same for the dynamodbattribute marshaller. The json marshaller now ignores the attribute.

TL;DR
With the JSON package, you can use pointers to scalar values to distinguish between the situations

1. The value was set to true
2. The value was set to false
3. The value was _not_ set

Do you agree that this behavior would also make sense for the dynamodbattribute package?

[jasdel]

Thanks for the feedback @seiffert. I think we can consider this a bug in the SDK's attribute marshaler, because with the current functionality it would be very difficult to provide zero value for pointer type values.

I think we can change this behavior to only do an omit empty check for the pointer and not for the underlying value. To fix this, I think we need to how Encoder.encode determines if a value should be skipped or not.

Locally I added the following test to service/dynamodb/dynamodbattribute/encode_test.go that highlights this problem.

type testOmitEmptyScalar struct {
    IntZero       int  `dynamodbav:",omitempty"`
    IntPtrNil     *int `dynamodbav:",omitempty"`
    IntPtrSetZero *int `dynamodbav:",omitempty"`
}

func TestMarshalOmitEmpty(t *testing.T) {
    expect := &dynamodb.AttributeValue{
        M: map[string]*dynamodb.AttributeValue{
            "IntPtrSetZero": {N: aws.String("0")},
        },
    }

    m := testOmitEmptyScalar{IntPtrSetZero: aws.Int(0)}

    actual, err := Marshal(m)
    assert.NoError(t, err)
    assert.Equal(t, expect, actual)
}

[seiffert]

Thanks for the fast response and fix!

[xibz]

@seiffert - No problem and thank you for finding this!