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

Fixed URL parsing of replsets #58

Merged
merged 1 commit into from
Apr 17, 2014
Merged

Conversation

Qard
Copy link

@Qard Qard commented Mar 20, 2014

Fixes replset and auth parsing issues, as described in #19.

@hallas
Copy link

hallas commented Apr 1, 2014

Yes please 👍

@rauchg
Copy link
Contributor

rauchg commented Apr 17, 2014

Can you remove the package.json changes please?

@Qard
Copy link
Author

Qard commented Apr 17, 2014

Ah, yep I'll fix that tonight. I didn't even notice it did that.

I just did npm i --save mongodb-url-parser and didn't realize the formatting was different.

@tj
Copy link
Contributor

tj commented Apr 17, 2014

nicer formatting anyway :D legacy comma-first days

@tj
Copy link
Contributor

tj commented Apr 17, 2014

looks like the new mongoskin does this for you I think, or mongodb-native, can't read the damn code so it's hard to see wtf is going on but I'm going to try it with this stuff removed

@hallas
Copy link

hallas commented Apr 17, 2014

ok, would be nice to have this in

rauchg added a commit that referenced this pull request Apr 17, 2014
Fixed URL parsing of replsets
@rauchg rauchg merged commit 0e02c86 into Automattic:master Apr 17, 2014
@rauchg
Copy link
Contributor

rauchg commented Apr 17, 2014

Tests fail

․․․․․․

  43 passing (597ms)
  1 failing

  1) monk connection to a replica set (string):
     Error: database names cannot contain the character ' '
      at validateDatabaseName (/Users/guillermorauch/Projects/monk/node_modules/mongoskin/node_modules/mongodb/lib/mongodb/db.js:241:59)
      at new Db (/Users/guillermorauch/Projects/monk/node_modules/mongoskin/node_modules/mongodb/lib/mongodb/db.js:79:3)
      at SkinServer.db (/Users/guillermorauch/Projects/monk/node_modules/mongoskin/lib/mongoskin/server.js:49:14)
      at Object.exports.db (/Users/guillermorauch/Projects/monk/node_modules/mongoskin/lib/mongoskin/index.js:163:21)
      at new Manager (/Users/guillermorauch/Projects/monk/lib/manager.js:74:27)
      at Manager (/Users/guillermorauch/Projects/monk/lib/manager.js:33:12)
      at Context.<anonymous> (/Users/guillermorauch/Projects/monk/test/monk.test.js:34:7)
      at Test.Runnable.run (/Users/guillermorauch/Projects/monk/node_modules/mocha/lib/runnable.js:196:15)
      at Runner.runTest (/Users/guillermorauch/Projects/monk/node_modules/mocha/lib/runner.js:374:10)
      at /Users/guillermorauch/Projects/monk/node_modules/mocha/lib/runner.js:452:12
      at next (/Users/guillermorauch/Projects/monk/node_modules/mocha/lib/runner.js:299:14)
      at /Users/guillermorauch/Projects/monk/node_modules/mocha/lib/runner.js:309:7
      at next (/Users/guillermorauch/Projects/monk/node_modules/mocha/lib/runner.js:247:23)
      at Object._onImmediate (/Users/guillermorauch/Projects/monk/node_modules/mocha/lib/runner.js:276:5)
      at processImmediate [as _immediateCallback] (timers.js:330:15)

@diorahman
Copy link
Contributor

To be valid, the connection string should be: 127.0.0.1,localhost/monk-test [1,2]

  1. http://docs.mongodb.org/manual/reference/connection-string/
  2. https://www.npmjs.org/package/mongodb-uri

@Qard
Copy link
Author

Qard commented Apr 21, 2014

As diorahman said, the connection string used in the test is not a valid mongodb connection string. Perhaps monk or mongoskin had translated it to something valid until now, but mongodb-url-parser intends to follow the official url spec. I guess it's up to you whether or not you want to break compatibility with that particular use--I don't suspect it's common--or revert.

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

Successfully merging this pull request may close these issues.

6 participants