Open bobthebuilderio opened 4 years ago
@bobthebuilderio Thanks for posting! We'll take a look as soon as possible.
In the mean time, there are a few ways you can help speed things along:
Please remember: never post in a public forum if you believe you've found a genuine security vulnerability. Instead, disclose it responsibly.
For help with questions about Sails, click here.
As an additional piece of information, it is worth noting that we do use DynamoDb Waterline Adapter. We added some extra feature support ourselves internally as it was a bit behind. Yet I still don't see how the cases above could occur regardless of anything else. There seem to be a Major / fundamental issue somewhere.
I do not think the issue has to do with the ORM itself, as the data if logged before the array handling function described in issue 2 is correct, but get's mixed after the function gets called.
Can you share any actual code or create a minimal github repo that can reproduce these issues?
This is the main controller that fetches the data - THIS IS THE CODE THAT IS GENERATING THE ISSUE. I will follow up with another comment of the file where issue was fixed.
`module.exports = {
friendlyName: sails.('controllers.orderItem.arango.listByOrderId.friendlyName'), description: sails.('controllers.orderItem.arango.listByOrderId.description'),
inputs: { orderId: { description: sails.__('controllers.orderItem.arango.listByOrderId.inputs.orderId.description'), type: 'string', required: true, }, },
exits: { orderItemNotFound: { responseType: 'orderItemNotFound' }, universalItemNotFound: { responseType: 'universalItemNotFound' }, orderNotFound: { responseType: 'orderNotFound' }, serverError: { responseType: 'serverError' }, success: { description: 'order with itemslist' }, },
fn: async function (inputs, exits) { try {
const { orderId } = inputs;
// Get item lists
const orderWithItemsList = await sails.helpers.models.arangodb.orderItem.listByOrderId.with({ orderId });
const { order, itemsList } = orderWithItemsList;
// Validation added to investigate bug
_validateItemsList2({ itemsList });
// !!!!!!! DATA IS OK HERE STILL !!!!!! //
// Prepare return data
const newItemsList = await _addMissingDetailsList({ itemsList });
console.log('TAKE 2');
// Validation added to investigate bug
_validateItemsList2({ itemsList: newItemsList });
//!!!!!!!!! BUG APPEARS IN THIS CHECK !!!!! DATA MIXED WITH ANOTHER USER REQUEST THAT HAPPENED AT THE SAME TIME //
const newOrder = await _addMissingDetailsToOrder({ order });
console.log('TAKE 3');
// Validation added to investigate bug
_validateItemsList2({ itemsList: newItemsList });
orderWithItems = {
...sails.dtos.arango.OrderDTO({ order: newOrder }),
itemsList: sails.dtos.arango.ItemListDTO({ items: newItemsList }),
};
console.log('TAKE 4');
// Validation added to investigate bug
_validateItemsList2({ itemsList: orderWithItems.itemsList });
// Done!
exits.success({ orderWithItems });
} catch (error) {
switch (_.get(error, 'raw.code') || _.get(error, 'code')) {
case 'orderItemNotFound':
exits.orderItemNotFound({ message: sails.__('controllers.orderItem.arango.listByOrderId.exits.orderItemNotFound') });
break;
case 'universalItemNotFound':
exits.universalItemNotFound({ message: sails.__('controllers.orderItem.arango.listByOrderId.exits.universalItemNotFound') });
break;
case 'orderNotFound':
exits.orderNotFound({ message: sails.__('controllers.orderItem.arango.listByOrderId.exits.orderNotFound') });
break;
default:
exits.serverError(error);
}
} finally {
return exits;
}
}, };
/**
*/ async function _addMissingDetailsList ({ itemsList }) {
const itemsListToReturn = [];
for (item of itemsList) {
const orderItem = await OrderItem.getByCodeIfExists.with({ code: item.itemCode, orderId: item.orderId, });
const universalItem = await UniversalItem.getByCodeIfExists.with({ code: item.itemCode, });
item.stainDetails = orderItem.stainDetails; item.frontImage = universalItem.frontImage; itemsListToReturn.push(item); }
return itemsListToReturn; }
/**
*/ async function _addMissingDetailsToOrder ({ order }) { const { pickupImageBags } = await sails.hooks.srvOrder.order.getByIdIfExists({ orderId: order.orderId });
order.pickupImageBags = !_.isArray(pickupImageBags) ? [pickupImageBags] : pickupImageBags;
return order; }
/**
*/ function _validateItemsList2 ({ itemsList }) {
const itemsList2 = _.groupBy(itemsList, 'orderAlphaId');
if (Object.keys(itemsList2).length > 1) { console.log('CUSTOMER APPROVAL MIXED ORDERS AT BACKEND MAPPING LEVEL');
} }
`
THIS IS THE CODE THAT FIXED THE ISSUE. AS YOU CAN SEE SIMPLE CHANGE TO THE CODE. NOTHING THAT IS OBVIOUSLY WRONG OR COULD CAUSE AN ISSUE LIKE THIS. SOMETHING HAS TO DO WITH ASYNC/AWAIT / LOOPS. As the issue arises when we have promise.all or awaits in for loops. Might be just talking gibberish :) !
`module.exports = {
friendlyName: sails.('controllers.orderItem.arango.listByOrderId.friendlyName'), description: sails.('controllers.orderItem.arango.listByOrderId.description'),
inputs: { orderId: { description: sails.__('controllers.orderItem.arango.listByOrderId.inputs.orderId.description'), type: 'string', required: true, }, },
exits: { orderItemNotFound: { responseType: 'orderItemNotFound' }, universalItemNotFound: { responseType: 'universalItemNotFound' }, orderNotFound: { responseType: 'orderNotFound' }, serverError: { responseType: 'serverError' }, success: { description: 'order with itemslist' }, },
fn: async function (inputs, exits) { try {
const { orderId } = inputs;
// Get item lists
const orderWithItemsList = await sails.helpers.models.arangodb.orderItem.listByOrderId.with({ orderId });
const { order, itemsList } = orderWithItemsList;
// Prepare return data
await _addMissingDetailsList({ itemsList });
const newOrder = await _addMissingDetailsToOrder({ order });
orderWithItems = {
...sails.dtos.arango.OrderDTO({ order: newOrder }),
itemsList: sails.dtos.arango.ItemListDTO({ items: itemsList }),
};
// Done!
exits.success({ orderWithItems });
} catch (error) {
switch (_.get(error, 'raw.code') || _.get(error, 'code')) {
case 'orderItemNotFound':
exits.orderItemNotFound({ message: sails.__('controllers.orderItem.arango.listByOrderId.exits.orderItemNotFound') });
break;
case 'universalItemNotFound':
exits.universalItemNotFound({ message: sails.__('controllers.orderItem.arango.listByOrderId.exits.universalItemNotFound') });
break;
case 'orderNotFound':
exits.orderNotFound({ message: sails.__('controllers.orderItem.arango.listByOrderId.exits.orderNotFound') });
break;
default:
exits.serverError(error);
}
} finally {
return exits;
}
}, };
/**
*/ async function _addMissingDetailsList ({ itemsList }) {
for (let i = 0; i < itemsList.length; i++) {
const orderItem = await OrderItem.getByCodeIfExists.with({ code: itemsList[i].itemCode, orderId: itemsList[i].orderId, });
const universalItem = await UniversalItem.getByCodeIfExists.with({ code: itemsList[i].itemCode, });
itemsList[i].stainDetails = orderItem.stainDetails; itemsList[i].frontImage = universalItem.frontImage;
}
}
/**
*/ async function _addMissingDetailsToOrder ({ order }) { const { pickupImageBags } = await sails.hooks.srvOrder.order.getByIdIfExists({ orderId: order.orderId });
order.pickupImageBags = !_.isArray(pickupImageBags) ? [pickupImageBags] : pickupImageBags;
return order; } `
@johnabrams7 I think this should not be labeled as a Question, rather should be something more serious and needs immediate attention. What do you think ?
@bobthebuilderio Yes, good point - went ahead and labeled it Bug instead 👍
Huge thanks for all the effort and dedication with this, along with a workaround! 🎉 Team will give this a look. In the meantime, can you provide us a minimal repo that reproduces this issue? PR welcome as well!
To confirm, which specific DynamoDb Waterline Adapter are you using with Sails? There appears to be multiple.
@johnabrams7 sorry for the delay, was overwhelmed. I will ask someone from the team to provide a clean repo that could reproduce the issue. Hopefully will be able to demostrate it as this appears on a production environment, so we might have to create a script that simulate simultaneous hits. Will update you soon.
@bobthebuilderio Now worries, that sounds great! We'll look forward to checking it out.
Node version: node:8.12-alpine Sails version (sails): "^1.2.3" ORM hook version (sails-hook-orm): "^2.1.1" Sockets hook version (sails-hook-sockets): "^1.4.0"
Unfortunately we faced two major issues on 2 important api built with sailsjs.
ISSUE 1: Somehow response data from one request say to '/user/login' end up being swapped with someone's else request response. So users on our apps got swapped and saw the other's info,.
ISSUE 2: Similarly, two parallel requests to an api say 'list-items' end up mixing items from both users together in the resonse. So one user gets some of his items and some of the other user's items & same with the other user.
What we did to solve the problem, was to refactor the code a bit: 1- We stopped using things like promise.all() and did some refactoring which solved ISSUE 1 2- We had a function receives items list as input goes into a for loop ( for x of y ) to call two awaits db calls for each of the items, create some additional properly for the item, push into a new array and return this array. This array was getting mixed up.
Regardless of our code and whether it could be written in a better way or not, how on earth is data being swapped like this ? is there anyone who knows or faced something like this with saisjs before ?
Thank you !