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

http adapter: should PouchDB encode the database name? #5574

Closed
gr2m opened this issue Aug 17, 2016 · 8 comments · Fixed by #5580

Comments

@gr2m
Copy link
Contributor

commented Aug 17, 2016

Let’s say I want to create a database called user/123 with PouchDB using the http adapter.

var dbName = 'user/123'
var MyPouchDB = PouchDB.defaults({prefix: 'http://localhost:5984/'})
MyPouchDB.plugin(require('pouchdb-adapter-http'))

var db = new MyPouchDB(dbName)
db.info().catch(console.log.bind(console))
// logs not_found error

The problem is that it sends a request to GET http://localhost:5984/user/123/, it is not encoding the database name.

I would expect this to happen within PouchDB, instead of me needing to do

var db = new MyPouchDB(encodeURIComponent(dbName))

What do you think?

@gr2m gr2m referenced this issue Aug 17, 2016
Merged
5 of 5 tasks complete
@gr2m

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2016

I think it’s as simple as a encodeURIComponent(opts.db) in https://github.com/pouchdb/pouchdb/blob/master/packages/pouchdb-adapter-http/src/index.js#L106

Would you accept a pull request for that? Note this might potentially break people’s code, in case they already encoded their database names

@gr2m

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2016

this is related: 94c7276

@gr2m

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2016

Okay I think I got it, PR incoming 🚀

@daleharvey

This comment has been minimized.

Copy link
Member

commented Aug 19, 2016

As far as I remember, we require the user to encode the / because I am not sure there is a way for us to differentiate when the / is part of the name, or when couchdb is hosted under a path, very possible I am misremembering something though

@KlausTrainer

This comment has been minimized.

Copy link
Member

commented Aug 19, 2016

What about using something like this in order to check if the database name` is already URI encoded:

function isURIEncoded(dbName) {
  return !(dbName === decodeURIComponent(dbName));
}
gr2m added a commit that referenced this issue Aug 19, 2016
@daleharvey

This comment has been minimized.

Copy link
Member

commented Aug 19, 2016

That doesnt address the issue, how to we differentiate new PouchDB('http://foo.com/user/123'); from new PouchDB("http://foo.com/db/mydatabase")?

@gr2m

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2016

see my pull request here: #5580

It only encodes the database name if options.prefix is a URL, which I guess makes sense, and it resolves the issue for us at Hoodie, where we just pass on a PouchDB constructor that should always behave the same, no matter what adapter is used

nolanlawson added a commit that referenced this issue Aug 20, 2016
@nolanlawson

This comment has been minimized.

Copy link
Member

commented Aug 20, 2016

fixed by #5580

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.