Use IS [NOT] NULL instead of DBMS_LOB.COMPARE for nil CLOB/BLOB queries #2415
Conversation
lib/arel/visitors/oracle_common.rb
Outdated
| return super unless %i(text binary).include?(cached_column_for(left)&.type) | ||
| return super if o.right.nil? |
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
Maybe it's too late here but I don't see a difference with the previous test.
| expect(TestEmployee.where(binary_data: nil)).to have_attributes(count: 1) | ||
| end | ||
|
|
||
| it "should find serialized NULL BLOB data when queried with nil" do |
There was a problem hiding this comment.
here the test name appears to be the same as the previous one
| @@ -31,6 +31,10 @@ class ::TestEmployee < ActiveRecord::Base; end | |||
| class ::Test2Employee < ActiveRecord::Base | |||
| serialize :comments | |||
| end | |||
| class ::TestSerializedHashEmployee < ActiveRecord::Base | |||
There was a problem hiding this comment.
maybe this class name should be Test2SerializedHashEmployee because in spec/active_record/oracle_enhanced/type/binary_spec.rb there is already a class TestSerializedHashEmployee.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Thank you for the review @akostadinov! I made some changes based on your comments. |
| Test2Employee.delete_all | ||
| Test2Employee.create!(comments: nil) | ||
| Test2Employee.create!(comments: { some: 'text' }) | ||
| expect(Test2Employee.where(comments: nil)).to have_attributes(count: 1) |
There was a problem hiding this comment.
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.
Closes #2410