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

Extract AP Page and Action caching from Rails #7833

Merged
merged 4 commits into from Oct 4, 2012
Merged

Extract AP Page and Action caching from Rails #7833

merged 4 commits into from Oct 4, 2012

Conversation

frodsan
Copy link
Contributor

@frodsan frodsan commented Oct 3, 2012

This functionality will be available from actionpack-deprecated_caching gem.
The gem is currently hosted at https://github.com/frodsan/actionpack-deprecated_caching.

pd: I'm not sure about the name.

@guilleiguaran
Copy link
Member

:shipit:

2 similar comments
@steveklabnik
Copy link
Member

:shipit:

@sikachu
Copy link
Member

sikachu commented Oct 3, 2012

:shipit:

FileUtils.rm_rf(File.dirname(FILE_STORE_PATH))
super
@store = ActiveSupport::Cache::MemoryStore.new
@controller = FragmentCachingMetalTestController.new
Copy link
Member

Choose a reason for hiding this comment

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

Do we need all this setup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Right

@andrewle
Copy link

andrewle commented Oct 3, 2012

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.

@dhh
Copy link
Member

dhh commented Oct 4, 2012

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.

@rafaelfranca
Copy link
Member

@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
@rafaelfranca rafaelfranca merged commit b0a7068 into rails:master Oct 4, 2012
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants