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: mongodb connectionurl parse options #7438

Closed
wants to merge 4 commits into from
Closed

fix: mongodb connectionurl parse options #7438

wants to merge 4 commits into from

Conversation

gogotaro
Copy link
Contributor

@gogotaro gogotaro commented Feb 28, 2021

Description of change

src\driver\DriverUtils.ts

const optionsList = afterQuestionMark.split("&");
let optionKey: string;
let optionValue: string;

optionsList.forEach(optionItem => {
    optionKey = optionItem.split("=")[0];
    optionValue = optionItem.split("=")[1];
    optionsObject[optionKey] = optionValue;
});

Loop to create option object "optionsObject". Example, with connection string below

mongodb://testuser:testpwd@test-primary.example.com:27017/testdb?retryWrites=true&w=majority&useUnifiedTopology=true

It'll turn options to

{
    retryWrites: 'true',
    w: 'majority',
    useUnifiedTopology: 'true'
}

After that It use "optionsObject" to merge with "connectionUrl" before return as code below.

let connectionUrl: any = {
    type: type,
    host: host,
    hostReplicaSet: hostReplicaSet,
    username: decodeURIComponent(username),
    password: decodeURIComponent(password),
    port: port ? parseInt(port) : undefined,
    database: afterBase || undefined
};

for (const [key, value] of Object.entries(optionsObject)) {
    connectionUrl[key] = value
}

return connectionUrl;

Pull-Request Checklist

  • Code is up-to-date with the master branch
  • npm run lint passes with this change
  • npm run test passes with this change
  • This pull request links relevant issues as Fixes #7437
  • There are new or updated unit tests validating the change
  • Documentation has been updated to reflect this change
  • The new commits follow conventions explained in CONTRIBUTING.md

Cabelo Doido added 2 commits February 28, 2021 17:26
- Loop every options in mongodb connection url and turn it as object to merge with connection url object before return of method "parseMongoDBConnectionUrl"
- unit test of mongodb replicaset parse connectionurl of #7401
- unit test of mongodb options parse connectionurl of #7437
/home/circleci/typeorm/src/driver/DriverUtils.ts
  192:39  error  Missing semicolon  @typescript-eslint/semi
if (replicaSet) {
hostReplicaSet = hostAndPort;
} else {
[host, port] = hostAndPort.split(":");
}

