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

Use IS [NOT] NULL instead of DBMS_LOB.COMPARE for nil CLOB/BLOB queries #2415

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 4 additions & 1 deletion lib/arel/visitors/oracle_common.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,14 @@ module OracleCommon
# Fixes ORA-00932: inconsistent datatypes: expected - got CLOB
def visit_Arel_Nodes_Equality(o, collector)
left = o.left
right = o.right

return super if right.nil?
return super unless %i(text binary).include?(cached_column_for(left)&.type)

# https://docs.oracle.com/cd/B19306_01/appdev.102/b14258/d_lob.htm#i1016668
# returns 0 when the comparison succeeds
comparator = Arel::Nodes::NamedFunction.new("DBMS_LOB.COMPARE", [left, o.right])
comparator = Arel::Nodes::NamedFunction.new("DBMS_LOB.COMPARE", [left, right])
collector = visit comparator, collector
collector << " = 0"
collector
Expand Down
25 changes: 25 additions & 0 deletions spec/active_record/oracle_enhanced/type/binary_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
end
class ::TestEmployee < ActiveRecord::Base
end
class ::TestSerializedHashEmployee < ActiveRecord::Base
self.table_name = "test_employees"
serialize :binary_data, type: Hash, coder: YAML
end
@binary_data = "\0\1\2\3\4\5\6\7\8\9" * 10000
@binary_data2 = "\1\2\3\4\5\6\7\8\9\0" * 10000
end
Expand Down Expand Up @@ -116,4 +120,25 @@ class ::TestEmployee < ActiveRecord::Base
@employee.reload
expect(@employee.binary_data).to eq(@binary_data)
end

it "should find NULL BLOB data when queried with nil" do
TestEmployee.delete_all
TestEmployee.create!(binary_data: nil)
TestEmployee.create!(binary_data: @binary_data)
expect(TestEmployee.where(binary_data: nil)).to have_attributes(count: 1)
end

it "should find serialized NULL BLOB data when queried with nil" do
TestSerializedHashEmployee.delete_all
TestSerializedHashEmployee.create!(binary_data: nil)
TestSerializedHashEmployee.create!(binary_data: { data: 'some data' })
expect(TestSerializedHashEmployee.where(binary_data: nil)).to have_attributes(count: 1)
end

it "should find serialized NULL BLOB data when queried with {}" do
TestSerializedHashEmployee.delete_all
TestSerializedHashEmployee.create!(binary_data: nil)
TestSerializedHashEmployee.create!(binary_data: { data: 'some data' })
expect(TestSerializedHashEmployee.where(binary_data: {})).to have_attributes(count: 1)
end
end
25 changes: 25 additions & 0 deletions spec/active_record/oracle_enhanced/type/text_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ class ::TestEmployee < ActiveRecord::Base; end
class ::Test2Employee < ActiveRecord::Base
serialize :comments
end
class ::TestSerializedHashEmployee < ActiveRecord::Base
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this class name should be Test2SerializedHashEmployee because in spec/active_record/oracle_enhanced/type/binary_spec.rb there is already a class TestSerializedHashEmployee.

Copy link
Author

Choose a reason for hiding this comment

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

I thought those classes were scoped to the describe block, so there shouldn't be any conflict. E.g., there's already a TestEmployee class defined in both binary_spec.rb and text_spec.rb.

Happy to make this change though, if that's the safer route.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the describe block scopes these anyhow, I don't see how it would. More likely to me, Ruby just reopens the class and changes it as it goes which seems messy to me.

self.table_name = "test_employees"
serialize :comments, type: Hash, coder: YAML
end
class ::TestEmployeeReadOnlyClob < ActiveRecord::Base
self.table_name = "test_employees"
attr_readonly :comments
Expand Down Expand Up @@ -241,4 +245,25 @@ class ::TestSerializeEmployee < ActiveRecord::Base
)
expect(Test2Employee.where(comments: search_data)).to have_attributes(count: 1)
end

it "should find NULL CLOB data when queried with nil" do
TestEmployee.delete_all
TestEmployee.create!(comments: nil)
TestEmployee.create!(comments: @char_data)
expect(TestEmployee.where(comments: nil)).to have_attributes(count: 1)
end

it "should find serialized NULL CLOB data when queried with nil" do
Test2Employee.delete_all
Test2Employee.create!(comments: nil)
Test2Employee.create!(comments: { some: 'text' })
expect(Test2Employee.where(comments: nil)).to have_attributes(count: 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I understand, this test should be using the TestSerializedHashEmployee or TestSerializedHashEmployee2 class. Because it is named ... serialized .... Let me know if I didn't get your idea right.

end

it "should find serialized NULL CLOB data when queried with {}" do
TestSerializedHashEmployee.delete_all
TestSerializedHashEmployee.create!(comments: nil)
TestSerializedHashEmployee.create!(comments: { some: 'text' })
expect(TestSerializedHashEmployee.where(comments: {})).to have_attributes(count: 1)
end
end