Remove port when using mongodb+srv #497

jareeve · 2019-02-06T17:37:28Z

Description

When using protocol mongodb+srv the port must not be set in the connection url.

Related issues

#496

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide

slnode · 2019-02-06T17:37:30Z

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

dhmlau · 2019-02-07T04:15:38Z

@jareeve, thanks for the PR. Could you please sign the CLA https://cla.strongloop.com/agreements/strongloop/loopback-connector-mongodb? Thanks.

dhmlau · 2019-02-07T04:15:54Z

lib/mongodb.js

@@ -57,6 +57,11 @@ function generateMongoDBURL(options) {
  options.port = options.port || 27017;
  options.database = options.database || options.db || 'test';
  var username = options.username || options.user;
+  var portUrl = ''


missing the ending ;

added the ; and linter is now passing

jareeve · 2019-02-07T11:05:49Z

Looks like travis is failing with:

1 failing

mongodb connector
does not execute a nested $where:
Uncaught AssertionError: expected Error {
code: 'OPERATOR_NOT_ALLOWED_IN_QUERY',
statusCode: 400,
details: Object {
operators: Array [ '$where' ],
where: Object {
content: Object {
$where: 'function() {return this.content.contains("content")}'
}
}
},
message: 'Operators "$where" are not allowed in query'
} to not exist
at Post.find (test/mongodb.test.js:830:24)
at /home/travis/build/strongloop/loopback-connector-mongodb/node_modules/loopback-datasource-juggler/lib/dao.js:1525:7
at process._tickCallback (internal/process/next_tick.js:61:11)

But I saw this failure before I made any code changes. Is it an already known failure?

dhmlau · 2019-02-07T19:18:47Z

@jannyHou, seems like it might be related to the PR that you've worked on: #484. Could you please help on the test failure? Thanks.

b-admike

@jareeve Thank you for your patch! Can you please add some notes in the README for the connector about this change? (something like what you have in the description of the issue related to this PR). Also, please add test(s) to verify that the correct connection url string is built for protocol type.

jannyHou · 2019-02-08T20:10:48Z

@dhmlau My fix doesn't seem to be merged into master. If master fails then we should fix it first.

jannyHou

@jareeve Thank you for the contribution!
Don't worry about the failing test, it shouldn't be related to your PR.

I left a nitpick comment for code refactor. Otherwise your change looks reasonable to me.

And yes please add at least a unit test to verify the generated url is correct. If you can test a 'mongodb+srv' connection works that's even better.

jannyHou · 2019-02-08T20:22:52Z

lib/mongodb.js

@@ -57,6 +57,11 @@ function generateMongoDBURL(options) {
  options.port = options.port || 27017;
  options.database = options.database || options.db || 'test';
  var username = options.username || options.user;
+  var portUrl = '';
+  // only include port if not using mongodb+srv
+  if(options.protocol !== 'mongodb+srv') {


Nitpick: can we use a truthy assertion here?
Like

if(options.protocal === 'mongodb+srv') { options.port = ''; }

And then you don't have to make the code change on line 74 and 83.

Not sure I follow, I need the change on 74 and 83 to remove the : as well as the port number. Pretty sure the url will not be valid with : in it.

jareeve · 2019-02-11T11:29:00Z

@jannyHou I have added a unit test and added some details to the readme about using mongo+srv. I have posted a comment above about your proposed change as i think it will not work.

jareeve · 2019-02-20T15:48:21Z

@jannyHou let me know if there is anything else you need me to do on this pr.

dhmlau · 2019-02-24T13:59:01Z

@slnode test please

dhmlau · 2019-03-09T15:11:04Z

@slnode test please
There's PR merged 12 days ago has most of the CI passed. Want to kick it off again for this PR.

@jannyHou @b-admike , please review again. Thanks

dhmlau · 2019-03-09T22:59:00Z

CI failed because of the error mentioned above.

emonddr · 2019-03-19T13:52:05Z

test/mongodb.test.js

+        };
+        module.generateMongoDBURL(options).should.be.eql('mongodb://fakeHostname:9999/fakeDatabase');
+      });
+      it('when protocol is mongodb and no username/password', function () {


name of test should be 'when protocol is mongodb and username/password' since this test
IS passing user name and password in the options

Corrected to state it has username and password

emonddr · 2019-03-19T13:53:07Z

test/mongodb.test.js

+        // mongodb+srv url should not have the port in it
+        module.generateMongoDBURL(options).should.be.eql('mongodb+srv://fakeHostname/fakeDatabase');
+      });
+      it('when protocol is mongodb+srv and no username/password', function () {


Name of test should be 'when protocol is mongodb+srv and username/password' since the
user name and password IS being passed in the options.

Corrected to state it has username and password

emonddr

Great work. :)
Please see my comments about 2 mocha test 'titles' that need to be changed.

dhmlau · 2019-04-01T13:55:53Z

@jareeve , could you please address the review comment from @emonddr ? Thanks.

jareeve · 2019-04-04T09:48:04Z

@emonddr @dhmlau sorry for the delay but i missed the original comments. i have now updated the test names.

emonddr · 2019-04-04T13:03:14Z

@jareeve , thanks for correcting the test case names.

Please fix the lint problem with the long commit message
Click Details

and you will see:

This title is too long:

emonddr · 2019-04-04T13:07:31Z

@jannyHou , it seems npm run test is failing on a test not related to @jareeve's changes. Please see @dhmlau comment above. thx. I have copied it below for convenience.

jareeve · 2019-04-04T13:51:00Z

I think I have fixed everything due to me.

dhmlau · 2019-04-04T15:15:18Z

@slnode test please

dhmlau · 2019-04-04T15:17:23Z

@jareeve, thanks! one last thing, could you please squash your commits?
See instructions https://loopback.io/doc/en/lb4/submitting_a_pr.html#10-final-rebase-and-squashing-of-commits. Thanks.

dhmlau · 2019-04-04T15:17:54Z

@emonddr , could you please help land this PR? Thanks!

dhmlau · 2019-04-17T01:03:53Z

@jannyHou @emonddr, I've rebased the PR and fixed the commit message linting error. Could you please review again? Thanks.

jannyHou · 2019-04-17T14:54:02Z

@slnode test please

jannyHou

The windows failure is not related, the vm needs to upgrade mongodb from 3.2 to 3.4 to support new features.

dhmlau · 2019-04-18T12:58:49Z

@jareeve thanks for your contribution. Your PR has landed! 🎉

jareeve requested review from b-admike, dhmlau, jannyHou and virkt25 as code owners February 6, 2019 17:37

jareeve mentioned this pull request Feb 6, 2019

Using protocol mongdb+srv fails because it is not valid to set port in connection url #496

Closed

dhmlau reviewed Feb 7, 2019

View reviewed changes

b-admike reviewed Feb 8, 2019

View reviewed changes

jannyHou suggested changes Feb 8, 2019

View reviewed changes

dhmlau added the community-contribution label Feb 8, 2019

dhmlau mentioned this pull request Feb 15, 2019

mongo url parser fails loopbackio/loopback4-example-shopping#30

Closed

dhmlau approved these changes Feb 24, 2019

View reviewed changes

dhmlau requested review from jannyHou and b-admike March 9, 2019 15:11

emonddr reviewed Mar 19, 2019

View reviewed changes

emonddr self-requested a review March 19, 2019 13:54

emonddr suggested changes Mar 19, 2019

View reviewed changes

dhmlau mentioned this pull request Apr 1, 2019

Monthly Milestone - April 2019 🌿🐰☔️ loopbackio/loopback-next#2669

Closed

46 tasks

dhmlau assigned emonddr Apr 4, 2019

jareeve force-pushed the mongodb-srv branch from 5769b61 to 99d8863 Compare April 4, 2019 16:00

dhmlau mentioned this pull request Apr 6, 2019

Fix CI on OPERATOR_NOT_ALLOWED_IN_QUERY #503

Closed

3 tasks

Remove port when using mongodb+srv

869ddd7

dhmlau force-pushed the mongodb-srv branch from 99d8863 to 869ddd7 Compare April 17, 2019 01:01

dhmlau requested a review from emonddr April 17, 2019 01:04

jannyHou approved these changes Apr 17, 2019

View reviewed changes

dhmlau merged commit cf10c6b into loopbackio:master Apr 18, 2019

dhmlau mentioned this pull request Apr 27, 2019

MongoDB Atlas Connection to Loopback #511

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove port when using mongodb+srv #497

Remove port when using mongodb+srv #497

jareeve commented Feb 6, 2019

slnode commented Feb 6, 2019

dhmlau commented Feb 7, 2019

dhmlau Feb 7, 2019

jareeve Feb 7, 2019 •

edited

Loading

jareeve commented Feb 7, 2019 •

edited

Loading

dhmlau commented Feb 7, 2019

b-admike left a comment

jannyHou commented Feb 8, 2019

jannyHou left a comment

jannyHou Feb 8, 2019

jareeve Feb 11, 2019

jareeve commented Feb 11, 2019

jareeve commented Feb 20, 2019

dhmlau commented Feb 24, 2019

dhmlau commented Mar 9, 2019 •

edited

Loading

dhmlau commented Mar 9, 2019

emonddr Mar 19, 2019

jareeve Apr 4, 2019

emonddr Mar 19, 2019

jareeve Apr 4, 2019

emonddr left a comment

dhmlau commented Apr 1, 2019

jareeve commented Apr 4, 2019

emonddr commented Apr 4, 2019 •

edited

Loading

emonddr commented Apr 4, 2019 •

edited

Loading

jareeve commented Apr 4, 2019

dhmlau commented Apr 4, 2019

dhmlau commented Apr 4, 2019

dhmlau commented Apr 4, 2019

dhmlau commented Apr 17, 2019

jannyHou commented Apr 17, 2019

jannyHou left a comment

dhmlau commented Apr 18, 2019

Remove port when using mongodb+srv #497

Remove port when using mongodb+srv #497

Conversation

jareeve commented Feb 6, 2019

Description

Related issues

Checklist

slnode commented Feb 6, 2019

dhmlau commented Feb 7, 2019

dhmlau Feb 7, 2019

Choose a reason for hiding this comment

jareeve Feb 7, 2019 • edited Loading

Choose a reason for hiding this comment

jareeve commented Feb 7, 2019 • edited Loading

dhmlau commented Feb 7, 2019

b-admike left a comment

Choose a reason for hiding this comment

jannyHou commented Feb 8, 2019

jannyHou left a comment

Choose a reason for hiding this comment

jannyHou Feb 8, 2019

Choose a reason for hiding this comment

jareeve Feb 11, 2019

Choose a reason for hiding this comment

jareeve commented Feb 11, 2019

jareeve commented Feb 20, 2019

dhmlau commented Feb 24, 2019

dhmlau commented Mar 9, 2019 • edited Loading

dhmlau commented Mar 9, 2019

emonddr Mar 19, 2019

Choose a reason for hiding this comment

jareeve Apr 4, 2019

Choose a reason for hiding this comment

emonddr Mar 19, 2019

Choose a reason for hiding this comment

jareeve Apr 4, 2019

Choose a reason for hiding this comment

emonddr left a comment

Choose a reason for hiding this comment

dhmlau commented Apr 1, 2019

jareeve commented Apr 4, 2019

emonddr commented Apr 4, 2019 • edited Loading

emonddr commented Apr 4, 2019 • edited Loading

jareeve commented Apr 4, 2019

dhmlau commented Apr 4, 2019

dhmlau commented Apr 4, 2019

dhmlau commented Apr 4, 2019

dhmlau commented Apr 17, 2019

jannyHou commented Apr 17, 2019

jannyHou left a comment

Choose a reason for hiding this comment

dhmlau commented Apr 18, 2019

jareeve Feb 7, 2019 •

edited

Loading

jareeve commented Feb 7, 2019 •

edited

Loading

dhmlau commented Mar 9, 2019 •

edited

Loading

emonddr commented Apr 4, 2019 •

edited

Loading

emonddr commented Apr 4, 2019 •

edited

Loading