feat: Fix for issue#663

[jaishirole]

Signed-off-by: jaishirole [email protected]

Description of changes

Fixes https://github.com/loopbackio/loopback-connector-mongodb/issues/663 Makes sure that fix does cover https://github.com/loopbackio/loopback-connector-mongodb/issues/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

• [x] DCO (Developer Certificate of Origin) signed in all commits

• [x] npm test passes on your machine

• [x] New tests added or existing tests modified to cover all changes

• [x] Code conforms with the style guide

• [x] Commit messages are following our guidelines

[jaishirole]

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

[jaishirole]

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]

I suspect it's because of these lines:

https://github.com/loopbackio/loopback-connector-mongodb/blob/5936ba0cd74a2b7dd84bb0925e0a9636de4f5d85/setup.sh#L44

https://github.com/loopbackio/loopback-connector-mongodb/blob/5936ba0cd74a2b7dd84bb0925e0a9636de4f5d85/setup.sh#L49

The CI pipeline pins to v4.4:

https://github.com/loopbackio/loopback-connector-mongodb/blob/5936ba0cd74a2b7dd84bb0925e0a9636de4f5d85/.github/workflows/continuous-integration.yaml#L25

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.

[jaishirole]

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

[jaishirole]

I suspect it's because of these lines:

https://github.com/loopbackio/loopback-connector-mongodb/blob/5936ba0cd74a2b7dd84bb0925e0a9636de4f5d85/setup.sh#L44

https://github.com/loopbackio/loopback-connector-mongodb/blob/5936ba0cd74a2b7dd84bb0925e0a9636de4f5d85/setup.sh#L49

The CI pipeline pins to v4.4:

https://github.com/loopbackio/loopback-connector-mongodb/blob/5936ba0cd74a2b7dd84bb0925e0a9636de4f5d85/.github/workflows/continuous-integration.yaml#L25

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]

Pull Request Test Coverage Report for Build 2336363911

• 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.327%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/mongodb.js	55	57	96.49%
<!--	Total:	55	57

Totals
Change from base Build 2197018116:	0.8%
Covered Lines:	956
Relevant Lines:	1098

---

💛 - Coveralls

[achrinza]

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

[jaishirole]

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

[jaishirole]

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

[achrinza]

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]

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]

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]

Thanks for applying the changes and sorry for the delayed review; Everything LGTM :+1:

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

[jaishirole]

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]

Hi all, is there any update on this? Thanks

[marcioluis]

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