Reset the column cache after adding default_miq_group_id to tenants

[carbonin]

Before this change, rails was not aware of the column if it was added in the same process that it was queried in.

Specifically the column cache here is not reset after the add_column call so for https://github.com/ManageIQ/manageiq/blob/master/db/migrate/20151021174140_assign_tenant_default_group.rb#L5 to work properly, we need to reset the info manually.

For example, the column would be added, but when we tried to set the value in 20151021174140_assign_tenant_default_group.rb we would silently fail or get a backtrace with:

99000000000001 is out of range for ActiveModel::Type::Integer with limit 4

After this change, the column value is set correctly.

https://bugzilla.redhat.com/show_bug.cgi?id=1344112

[carbonin]

@jrafanie please review

[chessbyte]

Does creating the stub class on line 2 of this file negate the need for this reset?

[Fryguy]

@carbonin As discussed, this is really just a hack because of oddities (dare I say, bugs) in ActiveRecord::Migration, but it's good to get us over the hump of not being able to migrate the database.

cc @lpichler

[Fryguy]

@chessbyte Marking this as a blocker, because you can't upgrade in one-shot without it. That is, db:migrate fails, and then you have to manually db:migrate again as a workaround.

[jrafanie]

Just so we don't lose it in gitter, I believe AR's add_column, remove_column, change_column, etc., found here for the abstract adapter, should reset the column information after the execute call:

def add_column(table_name, column_name, type, options = {})
  at = create_alter_table table_name
  at.add_column(column_name, type, options)
  ret = execute schema_creation.accept at
  Class.new(ActiveRecod::Base) { self.table_name = table_name }.reset_column_information!
  ret
end

If this makes sense, I can see about trying to open a PR on rails.

[jrafanie]

From gitter, 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.

[jrafanie]

@matthewd At your convenience, I'd love your opinion of this HACK. Is that feasible?

[Fryguy]

Merging this for now as it's a decent workaround that fixes upgrades. However, I'd like to see a general fix regardless. @carbonin or @jrafanie can you open an GH issue to track fixing it?

[matthewd]

Someone looked at something around here... @chrisarcand maybe?

IIRC, we decided it was Expected Behaviour that you needed to manually reset the column info if you wanted to use the column inside the migration, but that it was expected to happen automatically at the end of the migration.

[chrisarcand]

Yes, this is an issue along the same vein 😑

@carbonin As you may know, there are many separate caches at the class level of AR models + the adapter cache. My speculation, unproven: Columns are being looked up by model name at at the adapter level in some way we haven't seen and so our way of namespacing within the migration to avoid the caching doesn't work anymore.

I'll look further in to this next week, but as usual this fix is the most appropriate for now 👍

[jrafanie]

Someone looked at something around here... @chrisarcand maybe?

IIRC, we decided it was Expected Behaviour that you needed to manually reset the column info if you wanted to use the column inside the migration, but that it was expected to happen automatically at the end of the migration.

@matthewd @chrisarcand specifically, this is one migration adding a column that a followup migration doesn't know about. Within the same migration, it makes sense to reset the column information after adding/removing/changing a column if you want to use this column. In this PR, it was the former (one migration messing up a subsequent migration) and it's ugly to have this column cache knowledge of prior/subsequent migrations. It doesn't make sense to have to reset column information after you add/remove/change columns "just in case" some other subsequent migration needs to use this column. We'd have to put this all over all of our migrations and would be really disgusting.

My suggestion above was that it seems that only add/remove/change column calls know when we invalidate the table's cached columns, so they should reset this cache. I don't know where else we would know when the table's column cache is invalidated.

[matthewd]

Found the previous discussion: rails/rails#24300

So.. I'm resignedly-okay with needing the explicit reset within a migration (read: I think it could be better, but don't care enough to fight for it), but agree with you that requiring it across migrations seems too onerous. Maybe that "just" means this line should instead be invoked after each individual migration. As I'm thinking it through, that sounds pretty sensible.

[chrisarcand]

Maybe that "just" means this line should instead be invoked after each individual migration. As I'm thinking it through, that sounds pretty sensible.

@matthewd I don't think that will fix the issue, IIRC 😕 See the code in that issue you mention; the issue we faced (and continue to face) is that there's no single way to clear all model caches at once. .clear_cache! sadly only clears the adapter cache and leaves individual model caches populated.

[swrobel]

@chrisarcand did you ever find a command that will clear the cache for all models?

[jrafanie]

@swrobel See: ManageIQ/manageiq-schema#401 ... our migrations are in an engine that wraps all migrations with ActiveRecord::Base.connection.schema_cache.clear!, which does more than just column information. It's our humble opinion that caching of columns/indexes/key information during migrations is something that should not be happening. As seen in the first link, it's not developer friendly at all to expect developers to reset column information whenever they use a migration stub.