Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect count when using ID in the dynamic column name proc #355

Closed
lserman opened this issue Jun 16, 2022 · 2 comments
Closed

Incorrect count when using ID in the dynamic column name proc #355

lserman opened this issue Jun 16, 2022 · 2 comments

Comments

@lserman
Copy link

lserman commented Jun 16, 2022

Hello, our model has messages that belong to groups and we want to track the group's outbound message count, but only count messages for each recipient once per group (sending a message to the same recipient in the same group multiple times should only increment the groups message count once).

To do this our dynamic column count looked like this:

counter_cache :group, column_name: message -> { message.counter_cache_column_name }

def counter_cache_column_name
  group.messages.where(recipient: recipient).where.not(id: id).exists? nil : :messages_count
end

After we create the message which increments the counter by 1, we send it off to Sidekiq where it is updated with it's delivery timestamp. At this point, the counter increments to 2.

This gem uses ActiveRecord's dup to check if the cache column has changed after each update. dup does not copy the record's ID, instead it sets id to nil. Because of that, the cache column was always "changing" as the dynamic column was checking where id IS NOT NULL, unexpectedly leading to a nil column name vs. the non-dupes column name of messages_count.

I am not sure of the proper fix or if there was a better way to get this functionality without running into this issue, but at the very least I'd like to document this behavior here for the next person as it was quite the goose chase.

@magnusvk
Copy link
Owner

Thanks, but this seems quite specific to your particular use of this. Going to close the issue since I don't think this is a problem with counter_culture.

@lserman
Copy link
Author

lserman commented Oct 12, 2022

OK. To summarize though, using id in your column name logic is broken as it will always evaluate to nil. I do think that it is a problem with counter_culture but one that may not be worth fixing. I can live with that 👍

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

No branches or pull requests

2 participants