Skip to content

Rem/mysql#13

Open
Kamynale wants to merge 9 commits intoOpexflow:masterfrom
Kamynale:rem/mysql
Open

Rem/mysql#13
Kamynale wants to merge 9 commits intoOpexflow:masterfrom
Kamynale:rem/mysql

Conversation

@Kamynale
Copy link
Contributor

@Kamynale Kamynale commented Sep 2, 2020

No description provided.

server/app.js Outdated
console.log('There is no such user, adding now');
pool.query(`INSERT INTO Users SET
id = '${profile.id}', login = '${profile.displayName}', email = '${email}', photo='${photo}', createdAt = NOW(), balance = 10000
ON DUPLICATE KEY UPDATE login = '${profile.displayName}', email = '${email}', photo='${photo}'
Copy link
Collaborator

Choose a reason for hiding this comment

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

в каком случае выполнится здесь ON DUPLICATE KEY UPDATE ... ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

При новой авторизации пользователя сравнивается id, если идентичны, проверяется login, если он изменился, тогда срабатывает функция update

Copy link
Collaborator

Choose a reason for hiding this comment

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

но вверху pool.query(SELECT * from Users where id=${profile.id}, (err, rows) => { который не даст попасть в ON DUPLICATE KEY UPDATE

server/app.js Outdated
console.log('There is no balance User in Transaction, adding now');
pool.query(`INSERT INTO Transactions SET
id = '${profile.id}', login = '${profile.displayName}'
ON DUPLICATE KEY UPDATE login = '${profile.displayName}'
Copy link
Collaborator

Choose a reason for hiding this comment

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

аналогично

Copy link
Collaborator

Choose a reason for hiding this comment

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

для чего вообще INSERT INTO Transactions SET здесь делать?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Иначе баланс не будет отображаться при регистрации пользователя. Как только он проходит авторизацию, идет запись в транзакцию с id и балансом

server/app.js Outdated
/*pool.query(`SELECT * from Users where id=${req.user.id}`, (err, rows) => {
res.end(JSON.stringify({ user: req.user, finance: { balance: rows && rows[0] && rows[0].balance }}));
})*/
pool.query(`SELECT * from Transactions where id=${req.user.id}`, (err, rows) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

расскажи как Transactions и баланс соотносятся?

const sql = `SELECT balance FROM Transactions WHERE id='${req.user.id}' AND balance>=${req.params.price}`;
const transaction = `UPDATE Transactions Set balance = balance - ${req.params.price} WHERE id='${req.user.id}'`

pool.getConnection(function (err, connection) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

для чего здесь getConnection ? почему здесь есть, а выше нет?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Если правильно понимаю, данная функция обертка помогает уйти от проблем с асинхронностью приложения и без нее не работает полный пул соединения, все падает

connection.beginTransaction(function (err) {
if (err) { //Transaction Error (Rollback and release connection)
connection.rollback(function () {
connection.release();
Copy link
Collaborator

Choose a reason for hiding this comment

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

зачем обрывать соединение?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Для безопасности транзакции

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Это был 1 из примеров, где парень описывал, что данная транзакция будет более безопасной. Смысл был защиты от хакерских атак, в данную функцию не успевают подставить левые данные и постоянно метяется хеш т.к. новое соединение. На скорость подключения это не особо влияет, но могу убрать

server/app.js Outdated
pool.query(`UPDATE Users SET balance = balance - '${req.params.price}' WHERE id = '${req.user.id}'`);
return res.end(JSON.stringify({}));
const sql = `SELECT balance FROM Transactions WHERE id='${req.user.id}' AND balance>=${req.params.price}`;
const transaction = `UPDATE Transactions Set balance = balance - ${req.params.price} WHERE id='${req.user.id}'`
Copy link
Collaborator

Choose a reason for hiding this comment

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

я ожидал, что в транзакциях будут все сделки находится, а здесь по сути тоже самое, что и было, только в другую таблицу переместилось.

if (err) { //Query Error (Rollback and release connection)
connection.rollback(function () {
connection.release();
console.log('no money');
Copy link
Collaborator

Choose a reason for hiding this comment

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

очень много повторений, почему это не вынести в отдельный метод?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Подразумевается импорт функции и вызов через new?

@Kamynale Kamynale closed this Sep 4, 2020
@Kamynale Kamynale reopened this Sep 4, 2020
server/app.js Outdated
console.log('There is no balance User in Transaction, adding now');
pool.query(`INSERT INTO Transactions SET
id = '${profile.id}', login = '${profile.displayName}'
ON DUPLICATE KEY UPDATE login = '${profile.displayName}'
Copy link
Collaborator

Choose a reason for hiding this comment

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

для чего в Transactions login, если есть id ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Дополнительная проверка

Copy link
Collaborator

Choose a reason for hiding this comment

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

это лишняя информация, так можно всю инфу из таблицы в таблицу таскать

server/app.js Outdated
pool.query(`UPDATE Users SET balance = balance - '${req.params.price}' WHERE id = '${req.user.id}'`);
return res.end(JSON.stringify({}));
const sql = `SELECT balance FROM Transactions WHERE id='${req.user.id}' AND balance>=${req.params.price}`;
const transaction = `UPDATE Transactions Set stock='SBER',commission=0, price=${req.params.price}+commission, balance = balance - price WHERE id='${req.user.id}'`
Copy link
Collaborator

Choose a reason for hiding this comment

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

чёт некрасиво написано, где-то пробелов нет, где-то три пробела. прайс зависит от commission, баланс от price. в UPDATE всё заглавными, в Set только первая буква ...
Тут явно можно улучшить

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Насчет красоты согласен, из phpmyadmin копировал и получился такой формат- не заметил. На счет функций, у нас баланс и так будет зависеть от цены в любом случае. Если мы разделяли таблицу на price, commission, тогда я думаю что и есть необходимость использования строк. Если мы будем знать комиссию биржи, тогда возможно будет переписать, как то иначе. Но как по мне, данная функция, при правильной передачи данных, является максимально профитной. никаких ошибок не будет, так как привязка идет к переменным

server/app.js Outdated
return res.end(JSON.stringify({}));
const sql = `SELECT balance FROM Transactions WHERE id='${req.user.id}' AND balance>=${req.params.price}`;
const transaction = `UPDATE Transactions Set stock='SBER',commission=0, price=${req.params.price}+commission, balance = balance - price WHERE id='${req.user.id}'`
const logs = `INSERT INTO Transactions_logs SET id=(SELECT id FROM Transactions WHERE id='${req.user.id}'), balance=(SELECT balance FROM Transactions WHERE id='${req.user.id}'), price=(SELECT price FROM Transactions WHERE id='${req.user.id}'), commission=(SELECT commission FROM Transactions WHERE id='${req.user.id}'), stock=(SELECT stock FROM Transactions WHERE id='${req.user.id}')`
Copy link
Collaborator

Choose a reason for hiding this comment

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

миллион селектов, хотя можно один раз всё достать

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Попробую переписать

Copy link
Contributor Author

Choose a reason for hiding this comment

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

INSERT INTO Transactions_logs(id,balance,price,commission,stock) SELECT id,balance,price,commission,stock FROM Transactions WHERE id='${req.user.id}')
Понял свою ошибку

server/app.js Outdated
return res.end(JSON.stringify({}));
const sql = `SELECT balance FROM Transactions WHERE id='${req.user.id}' AND balance>=${req.params.price}`;
const transaction = `UPDATE Transactions Set stock='SBER',commission=0, price=${req.params.price}+commission, balance = balance + price WHERE id='${req.user.id}'`
const logs = `INSERT INTO Transactions_logs SET id=(SELECT id FROM Transactions WHERE id='${req.user.id}'), balance=(SELECT balance FROM Transactions WHERE id='${req.user.id}'), price=(SELECT price FROM Transactions WHERE id='${req.user.id}'), commission=(SELECT commission FROM Transactions WHERE id='${req.user.id}'), stock=(SELECT stock FROM Transactions WHERE id='${req.user.id}')`
Copy link
Collaborator

Choose a reason for hiding this comment

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

выше нечто похожее видел уже. Это ж дублирование получается, оно здесь точно надо?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Как вариант можно вынести константу за пределы функции этих 2 функций и брать из вне. Но константа transaction изменена, ее все равно необходимо оставить внутри функции.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

И тогда придется искать новую переменную вместо user.id так как она находится в теле функции

Copy link
Collaborator

@pskucherov pskucherov left a comment

Choose a reason for hiding this comment

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

нужны правки

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants