Skip to content
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

util-dynamodb convertToAttr throws error when trying to serialize ES6 classes #1535

Closed
trivikr opened this issue Sep 18, 2020 · 8 comments · Fixed by #1969
Closed

util-dynamodb convertToAttr throws error when trying to serialize ES6 classes #1535

trivikr opened this issue Sep 18, 2020 · 8 comments · Fixed by #1969
Assignees

Comments

@trivikr
Copy link
Member

trivikr commented Sep 18, 2020

Describe the bug
util-dynamodb convertToAttr throws error when trying to serialize ES6 classes
Refs: #1531 (comment)

SDK version number
master

Is the issue in the browser/Node.js/ReactNative?
N/A

To Reproduce (observed behavior)
A unit test is added under unsupported type in PR #1531

Expected behavior
Questionable, as v2 DynamodDB serializes ES6 class objects as maps.

Storing as string using JSON.stringify is more helpful, as class properties can be deserialized using JSON.parse:

JSON.stringify will also store Date (ref: #1534) in ISO 8601 format as follows:

> const now = new Date();
> const serialized = JSON.stringify(now);
> const past = new Date(JSON.parse(serialized));
> now.getTime() === past.getTime()
true
@russell-dot-js
Copy link
Contributor

IMO it might be nice if we expose an interface similar to the Go dynamo SDK

export interface DynamoSerializable {
  toDynamoDB: () => any;
}

Then in document client, prior to converting to attribute values:

function isDynamoSerializable<T>(value: T): T is DynamoSerializable {
  return !!((T as DynamoSerializable).toDynamoDB);
}

const valueToConvert = isDynamoSerializable(value) ?
  value.toDynamoDB() :
  value;

const attrs = convertToAttr(valueToConvert);

@trivikr
Copy link
Member Author

trivikr commented Sep 28, 2020

The comment by @russell-dot-js seems to be a better solution where user provides us with serialization function for class objects.

Verified by using serialize and deserialize methods from class-transformer in the code below:

import { deepStrictEqual } from "assert";
import { serialize, deserialize } from "class-transformer";
import { AttributeValue } from "@aws-sdk/client-dynamodb";

interface DynamoSerializable {
  toDynamoDB: () => AttributeValue;
}

class UserClass implements DynamoSerializable {
  constructor(private firstName: string, private lastName: string) {}

  toDynamoDB() {
    return { S: serialize(this) };
  }
}

const userFromStr = (str: string) => deserialize(UserClass, str);

const userClassObj = new UserClass("firstName", "lastName");
const serialized = userClassObj.toDynamoDB();
const deserializedObj = userFromStr(serialized.S);

deepStrictEqual(userClassObj, deserializedObj); // doesn't throw AssertionError

EDIT: modified DynamoSerializable.toDynamoDB() to return AttributeValue

@trivikr
Copy link
Member Author

trivikr commented Sep 28, 2020

As originally suggested by @russell-dot-js, since toDynmaoDB() will be called from marshall function which is util-dynamodb, we can enforce returning NativeAttributeValue instead, as follows:

import { deepStrictEqual } from "assert";
import { serialize, deserialize } from "class-transformer";
import { NativeAttributeValue } from "@aws-sdk/util-dynamodb";

interface DynamoSerializable {
  toDynamoDB: () => NativeAttributeValue;
}

class UserClass implements DynamoSerializable {
  constructor(private firstName: string, private lastName: string) {}

  // returns string which is NativeAttributeValue
  toDynamoDB() {
    return serialize(this);
  }
}

const userFromStr = (str: string) => deserialize(UserClass, str);

const userClassObj = new UserClass("firstName", "lastName");
const serialized = userClassObj.toDynamoDB();
const deserializedObj = userFromStr(serialized);

deepStrictEqual(userClassObj, deserializedObj);

@russell-dot-js
Copy link
Contributor

The comment by @russell-dot-js seems to be a better solution where user provides us with serialization function for class objects.

Verified by using serialize and deserialize methods from class-transformer in the code below:

import { deepStrictEqual } from "assert";
import { serialize, deserialize } from "class-transformer";
import { AttributeValue } from "@aws-sdk/client-dynamodb";

interface DynamoSerializable {
  toDynamoDB: () => AttributeValue;
}

class UserClass implements DynamoSerializable {
  constructor(private firstName: string, private lastName: string) {}

  toDynamoDB() {
    return { S: serialize(this) };
  }
}

const userFromStr = (str: string) => deserialize(UserClass, str);

const userClassObj = new UserClass("firstName", "lastName");
const serialized = userClassObj.toDynamoDB();
const deserializedObj = userFromStr(serialized.S);

deepStrictEqual(userClassObj, deserializedObj); // doesn't throw AssertionError

EDIT: modified DynamoSerializable.toDynamoDB() to return AttributeValue

@trivikr I dig it, but just keep in mind that most consumers will not be using ES6 classes for their DTO's, most JS teams tend to use native JS objects over classes. For us, at least, we only use ES6 classes when necessary, e.g. to use Typescript decorators. But we only use them when we must, not because we want to (they're quite despisable IMHO).

I think we can generally solve everything by allowing consumers to pass custom parse/transform functions to the document client, while implementing this interface will solve some problems, it might be a bit niche and redundant with the other solution in place.

@trivikr
Copy link
Member Author

trivikr commented Sep 29, 2020

I think we can generally solve everything by allowing consumers to pass custom parse/transform functions to the document client, while implementing this interface will solve some problems, it might be a bit niche and redundant with the other solution in place.

@russell-dot-js Did you mean something like the premarshall configuration suggested in #1533?

@trivikr
Copy link
Member Author

trivikr commented Oct 1, 2020

Update: we're exploring serializing of ES6 classes and more using premarshalling at #1533 (comment)

@alexforsyth alexforsyth added feature-request New feature or enhancement. May require GitHub community feedback. High Priority needs-triage This issue or PR still needs to be triaged. and removed feature-request New feature or enhancement. May require GitHub community feedback. labels Nov 12, 2020
@trivikr
Copy link
Member Author

trivikr commented Jan 26, 2021

As discussed in #1533 (comment), we'll be adding support to marshall all Objects by passing a configuration (say marshallObjects) in marshall function.

@trivikr trivikr removed the needs-triage This issue or PR still needs to be triaged. label Jan 29, 2021
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants