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.

Sign up for GitHub

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

Jump to bottom

JSON configuration doesn't not support url as an object anymore #1112

Closed
jraoult opened this issue Apr 23, 2024 · 4 comments · Fixed by #1119
Closed

JSON configuration doesn't not support url as an object anymore #1112

jraoult opened this issue Apr 23, 2024 · 4 comments · Fixed by #1119
Labels
c: bug Something isn't working p: 1-normal Nothing urgent
Milestone
v7.x

Comments

@jraoult
Copy link
Contributor

jraoult commented Apr 23, 2024

Describe the bug

Thanks to a change introduced in #599 (comment) it was possible prior v7.0.0 to use a JSON configuration with the url property being an object containing the property connectionString.

For example, the following would work:

{
  "url": {
    "connectionString": "postgres://postgres:password@localhost:5432/postgres",
    "ssl": false
  }
}

but doesn't in v7.0.0. I suspect it is due to this check here:

node-pg-migrate/bin/node-pg-migrate.ts

Line 392 in dbc4207

if ('url' in json && json.url && isString(json.url)) {
that explicitly exclude url being anything else than a string whereas before it was not:

node-pg-migrate/bin/node-pg-migrate

Lines 302 to 303 in 9331f6f

if (json.url) {
DB_CONNECTION = typeof DB_CONNECTION !== 'undefined' ? DB_CONNECTION : json.url

If it is an intentional breaking change, it is worth updating the documentation to remove the note saying:

url can be connection string or config object accepted by pg see

Steps to reproduce

Try to run with the following configuration:

{
  "url": {
    "connectionString": "postgres://postgres:password@localhost:5432/postgres",
    "ssl": false
  }
}

You will eventually get the error:

could not connect to postgres: Error: SASL: SCRAM-SERVER-FIRST-MESSAGE: client password must be a string

Logs

No response

System Info

N/A

Used Module System

esm
@jraoult jraoult added the s: pending triage Pending Triage label Apr 23, 2024
@Shinigami92
Copy link
Collaborator

Shinigami92 commented Apr 23, 2024 •
edited

If it is an intentional breaking change, [...]

No, it was not an intentional breaking change
But also it was not supported officially 👀
It was just something that was possible but not checked nor known 🤔

It seems to be that the url can be like this https://github.com/DefinitelyTyped/DefinitelyTyped/blob/9ec204164d296bac9574f8113417236cae2a5d62/types/pg/index.d.ts#L18-L22

Would you like to draft a PR to allow this explicitly?
You would need to touch

node-pg-migrate/bin/node-pg-migrate.ts

Lines 391 to 403 in 569d173

// @ts-expect-error: this is a TS 4.8 bug
if ('url' in json && json.url && isString(json.url)) {
// @ts-expect-error: this is a TS 4.8 bug
DB_CONNECTION ??= json.url;
} else if (isClientConfig(json)) {
DB_CONNECTION ??= {
user: json.user,
host: json.host || 'localhost',
database: json.name || json.database,
password: json.password,
port: json.port || 5432,
ssl: json.ssl,
};
for this and also need to add an integration-test in https://github.com/salsita/node-pg-migrate/blob/569d173274e80e45262706398672f021de0f3fb1/.github/workflows/postgres-test.yml

I'm asking to let you the implementation, because so you can take some work from my shoulders as well as I can do a second eye-pair review and additionally you gain the credits of contributing to the project 😃
I would call this win-win-win

@Shinigami92 Shinigami92 added c: bug Something isn't working p: 1-normal Nothing urgent and removed s: pending triage Pending Triage labels Apr 23, 2024
@Shinigami92 Shinigami92 added this to the v7.x milestone Apr 23, 2024
@jraoult
Copy link
Contributor Author

jraoult commented Apr 23, 2024

Thank you for the confirmation @Shinigami92. I'll try to create a PR tomorrow for it but basically the fix is to remove isString check and the following a few lines below will do the rest:

node-pg-migrate/bin/node-pg-migrate.ts

Lines 512 to 515 in 569d173

const databaseUrl =
typeof DB_CONNECTION === 'string'
? { connectionString: DB_CONNECTION }
: DB_CONNECTION;

As a side note, I think it was officially supported per (cf. https://salsita.github.io/node-pg-migrate/#/cli?id=json-configuration):

url can be connection string or config object accepted by pg see

Which links to https://node-postgres.com/features/connecting#connection-uri. It was even given as an example of valid configuration by @dolezel in #599 (comment)

@Shinigami92
Copy link
Collaborator

Even when officially supported, the CI did not FAIL!
It needs to fail integration-tests! So then it is supported officially 😺

And thanks 🙏 I will leave an eye open for your PR 🙂

@jraoult jraoult mentioned this issue Apr 25, 2024
Merged
@jraoult
Copy link
Contributor Author

jraoult commented Apr 25, 2024

@Shinigami92 there it is #1119

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c: bug Something isn't working p: 1-normal Nothing urgent
Projects
None yet
Milestone
v7.x
Development

Successfully merging a pull request may close this issue.

fix: restore support url as an object in configuration jraoult/node-pg-migrate
2 participants
@jraoult @Shinigami92