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

Proposals listing caching has broken the listing and searching under Rails 5.1 default configurations #7513

Closed
ahukkanen opened this issue Mar 2, 2021 · 12 comments · Fixed by #7532
Labels
type: bug Issues that describe a bug

Comments

@ahukkanen
Copy link
Contributor

ahukkanen commented Mar 2, 2021

Describe the bug
When ordering the proposals with the "recent" ordering, the order seems broken (see the screenshot or go to see the issue at Metadecidim).

There are two separate issues regarding this that are most likely technically related:

  1. The order is incorrect, e.g. proposals from February and November 2019 comes 4th, 5th and 6th in the results and after them the next result seems correct (February 2021)
  2. The result listing produces duplicate records (see the exclamation marks in the attached screenshot)

There is also lots of other issues related to this. Basically there are collisions with the cache keys of the proposal cards (see comments below).

To Reproduce
Steps to reproduce the behavior:

  1. Go to https://meta.decidim.org/processes/roadmap/f/122/proposals?order=recent
  2. (Alternatively, go without the "order" parameter and change the order yourself from the dropdown)
  3. Wait for the page to load
  4. See incorrect ordering and possible duplicates in the results

Expected behavior
I would expect the proposals to come ordered by their created_at dates when I change proposal ordering to "Recent".

Screenshots
metadecidim-proposals-recent-order

Stacktrace
No stacktrace, the issue is most likely with the filtering or the base query used to fetch the records.

EDIT: The issue is related to caching, see comments below.

Extra data (please complete the following information):

  • Device: Laptop
  • Device OS: Ubuntu 20.04
  • Browser: Chrome
  • Decidim Version: 0.24.0.rc1
  • Decidim installation: MetaDecidim

Additional context
Possibly incorrect joins in the query causing the results to get the created_at date from some other record and causing the results to produce duplicates?

Did not dig deeper, just seen similar issues in the past caused by incorrect joins.

EDIT: The issue is related to caching, see comments below.

@ahukkanen
Copy link
Contributor Author

Looking at the version history, this might be related to proposal caching added at #6808 by @armandfardeau. Caching is generally "famous" for creating such issues along the way.

The cache key generation looks like a potential source of the problem:

It uses cache_version which is generated using the updated_at timestamps:
https://github.com/rails/rails/blob/63d3f3f4d868a5ed9eacf00af2a80278aa005051/activerecord/lib/active_record/integration.rb#L98-L102

Documentation or `cache_version says the following:

Returns a cache version that can be used together with the cache key to form a recyclable caching scheme.

https://api.rubyonrails.org/classes/ActiveRecord/Integration.html#method-i-cache_version

Not sure about this but it is one potential source for this.

Maybe we could test temporarily disabling caching at Metadecidim to test this hypothesis?

@ahukkanen
Copy link
Contributor Author

I am actually pretty sure this is cache related issue @armandfardeau. Proposal search is also broken at Metadecidim because of this issue. And I would expect a lot more issues around this.

Rails default configurations for caching have changed in Rails 5.2:
https://guides.rubyonrails.org/configuring.html#for-5-2-defaults-from-previous-versions-below-and

config.active_record.cache_versioning: true

Metadecidim is loading defaults from Rails 5.1:
https://github.com/decidim/metadecidim/blob/6339d1685b83df975a24e2bbea83e967bc5ba5c9/config/application.rb#L12

If you match the config.load_defaults with Metadecidim, this is what I get:

irb(main):001:0> Decidim::Proposals::Proposal.cache_versioning
=> false
irb(main):002:0> Decidim::Proposals::Proposal.last.cache_version
=> nil

And if this is nil, the cache key generation generates lots of potential duplicates:

def cache_hash
hash = []
hash << "decidim/proposals/proposal_m"
hash << I18n.locale.to_s
hash << model.cache_version
hash << model.proposal_votes_count
hash << model.endorsements_count
hash << Digest::MD5.hexdigest(model.component.settings.to_json)
hash << Digest::MD5.hexdigest(resource_image_path) if resource_image_path
if current_user
hash << current_user.cache_version
hash << current_user.follows?(model) ? 1 : 0
end
hash << Digest::MD5.hexdigest(model.followers.to_json)
hash << Digest::MD5.hexdigest(model.coauthorships.map(&:cache_version).to_s)
hash.join("/")
end

@ahukkanen ahukkanen changed the title Proposals recent ordering broken Proposals listing caching has broken the listing and searching under Rails 5.1 default configurations Mar 3, 2021
@mrcasals
Copy link
Contributor

mrcasals commented Mar 3, 2021

@ahukkanen independently of the defaults, this could be solved by adding the proposal ID to the cache key, right?

@ahukkanen
Copy link
Contributor Author

@mrcasals Yes, I was looking at the module methods and there is also cache_key_with_version (since Rails 5.2):
https://api.rubyonrails.org/classes/ActiveRecord/Integration.html#method-i-cache_key_with_version

@mrcasals
Copy link
Contributor

mrcasals commented Mar 3, 2021

Ah, it looks good! Replacing cache_version with cache_key_with_version should solve the issue, then, right?

@ahukkanen
Copy link
Contributor Author

@mrcasals Yes, I think so but would have to be tested.

@mrcasals mrcasals added the type: bug Issues that describe a bug label Mar 3, 2021
@ahukkanen
Copy link
Contributor Author

There is by the way another place too where cache_version is used (hero content block):

hash << current_organization.cache_version

I don't think this would generate duplicates that easily because it calculates a digest of the hero block's attributes (which are likely to be different for different organizations) but just in case, maybe that should be changed too?

@armandfardeau
Copy link
Contributor

Hi! Sorry for the mess I'm working on a fix.

@ahukkanen
Copy link
Contributor Author

@mrcasals I believe you've deployed this already to Metadecidim?

Because this issue still persists but I'm not sure if it's related to the cache key.

@mrcasals
Copy link
Contributor

mrcasals commented Mar 8, 2021

@ahukkanen no, it's not deployed yet because it wasn't backported. I'll try to make the backport when I find a little time.

@mrcasals
Copy link
Contributor

mrcasals commented Mar 8, 2021

@ahukkanen deployed already, seems to be working fine for me!

@ahukkanen
Copy link
Contributor Author

Great @mrcasals, seems so to me as well!

I think I might have found one more issue with this, though. I'll first confirm and then post a bug report (if I can confirm it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Issues that describe a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants