Sqlite3 Migration Error Fixed (issue #26087) #26089

travisoneill · 2016-08-09T01:16:22Z

Summary

Fixed error in issue #26087

Other Information

Error seems to be coming from an undefined method error when 'type' = nil on line 306:

type = type.to_sym

~~Changed to the following to handle nil case~~
~~type = type&.to_sym || :placeholder~~

Now handles nil by passing it through without type error rather than assigning a placeholder on suggestion of @rafaelfranca:

type = type.to_sym if type

Sqlite3 test suite runs with 0 errors and 3 skips (same as when run on unmodified code)

rails-bot · 2016-08-09T01:16:24Z

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @matthewd (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

maclover7 · 2016-08-09T01:24:42Z

Can we add a regression test here, to ensure that the bug is now fixed? 😬

travisoneill · 2016-08-09T01:35:04Z

How do I do a regression test? Is it a built in test like the Sqlite test suite that I ran from the contribution guide?

maclover7 · 2016-08-09T02:04:50Z

You can add a regression test by creating another test case -- I'd advise you stick around this area, where it seems like there are other tests that deal with migration errors.

Also, when get a chance, can you take a look at the Travis CI errors? Thanks!

travisoneill · 2016-08-09T03:04:43Z

Looks like the Travis CI is failing because I used the safe navigation operator from 2.3.0 Will change to work on older versions.

Edit: pushed change

travisoneill · 2016-08-09T17:37:09Z

~~Is there a rake task that will run these tests? Here's what I wrote, not sure about the syntax, though so would like to test out before pushing~~ Running. Here's the code:

   #/activerecord/test/cases/migration_test.rb:312
    def test_allows_sqlite3_rollback_on_invalid_column_type
      if current_adapter?(:SQLite3Adapter)
        Person.connection.create_table :something, force: true do |t|
          t.column :number, :integer
          t.column :name, :string
          t.column :foo, :bar
        end
        assert Person.connection.column_exists?(:something, :foo)
        assert_nothing_raised { Person.connection.remove_column :something, :foo, :bar }
        assert !Person.connection.column_exists?(:something, :foo)
        assert Person.connection.column_exists?(:something, :name)
        assert Person.connection.column_exists?(:something, :number)
      end
    end

travisoneill · 2016-08-09T19:39:00Z

Noticed a very minor issue wile running the tests. Doesn't break, just yells at you: rails/activerecord/test/cases/scoping/relation_scoping_test.rb:249: warning: ambiguous first argument; put parentheses or a space even after `/' operator

assert_match /LIMIT 10|ROWNUM <= 10|FETCH FIRST 10 ROWS ONLY/, sql

I got rid of this message by simply putting parens around the arguments:

assert_match(/LIMIT 10|ROWNUM <= 10|FETCH FIRST 10 ROWS ONLY/, sql)

Would you like me to add this change to the pull request?

alexcameron89 · 2016-08-09T21:00:22Z

@travisoneill You accidentally pushed your .byebug_history

travisoneill · 2016-08-09T22:09:17Z

@alexcameron89 Thanks for catching that. Fixed

travisoneill · 2016-08-10T18:07:55Z

@matthewd @maclover7
Do you guys need anything anything else done here?

maclover7 · 2016-08-11T01:33:27Z

Can you please squash down your five commits to one? Thanks!

travisoneill · 2016-08-11T04:28:05Z

@maclover7 Squashed commits but now failing Travis CI. Coming from tests in '/actionview' Diffs are exactly the same as before I squashed and when I ran tests locally it fails in 'master' too, so maybe unrelated? Do you have any idea what's going on or how to fix it?

travisoneill · 2016-08-11T17:32:40Z

@maclover7 Merged with master to get the Travis CI to pass. Do I need to squash these merge commits too?

maclover7 · 2016-08-11T17:36:19Z

If possible, that'd be great :)

travisoneill · 2016-08-11T18:15:25Z

@maclover7 When I squashed this time it drags in changes that other people made and failing codeclimate because of those changes. Is that OK? (sorry for all the questions, it's my first time contributing to a large project).

maclover7 · 2016-08-12T01:30:37Z

Up to you what you'd like to do. I'd encourage fixing them :)

travisoneill · 2016-08-12T22:15:25Z

@maclover7 Fixed, squashed, and passing! Learned some cool Git stuff there too! Let me know if there is anything else that needs doing.

maclover7 · 2016-08-12T22:25:12Z

Thank you for all of your efforts @travisoneill! Now we will have to wait until someone higher up the chain can come and review your PR.

rafaelfranca · 2016-08-12T23:05:38Z

activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb

@@ -303,7 +303,7 @@ def [](name)
      #   end
      def column(name, type, options = {})
        name = name.to_s
-        type = type.to_sym
+        type = type ? type.to_sym : :__placeholder


if type information is not there should not we just not call to_sym on it and pass nil down in the stream?

@rafaelfranca It calls type.to_sym one more time before finishing so I would have to go in and also handle the nil case there to fix. Would that be preferable to this solution? Can fix it quickly if so.

where it does call it? Yeah, I think it is better to just ignore nil than adding a placeholder.

@rafaelfranca /activerecord/lib/active_record/connection_adapters/abstract/schema_statements
have a working fix now. Will push up.

rafaelfranca · 2016-08-16T08:46:52Z

activerecord/test/cases/migration_test.rb

@@ -309,6 +309,21 @@ def migrate(x)
        "On error, the Migrator should revert schema changes but it did not."
    end

+    def test_allows_sqlite3_rollback_on_invalid_column_type
+      if current_adapter?(:SQLite3Adapter)


Let's put this if outside the test

invalid column type /activerecord/lib/active_record/connection_adapters /abstract/schema_definitions.rb:306 type = type.to_sym Changed to the following to handle nil case: type = type.to_sym if type Added regression test for this case: /activerecord/test/cases/migration_test.rb:554 if current_adapter?(:SQLite3Adapter) def test_allows_sqlite3_rollback_on_invalid_column_type Person.connection.create_table :something, force: true do |t| t.column :number, :integer t.column :name, :string t.column :foo, :bar end assert Person.connection.column_exists?(:something, :foo) assert_nothing_raised { Person.connection.remove_column :something, :foo, :bar } assert !Person.connection.column_exists?(:something, :foo) assert Person.connection.column_exists?(:something, :name) assert Person.connection.column_exists?(:something, :number) ensure Person.connection.drop_table :something, if_exists: true end end

travisoneill · 2016-08-19T01:02:00Z

@rafaelfranca Made your changes. Test now in new if block at activerecord/test/cases/migration_test.rb:554:

  if current_adapter?(:SQLite3Adapter)
    def test_allows_sqlite3_rollback_on_invalid_column_type
      Person.connection.create_table :something, force: true do |t|
        t.column :number, :integer
        t.column :name, :string
        t.column :foo, :bar
      end
      assert Person.connection.column_exists?(:something, :foo)
      assert_nothing_raised { Person.connection.remove_column :something, :foo, :bar }
      assert !Person.connection.column_exists?(:something, :foo)
      assert Person.connection.column_exists?(:something, :name)
      assert Person.connection.column_exists?(:something, :number)
    ensure
      Person.connection.drop_table :something, if_exists: true
    end
  end

travisoneill · 2016-08-19T03:16:25Z

@rafaelfranca How do I deal with this conflict with base branch? Do I need to rebase and push again?

rafaelfranca · 2016-08-19T03:21:17Z

I can fix it myself.

Sqlite3 Migration Error Fixed (issue #26087)

rafaelfranca · 2016-08-19T03:28:51Z

Backported in 7938aab

rails-bot assigned matthewd Aug 9, 2016

travisoneill mentioned this pull request Aug 9, 2016

Sqlite3 Migration & Rollback Error Handling With Invalid Column Type #26087

Closed

maclover7 added the activerecord label Aug 9, 2016

travisoneill force-pushed the sqlite_rollback_fix branch from 9e9bebf to 7dc082f Compare August 11, 2016 03:39

travisoneill force-pushed the sqlite_rollback_fix branch 3 times, most recently from 1e630dc to 3158a65 Compare August 11, 2016 18:09

travisoneill force-pushed the sqlite_rollback_fix branch 2 times, most recently from 53519cc to 90e4e1c Compare August 12, 2016 21:16

rafaelfranca reviewed Aug 12, 2016
View reviewed changes

travisoneill force-pushed the sqlite_rollback_fix branch 2 times, most recently from d6498d0 to abb61a3 Compare August 14, 2016 08:24

rafaelfranca reviewed Aug 16, 2016
View reviewed changes

travisoneill force-pushed the sqlite_rollback_fix branch 3 times, most recently from f178ace to 1e6610f Compare August 17, 2016 23:12

travisoneill force-pushed the sqlite_rollback_fix branch from 1e6610f to 906ff07 Compare August 18, 2016 00:22

rafaelfranca added a commit that referenced this pull request Aug 19, 2016

Merge pull request #26089 from travisoneill/sqlite_rollback_fix

7938aab

Sqlite3 Migration Error Fixed (issue #26087)

rafaelfranca merged commit 906ff07 into rails:master Aug 19, 2016

rafaelfranca added a commit that referenced this pull request Aug 19, 2016

Merge pull request #26089 from travisoneill/sqlite_rollback_fix

eabf024

Sqlite3 Migration Error Fixed (issue #26087)

prathamesh-sonpatki mentioned this pull request Sep 4, 2016

rake migration accepts none existed datatype and 'string' column name. #26385

Closed

travisoneill deleted the sqlite_rollback_fix branch October 11, 2016 04:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sqlite3 Migration Error Fixed (issue #26087) #26089

Sqlite3 Migration Error Fixed (issue #26087) #26089

travisoneill commented Aug 9, 2016 •

edited

rails-bot commented Aug 9, 2016

maclover7 commented Aug 9, 2016

travisoneill commented Aug 9, 2016

maclover7 commented Aug 9, 2016

travisoneill commented Aug 9, 2016 •

edited

travisoneill commented Aug 9, 2016 •

edited

travisoneill commented Aug 9, 2016

alexcameron89 commented Aug 9, 2016

travisoneill commented Aug 9, 2016

travisoneill commented Aug 10, 2016

maclover7 commented Aug 11, 2016 •

edited

travisoneill commented Aug 11, 2016

travisoneill commented Aug 11, 2016

maclover7 commented Aug 11, 2016

travisoneill commented Aug 11, 2016 •

edited

maclover7 commented Aug 12, 2016

travisoneill commented Aug 12, 2016

maclover7 commented Aug 12, 2016

rafaelfranca Aug 12, 2016

travisoneill Aug 12, 2016

rafaelfranca Aug 12, 2016

travisoneill Aug 13, 2016

rafaelfranca Aug 16, 2016

travisoneill commented Aug 19, 2016

travisoneill commented Aug 19, 2016

rafaelfranca commented Aug 19, 2016

rafaelfranca commented Aug 19, 2016

Sqlite3 Migration Error Fixed (issue #26087) #26089

Sqlite3 Migration Error Fixed (issue #26087) #26089

Conversation

travisoneill commented Aug 9, 2016 • edited

Summary

Other Information

rails-bot commented Aug 9, 2016

maclover7 commented Aug 9, 2016

travisoneill commented Aug 9, 2016

maclover7 commented Aug 9, 2016

travisoneill commented Aug 9, 2016 • edited

travisoneill commented Aug 9, 2016 • edited

travisoneill commented Aug 9, 2016

alexcameron89 commented Aug 9, 2016

travisoneill commented Aug 9, 2016

travisoneill commented Aug 10, 2016

maclover7 commented Aug 11, 2016 • edited

travisoneill commented Aug 11, 2016

travisoneill commented Aug 11, 2016

maclover7 commented Aug 11, 2016

travisoneill commented Aug 11, 2016 • edited

maclover7 commented Aug 12, 2016

travisoneill commented Aug 12, 2016

maclover7 commented Aug 12, 2016

rafaelfranca Aug 12, 2016

Choose a reason for hiding this comment

travisoneill Aug 12, 2016

Choose a reason for hiding this comment

rafaelfranca Aug 12, 2016

Choose a reason for hiding this comment

travisoneill Aug 13, 2016

Choose a reason for hiding this comment

rafaelfranca Aug 16, 2016

Choose a reason for hiding this comment

travisoneill commented Aug 19, 2016

travisoneill commented Aug 19, 2016

rafaelfranca commented Aug 19, 2016

rafaelfranca commented Aug 19, 2016

travisoneill commented Aug 9, 2016 •

edited

travisoneill commented Aug 9, 2016 •

edited

travisoneill commented Aug 9, 2016 •

edited

maclover7 commented Aug 11, 2016 •

edited

travisoneill commented Aug 11, 2016 •

edited