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

Changing an organization's image should invalidate the associated articles cached organization #17041

Closed
jeremyf opened this issue Mar 29, 2022 · 2 comments · Fixed by #17052
Closed
Assignees
Labels
area: homepage homepage and feed

Comments

@jeremyf
Copy link
Contributor

jeremyf commented Mar 29, 2022

Current behavior:

Given an article A that is associated with organization B.
When I update the organization B's image.
Then article A's organization image does not reflect the new image for B.

Expected behavior:

Given an article A that is associated with organization B.
When I update the organization B's image.
Then article A's organization image reflects the new image for B.

In the below image, I would expect that if I changed the DEV image to something else that the DEV icon behind Gracie would reflect that change.

Screen Shot 2022-03-28 at 9 52 45 PM

The following code highlights how we cache the article's organization:

<% if story.cached_organization && !@organization_article_index %>
<a class="crayons-logo crayons-logo--l" href="/<%= story.cached_organization.slug %>">
<img alt="<%= story.cached_organization.name %> logo" src="<%= story.cached_organization.profile_image_90 %>" class="crayons-logo__image" loading="lazy" />
</a>
<% end %>

The current work around is that you would need to edit all articles to recalculate the cache.

@jeremyf jeremyf added the area: homepage homepage and feed label Mar 29, 2022
@github-actions
Copy link
Contributor

Thanks for the issue, we will take it into consideration! Our team of engineers is busy working on many types of features, please give us time to get back to you.

Feature requests that require more discussion may be closed. Read more about our feature request process on forem.dev.

To our amazing contributors: issues labeled type: bug are always up for grabs, but for feature requests, please wait until we add a ready for dev before starting to work on it.

To claim an issue to work on, please leave a comment. If you've claimed the issue and need help, please ping @forem/oss. The OSS Community Manager or the engineers on OSS rotation will follow up.

For full info on how to contribute, please check out our contributors guide.

jeremyf added a commit that referenced this issue Mar 29, 2022
In helping track down #17041 I was looking at our cache
busting logic.  We were looking up a constant via the `.const_get` call
from a string already defined inline.  This refactor short-circuits
naming a string, capitalizing it, then looking in the class's registered
constants.

There's also a subtle bug in the original; When `provider` equals
"another_cache", the `#capitalize` method would return
`"Another_cache"`, whereas `#classify` will return `"AnotherCache"`.

Below are some benchmarks for the change.

```ruby
require "benchmark"

module EdgeCache
  class Bust
    def by_class
      Fastly
    end

    def by_const_get
      self.class.const_get("fastly".classify)
    end
  end
end

Benchmark.bmbm do |x|
  x.report("by_class") do
    1000.times do
      EdgeCache::Bust.new.by_class
    end
  end
  x.report("by_const_get") do
    1000.times do
      EdgeCache::Bust.new.by_const_get
    end
  end
end
```

```shell
> bin/rails runner /Users/jfriesen/git/forem/bench.rb

Rehearsal ------------------------------------------------
by_class       0.001239   0.000186   0.001425 (  0.001342)
by_const_get   0.011398   0.000136   0.011534 (  0.011538)
--------------------------------------- total: 0.012959sec

                   user     system      total        real
by_class       0.000973   0.000030   0.001003 (  0.000969)
by_const_get   0.011102   0.000032   0.011134 (  0.011104)
```
jeremyf added a commit that referenced this issue Mar 29, 2022
In helping track down #17041 I was looking at our cache
busting logic.  We were looking up a constant via the `.const_get` call
from a string already defined inline.  This refactor short-circuits
naming a string, capitalizing it, then looking in the class's registered
constants.

There's also a subtle bug in the original; When `provider` equals
"another_cache", the `#capitalize` method would return
`"Another_cache"`, whereas `#classify` will return `"AnotherCache"`.

Below are some benchmarks for the change.

```ruby
require "benchmark"

module EdgeCache
  class Bust
    def by_class
      Fastly
    end

    def by_const_get
      self.class.const_get("fastly".classify)
    end
  end
end

Benchmark.bmbm do |x|
  x.report("by_class") do
    1000.times do
      EdgeCache::Bust.new.by_class
    end
  end
  x.report("by_const_get") do
    1000.times do
      EdgeCache::Bust.new.by_const_get
    end
  end
end
```

```shell
> bin/rails runner /Users/jfriesen/git/forem/bench.rb

Rehearsal ------------------------------------------------
by_class       0.001239   0.000186   0.001425 (  0.001342)
by_const_get   0.011398   0.000136   0.011534 (  0.011538)
--------------------------------------- total: 0.012959sec

                   user     system      total        real
by_class       0.000973   0.000030   0.001003 (  0.000969)
by_const_get   0.011102   0.000032   0.011134 (  0.011104)
```
@jeremyf jeremyf self-assigned this Mar 29, 2022
jeremyf added a commit that referenced this issue Mar 29, 2022
Prior to this commit, when we change attributes for a user we might
resave each article.  The purpose of the resave was to handle some of
the cached attributes of an article.

We need to extend this behavior into organizations.  When an
organization changes it's icon, we want to update each article
associated with the organization.

While this change doesn't do that, it does make it easier for this
update to happen.

This begins to address #17041
jeremyf added a commit that referenced this issue Mar 30, 2022
Prior to this commit, when we change attributes for a user we might
resave each article.  The purpose of the resave is to update some of the
cached attributes of an article.

