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

service/dynamodb/dynamodbattribute: Marshal empty map inserts Null AttributeValue instead of empty Map #682

Open
sethjback opened this Issue May 11, 2016 · 11 comments

Comments

Projects
None yet
5 participants
@sethjback

sethjback commented May 11, 2016

I am curious as to the behavior of the dynamodbattribute Encoder: it iterates over maps, converting members to the appropriate dynamodb.AttributeValue. If the resulting map len is 0, however, it opts to return a Null AttributeValue rather than an empty Map attribute value.

This makes writing update expressions in the future problematic without first retrieving the existing item from dynamodb and checking for either a Null or missing attribute (in both cases writing to a nested field will return the "document path provided ... is invalid " error).

I know you can work around this issue by implementing your own Marshaler, but I am curious as to the reason for returning Null rather than an empty map. In both cases you are actually adding an AttributeValue to the item in the table, and in my mind a Null AttributeValue is different from an empty Map AttributeValue.

@jasdel

This comment has been minimized.

Member

jasdel commented May 11, 2016

Hi @sethjback thanks for contacting us. Setting NULL for empty maps/slices, and nil pointers was intentionally to follow the existing pattern of the ConvertTo functions. Because of this we also added the ability to omitempty fields. This setting can be applied to individual struct fields via struct tag omitempty. This won't encode empty maps/slices but prevents the fields being encoded at all.

With that said, I can see the reason for wanting the ability to encode empty maps and slices as {} and [] respectivily. I think we should be able to add this to the encoder's configuration options, and possibly as a struct tag for maps and slices

@jasdel jasdel self-assigned this May 11, 2016

@guyc

This comment has been minimized.

guyc commented Jun 23, 2016

👍 I'm having the same problem, and would love to see an option to ensure empty maps and arrays are preserved.

@jasdel

This comment has been minimized.

Member

jasdel commented Jun 23, 2016

A possible workaround for this until the functionality is implemented would be to implement the dynamodbattributes.Marshaler interface on your map/list type that could be empty. The returned dynamodb.AttributeValue would be an empty list value.

This item is in our backlog and haven't begun work to address it yet. with that said we're more than glad to review community PRs, and help with any contributions people would like to make.

In this case I could see this be implemented as another struct tag. Maybe something like zeroempty. When a slice or map field is found to be empty and has that tag an empty dynamodb.AttributeValue for Map or List would be set as needed. It would be nice if we could provide this functionality in more configurable way, but I'm not positive how that would look.

@guyc

This comment has been minimized.

guyc commented Jun 23, 2016

Thanks @jasdel, we are using a custom marshaller and that's working fine. One thought on using a struct tag: we are marshalling a complex object passed from a UI library as JSON, and in the API side we expect it to round-trip through dynamodb unaltered. We don't really want to define structs for all of the deep elements in the structure to be able to tag them. For this use case a new setting in MarshalOptions that indicates zero-length slices and maps should be preserved would be ideal.

@jasdel

This comment has been minimized.

Member

jasdel commented Jun 28, 2016

Agreed, i think it makes sense to include an optional flag in the MarshalOptions also for this functionality. Not sure yet if two flags or a single would be better. Two may make more sense to follow a pattern of NullEmptyStrings

@jasdel jasdel changed the title from DynamoDB: Marshal empty map inserts Null AttributeValue instead of empty Map to service/dynamodb/dynamodbattribute: Marshal empty map inserts Null AttributeValue instead of empty Map Apr 12, 2017

@marcato15

This comment has been minimized.

marcato15 commented Aug 3, 2017

Checking in on the status to see if this is still just in the feature request but not implemented stage.

@jasdel

This comment has been minimized.

Member

jasdel commented Aug 4, 2017

Hi @marcato15 this feature is still in feature request phase. If you're looking to create a PR we're glad to review it. A new flag option in MarshalOptions is probably the best way to make this an opt in feature.

@marcato15

This comment has been minimized.

marcato15 commented Aug 4, 2017

I got it working via a custom marshaler but might give a try to add the feature to the sdk. The only thing I'm running into is that I may want an empty list on a top level attribute but not necessarily on children attribute, as having an empty map inside an empty list might still initialize both an empty list and an empty map inside that list.

