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

Add (back) rails-level JSON caching #11333

Merged
merged 1 commit into from
Jul 21, 2019
Merged

Add (back) rails-level JSON caching #11333

merged 1 commit into from
Jul 21, 2019

Conversation

Gargron
Copy link
Member

@Gargron Gargron commented Jul 17, 2019

This is my idea for #11330 with prettier (and less) code, replacing render_cached_json with render_with_cache.

The :key, unless supplied explicitly, is generated from the controller name, controller action, the cache key of the object being serialized, as well as fields chosen. For example, it could look like this:

accounts/show:accounts/1:id,type,preferred_username,public_key,endpoints

When no fields are passed (such as when all fields should be rendered), it's a lot shorter:

accounts/show:accounts/1

When there's no record, the key is even simpler:

api/v1/instances/show

The method supports being passed :json a record or hash, a method name as a symbol, or a block. The latter two will only get evaluated if there is no cache. Example:

def show
  render_with_cache json: :foo
end

private

def foo
  # Heavy computing here
end

This means invocations of render_with_cache look almost identical to just using render and you don't need to think about manually specifying cache keys.

@Gargron Gargron force-pushed the fix-render-caching branch 2 times, most recently from e7a7124 to dab1611 Compare July 17, 2019 04:45
@Gargron Gargron requested a review from ClearlyClaire July 17, 2019 04:50
@Gargron Gargron force-pushed the fix-render-caching branch 2 times, most recently from 2682add to 237d214 Compare July 21, 2019 02:23
@Gargron Gargron marked this pull request as ready for review July 21, 2019 02:24
@Gargron Gargron force-pushed the fix-render-caching branch from 237d214 to d62c710 Compare July 21, 2019 02:27
@Gargron Gargron requested a review from ClearlyClaire July 21, 2019 02:27
def render_with_cache(**options)
raise ArgumentError, 'only JSON render calls are supported' unless options.key?(:json) || block_given?

key = options.delete(:key) || [[params[:controller], params[:action]].join('/'), options[:json].respond_to?(:cache_key) ? options[:json].cache_key : nil, options[:fields].nil? ? nil : options[:fields].join(',')].compact.join(':')
Copy link
Contributor

Choose a reason for hiding this comment

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

The cache key being dependent on the controller's action would mean that StatusesController#show and StatusesController#activity would have different cache keys, which isn't optimal. Not sure that would really be a problem in practice, though. What is StatusesController#activity used for anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be the correct behavior because those actions return different json. Activity is used for the resolveable id of activities.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do they really return different json?
Hm, also… that's not directly related, but #activity doesn't seem to require signatures even if authorized_fetch_mode? is true

Copy link
Member Author

Choose a reason for hiding this comment

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

The show returns a Note object, the activity returns a Create or Announce object.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, right! Sorry about that.

Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

I'm not super comfortable with how it moves the cache key logic in one place which tries to figure out a lot of cases by itself, but that's just my opinion. Otherwise it looks fine.

@Gargron Gargron merged commit c669bb4 into master Jul 21, 2019
@Gargron Gargron deleted the fix-render-caching branch July 21, 2019 20:32
hiyuki2578 pushed a commit to ProjectMyosotis/mastodon that referenced this pull request Oct 2, 2019
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.

2 participants