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

PG set stringtype=unspecified to handle string/uuid edge case #1087

Open
wants to merge 7 commits into
base: 61-stable
Choose a base branch
from
Open

PG set stringtype=unspecified to handle string/uuid edge case #1087

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
34a2bd9
Update for next significant Rails version
enebo Dec 17, 2020
8a850a5
Add string/uuid WHERE IN tests
imustafin Mar 8, 2021
947069a
Fallback to #to_s in extract_raw_bin_values
imustafin Mar 9, 2021
a31a822
PG connection stringtype=unspecified, remove string/uuid checks
imustafin Mar 9, 2021
2396e68
Merge branch '61-stable' into pg-uuid-casts
enebo Jul 29, 2021
195f713
Remove respond_to? check since Rails 6.1.4 reverted needing it
enebo Jul 29, 2021
5e37a86
Update lib/arjdbc/postgresql/connection_methods.rb
enebo Jul 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/arjdbc/abstract/core.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ def translate_exception(exception, message:, sql:, binds:)
end

def extract_raw_bind_values(binds)
binds.map(&:value_for_database)
Copy link
Member

Choose a reason for hiding this comment

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

Is this something only missing for postgresql?

Copy link
Author

Choose a reason for hiding this comment

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

I'm really not sure. This change is not really relevant to the purpose of this PR.

Also, #1088 (comment) reports that this should not be an issue in Rails 6.1.4

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah I guess we will not worry about that one. I am trying to get our CI working again. Looks like some changes in bundler and retrieving via git was messed up. Once I get it ok I will rerun this.

binds.map { |b| b.respond_to?(:value_for_database) ? b.value_for_database : b.to_s }
end

# this version of log() automatically fills type_casted_binds from binds if necessary
Expand Down
1 change: 1 addition & 0 deletions lib/arjdbc/postgresql/connection_methods.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ def postgresql_connection(config)
config[:username] ||= ( config[:user] || ENV['PGUSER'] || ENV_JAVA['user.name'] )
config[:password] ||= ENV['PGPASSWORD'] unless config.key?(:password)
properties = ( config[:properties] ||= {} )
properties['stringtype'] = 'unspecified' # for simple strings looking like UUIDs
enebo marked this conversation as resolved.
Show resolved Hide resolved
# PG :connect_timeout - maximum time to wait for connection to succeed
if connect_timeout = ( config[:connect_timeout] || ENV['PGCONNECT_TIMEOUT'] )
properties['socketTimeout'] ||= connect_timeout
Expand Down
28 changes: 0 additions & 28 deletions src/java/arjdbc/postgresql/PostgreSQLRubyJdbcConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ public class PostgreSQLRubyJdbcConnection extends arjdbc.jdbc.RubyJdbcConnection
private static final long serialVersionUID = 7235537759545717760L;
private static final int HSTORE_TYPE = 100000 + 1111;
private static final Pattern doubleValuePattern = Pattern.compile("(-?\\d+(?:\\.\\d+)?)");
private static final Pattern uuidPattern = Pattern.compile("\\{?\\p{XDigit}{4}(?:-?(\\p{XDigit}{4})){7}\\}?"); // Fuzzy match postgres's allowed formats

private static final Map<String, Integer> POSTGRES_JDBC_TYPE_FOR = new HashMap<>(32, 1);
static {
Expand Down Expand Up @@ -650,33 +649,6 @@ private void setRangeParameter(final ThreadContext context,
statement.setObject(index, pgRange);
}

@Override
protected void setStringParameter(final ThreadContext context,
final Connection connection, final PreparedStatement statement,
final int index, final IRubyObject value,
final IRubyObject attribute, final int type) throws SQLException {

if ( attributeSQLType(context, attribute) == context.nil ) {
/*
We have to check for a uuid here because in some cases
(for example, when doing "exists?" checks, or with legacy binds)
ActiveRecord doesn't send us the actual type of the attribute
and Postgres won't compare a uuid column with a string
*/
final String uuid = value.toString();
int length = uuid.length();

// Checking the length so we don't have the overhead of the regex unless it "looks" like a UUID
if (length >= 32 && length < 40 && uuidPattern.matcher(uuid).matches()) {
setUUIDParameter(statement, index, uuid);
return;
}
}

super.setStringParameter(context, connection, statement, index, value, attribute, type);
}


private void setUUIDParameter(final PreparedStatement statement,
final int index, final IRubyObject value) throws SQLException {
setUUIDParameter(statement, index, value.toString());
Expand Down
18 changes: 16 additions & 2 deletions test/db/postgresql/types_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ def self.startup
execute "CREATE TABLE postgresql_uuids (" <<
" id SERIAL PRIMARY KEY," <<
" guid uuid," <<
" compact_guid uuid" <<
" compact_guid uuid," <<
" string varchar(50)" <<
");"

execute("CREATE TABLE postgresql_ranges (" <<
Expand Down Expand Up @@ -237,7 +238,7 @@ def setup

@connection.execute("INSERT INTO postgresql_timestamp_with_zones (id, time) VALUES (1, '2010-01-01 10:00:00-1')")

@connection.execute("INSERT INTO postgresql_uuids (id, guid, compact_guid) VALUES(1, 'd96c3da0-96c1-012f-1316-64ce8f32c6d8', 'f06c715096c1012f131764ce8f32c6d8')")
@connection.execute("INSERT INTO postgresql_uuids (id, guid, compact_guid, string) VALUES(1, 'd96c3da0-96c1-012f-1316-64ce8f32c6d8', 'f06c715096c1012f131764ce8f32c6d8', '3a66d5d3-c18e-4437-889a-a7c1037bf4ad')")
@first_uuid = PostgresqlUUID.find(1)
end

Expand Down Expand Up @@ -570,6 +571,19 @@ def test_network_address_values_ipaddr
def test_uuid_values
assert_equal 'd96c3da0-96c1-012f-1316-64ce8f32c6d8', @first_uuid.guid
assert_equal 'f06c7150-96c1-012f-1317-64ce8f32c6d8', @first_uuid.compact_guid
assert_equal '3a66d5d3-c18e-4437-889a-a7c1037bf4ad', @first_uuid.string
end

def test_query_string_like_uuid_where_in
assert_equal 1, PostgresqlUUID.where(:string => [
'3a66d5d3-c18e-4437-889a-a7c1037bf4ad', 'not uuid'
]).first.id
end

def test_query_uuid_where_in
assert_equal 1, PostgresqlUUID.where(:guid => [
'd96c3da0-96c1-012f-1316-64ce8f32c6d8', 'a2170479-a3a2-42df-b80f-ae78f99cdca7'
]).first.id
end

def test_bit_string_values
Expand Down