Skip to content

Conversation

@doits
Copy link
Contributor

@doits doits commented Feb 9, 2025

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

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
@dramalho
Copy link

dramalho commented Feb 9, 2025

Lovely, thank you ☺️


expect { post.title = 'aaa' }.to change { post.title }.from(nil).to('aaa')
end
end
Copy link
Owner

Choose a reason for hiding this comment

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

Thank you for including a test!

@shioyama
Copy link
Owner

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?

@doits doits force-pushed the fix_container_cache branch from 050d333 to d06c738 Compare March 19, 2025 08:01
@doits doits force-pushed the fix_container_cache branch from d06c738 to 790d77c Compare March 19, 2025 08:03
@doits
Copy link
Contributor Author

doits commented Mar 19, 2025

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

@AndyStabler
Copy link

AndyStabler commented Jul 4, 2025

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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants