allow pg query param syntax by abeidahmed · Pull Request #42840 · rails/rails ·...

Copy link

Contributor

ActiveRecord::DatabaseConfigurations::UrlConfig.new(:production, :production, 'postgres:///?user=user&password=passwd&dbname=theapp_production', {}).configuration_hash

ActiveRecord::DatabaseConfigurations::UrlConfig.new(:production, :production, 'postgres://user:passwd@/theapp_production', {})

Navdevl commented on Jul 22

@abeidahmed Adding in the benchmarks if incase needed.

Sample code used:

require 'benchmark'

values = [nil, 'something']

record = {}
5000.times do |n|
  record[n] = values.sample
end

n = 100000

Benchmark.bm do |x|
  x.report { n.times do; record.reject! { |key, value| value.nil? }; end }
  x.report { n.times do; record.compact!; end }
end

The output is:

     user     system      total        real
12.511272   0.006561  12.517833 ( 12.527778)
 2.243834   0.001086   2.244920 (  2.246199)

@abeidahmed Regarding the Postgres connection string not following the RFC2396,

Here PostgreSQLAdapter doc, It is already documented that there are default values for the host attribute.

I also checked, and the ruby-pg accepts the non-URI Postgresql connection strings (which makes sense, but I decided to check anyway).

Sample code used:

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails", branch: "main"
  gem "pg"
end

connection_str = PG::Connection.parse_connect_args('postgresql://vagrant@/postgres')
connection = PG.connect(connection_str)
puts connection.conninfo_hash.slice(:user, :password, :port, :database, :host, :dbname)
result = connection.exec('CREATE TABLE users ( id serial PRIMARY KEY, name VARCHAR(50) );')
puts result.inspect
result = connection.exec("INSERT INTO users (name) VALUES ('John Doe 1');")
puts result.inspect
result = connection.exec("SELECT * FROM users;")
puts result.map(&:to_h)

connection_str = PG::Connection.parse_connect_args('postgresql://vagrant@/test')
connection = PG.connect(connection_str)
puts connection.conninfo_hash.slice(:user, :password, :port, :database, :host, :dbname)
result = connection.exec('CREATE TABLE users ( id serial PRIMARY KEY, name VARCHAR(50) );')
puts result.inspect
result = connection.exec("INSERT INTO users (name) VALUES ('John Doe 2');")
puts result.inspect
result = connection.exec("SELECT * FROM users;")
puts result.map(&:to_h)

So, for me, there are few options:

• create a separated parse for Postgres' connection strings and skip the URI parse since it doesn't follow RFC.
• skip the URI parse or rescue the error and build the connection hash later based on connection.conninfo_hash. It doesn't feel a good solution for me though.
• Other options could be to document that the ConnectionUrlResolver only supports connection strings that follow the RFC2396. In this case, it will not support all kinds of Postgres connection strings.

p8 commented on Jul 24 •

Thanks for the PR @abeidahmed!
Can you add a test?
And could you also rebase against main? The failing CI should be fixed.

abeidahmed commented on Jul 25

@abeidahmed Regarding the Postgres connection string not following the RFC2396,

Here PostgreSQLAdapter doc, It is already documented that there are default values for the host attribute.

I also checked, and the ruby-pg accepts the non-URI Postgresql connection strings (which makes sense, but I decided to check anyway).

Sample code used:

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails", branch: "main"
  gem "pg"
end

connection_str = PG::Connection.parse_connect_args('postgresql://vagrant@/postgres')
connection = PG.connect(connection_str)
puts connection.conninfo_hash.slice(:user, :password, :port, :database, :host, :dbname)
result = connection.exec('CREATE TABLE users ( id serial PRIMARY KEY, name VARCHAR(50) );')
puts result.inspect
result = connection.exec("INSERT INTO users (name) VALUES ('John Doe 1');")
puts result.inspect
result = connection.exec("SELECT * FROM users;")
puts result.map(&:to_h)

connection_str = PG::Connection.parse_connect_args('postgresql://vagrant@/test')
connection = PG.connect(connection_str)
puts connection.conninfo_hash.slice(:user, :password, :port, :database, :host, :dbname)
result = connection.exec('CREATE TABLE users ( id serial PRIMARY KEY, name VARCHAR(50) );')
puts result.inspect
result = connection.exec("INSERT INTO users (name) VALUES ('John Doe 2');")
puts result.inspect
result = connection.exec("SELECT * FROM users;")
puts result.map(&:to_h)

So, for me, there are few options:

• create a separated parse for Postgres' connection strings and skip the URI parse since it doesn't follow RFC.
• skip the URI parse or rescue the error and build the connection hash later based on connection.conninfo_hash. It doesn't feel a good solution for me though.
• Other options could be to document that the ConnectionUrlResolver only supports connection strings that follow the RFC2396. In this case, it will not support all kinds of Postgres connection strings.

This would be helpful. Thank you. Let me give this one more shot!

abeidahmed commented on Jul 26

Hey @p8, I added some tests. Could you please take a look and let me know? Thanks!

abeidahmed commented on Aug 2

Friendly bump.

p8 commented on Aug 3

@abeidahmed this looks good to me. Could you also add a Changelog entry? Then we can wrap this up.

abeidahmed commented on Aug 10

Is there anything left to do in this PR? If not, can we merge this? Thanks.