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

feat: Fix for issue#663 #666

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

jaishirole
Copy link

Signed-off-by: jaishirole [email protected]

Description of changes

Fixes #663
Makes sure that fix does cover #610 too

  1. Since Mongo Connection URLs can have commas in it, the URL class based on WHATWG standard in current state, is unable to parse these URLs. Though there exists a method to construct a MongoDB URL from the settings, the code change was needed to parse a MongoURL and explicitly insert database if one exists in the settings. This will happen only if
    the url doesn't have a database specified and settings has it.

  2. Please note that I have modified expected results of 3 existing unit test cases as the current Mongo version now returns a different error message than in the past.

  3. The package-lock.json is added based on modifications seen on my system.

  4. This is my first commit in Loopback project, looking forward to the feedback if something isn't done right way. Thanks!

Checklist

image

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • Commit messages are following our guidelines

@jaishirole
Copy link
Author

@dhmlau @achrinza My first PR is up for review. Need your help to trigger the checks workflow. Thanks!

@jaishirole
Copy link
Author

Hi @achrinza , on my local setup when I got a Mongo docker image and then ran setup.sh to be able to run tests, it returned error message The dollar ($) prefixed field '$rename' in '$rename' is not allowed in the context of an update's replacement document. Consider using an aggregation pipeline with . I see that the Mongo of the checks environment, perhaps is of older version (not sure, but a guess). How do I resolve this?
If I take off this change, unit tests will fail locally for me, but will pass in the PR checks..
Can you guide me here?

@achrinza
Copy link
Member

I suspect it's because of these lines:

docker pull mongo:latest > /dev/null 2>&1

CONTAINER_STATUS=$(docker run --name $MONGODB_CONTAINER -p $PORT:27017 -d mongo:latest 2>&1)

The CI pipeline pins to v4.4:

Compare to latest which AFAIK points to 5.0.7.

Try changing the line in your local copy of setup.sh to point to mongodb:4.4 instead.

Copy link
Member

@achrinza achrinza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR, @jaishirole, most of it looks great! Just have a few comments 👇

lib/mongodb.js Outdated
self.settings.url = urlObj.toString();
// This is special processing if database is not part of url, but is in settings
if (self.settings.database && self.settings.url) {
if ((self.settings.url.indexOf('/' + self.settings.database) === -1) && (self.settings.url.indexOf('authSource=' + self.settings.database) === -1)) {
Copy link
Member

@achrinza achrinza Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this condition supposed to be OR, or is authSource identical to the /database-name syntax?

Suggested change
if ((self.settings.url.indexOf('/' + self.settings.database) === -1) && (self.settings.url.indexOf('authSource=' + self.settings.database) === -1)) {
if ((self.settings.url.indexOf('/' + self.settings.database) === -1) || (self.settings.url.indexOf('authSource=' + self.settings.database) === -1)) {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@achrinza My thinking is, if someone is including authSource, they do know of their database. If I change this to an 'OR', I see that for every mongoURL, the new logic will be invoked. I was trying to see how to narrow down calls into it and hence '&&' made sense as this lets us process the URL if url doesn't have a database specified (after /) and not even in authSource parameter.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@achrinza This indeed is a flaky condition, let me think over and push a fix. The unit tests directly call the new function, hence I missed testing this part. Will take care of it in new commit. Can you trigger the workflow, to see other issues are taken care now?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@achrinza Pushing a new commit with modified condition as well as adding a few more functional test cases that will execute the new function more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Locally passing test coverage post latest commit:
image

lib/mongodb.js Show resolved Hide resolved
test/mongodb.test.js Show resolved Hide resolved
test/mongodb.test.js Show resolved Hide resolved
@jaishirole
Copy link
Author

@achrinza Thanks much for the review comments. I'll incorporate the changes later today.

@jaishirole
Copy link
Author

I suspect it's because of these lines:

docker pull mongo:latest > /dev/null 2>&1

CONTAINER_STATUS=$(docker run --name $MONGODB_CONTAINER -p $PORT:27017 -d mongo:latest 2>&1)

The CI pipeline pins to v4.4:

Compare to latest which AFAIK points to 5.0.7.

Try changing the line in your local copy of setup.sh to point to mongodb:4.4 instead.

In the new commit, added handling so that even for latest Mongo, it should work. :)

@coveralls
Copy link

coveralls commented Apr 19, 2022

Pull Request Test Coverage Report for Build 2190805686

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 55 of 57 (96.49%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.8%) to 82.318%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/mongodb.js 55 57 96.49%
Totals Coverage Status
Change from base Build 2071440768: 0.8%
Covered Lines: 956
Relevant Lines: 1098

💛 - Coveralls

@achrinza
Copy link
Member

Not sure why the coverage plummeted to zero, but the test runs themselves look ok.

@jaishirole
Copy link
Author

@achrinza Sorry, didn't get time during the day. Will look into tomorrow. I have an idea of what is missing.

@jaishirole
Copy link
Author

@achrinza Can you help trigger the Workflow to see what happens this time ?

@achrinza
Copy link
Member

Workflow triggered - Just a heads up, our workflow doesn't allow merging PRs with Merge Commits, so they'll need to be dropped and, if needed, replaced with a rebase instead.

@jaishirole
Copy link
Author

Workflow triggered - Just a heads up, our workflow doesn't allow merging PRs with Merge Commits, so they'll need to be dropped and, if needed, replaced with a rebase instead.

Oh, didn't know. Looking into how to remove the merge commit from this PR now.. Any tips ?

@jaishirole
Copy link
Author

Workflow triggered - Just a heads up, our workflow doesn't allow merging PRs with Merge Commits, so they'll need to be dropped and, if needed, replaced with a rebase instead.

Hi @achrinza , the merge commit is now dropped. Please review and let me know if I should fix anything more. Thanks!

Copy link
Member

@achrinza achrinza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay in review. The changes LGTM 👍

Just need to squash into a single Git Commit.

A suggested Git Commit message would be:

fix: connection string seed list parsing

closes: https://github.com/loopbackio/loopback-connector-mongodb/issues/663

@achrinza achrinza added bug v6.x Affects v6.x labels May 20, 2022
@achrinza
Copy link
Member

Thanks for applying the changes and sorry for the delayed review; Everything LGTM 👍

Just need to squash into a single Git Commit and it can be merged.

@jaishirole
Copy link
Author

Thanks for applying the changes and sorry for the delayed review; Everything LGTM 👍

Just need to squash into a single Git Commit and it can be merged.

@achrinza I was on vacation and the squashing took unnecessarily longer. I'm sorry for the delay.
I've squashed all into one commit and when ran the UT locally, saw it passing:
image

Can you please help with the next steps here? Thanks much for all your guidance so far!

@gian1200
Copy link

Hi all, is there any update on this? Thanks

@marcioluis
Copy link

@dhmlau Hi all, is there any update on this? Thanks
I'm using https://www.npmjs.com/package/patch-package to fix this locally on my project as a workaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug v6.x Affects v6.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to connect to Replica Set or Sharded Cluster
5 participants