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 2 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
2 changes: 2 additions & 0 deletions lib/arel/visitors/oracle_common.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ module OracleCommon
# Fixes ORA-00932: inconsistent datatypes: expected - got CLOB
def visit_Arel_Nodes_Equality(o, collector)
left = o.left

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

Choose a reason for hiding this comment

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

Just a nitpick, maybe create a variable right and check it for being nil earlier because it seems a more lightweight check than the other one. But it's fine as it is too.


# https://docs.oracle.com/cd/B19306_01/appdev.102/b14258/d_lob.htm#i1016668
# returns 0 when the comparison succeeds
Expand Down
32 changes: 32 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,32 @@ 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
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's too late here but I don't see a difference with the previous test.

end

it "should find serialized NULL BLOB data when queried with nil" do
Copy link
Contributor

Choose a reason for hiding this comment

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

here the test name appears to be the same as the previous one

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