Parse-server: beforeSave response incomplete when incremented to 0

[rihadavid]

When I increment number in beforeSave and the result value is 0, the value is not returned in the saved object.

STEPS TO REPRODUCE:

Let's create MyObject class with points and num number columns. Manually create a MyObject instance that has 10 in points column and 10 in num column. And define those functions in cloud code:

Parse.Cloud.define("myFunction", function (request, response) {
    var MyObject = Parse.Object.extend("MyObject");
    var myObject = new MyObject();
    myObject.id = request.params.objectId;
    myObject.save(null).then(
        function (savedObject) {
            console.log("points: " + savedObject.get("points")); //this is undefined
            console.log("num: " + savedObject.get("num")); //this is 1
            response.success("success");
        });
});

Parse.Cloud.beforeSave("MyObject", function (request, response) {
    var objectToSave = request.object;
    if (!request.master) {
        objectToSave.increment("points", -10);
        objectToSave.increment("num", -9);
    }
    response.success();
});

Now call myFunction with objectId of the MyObject instance in parameter objectId. When the function performs the save, number columns will be incremented in beforeSave, points by -10 and num by -9. So now points will be 0 and num will be 1.
The savedObject in myFunction should contain the updated points value and the updated num value. But because the updated (incremented) numeric value of the field is 0, savedObject.get("points") is undefined. All other values in response, like the num that is 1, are contained in the savedObject. If the points result value would be different than 0, like if I had 20 points and increment -10, I get the points value in the savedObject as expected.

IMPORTANT:

1) If you create new MyObject inside myFunction, set points and num to 10 and then call save, calling the beforeSave, it works as expected, the issue doesn't happen.
2) If you use objectToSave.set("points", 0); instead of increment to 0 in beforeSave, it works as expected, the issue doesn't happen.

We have many functions that rely on this - user buys some stuff for virtual currency and if it costs exactly as the money he has, his balance will not be updated in the UI. For now, we are using an ugly workaround.

Tested on ParseServer 2.6.2, 2.2.18, 2.2.19.

[BorutPfeifer]

I have a very similar sounding issue around updating to a 0 value, that I wonder if it might be related. The difference is, while I do have a beforeSave function implemented, it's not the one changing the value of the field, but just trying to update a field to 0 gets ignored.

First I created the object with a value in a particular field as -1, it goes through beforeSave, and then later I try to update it to 0 with a subsequent request. The request goes out correctly with 0 as the value, but the value in the log of the field going into the subsequent beforeSave call is still -1.

[flovilmart]

Would you have some sample code to provide that has that behavior? That would help crafting a test as well as providing a solution

[BorutPfeifer]

Ah, actually I figured it out (probably not the same thing) - the entire update of the object I was sending didn't go through, because I didn't specify the contentType as "application/json" on the PUT. Weirdly, the creation of the object goes through fine without the contentType set (I was only setting contentType on POST, not PUT). And the logging made it look like the old object record was being passed in. Worth noting, this behavior worked fine under Parse.com (in both PUT cases, the create and the update).

[natanrolnik]

@rihadavid are you still having this issue or can we close it?

[montymxb]

Closing as noone has followed up with this yet. If there's still an issue here let us know and we'll open this back up!

[rihadavid]

@natanrolnik @montymxb @flovilmart
Please reopen. I've just tested this in the latest Parse 2.6.2 and the issue is still present. I also updated the initial post, with easy steps to reproduce. Hope it helps. Thanks

[rihadavid]

@natanrolnik @montymxb @flovilmart
Just reminding that this should be reopened.

[montymxb]

Apologies @rihadavid ! Didn't see the intial request to have this reopened. Thanks for adding details regarding the causes (and conditions where this does not happen). I'll run this personally and see if I can't verify your case.

[montymxb]

@rihadavid using the details you provided I have been able to replicate this issue. However it looks to be localized in the representation of the given value in JS, not in the actual object in the DB. If you run something like this in your cloud function (modified from your example)...

var message = "::before::\n";
message+= "points: " + savedObject.get("points")+"\n";
message+= "num: " + savedObject.get("num")+"\n";

savedObject.fetch().then(function(fetchedObject) {
  message+= "::after::\n";
  message+= "points: " + fetchedObject.get("points")+"\n";
  message+= "num: " + fetchedObject.get("num")+"\n";
  response.success(message);
});

you would get a response that would show both a before and after state of the same object, with the after state showing the correct value of 0 instead of undefined.

I personally ran this using the php sdk (my own preference) just to setup the object, run the cloud function, and then re-query to check the object again for it's expected values post-modification. On the client side everything looks ok 👍 (even with the misrepresented value in cloud code) leading me to believe it's just an issue there.

I'll take a look and see exactly why undefined is the result of this increment operation, but you should be able to rest easy knowing that the underlying data is all good.

[rihadavid]

@montymxb yes, the data in DB are ok, but the problem is that we are using this response.success(savedObject); as the result of the function - and the client side receives the incomplete object. Yes, we could fetch the object as a workaround, but people don't expect this to happen, since it works in all other cases, just not with incrementation to zero.
Thanks for looking at the issue!

[montymxb]

No problem, sorry to have you waiting on this if anything! I'll let you know what I find on the JS side of things.

[montymxb]

@rihadavid I took some time to dig in and haven't been able to _quite_ pinpoint the underlying issue yet. Just letting you know this is still on the table.

[montymxb]

@rihadavid so I believe I have found the issue. Looks to be an issue with the following line in RestWrite.js.

const dataValue = data[fieldName];
const responseValue = response[fieldName];

response[fieldName] = responseValue || dataValue; // <-- line in question

// Strips operations from responses
if (response[fieldName] && response[fieldName].__op) {
  delete response[fieldName];
  if (clientSupportsDelete && dataValue.__op == 'Delete') {
    response[fieldName] = dataValue;
  }
}

In the case where you have a beforeSave hook that decremented a numeric value by -10 where the existing value was 10, the value of responseValue would be 0. Given that it would evaluate to false and would defer to using dataValue in the response. The thing is that since the data value has an __op property (part of how increment works) it is _immediately_ stripped from the response in delete response[fieldName];, right after being set!

I have a local test case that I'll put into a PR, along with the patch. Honestly it's a pretty basic fix if you consider that (of the possible __ops) it's basically just increment that could pass a false condition with it's result. So all we really need to guard against is a zero condition.

if(responseValue || responseValue === 0) {
  response[fieldName] = responseValue;
} else {
  response[fieldName] = dataValue;
}

The above would go in place of the problematic line. Running tests locally indicates everything looks good, but we'll see what the PR resolves to. In the meantime you can always self-patch your instance up until this comes out in a release.

[montymxb]

This should be taken care of now with the recent merge. Thanks again for helping us out with this!