Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Parse.Object.saveEventually fails if server is restarting while calling it #2090

Closed
wants to merge 3 commits into from
Closed

fix: Parse.Object.saveEventually fails if server is restarting while calling it #2090

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3510aa7
uses save json instead of toJSON to avoid ACL object being parsed bac…
mortenmo Mar 16, 2024
4d871ff
missing mock function
mortenmo Mar 17, 2024
35da171
Merge branch 'alpha' into fix-eventually-queue-acl-issue
mtrezza Mar 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/EventuallyQueue.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ const EventuallyQueue = {
queueData[index] = {
queueId,
action,
object: object.toJSON(),
object: object._getSaveJSON(),
Copy link
Member

@dplewis dplewis Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mortenmo Thanks for the PR! I think the validation check on ACL should removed instead of removing ACL from Eventually Queue. I honestly don't know why the ACL check is there. .validate is called in both save and set , save also calls set. set transform the ACL json into Parse.ACL (which has it own checks) then performs the .validate then throws the error.

The following integration test fails.

it('can set ACL from toJSON', async () => {
    Parse.User.enableUnsafeCurrentUser();
    const user = new Parse.User();
    const object = new TestObject();
    user.set('username', 'torn');
    user.set('password', 'acl');
    await user.signUp();
    const acl = new Parse.ACL(user);
    object.setACL(acl);
    const json = object.toJSON();
    await object.save(json); // This breaks ACL must be a Parse.ACL.
 });

Here is an integration test replicate the issue for this PR.

it('can saveEventually on object with ACL', async () => {
    Parse.User.enableUnsafeCurrentUser();
    const parseServer = await reconfigureServer();
    const user = new Parse.User();
    user.set('username', 'torn');
    user.set('password', 'acl');
    await user.signUp();

    const acl = new Parse.ACL(user);
    const object = new TestObject({ hash: 'saveSecret' });
    object.setACL(acl);

    await new Promise((resolve) => parseServer.server.close(resolve));

    await object.saveEventually();

    let length = await Parse.EventuallyQueue.length();
    assert(Parse.EventuallyQueue.isPolling());
    assert.strictEqual(length, 1);

    await reconfigureServer({});

    while (Parse.EventuallyQueue.isPolling()) {
      await sleep(100);
    }
    assert.strictEqual(Parse.EventuallyQueue.isPolling(), false);

    length = await Parse.EventuallyQueue.length();
    while (length) {
      await sleep(100); // <---------------------- stalls as data is lost
    }
    length = await Parse.EventuallyQueue.length();
    assert.strictEqual(length, 0);

    const query = new Parse.Query('TestObject');
    query.equalTo('hash', 'saveSecret');
    let results = await query.find();
    while (results.length === 0) {
      results = await query.find();
    }
    assert.strictEqual(results.length, 1);
});

Removing the ACL check will fix this issue.

serverOptions,
id: object.id,
className: object.className,
Expand Down
3 changes: 3 additions & 0 deletions src/__tests__/EventuallyQueue-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@
toJSON() {
return this.attributes;
}
_getSaveJSON() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the diff to toJSON?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main thing that makes this work is that _getSaveJSON() (outside the mock) does not include the ACL object while toJSON does. In the mocks, there is no real different why the tests didn't fail already they don't

When using saveEventually and connection is down, the object gets stored in the queue as json. Today it uses 'getJSON' and that includes a json representation of ACL. _getSaveJSON doesn't.

This json object is later used (line 276) to save the queued object when the server is back up:

await object.save(queueObject.object, queueObject.serverOptions);

save here does not support ACL in json format (or ACL at all, don't think you can change access outside of server).

There are TBH probably even better fixes here. We should probably only store the dirty fields of the object and not overwrite everything (but it does already make sure before line 276 that its not overwriting other things).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reason why the EventuallyQueue tests didn't catch this is because ACLs aren't on the objects before they are loaded from the servers.

Doing this..

x = new Parse.Object('TestObject');
  ParseObject2 {id: undefined, _localId: undefined, _objCount: 1762, className: 'TestObject'}
x.set('foo', 'bar');
x.toJSON()
{foo: 'bar'}
x.save();

Would work fine. But in reality you often modify existing objects. When objects are loaded from server, the toJSON includes a lot more properties. Here is the same object:

x = await new Parse.Query('TestObject').find();
x[0].toJSON()

   {"foo":"bar",
    "createdAt":"2024-03-17T20:52:35.213Z",
    "updatedAt":"2024-03-17T20:53:30.133Z",
    "ACL":{"*":{"read":true,"write":true}},
    "objectId":"sZhK7r5owf"}

All tests in EventuallyQueue-test just creates non-server saved objects which doesn't catch this problem with "real" objects from servers.

example:

const object = new ParseObject('TestObject');
    object.set('foo', 'bar');
    object.set('test', '1234');

return this.attributes;

Check failure on line 29 in src/__tests__/EventuallyQueue-test.js

View workflow job for this annotation

GitHub Actions / build (Node 18, 18.19.0) 

Expected indentation of 4 spaces but found 6

Check failure on line 29 in src/__tests__/EventuallyQueue-test.js

View workflow job for this annotation

GitHub Actions / build (Node 20, 20.10.0) 

Expected indentation of 4 spaces but found 6
}
static extend(className) {
class MockSubclass {
constructor() {
Expand Down
Loading