customink / secondbase

Seamless second database integration for Rails.
MIT License
220 stars 31 forks source link

Schema Cache Support #40

Closed metaskills closed 7 years ago

metaskills commented 7 years ago

For a long time (since Rails 4) ActiveRecord has had a schema cache that assist in complex reflection DB loads. This feature is going to get a little better in Rails 5.1 and change to YAML formatting vs a Marshal dump. Never heard of the schema cache? Read more here:

Rails feature that you've never heard about: schema cache http://blog.iempire.ru/2016/12/13/schema-cache/

First, was pretty excited to see how our on base again won for us by changing the db paths. So this was easy to implement via our standard practice. I choose not to get into monkey patching or alias the ActiveRecord's Railtie initializers. Rather, I added a section to the README under the advanced section on how to create an initializer that would mimic what ActiveRecord is doing. Thoughts?

hmadison commented 7 years ago

This all being said, I really don't know how far we want to go and dig into rails inside the gem. If there is a strong reason for not having us provide the patch in lib i'm all for not putting it in.

metaskills commented 7 years ago

@hmadison I had a little bit of fear and doubt when I was reading the Rails source and saw this in the ActiveRecord active_record.check_schema_cache_dump initializer.

if config.active_record.delete(:use_schema_cache_dump)

We do have a Railtie and gem initializers do support stacking :after others. However, my fear was that code above would actually delete the config and leave us examining a nil. I guess we could side step this and use :before. If it did delete, then the doc's implementation would no-op too.

One other reason for doing in the advanced section which may be moot is that Rails v5.1 is switching to YAML. So if we did add it, we would have to change form Marshal to YAML load and our code would end up with branching conditional logic inside our initializer block. Not a big deal, but this felt like a good first step. I would really like to see this used in an application first and then retool to an initializer later with that groundwork done.

hmadison commented 7 years ago

Ok, thanks for the explanation. Looks good to me.

wingrunr21 commented 7 years ago

Is a manual initializer like that preferable to an initializer defined within the engine that is enabled/disabled via a config option? I ask because that is a bit of a complex initializer and without a generator or having it vendored into the gem itself it could be prone to typos.

metaskills commented 7 years ago

Is a manual initializer like that preferable to an initializer defined within the engine

Not at all. Technically this should be a :before initializer in the Railtie.

metaskills commented 7 years ago

Thanks Stafford. I think two requests to move this to a true initializer is the right way to go. I really appreciate the help y'all! If these tests now pass, I'll merge it in.

metaskills commented 7 years ago

Thanks again, just pushed up v2.1.2