Increment on embedded document does not return entire document after Parse.Object.save()

[MobileVet]

New Issue Checklist

• [X ] I am not disclosing a vulnerability.
• [X ] I am not just asking a question.
• [X ] I have searched through existing issues.
• [X ] I can reproduce the issue with the latest version of Parse Server.

Issue Description

Although not explicitly stated in the documentation, one is able to increment values in an embedded document. For example, with Metric as our class with the following schema

{
  topLevel: number,
  stats: {
    first: number,
    second: number
  }
}

One can call
metric.increment('stats.first')

This appropriately increments the value at Metric.stats.first

Where the bug shows up is when you save the metric. The returned / updated value of metric does NOT include the rest of the properties in the embedded document stats. Even if you had fetched the ENTIRE object prior to the increment command, the save will ONLY return the updated info of the embedded document property.

Steps to reproduce

• Fetch an object completely w/ all of its properties AND embedded object properties
• Increment an embedded document property
• Save the object

Actual Outcome

• Notice that ALL properties are present EXCEPT embedded document properties that were NOT incremented

Expected Outcome

• The entire object w/ all embedded document properties is returned

My guess is that inside the source code, we are not merging the updated value with a 'spread' version of the prior object properly. This despite having all of the values prior to the increment which is confirmed in the verbose log AND using VS Code to debug in real time.

Environment

Server

• Parse Server version: 4.2.0
• Operating system: Ubuntu & Mac OS 10.15
• Local or remote host (AWS, Azure, Google Cloud, Heroku, Digital Ocean, etc): Heroku & Local

Database

• System (MongoDB or Postgres): MongoDB
• Database version: 3.6.19 & 4.2.9
• Local or remote host (MongoDB Atlas, mLab, AWS, Azure, Google Cloud, etc): MongoDB Atlas

Client

• SDK (iOS, Android, JavaScript, PHP, Unity, etc): Javascript
• SDK version: n/a

Logs

verbose: REQUEST for [POST] /parse/batch: {
  "requests": [
    {
      "method": "PUT",
      "body": {
        "answeredIncorrectlyCount": {
          "__op": "Increment",
          "amount": 1
        },
        "choiceStats.d2": {
          "__op": "Increment",
          "amount": 1
        },
        "ACL": {
          "*": {
            "read": true
          }
        }
      },
      "path": "/parse/classes/GlobalQuestionMetric/myGArd860W"
    }
  ]
}
verbose: RESPONSE from [POST] /parse/batch: {
  "response": [
    {
      "success": {
        "answeredIncorrectlyCount": 6,
        "choiceStats": {
          "d2": 1
        },
        "updatedAt": "2020-09-09T17:18:59.695Z"
      }
    }
  ]
}
verbose: RESPONSE from [POST] /parse/functions/recordQuiz: {
  "response": {
    "result": {
      "globalMetrics": [
        {
          "questionSerial": "12345",
          "answeredCorrectlyCount": 14,
          "createdAt": "2020-07-18T01:02:18.335Z",
          "updatedAt": "2020-09-09T17:18:59.695Z",
          "answeredIncorrectlyCount": 6,
          "choiceStats": {
            "d2": 1
          },
          "ACL": {
            "*": {
              "read": true
            }
          },
          "objectId": "myGArd860W",
          "__type": "Object",
          "className": "GlobalQuestionMetric"
        }
      ]
    }
  }
}
verbose: REQUEST for [GET] /parse/classes/GlobalQuestionMetric: {
  "where": {
    "objectId": {
      "$in": [
        "myGArd860W"
      ]
    }
  },
  "limit": 1
}
verbose: RESPONSE from [GET] /parse/classes/GlobalQuestionMetric: {
  "response": {
    "results": [
      {
        "objectId": "myGArd860W",
        "questionSerial": "12345",
        "answeredCorrectlyCount": 14,
        "createdAt": "2020-07-18T01:02:18.335Z",
        "updatedAt": "2020-09-09T17:18:59.695Z",
        "answeredIncorrectlyCount": 6,
        "choiceStats": {
          "a1": 2,
          "a2": 2,
          "d3": 2,
          "d2": 1
        },
        "ACL": {
          "*": {
            "read": true
          }
        }
      }
    ]
  }
}

[davimacedo]

I am not sure if it is a bug since it is returning, and only returning, the key that was changed and all other elements continue the same. Do you see any bad behavior in the client side because of this?

[MobileVet]

I am not sure if it is a bug since it is returning, and only returning, the key that was changed and all other elements continue the same. Do you see any bad behavior in the client side because of this?

