These are chat archives for ManageIQ/manageiq/rails5

10th
Jun 2016
Nick Carboni
@carbonin
Jun 10 2016 18:01

https://github.com/ManageIQ/manageiq/blob/master/db/migrate/20151021174044_add_tenant_default_group.rb + https://github.com/ManageIQ/manageiq/blob/master/db/migrate/20151021174140_assign_tenant_default_group.rb = 99000000000001 is out of range for ActiveModel::Type::Integer with limit 4

Looks like add_column doesn't invalidate the column cache, essentially rails/rails#24273

Joe Rafaniello
@jrafanie
Jun 10 2016 18:02
:cry:
Jason Frey
@Fryguy
Jun 10 2016 18:03
Ugh
Nick Carboni
@carbonin
Jun 10 2016 18:03
In this case Tenant.columns.select { |c| c.name == "default_miq_group_id" }.inspect yields an empty array before the add, after the add and before the update in the subsequent migration
Jason Frey
@Fryguy
Jun 10 2016 18:03
So what's the effect... Can't migrate? Have to rerun migration after it fails?
Nick Carboni
@carbonin
Jun 10 2016 18:03
Have to rerun
Jason Frey
@Fryguy
Jun 10 2016 18:04
:poop:
Nick Carboni
@carbonin
Jun 10 2016 18:04
That will start a new process and have a new column cache for the model
Joe Rafaniello
@jrafanie
Jun 10 2016 18:04
@matthewd :pray: please tell me you think it's obvious that add_column/remove_column knows that it should clear the cache but doesn't. Are we supposed to add reset_column_info everywhere we do add/remove column?
Does it make sense to monkey patch add/remove column to auto-create a model stub based on the table name, and reset the column info after it does the add/remove?
Jason Frey
@Fryguy
Jun 10 2016 18:05
Is it add_column/remove_column that's at fault, or that we are using stubs?
Joe Rafaniello
@jrafanie
Jun 10 2016 18:05
stubs share the same backing cache
Jason Frey
@Fryguy
Jun 10 2016 18:06
well that, in itself, breaks the migration isolation
Joe Rafaniello
@jrafanie
Jun 10 2016 18:06
two different stubs share the same cache
Jason Frey
@Fryguy
Jun 10 2016 18:06
but yeah, it's strange that the cache is wrong immediately after an add_column or remove_column
Joe Rafaniello
@jrafanie
Jun 10 2016 18:06
based on table name
It's also weird that it doesn't know about the column at all in the cache but yet says "you can't fit that in this column type"
almost like it's assuming a *_id column is an Integer column
@carbonin So, I guess for now, we know how to fix it in the failing migration (the victim that tries to use that column)
Nick Carboni
@carbonin
Jun 10 2016 18:09
Do we want to do it there or in the migration that adds the column?
Joe Rafaniello
@jrafanie
Jun 10 2016 18:11
So, if we can guarantee that no database is stuck at the "victim(later one that tries to use the column)" migration, we can do it right after we add the column
I didn't look to see how old that migration is
Nick Carboni
@carbonin
Jun 10 2016 18:12
Ah it's old ...
But they're both old ...
2015-10-21
Looks like they are in capablanca
So feels like a regression
Joe Rafaniello
@jrafanie
Jun 10 2016 18:15
hmm
So, rails5?
Jason Frey
@Fryguy
Jun 10 2016 18:15
yeah, probably removed because THEY SHOULDN"T BE NEEDED
:P
Nick Carboni
@carbonin
Jun 10 2016 18:29
Oh so default_miq_group_id just didn't get set on a region 0 db
Jason Frey
@Fryguy
Jun 10 2016 18:29
o_O
Nick Carboni
@carbonin
Jun 10 2016 18:29
I assume that's not okay ...
So it didn't blow up because we weren't over the integer limit, but also didn't set the value ...
Jason Frey
@Fryguy
Jun 10 2016 18:30
can you clarify...is there a data migration setting it to 0 that's failing, or something else?
Isn't setting or failing
Jason Frey
@Fryguy
Jun 10 2016 18:31
wtf
Nick Carboni
@carbonin
Jun 10 2016 18:31
The value is nil after migration
Jason Frey
@Fryguy
Jun 10 2016 18:31
Don't we have specs for that?
or do the specs pass, but real migration fails?
Nick Carboni
@carbonin
Jun 10 2016 18:32
Sure, but not ones that add the column and try to set it in one go
Jason Frey
@Fryguy
Jun 10 2016 18:32
I'm kind of in the @jrafanie camp of prepend add and remove column for now
would that also solve this case?
Nick Carboni
@carbonin
Jun 10 2016 18:33
I think so, I should be able to test
One sec
Nick Carboni
@carbonin
Jun 10 2016 18:39
Yup, Tenant.reset_column_information after the add_column also solves this
Nick Carboni
@carbonin
Jun 10 2016 18:47
PR ManageIQ/manageiq#9177
Oleg Barenboim
@chessbyte
Jun 10 2016 18:54
@carbonin maybe the issue is that there was no stub class in the migration?
Nick Carboni
@carbonin
Jun 10 2016 18:54
Shouldn't need one for just an add_column I don't think ...
Joe Rafaniello
@jrafanie
Jun 10 2016 18:57
Yeah, I don't think so. I believe the problem is that even if we use two different class stubs (with the same table name) in two different migrations, they end up using the same cache key'd by the table name
see here
and add/remove/change column doesn't invalidate the cache
Oleg Barenboim
@chessbyte
Jun 10 2016 18:59
gotcha - can you guys make sure that gets added to PR description
Nick Carboni
@carbonin
Jun 10 2016 18:59
:+1:
Joe Rafaniello
@jrafanie
Jun 10 2016 18:59
Great idea, WHY is important especially for hacks
Oleg Barenboim
@chessbyte
Jun 10 2016 19:05
if I don't grok it today, you may not grok it 6-12 months from now
Joe Rafaniello
@jrafanie
Jun 10 2016 19:05
:heart:
Jason Frey
@Fryguy
Jun 10 2016 19:10
Please include the why in the commit message for future git blame users. I know for a fact I removed this in the first place because it shouldn't be needed. I wouldn't have done that if the git blame told me exactly why it was added
Joe Rafaniello
@jrafanie
Jun 10 2016 19:10
it feels like we should have clear_columns_cache like this but after the super call
super
Class.new(Application::Record) { self.table_name = table_name }.reset_column_information!
or something like that