feat: Fix for issue#663 #666

jaishirole · 2022-04-19T13:29:18Z

Signed-off-by: jaishirole [email protected]

Description of changes

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

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.
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.
The package-lock.json is added based on modifications seen on my system.
This is my first commit in Loopback project, looking forward to the feedback if something isn't done right way. Thanks!

Checklist

DCO (Developer Certificate of Origin) signed in all commits
npm test passes on your machine

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

jaishirole · 2022-04-19T13:30:35Z

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

jaishirole · 2022-04-19T14:01:51Z

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 · 2022-04-19T14:12:56Z

I suspect it's because of these lines:

loopback-connector-mongodb/setup.sh

Line 44 in 5936ba0

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

loopback-connector-mongodb/setup.sh

Line 49 in 5936ba0

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

The CI pipeline pins to v4.4:

loopback-connector-mongodb/.github/workflows/continuous-integration.yaml

Line 25 in 5936ba0

mongodb-version: [4.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.

achrinza

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

achrinza · 2022-04-19T13:39:33Z

lib/mongodb.js

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


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

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

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

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

Locally passing test coverage post latest commit:

lib/mongodb.js

test/mongodb.test.js

jaishirole · 2022-04-19T14:39:27Z

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

jaishirole · 2022-04-19T16:11:14Z

I suspect it's because of these lines:

loopback-connector-mongodb/setup.sh

Line 44 in 5936ba0

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

loopback-connector-mongodb/setup.sh

Line 49 in 5936ba0

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

The CI pipeline pins to v4.4:

loopback-connector-mongodb/.github/workflows/continuous-integration.yaml

Line 25 in 5936ba0

mongodb-version: [4.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 · 2022-04-19T17:10:43Z

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.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

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
Change from base Build 2071440768:	0.8%
Covered Lines:	956
Relevant Lines:	1098

💛 - Coveralls

achrinza · 2022-04-19T22:19:45Z

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

jaishirole · 2022-04-20T16:31:39Z

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

jaishirole · 2022-04-28T18:31:40Z

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

achrinza · 2022-04-28T18:35:25Z

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 · 2022-04-28T19:02:41Z

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 · 2022-05-02T14:08:24Z

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!

achrinza

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 · 2022-05-20T19:13:46Z

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.

closes: loopbackio#663 Signed-off-by: jaishirole <[email protected]>

jaishirole · 2022-05-31T19:51:51Z

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:

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

gian1200 · 2023-11-10T23:31:47Z

Hi all, is there any update on this? Thanks

marcioluis · 2024-01-05T00:00:23Z

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

manarhusrieh · 2024-11-29T11:49:28Z

I am worried that this package is no longer maintained. @dhmlau @achrinza is it going to be merged?

dhmlau · 2024-11-29T18:57:55Z

@manarhusrieh, this PR was approved by @achrinza, so I'd assume it's good to go.
But I see there are merging conflicts, @jaishirole, could you please resolve that?

Question for @achrinza, this PR is for the master branch, but it has the v6.x label. Was it intended for 6.x too? If yes, we'd need another PR.

achrinza · 2024-11-30T02:35:59Z

@dhmlau This should be merged into v7 now. v6 was the latest major version at that time.

manarhusrieh · 2024-11-30T10:56:52Z

@achrinza can't this be merged to version 6? This is an issue with the package, not a new feature. We are currently using this PR package instead of the official one and it is working properly for more than a year.

dhmlau · 2024-12-04T13:36:14Z

@manarhusrieh, could you please submit a new PR against the 6.x branch?
This PR is opened against the master branch which is 7.x. Thanks.

manarhusrieh · 2024-12-04T16:32:13Z

@dhmlau OK, I'll prepare a PR against version 6. Thanks

manarhusrieh · 2024-12-09T10:19:25Z

@dhmlau I have created the new PR for branch 6.x #741

jaishirole requested review from dhmlau and achrinza as code owners April 19, 2022 13:29

achrinza reviewed Apr 19, 2022

View reviewed changes

jaishirole force-pushed the master branch from e131448 to 2d731a0 Compare May 2, 2022 13:21

achrinza approved these changes May 9, 2022

View reviewed changes

achrinza added bug v6.x Affects v6.x labels May 20, 2022

fix: connection string seed list parsing

a955e3d

closes: loopbackio#663 Signed-off-by: jaishirole <[email protected]>

jaishirole force-pushed the master branch from 6cfdac1 to a955e3d Compare May 31, 2022 19:48

manarhusrieh mentioned this pull request Dec 9, 2024

Fix for issue#663 on branch 6.x #741

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Fix for issue#663 #666

feat: Fix for issue#663 #666

jaishirole commented Apr 19, 2022

jaishirole commented Apr 19, 2022

jaishirole commented Apr 19, 2022

achrinza commented Apr 19, 2022

achrinza left a comment

achrinza Apr 19, 2022 •

edited

Loading

jaishirole Apr 19, 2022

jaishirole Apr 19, 2022

jaishirole Apr 28, 2022

jaishirole Apr 28, 2022

jaishirole commented Apr 19, 2022

jaishirole commented Apr 19, 2022

coveralls commented Apr 19, 2022 •

edited

Loading

achrinza commented Apr 19, 2022

jaishirole commented Apr 20, 2022

jaishirole commented Apr 28, 2022

achrinza commented Apr 28, 2022

jaishirole commented Apr 28, 2022

jaishirole commented May 2, 2022

achrinza left a comment

achrinza commented May 20, 2022

jaishirole commented May 31, 2022

gian1200 commented Nov 10, 2023

marcioluis commented Jan 5, 2024

manarhusrieh commented Nov 29, 2024

dhmlau commented Nov 29, 2024

achrinza commented Nov 30, 2024 •

edited

Loading

manarhusrieh commented Nov 30, 2024

dhmlau commented Dec 4, 2024

manarhusrieh commented Dec 4, 2024

manarhusrieh commented Dec 9, 2024

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

feat: Fix for issue#663 #666

Are you sure you want to change the base?

feat: Fix for issue#663 #666

Conversation

jaishirole commented Apr 19, 2022

Description of changes

Checklist

jaishirole commented Apr 19, 2022

jaishirole commented Apr 19, 2022

achrinza commented Apr 19, 2022

achrinza left a comment

Choose a reason for hiding this comment

achrinza Apr 19, 2022 • edited Loading

Choose a reason for hiding this comment

jaishirole Apr 19, 2022

Choose a reason for hiding this comment

jaishirole Apr 19, 2022

Choose a reason for hiding this comment

jaishirole Apr 28, 2022

Choose a reason for hiding this comment

jaishirole Apr 28, 2022

Choose a reason for hiding this comment

jaishirole commented Apr 19, 2022

jaishirole commented Apr 19, 2022

coveralls commented Apr 19, 2022 • edited Loading

Pull Request Test Coverage Report for Build 2190805686

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

achrinza commented Apr 19, 2022

jaishirole commented Apr 20, 2022

jaishirole commented Apr 28, 2022

achrinza commented Apr 28, 2022

jaishirole commented Apr 28, 2022

jaishirole commented May 2, 2022

achrinza left a comment

Choose a reason for hiding this comment

achrinza commented May 20, 2022

jaishirole commented May 31, 2022

gian1200 commented Nov 10, 2023

marcioluis commented Jan 5, 2024

manarhusrieh commented Nov 29, 2024

dhmlau commented Nov 29, 2024

achrinza commented Nov 30, 2024 • edited Loading

manarhusrieh commented Nov 30, 2024

dhmlau commented Dec 4, 2024

manarhusrieh commented Dec 4, 2024

manarhusrieh commented Dec 9, 2024

achrinza Apr 19, 2022 •

edited

Loading

coveralls commented Apr 19, 2022 •

edited

Loading

achrinza commented Nov 30, 2024 •

edited

Loading