My current implementation is a generic function that works for any struct and you pass it a list of the fields you want to set as empty lists/maps if they are nil. While I don't like that approach I'm struggling to find a binary setting that'd allow the right types of lists/map to be set as empty without setting nested lists/maps.

@jasdel

This comment has been minimized.

Member

jasdel commented Aug 4, 2017

Thanks for the update. in this context I think you are correct the best way to solve the non-boolean path is with custom marshalers.

One potential alternative is to add a new struct tags that instructs the marshaler to render the field as an empty value. Kind of the opposite to omitempty for maps and slices. Maybe something like includeempty the naming seems odd though since "include" is the default functionality.

@marcato15

This comment has been minimized.

marcato15 commented Aug 4, 2017

I really like the idea of adding a struct tag. I may push on that for a bit and see what I can do. I think includeempty may be the best idea, even if it as you mention it does sound weird.

jcoyne added a commit to sul-dlss-labs/taco that referenced this issue Apr 12, 2018

Add custom Marshal/Unmarshal to support empty lists
This makes it possible to round trip our data without loosing empty
lists. Note, dynamodb doesn't support empty strings, and these changes
do nothing to work around that.

Fixes #387

See: aws/aws-sdk-go#682

jcoyne added a commit to sul-dlss-labs/taco that referenced this issue Apr 12, 2018

Add custom Marshal/Unmarshal to support empty lists
This makes it possible to round trip our data without loosing empty
lists. Note, dynamodb doesn't support empty strings, and these changes
do nothing to work around that.

Fixes #387

See: aws/aws-sdk-go#682

jcoyne added a commit to sul-dlss-labs/taco that referenced this issue Apr 12, 2018

Add custom Marshal/Unmarshal to support empty lists
This makes it possible to round trip our data without loosing empty
lists. Note, dynamodb doesn't support empty strings, and these changes
do nothing to work around that.

Fixes #387

See: aws/aws-sdk-go#682

jcoyne added a commit to sul-dlss-labs/taco that referenced this issue Apr 12, 2018

Add custom Marshal/Unmarshal to support empty lists
This makes it possible to round trip our data without loosing empty
lists. Note, dynamodb doesn't support empty strings, and these changes
do nothing to work around that.

Fixes #387

See: aws/aws-sdk-go#682

jcoyne added a commit to sul-dlss-labs/taco that referenced this issue Apr 23, 2018

Add custom Marshal/Unmarshal to support empty lists
This makes it possible to round trip our data without loosing empty
lists. Note, dynamodb doesn't support empty strings, and these changes
do nothing to work around that.

Fixes #387

See: aws/aws-sdk-go#682

jcoyne added a commit to sul-dlss-labs/taco that referenced this issue Apr 24, 2018

Add custom Marshal/Unmarshal to support empty lists
This makes it possible to round trip our data without loosing empty
lists. Note, dynamodb doesn't support empty strings, and these changes
do nothing to work around that.

Fixes #387

See: aws/aws-sdk-go#682

jcoyne added a commit to sul-dlss-labs/taco that referenced this issue Apr 24, 2018

Add custom Marshal/Unmarshal to support empty lists
This makes it possible to round trip our data without loosing empty
lists. Note, dynamodb doesn't support empty strings, and these changes
do nothing to work around that.

Fixes #387

See: aws/aws-sdk-go#682

eefahy added a commit to sul-dlss-labs/taco that referenced this issue Apr 24, 2018

Add custom Marshal/Unmarshal to support empty lists
This makes it possible to round trip our data without loosing empty
lists. Note, dynamodb doesn't support empty strings, and these changes
do nothing to work around that.

Fixes #387

See: aws/aws-sdk-go#682
@0xdevalias

This comment has been minimized.

0xdevalias commented May 27, 2018

If you're ok with hacks in the interim, this was the simplest way I found to workaround this:

av, err := dynamodbattribute.MarshalMap(user)
if err != nil {
  return nil, err
}

// Hack to work around https://github.com/aws/aws-sdk-go/issues/682
emptyMap := make(map[string]*dynamodb.AttributeValue)
av["foo"] = &dynamodb.AttributeValue{M: emptyMap}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment