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

Use user name as default database when user is non-default #1679

Merged
merged 2 commits into from
Jan 23, 2020
Merged

Use user name as default database when user is non-default #1679

merged 2 commits into from
Jan 23, 2020

Conversation

charmander
Copy link
Collaborator

@charmander charmander commented Jun 17, 2018 •
edited
Loading

Not entirely backwards-compatible. Improved version of #1660.

@charmander charmander added this to the [email protected] milestone Jun 17, 2018
@brianc
Copy link
Owner

brianc commented Aug 10, 2018

Ah I'm down with this. Should we close #1660 in favor of this one?

@charmander
Copy link
Collaborator Author

Yep, I think so.

@brianc
Copy link
Owner

brianc commented Jan 11, 2020

@charmander would you mind re-targeting bmc/8.0 branch instead of master? I'm gonna land all the PRs there & then do one merge + semver bump + release on master. 🙏

@charmander charmander changed the base branch from master to bmc/8.0 January 12, 2020 02:06
@brianc
Copy link
Owner

brianc commented Jan 16, 2020 •
edited
Loading

Just so I'm clear on the release notes here this change is something like...:

If a user name is not specified, available in the environment, or available at pg.defaults, we used to use the username of the process user as the name of the database. Now we will use the username supplied to the client, if it exists. What this means is if you used to have

new Client({
  username: 'foo'
})

We would default the database name to the process user where as now we will use the username supplied to the client. If you have not supplied username to the client, and it isn't available through any of its existing lookup mechanisms (environment variables, pg.defaults) then it will still use the process user for the database name.

(aside: we should just nuke pg.defaults in 9.0...I feel like it was a bad api design)

brianc
brianc approved these changes Jan 16, 2020
View reviewed changes
Copy link
Owner

@brianc brianc left a comment

Choose a reason for hiding this comment

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

👏 this is better, fo sho

@charmander
Copy link
Collaborator Author

@brianc Yep! That’s nicely detailed. (username should be user in the final version, mind.)

(aside: we should just nuke pg.defaults in 9.0...I feel like it was a bad api design)

I’m all for that too.

@brianc brianc merged commit 7aee261 into brianc:bmc/8.0 Jan 23, 2020
@charmander charmander deleted the user-as-default-database branch January 24, 2020 01:43
brianc pushed a commit that referenced this pull request Jan 28, 2020
@charmander @brianc
Use user name as default database when user is non-default (#1679)
c26caa8 
Not entirely backwards-compatible.
brianc added a commit that referenced this pull request Mar 30, 2020
@brianc @charmander
@gabegorelick @NatalieWolfe
8.0 Release (#2117)
aafd8ac 
* Drop support for EOL versions of node (#2062)

* Drop support for EOL versions of node

* Re-add testing for [email protected]

* Revert changes to .travis.yml

* Update packages/pg-pool/package.json

Co-Authored-By: Charmander <[email protected]>

Co-authored-by: Charmander <[email protected]>

* Remove password from stringified outputs (#2066)

* Remove password from stringified outputs

Theres a security concern where if you're not careful and you include your client or pool instance in console.log or stack traces it might include the database password.  To widen the pit of success I'm making that field non-enumerable.  You can still get at it...it just wont show up "by accident" when you're logging things now.

The backwards compatiblity impact of this is very small, but it is still technically somewhat an API change so...8.0.

* Implement feedback

* Fix more whitespace the autoformatter changed

* Simplify code a bit

* Remove password from stringified outputs (#2070)

* Keep ConnectionParameters’s password property writable

`Client` writes to it when `password` is a function.

* Avoid creating password property on pool options

when it didn’t exist previously.

* Allow password option to be non-enumerable

to avoid breaking uses like `new Pool(existingPool.options)`.

* Make password property definitions consistent

in formatting and configurability.

Co-authored-by: Charmander <[email protected]>

* Make `native` non-enumerable (#2065)

* Make `native` non-enumerable

Making it non-enumerable means less spurious "Cannot find module"
errors in your logs when iterating over `pg` objects.

`Object.defineProperty` has been available since Node 0.12.

See #1894 (comment)

* Add test for `native` enumeration

Co-authored-by: Gabe Gorelick <[email protected]>

* Use class-extends to wrap Pool (#1541)

* Use class-extends to wrap Pool

* Minimize diff

* Test `BoundPool` inheritance

Co-authored-by: Charmander <[email protected]>
Co-authored-by: Brian C <[email protected]>

* Continue support for creating a pg.Pool from another instance’s options (#2076)

* Add failing test for creating a `BoundPool` from another instance’s settings

* Continue support for creating a pg.Pool from another instance’s options

by dropping the requirement for the `password` property to be enumerable.

* Use user name as default database when user is non-default (#1679)

Not entirely backwards-compatible.

* Make native client password property consistent with others

i.e. configurable.

* Make notice messages not an instance of Error (#2090)

* Make notice messages not an instance of Error

Slight API cleanup to make a notice instance the same shape as it was, but not be an instance of error.  This is a backwards incompatible change though I expect the impact to be minimal.

Closes #1982

* skip notice test in travis

* Pin [email protected] for regression in async iterators

* Check and see if node 13.8 is still borked on async iterator

* Yeah, node still has changed edge case behavior on stream

* Emit notice messages on travis

* Revert "Revert "Support additional tls.connect() options (#1996)" (#2010)" (#2113)

This reverts commit 510a273.

* Fix ssl tests (#2116)

* Convert Query to an ES6 class (#2126)

The last missing `new` deprecation warning for pg 8.

Co-authored-by: Charmander <[email protected]>
Co-authored-by: Gabe Gorelick <[email protected]>
Co-authored-by: Natalie Wolfe <[email protected]>
@charmander charmander mentioned this pull request Nov 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brianc brianc brianc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
[email protected]
Development

Successfully merging this pull request may close these issues.
2 participants
@charmander @brianc