Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Default timestamps to non-null #3334

Merged
merged 1 commit into from Oct 16, 2011

Conversation

Projects
None yet
Contributor

mperham commented Oct 15, 2011

There doesn't appear to be any reason to allow nullable timestamps.

tenderlove added a commit that referenced this pull request Oct 16, 2011

Merge pull request #3334 from mperham/master
Default timestamps to non-null

@tenderlove tenderlove merged commit 8919a68 into rails:master Oct 16, 2011

Contributor

vjebelev commented on 3dbedd2 Oct 17, 2011

I get a fresh test failure under postgres (9.1) that I suspect is caused by this commit:

vlad@mbp:activerecord> rake postgresql:rebuild_databases
vlad@mbp:activerecord> ARCONN=postgresql ruby -Itest test/cases/migration_test.rb
Using postgresql with Identity Map off
Loaded suite test/cases/migration_test
Started
.....................................................................................................E.............................
Finished in 18.715265 seconds.

  1. Error:
    test_native_decimal_insert_manual_vs_automatic(MigrationTest):
    ActiveRecord::StatementInvalid: PGError: ERROR: column "created_at" is of type timestamp without time zone but expression is of type integer
    LINE 1: ..., updated_at) values (12345678901234567890.0123456789, 0, 0)
    ^
    HINT: You will need to rewrite or cast the expression.
    : insert into people (wealth, created_at, updated_at) values (12345678901234567890.0123456789, 0, 0)
    /web/rails/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb:581:in async_exec' /web/rails/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb:581:inblock in execute'
    /web/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:257:in block in log' /web/rails/activesupport/lib/active_support/notifications/instrumenter.rb:20:ininstrument'
    /web/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:252:in log' /web/rails/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb:580:inexecute'
    test/cases/migration_test.rb:478:in test_native_decimal_insert_manual_vs_automatic' /Users/vlad/.rvm/gems/ruby-1.9.2-p180/gems/mocha-0.10.0/lib/mocha/integration/mini_test/version_142_to_172.rb:27:inrun'
    /web/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:35:in block in run' /web/rails/activesupport/lib/active_support/callbacks.rb:408:in_run_setup_callbacks'
    /web/rails/activesupport/lib/active_support/callbacks.rb:81:in run_callbacks' /web/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:34:inrun'

131 tests, 372 assertions, 0 failures, 1 errors, 0 skips

Test run options: --seed 35916

Contributor

yahonda replied Oct 17, 2011

I've got a same error with PostgreSQL 9.1 and Oracle. Then opened a pull request #3349.

Contributor

pcreux replied Jan 23, 2014

@mperham What was the reason for this change? This seems like a sane default to me.

Contributor

pcreux replied Jan 23, 2014

@mperham Sorry - this comment was supposed to go on the REVERT of this commit. 😕

bzanchet commented Mar 1, 2012

I think I'm a bit late here, but still: if you add timestamps when changing a table rather than creating it, NOT NULL fields need a default value. This migration used to work, and now fails because of this change:

class AddTimestampsToMyDearestTable < ActiveRecord::Migration
  def change
    change_table(:some_table) do |t|
      t.timestamps
    end
  end
end

Is that intentional? Is there a workaround?

Owner

fxn commented Mar 1, 2012

I think this PR kind of makes sense because most of the time you want those constraints. But @bzanchet's use case needs to be supported.

The documentation for this method should be updated I think, first to document the constraints, which are missing now (this was missing in the PR, it should have not been accepted without doc updates). Then to explain that you need to pass :null => true in updates.

The migrations guide should probably also reflect this somehow.

Owner

fxn commented Mar 1, 2012

I have not been able to look into this in detail, but from a quick read of the patch I wonder if adding the constraint to add_timestamps shouldn't actually be reverted. In the general case you already have records, and it is not a good assumption that you are going to fill those columns in general, because in general that information is not available.

What do you guys think?

fxn added a commit that referenced this pull request Mar 2, 2012

revert setting NOT NULL constraints in add_timestamps
Commit 3dbedd2 added NOT NULL constraints both to table
creation and modification. For creation the new default
makes sense, but the generic situation for changing a
table is that there exist records. Those records have
no creation or modification timestamps, and in the
general case you don't even know them, so when updating
a table these constraints are not going to work. See
a bug report for this use case in #3334.

fxn added a commit that referenced this pull request Mar 2, 2012

revert setting NOT NULL constraints in add_timestamps
Commit 3dbedd2 added NOT NULL constraints both to table
creation and modification. For creation the new default
makes sense, but the generic situation for changing a
table is that there exist records. Those records have
no creation or modification timestamps, and in the
general case you don't even know them, so when updating
a table these constraints are not going to work. See
a bug report for this use case in #3334.
Contributor

kenn commented Mar 15, 2012

+1 for revert.

Today I set up a new machine and rake db:create then db:migrate generated a different schema.rb than what's committed in our git repo.

That means schema.rb could be inconsistent between our team members, or worse, different from what's already deployed on production.

In other words,

create_table :sometable do |t|
  t.timestamps
end

in migrations could mean different things, depending on when it got run - before or after this change was introduced. This is nondeterministic situation.

I think we need incremental, trackable schema.rb to make this change acceptable.

For now, I'll have to instruct new developers to our team that they should not commit this change in schema.rb and run git reset --hard instead, when they finished setup.

Just an FYI for anyone else. I found that this change can cause issues if you add timestamps to a join table like below:

class AddTagsPostssJoinTable < ActiveRecord::Migration
  def change
    create_table :tags_posts, :id => false do |t|
      t.references :tag
      t.references :post
      t.timestamps
    end
  end
end

The easy solution is to just remove the timestamps from this table, as they don't really add anything.

jarl-dk commented Sep 26, 2012

I experience the same annoyances as @kenn (though I do not agree that it should be reverted). Changing the behaviour of such a method (timestamps), has wider consequences than many other changes. Basically it changes the semantics of all my historical migrations. And changing history is kind of awkward when developing an application incrementally.

To align a fresh db/schema.rb (generated on Rails 3.2.x) with the one that is in production (or with an incrementally evolved development DB) I need to make a migration that introduces (or removes) these not null constraints on all my existing timestamp columns. I think the release notes for 3.2 should emphasize this.

Some people may not care, but one day they will wonder why the production database differs from their test/development, or maybe even differ among team members.

mlanett commented Apr 25, 2013

It also can break historical migrations. My software has migrations which (1) create a table (2) insert data using SQL. This breaks now when spinning up new instances because the data migration didn't insert timestamps, since null was Ok and they were not important.

I disagree with Mike's original claim. Timestamps are a nice-to-have simple free audit mechanism. They are in no way required to be present for most apps to function.

m5rk commented Dec 20, 2013

For what it's worth, an another way to have done this: introduce a new helper (e.g., timestamps_not_null) instead of changing the behavior of the existing helper.

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