Skip to content

Conversation

@jacobwgillespie
Copy link
Contributor

Hey, it looks like as a side effect of makeRootDatabaseConnectionString() passing through pg-connection-string is that it fails to preserve some more complex connection string parameters, for instance sslrootcert. When that option is specified, it actually loads the certificate data, so parsed.ssl becomes {ca: '...'} rather than true.

This means that when makeRootDatabaseConnectionString() converts it back into a connection string, it adds ?ssl=%5Bobject%20Object%5D.

This PR switches to use the url module to just swap out the pathname, which in theory should be less disruptive than attempting to handle every possible connection parameter, as it preserves the connection URL that the user has provided (as a plus, this removes the // TODO: factor in other connection parameter). I'm not 100% familiar with why this was the way it was before, so I may be missing something, but hopefully this is more robust.

There are two commits in this PR, one to add a failing test, and one to modify makeRootDatabaseConnectionString(). I'm opening the PR with only the first so that Travis CI will start a build, then I'll push the 2nd.

@benjie
Copy link
Member

benjie commented Sep 1, 2020

I'm not 100% familiar with why this was the way it was before, so I may be missing something, but hopefully this is more robust

Honestly, I can't figure out what would have possessed me to write so much unnecessary (and less correct) code; I'm going to blame it on lack of sleep 🥱

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Thanks; this is great! I tweaked the code to use the if/else pattern rather than early returns (to match my personal preference).

@benjie benjie changed the title Fix issue where makeRootDatabaseConnectionString() does not preserve sslrootcert fix: preserve sslrootcert when rewriting URLs Sep 1, 2020
@benjie benjie merged commit 6be69a9 into graphile:main Sep 1, 2020
@benjie
Copy link
Member

benjie commented Sep 1, 2020

Released in v1.0.1

@jacobwgillespie jacobwgillespie deleted the root-db-url branch September 1, 2020 14:28
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.

2 participants