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

Tie default DB name to database user name #1660

Closed
wants to merge 2 commits into from

Conversation

solidsnack
Copy link

@solidsnack solidsnack commented May 25, 2018

The previous behaviour was at variance with the native client behaviour, per:

https://www.postgresql.org/docs/10/static/libpq-connect.html#LIBPQ-PARAMKEYWORDS

The database name. Defaults to be the same as the user name.
In certain contexts, the value is checked for extended formats...

@solidsnack solidsnack force-pushed the patch-1 branch 7 times, most recently from de7fb60 to 6063d7e Compare May 26, 2018 02:52
The previous behaviour was at variance with the native client behaviour,
per:

  https://www.postgresql.org/docs/10/static/libpq-connect.html#LIBPQ-PARAMKEYWORDS

> The database name. Defaults to be the same as the user name.
> In certain contexts, the value is checked for extended formats...
@solidsnack
Copy link
Author

solidsnack commented May 26, 2018

I am really not sure about the test 'modified values are passed to created clients' in test/integration/client/configuration-tests.js, with regards to setting defaults.database.

  • Maybe the ability to set defaults.database should be removed? It's not clear how to reconcile it with the behavior of libpq, "Defaults to be the same as the user name.". Maybe it should only take effect if no username is set?

  • I'm not sure I understand the use case for the defaults.

The motivation for this patch, is that we have some DATABASE_URLs set across our infra that rely on the libpq behavior, that is also broadly compatible with other Postgres clients. I'm sure many other people are in the same situation.

It seems like something changed here, perhaps in the way Knex uses node-postgres, because these URLs used to work fine with our Node applications, too.

@charmander
Copy link
Collaborator

Note that this is a breaking change for anything setting or relying on pg.defaults.database.

@@ -50,8 +54,11 @@ var ConnectionParameters = function (config) {
config = Object.assign({}, config, parse(config.connectionString))
}

var dbDefaulting = function () { return this.user || defaults['database'] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible for this.user to be falsy?

Copy link
Author

@solidsnack solidsnack May 29, 2018

Choose a reason for hiding this comment

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

Looking at the implementation, it's set with val('user', config) which can be false when (a) there is no default set and (b) the corresponding environment variable is not set.

From the outside, URLs like postgres:/// don't have a user, so it stands to reason the implementation has to allow it to be falsey.

Copy link
Collaborator

@charmander charmander May 30, 2018

Choose a reason for hiding this comment

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

The default has to be set to something; if it’s undefined, the connection is going to fail anyway. I think this can just be const getDefaultDatabase = () => this.user. (At that point, it doesn’t even need to be a function – this.user can be passed in directly.)

@solidsnack
Copy link
Author

solidsnack commented May 29, 2018

Note that this is a breaking change for anything setting or relying on pg.defaults.database.

Perhaps there is some intermediate possible?

The current behavior is such that node-postgres interprets Postgres URLs in a manner different from libpq and other implementations. I guess the first question to answer is: is that something we're okay with or would we like to see Postgres URLs work the same with Node apps as they do with things like psql and psycopg2?

@charmander
Copy link
Collaborator

A breaking change just means it needs a major version increment.

@solidsnack
Copy link
Author

Maybe if we allowed defaults.database to be a function as well as a string, this could be a point release.

  • If it is not set, use the username.
  • If it is a string, use the string.
  • If it is a function, pass the parsed connection info into the function.

@brianc
Copy link
Owner

brianc commented Oct 3, 2018

I will merge this as soon as I cut the next major version. Sorry for the delay.

@brianc
Copy link
Owner

brianc commented Oct 3, 2018

Actually looks like 1679 supersedes this, so closing in favor of that one.

@brianc brianc closed this Oct 3, 2018
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.

3 participants