awslabs / dynamodb-data-mapper-js

A schema-based data mapper for Amazon DynamoDB.
https://awslabs.github.io/dynamodb-data-mapper-js/
Apache License 2.0
816 stars 106 forks source link

Add tests to ensure ItemNotFoundException inherits from Error #167

Closed JakeChampion closed 3 years ago

JakeChampion commented 4 years ago

Currently if an item does not exist in the dynamodb table an error is thrown but the error that is thrown is not inheriting from the error class they are constructed from. It turns out this is a bug/intential-change in TypeScript. TypeScript have a page on their Wiki which explains how to extend from Error correctly -- https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#extending-built-ins-like-error-array-and-map-may-no-longer-work

import {
    DataMapper,
    DynamoDbSchema,
    DynamoDbTable,
} from '@aws/dynamodb-data-mapper';
import DynamoDB = require('aws-sdk/clients/dynamodb');

const client = new DynamoDB({
    region: 'us-west-2'
});
const mapper = new DataMapper({
    client
});

class MyDomainModel {
    id: string;
}

Object.defineProperties(MyDomainModel.prototype, {
    [DynamoDbTable]: {
        value: 'MyTable'
    },
    [DynamoDbSchema]: {
        value: {
            id: {
                type: 'String',
                keyType: 'HASH'
            },
        },
    },
});

async function main() {
    // delete an object
    const toDelete = new MyDomainModel();
    toDelete.id = 'DELETE_ME';
    // this throws as it's fetching an object that does not exist,
    // the error should be an instance of ItemNotFoundException
    await mapper.get(toDelete)
        .catch(err => console.log(err instanceof ItemNotFoundException)); // <---- This should be true but currently is false
}

main();
sthulb commented 4 years ago

@JakeChampion can you add a description of the changes?

JakeChampion commented 4 years ago

@sthulb I've updated the description

JakeChampion commented 4 years ago

@edermanoel94 I think I have already added tests for this --> https://github.com/awslabs/dynamodb-data-mapper-js/pull/167/files#diff-98df80367daa25bbae8c1e5d43887c9cR23-R32

JakeChampion commented 4 years ago

@jeskew and/or @edermanoel94 is there anything I can do to help this get merged and released?

edermanoel94 commented 4 years ago

@JakeChampion for me its pretty good. i dont have any power here for merge.

JakeChampion commented 4 years ago

I have a feeling this will never be merged

JakeChampion commented 3 years ago

Giving up on this pull-request for now. Feel free to re-open it if you want to include this fix