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

Remove port when using mongodb+srv #497

Merged
merged 1 commit into from Apr 18, 2019

Conversation

Projects
None yet
6 participants
@jareeve
Copy link
Contributor

commented Feb 6, 2019

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

@jareeve jareeve requested review from b-admike, dhmlau, jannyHou and virkt25 as code owners Feb 6, 2019

@slnode

This comment has been minimized.

Copy link

commented Feb 6, 2019

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

@dhmlau

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

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

@@ -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 = ''

This comment has been minimized.

Copy link
@dhmlau

dhmlau Feb 7, 2019

Contributor

missing the ending ;

This comment has been minimized.

Copy link
@jareeve

jareeve Feb 7, 2019

Author Contributor

added the ; and linter is now passing

@jareeve

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

Looks like travis is failing with:


1 failing

  1. 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

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

@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
Copy link
Member

left a comment

@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

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2019

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

@jannyHou
Copy link
Contributor

left a comment

@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.

@@ -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') {

This comment has been minimized.

Copy link
@jannyHou

jannyHou Feb 8, 2019

Contributor

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.

This comment has been minimized.

Copy link
@jareeve

jareeve Feb 11, 2019

Author Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

@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.

@dhmlau dhmlau referenced this pull request Feb 15, 2019

Closed

mongo url parser fails #30

@jareeve

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

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

@dhmlau

dhmlau approved these changes Feb 24, 2019

@dhmlau

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2019

@slnode test please

@dhmlau

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2019

@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 dhmlau requested review from jannyHou and b-admike Mar 9, 2019

@dhmlau

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2019

CI failed because of the error mentioned above.

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

This comment has been minimized.

Copy link
@emonddr

emonddr Mar 19, 2019

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

This comment has been minimized.

Copy link
@jareeve

jareeve Apr 4, 2019

Author Contributor

Corrected to state it has username and password

// 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 () {

This comment has been minimized.

Copy link
@emonddr

emonddr Mar 19, 2019

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.

This comment has been minimized.

Copy link
@jareeve

jareeve Apr 4, 2019

Author Contributor

Corrected to state it has username and password

@emonddr emonddr self-requested a review Mar 19, 2019

@emonddr
Copy link

left a comment

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

@dhmlau

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

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

@dhmlau dhmlau referenced this pull request Apr 1, 2019

Closed

Monthly Milestone - April 2019 🌿🐰☔️ #2669

20 of 46 tasks complete
@jareeve

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

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

@emonddr

This comment has been minimized.

Copy link

commented Apr 4, 2019

@jareeve , thanks for correcting the test case names.

Please fix the lint problem with the long commit message
Click Details
image
and you will see:
image

This title is too long:

image

@emonddr

This comment has been minimized.

Copy link

commented Apr 4, 2019

@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.

image

@jareeve

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

I think I have fixed everything due to me.

@dhmlau

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

@slnode test please

@dhmlau

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

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

@jareeve jareeve force-pushed the jareeve:mongodb-srv branch from 5769b61 to 99d8863 Apr 4, 2019

@dhmlau dhmlau referenced this pull request Apr 6, 2019

Closed

Fix CI on OPERATOR_NOT_ALLOWED_IN_QUERY #503

0 of 3 tasks complete

@dhmlau dhmlau force-pushed the jareeve:mongodb-srv branch from 99d8863 to 869ddd7 Apr 17, 2019

@dhmlau

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

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

@dhmlau dhmlau requested a review from emonddr Apr 17, 2019

@jannyHou

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

@slnode test please

@jannyHou
Copy link
Contributor

left a comment

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

@dhmlau dhmlau merged commit cf10c6b into strongloop:master Apr 18, 2019

9 of 16 checks passed

[cis-jenkins] PR Builder Build finished.
Details
[cis-jenkins] downstream: bluemix-metering@develop Failed! (869ddd7)
Details
[cis-jenkins] downstream: bluemix-service-broker@develop Failed! (869ddd7)
Details
[cis-jenkins] downstream: plan-manager@develop Failed! (869ddd7)
Details
loopback-connector-mongodb Failed! (869ddd7)
Details
loopback-connector-mongodb/node=6.x,os=windows Failed! (869ddd7)
Details
pr-builder ${BUILD_FAILURE_ANALYZER}
Details
Commit Linter commits are all properly formatted
Details
PR Linter PR is up to date
Details
[cis-jenkins] x64 && linux && nvm && dbs,10 Success! (869ddd7)
Details
[cis-jenkins] x64 && linux && nvm && dbs,6 Success! (869ddd7)
Details
[cis-jenkins] x64 && linux && nvm && dbs,8 Success! (869ddd7)
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
loopback-connector-mongodb/node=4.x,os=windows Success! (869ddd7)
Details
security/snyk - package.json (StrongLoop) No manifest changes detected
@dhmlau

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

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

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