Skip to content

Commit

Permalink
Fix flaky test with transactions (#7187)
Browse files Browse the repository at this point in the history 
* Fix flaky test with transactions

* Add CHANGELOG entry

* Fix the other transactions related tests that became flaky because now Parse Server tries to submit the transaction multilpe times in the case of TransientError

* Remove fit from tests
  • Loading branch information
@davimacedo
davimacedo committed Feb 18, 2021
1 parent 9a9fc5f commit a430d6f
Show file tree
Hide file tree
Showing 6 changed files with 161 additions and 103 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ ___
- NEW: LiveQuery support for $and, $nor, $containedBy, $geoWithin, $geoIntersects queries [#7113](https://github.com/parse-community/parse-server/pull/7113). Thanks to [dplewis](https://github.com/dplewis)
- NEW: Supporting patterns in LiveQuery server's config parameter `classNames` [#7131](https://github.com/parse-community/parse-server/pull/7131). Thanks to [Nes-si](https://github.com/Nes-si)
- NEW: `requireAnyUserRoles` and `requireAllUserRoles` for Parse Cloud validator. [#7097](https://github.com/parse-community/parse-server/pull/7097). Thanks to [dblythy](https://github.com/dblythy)
- IMPROVE: Retry transactions on MongoDB when it fails due to transient error [#7187](https://github.com/parse-community/parse-server/pull/7187). Thanks to [Antonio Davi Macedo Coelho de Castro](https://github.com/davimacedo).
- IMPROVE: Bump tests to use Mongo 4.4.4 [#7184](https://github.com/parse-community/parse-server/pull/7184). Thanks to [Antonio Davi Macedo Coelho de Castro](https://github.com/davimacedo).
- IMPROVE: Added new account lockout policy option `accountLockout.unlockOnPasswordReset` to automatically unlock account on password reset. [#7146](https://github.com/parse-community/parse-server/pull/7146). Thanks to [Manuel Trezza](https://github.com/mtrezza).
- IMPROVE: Parse Server is from now on continuously tested against all recent MongoDB versions that have not reached their end-of-life support date. Added MongoDB compatibility table to Parse Server docs. [7161](https://github.com/parse-community/parse-server/pull/7161). Thanks to [Manuel Trezza](https://github.com/mtrezza).
Expand Down
35 changes: 21 additions & 14 deletions spec/ParseServerRESTController.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ describe('ParseServerRESTController', () => {
.then(() => {
spyOn(databaseAdapter, 'createObject').and.callThrough();

RESTController.request('POST', 'batch', {
return RESTController.request('POST', 'batch', {
requests: [
{
method: 'POST',
Expand All @@ -218,19 +218,23 @@ describe('ParseServerRESTController', () => {
expect(response[1].success.objectId).toBeDefined();
expect(response[1].success.createdAt).toBeDefined();
const query = new Parse.Query('MyObject');
query.find().then(results => {
expect(databaseAdapter.createObject.calls.count()).toBe(2);
expect(databaseAdapter.createObject.calls.argsFor(0)[3]).toBe(
databaseAdapter.createObject.calls.argsFor(1)[3]
);
return query.find().then(results => {
expect(databaseAdapter.createObject.calls.count() % 2).toBe(0);
expect(databaseAdapter.createObject.calls.count() > 0).toEqual(true);
for (let i = 0; i + 1 < databaseAdapter.createObject.calls.length; i = i + 2) {
expect(databaseAdapter.createObject.calls.argsFor(i)[3]).toBe(
databaseAdapter.createObject.calls.argsFor(i + 1)[3]
);
}
expect(results.map(result => result.get('key')).sort()).toEqual([
'value1',
'value2',
]);
done();
});
});
});
})
.catch(done.fail);
});

it('should not save anything when one operation fails in a transaction', done => {
Expand Down Expand Up @@ -513,18 +517,18 @@ describe('ParseServerRESTController', () => {
const results3 = await query3.find();
expect(results3.map(result => result.get('key')).sort()).toEqual(['value1', 'value2']);

expect(databaseAdapter.createObject.calls.count()).toBe(13);
expect(databaseAdapter.createObject.calls.count() >= 13).toEqual(true);
let transactionalSession;
let transactionalSession2;
let myObjectDBCalls = 0;
let myObject2DBCalls = 0;
let myObject3DBCalls = 0;
for (let i = 0; i < 13; i++) {
for (let i = 0; i < databaseAdapter.createObject.calls.count(); i++) {
const args = databaseAdapter.createObject.calls.argsFor(i);
switch (args[0]) {
case 'MyObject':
myObjectDBCalls++;
if (!transactionalSession) {
if (!transactionalSession || (myObjectDBCalls - 1) % 2 === 0) {
transactionalSession = args[3];
} else {
expect(transactionalSession).toBe(args[3]);
Expand All @@ -535,7 +539,7 @@ describe('ParseServerRESTController', () => {
break;
case 'MyObject2':
myObject2DBCalls++;
if (!transactionalSession2) {
if (!transactionalSession2 || (myObject2DBCalls - 1) % 9 === 0) {
transactionalSession2 = args[3];
} else {
expect(transactionalSession2).toBe(args[3]);
Expand All @@ -550,9 +554,12 @@ describe('ParseServerRESTController', () => {
break;
}
}
expect(myObjectDBCalls).toEqual(2);
expect(myObject2DBCalls).toEqual(9);
expect(myObject3DBCalls).toEqual(2);
expect(myObjectDBCalls % 2).toEqual(0);
expect(myObjectDBCalls > 0).toEqual(true);
expect(myObject2DBCalls % 9).toEqual(0);
expect(myObject2DBCalls > 0).toEqual(true);
expect(myObject3DBCalls % 2).toEqual(0);
expect(myObject3DBCalls > 0).toEqual(true);
});
});
}
Expand Down
28 changes: 17 additions & 11 deletions spec/batch.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -228,10 +228,13 @@ describe('batch', () => {
expect(response.data[1].success.createdAt).toBeDefined();
const query = new Parse.Query('MyObject');
query.find().then(results => {
expect(databaseAdapter.createObject.calls.count()).toBe(2);
expect(databaseAdapter.createObject.calls.argsFor(0)[3]).toBe(
databaseAdapter.createObject.calls.argsFor(1)[3]
);
expect(databaseAdapter.createObject.calls.count() % 2).toBe(0);
expect(databaseAdapter.createObject.calls.count() > 0).toEqual(true);
for (let i = 0; i + 1 < databaseAdapter.createObject.calls.length; i = i + 2) {
expect(databaseAdapter.createObject.calls.argsFor(i)[3]).toBe(
databaseAdapter.createObject.calls.argsFor(i + 1)[3]
);
}
expect(results.map(result => result.get('key')).sort()).toEqual([
'value1',
'value2',
Expand Down Expand Up @@ -542,18 +545,18 @@ describe('batch', () => {
const results3 = await query3.find();
expect(results3.map(result => result.get('key')).sort()).toEqual(['value1', 'value2']);

expect(databaseAdapter.createObject.calls.count()).toBe(13);
expect(databaseAdapter.createObject.calls.count() >= 13).toEqual(true);
let transactionalSession;
let transactionalSession2;
let myObjectDBCalls = 0;
let myObject2DBCalls = 0;
let myObject3DBCalls = 0;
for (let i = 0; i < 13; i++) {
for (let i = 0; i < databaseAdapter.createObject.calls.count(); i++) {
const args = databaseAdapter.createObject.calls.argsFor(i);
switch (args[0]) {
case 'MyObject':
myObjectDBCalls++;
if (!transactionalSession) {
if (!transactionalSession || (myObjectDBCalls - 1) % 2 === 0) {
transactionalSession = args[3];
} else {
expect(transactionalSession).toBe(args[3]);
Expand All @@ -564,7 +567,7 @@ describe('batch', () => {
break;
case 'MyObject2':
myObject2DBCalls++;
if (!transactionalSession2) {
if (!transactionalSession2 || (myObject2DBCalls - 1) % 9 === 0) {
transactionalSession2 = args[3];
} else {
expect(transactionalSession2).toBe(args[3]);
Expand All @@ -579,9 +582,12 @@ describe('batch', () => {
break;
}
}
expect(myObjectDBCalls).toEqual(2);
expect(myObject2DBCalls).toEqual(9);
expect(myObject3DBCalls).toEqual(2);
expect(myObjectDBCalls % 2).toEqual(0);
expect(myObjectDBCalls > 0).toEqual(true);
expect(myObject2DBCalls % 9).toEqual(0);
expect(myObject2DBCalls > 0).toEqual(true);
expect(myObject3DBCalls % 2).toEqual(0);
expect(myObject3DBCalls > 0).toEqual(true);
});
});
}
Expand Down
17 changes: 14 additions & 3 deletions src/Adapters/Storage/Mongo/MongoStorageAdapter.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1046,9 +1046,20 @@ export class MongoStorageAdapter implements StorageAdapter {
}

commitTransactionalSession(transactionalSection: any): Promise<void> {
return transactionalSection.commitTransaction().then(() => {
transactionalSection.endSession();
});
const commit = retries => {
return transactionalSection
.commitTransaction()
.catch(error => {
if (error && error.hasErrorLabel('TransientTransactionError') && retries > 0) {
return commit(retries - 1);
}
throw error;
})
.then(() => {
transactionalSection.endSession();
});
};
return commit(5);
}

abortTransactionalSession(transactionalSection: any): Promise<void> {
Expand Down
86 changes: 51 additions & 35 deletions src/ParseServerRESTController.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,44 +48,60 @@ function ParseServerRESTController(applicationId, router) {
}

if (path === '/batch') {
let initialPromise = Promise.resolve();
if (data.transaction === true) {
initialPromise = config.database.createTransactionalSession();
}
return initialPromise.then(() => {
const promises = data.requests.map(request => {
return handleRequest(request.method, request.path, request.body, options, config).then(
response => {
if (options.returnStatus) {
const status = response._status;
delete response._status;
return { success: response, _status: status };
const batch = transactionRetries => {
let initialPromise = Promise.resolve();
if (data.transaction === true) {
initialPromise = config.database.createTransactionalSession();
}
return initialPromise.then(() => {
const promises = data.requests.map(request => {
return handleRequest(request.method, request.path, request.body, options, config).then(
response => {
if (options.returnStatus) {
const status = response._status;
delete response._status;
return { success: response, _status: status };
}
return { success: response };
},
error => {
return {
error: { code: error.code, error: error.message },
};
}
return { success: response };
},
error => {
return {
error: { code: error.code, error: error.message },
};
}
);
});
return Promise.all(promises).then(result => {
if (data.transaction === true) {
if (result.find(resultItem => typeof resultItem.error === 'object')) {
return config.database.abortTransactionalSession().then(() => {
return Promise.reject(result);
});
} else {
return config.database.commitTransactionalSession().then(() => {
);
});
return Promise.all(promises)
.then(result => {
if (data.transaction === true) {
if (result.find(resultItem => typeof resultItem.error === 'object')) {
return config.database.abortTransactionalSession().then(() => {
return Promise.reject(result);
});
} else {
return config.database.commitTransactionalSession().then(() => {
return result;
});
}
} else {
return result;
});
}
} else {
return result;
}
}
})
.catch(error => {
if (
error &&
error.find(
errorItem => typeof errorItem.error === 'object' && errorItem.error.code === 251
) &&
transactionRetries > 0
) {
return batch(transactionRetries - 1);
}
throw error;
});
});
});
};
return batch(5);
}

let query;
Expand Down
97 changes: 57 additions & 40 deletions src/batch.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,49 +83,66 @@ function handleBatch(router, req) {
req.config.publicServerURL
);

let initialPromise = Promise.resolve();
if (req.body.transaction === true) {
initialPromise = req.config.database.createTransactionalSession();
}

return initialPromise.then(() => {
const promises = req.body.requests.map(restRequest => {
const routablePath = makeRoutablePath(restRequest.path);

// Construct a request that we can send to a handler
const request = {
body: restRequest.body,
config: req.config,
auth: req.auth,
info: req.info,
};

return router.tryRouteRequest(restRequest.method, routablePath, request).then(
response => {
return { success: response.response };
},
error => {
return { error: { code: error.code, error: error.message } };
}
);
});
const batch = transactionRetries => {
let initialPromise = Promise.resolve();
if (req.body.transaction === true) {
initialPromise = req.config.database.createTransactionalSession();
}

return Promise.all(promises).then(results => {
if (req.body.transaction === true) {
if (results.find(result => typeof result.error === 'object')) {
return req.config.database.abortTransactionalSession().then(() => {
return Promise.reject({ response: results });
});
} else {
return req.config.database.commitTransactionalSession().then(() => {
return initialPromise.then(() => {
const promises = req.body.requests.map(restRequest => {
const routablePath = makeRoutablePath(restRequest.path);

// Construct a request that we can send to a handler
const request = {
body: restRequest.body,
config: req.config,
auth: req.auth,
info: req.info,
};

return router.tryRouteRequest(restRequest.method, routablePath, request).then(
response => {
return { success: response.response };
},
error => {
return { error: { code: error.code, error: error.message } };
}
);
});

return Promise.all(promises)
.then(results => {
if (req.body.transaction === true) {
if (results.find(result => typeof result.error === 'object')) {
return req.config.database.abortTransactionalSession().then(() => {
return Promise.reject({ response: results });
});
} else {
return req.config.database.commitTransactionalSession().then(() => {
return { response: results };
});
}
} else {
return { response: results };
});
}
} else {
return { response: results };
}
}
})
.catch(error => {
if (
error &&
error.response &&
error.response.find(
errorItem => typeof errorItem.error === 'object' && errorItem.error.code === 251
) &&
transactionRetries > 0
) {
return batch(transactionRetries - 1);
}
throw error;
});
});
});
};
return batch(5);
}

module.exports = {
Expand Down

0 comments on commit a430d6f

Please sign in to comment.