new Database ( incorrectPathToDB, { fileMustExist: true } ) does not throw error (while readOnly misspell does)

[Anatoly-IVANOV]

Unless I’m too sleep deprived and missing something obvious, I can’t find any logic to throw an error on incorrect DB file path (there’s only a partial check for directory existence "Cannot open database because the directory does not exist").

So the new class inst code proceeds to attempt to create a temp DB and look for a non-existent table inside. No error caught:

Does not work — no error caught

try {
  let db = new betterSQLite3( incorrectPathToDB, {
    readonly: false,
    fileMustExist: true,
  } );
}
catch ( error ) {
  console.log( 'Disaster. All is... Wait... we didn’t catch anything?' ) // not shown in the console
  console.log( error ); // nothing... silence
}

Proceeding to:

node_modules/better-sqlite3/lib/methods/wrappers.js:5
	return this[cppdb].prepare(sql, this, false);
	                   ^
SqliteError: no such table: test_table

Works — error caught

try {
  let db = new betterSQLite3( correctPathToDB, {
    readOnly: false,
    fileMustExist: true,
  } );
}
catch ( error ) {
  console.log( 'Disaster. All is lost. But we have the details:' )
  console.log( error ); // Misspelled option "readOnly" should be "readonly"
}

using the:

if ('readOnly' in options) throw new TypeError('Misspelled option "readOnly" should be "readonly"');

Related to:

#220

[Prinzhorn]

This works as expected for me:

const fs = require('fs');
const betterSQLite3 = require('better-sqlite3');

const incorrectPathToDB = 'nope.db'

try {
  console.log(fs.existsSync(incorrectPathToDB));
  let db = new betterSQLite3( incorrectPathToDB, {
    readonly: false,
    fileMustExist: true,
  } );
}
catch ( error ) {
  console.log( 'Disaster. All is... Wait... we didn’t catch anything?' ) // not shown in the console
  console.log( error ); // nothing... silence
}

> node index.js
false
Disaster. All is... Wait... we didn’t catch anything?
SqliteError: unable to open database file
    at new Database (/tmp/sql/node_modules/better-sqlite3/lib/database.js:51:26)
    at Object.<anonymous> (/tmp/sql/index.js:8:12)
    at Module._compile (internal/modules/cjs/loader.js:1068:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1097:10)
    at Module.load (internal/modules/cjs/loader.js:933:32)
    at Function.Module._load (internal/modules/cjs/loader.js:774:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
    at internal/main/run_main_module.js:17:47

Maybe your path is an empty string and you're creating a temporary database? Turns out this is not documented that an empty string does that (it's SQLite behavior) https://github.com/JoshuaWise/better-sqlite3/blob/02c9c250bc77311b08bec574d3d40890c0b17256/lib/database.js#L33

[Prinzhorn]

@JoshuaWise arguably if(fileMustExist && anonymous) should throw because they contradict and are incompatible options? Around here somewhere

https://github.com/JoshuaWise/better-sqlite3/blob/02c9c250bc77311b08bec574d3d40890c0b17256/lib/database.js#L39-L44

[JoshuaWise]

Also can't reproduce this. As @Prinzhorn said, probably incorrectPathToDB is an empty string or undefined, both of which cause SQLite3 to open a new temporary database (which means whatever table you're trying to access doesn't exist, because the database is empty).

[JoshuaWise]

@JoshuaWise arguably if(fileMustExist && anonymous) should throw because they contradict and are incompatible options? Around here somewhere

https://github.com/JoshuaWise/better-sqlite3/blob/02c9c250bc77311b08bec574d3d40890c0b17256/lib/database.js#L39-L44

Perhaps that could be a helpful error message.

[Anatoly-IVANOV]

@Prinzhorn & @JoshuaWise — Yes, if path is passed through an incorrectPathToDB var which is undefined, causes the "creation of a new temporary database (which means whatever table you're trying to access doesn't exist, because the database is empty)".

Exactly. 😀

In my view, that’s not the behavior described by fileMustExist: true.

IMO:

• at the very least — we’d want to log a Creating new empty DB message.
• better — with the fileMustExist: true, we would need to throw an error in any case except correct file found. Using, as @Prinzhorn suggested:

fs.existsSync(str_PathToDB)

My initial concern was raised when I just hardcoded a ./testAwesome.db as the first param, not even using a var.

@JoshuaWise ’s comment somewhere that "we don’t use any fancy path logic" inspired me to input an absolute path and the DB was correctly accessed without any complaints of inexisting tables.

[Anatoly-IVANOV]

OK, I drilled in a bit further down my commits… it turns out it is rather an edge case.

To very quickly test a try/catch, I add an inline or outside + 1 to a param (which might mess JS truthy/falsy, var type etc). Somehow, here, it does not throw an error.

To reproduce:

const correctPathToDB = '/Users/johndoe/Applications/GoodApp/perfect.db'
const incorrectPathToDB = correctPathToDB + 1;

  console.log( `Incorrect path is: ${incorrectPathToDB}` );
  console.log( `Var type is: ${typeof incorrectPathToDB}` ); // string, passing a 'Expected first argument to be a string' check

  try {
    console.log( fs.existsSync( incorrectPathToDB ) );
    let db = new betterSQLite3( incorrectPathToDB, {
      readonly: false,
      fileMustExist: true,
    } );
  }
  catch ( error ) {
    console.log( 'Disaster. All is... Wait... we didn’t catch anything?' ) // not shown in the console
    console.log( error ); // nothing... silence
  }

This silently creates a /Users/johndoe/Applications/GoodApp/perfect.db1 and then complains about inexisting tables.

My suggestion

• add a Creating new DB perfect.db1 message.… that would help understand the silence
• add the path format requirements for the DB file location param

In my case, a relative path did not work and I assembled an absolute using Node's path and url.

Assuming fantastic.db is in the root of the app’s folder:

import * as path from 'path';
import * as url from 'url';

/**
 * - The import.meta object exposes context-specific metadata to a JavaScript
 *   module. It contains the module's URL. 
 * - url.fileURLToPath() converts a URL into a OS-independent absolute path
 * - path.dirname() returns the directory of a path without a trailing slash
 */
const correctPathToDB = path.dirname( url.fileURLToPath(
  import.meta.url ) ) + '/' + 'perfect.db';

[JoshuaWise]

If you were able to reproduce the problem with a non-empty path argument, it sounds like this is a bug in your operating system. The fileMustExist option simply causes us to pass SQLITE_OPEN_READWRITE instead of (SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE) to SQLite's sqlite3_open_v2() C function. SQLite then converts those flags to filesystem flags. It looks like that's the part that's failing.

[Anatoly-IVANOV]

Woha! Thanks for the clear-up of the logic behind the fileMustExist option. Lemmy test from the SQLite command line tool when I get a second.

Meanwhile, would you be interested to add an if ( fs.existsSync(str_PathToDB) ) check as @Prinzhorn suggested above? That would bomb-proof the arg even further and avoid relying on SQLite’s C interface.

[deadcoder0904]

@JoshuaWise would love this to be solved?

i'm using better-sqlite3 with docker & its very confusing which folder does not exist.

i would love a better description of the error message than this one. having seen this error 100x today.

[deadcoder0904]

// Make sure the specified directory exists
if (!anonymous && !fs.existsSync(path.dirname(filename))) {
	throw new TypeError('Cannot open database because the directory ' + path.dirname(filename) + 'does not exist');
}

wouldn't this be enough here?

edit: i guess i can log the url before it gets called although the above would still be an improvement.

console.log(url)
const client = sqlite(url, { verbose: console.log })