fix: user object may contain already deleted fields

[rdhelms]

New Pull Request Checklist

• I am not disclosing a vulnerability.
• I am creating this PR in reference to an issue.

Issue Description

As discussed in #1347, the return from Parse.User.logIn can include fields that no longer exist on the user, even if the network response from parse-server does not include the removed fields. There is also an issue with the return from Parse.Cloud.run.

Related issue: #1347

Approach

This PR is a work in progress, initially containing only a failing test for the Parse.User.logIn case. If the failing test is confirmed to represent the issue well, a failing test for Parse.Cloud.run can also be added.

No solution for the Parse.User.logIn case has yet been proposed.
The proposed solution for the Parse.Cloud.run case is to update one of the cases in decode.js to call ParseObject.fromJSON(value, true) instead of ParseObject.fromJSON(value).

TODOs before merging

• Add failing test for Parse.Cloud.run case
• Add failing test for Parse.User.logIn case
• Fix Parse.Cloud.run case
• Fix Parse.User.logIn case
• Add entry to changelog

[parse-github-assistant]

Thanks for opening this pull request!

• 🎉 We are excited about your hands-on contribution!

[mtrezza]

Thanks for the test! Do you think you could rewrite this in async/await? We are slowly getting rid of promise chains in tests.

[rdhelms]

@mtrezza Rewrote using async/await 👍 Happy to make any other adjustments. I also started trying to write a failing test in Cloud-test.js, but for some reason the attributes of my mock response aren't getting processed as I expected...any suggestions here?

    CoreManager.setRESTController({
      request (method, path) {
        expect(path).toBe('functions/myfunction')

        return Promise.resolve(
          {
            result: {
              objectId: 'abc123',
              className: 'Item',
              __type: 'Object',
              createdAt: '2015-01-01T00:00:00.000Z',
              updatedAt: '2015-01-01T00:00:00.000Z',
              label: 'foobar',
            },
          },
          200
        );
      },
      ajax () {},
    });

    Cloud.run('myfunction').then(response => {
      expect(response.get('label')).toBe('foobar')
    });

Giving this error during npm test:

And I'm seeing this in the debugger for response:

[rdhelms]

@mtrezza I finished creating the failing test for the Cloud.run case that @aforemny identified in #1347, so both of the identified cases now have failing tests. Is this enough info for you guys to start working on a solution?

A couple notes:

