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

Allow schema cache path to be defined in the database configuration file #38283

Merged
merged 1 commit into from Jan 23, 2020
Merged

Allow schema cache path to be defined in the database configuration file #38283

merged 1 commit into from Jan 23, 2020

Conversation

kytrinyx
Copy link
Contributor

@kytrinyx kytrinyx commented Jan 22, 2020

Summary

This updates the database tasks for dumping the Active Record schema cache as well as clearing the schema cache file, allowing the path to be defined in the database configuration YAML file.

As before, the value can also be defined in an ENV variable, though this would not work for a multi-db application. If the value is specified neither in the DB config, nor in the ENV, then the path will continue to be derived from the DB config spec_name.

Other Information

In order to make the schema cache database tasks more uniform, I added a clear_schema_cache method to the database tasks, in symmetry with the dump_schema_cache method. Then I pushed the filename logic into those methods, making it easier to test in a way that is understandable. Update: had to back that out, as it was a breaking API change

The schema cache dumping task was only very lightly tested, so I fleshed those tests out a bit, and the clearing task was not tested, so I added tests for those.

I didn't find any in-depth documentation for the schema cache or the database configuration in the Rails guides, so I only added a bit of method documentation on the new schema_cache_path accessor on the database config. If you can think of any other documentation that needs updating, please let me know!

@eileencodes eileencodes self-assigned this Jan 22, 2020
@eileencodes eileencodes added this to the 6.1.0 milestone Jan 22, 2020
Copy link
Member

@tenderlove tenderlove left a comment

Choose a reason for hiding this comment

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

I like that this allows us to specify the schema file in the db config. I think this is a güd change. (@kytrinyx didn't mention it, but we already do this at GitHub, so this is kind of upstreaming our changes.)

Copy link
Member

@rafaelfranca rafaelfranca left a comment

Choose a reason for hiding this comment

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

Nice work! We were going to apply a similar change as well. I made two comments about the CHANGELOG.

activerecord/lib/active_record/tasks/database_tasks.rb Outdated Show resolved Hide resolved
activerecord/CHANGELOG.md Show resolved Hide resolved
Copy link
Member

@eileencodes eileencodes left a comment

Choose a reason for hiding this comment

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

This looks great, can you squash your commits into one?

Copy link
Member

@eileencodes eileencodes left a comment

Choose a reason for hiding this comment

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

Oops I missed something when I reviewed before

Copy link
Member

@seejohnrun seejohnrun left a comment

Choose a reason for hiding this comment

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

This PR looks good! I left a few comments around consistency with other HashConfig / DatabaseConfig method 🎉

activerecord/lib/active_record/tasks/database_tasks.rb Outdated Show resolved Hide resolved
activerecord/lib/active_record/tasks/database_tasks.rb Outdated Show resolved Hide resolved
@kytrinyx
Copy link
Contributor Author

I've reworked this based on the discussions so far:

  1. This reverts back to the original API of the dump_schema_cache method
  2. It changes the API of the schema_dump_filename but it is backwards compatible
  3. I've added the method with a raise NotImplementedError in the DatabaseConfig class
  4. I decided not to change the parameter to schema_dump_filename to spec_name but rather keep the original, since we're no longer in a context of a DatabaseConfig object.
  5. I tried moving the default name implementation into the HashConfig#schema_cache_path, but realized that it was too gross for the db config class to know about the rails top-level config, so I have left it in the DatabaseTasks.

Let me know if there are any other changes you can think of, and if not I'll squash all the commits.

This updates the database tasks for dumping the Active Record schema cache as
well as clearing the schema cache file, allowing the path to be defined in the
database configuration YAML file.

As before, the value can also be defined in an ENV variable, though this would
not work for a multi-db application. If the value is specified neither in the
DB config, nor in the ENV, then the path will continue to be derived from the
DB config spec_name.

Note that in order to make this change cleaner I also moved a bit of logic
out of a rake task and into the DatabaseTasks class, for symmetry.

We have two rake tasks for the schema cache:

    $ rake db:schema:cache:dump
    $ rake db:schema:cache:clear

The cache:dump task was implemented in DatabaseTasks, but the
cache:clear one was not.

I also added some tests for the behavior that I was changing, since some of
the code paths weren't tested.
@eileencodes eileencodes merged commit 0ec8195 into rails:master Jan 23, 2020
@kytrinyx kytrinyx deleted the schema-cache-path branch January 23, 2020 16:09
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

5 participants