ユーザ名がUniqueか検証したい[Node.js + MySQL]

Question

前提・実現したいこと

Node.jsもMySQLも初心者で、勉強もかねてブログを作ってます。
入力されたusernameが既にDB内に存在するか検証したいのですが、うまく動きません。

###想定している動作例
HPにはusernameとpasswordを入力するformと送信ボタンを用意する。(URLはlocalhost:3000/signup)
送信ボタンを押すとlocalhost:3000/signup/validateに遷移。

usernameとpasswordが入力されなかった場合、isFormFilledの時点でisValidatedがfalseになって、errorsを返す。
usernameとpasswordが入力されているが、そのusernameを持つデータがDBに既に存在したらisValidatedがfalseになって、errorsを返す。
そのusernameをもつデータが存在しない場合、それをDBに登録する。(未実装)

質問

• checkStatus関数を使用して、usernameとpasswordが空の場合、errorsにエラーが、isValidateもfalseが入っていることは確認できました。。

しかし、既にDBに登録されているusernameと適当なパスワードを入力した場合、errorsにエラーは入るものの、isValidatedがtrueのままになっていました。
isValidatedもfalseになってほしいのですが、何故trueのままなのでしょうか？

• account.jsに関して、originalとerrorsの代入が終わる前にconsole.logs(errors)が実行されてしまうのですが、どうすれば順番に処理されるのでしょうか？

該当のソースコード

account-control.js

var { MYSQL_PASSWORD } = require("../../config/app.config").security;
var mysql = require("mysql");
var connection = mysql.createConnection({
  host: "localhost",
  user: "root",
  password: MYSQL_PASSWORD,
  database: "bbs"
});

class Authenticator {
  static createRegistUsersData(body) {
    return {
      username: body.username,
      password: body.password
    };
  }

  static validateRegistUsersData(body) {
    var isValidated = true, errors = {};

    var isFormFilled = () => {
      return new Promise((resolve, reject) => {
        if (!body.username) {
          isValidated = false;
          errors.username = "usernameが未入力です。";
        }
        if (!body.password) {
          isValidated = false;
          errors.password = "passwordが未入力です。";
        }

        if (isValidated === false) {
          reject();
        } else {
          resolve();
        }
      });
    };

    var isUsernameUnique = () => {
      return new Promise((resolve, reject) => {
        if (body.username && body.password) {
          Promise.all([() => {
            connection.connect();
            connection.query({
              sql: "SELECT * FROM users WHERE username = ?;",
              values: [body.username]
            }, (error, results, fields) => {
              if (results == "") {
                console.log("unique");
                resolve();
              } else {
                errors.username = "usernameは既に使用されています。";
                isValidated = false;
                reject();
              }
            });
          }, () => {
            connection.end();
          }]);
        }
      });
    };

    var checkStatus = () => {
      return new Promise((resolve, reject) => {
        console.log(errors);
        console.log(isValidated);
        resolve();
      });
    };

    Promise.resolve()
      .then(isFormFilled())
      .then(isUsernameUnique())
      .then(checkStatus())
      .then(() => {
        return isValidated ? undefined : errors;
      });
  }
}

module.exports = Authenticator;

account.js

var router = require("express").Router();
var Authenticator = require("../lib/security/account-controll");


router.get("/", (req, res) => {
  res.render("./account/signup.ejs", { message: req.flash("message") });
});

router.post("/validate", (req, res) => {
  var original, errors;
  Promise.resolve()
    .then(() => {
      original = Authenticator.createRegistUsersData(req.body);
      errors = Authenticator.validateRegistUsersData(req.body);
    })
    .then(() => {
      console.log(errors);
      if (errors) {
        res.render("./account/signup.ejs", { original, errors });
      }
      else {
        res.render("./account/regist-complete.ejs");
      }
    });
});



module.exports = router;

###追記
1.staticなメソッドをasyncさせる方法はないのでしょうか？
2.以下のvalidateRegisterUsersData()のようなPromiseチェーンを組むときに3変数になってしまうのですが、isFilledUsernameの引数をusernameだけにしてもPromiseチェーンは組めるのでしょうか？
3.以下の2点を守ってコードを書き換える際に、Promiseチェーンを組むときは以下のような形になると思ったのですが、間違ってますか？

• ejsにエラー情報を渡したいので、errorsの中にエラーを格納したい。
• クラスを崩して、すべてのメソッドをモジュールとしてexportsするのは避けたいので、Authenticatorクラスの関数として実装したい。

  static validateRegistUsersData(body) {
    console.log("validateRegistUsersData");
    var isValidated = true, errors = {};
    Promise.resolve(body.username, isValidated, errors)
      .then(this.__isFilledUsername)
      .then((results) => new Promise((resolve, reject) => {
        resolve(results.isValidated);
      }));
  }


  // __isFilledUsername = username, isValidated, errors => new Promise() だとエラー
  __isFilledUsername(username, isValidated, errors) {
    console("__isFilledUsername");
    new Promise((resolve, reject) => {
      console.log(username);
      resolve({ isValidated, errors });
    });
  }

  static __isFilledPassword(password, isValidated, errors) {
    new Promise((resolve, reject) => {
      isValidated = false;
      errors.password = "passwordが未入力です。";
    });
  }