• The issue with my previous attempt to write the Cloud.run failing test was that the test was running through too much mocked logic, so I had to add more jest.dontMock statements to the top of the file. This made me wonder if more of the test files should have those jest.dontMock statements (or maybe adjusting jest's automock behavior) so that it's easier for contributors to add new tests (and so that more of Parse's actual logic is tested)
• The comment from @aforemny about the Cloud.run issue only happening with SingleInstanceController can be verified by removing ParseObject.enableSingleInstance() from Cloud-test.js. The test case only fails when the single instance controller is enabled.

Here are the failing tests:

Parse.User.logIn case

FAIL src/__tests__/ParseUser-test.js (13.[23](https://github.com/parse-community/Parse-SDK-JS/runs/5055265717?check_suite_focus=true#step:9:23)6s)
  ● ParseUser › user who logs out, deletes attribute, then logs back in has correct attributes

    expect(received).toBe(expected) // Object.is equality

    Expected: undefined
    Received: "This field is returned in the first login but not the second"

      993 |     expect(user2.id).toBe('uid2');
      994 |     expect(user2.get('username')).toBe('username');
    > 995 |     expect(user2.get('fieldToBeDeleted')).toBe(undefined); // Failing test PR #1442
          |                                           ^
      996 |   });
      997 |
      998 |   it('can retreive a user with sessionToken (me)', async () => {

      at Object.<anonymous> (src/__tests__/ParseUser-test.js:995:43)

Parse.Cloud.run case

FAIL src/__tests__/Cloud-test.js
  ● CloudController › run same function twice with different responses

    expect(received).toBe(expected) // Object.is equality

    Expected: undefined
    Received: "foobar"

      263 |     const response2 = await Cloud.run('myfunction');
      264 |     expect(response2.get('label2')).toBe('control to confirm correct mock usage');
    > 265 |     expect(response2.get('label')).toBe(undefined); // Failing test PR #1442
          |                                    ^
      266 |   });
      267 |
      268 |   it('startJob passes encoded requests', () => {

      at Object.<anonymous> (src/__tests__/Cloud-test.js:265:36)

[mtrezza]

Thanks! While writing the tests, did you also identify possible solutions for the fix?

[rdhelms]

@mtrezza The most straightforward "solution" that comes to mind is simply disabling the Single Instance Controller. It seems to me that there may be a fundamental flaw in the Single Instance Controller's assumption that its objectState is reliable. It's completely possible that data could have been changed in Mongo in such a way that the Single Instance Controller's state is stale. And if that's the case, then I'm not sure how we can ever use the Single Instance Controller reliably.

But I don't know the context behind the creation and intended use case of the Single Instance Controller, and I don't know how significant the potential benefits are. If it has demonstrable value, then perhaps we can figure out a way to be smarter about clearing the state in the particular cases that we're dealing with here (Parse.Cloud.run and Parse.User.logIn).

If I don't enable the Single Instance Controller in the Cloud and ParseUser tests, the only thing that fails is the test about maintaining the session token when refetched. If that behavior is essential, then we'd need to find a way to still use the Single Instance Controller when it's needed, while resetting the state whenever there's a chance the controller's state is stale.

 FAIL  src/__tests__/ParseUser-test.js
  ● ParseUser › maintains the session token when refetched

    expect(received).toBe(expected) // Object.is equality

    Expected: "abc141"
    Received: undefined

      1286 |       );
      1287 |       expect(u.getSessionToken()).toBe('abc141');
    > 1288 |       expect(u2.getSessionToken()).toBe('abc141');
           |                                    ^
      1289 |       expect(u.get('number')).toBe(undefined);
      1290 |       expect(u2.get('number')).toBe(undefined);
      1291 |       done();

      at src/__tests__/ParseUser-test.js:1288:36

[mtrezza]

It's completely possible that data could have been changed in Mongo in such a way that the Single Instance Controller's state is stale.

What could that be for example?

[rdhelms]

@mtrezza In our case, we noticed that someone using our web app and mobile app at the same time could end up in a bad state. The web app was fetching the correct data from the server, but the web client was still using stale data that had been removed via the mobile app.

But, in general, any direct interaction with the data in MongoDB will result in the Single Instance Controller's state being out of date.

[rdhelms]

@mtrezza As an example of what I'm talking about, I just pushed a commit with all the unit tests passing where I call ParseObject._clearAllState() in Cloud.run, and user._clearServerData() in User.logIn. But I'm not sure if that defeats the general intent behind the Single Instance controller.

[rdhelms]

@mtrezza It looks like clearing the data as I mentioned causes a couple twitter/facebook flows to fail in the integration tests. If you or anyone else has any suggestions for ways to update the Single Instance controller to be more reliable, I'm happy to make more updates here. But in the short term on our end, we're probably going to disable the Single Instance controller.

[rdhelms]

@mtrezza It turns out that disabling single instance mode has some pretty terrible side effects when it comes to how Parse updates the current user in local storage...so I think we're going to need to continue trying a solution here within the SDK. I can continue investigating when I have the time, but any assistance would be greatly appreciated.

It would also be incredibly helpful if the impact of enabling or disabling single instance mode could be better documented.

[mtrezza]

Maybe you could take a look at how single instance is used in code and what effects it has to suggest an amendment to the docs.

[codecov]

Codecov Report

Merging #1442 (47d47ff) into alpha (fd22644) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##            alpha    #1442   +/-   ##
=======================================
  Coverage   99.94%   99.94%           
=======================================
  Files          61       61           
  Lines        5948     5950    +2     
  Branches     1354     1355    +1     
=======================================
+ Hits         5945     5947    +2     
  Misses          3        3           

Impacted Files	Coverage Δ
src/ParseObject.js	99.89% <100.00%> (+<0.01%)	⬆️
src/ParseUser.js	99.57% <100.00%> (ø)
src/decode.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd22644...47d47ff. Read the comment docs.