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

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
29c8d6c
Use IS [NOT] NULL instead of DBMS_LOB.COMPARE for nil CLOB/BLOB queries
Oct 16, 2024
e871d2a
Define test models for Serialized LOB columns
Oct 21, 2024
33ec70a
Slightly adjust the check for nil when comparing LOBs
Nov 19, 2024
306871d
Remove duplicate spec
Nov 19, 2024
51b7b88
Remove new test class after specs run to avoid conflicts in other spe…
Dec 7, 2024
5f07e23
Update BLOB specs to match CLOB specs in text_spec
Dec 7, 2024
c8afffa
Fix Rubocop offenses
Dec 7, 2024
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, you're right. I think I conflated this with my understanding of helper methods in RSpec.

I also overlooked that the existing specs seem to be dealing with this using remove_const after all the specs have run.

I've added a remove_const to clean up the new test class I added. Do you think that addresses the issue? If not, I'll finally give in and change the name 😅. Maybe to something like TextSpecSerializedHashEmployee so we don't have to keep track of what Test Employee number we're up to across files.

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.

Copy link
Author

Choose a reason for hiding this comment

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

I think TestEmployee2 or TestSerializeEmployee should work for this test case, as both have serialize :comments, but I switched it to TestSerializeEmployee to keep things matched up a little better.

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