feat(util-dynamodb): marshall to convert JavaScript object into DynamoDB Record

[trivikr]

Issue #, if available:
Refs: #1223

Description of changes:
Adds utility function marshall to convert JavaScript object into DynamoDB Record

const { DynamoDB } = require("@aws-sdk/client-dynamodb");
const { marshall } = require("@aws-sdk/util-dynamodb");

const client = new DynamoDB(clientParams);
const params = {
  TableName: "Table",
  Item: marshall({
    HashKey: "hashKey",
    NumAttribute: 1,
    BoolAttribute: true,
    ListAttribute: [1, "two", false],
    MapAttribute: { foo: "bar" },
    NullAttribute: null,
  }),
};

await client.putItem(params);

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[codecov-commenter]

Codecov Report

Merging #1531 into master will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1531      +/-   ##
==========================================
+ Coverage   79.80%   79.84%   +0.03%     
==========================================
  Files         298      304       +6     
  Lines       11502    11753     +251     
  Branches     2475     2511      +36     
==========================================
+ Hits         9179     9384     +205     
- Misses       2323     2369      +46     

Impacted Files	Coverage Δ
...l_tests/aws-json/commands/EmptyOperationCommand.ts	90.00% <0.00%> (-10.00%)	⬇️
...ts/aws-query/commands/NoInputAndNoOutputCommand.ts	90.00% <0.00%> (-10.00%)	⬇️
.../aws-restxml/commands/NoInputAndNoOutputCommand.ts	90.00% <0.00%> (-10.00%)	⬇️
...aws-restjson/commands/NoInputAndNoOutputCommand.ts	90.00% <0.00%> (-10.00%)	⬇️
protocol_tests/aws-ec2/commands/XmlBlobsCommand.ts	95.00% <0.00%> (-5.00%)	⬇️
protocol_tests/aws-ec2/commands/XmlEnumsCommand.ts	95.00% <0.00%> (-5.00%)	⬇️
protocol_tests/aws-ec2/commands/XmlListsCommand.ts	95.00% <0.00%> (-5.00%)	⬇️
...rotocol_tests/aws-query/commands/XmlMapsCommand.ts	95.00% <0.00%> (-5.00%)	⬇️
...otocol_tests/aws-query/commands/XmlBlobsCommand.ts	95.00% <0.00%> (-5.00%)	⬇️
...otocol_tests/aws-query/commands/XmlEnumsCommand.ts	95.00% <0.00%> (-5.00%)	⬇️
... and 130 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17620d6...015fd9b. Read the comment docs.

[russell-dot-js]

It would be really nice to expose some way to expose custom marshalling/unmarshalling config. Even if it's just a function for custom marshalling. One of the big drawbacks of the current DocumentClient implementations is that JS Dates serialize to maps, and the data gets completely hosed (saves as {} IIRC). What if we had something like this?

premarshall: ({key: string, value: any}) => any;

new DocumentClient({
  premarshall: ({key, value}) => {
    if(value instanceof Date) {
      return value.toIsoString(); // or convert to number depending on use case
    }
    if(value instanceof moment) {
      return value.format();
    }

    if(key === 'someKeyIDontWantInDynamo') {
      return undefined;
    }
    return value;
  }
})

Then the library would call premarshall prior to converting to AttributeValues

IMO it's more important to have this on the way in to dynamo on the way out, because there's always some casting from DB values to runtime values anyway (e.g. converting iso string back to date, stripping database concerns, etc). But some way to inject some casting logic that could be applied to Item, Attributes, Items, etc based on endpoint is a secondary item on my wishlist.

Obviously this is something that consumers can do themselves before calling UpdateItem / PutItem, but it creates additional complexity if, for example, every consumer has to wrap the documentClient with their own functionality to marshall. Or, even worse, have one-off code like such:

documentClient.putItem({
  Key,
  Item: {
     ...value,
     createdAt: value.createdAt.toISOString(),
   }
})

Code like the above becomes highly error prone, because even with typescript, adding a new Date value to typeof T will not break any assumptions and consumers have to jump through additional hoops to keep their DB logic in line with their types, when in reality it should "just work" (without having to cross the line into a full blown ORM).

What are your thoughts?

[trivikr]

@russell-dot-js Custom premarshaller is a good idea. Can you create a feature-request for it?
Link: https://github.com/aws/aws-sdk-js-v3/issues/new?assignees=&labels=feature-request&template=---feature-request.md&title=

Since v3 is is prerelease, we can still modify the marshaller (convertToAttr).
Is Date the only type you have concern about? Or are there generic JavaScript types which should be included in default implementation?

[russell-dot-js]

@russell-dot-js Custom premarshaller is a good idea. Can you create a feature-request for it?
Link: https://github.com/aws/aws-sdk-js-v3/issues/new?assignees=&labels=feature-request&template=---feature-request.md&title=

Since v3 is is prerelease, we can still modify the marshaller (convertToAttr).
Is Date the only type you have concern about? Or are there generic JavaScript types which should be included in default implementation?

@trivikr I'll take the discussion to the issue, and I'm happy to implement the feature, but should there be a default implementation? I wouldn't want to break consumers who pass dates blindly and expect a number or a string... then again, right now, dates don't work at all. Maybe even logging a warning when an unmarshalled date makes it to convertToAttr would be good enough...

My concern about default implementation is that everyone has a different way of using dynamo. Some people may cram 4 attributes into a sort key, then convert back to whatever those fields were to begin with on the way out. So it might be tough for a default implementation to make an assumption on how other native JS types should be handled - but we can at least identify when we receive those and don't know how to marshall them.

[russell-dot-js]

overall LGTM, but may be regressions from current DocumentClient Converter's handling of objects

other misc. comments added

[russell-dot-js]

is this code borrowed from the current (v2) DocumentClient? or is this a new implementation?

if I'm reading correctly, this will only serialize pure JS Objects to JSON.
Anyone who's using ES6 classes or other easily serializable objects would be hosed without custom premarshall?

Again, borrowing the rules from JSON serialization would be $

[trivikr]

is this code borrowed from the current (v2) DocumentClient? or is this a new implementation?

This PR is new implementation in TypeScript based on the behavior of v2 DocumentClient.

Anyone who's using ES6 classes or other easily serializable objects would be hosed without custom premarshall?

Can you provide an example?

[russell-dot-js]

For sure
Here is the source, btw:
https://github.com/aws/aws-sdk-js/blob/5155fde557ecc1d166bb5040823289254bea1641/lib/dynamodb/converter.js#L27

The old DocumentClient defaulted to treating objects as map. Thus

class FooObj {
  constructor() {
    this.foo = 'foo';
  }
}

JSON.stringify(new FooObj())
"{"foo":"foo"}"

// new implementation needs constructor name to be Object or it errors
(new FooObj()).constructor.name
"FooObj"

// old implementation checked typeof
typeof new FooObj()
"object"

See https://github.com/aws/aws-sdk-js/blob/cc29728c1c4178969ebabe3bbe6b6f3159436394/lib/dynamodb/types.js#L3

[trivikr]

This is a miss.

However, using JSON.stringify() for serializing and JSON.parse() for deserializing is a norm in JavaScript world.

• Example StackOverflow discussion: https://stackoverflow.com/a/40201783
• MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify

I created an issue to discuss this internally, and will post updates in #1535

[AllanZhengYP]

This code doesn't check the prototype chain. Maybe if ( data instanceof Object){} works better here.

[trivikr]

We're not using data instanceof Object, as it applies to binary data as shown in code below:

const data = new ArrayBuffer();
data instanceof Object;

[AllanZhengYP]

Would checking in order of set, list, map, scalars help?

[trivikr]

Would checking in order of set, list, map, scalars help?

The check for binary happens under scalars, so map check should be moved after it.
Currently, check for scalars is in an else block, so check for map will have to be moved inside scalars.

[trivikr]

Can you share an example where data?.constructor?.name === "Object" will fail for JS for maps because of prototype chain?

[trivikr]

should there be a default implementation for Date?

The recommended way in DynamoDB is to store dates is ISO 8601 strings.
Documentation: https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/HowItWorks.NamingRulesDataTypes.html#HowItWorks.DataTypes.String

I'll store date by converting it to ISO 8601 string using toISOString() in convertToAttr function
MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toISOString

[russell-dot-js]

should there be a default implementation for Date?

The recommended way in DynamoDB is to store dates is ISO 8601 strings.
Documentation: https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/HowItWorks.NamingRulesDataTypes.html#HowItWorks.DataTypes.String

I'll store date by converting it to ISO 8601 string using toISOString() in convertToAttr function
MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toISOString

Reading those docs, it doesn't say the recommended way is ISO 8601. It says you "can use ISO 8601". Immediately before that, in the numbers docs, it says you can use numbers also to represent date. It does not make a recommendation.

We use ISO 8601, but I don't think the default sdk should make assumptions on what you want to do. In other languages (e.g. Go, .NET, Java), the dynamo client uses JSON serialization by default. This is why I am in favor of a pre marshall config rather than baking assumptions into DocumentClient.

[trivikr]

I created a feature request for supporting date #1534

This PR will continue throwing error for date - as it fixes the bug with v2 DynamoDB client which converts Date to an empty map as follows:

Code

const AWS = require("aws-sdk");

(async () => {
  const region = "us-west-2";

  const TableName = `test-doc-client`;
  const id = "id1";
  const date = new Date();

  const client = new AWS.DynamoDB.DocumentClient({ region });
  await client.put({ TableName, Item: { id, date } }).promise();
  console.log(await client.get({ TableName, Key: { id } }).promise());
})();

Output

{ Item: { id: 'id1', date: {} } }

[AllanZhengYP]

This code doesn't check the prototype chain. Maybe if ( data instanceof Object){} works better here.

[trivikr]

The build failure is related to intermittent test failure of NodeHttpHandler
Created issue at #1539

[AllanZhengYP]

It's not a blocker, but having cross-platform test cases here is necessary. It will validate this works for Buffer as well as Blob and File

[trivikr]

It's not a blocker, but having cross-platform test cases here is necessary. It will validate this works for Buffer as well as Blob and File

Created an issue for follow-up #1542

[AllanZhengYP]

Awesome! Thanks for putting this together!

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.