decidim / decidim

The participatory democracy framework. A generator and multiple gems made with Ruby on Rails
https://decidim.org/
GNU Affero General Public License v3.0
1.48k stars 405 forks source link

Changing image urls is not taken into account in certain cells #13422

Open microstudi opened 2 months ago

microstudi commented 2 months ago

Describe the bug

Cells in Decidim tend to be cached, being the cache hash generated with something like this:

      def background_image
        model.images_container.attached_uploader(:background_image).variant_url(:big)
      end

      def cache_hash
        hash = []
        ...
        hash << background_image

        hash.join(Decidim.cache_key_separator)
      end

This is good because it has a the url represntation in the cache_hash, that will force to renew the cache when it changes.

However, some cells that display images do not do that, I've identified that the ones inheriting from HighlightedParticipatorySpacesCell do not use the image url for the cache hash:

      def cache_hash
        [I18n.locale, highlighted_spaces.map(&:cache_key_with_version)].join(Decidim.cache_key_separator)
      end

Before the introduction of #12576 it didn't matter much because the links generated for the images always where directed to the ActiveStorage controller in rails with links that don't expire (in principle), then redirected to the final storage provider. But now, this URLs are generated with the final link to the provider. For instance, a S3 link goes like this:

https://PROVIDER-HOST/OBJECT_ID?response-content-disposition=...X-Amz-Expires=300

The problem comes that these type of links expires after certain time (5 minutes in the example) but they get cached when displayed in a cell.

This problem should be solved at the cell cache level, carefully reviewing all that might have images.

There's also an alternative: making the links public as it is described in the activestorage docs

# config/storage.yml
s3:
  service: S3
  public: true

However, this does not work because of another bug that I'll describe in another issue.

To Reproduce

  1. Use a Decidim instance with a cloud provider for ActiveStorage (S3 for instance)
  2. Ensure you have some active process with images
  3. Go to the home, see all the images correctly
  4. Wait 5 minutes (that might depend)
  5. See that the images for certain cells are broken

Expected behavior

Images should be always displayed, no matter how the URLs are generated

Screenshots

No response

Stacktrace

No response

Extra data

Additional context

No response

froger commented 2 months ago

Hello!
As I understand, fixing this issue would significantly impact the cache capabilities of Decidim (as we would need to avoid caching on image paths, and run query DB for every active storage images).

I am wondering if recommending using public: true or a CDN with S3-compatible object storage would be a better approach. I believe this issue could be solved with some documentation, allowing us to focus on the other issue you will describe. What do you think @microstudi ?

microstudi commented 2 months ago

I think this can be fixed just by including the image path in the cache_hash, just like the hero_cell. However it's true that the cache won't be a long one, every time the path changes the cache will be invalidated. This might be every 5 minutes or so.

About the public: true, yes that works but that requires that the aws provider uses the same domain for the public link than the private one. But this is not usually the case, for instance in AWS you need to name your bucket as the domain name you want to use as CDN, then add the option virtual_host: true. In other providers this might change and the gem aws-s3 does not provide an option to manually specify the domain for you S3 url. But you can monkeypatch the library. I haven't tested Azured or Google Cloud, I cannot offer information on that (but I do know that they also create expiring links).

decentral1se commented 1 month ago

This is also happening with the disk storage 😬 Over here.

When I run tmp:clear process images are back, but after 2-3 minutes, they 404.

microstudi commented 1 month ago

It is a concerning problem yes, there are various solutions, the easiest is to make sure to update to the last version, ensuring this PR is merged https://github.com/decidim/decidim/pull/13402 and deactivate the cache:

DECIDIM_CACHE_EXPIRATION_TIME=1 for instance

decentral1se commented 1 month ago

Our work-around so far is with the following in config/environments/production.rb running the latest 0.28.x stable. We've already just done a lengthy upgrade process through multiple versions...

config.cache_store = :null_store