shiftcode / dynamo-easy

DynamoDB client for NodeJS and browser with a fluent api to build requests. We take care of the type mapping between JS and DynamoDB, customizable trough typescript decorators.
https://shiftcode.github.io/dynamo-easy/
MIT License
204 stars 27 forks source link

TransactPut fails when GSIPartitionKey is defined as "string | null" and a value is used for this property. #281

Closed sans-ericstewart closed 4 years ago

sans-ericstewart commented 4 years ago

Describe the Bug When a GSI Partition Key is declared as string or null, put transactions fail when the value for the GSI Partition Key is a string. The put transaction does not fail when it's value is null.

This situation occurs when you want to use a sparse index on the GSI. In this case, we declare the GSI Partition Key to be string or null.

Code to Reproduce the Issue

// ===== DEPENDENCIES =====

// ----- External -----

import "reflect-metadata";
import { ClientConfiguration } from "aws-sdk/clients/dynamodb";
import { DynamoDB } from "aws-sdk";
import {
  GSIPartitionKey,
  GSISortKey,
  Model,
  PartitionKey,
  attribute,
  TransactPut,
  TransactWriteRequest,
} from "@shiftcoders/dynamo-easy";

// ===== SOURCE =====

// ----- Configuration -----

const clientConfig: ClientConfiguration = {
  endpoint: "http://localhost:8000",
  region: "us-east-1",
};

// ----- Account Model -----

@Model({ tableName: "dynamo_easy_test" })
export class Account {
  createdAt!: Date;
  @GSIPartitionKey("MasterAccountIdIndex")
  masterAccountId!: string | null;
  name!: string;
  @PartitionKey()
  @GSISortKey("MasterAccountIdIndex")
  pk!: string;
  updatedAt!: Date;
}

const transaction = new TransactWriteRequest(new DynamoDB(clientConfig));

// ----- Model Data -----

const account1: Account = {
  createdAt: new Date("2019-01-15T00:00:00Z"),
  masterAccountId: null,
  name: "Beacon Academy",
  pk: "725550fb-24b6-4e26-99ab-90281595bfff",
  updatedAt: new Date("2019-03-10T00:00:00Z"),
};

const account2: Account = {
  createdAt: new Date("2019-01-15T00:00:00Z"),
  masterAccountId: "725550fb-24b6-4e26-99ab-90281595bfff",
  name: "Team RWBY",
  pk: "495358bb-01fd-430d-8ffb-535aef513429",
  updatedAt: new Date("2019-03-10T00:00:00Z"),
};

// ----- Run -----

const pkExistsCondition = attribute("pk").attributeExists();

try {
  const put1 = new TransactPut(Account, account1).onlyIf(pkExistsCondition);
  const put2 = new TransactPut(Account, account2).onlyIf(pkExistsCondition);
} catch (error) {
  console.log(error);
}

Expected behavior Both transactions should be assembled and be ready to execute with transaction writer. However, the second put transaction fails with the following error. Curiously, the first transaction is assembled without issue.

TypeError: Cannot read property 'properties' of undefined
    at Metadata.forProperty (/Users/estewart/Developer/Learning/DynamoDB/DynamoEasyIssue/node_modules/@shiftcoders/dynamo-easy/dist/decorator/metadata/metadata.js:24:31)
    at /Users/estewart/Developer/Learning/DynamoDB/DynamoEasyIssue/node_modules/@shiftcoders/dynamo-easy/dist/mapper/mapper.js:64:101
    at Array.forEach (<anonymous>)
    at Object.toDb (/Users/estewart/Developer/Learning/DynamoDB/DynamoEasyIssue/node_modules/@shiftcoders/dynamo-easy/dist/mapper/mapper.js:49:19)
    at Object.objectToDb [as toDb] (/Users/estewart/Developer/Learning/DynamoDB/DynamoEasyIssue/node_modules/@shiftcoders/dynamo-easy/dist/mapper/for-type/object.mapper.js:20:26)
    at toDbOne (/Users/estewart/Developer/Learning/DynamoDB/DynamoEasyIssue/node_modules/@shiftcoders/dynamo-easy/dist/mapper/mapper.js:103:18)
    at /Users/estewart/Developer/Learning/DynamoDB/DynamoEasyIssue/node_modules/@shiftcoders/dynamo-easy/dist/mapper/mapper.js:72:38
    at Array.forEach (<anonymous>)
    at Object.toDb (/Users/estewart/Developer/Learning/DynamoDB/DynamoEasyIssue/node_modules/@shiftcoders/dynamo-easy/dist/mapper/mapper.js:49:19)
    at new TransactPut (/Users/estewart/Developer/Learning/DynamoDB/DynamoEasyIssue/node_modules/@shiftcoders/dynamo-easy/dist/dynamo/transactwrite/transact-put.js:12:37)

Desktop (please complete the following information):

Package:

{
  "name": "dynamo-easy-issue",
  "version": "0.0.1",
  "description": "Proof of concept for an issue with GSI Primary keys that allow null",
  "main": "src/index.ts",
  "author": "Eric Lee Stewart",
  "license": "ISC",
  "scripts": {
    "build": "tsc",
    "run": "node dist/app/index.js",
    "transpile": "tsc"
  },
  "dependencies": {
    "@shiftcoders/dynamo-easy": "^5.6.0",
    "aws-sdk": "^2.637.0",
    "reflect-metadata": "^0.1.13",
    "tslib": "^1.11.1",
    "uuid": "^7.0.2"
  },
  "devDependencies": {
    "@types/node": "^13.9.0",
    "@types/uuid": "^7.0.0",
    "typescript": "^3.8.3"
  }
}
simonmumenthaler commented 4 years ago

Hi @sans-ericstewart

Thanks for the detailed issue description.

Basically it is not a bug but somehow an expectable behaviour. dynamo-easy relies on typescript reflection information design:type to skip type checks on values at runtime. When defining masterAccountId: string | null the emitted design type is Object and not String --> this results in dynamo-easy trying to treat your string value like an object and therefore it fails. The null value on the other hand is ignored and won't be sent to dynamoDB since this is the default behaviour of dynamo-easy.

Unfortunately dynamo-easy does not yet support null values for properties.

My first idea was to handle your case by simply creating a custom mapper, but all null or undefined values are filtered out before the mapper is called. So this won't work as well.

tl;dr

simonmumenthaler commented 4 years ago

I opened the issue #285 for the feature request.