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

Fix migration compatibility to create SQLite references/belongs_to column as a integer when migration version is 6.0 #43295

Merged
merged 1 commit into from Dec 21, 2021

Conversation

marcelolx
Copy link
Contributor

Since #38717 the ReferenceDefinition wasn't being instantiated with the column type as :integer in the compatibility module when the adapter is SQLite for 6.0 migrations which make such columns be created as bigint instead of integer. Differing from the way they are created in prior versions or newer versions, which is always integer.

Fixes #43168

…lumn as a integer when migration version is 6.0

Since rails#38717 the `ReferenceDefinition` wasn't being
instantiated with the column type as `:integer` in
the compatibility module when the adapter is SQLite
for 6.0 migrations which make such columns be created
as `bigint` instead of `integer`. Differing from
the way they are created in prior versions or newer
versions, which is always `integer`.

Fixes rails#43168
@rafaelfranca rafaelfranca merged commit 2ea5268 into rails:main Dec 21, 2021
rafaelfranca added a commit that referenced this pull request Dec 21, 2021
rafaelfranca added a commit that referenced this pull request Dec 21, 2021
@marcelolx marcelolx deleted the issue-43168 branch December 21, 2021 19:08
@pixeltrix
Copy link
Contributor

Getting some failures locally after this change:

Failure:
ActiveRecord::Migration::CompatibilityTest#test_references_stays_as_integer_column_on_add_reference_6_0 [/Users/andyw/Repositories/rails/rails/activerecord/test/cases/migration/compatibility_test.rb:560]:
Expected: "integer"
  Actual: "INTEGER"

and

Failure:
ActiveRecord::Migration::CompatibilityTest#test_references_stays_as_integer_column_on_create_table_with_reference_6_0 [/Users/andyw/Repositories/rails/rails/activerecord/test/cases/migration/compatibility_test.rb:535]:
Expected: "integer"
  Actual: "INTEGER"

After a bit of digging I think it might be related to the STRICT tables option in sqlite3 3.37.0 and later - it only allows the following for types:

INT
INTEGER
REAL
TEXT
BLOB
ANY

Not sure what the correct fix is - not seeing any other failures on 3.37.1 which is what I'm running on my Mac. Maybe just a case-insensitive regex match?

@marcelolx
Copy link
Contributor Author

marcelolx commented Jan 8, 2022

@pixeltrix Oh that's intriguing, but probably yeah, a case-insensitive regex match should fix it. I'll take a look into it better tomorrow, but I think no matter which is the problem it would make sense to case insensitive match it , it needs to be integer, no matter if it is INTEGER or integer (I believe it is the same thing, or at least I would expect it to be)

@marcelolx
Copy link
Contributor Author

I did install sqlite3 3.37.2 here and I can see the same errors, so I think yes the best option is to do a case insensitive match.

Opened a PR to fix it, let me know if that works #44135

marcelolx added a commit to marcelolx/rails that referenced this pull request Jan 11, 2022
As mentioned here rails#43295 since sqlite3 3.37 sqlite3 returns the type of the columns in capital letters while in versions before it did return in lowercase, matching disregarging case sensitiveness will avoid the tests fail in more recent versions of sqlite3 since what matters is the name of the type and not if it is uppercase or lowercase.

Co-Authored-By: Name <pixeltrix@users.noreply.github.com>
@nasmorn
Copy link

nasmorn commented Apr 25, 2022

This PR actually causes my PostgreSQL migration to run as integer instead of big int. It only happens on 6.1.5 when this was introduced. I removed it by monkey patching the class and only reversed the order of these prepend statements.

             prepend TableDefinition
             prepend SQLite3::TableDefinition

What would be the best way to do something here? Oddly enough this patch is not on 6.1-stable so idk how to proceed.

@marcelolx
Copy link
Contributor Author

marcelolx commented Apr 25, 2022

@nasmorn The PR linked to this issue here #44268 does solve your problem?

And I think the patch is in 6.1-stable, at least this commit here df47587 says that it is

@nasmorn
Copy link

nasmorn commented Apr 25, 2022

@marcelolx thank you for the quick reply. Yes that actually solves it and also explains why the code is no longer on stable, since it has already been patched.

@marcelolx
Copy link
Contributor Author

@nasmorn yes, sorry for causing this problem for you'll

@nasmorn
Copy link

nasmorn commented Apr 25, 2022

No problem. I actually feel it is a case of the system working. The only weird thing is that I couldn't find anything in Google for

Rails 6.0 migrations using integer instead of bigint under 6.1

Which I hope this comment remedies

@marcelolx
Copy link
Contributor Author

@nasmorn The problem was that with some changes for SQLite the migration when version 6.0 was changing the column type from integer to bigint (SQLite default is Integer while MySQL/Postgres is bigint), this PR here did aim to not change the type of the column when SQLite but did end affecting MySQL/Postgres while the intent was not it.

I think you haven't found anything on google because most people do upgrade to the latest version of 6.1 which did already include the fix for what I did broke on this PR

@arturopie
Copy link
Contributor

@nasmorn The problem was that with some changes for SQLite the migration when version 6.0 was changing the column type from integer to bigint (SQLite default is Integer while MySQL/Postgres is bigint), this PR here did aim to not change the type of the column when SQLite but did end affecting MySQL/Postgres while the intent was not it.

I think you haven't found anything on google because most people do upgrade to the latest version of 6.1 which did already include the fix for what I did broke on this PR

We just upgraded to 6.1.5.1, the latest 6.1 version right now, and I don't think the fix you are referring to is in 6.1.5.1.

@marcelolx
Copy link
Contributor Author

@arturopie Well, the fix f617ec1 was merged into 6-1-stable, only if that fix doesn't fix your problem.

image

p8 pushed a commit to p8/rails that referenced this pull request Sep 29, 2022
As mentioned here rails#43295 since sqlite3 3.37 sqlite3 returns the type of the columns in capital letters while in versions before it did return in lowercase, matching disregarging case sensitiveness will avoid the tests fail in more recent versions of sqlite3 since what matters is the name of the type and not if it is uppercase or lowercase.

Co-Authored-By: Name <pixeltrix@users.noreply.github.com>
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.

Integer column will change to bigint type after running db:migrate in Rails 6.1 and SQLite3
5 participants