Conversation
29880d7 to
1c401fa
Compare
1c401fa to
e91e6c1
Compare
There was a problem hiding this comment.
callback/not_exist_error.js と出力内容が一致していないです。
There was a problem hiding this comment.
@cafedomancer
async/awaitについても出力内容が一致していなかったため、全て揃えました。
エラーなしバージョンの各ファイル出力
callback
chiroru@MacBook-Pro-4 callback % node not_exist_error.js
メモリ内のSQLiteデータベースに接続しました。
テーブルが作成されました。
行が追加されました。id: 1
1: Node.js入門
テーブルが削除されました。
データベース接続を閉じました。
promise
chiroru@MacBook-Pro-4 promise % node not_exist_error.js
メモリ内のSQLiteデータベースに接続しました。
テーブルが作成されました。
行が追加されました。id: 1
1: Node.js入門
テーブルが削除されました。
データベース接続を閉じました。
async/await
chiroru@MacBook-Pro-4 async_await % node not_exist_error.js
メモリ内のSQLiteデータベースに接続しました。
テーブルが作成されました。
行が追加されました。id: 1
1: Node.js入門
テーブルが削除されました。
データベース接続を閉じました。
There was a problem hiding this comment.
callback/exist_error.js と出力内容が一致していないです。
There was a problem hiding this comment.
@cafedomancer
3ファイルの実行結果を全て出力が同じになるようにしました。
エラーありバージョンの各ファイル出力
callback
chiroru@MacBook-Pro-4 callback % node exist_error.js
メモリ内のSQLiteデータベースに接続しました。
テーブルが作成されました。
行が追加されました。id: 1
エラーが発生しました: SQLITE_CONSTRAINT: UNIQUE constraint failed: books.title
データベース接続を閉じました。
promise
chiroru@MacBook-Pro-4 promise % node exist_error.js
メモリ内のSQLiteデータベースに接続しました。
テーブルが作成されました。
行が追加されました。id: 1
エラーが発生しました: SQLITE_CONSTRAINT: UNIQUE constraint failed: books.title
データベース接続を閉じました。
async/await
chiroru@MacBook-Pro-4 async_await % node exist_error.js
メモリ内のSQLiteデータベースに接続しました。
テーブルが作成されました。
行が追加されました。id: 1
エラーが発生しました: SQLITE_CONSTRAINT: UNIQUE constraint failed: books.title
データベース接続を閉じました。
There was a problem hiding this comment.
ファイル名は名詞形になるようにしてください。exist は動詞なので、名詞である error を修飾することはできません。
There was a problem hiding this comment.
@cafedomancer
エラーが発生しないファイル名をnonexistent_error.js、
エラーが発生するファイル名をexisting_error.jsとしました。
どちらも形容詞+名詞で、ファイル名が適切な名詞形になったと思われます。
| import sqlite3 from "sqlite3"; | ||
|
|
||
| const db = new sqlite3.Database(":memory:", () => { | ||
| process.stdout.write("メモリ内のSQLiteデータベースに接続しました。\n"); |
There was a problem hiding this comment.
process.stdout.write ではなく console.log を使ってください。
There was a problem hiding this comment.
@cafedomancer
前提:xxxのコミットでデータベース接続の処理はdb.jsにまとめました。
db.js
export const db = new sqlite3.Database(":memory:", () => {
console.log("メモリ内のSQLiteデータベースに接続しました。");
});db.js内でこちらの対応を行いました。
| const db = new sqlite3.Database(":memory:", () => { | ||
| process.stdout.write("メモリ内のSQLiteデータベースに接続しました。\n"); | ||
| }); | ||
|
|
||
| db.run( | ||
| `CREATE TABLE IF NOT EXISTS books (id INTEGER PRIMARY KEY AUTOINCREMENT, title TEXT NOT NULL UNIQUE)`, | ||
| () => { | ||
| process.stdout.write("テーブルが作成されました。\n"); | ||
|
|
||
| db.run( | ||
| `INSERT INTO books (title) VALUES (?)`, | ||
| ["Node.js入門"], | ||
| function () { | ||
| process.stdout.write(`行が追加されました。id: ${this.lastID}\n`); | ||
|
|
||
| db.all(`SELECT id, title FROM books`, [], (err, rows) => { | ||
| rows.forEach((row) => { | ||
| process.stdout.write(`${row.id}: ${row.title}\n`); | ||
| }); | ||
|
|
||
| db.run(`DROP TABLE books`, () => { | ||
| process.stdout.write("テーブルが削除されました。\n"); | ||
|
|
||
| db.close(() => { | ||
| process.stdout.write("データベース接続を閉じました。\n"); | ||
| }); | ||
| }); | ||
| }); | ||
| }, | ||
| ); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
4 行目の時点でデータベースへの接続が完了するのであれば、7 行目の以降の処理はデータベースへの接続を待たずに処理を行っていることになります。
There was a problem hiding this comment.
以下のように、db.jsにデータベース接続の処理をまとめたことによって、データベース接続が完了した後にテーブル作成以降の処理が走ることになります。
db.jsの一部
import sqlite3 from "sqlite3";
export const db = new sqlite3.Database(":memory:", () => {
console.log("メモリ内のSQLiteデータベースに接続しました。");
});callback/nonexistent_error.jsの1〜6行目
import { db } from "../db.js";
db.run(
`CREATE TABLE IF NOT EXISTS books (id INTEGER PRIMARY KEY AUTOINCREMENT, title TEXT NOT NULL UNIQUE)`,
() => {
process.stdout.write("テーブルが作成されました。\n");
03.asynchronous/db.js
Outdated
| if (err) { | ||
| reject(err); | ||
| } else { | ||
| resolve({ lastID: this.lastID, changes: this.changes }); |
There was a problem hiding this comment.
{ lastID: this.lastID, changes: this.changes } は this そのものだと思います。
There was a problem hiding this comment.
@cafedomancer
すみません、こちらのご指摘についてですが、何に対して指摘されているのかが理解できなかったのでもう少し詳細にご指摘いただきたいです🙇♂️
このご指摘で指摘内容を理解できないということは何か知っておかなければならない知識が抜けてるということでしょうか。
There was a problem hiding this comment.
this に格納されている値を調べれば、何を言っているのか分かると思います。それでも分からなければ、また聞いてください。
There was a problem hiding this comment.
@cafedomancer
thisに格納されている値をログに出したところ、Statement { lastID: 1, changes: 1 }と値が返ってきました。
この内容が{ lastID: this.lastID, changes: this.changes }と同じということだと理解しました。
なので、resolve時に渡す値をthisのみにしました。
動作確認も行い、正常に動作することも確認しました。
run(sql, params = []) {
return new Promise((resolve, reject) => {
this.db.run(sql, params, function (err) {
if (err) {
reject(err);
} else {
resolve(this);
}
});
});
}
03.asynchronous/db.js
Outdated
| } | ||
|
|
||
| export function close() { | ||
| db.close(); |
There was a problem hiding this comment.
@cafedomancer
Promiseとしてcloseを扱えるように修正いたしました。
close() {
return new Promise((resolve, reject) => {
this.db.close((err) => {
if (err) {
reject(err);
} else {
resolve();
}
});
});
}
03.asynchronous/db.js
Outdated
| db.close(); | ||
| process.stdout.write("データベース接続を閉じました。\n"); |
There was a problem hiding this comment.
31 行目の実行時点では、close の処理は必ずしも完了していないはずです。また、close したときにどのような処理を行うか (例えばメッセージを出力するなど) は、関数の呼び出し側で自由に行えるようになっているべきです。
There was a problem hiding this comment.
@cafedomancer
各ファイル内(promise,async/await・エラーあり,エラーなし)でclosedメソッドの処理が終了した後に「データベース接続を閉じました。」の文言が出力されるようにしました。
| @@ -0,0 +1,29 @@ | |||
| import { run, all, close } from "../db.js"; | |||
|
|
|||
| async function manageBooks() { | |||
There was a problem hiding this comment.
await を使うために関数を定義しているのであれば、そうする代わりに Top-level await を使ってください。
There was a problem hiding this comment.
@cafedomancer
こちらの記事を参考にTop-level awaitについて理解し、Top-level awaitを用いて実装し直しました。
エラーあり・エラーなしの2つとも修正いたしました。
03.asynchronous/db.js
Outdated
There was a problem hiding this comment.
このファイルに定義されているのはデータベースそのもの (db) だけではないと思います。
There was a problem hiding this comment.
@cafedomancer
こちらに関しても指摘の意図が汲み取れなかったのですが、「ファイル名が適切でない」と推測しましたのでファイル名をdb.jsからdb_operations.jsに変更しました。
それに伴い各ファイルのimport文も修正いたしました。
There was a problem hiding this comment.
ファイル名に対してコメントを残していますよ。対応としてはそれで良いですー。
There was a problem hiding this comment.
shebang を付けてもファイルに実行権限が付与されていなければ意味がないですよ。どちらでもいいですが、shebang なし・実行権限なしに統一する、shebang あり、実行権限ありに統一する、のどちらかの対応を行ってください。
03.asynchronous/db_operations.js
Outdated
| @@ -0,0 +1,49 @@ | |||
| import sqlite3 from "sqlite3"; | |||
|
|
|||
| export const createDatabase = () => { | |||
There was a problem hiding this comment.
元の sqlite3.Database 関数ではデータベースファイルのパスを指定できるようになっていたはずです。
There was a problem hiding this comment.
@cafedomancer
すみません。「元の sqlite3.Database 関数ではデータベースファイルのパスを指定できるようになっていたはずです。」の文章から何を修正したら良いのかが推測できなかったので、もう少しブレイクダウンして説明していただきたいです🙇♂️
There was a problem hiding this comment.
sqlite3.Database() 関数の元の機能を維持してください。つまり、sqlite3.Database() 関数のインタフェースと自分で定義した関数のインタフェースを一致させてください。
There was a problem hiding this comment.
@cafedomancer
引数にfilenameをとるようにし、呼び出す側でインメモリデータベースを指定するように修正しました。
引数名のfilenameは公式ドキュメントで記載されている文言と一致させました。
https://github.com/TryGhost/node-sqlite3/wiki/API
createDatabase
export function createDatabase(filename) {
return new Promise((resolve, reject) => {
const db = new sqlite3.Database(filename, (err) => {
if (err) {
reject(err);
} else {
resolve(db);
}
});
});
}promise
createDatabase(":memory:")
.then((resolvedDB) => {
.....async/await
let db = await createDatabase(":memory:");
.....|
|
||
| import { createDatabase, run, all, close } from "../db_operations.js"; | ||
|
|
||
| let db = await createDatabase(); |
There was a problem hiding this comment.
この変数は再代入されないので let で定義する必要はないですね。
There was a problem hiding this comment.
@cafedomancer
constに変更しました。
const db = await createDatabase();| await run(db, "INSERT INTO books (title) VALUES (?)", ["Node.js入門"]); | ||
| } catch (err) { | ||
| if ( | ||
| String(err).includes( |
There was a problem hiding this comment.
以下のような文字列を例外として送出すると条件にマッチしてしまうと思いますよ。
try {
throw "The following error has not occurred: UNIQUE contraint failed";
} catch (...) {
...
}There was a problem hiding this comment.
@cafedomancer
条件はSQLITE_CONSTRAINT: UNIQUE constraint failed: books.titleが含まれているかであり、The following error has not occurred: UNIQUE contraint failedは期待するエラー文と異なるので条件にマッチしないかと思います。
実際に上記の例外を創出させてみました。
対象部分のコード
try {
throw "The following error has not occurred: UNIQUE contraint failed";
} catch (err) {
if (
String(err).includes(
"SQLITE_CONSTRAINT: UNIQUE constraint failed: books.title",
)
) {
console.error(`エラーが発生しました: ${String(err)}`);
} else {
console.error(`期待外のエラーが発生しました: ${String(err)}`);
throw err;
}
}実行結果
chiroru@MacBook-Pro-4 async_await % ./existing_error.js
メモリ内のSQLiteデータベースに接続しました。
テーブルが作成されました。
期待外のエラーが発生しました: The following error has not occurred: UNIQUE contraint failed
データベース接続を閉じました。
node:internal/modules/run_main:129
triggerUncaughtException(
^
The following error has not occurred: UNIQUE contraint failed
(Use `node --trace-uncaught ...` to show where the exception was thrown)
Node.js v20.17.0
There was a problem hiding this comment.
ああ、すみません、文字列に誤りがありましたが、文字列の中身は重要ではないです。以下だとマッチするはずです。
try {
throw "The following error has not occurred: SQLITE_CONSTRAINT: UNIQUE constraint failed: books.title";
} catch (...) {
...
}There was a problem hiding this comment.
- SQLiteのエラーNoも判定材料に入れることで予期せぬマッチを避けるようにしました。
try {
const result = await run(db, "INSERT INTO books (title) VALUES (?)", [
"Node.js入門",
]);
console.log(`行が追加されました。id: ${result.lastID}`);
await run(db, "INSERT INTO books (title) VALUES (?)", ["Node.js入門"]);
} catch (err) {
if (
err.errno === 19 &&
String(err).includes(
"SQLITE_CONSTRAINT: UNIQUE constraint failed: books.title",
)
) {
console.error(`エラーが発生しました: ${String(err)}`);
} else {
throw err;
}
}実行結果
chiroru@MacBook-Pro-4 async_await % ./existing_error.js
メモリ内のSQLiteデータベースに接続しました。
テーブルが作成されました。
行が追加されました。id: 1
エラーが発生しました: Error: SQLITE_CONSTRAINT: UNIQUE constraint failed: books.title
エラーが発生しました: SQLITE_ERROR: no such table: nonexistent_table
テーブルが削除されました。
データベース接続を閉じました。
chiroru@MacBook-Pro-4 async_await %
記載していただいた文での検証
- スローした文で条件にマッチしておらず、期待通りに条件分岐ができている。
try {
throw "The following error has not occurred: SQLITE_CONSTRAINT: UNIQUE constraint failed: books.title";
} catch (err) {
if (
err.errno === 19 &&
String(err).includes(
"SQLITE_CONSTRAINT: UNIQUE constraint failed: books.title",
)
) {
console.error(`エラーが発生しました: ${String(err)}`);
} else {
throw err;
}
}実行結果
chiroru@MacBook-Pro-4 async_await % ./existing_error.js
メモリ内のSQLiteデータベースに接続しました。
テーブルが作成されました。
データベース接続を閉じました。
node:internal/modules/run_main:129
triggerUncaughtException(
^
The following error has not occurred: SQLITE_CONSTRAINT: UNIQUE constraint failed: books.title
(Use `node --trace-uncaught ...` to show where the exception was thrown)
Node.js v20.17.0
chiroru@MacBook-Pro-4 async_await %
There was a problem hiding this comment.
ちょっと極端ですが、以下のようなコードだと条件にマッチしてしまうと思いますよ。やみくもに条件を増やすのではなく、根本的な対応を考えてください。
class MyString extends String {
constructor(errno, string) {
super(string);
this.errno = errno;
}
}
try {
throw new MyString(19, "The following error has not occurred: SQLITE_CONSTRAINT: UNIQUE constraint failed: books.title");
} catch (...) {
...
}There was a problem hiding this comment.
@cafedomancer
捕捉するエラーにtitleの制約を設けて今回のケース以外にエラーが捕捉されないように修正しました。
const title = "Node.js入門";
try {
const result = await run(db, "INSERT INTO books (title) VALUES (?)", [
title,
]);
console.log(`行が追加されました。id: ${result.lastID}`);
await run(db, "INSERT INTO books (title) VALUES (?)", [title]);
} catch (err) {
if (
title === "Node.js入門" &&
String(err).includes(
"SQLITE_CONSTRAINT: UNIQUE constraint failed: books.title",
)
) {
console.error(`エラーが発生しました: ${String(err)}`);
} else {
throw err;
}
}| } catch (err) { | ||
| console.error(`エラーが発生しました: ${err.message}`); | ||
| } |
There was a problem hiding this comment.
@cafedomancer
insertの処理同様、期待するエラーのみを捕捉するように修正しました。
try {
await all(db, "SELECT * FROM nonexistent_table WHERE id = ?", [-1]);
} catch (err) {
if (
String(err).includes("SQLITE_ERROR: no such table: nonexistent_table")
) {
console.error(`エラーが発生しました: ${err.message}`);
} else {
throw err;
}
}| .then((resolvedDB) => { | ||
| console.log("メモリ内のSQLiteデータベースに接続しました。"); | ||
| return run( | ||
| (db = resolvedDB), |
There was a problem hiding this comment.
- ブロック文体ではなく簡潔文体を使用するようにしました。
- 引数の中で変数の代入を行わないようにしました。
.then((resolvedDB) => {
console.log("メモリ内のSQLiteデータベースに接続しました。");
db = resolvedDB;
})
.then(() =>
run(
db,
"CREATE TABLE books (id INTEGER PRIMARY KEY AUTOINCREMENT, title TEXT NOT NULL UNIQUE)",
),
)| .finally(() => { | ||
| return close(db); | ||
| }) |
There was a problem hiding this comment.
@cafedomancer
こちらの部分も簡潔文体に修正しました。
.finally(() => close(db))| .finally(() => { | ||
| close(db); | ||
| }) |
There was a problem hiding this comment.
| await run(db, "INSERT INTO books (title) VALUES (?)", [title]); | ||
| } catch (err) { | ||
| if ( | ||
| title === "Node.js入門" && |
There was a problem hiding this comment.
これは例外とは関係のない条件だと思います。例外に関してだけ条件判定を行ってください。どのように対応すべきかは調べればいくらでも出てくると思いますよー。
There was a problem hiding this comment.
どのように対応すべきかは調べればいくらでも出てくると思いますよー。
「js catch エラー捕捉範囲」やMDNのtry...catchのページを見たのですが、今回に活かせるような記事を見つけることができなかったので、エラーの発生方法を変えて対応しました。
存在しないカラムにデータを入れようとしてエラーを起こしています。
また、今回のケースだけでなく他にも存在しないカラムに対してデータ操作が行われようとした際にも対応できるように、エラー文のカラム名の部分は動的にしました。
try {
const result = await run(db, "INSERT INTO books (title) VALUES (?)", [
"Node.js入門",
]);
console.log(`行が追加されました。id: ${result.lastID}`);
await run(db, "INSERT INTO books (nonexistent_column) VALUES (?)", [
"Node.js入門",
]);
} catch (err) {
const columnError = err.message.match(
/SQLITE_ERROR: table books has no column named (\w+)/,
);
if (columnError) {
console.error(`エラーが発生しました: ${columnError[0]}`);
} else {
throw err;
}
}There was a problem hiding this comment.
MDN の try...catch のページを読み直した方がよいかと。
There was a problem hiding this comment.
MDN の try...catch のページを読み直した方がよいかと。
やはりどの部分が今回に活かせるかがわからなかったのですが、どの部分が今回のケースの回稀有に役立つ部分かを教えていただきたいです。
一旦、同じ名前のデータを入れようとしてエラーを意図的に起こすのではなく、存在しないカラムに対してデータを追加しようとすることでエラーを起こすようにして修正しました。
There was a problem hiding this comment.
以下の部分です。
よくある使用例としては、次のように想定済みの一部のエラーのみを捕捉(および無視)し、それ以外の場合はエラーを送出し直す場合です。
一旦、同じ名前のデータを入れようとしてエラーを意図的に起こすのではなく、存在しないカラムに対してデータを追加しようとすることでエラーを起こすようにして修正しました。
不要な修正なので差し戻してください。
There was a problem hiding this comment.
@cafedomancer
以下のコードのように3つの条件が今回期待するエラー(ユニーク制約へのエラー)なので、それを捕捉するのに必要な条件をif文にかきました。
sqlite3のエラーのinstanceofはErrorのみということで、それに加えてerr.codeとエラーメッセージ内の文章を組み合わせて条件としました。
} catch (err) {
if (
err instanceof Error &&
err.code === "SQLITE_CONSTRAINT" &&
err.message.includes("UNIQUE constraint failed")
) {
console.error(`エラーが発生しました: ${err.message}`);
} else {
throw err;
}da667e1 to
a740307
Compare
|
必要以上にadd・commitの解除を行ってしまったため、force-pushを行いました🙇♂️ |
| if ( | ||
| String(err).includes("SQLITE_ERROR: no such table: nonexistent_table") | ||
| ) { |
There was a problem hiding this comment.
@cafedomancer
修正漏れすみませんでした🙇♂️
try {
await all(db, "SELECT * FROM nonexistent_table WHERE id = ?", [-1]);
} catch (err) {
if (
err instanceof Error &&
err.code === "SQLITE_ERROR" &&
err.message.includes("no such table")
) {
console.error(`エラーが発生しました: ${err.message}`);
} else {
throw err;
}
}
プラクティス
非同期処理(JavaScript)
実行結果
(※) エラーはbooksテーブルののtitleカラムにユニーク制約がかかっているところに同じ本の名前を登録しようとして意図的に起こしている
Callback
エラーなし
エラーあり
Promise
エラーなし
エラーあり
async/await
エラーなし
エラーあり
ESlint実行
Prettier実行
使用方法
ローカル環境にクローンする
当ブランチに移動
パッケージインストール
実行