save call on object with beforeSave trigger removes the data of pointer fields

[sud80]

• You've met the prerequisites.
• You're running the latest version of Parse Server.
• You've searched through existing issues. Chances are that your issue has been reported or resolved before.

Environment Setup

nodejs: 4.4.0
parse-server: 2.2.2

Description of the issue

When the beforeSave trigger for an object is present, the save call overwrites the pointer fields with only the object id even thou' the pointer field had fetched object before calling save.

Steps to reproduce

new Parse.Query('Person')
  .first()
  .then(person => {    
    console.log(person.createdAt, person.get('name')); // prints correctly
    const account = new Parse.Object('Account', {person});
    console.log(account.get('person').createdAt, account.get('person').get('name')); // prints correctly
    return account.save();
  })
  .then(account => {
    // prints correctly only if there are no beforeSave triggers.
    console.log(account.get('person').createdAt, account.get('person').get('name')); 
  });

For the above, I would ideally expect the console.log to print both person's createdAt and name in all the three logs. But the last console.log prints the values correctly only if beforeSave trigger is not present. If beforeSave trigger present, then it just contains the pointer value and loses the object data.

Logs/Trace

[flovilmart]

is it a duplicate from #1288?

[sud80]

Hi, These are two different issues.
#1288 is about when a object is created and a pointer field is added in beforeSave, the pointer field is dropped as it is not in schema.
This particular issue is different one. When a pointer field(let us say fieldB) of a object A is set to a fetched object (objectB) on client side, the save function doesn't preserve the fetched objectB. It has only pointer values. The example I have put above should make it clear. Let me know if it is not clear.

[flovilmart]

can you update to 2.2.5 and confirm that the issue is still here please? I'll try to add a test case when you confirm and work towards a fix. Also did it occur on parse.com?

[sud80]

@flovilmart Thanks for looking in to this. The same issue still happens in latest version 2.2.6 as well. In parse.com it works fine (tried with sdk version 1.6.14).

[sud80]

@flovilmart any updates on this. Currently this is blocking our migration.

[guptasachin25]

any updates on this?

[sud80]

@flovilmart any updates on this. This has been pending for quite sometime. Can you let us know.

[airdrummingfool]

I just ran into this on Parse-Server 2.2.9 - it was working correctly for me on hosted Parse SDK 1.5.0. I wrote a test to confirm the behavior, it is PR #1776.

[airdrummingfool]

This has been biting me quite a bit, so I attempted to track it down - with only partial success.

The issue can be traced to code included twice in RestWrite.js (roughly lines 733-738 and 764 -769):

if (this.storage['changedByTrigger']) {
  resp = Object.keys(this.data).reduce((memo, key) => {
    memo[key] = resp[key] || this.data[key];
    return memo;
  }, resp);
}

added by @flovilmart in e1c4755. Obviously this code is important and cannot simply be excised, however disabling it does allow the my test in #1776 to pass (though it breaks a few other tests along the way).

I tried changing RestWrite.js so that changedByTrigger was only set if the object was actually modified in the beforeSave: this fixed simple cases like my test (and didn't break any other tests), but it obviously doesn't fix non-trivial uses of beforeSave (i.e. a beforeSave that actually does something).

This code affects the way the local Parse Client commits server responses: since the server response now includes child object keys (as pointers), the Parse Client likely overwrites the local child objects completely instead of merging (although I'm not sure merging would be an effective strategy).

And that's where I'm at now. I'm hoping this is useful to someone who is a bit more familiar with the internals and the client/server interaction.

[drew-gross]

Wow great detective work! I'll take a closer look in the morning, in the meantime if you want to keep digging, the way I usually work on bugs is to just run that one test (by changing it to fit) then run tests like VERBOSE=1 npm test to see each request and response. Usually that is enough for me to see the bug but if not, you can then use those same requests on an app on Parse.com with different keys and URL and compare the responses.

[airdrummingfool]

@drew-gross I logged the requests and responses as you suggested, and it appears that the issue is simply that the server response now contains a key for the child object (in pointer form). Here are the raw server responses from the final save in my test, first from SDK 1.5.0 on Hosted Parse, and then again using Parse-Server 2.2.10 (both requests were sent from the same test code, using client SDK version 1.8.1):

Hosted Parse, Server SDK 1.5.0

{ createdAt: '2016-05-19T08:34:35.741Z',
  objectId: '61QhOtURrj' }

Local Parse-Server, v2.2.10

{ objectId: 'gcBNk4qu3U',
  createdAt: '2016-05-19T08:40:31.863Z',
  aTestObject:
   { __type: 'Pointer',
     className: 'TestObject',
     objectId: 'nSXHOfKhKH' },
  updatedAt: '2016-05-19T08:40:31.863Z' }

It seems like in general, the goal is for the server to only return data that has changed. Based on that, we shouldn't need to return a key for a child object if we didn't do anything with the child object in the beforeSave. Unfortunately, at the place where the child keys are being added to the response, there's no way to tell if a given key is an existing object or a new one. We could /almost/ just leave out all pointers, but this would break the ability to add child objects in beforeSave (i.e. #1288).

Based on that, it seems like this could be solved client-side during the server response merge: if a child object pointer in the response exists on the local object (and the object IDs match), don't merge the pointer over the local object's (potentially fetched) child. If the IDs don't match or it doesn't already exist, copy it into place. If the child object is returned as a delete op, then delete the local child object. I believe that covers all of the scenarios, however this would necessitate an update to every client SDK (ew).

[drew-gross]

Yep, our goal should be to make Parse Server behave the same as Parse.com. This sounds like a tricky issue, if you want to help with a fix, the easiest way would be to write a test case that fails reliably that we can add to our test suite. I can give you some pointers on how to do that if you are interested.

[airdrummingfool]

@drew-gross I wrote one already and submitted a PR, see #1776.

[drew-gross]

Ah yes thats right. I forgot about that, sorry.

[airdrummingfool]

@drew-gross No worries :)

I've now played around extensively with client-triggered beforeSaves in Hosted Parse 1.5.0 & 1.6.0, and Parse-Server 2.2.10. It appears that, in every version, a non-trivial beforeSave (i.e. a beforeSave that makes any change to the object) causes the client to clobber an existing child object with a pointer. It is worth noting that in a trivial beforeSave, HP 1.5.0/1.6.0+ will not cause a client to overwrite a fetched child object, while Parse-Server 2.2.10 will.

However, the behavior gets a bit fuzzier when testing a Cloud Code function that creates an object and triggers a beforeSave (i.e. none of the object creation work is done on the client). This is the situation that I rely on right now.

It looks like this behavior changed between Hosted Parse SDK versions 1.5.0 and 1.6.0+. I have a Cloud Code function that creates a new object (made up of multiple pre-existing, fetched child objects), saves it, and then returns it. This new object is of a class that has a non-trivial beforeSave function (the beforeSave modifies the object by adding multiple properties).

• In the 1.5.0 code I run currently, I can run the Cloud Code function and the response includes fully hydrated child objects (they are fetched in the Cloud Code function before creating the new object).
• Running the exact same function in 1.6.0 returns a new object that is comprised entirely of pointers, i.e. none of the child objects are hydrated. If I disable the new object class' beforeSave (or just return immediately from it) the child objects are returned hydrated; disabling the beforeSave is not necessary in 1.5.0.

All of this leads me to the following conclusions:

1. given that only trivial beforeSaves seem to preserve client-side fetched child objects, and the goal is to behave like Parse.com, this bug report is fairly limited in scope, and my test case accurately reproduces the issue in its entirety.
2. The actual issue that affects me (not sure about @sud80 or @guptasachin25) involves Cloud Code/beforeSave interaction behavior that changed in Hosted Parse 1.6.0. Since the goal is to behave like Parse.com (and I'm assuming the newer SDK versions), then that specific issue likely is a wontfix, which is unfortunate but understandable. Is it appropriate to add it in as a feature request, or is "behave like Parse.com" a hard rule? Even if it were allowable it seems unlikely to be changed in the short term; after inspecting the code I don't see a clear path to a fix.

[drew-gross]

Yes, 1.6.0 had a log of changes. If you have discovered a bug in the sdk, please post an issue in it's repo.

Behaving like Parse.com is pretty much a hard rule in terms of the API. The goal is for people to be able to migrate their app off of Parse with as few changes as possible. In some cases, like Parse.Cloud.useMasterKey(), that's just not feasible, but wherever it is feasible, we would like to make that possible.

[airdrummingfool]

I was just assuming the change in Cloud Code/beforeSave behavior from 1.5.0 to 1.6.0 was intentional (or a side effect of something intentional); are you suggesting that it could be a bug in the JS SDK? Actually, now that I look around a bit, it appears that parse-community/Parse-SDK-JS#91 and parse-community/Parse-SDK-JS#251 are existing reports (mentioning them here for reference). I'll continue the Cloud Code/beforeSave behavior conversation in one of those.

[drew-gross]

Fixed by #1884