search

mapbox / dyno

simple dynamodb client
MIT License
78 stars 31 forks source link

Requests #125

Closed JustAboutJeff closed 5 years ago

JustAboutJeff commented 7 years ago

Thanks for creating dyno !

The error handling for sendCompletely first attempts to check if the result object includes a Responses object of any value prior to invoking the callback. However, in cases where this property is not present, such as with a ValidationException error, this check results in it's own uncaught TypeError and fails to ever invoke the callback.

This change allows for a safer Responses check so that an error may be passed back to the caller as expected.

rclark commented 7 years ago

cc @mcwhittemore -- We've been talking about restructuring the code more dramatically here. Do these changes fall in line with where we'd like to get to?

I believe test failures represent the PRs-don't-get-credentials issue on Travis.

mcwhittemore commented 7 years ago

@rclark - I think this is a good first step before we refactor requests.

@JustAboutJeff - can you write a test that covers this use case. Pretty soon we want to clean up the requests code a bunch and a test would help make sure this doesn't regress.

JustAboutJeff commented 7 years ago

@mcwhittemore thanks for the response! I took a stab at writing a test to capture this scenario within the established code style and format.

mcwhittemore commented 7 years ago

Thanks! I just cloned your PR to get the tests to run.

JustAboutJeff commented 7 years ago

@mcwhittemore took another pass at the spec to better capture the "error only" callback scenario the fix addresses.

mcwhittemore commented 7 years ago

@JustAboutJeff and @rclark - why not just drop this line? https://github.com/mapbox/dyno/pull/127/files#diff-14b9fbb5b6f7e8905b1fd8b9d01c5946L221

mcwhittemore commented 7 years ago

@rclark - wanna look at #127. I think it solves the problems @JustAboutJeff brought up

Trying to match responses from the batchWriteAll and batchGetAll functions to the AWS docs proves hard as their docs indicate that requests with an error will have no data and the act of merging those responses means we have to have both an error and data. This exposes some problems with these functions. @rclark has brought this up before. My thought is to merge this PR and then look at ripping out batchWriteAll and batchGetAll.