Strange migration error in tests during GH workflow

Development

AI-generated summary

The discussion revolves around a strange migration error in tests during GitHub workflow. merefield is trying to create a foreign key relationship with the Topics table but is encountering an error where it’s trying to access a field (topics.off_topic_count) that was removed in a core migration years ago. Despite confirming that the migration to remove this field has run, the error persists.

merefield suspects that the issue might be related to using t.references in the migration, which is flagged by Rubocop as high-risk. However, david believes that the Rubocop advice, while good, is not the cause of this error. Instead, he thinks the issue might be due to invoking Rake tasks inside a migration, which can have weird side effects.

david suggests that the ActiveRecord schema cache might be populated early in the migrations, causing it to try to load columns that no longer exist. He recommends avoiding invoking Rake tasks altogether and using raw SQL within the migration instead. merefield decides to move the code to a SQL statement, and as confirmed in merefield, this resolves the issue.

1

I’m trying to create a foreign key relationship with the Topics table.

The problem is it is failing in github workflow test environment during tests for the strangest reason, it is trying to access a field from the parent table that no longer exists and was removed in a core migration years ago!

The error is PG::UndefinedColumn: ERROR: column topics.off_topic_count does not exist

well yeah, because it was removed in a core migration in 2018!

I’ve confirmed this has already run during the test process:

== 20180917024729 RemoveSuperfluousColumns: migrating =========================
== 20180917024729 RemoveSuperfluousColumns: migrated (0.0410s) ================

I’m not in any way referencing this old parent table field explicitly … it seems to be generating the SQL itself … but inappropriately for the current definition of things.

== 20231119010101 CreateLocationsTopicTable: migrating ========================rake aborted!

[12035](https://github.com/paviliondev/discourse-locations/actions/runs/7039607316/job/19158951878?pr=103#step:19:12036)StandardError: An error has occurred, this and all later migrations canceled: (StandardError)

[12036](https://github.com/paviliondev/discourse-locations/actions/runs/7039607316/job/19158951878?pr=103#step:19:12037)

[12037](https://github.com/paviliondev/discourse-locations/actions/runs/7039607316/job/19158951878?pr=103#step:19:12038)PG::UndefinedColumn: ERROR: column topics.off_topic_count does not exist

[12038](https://github.com/paviliondev/discourse-locations/actions/runs/7039607316/job/19158951878?pr=103#step:19:12039)LINE 1: ...cs"."deleted_at", "topics"."highest_post_number", "topics"."...

The table def is really simple:

class CreateLocationsTopicTable < ActiveRecord::Migration[7.0]
  def change
    create_table :locations_topic do |t|
      t.references :topic, foreign_key: true
      t.float :latitude, null: false

SNIP

The even weirder part is that this migration works in Production!

Any insights most welcome!

2

Oh, I’ve been good ole RUBOCOPPED for this :policeman: :police_car:

Offenses:

db/migrate/20231119010101_create_locations_topic_table.rb:6:7: C: Discourse/NoAddReferenceOrAliasesActiveRecordMigration: AR methods add_reference, add_belongs_to, t.references, and t.belongs_to are
high-risk for large tables and have too many background magic operations.
Instead, write a disable_ddl_transactions! migration and write custom SQL to
add the new column and CREATE INDEX CONCURRENTLY. Use the IF NOT EXISTS clause
to make the migration re-runnable if it fails partway through.

      t.references :topic, foreign_key: true
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Now that might be part of the issue.

Too much :magic_wand: going on, that’s the problem!

You live and you learn! :mortar_board:

3

The rubocop advice is good, but I don’t think it’s the cause of this error. (the ‘risk’ it mentions refers to locks on high-traffic tables which would affect production traffic. So not something which would cause errors in a test environment)

You can probably reproduce the same error locally by doing RAILS_ENV=test bin/rake db:drop db:create db:migrate (i.e. migrating a database totally from scratch, with the locations plugin enabled)

I suspect the issue is that you’re invoking Rake tasks inside a migration. In core we generally avoid running any kind of application code in migrations because of the weird side effects that can happen. Best to stick to raw SQL.

In this case, my assumption is that the ActiveRecord schema cache is being populated early in the migrations (when topics.off_topic_count still exists). Then, when your rake task runs, it’s running with an old schema cache and so ActiveRecord tries to load columns which no longer exist.

You can probably mitigate by adding ActiveRecord::Base.clear_cache! before invoking the rake tasks… but don’t take that as a recommendation :wink:. Best thing would be to avoid invoking the rake tasks altogether. If you need to do manipulate something in the database, use raw SQL within the migration.

4

Thanks David, I’ll move it to a SQL statement and see if that resolves …

5

That did it, thanks David!

https://github.com/paviliondev/discourse-locations/pull/103/commits/296a890791f6507d35045afbbef7c4e1e98befdf