Simply return `value` after write in active_record container backend
Simply return value after write in active_record container backend
This fixes a bug where #read is overwritten by the cache plugin and
therefore the container backend returns stale data. It should always
return the value it wrote.
Since value is never modified by this backend it makes no sense to
read it again after setting it.
Besides fixing the bug it might give a small perfomance boost, too.
fixes https://github.com/shioyama/mobility/pull/543/files#r1944637671
thanks @dramalho for uncovering the bug and hints
Lovely, thank you βΊοΈ
This seems reasonable to me, my only concern with returning value directly is how this would interact with e.g. the presence plugin. Currently I believe if you have the plugin enabled and assign "", you will get back nil, whereas with this change I think you will get back "". Can you check?
Currently I believe if you have the plugin enabled and assign "", you will get back nil, whereas with this change I think you will get back ""
I don't think this is how ruby works with setters if I am correct, e.g. any setter always returns the passed in value no matter what:
module Something
def self.bla
puts "calling getter"
@bla
end
def self.bla=(value)
puts "calling setter"
@bla = value
# try to return something different from setter
"something different"
end
end
Something.bla
# calling getter
# => nil
Something.bla = 'my value'
# calling setter
# => "my value"
Something.bla
# calling getter
# => "my value"
Or did you think of something different?
I added a spec though to check that the presence plugin is applied to the value written to the backend, e.g. it does not save "" to the the database. This still works because #write is overwritten by the plugin so the call chain still ensures the the presence plugin does its work (ActiveRecord::Container#write already gets the value which was modified by the presence plugin).
Thanks for this fix @doits ππ» It's always nice when someone has already fixed what seemed to be a very specific issue. π Any chance we could get this merged in @shioyama ?