Recursive directory-creation within database creation

[19h]

We should be aware of a case where a database is located in a location which is not existing in the first place.

Considering, we're trying to open a database:

root = require("levelup")("./db/root.db");

whereas ./db/ is not existing.

A dubious error is thrown, but does not explain the allocation is invalid because the directory itself is not existing:

OpenError: IO error: ./db/root.db/LOCK: No such file or directory
    at [..]/levelup.js:113:25

I propose a reverse-lookup of the directory-path (cherry-picking ftw) and either an error being thrown explaining the problem further or the directories to the target path being created recursively.

[mcollina]

👍
I further suggest the use of the awesome mkdirp: https://npmjs.org/package/mkdirp.

[19h]

It would be easy to implement this into LevelUP, however, considering a typo in the levelUP creation call will create the database-directory unexpectedly.

IMHO throwing an error is the only way to not raise confusion and create a regression to possible applications.

That is, we could simply do:

path = "./path/to/database/foo.db";
path_stack = [];
for ( file in (path = path.split("/")) ) {
    path_stack.push(path[file]);
    if ( !fs.existsSync(path[file]) )
        throw "Error: " + path_stack.join("/") + " does not exist."
}

We could also accept an override flag creating the directories in case they are inexistent:

level("/dev/hdfs/foo", {
    createIfPathInexistent: true
})

Objections?

[mcollina]

IMHO I'll create it anyway, with an inverted override flag:

level("/dev/hdfs/foo", {
    createIfPathInexistent: false
})

But that's only personal taste.

[19h]

Yes, I understand the approach you're proposing. But I'd rather target a change which would not change existing applications of the case (thus the non-inverted flag) — you can never estimate the impact of a change which maybe causing a regression.

[juliangruber]

I'd say out of scope, throwing errors is ok and creating directories beforehand isn't complicated.

What we could is catch that error, check if the given path doesn't exist and then print a nicer message.

[rvagg]

further to my comments in #136, if path-existence is a problem for your particular use-case I would strongly recommend using mkdirp over your path before passing it to LevelUP as @mcollina suggested. It's nice and clean and will not error if the directory already exists.

[19h]

OK. Let's talk with the LevelDB guys about this. I started a discussion by creating an issue here. (Although I should have broken down the error-message to LevelDB-only.. copy and paste ftw)

[luk-]

In the linked issue, you make it sound as if the node leveldb community is
behind the idea of the feature being included in core leveldb, when it's
just you and another individual.

A feature like this makes zero sense for a production environment, and the
extra code adds unnecessary complexity.

Please consider updating the issue on google code to clarify that it is
only you looking for the feature. The people who work on that project are
very busy, and this is a waste of their time.

On Saturday, May 11, 2013, Kenan Sulayman wrote:

OK. Let's talk with the LevelDB guys about this. I started a discussion by
creating an issue herehttps://code.google.com/p/leveldb/issues/detail?id=167&can=4
.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/135#issuecomment-17757389
.

[19h]

Actually, we're using this in production. Also, this was an issue; "waste of their time" is distracting. I'll update the issue accordingly, though.

[mcollina]

@KenanSulayman as you are using this in production, you can:

levelup(mkdirp.sync("/path/to/nowhere/))

We can even put this solution as a FAQ in the readme.

[19h]

We're even using the mkdirp code [https://github.com/KenanSulayman/node-levelup/commit/84ef555306860186e02fdfb298f8745068a79801 Line 69+] as I implemented in (#136), your solution is far simpler.. but I already deployed the code :/ Thanks though! 👍