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

Integrating generic connection-string #32

Closed
wants to merge 22 commits into from

Conversation

vitaly-t
Copy link

@vitaly-t vitaly-t commented Jul 6, 2019


Many tests now can be retired, because this library doesn't do the parsing anymore. But for now I kept as many tests as I could.

This implementation uses connection-string underneath, while implementing the functionality as close as possible to the original, and to keep it mostly compatible with the existing driver.

This first version ignores multi-host support, but makes it very easy to activate, once the driver starts supporting it, since connection-string supports it out-of-the-box.

However, some functionality had to be changed, and below is the list of all breaking changes...

  • Unix sockets support is now much simpler: the host name either ends with .sock, or contains / after decoding.
  • Special symbols in the user name or password need to be URL-encoded, the same as any other part of the URL, as it must be compliant with the URL Connection String standard.
  • Since database now always comes from the first path segment, there is no more need in custom parameter db. In the simplest form, /dbname results in the database set correctly.
  • Minor changes as to the short-syntax expectations now reflect what the URL spec requires. For example, passing in just some-name sets the host, not the database name. For the database shortest syntax it is /some-name, and for just host + database, it is host/database. See more examples here.

For other references, see connection-string. It supports abbreviations, defaults, string generation, etc, many nice features.

UPDATE

I have added a good usage example - connection-string-demo.

@vitaly-t vitaly-t changed the title Fix tests for Windows Integrating connection-string Jul 6, 2019
@vitaly-t vitaly-t changed the title Integrating connection-string Integrating generic connection-string Jul 6, 2019
@vitaly-t vitaly-t mentioned this pull request Jul 6, 2019
correcting comment for `.sock` ending.
Improving comments, not to mention `/` support, as both primary and secondary syntax support it the same - encoded.
improving tests.
updating connection-string version.
dependency update.
dependency update.
fixing tests.
@vitaly-t
Copy link
Author

vitaly-t commented Jul 8, 2019

@monteslu Do you think this may happen? 😄

@abowerman
Copy link
Contributor

Will review, is this still work in progress?

@vitaly-t
Copy link
Author

vitaly-t commented Jul 8, 2019

is this still work in progress?

@abowerman nope, it is all done and good to go! :)

Upgrading the dependency, for better Unix sockets support.
Simplifying support for Unix sockets.
Fixing tests for the new sockets syntax.
@vitaly-t
Copy link
Author

vitaly-t commented Jul 9, 2019

@abowerman I have made some important improvements as to how Unix sockets are supported, both here and in the connection-string. It is so much simpler now, and more logical.

No more dual syntax for sockets! 😄

And I have documented it here:

Q: How can I specify a Unix-socket host?

A: Any host name that either ends with .sock or contains /, after decoding, is recognized as a Unix socket, with the host's type property set to socket after parsing.

Copy link
Contributor

@hjr3 hjr3 left a comment

Choose a reason for hiding this comment

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

The code all looks good to me.

I am loathe to introduce breaking changes though. This package has been stable for a long time and I think a major version bump will still require long term support for the 2.x.x release. My preference would be to replace the parser with the generic connection-string but maintain backwards compatibility.

@vitaly-t
Copy link
Author

vitaly-t commented Jul 9, 2019

@hjr3 Considering that node-postgres is still stuck with v0.1.3, there won't be any progress, unless we make it 😉

My preference would be to replace the parser with the generic connection-string but maintain backwards compatibility.

This is effectively what we are doing here, the package now simply provides the backward compatibility, nothing more, although not entirely, because 100% is impossible at this point, that's why we bump the version.

This will in turn show how easy it is indeed to get node-postgres to move toward using connection-string directly. But, one step at a time 😄

dependency update
@vitaly-t
Copy link
Author

@abowerman Is it ok to merge it now?

dependency update
@abowerman
Copy link
Contributor

Looks like you are still making changes here.

@vitaly-t
Copy link
Author

@abowerman I've been making improvements to the library, and here only updating dependencies, nothing else. This shouldn't stop you from merging the PR ;)

@vitaly-t
Copy link
Author

@abowerman Just checking back, as it's been 12 days since...

@vitaly-t
Copy link
Author

vitaly-t commented Jul 27, 2019

@abowerman just pinging ;) Can it be merged, please?

@vitaly-t
Copy link
Author

How come we lost all the response here after...?

@hjr3 @abowerman Could someone, please review and merge it?

@abowerman
Copy link
Contributor

Am reluctant to introduce dependencies and breaking changes here. This is fairly heavily used and currently does not have any dependencies. @hjr3 @monteslu Thoughts?

@vitaly-t
Copy link
Author

vitaly-t commented Aug 20, 2019

For one thing, it is marked as version 3.0.0, within the PR itself. And for another, the driver has been stuck on the ancient version anyhow. So it won't break anything, if we just follow the major version increment with this. Because what this really does more than anything, brings in a well-known standard of the connection URL, and getting rid of proprietary patches here and there.

@hjr3
Copy link
Contributor

hjr3 commented Aug 21, 2019

This change guts the code base and makes pg-connection-string a thin wrapper around connection-string. The code is looks fine, but there is something that just feels wrong about it.

This is an old repository. It is depended on by a lot of people. The repository itself has a reputation behind it. There is a (loose) governance model. By merging this PR, then we give all that up. Any bug or feature request will have to be redirected to connection-string. The go-forward role for the current maintainers will only be bumping the version of connection-string periodically.

Maybe connection-string is the better option though! @vitaly-t is very motivated to improve the current state of connection string parsing and this repository has been pretty quiet. If connection-string is the better option, we should probably deprecate this repository and redirect people to connection-string. No reason to make a 3.0.0 version of this repository.

My vote is to update the README that this repository is deprecated, the .2.x.x branch will receive security updates only and people should start migrating to connection-string.

@vitaly-t
Copy link
Author

vitaly-t commented Aug 21, 2019

@hjr3 The important thing, issues around the lack of code consolidation keep rising, but nothing is being done about it. Here's the last one, just from today, that's why I'm trying to revive it again.

b.t.w. I'm not only the developer of connection-string, but also developer of pg-promise, and an active user of the entire platform myself. I am currently in between a few projects - all using it, so yes, I am concerned :)

@hjr3
Copy link
Contributor

hjr3 commented Aug 21, 2019

Yup, I hear you. I am all for node-postgres updating its dependency to be on connection-string instead.

@vitaly-t
Copy link
Author

vitaly-t commented Aug 29, 2019

As part of moving all my projects into native TypeScript, I've just published a pre-release update for this one - 3.0.0-Beta.0.

updating `connection-string` dependency.
updating for use of the latest `connection-string` as a class-only.
@vitaly-t
Copy link
Author

@abowerman, @hjr3

Updated to use the new 3.x version of connection-string, re-done in TypeScript.

@qfox
Copy link

qfox commented Oct 5, 2019

Hello fellows. Am I right that better to migrate node-postgres to connection-string and keep pg-connection-string as is?
I have some time to work on it and deciding what to do

@vitaly-t
Copy link
Author

vitaly-t commented Oct 5, 2019

@zxqfox For the lack of the progress either way, to move toward a better generic approach, I no longer want to advocate any kind of integration. For people who want to use connection-string, they can just do so independently, and generate a connection object from it to be passed into the driver, as shown within the Adapters.

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.

Doesn't respect host in query parameters
4 participants