return {
let connectionUrl: any = {
Copy link

@RA80533 RA80533 Mar 3, 2021

Choose a reason for hiding this comment

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

The explicit any type annotation widens the return type of parseMongoDBConnectionUrl() in a regressive manner.

Suggested change
let connectionUrl: any = {
let connectionUrl = {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, But origin of this code before my changed, It's return as any too then I try to not change It's type.

But your comment was make sense, I'll try to fixed it's type and test it to make sure that's not effect another soon.

Comment on lines +190 to +193
// Loop to set every options in connectionUrl to object
for (const [key, value] of Object.entries(optionsObject)) {
connectionUrl[key] = value;
}
Copy link

Choose a reason for hiding this comment

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

While this is written well, its intent can be made even clearer by using a simple spread operator with optionsObject like so:

let connectionUrl: any = {
    type: type,
    host: host,
    hostReplicaSet: hostReplicaSet,
    username: decodeURIComponent(username),
    password: decodeURIComponent(password),
    port: port ? parseInt(port) : undefined,
    database: afterBase || undefined,
    ...optionsObject
};

@@ -134,12 +134,27 @@ export class DriverUtils {
let port = undefined;
let hostReplicaSet = undefined;
let replicaSet = undefined;
// remove mongodb query params

let optionsObject: any = {};
Copy link

Choose a reason for hiding this comment

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

If optionsObject is annotated as Record<string, string> instead of any, it can be safely merged into connectionUrl without widening the resulting type.

Suggested change
let optionsObject: any = {};
let optionsObject: Record<string, string> = {};

Comment on lines +187 to +193
database: afterBase || undefined
};

// Loop to set every options in connectionUrl to object
for (const [key, value] of Object.entries(optionsObject)) {
connectionUrl[key] = value;
}
Copy link

Choose a reason for hiding this comment

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

Suggested change
database: afterBase || undefined
};
// Loop to set every options in connectionUrl to object
for (const [key, value] of Object.entries(optionsObject)) {
connectionUrl[key] = value;
}
database: afterBase || undefined,
...optionsObject
};

gogotaro and others added 2 commits March 14, 2021 21:55
* fix: fixed all known enum issues (#7419)

* fix #5371

* fix #6471;
fix: `enumName` changes not handled;
fix: `enumName` does not handle table schema;

* fixed falling test;

* added test for #7217

* fix #6047, #7283;

* fix #5871

* added support for `enumName` in `joinColumns` (#5729)

* fix #5478

* fixed falling test;
updated `postgres-enum` test;

* added column `array` property change detection (#5882);
updated `postgres-enum` test;

* fix #5275

* added validation for `enum` property (#2233)

* fix #5648

* improved missing "enum" or "enumName" properties validation;

* fix #4897, #6376

* lint fix;

* fixed falling tests;

* fixed falling tests;

* removed .only

* fix #6115

* refactor: improve README.md and DEVLOPER.md code examples formatting (#7436)

* fix: correctly get referenceColumn value in `getEntityValueMap` (#7005)

* test: add test case (#7002)

* fix: correctly get referenceColumn value in `getEntityValueMap`

* test: reproduction for issue #3246 (#3247)

* Add reproduction for issue 3246

* Update test/github-issues/3246/issue-3246.ts

Co-authored-by: Json Choi <1890mah@gmail.com>

Co-authored-by: Dan Imbrogno <dan.imbrogno@gmail.com>
Co-authored-by: AlexMesser <dmzt08@gmail.com>
Co-authored-by: Json Choi <1890mah@gmail.com>

* code refactoring in test;

* added test for #2758

* feat: allow to pass the given table name as string in RelationDecorators (#7448)

* feat(RelationDecorators): allow to pass the given table name as string

* Update EntityMetadataBuilder.ts

added parentheses;

Co-authored-by: Emily Marigold Klassen <forivall@users.noreply.github.com>

* feat: add option for installing package using CLI (#6889)

* init cli: add options for installing package

* yarg choice, add await, revert formatter changes

* init flag - set default to npm

Co-authored-by: AlexMesser <dmzt08@gmail.com>
Co-authored-by: Henry Boisdequin <boisdequinhenry19@gmail.com>
Co-authored-by: Json Choi <1890mah@gmail.com>
Co-authored-by: Dan Imbrogno <41128441+danimbrogno-pml@users.noreply.github.com>
Co-authored-by: Dan Imbrogno <dan.imbrogno@gmail.com>
Co-authored-by: Emily Marigold Klassen <forivall@gmail.com>
Co-authored-by: Emily Marigold Klassen <forivall@users.noreply.github.com>
Co-authored-by: Gaurav Sharma <gtpan77@gmail.com>
* fix: fixed all known enum issues (#7419)

* fix #5371

* fix #6471;
fix: `enumName` changes not handled;
fix: `enumName` does not handle table schema;

* fixed falling test;

* added test for #7217

* fix #6047, #7283;

* fix #5871

* added support for `enumName` in `joinColumns` (#5729)

* fix #5478

* fixed falling test;
updated `postgres-enum` test;

* added column `array` property change detection (#5882);
updated `postgres-enum` test;

* fix #5275

* added validation for `enum` property (#2233)

* fix #5648

* improved missing "enum" or "enumName" properties validation;

* fix #4897, #6376

* lint fix;

* fixed falling tests;

* fixed falling tests;

* removed .only

* fix #6115

* refactor: improve README.md and DEVLOPER.md code examples formatting (#7436)

* fix: correctly get referenceColumn value in `getEntityValueMap` (#7005)

* test: add test case (#7002)

* fix: correctly get referenceColumn value in `getEntityValueMap`

* test: reproduction for issue #3246 (#3247)

* Add reproduction for issue 3246

* Update test/github-issues/3246/issue-3246.ts

Co-authored-by: Json Choi <1890mah@gmail.com>

Co-authored-by: Dan Imbrogno <dan.imbrogno@gmail.com>
Co-authored-by: AlexMesser <dmzt08@gmail.com>
Co-authored-by: Json Choi <1890mah@gmail.com>

* code refactoring in test;

* added test for #2758

* feat: allow to pass the given table name as string in RelationDecorators (#7448)

* feat(RelationDecorators): allow to pass the given table name as string

* Update EntityMetadataBuilder.ts

added parentheses;

Co-authored-by: Emily Marigold Klassen <forivall@users.noreply.github.com>

* feat: add option for installing package using CLI (#6889)

* init cli: add options for installing package

* yarg choice, add await, revert formatter changes

* init flag - set default to npm

Co-authored-by: AlexMesser <dmzt08@gmail.com>
Co-authored-by: Henry Boisdequin <boisdequinhenry19@gmail.com>
Co-authored-by: Json Choi <1890mah@gmail.com>
Co-authored-by: Dan Imbrogno <41128441+danimbrogno-pml@users.noreply.github.com>
Co-authored-by: Dan Imbrogno <dan.imbrogno@gmail.com>
Co-authored-by: Emily Marigold Klassen <forivall@gmail.com>
Co-authored-by: Emily Marigold Klassen <forivall@users.noreply.github.com>
Co-authored-by: Gaurav Sharma <gtpan77@gmail.com>
@AlexMesser
Copy link
Collaborator

can you please send a clear PR without merge commits?

@gogotaro
Copy link
Contributor Author

can you please send a clear PR without merge commits?

Sure I'll close this PR and create new pr without merge. Will inform you within 48 hours.

@gogotaro gogotaro closed this Mar 30, 2021
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.

5 participants