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: decode encoded value from urlParts.pathname for database option #14963

Merged
merged 8 commits into from Sep 14, 2022

Conversation

fzn0x
Copy link
Member

@fzn0x fzn0x commented Sep 7, 2022

Pull Request Checklist

Please make sure to review and check all of these items:

  • Have you added new tests to prevent regressions?
  • Does yarn test or yarn test-DIALECT pass with this change (including linting)?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description Of Change

Close #14962

options.database returning bad value for database connection, this affected to all database except the options.dialect with postgres as the value.

if (options.dialect === 'postgres') {

You can notice the strange thing by running this code with NodeJS:

import url from 'url';

const urlParts = url.parse("pg://wpx%20ss:wpx%20ss@21.77.77:4001/database ss", true);

const options = {}

// adding decodeURI solve the database issue.
options.database = decodeURI(urlParts.pathname.replace(/^\//, ''));
console.log(urlParts.auth);

// urlParts.auth already decode the encoded URI value by default
if (urlParts.auth) {
  const authParts = urlParts.auth.split(':');
  options.username = authParts[0];
  if (authParts.length > 1) {
    options.password = authParts.slice(1).join(':');
  }
}

console.log(options)

Todos

  • Add decodeURI for handling urlParts.pathname value

@fzn0x fzn0x added the type: bug label Sep 7, 2022
@fzn0x fzn0x changed the title fix(utils): decode encoded value from urlParts.pathname for database option fix: decode encoded value from urlParts.pathname for database option Sep 7, 2022
@fzn0x fzn0x self-assigned this Sep 7, 2022
src/utils/url.ts Outdated Show resolved Hide resolved
@fzn0x
Copy link
Member Author

fzn0x commented Sep 8, 2022

Do you have any suggestion for the unit test? @WikiRik

Since the Utils does not include url functions on sequelize/core

import { Utils } from '@sequelize/core';

We need to add a another PR that includes the url utility or all the other utilities too and release it to new v7 alpha version, you can take a part for this, or the alternative solution is I am to import the url utility directly.

@fzn0x
Copy link
Member Author

fzn0x commented Sep 8, 2022

Or skip the unit test also fine for me, since the decodeURIComponent itself is the function coming from nodejs core, which is the part of the built-in we probably no doubt to use.

@WikiRik
Copy link
Member

WikiRik commented Sep 8, 2022

Do you have any suggestion for the unit test? @WikiRik

I was thinking we could add unit tests for parseConnectionString to check if everything is parsing correctly. I do think that testing decodeURIComponent separately is not needed.

fzn0x added a commit that referenced this pull request Sep 8, 2022
Targetting the v7 alpha release, for the #14963 unit testing.
WikiRik pushed a commit that referenced this pull request Sep 9, 2022
Targetting the v7 alpha release, for the #14963 unit testing.
@fzn0x
Copy link
Member Author

fzn0x commented Sep 11, 2022

Hi @WikiRik did you have any inputs left on this PR or we good to go?

@WikiRik
Copy link
Member

WikiRik commented Sep 11, 2022

Looks good to me, but I'll wait with approval until further comment by @wkrasnicki-vizlib

@wkrasnicki-vizlib
Copy link

Yes, this solves the issue. Thank you guys

@WikiRik WikiRik merged commit 12983a1 into main Sep 14, 2022
@WikiRik WikiRik deleted the fix/sequelize-utils-url branch September 14, 2022 07:27
@fzn0x
Copy link
Member Author

fzn0x commented Sep 14, 2022

Glad it helps, thank you guys!

@github-actions
Copy link
Contributor

🎉 This PR is included in version 7.0.0-alpha.17 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

Spaces are not encoded/decoded correctly in the Postgres database name when using connection string
3 participants