Use IS [NOT] NULL instead of DBMS_LOB.COMPARE for nil CLOB/BLOB queries#2415
Use IS [NOT] NULL instead of DBMS_LOB.COMPARE for nil CLOB/BLOB queries#2415brianthoman wants to merge 7 commits intorsim:masterfrom
Conversation
lib/arel/visitors/oracle_common.rb
Outdated
| left = o.left | ||
|
|
||
| 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
| 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.
There was a problem hiding this comment.
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.
|
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.
There was a problem hiding this comment.
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.
…cs. Also, clean up CLOB specs a bit.
akostadinov
left a comment
akostadinov
left a comment
There was a problem hiding this comment.
Thank you for fixing this and for the updates! To me everything seems alright now.
2 participants
Closes #2410