We need to extend this behavior into organizations.  When an
organization changes it's icon, we want to update each article
associated with the organization.

While this change doesn't do that, it does make it easier for this
update to happen.

This also removes some redundant code.  The following removed code has
an `article.save` call.  Which, the callbacks in the article (see below)
already clear the cache.

```ruby
def resave_articles
  articles.find_each do |article|
    if article.path
      cache_bust = EdgeCache::Bust.new
      cache_bust.call(article.path)
      cache_bust.call("#{article.path}?i=i")
    end
    article.save
  end
end
```

https://github.com/forem/forem/blob/8e6981aac59e86a9a2c69db1e50fb1119f509c8d/app/models/article.rb#L164

https://github.com/forem/forem/blob/8e6981aac59e86a9a2c69db1e50fb1119f509c8d/app/models/article.rb#L881-L888

This begins to address #17041
@jeremyf
Copy link
Contributor Author

jeremyf commented Mar 30, 2022

In the Organization model we have the following:

def update_articles
return unless saved_change_to_slug || saved_change_to_name || saved_change_to_profile_image
articles.update(cached_organization: Articles::CachedEntity.from_object(self))
end
def update_articles_cached_organization
return unless saved_change_to_attribute?(:name)
articles.update(cached_organization: Articles::CachedEntity.from_object(self))
end

Which are called in these two spots:

before_save :update_articles

after_update_commit :update_articles_cached_organization

And as part of "updating" the articles, we call the following:

before_save :update_cached_user

Which has the method body:

forem/app/models/article.rb

Lines 828 to 831 in 0fc1a40

def update_cached_user
self.cached_organization = organization ? Articles::CachedEntity.from_object(organization) : nil
self.cached_user = user ? Articles::CachedEntity.from_object(user) : nil
end

So we're attempting to update an article's cached organization three different times. And it is possible that the Article. update_cached_user is using a cached value for organization.

We definitely don't need to update these things so many times.

jeremyf added a commit that referenced this issue Mar 30, 2022
There are three major things occurring in this pull request:

1. Renaming `Article#update_cached_user` to `Article#set_cached_entities`.
2. Reducing an organization's direct knowledge of which of the org's
   attributes an article caches.
3. Removing duplicate calls to update the article associated with the
   organization.

For renaming to `Article#set_cached_entities`, the prior method implied
we were updating the persistence layer.  However, we were not making any
save nor update calls.  This rename should clarify intention.

For reducing knowledge, the comments for
`Article::ATTRIBUTES_CACHED_FOR_RELATED_ENTITY` should explain the details.

And last, removing the duplicate calls; we had three methods that were
attempting to build and update the `Article#cached_organization`'s
value.

Closes #17041
jeremyf added a commit to forem/forem-docs that referenced this issue Mar 30, 2022
jeremyf added a commit that referenced this issue Apr 1, 2022
There are three major things occurring in this pull request:

1. Renaming `Article#update_cached_user` to `Article#set_cached_entities`.
2. Reducing an organization's direct knowledge of which of the org's
   attributes an article caches.
3. Removing duplicate calls to update the article associated with the
   organization.

For renaming to `Article#set_cached_entities`, the prior method implied
we were updating the persistence layer.  However, we were not making any
save nor update calls.  This rename should clarify intention.

For reducing knowledge, the comments for
`Article::ATTRIBUTES_CACHED_FOR_RELATED_ENTITY` should explain the details.

And last, removing the duplicate calls; we had three methods that were
attempting to build and update the `Article#cached_organization`'s
value.

Closes #17041
jeremyf added a commit that referenced this issue Apr 1, 2022
Prior to this commit, when we change attributes for a user we might
resave each article.  The purpose of the resave is to update some of the
cached attributes of an article.

This removes some redundant code.  The following removed code has
an `article.save` call.  Which, the callbacks in the article (see below)
already clear the cache.

```ruby
def resave_articles
  articles.find_each do |article|
    if article.path
      cache_bust = EdgeCache::Bust.new
      cache_bust.call(article.path)
      cache_bust.call("#{article.path}?i=i")
    end
    article.save
  end
end
```

https://github.com/forem/forem/blob/8e6981aac59e86a9a2c69db1e50fb1119f509c8d/app/models/article.rb#L164

https://github.com/forem/forem/blob/8e6981aac59e86a9a2c69db1e50fb1119f509c8d/app/models/article.rb#L881-L888

This was discovered in triaging #17041
jeremyf added a commit that referenced this issue Apr 1, 2022
Prior to this commit, when we change attributes for a user we might
resave each article.  The purpose of the resave is to update some of the
cached attributes of an article.

This removes some redundant code.  The following removed code has
an `article.save` call.  Which, the callbacks in the article (see below)
already clear the cache.

```ruby
def resave_articles
  articles.find_each do |article|
    if article.path
      cache_bust = EdgeCache::Bust.new
      cache_bust.call(article.path)
      cache_bust.call("#{article.path}?i=i")
    end
    article.save
  end
end
```

https://github.com/forem/forem/blob/8e6981aac59e86a9a2c69db1e50fb1119f509c8d/app/models/article.rb#L164

https://github.com/forem/forem/blob/8e6981aac59e86a9a2c69db1e50fb1119f509c8d/app/models/article.rb#L881-L888

This was discovered in triaging #17041
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: homepage homepage and feed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant