Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pouchdb-adapter-leveldown does not assure folders #5668

Closed
gr2m opened this issue Sep 10, 2016 · 5 comments
Closed

pouchdb-adapter-leveldown does not assure folders #5668

gr2m opened this issue Sep 10, 2016 · 5 comments
Assignees

Comments

@gr2m
Copy link
Contributor

gr2m commented Sep 10, 2016

Issue

I would assume that if I create a new database containing a path separator like new PouchDB('foo/bar') then it’s the responsibility of the adapters to make sure that the foo folder gets created before it tries to write files, but it doesn’t seem to be the case right now?

Would you accept a PR to fix that, or is that by design?

Info

  • Environment: Node.js
  • PouchDB Version: 6.0.4
  • Adapter: LevelDB

Reproduce

var PouchDB = require('pouchdb-core').plugin(require('pouchdb-adapter-leveldb'))
new PouchDB('bar/baz').info().catch(console.log)
// OpenError: IO error: bar/baz/LOCK: No such file or directory
//     at ./node_modules/levelup/lib/levelup.js:119:34
//     at ./node_modules/leveldown/node_modules/abstract-leveldown/abstract-leveldown.js:39:16

Workaround

var pathResolve = require('path').resolve
var mkdirp = require('mkdirp')
var PouchDB = require('pouchdb-core')
  .plugin(require('pouchdb-adapter-leveldb'))
  .defaults({prefix: 'path/to/data/'})

var db = new state.PouchDB(name)
db.info()
.catch(function (error) {
  if (error.name === 'OpenError') {
    var path = db.__opts.prefix ? db.__opts.prefix + name : name

    return new Promise(function (resolve, reject) {
      mkdirp(pathResolve(path), function (error) {
        if (error) {
          return reject(error)
        }

        resolve()
      })
    })

    .then(function () {
      // reusing the db instance from above does not work for me
      return new state.PouchDB(name).info()
    })
  }

  throw error
})
gr2m added a commit to pouchdb/express-pouchdb that referenced this issue Sep 10, 2016
This commit includes a workaround for pouchdb/pouchdb#5668

This is a breaking change. If your app used the leveldb adapter (or another adapter that stores files) and you have database names containing `/`, then your app won’t be able to find the existing databases, as they are in folders like `user%2Fabc4567` instead of nested folders like `user/abc4567`. To migrate, look into the folder where you save your databases into and rename all database folders accordingly
gr2m added a commit to gr2m/express-pouchdb that referenced this issue Sep 10, 2016
This commit includes a workaround for pouchdb/pouchdb#5668

This is a breaking change. If your app used the leveldb adapter (or another adapter that stores files) and you have database names containing `/`, then your app won’t be able to find the existing databases, as they are in folders like `user%2Fabc4567` instead of nested folders like `user/abc4567`. To migrate, look into the folder where you save your databases into and rename all database folders accordingly
gr2m added a commit to hoodiehq/hoodie-store-server that referenced this issue Sep 10, 2016
gr2m added a commit to pouchdb/express-pouchdb that referenced this issue Sep 11, 2016
This commit includes a workaround for pouchdb/pouchdb#5668

This is a breaking change. If your app used the leveldb adapter (or another adapter that stores files) and you have database names containing `/`, then your app won’t be able to find the existing databases, as they are in folders like `user%2Fabc4567` instead of nested folders like `user/abc4567`. To migrate, look into the folder where you save your databases into and rename all database folders accordingly
gr2m added a commit to hoodiehq/hoodie-store-server that referenced this issue Sep 11, 2016
nolanlawson pushed a commit to pouchdb/express-pouchdb that referenced this issue Sep 11, 2016
* (#374) update to PouchDB@6

* (#274): remove incorret dbname encoding

This commit includes a workaround for pouchdb/pouchdb#5668

This is a breaking change. If your app used the leveldb adapter (or another adapter that stores files) and you have database names containing `/`, then your app won’t be able to find the existing databases, as they are in folders like `user%2Fabc4567` instead of nested folders like `user/abc4567`. To migrate, look into the folder where you save your databases into and rename all database folders accordingly
@nolanlawson
Copy link
Member

I don't see any reason not to do the mkdirp. 👍

@daleharvey
Copy link
Member

It has come up a few times before but I purposefully left it out, I think its the responsibility of the backend to ensure this type of functionality, in particular whoever chooses where to store the data should be the one to ensure that it exists, otherwise we will be creating directories when not needed / things go out of sync etc

I think added support for this should be done in leveldown, or at least we should understand why leveldown doesnt do it (maybe it could be an option we set by default)

@nolanlawson
Copy link
Member

Yeah I'm also fine with just punting on this. However I think a defensive mkdirp is pretty costless.

@gr2m
Copy link
Contributor Author

gr2m commented Oct 24, 2016

It has come up a few times before but I purposefully left it out, I think its the responsibility of the backend to ensure this type of functionality, in particular whoever chooses where to store the data should be the one to ensure that it exists, otherwise we will be creating directories when not needed / things go out of sync etc

On second thought, I disagree when it comes to database names with with a /. This is not about the prefix option, it’s when people want to create a database like user/uuid567. CouchDB will also create a folder user in that case and a uuid567.couch file inside it. I can’t think of a case where we would create directories when not needed?

I agree that we should investigate why this is not part of leveldown though

@nolanlawson
Copy link
Member

Users can do this fairly easily themselves, not having to include mkdirp is one less dependency for us, gonna close for now

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

No branches or pull requests

3 participants