Skip to content

Conversation

@jonathanstokes
Copy link
Contributor

I didn't see any contributor guidelines, but trying this anyway because the issue is a showstopper for us (since the uncaught error crashes Node).

Fixes #74.

Problem

When item.save(callbackFn) is called with invalid input, the server rejects the request with a 400 response, as expected. The client, however, does not call the given callbackFn to provide the error to the caller. Instead, an uncaught error is thrown trying to wrap the null data into a collection.

Not only does this behavior violate the specified contract by not calling the callback function, but the uncaught error can crash Node completely, with no way for an outsider to try/catch the save() call itself, since the error happens in its own callback.

This fix adds unit tests demonstrating the problem, and fixes the problem by only wrapping non-null response data.

To Reproduce

In issue #74 several people have described scenarios that led to this problem. One summary from @JohannesHome in the issue:

As a follow up. I was able to reproduce the issue finally, and the delete was unrelated to my problem but a http error of 4xx group as this issue relates to.

We basically have a situation where we do a object.save(handler) call when nothing was changed in the object. In this case the client is doing a put with an empty object {} as no value has changed, and the server response with 400.

The main issue is, therefore, that save, merge and duplicate do not propagate the error to the handler, but instead try to access data. All these functions generate the same mentioned error in case the server response with anything else besides 200 containing an empty body.

This should be easy to fix.

More specific repro scenarios include trying to save() a Person who has already been deleted (@super-cache-money) and setting an invalid field ID before save() (@vasiabikeru).

Summary

Please consider merging this into the API client to fix what appears to be a very major problem.

When item.save() gets a 400 Bad Request response, it chokes parsing the null `data` value without ever calling the save() callback.
When item.save(callbackFn) is called with invalid input, the server rejects the request with a 400 response, as expected. The client, however, does not call the given callbackFn to provide the error to the caller. Instead, an uncaught error is thrown trying to wrap the `null` data into a collection.

This fixes the problem by only wrapping non-`null` response `data`.
@DenisButCheR DenisButCheR requested a review from a team March 9, 2019 07:38
@DenisButCheR DenisButCheR merged commit cdee877 into pipedrive:master Mar 12, 2019
@jonathanstokes
Copy link
Contributor Author

@DenisButCheR, thanks for approving!

How does the package versioning work? I didn't change the package version from 6.0.3 in this PR, thinking maybe the build system would do that when it was approved. Looks like these changes are in, but still being called 6.0.3, so anyone who already had 6.0.3 won't pick up the change. Should I open a PR to rev to 6.0.4?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

{Object}.save not working

2 participants