Answer 1

実践ではMySQLはコネクション張りっぱなしですし、
その位置でPromise.allは意味無いですね。
あと、１メソッドの中身が何行あるんだ？ってくらいあってオブジェクト指向の意味が無かったりしますが・・・

まぁ、これらは本件に関係ないので後々の課題ということで気が向いたらぼちぼち改良させてください。

if (results == "") {

!?!?!?

JSの比較演算子は挙動の管理がクソ面倒なので、
TeratailのNode.jsランキングで１位を取るまではイコール２つ==の比較演算子は使用してはいけません。
唯一使っても良い例外はnull,undefinedを両対応で見分けるresults == nullだけです。

まぁ、[] == ""の結果はtrueになりますし、[{}] == ""の結果はfalseになる事を確認したので大丈夫ですが。

isValidatedもfalseになってほしいのですが、何故trueのままなのでしょうか？

全コードを舐めましたが、Node.js名物非同期処理の洗礼ですね。
Node.jsはシングルスレッドで動く関係で、普通のコールバック地獄を嫌がってPromiseで包んでも別に同期処理になるわけではないので、質問文の課題は解決しません。

この辺はNode.jsのイベントループの仕様を勉強してみてください。
つまり、一生thenの中身で数珠繋ぎにして後続の処理を解決させてください。

Promise.resolve()
  .then(isFormFilled())
  .then(isUsernameUnique())
  .then(checkStatus())
  .then(() => {
    return isValidated ? undefined : errors;
  });

君たち、そこで実行しちゃダメだよ。
.then(isFormFilled)のようにカッコ無しで繋ぐのが正式な使い方です。

Promise内でresolve('hogehoge')みたいに値を入れて実行して、
次のthenの第一引数に突っ込んだ関数の更に第一引数にこの'hogehoge'というデータを入れて実行させるというリレーをする為のものです。

つまり、コードとしてはほぼ惜しい所までは出来ているものの、
実際にこれを走らせるとthenの意味はまるでなくて、その場で全てのPromiseが同時に走るわ、引き継がせるデータがちゃんとリレー出来てないわという有様で、
意図したように動かないんじゃないの？という印象ですね。

特に設計思想としてはpromise走らせた結果としてresolve(isValided)を実行しながら返すという設計にすべきで、予めlet isValided = true;を実行しておいて、Promiseの中身で外のスコープの変数を書き換えてfalseにするのはお前Promiseの意味ないじゃんということで落第です。

どうすれば順番に処理されるのでしょうか？

Promise覚え直しですね。
とはいえ、thenの中に全部書けというのは相当辛いのでアドバイスします。
async/await覚えましょう。

トップレベルにmainみたいな関数用意して、こんな感じでやれば綺麗に書けるでしょう。
どの様にオブジェクト指向プログラミングをしていくかは別途考えてみてください。

// Node.jsがWebサーバであるならMySQLは繋ぎっぱなしにすべき
connection.connect();

// 最初の1行目で返すなら{}もreturnも不要でこの様に書いたほうがいい
// 関数は外のbodyみたいな値に依存するのではなく、引数で受け付けたほうがいい
const isUniqueUsername = username => new Promise((resolve, reject) => {
  // usernameが空か否かはisUniqueで確認するべきじゃないので、バリデート関数を別途用意すべき

  connection.query({
    sql: "SELECT * FROM users WHERE username = ?;",
    values: [username]
  }, (error, results, fields) => {
    // errorがnull以外の時は、MySQLのコネクション失敗やらの申告なエラーなのでrejectで対応
    if (error) {
      reject(error);
      return;
    }

    // 余計な事はせずにやること終わったらさっさとtrue or false返して終了する
    resolve(results.length === 0);
  });
});

const main = async () => {
  // bodyをどっから取ってくるかは知らんけどこんな感じ
  var isUnique = await isUniqueUsername(body.username);
  if (isUnique) {
    console.log('ユニークなIDです');
  } else {
    console.log('IDが重複しています');
  }
}

---

【追記箇所への回答】

1.staticなメソッドをasyncさせる方法はないのでしょうか？

まぁ、参考書とかもないですし情報少なくて辛いですよね。
これを参考にしてみてください。

※await構文の右辺がPromiseの場合、実行してresolve(value)のvalue部分を取り出しますが、String等の値であればそれをそのまま利用しようとします。

class Animal {
  static async meow () {
    return await 'meow!'
  }
}
Animal.meow().then(console.log);
// "meow!"が出力される

2.以下のvalidateRegisterUsersData()のようなPromiseチェーンを組むときに3変数になってしまうのですが、isFilledUsernameの引数をusernameだけにしてもPromiseチェーンは組めるのでしょうか？

できる or できないで言えばできます。
Promiseのreslve関数を実行するときの書式で、resolve(1, 2, 3)という風に3つの引数で実行した場合、
.then((a, b, c) => console.log(a, b, c))という風に3つの引数で受けることが可能です。

ただし、async / await構文で戻って来る場合、resolveの第一引数しか持って帰れません。
Promise返す関数は最小の挙動を記述すれば、そんなにたくさんの引数を持って帰りたいという要求は出るはずがありません。

3.以下の2点を守ってコードを書き換える際に、Promiseチェーンを組むときは以下のような形になると思ったのですが、間違ってますか？

そもそもの話で、Promiseが必要な関数/メソッドってどういうものだと思いますか？
それは「非同期処理であるべきもの」だけです。

非同期処理であるべきものとは何でしょうか？
それは主に「HDDに問い合わせる」「ネットワーク越しに問い合わせる」といった、メモリに保存済みのものを呼び出すものより圧倒的に遅いものがその対象になるべきです。

static __isFilledPassword(password, isValidated, errors) {
  new Promise((resolve, reject) => {
    isValidated = false;
    errors.password = "passwordが未入力です。";
  });
}

passwordが変数が空か否かを確認するメソッドは非同期処理であるべきか？
と聞かれれば、別にHDDやネットワーク越しに問い合わせるわけではないので不要です。

要するにお前Promiseである必要ないじゃんって話ですし、
isFilledPasswordというメソッド/関数ならば、普通のエンジニアはtrue or falseが帰って来ることを想定するはずです。
この辺のメソッド名の名付けや挙動は英語力も必要になってきます。

Authenticatorクラスの中の話なので、例えばinvalidReasonsという配列が帰ってきそうなメソッド名にしてはいかが？
配列が空、つまりreasons.length === 0がtrueならば妥当なデータ、falseならば不正な値というわけです。
これなら死ぬほど使いやすくなったと思いませんか？

class Authenticator {
  static invalidReasons({username, password}) {
    const reasons = [];
    if (username === '') {
      reasons.push('usernameが未入力です');
    }
    if (password === '') {
      reasons.push('passwordが未入力です。');
    }
    return reasons;
  }
}

isValidated = false;

細かいですが、こういう使い方はバグの原因なので極力やめてください。
こういう使い方する関数を「副作用がある関数」と呼び、一定以上の技量のエンジニアには嫌われる使い方です。
パフォーマンス改善のテクニックの一つではあるのですが、これを意識せず使いまくったシステムだと単純にバグの量が100倍増えるでしょう、それだけのコストを払う意味があるケースは実際殆ど存在せず禁じ手になっています。

また、今回の用途では正しく動作しません。
何故かというとString、Number、Booleanなどのプリミティブ値は値渡しなので、関数の内側で引数として宣言したisValidated変数の内容は変わりますが、引数として渡した外側の世界の変数には影響しません。

var add3 = it => {
    it += 3;
    return it;
}
var a = 1;
console.log(add3(a)); // 4
console.log(a); // 1

オブジェクトや配列等のオブジェクト系はJavaScriptでは一見参照渡しのように思えますが、
実際には参照の値渡しなので、やはり束縛が切れるので無意味です。

var nameChange = (obj, name) => {
  obj = {name};
  return obj;
}
var taro = {name: 'taro'}
var jiro = nameChange(taro, 'jiro')
console.log(taro); // {name: 'taro'}
console.log(jiro); // {name: 'jiro'}

// 副作用を起こしたければこうする
var nameChange2 = (obj, name) => {
  obj.name = name; // 変数への代入ではなく、プロパティを書き換えるような挙動にする
  return obj;
}
var taro = {name: 'taro'}
var jiro = nameChange2(taro, 'jiro')
console.log(taro); // {name: 'jiro'} <- 副作用の再現に成功
console.log(jiro); // {name: 'jiro'}

// __isFilledUsername = username, isValidated, errors => new Promise() だとエラー

ES2015のクラス構文の書き方を復習しましょう
クラス - MDN

それと、アロー関数のルールでusername, isValidated, errors => new Promise()のように引数が複数になる場合は引数の部分のカッコは省略できません。
アロー関数でカッコを省略できるのは引数が1個のケースだけです。

アロー関数 - MDN

Comments

[rebel_nit]

解答ありがとうございました！
色んなサイトを見ながら見様見真似で書いたので、以下に自分がきちんと理解できていないかを実感しました。

Promiseやawaitの勉強を少ししたうえで書いてみたのですが、もしよければ追記にもお答えしていただけるととても助かります！

[rebel_nit]

追記への解答ありがとうございました。
とても丁寧に答えていただけたので、どこの理解が足りないのかというのがわかって調べやすかったです！
Node.jsも少しずつ勉強していくので、また困ったことが出てきた時に助けていただけると嬉しいです！