mobility icon indicating copy to clipboard operation
mobility copied to clipboard

Simply return `value` after write in active_record container backend

Open doits opened this issue 11 months ago β€’ 4 comments

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

doits avatar Feb 09 '25 21:02 doits

Lovely, thank you ☺️

dramalho avatar Feb 09 '25 21:02 dramalho

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?

shioyama avatar Mar 19 '25 00:03 shioyama

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).

doits avatar Mar 19 '25 08:03 doits

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 ?

AndyStabler avatar Jul 04 '25 10:07 AndyStabler