Add the ability to try and get an item without an exception

[thrixton]

Having to handle an exception (ItemNotFoundException) when a get does not find an item is a difficult pattern and requires liberal nesting or messy try/catch blocks.
Would it be possible to add a tryGet method which instead of throwing an exception, returns null if the search key does not exist?

[jeskew]

mapper.get will return a promise and will only throw an exception if used with await. Instead of using try/catch, you could use promise callbacks:

mapper.get(item)
    .then(foundItem => {})
    .catch(() => {})

[thrixton]

Hi @jeskew , thanks for the reply.
Yes, I could use callbacks, however then I have to nest each call and I have 4 calls I have to handle.
It gets messy very quickly.
As well as that, I have to check in each call if the error is a ItemNotFoundException, duplicating code each time.

I'm currently using a helper function, but it would be great to have it as a "native" option.

export async function tryGetAsync<T>(obj: T, keys: {}): Promise<T> {
  return new Promise<T>(async (resolve, reject) => {
    const mapper = getMapper();
    try {
      const val = await mapper.get(Object.assign(obj, keys));
      resolve(val);
    } catch (error) {
      if (error.name === 'ItemNotFoundException') {
        resolve(null);
      } else {
        reject(error);
      }
    }
  });
}

[jeskew]

I understand the desire to avoid having to check the error name in the failure case callback, but the proposed tryGet would force callers to check whether the return was undefined in the success case. Essentially, you would be trading this:

mapper.get(item)
    .then(foundItem => { /* foundItem is always the object sought */ })
    .catch(err => {
        if (err.name === 'ItemNotFoundException') {
            // ...
        } else {
            throw err; // re-reject the promise
        }
    })

for this:

mapper.get(item)
    .then(foundItem => {
        if (!foundItem) {
            throw new Error("Item not found");
        } else {
            // the item was found
        }
    })

[thrixton]

In a simple case, you're correct.
However when I have 4 tables that I have to query, I branch logic based on if one key is not found and then don't care if the rest are not found until later, when I'd have to check them anyway.

To my mind, the below is clearer than than the alternative.

It also removes the need for the magic ItemNotFoundException string.

item = await mapper.tryGet(item);
if (item) {
  okObjectResult(item, cb);
} else {
  notFoundResult(cb);
}

vs

try {
  item = await mapper.get(item);
  okObjectResult(item, cb);
} catch (error) {
  if (error.name === 'ItemNotFoundException') {
    notFoundResult(cb);
  }
}

This is a very simple example, when it gets more complex, the benefits are multiplied.
It may just be my way of thinking, but I don't think of an item not being found as an exception case, and exceptions should be reserved for exception cases, my 2c only, other opinions obviously differ :)

Cheers,
John

[jeskew]

I think the use of await and try/catch is greatly complicating the examples provided. Think of the promise being returned from mapper.get as representing a binary decision tree similar to a Java Optional or a Haskell Maybe monad: it either contains the result requested or it doesn't. You just need to register callbacks for each of those branches:

mapper.get(item)
   .then(okObjectResult)
   .catch(itemLookupFailed);

If you always want to handle one specific error condition differently, the callback passed to catch can inspect the error:

const notFoundName = 'ItemNotFoundException';

function itemLookupFailed(err: Error) {
    if (err.name === notFoundName) {
        // handle the item requested not being in the table
    } else {
        // handle other, unforeseen errors
    }
}

The fact that a resolved promise will always contain the requested object makes them easy to compose:

const [item1, item2, item3, item4] = await Promise.all([
    mapper.get(item1Keys)
       .catch(itemLookupFailed),
    mapper.get(item2Keys)
       .catch(itemLookupFailed),
    mapper.get(item3Keys)
       .catch(itemLookupFailed),
    mapper.get(item4Keys)
       .catch(itemLookupFailed)
]);

[damonmaria]

I agree with the OP regarding this. Especially having to deal with a string ('ItemNotFoundException ') to perform the check. That DynamoDB and DocumentClient throw no error with getItem / get and return null (or undefined ?) for Item when it's not found is useful in many situations.

[denver-HJS]

I agree with @jeskew on this matter. The use case is exactly what Promise.all is meant for. In the more simple cases having a discrete error to expect allows for clearer error handling imo.

[ktachev]

I think get should either return Something or null. Get is not mutation thus will throw Error only if we don't have a connection with Dynamo or something else is broken inside get method runtime. If logic is correct you should get Object or null and nothing else. This is my opinion.