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

fix(generators): Harden mongodb.js to reliably extract database from any connection string #3264

Merged
merged 4 commits into from Jan 5, 2024

Conversation

jermsam
Copy link
Contributor

@jermsam jermsam commented Aug 21, 2023

The default connection for mongodb in mongodb.js fails when you try to connect with a connection string that uses mongodb-replicas sets or clusters ... (eg: mongodb://localhost:27017,127.0.0.1:27018/mo-db?replicaSet=rs0) ... This is because
new URL(connection).pathname.substring(1)
fails with an is not a valid URL error .... I propose the following replacement to mongodb.js as it will satisfy all most all formats of connection strings

@daffl
Copy link
Member

daffl commented Oct 11, 2023

Thank you for putting this PR up (and sorry for the late reply). I wonder if we should make this a helper since we'd want the generated code to be as short as possible.

@jermsam
Copy link
Contributor Author

jermsam commented Oct 13, 2023

Let me optimize it to be as short as possible

@jermsam
Copy link
Contributor Author

jermsam commented Oct 13, 2023

it's now just one additional line of code from what it was originally

@daffl daffl changed the title harden mongodb.js to reliably extract database from any connection st… fix(generators): Harden mongodb.js to reliably extract database from any connection string Jan 5, 2024
@daffl daffl merged commit 7b0f82c into feathersjs:dove Jan 5, 2024
0 of 2 checks passed
@daffl
Copy link
Member

daffl commented Jan 5, 2024

Thank you for updating and sorry for the delay.

daffl added a commit that referenced this pull request Jan 5, 2024
… database from any connection string (#3264)"

This reverts commit 7b0f82c.
@daffl
Copy link
Member

daffl commented Jan 5, 2024

Apologies again but I had to revert the merge. I thought the tests were failing because things weren't up to date with latest but the error was actually an invalid regular expression that I wasn't able to fix (see errors in https://github.com/feathersjs/feathers/actions/runs/7427523402/job/20213354620#step:8:3307). You can run all test locally by running npm test in the main repo.

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

Successfully merging this pull request may close these issues.

None yet

2 participants