Extract AP Page and Action caching from Rails #7833
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
| FileUtils.rm_rf(File.dirname(FILE_STORE_PATH)) | ||
| super | ||
| @store = ActiveSupport::Cache::MemoryStore.new | ||
| @controller = FragmentCachingMetalTestController.new |
There was a problem hiding this comment.
I copied from this line https://github.com/frodsan/rails/blob/2f81be178fd000956974d53ad265fffa58b50090/actionpack/test/controller/caching_test.rb#L48. I will look into it.
|
I'd like to understand the motivations behind moving this out of rails core. I had a brief exchange with @steveklabnik and understand that maybe developers should just be using Rack::Cache/Varnish + proper HTTP caching headers, but action caching also allows you hit your middleware and filter stack, which is useful for authentication. I'm all for moving page caching out though, as it bypasses the Rails stack altogether. |
|
Action caching relies on old-school manual expiration. No bueno. Much better to control your cache using key-based view and possibly controller based caches. You're still free to use this through the plugin, but it's not a good fit for core any more. The state of the art moved on. |
|
@frodsan lets move this forward. Please add a CHANGELOG entry and update the upgrading guide talking about the removal and pointing to the plugin. |
rafaelfranca
added a commit
that referenced
this pull request
Oct 4, 2012
Extract AP Page and Action caching from Rails
sgerrand
pushed a commit
to sgerrand/rails
that referenced
this pull request
Nov 2, 2013
According to the rationale at rails#7833 (comment), we should recommend new users to follow DHH's approach outlined at http://37signals.com/svn/posts/3113-how-key-based-cache-expiration-works. This is the first step, and perhaps in the future we can write some specific recommendations out.
yykamei
added a commit
to yykamei/rails
that referenced
this pull request
Oct 3, 2021
Instrumentation hooks for `write_page.action_controller` and `expire_page.action_controller` seem to be removed in rails#7833. So, subscribers for them are no longer necessary.
DerekCrosson
pushed a commit
to DerekCrosson/rails
that referenced
this pull request
Oct 8, 2021
…s url or fallback refactor: add port number refactor: add default fallback if default options has no host test(rails console): session host is correct refactor: set default url in correct file refactor: remove code that is not needed Remove unused instrumentation hooks from action_controller Instrumentation hooks for `write_page.action_controller` and `expire_page.action_controller` seem to be removed in rails#7833. So, subscribers for them are no longer necessary. Fix new method name in DatabaseConfig#config deprecation message Replace "ActionText" with "Action Text" [ci skip] http://guides.rubyonrails.org/api_documentation_guidelines.html#wording Remove unnecessary if in ExtendedDeterministicUniquenessValidator Specify ORDER BY enumsortorder for postgres enums Co-authored-by: Daniel Colson <danieljamescolson@gmail.com> Restore set_autoload_path triggering before bootstrap Fixes rails#43205. Automatically infer inverse_of with scopes Background --- I recently noticed we had a number of associations in GitHub that would benefit from having `inverse_of` set, and so I began adding them. I ended up adding them to virtually every association with a scope, at which point I wondered whether Rails might be able to automatically find these inverses for us. For GitHub, the changes in this commit end up automatically adding `inverse_of` to 171 of associations that were missing it. My understanding is that we do not automatically detect `inverse_of` for associations with scopes because the scopes could exclude the records we are trying to inverse from. But I think that should only matter if there is a scope on the inverse side, not on the association itself. For example: Scope on has_many ---- ```rb class Post < ActiveRecord::Base has_many :comments, -> { visible } end class Comment < ActiveRecord::Base belongs_to :post scope :visible, -> { where(visible: true) } scope :hidden, -> { where(visible: false) } end post = Post.create! comment = post.comments.hidden.create! assert comment.post ``` This code leaves `post.comments` in sort of a weird state, since it includes a comment that the association would filter out. But that's true regardless of the changes in this commit. Regardless of whether the comments association has an inverse, `comment.post` will return the post. The difference is that when `inverse_of` is set we use the existing post we have in memory, rather than loading it again. If there is a downside to having the `inverse_of` automatically set here I'm not seeing it. Scope on belongs_to ---- ```rb class Post < ActiveRecord::Base has_many :comments scope :visible, -> { where(visible: true) } scope :hidden, -> { where(visible: false) } end class Comment < ActiveRecord::Base belongs_to :post, -> { visible } end post = Post.hidden.create! comment = post.comments.create! assert_nil comment.post ``` This example is a different story. We don't want to automatically infer the inverse here because that would change the behavior of `comment.post`. It should return `nil`, since it's scoped to visible posts while this one is hidden. This behavior was not well tested, so this commit adds a test to ensure we haven't changed it. Changes --- This commit changes `can_find_inverse_of_automatically` to allow us to automatically detect `inverse_of` when there is a scope on the association, but not when there is a scope on the potential inverse association. (`can_find_inverse_of_automatically` gets called first with the association's reflection, then if it returns true we attempt to find the inverse reflection, and finally we call the method again with the inverse reflection to ensure we can really use it.) Since this is a breaking change—specifically in places where code may have relied on a missing `inverse_of` causing fresh copies of a record to be loaded—we've placed it behind the `automatic_scope_inversing` flag (whose name was inspired by `has_many_inversing`). It is set to true for new applications via framework defaults. Testing --- In addition to the inverse association tests, this commit also adds some cases to a few tests related to preloading. They are basically duplicates of existing tests, but with lower query counts. Testing this change with GitHub, the bulk of the failing tests were related to lower query counts. There were additionally 3 places (2 in tests and one in application code) where we relied on missing `inverse_of` causing fresh copies of a record to be loaded. There's still one Rails test that wouldn't pass if we ran the whole suite with `automatic_scope_inversing = true`. It's related to `TaggedPost`, which changes the polymorphic type from the base class `Post` to the subclass `TaggedPost`. ```rb class TaggedPost < Post has_many :taggings, -> { rewhere(taggable_type: "TaggedPost") }, as: :taggable end ``` Setting the inverse doesn't work because it ends up changing the type back to `Post`, something like this: ```rb post = TaggedPost.new tagging = post.taggings.new puts tagging.taggable_type => TaggedPost tagging.taggable = post puts tagging.taggable_type => Post ``` I think this is an acceptable change, given that it's a fairly specific scenario, and is sort of at odds with the way polymorphic associations are meant to work (they are meant to contain the base class, not the subclass). If someone is relying on this specific behavior they can still either keep `automatic_scope_inversing` set to false, or they can add `inverse_of: false` to the association. I haven't found any other cases where having the `inverse_of` would cause problems like this. Co-authored-by: Chris Bloom <chrisbloom7@gmail.com> Remove message from ActiveRecord::Rollback example Do not suggest that raising `ActiveRecord::Rollback` exception should have a message. There's no point of adding a message to `ActiveRecord::Rollback` exception because it will be rescued by transaction block and the message will be lost. Use queue_classic branch which works on psql 14 This fixes the failing CI for ActiveJob. This uses my fork, we can switch back when the following PR is merged: QueueClassic/queue_classic#334 Add ability to lazily load the schema cache on connection This adds a configuration option and code to enable lazy loading of the schema cache on the connection. If `config.active_record.lazily_load_schema_cache` is set to `true` then the Railtie will be completely skipped and the schema cache will be loaded when the connection is established rather than on boot. We use this method at GitHub currently in our custom adapter. It enables us to load schema caches lazily. It also is a solution for schema caches with multiple databases. The Railtie doesn't know about the other connections _before_ boot so it can only load the cache for `ActiveRecord::Base.connection`. Since this loads the cache on `establish_connection` Rails doesn't need to know anything special about the connections. Applications can continue dumping the cache like normal, the change is only to how the schema cache is loaded. To use the lazy loaded schema cache a `schema_cache_path` must be set in the database configuration, otherwise `db/schema_cache.yml` will be used. Followup questions: 1) Should we deprecate the Railtie? 2) The Railtie does more work than we're doing here because it checks the version against the current version. I'm not sure we really want to do this in Rails - we don't do it in ours at GitHub. Replace ableist language The word "Crazy" has long been associated with mental illness. While there may be other dictionary definitions, it's difficult for some of us to separate the word from the stigmatization, gaslighting, and bullying that often comes along with it. This commit replaces instances of the word with various alternatives. I find most of these more focused and descriptive than what we had before. Remove "matches?" from AS::N subscriber classes This was not being used anymore Move AllMessages behaviour into Matcher This avoids needing to delegate all methods to the actual subscriber class and we can deal just with the message name matching behaviour. Don't require role when passing shard to connected_to Originally we required `role` when switching `shard`'s because I felt it made it less confusing. However now that we're exploring some more multi-tenancy work I agree we need to move this condition. Otherwise if you're using one middleware to switch roles and another to switch shards, you may be forced to pass the role around in places you're only concerned with shard. This also simplifies the call for applications that don't use roles and only have writer shards. chore: squash commits refactor: include url helpers Replace more ableist language Along the same lines as ccb3cb5, this commit removes unnecessary references to mental health. As in that commit, I think many of these are more descriptive than what we had before. The commit changes only tests and documentation. Remove refenrence to destroy_all_in_batches config This feature was reverted. refactor: fix test fix(rails console): change session host to application default url options host
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This functionality will be available from
actionpack-deprecated_cachinggem.The gem is currently hosted at https://github.com/frodsan/actionpack-deprecated_caching.