@davimacedo Right, for a straight up save, that is what I would expect. You can see in the log that is happening properly... and as you say, expected.

However, this is a slightly different situation. This is the situation where the instance has already fetched the object. So the complete object is available. In that case, when you increment a regular property (answeredIncorrectlyCount in the log) and then Parse.Object.save() you get back all of the properties again.

I probably should have included the following sudo code

const metrics = Parse.Query('GlobalQuestionMetric).find()
/// At this point ALL properties are fully fetched
metrics.map(metric => metric.increment('answeredCorrectlyCount'))
await Parse.Object.saveAll(metrics)

/// At this point ALL properties are still available, including embedded doc properties (eg choiceStats.a1, choiceStats.a2)
metrics.map(metric => metric.increment('choiceStats.a1'))
await Parse.Object.saveAll(metrics)

/// At this point 'regular' properties are available, but ONLY choiceStats.a1 is included (choiceStats.a2 etc is missing)

Does that make the situation a little more clear?

[davimacedo]

Yes. It is clear. Thanks for the detailed reporting. We need to fix that for sure. Now I am not sure if we should fix that in Parse Server or in the JS SDK though. @dplewis @mtrezza thoughts?

[dplewis]

This maybe an issue with how the JS SDK handles object states internally. I’ll have to look into it, I believe there is a test case for this. I was the one that added increment on nested / embedded documents to the SDK although it wasn’t recommended.

[mtrezza]

Although it may only be somewhat related, I'll reference #6687

[MobileVet]

Yes. It is clear. Thanks for the detailed reporting. We need to fix that for sure. Now I am not sure if we should fix that in Parse Server or in the JS SDK though. @dplewis @mtrezza thoughts?

I had the same thought when filing this... do I put it on this repo or the JS SDK repo?

This maybe an issue with how the JS SDK handles object states internally. I’ll have to look into it, I believe there is a test case for this. I was the one that added increment on nested / embedded documents to the SDK although it wasn’t recommended.

I am very glad you did add this functionality. Mongo supports it natively so I was pleased to see it was exposed by Parse.

Also nice to hear there may be a test already. I will take a look as well, but if you have a particular place in mind @dplewis , please point me in the right direction.

[MobileVet]

@dplewis I THINK the issue is with the _handleSaveResponse method. ParseObject.js:527

At this point, the this variable has the proper values of choiceStats (with all embedded values). I think there are 2 issues.

First, line 541 results in a false condition because the '.' based attr is NOT found in response. Basically it doesn't know how to parse out the embedded document property element. As a result it adds this 'new' attribute property to changes when it shouldn't. For our particular case, I think we would have to be looking at a sliced version of the attr variable.

      } else if (!(attr.slice('.')[0] in response)) {

to avoid adding the embedded document update to changes.

That isn't 'fatal' though, it just tacks on some extra properties to the object.

I think the core issue is within ParseObject.js:571. It appears to be what is blowing away the old object (which is still valid in this at the call site)

stateController.commitServerChanges(this._getStateIdentifier(), changes);

Following a little further down the chain, it appears everything is correct (save for some extra props in changes) at UniqueInstanceStateController.js:264. One step deeper is ObjectStateMutation.js:207. This is where the overwrite happens, on line 215. It doesn't recognize that the change is only a PARTIAL representation of the prior object. You could clearly check for objects here and do a 'spread / merge' with the old values and the changes... however, I think that would break general $set.

I think the real issue is that for the increment operation to work like everything else, it needs to return the ENTIRE document from which it made the internal increment. Otherwise, you run into the above issue with the inability to $set new objects that fully replace old ones. (I could be wrong about that).

Is it possible to adjust that increment operation accordingly? I didn't quite see where that was taking place.

[dplewis]

You can checkout this PR and the conversation. parse-community/Parse-SDK-JS#729

Feel free to write a new test case to replicate your issue.

On the server you can look at the RestWrite.js, I think that returns the object

[MobileVet]

You can checkout this PR and the conversation. parse-community/Parse-SDK-JS#729

Feel free to write a new test case to replicate your issue.

On the server you can look at the RestWrite.js, I think that returns the object

@dplewis Done... though I am a little stymied by the result. My change does fix the issue when I use it in my project... but it doesn't result in passing tests. I think I am missing something simple.

As an aside, I think there are a couple ways to approach this. It may be better to adjust what is returned when the save() happens. If it returned the full updated embedded document, that would almost fix everything with little change. That said, there is an issue where some props are added to the changes element that shouldn't be.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[dplewis]

Closing via https://github.com/parse-community/Parse-SDK-JS/releases/tag/2.19.0