Add metadata to schema_migrations table #8399

Merged
merged 3 commits into from Dec 5, 2012

Projects

None yet

10 participants

@joshsusser
Contributor

This change adds three metadata columns to the schema_migrations table. The name and migrated_at (timestamp) columns will aid in diagnosis of problems related to running migrations and schema changes. The fingerprint column is in preparation for a future change that will detect when migrations are changed after they have already been run.

@joshsusser joshsusser Add metadata to schema_migrations
migrated_at: timestamp when migration run
fingerprint: md5 hash of migration source
name: filename without version or extension
0a5afa2
@rafaelfranca rafaelfranca and 3 others commented on an outdated diff Dec 2, 2012
activerecord/lib/active_record/schema_migration.rb
connection.create_table(table_name, :id => false) do |t|
- t.column :version, :string, :null => false
+ t.column "version", :string, :null => false
@rafaelfranca
rafaelfranca Dec 2, 2012 Member

Why are you not using symbols to the column name?

@joshsusser
joshsusser Dec 2, 2012 Contributor
  1. It's easier to read. 2) Strings are correct - look at how Rails generates db/schema.rb and you'll see.
@rafaelfranca
rafaelfranca Dec 2, 2012 Member

I don't think it's easier to read.

And the `db/schema.rb can use strings, but all the oficial documentation and the migration generator use symbol, so the correct are the symbols.

@tenderlove
tenderlove Dec 2, 2012 Member

This is internal implementation. Does it actually matter?

@rafaelfranca
rafaelfranca Dec 2, 2012 Member

Not really. Just wondering what the reason to change.

Today we change to strings, tomorrow to symbols. Without a good reason this can become a 👯.

@fxn
fxn Dec 3, 2012 Member

The symbol is just fine, there is no reason to change symbols to strings here. The fact that is internal does not justify a change. The change should have a justification by itself.

Josh, it is OK that you prefer strings, but this is a subjective change unrelated to the feature proposal in this PR. And it is a cosmetic change we do not agree with because symbols are idiomatic and strings are not, regardless of your personal preferences which I respect.

On the other hand the feature itself looks good to me.

@joshsusser
joshsusser Dec 3, 2012 Contributor

Francis, the symbol vs string thing is not consistently applied in the Rails framework or code base. There are many cases where symbols are accepted in place of strings. In fact, the docs for the #column method include both symbols and strings for the name in the examples. As I mentioned, the schema.rb file is generated to use strings. But this line here was not an attempt by me to infiltrate or influence the Rails code conventions - I was just making things more readable. And I do think it's more readable to have the name as a string while the types and option names are symbols - it's much easier to scan and see what it is that makes that line special. I've never understood the allure of using a symbol in place of a string just to save a single character, when there is absolutely no use of the symbol as a symbol inside the code. If you look at the implementation of the #column method in schema_definitions.rb, literally the first thing it does is coerce the name (1st arg, which you say should be a symbol) to a string, then the type (2nd arg) to a symbol. That's about as close to type hinting as we get in Ruby, and I am following what that code explicitly says. I have no idea how you get from name.to_s to symbols being idiomatic. Symbols are names of objects in code, not as data. If you are never comparing the symbol to another symbol, it should generally be a string. If someother time you want help making string vs symbol consistent in the framework, I'm happy to take part in that.

@fxn
fxn Dec 3, 2012 Member

@joshsusser I totally respect you think it is more readable for your taste. But we don't agree, and we do not want that cosmetic change in the middle of this PR that we otherwise like as a feature proposal. Could you please submit the patch without that change please?

@joshsusser
joshsusser Dec 3, 2012 Contributor

I do not have time to make any changes now. Take the patch as is and fix the symbol/string thing as you wish, or wait until I have time to update the code.

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Dec 2, 2012
...ord/connection_adapters/abstract/schema_statements.rb
@@ -490,7 +490,7 @@ def dump_schema_information #:nodoc:
sm_table = ActiveRecord::Migrator.schema_migrations_table_name
ActiveRecord::SchemaMigration.order('version').map { |sm|
- "INSERT INTO #{sm_table} (version) VALUES ('#{sm.version}');"
+ "INSERT INTO #{sm_table} (version, migrated_at, name) VALUES ('#{sm.version}',LOCALTIMESTAMP,'#{sm.name}');"
@rafaelfranca
rafaelfranca Dec 2, 2012 Member

Why are you not using the model to create these records?

@joshsusser
joshsusser Dec 2, 2012 Contributor

Because this code is generating the db_structure.sql file, so all it has to work with is SQL.

@rafaelfranca rafaelfranca commented on the diff Dec 2, 2012
activerecord/lib/active_record/schema_migration.rb
@@ -14,17 +14,38 @@ def self.index_name
end
def self.create_table
- unless connection.table_exists?(table_name)
+ if connection.table_exists?(table_name)
@rafaelfranca
rafaelfranca Dec 2, 2012 Member

Since this is for backward compatibility I would extract it to a method if a meaningful name.

@rafaelfranca rafaelfranca commented on the diff Dec 2, 2012
activerecord/lib/active_record/migration.rb
@@ -554,6 +555,10 @@ def basename
delegate :migrate, :announce, :write, :to => :migration
+ def fingerprint
@rafaelfranca
rafaelfranca Dec 2, 2012 Member

We will need to document this method.

@joshsusser
joshsusser Dec 2, 2012 Contributor

No other methods in MigrationProxy are documented. I'd suggest making the class nodoc, as it's an internal implementation detail. Or, since the HTML docs include the source of the implementation, the 1-line implementation is pretty much self-documenting.

@rafaelfranca
rafaelfranca Dec 2, 2012 Member

You are right. Sorry to bother you. The patch is good to go. Thank you so
much for the implementation. I'll merge when get chance.
On Dec 2, 2012 6:58 PM, "Josh Susser" notifications@github.com wrote:

In activerecord/lib/active_record/migration.rb:

@@ -554,6 +555,10 @@ def basename

 delegate :migrate, :announce, :write, :to => :migration
  • def fingerprint

No other methods in MigrationProxy are documented. I'd suggest making the
class nodoc, as it's an internal implementation detail. Or, since the HTML
docs include the source of the implementation, the 1-line implementation is
pretty much self-documenting.


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/8399/files#r2288049.

@p
p Dec 3, 2012

People read "internal" methods to understand how things work, and "there is other code that is equally bad" is not a very good reason to continue writing poor code.

@joshsusser
Contributor

I added another commit that dumps the exact migration history into schema.rb. The main advantage is this avoids possible mistakes made by assuming all migrations with a lower version have been run when loading schema.rb. In addition, it preserves name and fingerprint information for the migrations in the history, but won't break if that info is missing. When loading schema.rb, the migrated_at timestamp for the migration is set to the current time, which indicates when the migration was applied on that db instance, instead of on the dev box where the schema.rb was dumped. The database_structure.sql dump's migration history has been updated to include name and fingerprint info as well.

I put the migrations near the top of the file, but if that's a problem it's easy to move them to the bottom. I did put them in a code block so it's easy to collapse in an editor if there is a long list that is too annoying to scroll over.

Old schema.rb files are still supported and will continue to operate as before. There is no support for generating old-style files.

@joshsusser joshsusser closed this Dec 3, 2012
@joshsusser joshsusser reopened this Dec 3, 2012
@joshsusser

I moved this string into a local before puts-ing it to make it friendlier to certain editor's syntax highlighting.

@ugisozols ugisozols and 1 other commented on an outdated diff Dec 3, 2012
activerecord/test/cases/schema_dumper_test.rb
@@ -1,5 +1,5 @@
require "cases/helper"
-
+# require "cases/migration/helper"
@ugisozols
ugisozols Dec 3, 2012 Contributor

Is this needed?

@joshsusser
joshsusser Dec 3, 2012 Contributor

No. I missed deleting the commented out line before pushing. Didn't think it was worth the extra work to fix after the fact on its own.

@joshsusser
Contributor

That should do it.

@fxn
Member
fxn commented Dec 4, 2012

@joshsusser thanks for the style cleanup.

@tenderlove
Member

Merged in 0c692f4

@tenderlove tenderlove closed this Dec 5, 2012
@tenderlove tenderlove merged commit 94ef7b5 into rails:master Dec 5, 2012
@rubys
Contributor
rubys commented on f02d218 Dec 5, 2012

This causes rake test after rake db:migrate to fail with "Migrations are pending" and "ArgumentError: Can't set migrations while using :version option". See http://intertwingly.net/projects/AWDwR4/checkdepot/section-6.1.html (near the bottom) for details.

Contributor

Looks like I lost a change when packaging this patch. Fixed in PR 8431.

Contributor
rubys replied Dec 8, 2012

Another problem I noticed today (I'm guessing that it is related to this change, but I haven't done a bisect to prove it yet), it looks like this change makes db:rollback less useful. Compare:

http://intertwingly.net/projects/AWDwR4/checkdepot/section-10.1.html
http://intertwingly.net/projects/AWDwR4/checkdepot-32/section-10.1.html

In particular, look for rake db:rollback => Migrations are pending; run 'rake db:migrate RAILS_ENV=development' to resolve this issue.

Member

Great work Josh. This also opens up the possibility of making rake db:migrate:down not need the VERSION parameter. Maybe you fancy working on that :)

Member

heh, @fxn has actually just pointed out to me that we already have db:rollback which does this. please ignore me :)

@yahonda

Got TypeError: can't convert Time to String error with mysql2 and oracle enhanced adapters.
migrated_at column is created with :datetime datatype and :null => false, should not be equal to String.

@yahonda

Got TypeError: can't convert Time to String error with mysql2 and oracle enhanced adapters.
migrated_at column is created with :datetime datatype and :null => false, should not be equal to String.

@jeremy
Member
jeremy commented Dec 8, 2012

Just upgraded an app to latest master. rake test fails straightaway.

Turns out schema_migrations is empty after db:schema:load. The deprecated-version-number support doesn't seem to be working.

To reproduce, pin an app to rails 0c692f4^ and rake db:setup. That'll get you an old-style schema_migrations table and a schema.rb with the current version in it.

Then float rails to 0c692f4 and run rake. It'll raise in test/test_helper on ActiveRecord::Migration.check_pending!

@jeremy
Member
jeremy commented Dec 8, 2012

Running rake db:schema:dump manually got me the new schema_migrations table. But there are no fingerprints or migration names in it, all the migrated_at are the current timestamp, and rake test still blows up in the same spot.

@joshsusser
Contributor

@jeremy: 0c692f4 is incomplete - there were a couple issues resolved in later commits. Check that again after floating to 036d3e1 and see what's up?

@jeremy
Member
jeremy commented Dec 9, 2012

Yep - was trying after that commit. With a version number in schema.rb, the
schema_migrations table comes up empty after a schema load.

On Sat, Dec 8, 2012 at 4:04 PM, Josh Susser notifications@github.comwrote:

@jeremy https://github.com/jeremy: 0c692f40c692f4 incomplete - there were a couple issues resolved in later commits. Check
that again after floating to 036d3e1036d3e1 see what's up?


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/8399#issuecomment-11164932.

@joshsusser
Contributor

@jeremy, I can't reproduce that failure. I've fiddled around with a few scenarios and I can't get it to fail. (The easy way to get a schema.rb with the old version option in the define method is to copy the bottommost version from the migrations block and add it as a param: ActiveRecord::Schema.define(version: 12345) do. That should save some mucking about with git, and doesn't seem to change anything for me.) Anyway, I can't repro the failure. The missing metadata from the schema_migrations table is expected when loading using :versions, but the tests should pass, and that's what I see.

@joshsusser
Contributor

Oh I left out, if you are adding :version to the define method in schema.rb, you should comment out or delete the migrations block, as that is a conflict.

@jeremy
Member
jeremy commented Dec 9, 2012

I'm starting with a schema.rb with a version in it and no migrations block. Then I bundle update rails and run rake. Boom, doesn't work on any app I tried.

The missing metadata from the schema_migrations table is expected when loading using :versions

When using :version, the schema_migrations table still needs to have all the migration versions in it. Otherwise it appears there are pending migrations, resulting in the error I'm seeing.

@jeremy
Member
jeremy commented Dec 9, 2012

I moved this over to a feature branch until we get the kinks worked out: https://github.com/rails/rails/commits/schema-migrations-metadata

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment