Class: QueryGuard::Migrations::MigrationRiskDetectors

Inherits:
Object
  • Object
show all
Defined in:
lib/query_guard/migrations/migration_risk_detectors.rb

Overview

Detects risky patterns in Rails migration files. Uses pragmatic line-by-line analysis rather than complex regex. Returns structured finding data for each detected risk.

Class Method Summary collapse

Class Method Details

.detect_concurrent_index_without_disable_ddl(content, migration_name) ⇒ Object

Detect algorithm: :concurrently without disable_ddl_transaction! This is necessary for PostgreSQL to allow concurrent index creation



63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
 
# File 'lib/query_guard/migrations/migration_risk_detectors.rb', line 63

def self.detect_concurrent_index_without_disable_ddl(content, migration_name)
  risks = []

  # Check if migration uses algorithm: :concurrently
  has_concurrent_index = content.include?("algorithm: :concurrently")
  return [] unless has_concurrent_index

  # Check if disable_ddl_transaction! is present (but not in a comment)
  lines = content.lines
  has_disable_ddl = lines.any? do |line|
    # Remove comment part
    code_part = line.split('#').first
    code_part.include?("disable_ddl_transaction!")
  end

  unless has_disable_ddl
    lines.each_with_index do |line, index|
      # Skip if this line is commented out
      code_part = line.split('#').first
      next unless code_part.include?("algorithm: :concurrently")

      risks << {
        type: :concurrent_index_no_disable_ddl,
        severity: :error,
        line_number: index + 1,
        migration_name: migration_name,
        title: "algorithm: :concurrently Without disable_ddl_transaction!",
        description: "Using algorithm: :concurrently requires disable_ddl_transaction! in the migration class to avoid transaction errors.",
        message: "algorithm: :concurrently found but disable_ddl_transaction! not set",
        recommendation: "Add `disable_ddl_transaction!` to the migration class definition",
        metadata: {
          operation: "add_index",
          risk_level: :high,
          module: :schema_safety
        }
      }
    end
  end

  risks
end

.detect_data_backfill_in_migration(content, migration_name) ⇒ Object

Detect data backfill / app model usage inside migrations Migrations that use ActiveRecord models are risky because:

  • Models can change independently of migrations
  • Queries can fail if code changes
  • Large updates lock tables


110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
 
# File 'lib/query_guard/migrations/migration_risk_detectors.rb', line 110

def self.detect_data_backfill_in_migration(content, migration_name)
  risks = []
  lines = content.lines

  has_model_usage = false
  model_usage_lines = []

  lines.each_with_index do |line, index|
    next if line.strip.start_with?("#")
    next if line.strip.empty?

    # Detect direct model class usage (User.find_each, Comment.update_all, etc.)
    if line.match?(/\b[A-Z]\w*\.(find|find_each|find_in_batches|all|where|update|create|delete|update_all|delete_all|execute)\b/)
      # Skip if it's obviously not a model (like Date, Time, etc.)
      unless line.match?(/\b(Date|Time|DateTime|Hash|Array|String|Integer|Float|Symbol|Regexp)\b/)
        has_model_usage = true
        model_usage_lines << { line_num: index + 1, content: line.strip }
      end
    end

    # Also detect batched iterations (common pattern in data migrations)
    if line.include?("find_each") || line.include?("find_in_batches")
      has_model_usage = true
      model_usage_lines << { line_num: index + 1, content: line.strip }
    end
  end

  if has_model_usage
    model_usage_lines.each do |item|
      risks << {
        type: :data_backfill_in_migration,
        severity: :error,
        line_number: item[:line_num],
        migration_name: migration_name,
        title: "Data Backfill Using App Models in Migration",
        description: "Using ActiveRecord models in migrations is risky because models can change independently. Large data operations should use batching and happen separately from schema changes.",
        message: "ActiveRecord model usage detected (#{item[:content][0..50]}...)",
        recommendation: "Move data backfill to a separate rake task or post-deploy job. Use raw SQL with batching if migration-embedded, or use a data migration gem.",
        metadata: {
          operation: "data_backfill",
          risk_level: :high,
          module: :schema_safety
        }
      }
    end
  end

  risks
end

.detect_full_table_updates(content, migration_name) ⇒ Object

Detect full-table updates inside migrations



242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
 
# File 'lib/query_guard/migrations/migration_risk_detectors.rb', line 242

def self.detect_full_table_updates(content, migration_name)
  risks = []
  lines = content.lines

  lines.each_with_index do |line, index|
    next if line.strip.start_with?("#")

    # Detect Model.update_all or delete_all calls
    if line.include?(".update_all") || line.include?(".delete_all")
      risks << {
        type: :full_table_update,
        severity: :error,
        line_number: index + 1,
        migration_name: migration_name,
        title: "Full-Table Update in Migration",
        description: "Updating all rows in a migration locks the table and is slow on large tables.",
        message: "update_all or delete_all in migration",
        recommendation: "Use a separate rake task or background job for large updates. Process rows in batches with pauses.",
        metadata: { operation: "updateall", risk_level: :high, locking: true }
      }
    end
  end

  risks
end

.detect_non_null_additions(content, migration_name) ⇒ Object

Detect adding NOT NULL columns without safe rollout pattern



214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
 
# File 'lib/query_guard/migrations/migration_risk_detectors.rb', line 214

def self.detect_non_null_additions(content, migration_name)
  risks = []
  lines = content.lines

  lines.each_with_index do |line, index|
    next unless line.include?("add_column")
    next if line.strip.start_with?("#")

    # Check for null: false without default
    if line.include?("null: false") && !line.include?("default:")
      risks << {
        type: :non_null_no_default,
        severity: :error,
        line_number: index + 1,
        migration_name: migration_name,
        title: "Non-NULL Column Without Default",
        description: "Adding a NOT NULL column without a default value will fail on populated tables.",
        message: "add_column null: false (no default)",
        recommendation: "Provide a default value, or add the column as nullable and backfill in separate step.",
        metadata: { operation: "add_column", risk_level: :high }
      }
    end
  end

  risks
end

.detect_risks(migration_content, migration_name) ⇒ Object

Detects migration risks from file content



10
11
12
13
14
15
16
17
18
19
20
21
22
23
 
# File 'lib/query_guard/migrations/migration_risk_detectors.rb', line 10

def self.detect_risks(migration_content, migration_name)
  risks = []

  # Run all detectors
  risks.concat(detect_unsafe_index_additions(migration_content, migration_name))
  risks.concat(detect_concurrent_index_without_disable_ddl(migration_content, migration_name))
  risks.concat(detect_table_locking_operations(migration_content, migration_name))
  risks.concat(detect_non_null_additions(migration_content, migration_name))
  risks.concat(detect_full_table_updates(migration_content, migration_name))
  risks.concat(detect_data_backfill_in_migration(migration_content, migration_name))
  risks.concat(detect_unsafe_raw_sql(migration_content, migration_name))

  risks
end

.detect_table_locking_operations(content, migration_name) ⇒ Object

Detect operations that lock the table: remove_column, change_column, rename_column



161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
 
# File 'lib/query_guard/migrations/migration_risk_detectors.rb', line 161

def self.detect_table_locking_operations(content, migration_name)
  risks = []
  lines = content.lines

  lines.each_with_index do |line, index|
    next if line.strip.start_with?("#")
    next if line.strip.empty?

    # Check with word boundary to match method calls with or without parentheses
    case line
    when /\bremove_column\b/
      risks << {
        type: :remove_column_lock,
        severity: :error,
        line_number: index + 1,
        migration_name: migration_name,
        title: "Remove Column Locks Table",
        description: "Removing a column rewrites the entire table, causing extended lock.",
        message: "remove_column operation locks table",
        recommendation: "Use safe_remove_column from a migration-safe gem, or manually soft-delete the column first",
        metadata: { operation: "remove_column", risk_level: :high, locking: true }
      }
    when /\bchange_column\b/
      risks << {
        type: :change_column_lock,
        severity: :error,
        line_number: index + 1,
        migration_name: migration_name,
        title: "Change Column Locks Table",
        description: "Changing a column type rewrites the entire table, causing extended lock.",
        message: "change_column operation locks table",
        recommendation: "Create new column, migrate data with backfill, then drop old column in separate migration",
        metadata: { operation: "change_column", risk_level: :high, locking: true }
      }
    when /\brename_column\b/
      risks << {
        type: :rename_column_lock,
        severity: :warn,
        line_number: index + 1,
        migration_name: migration_name,
        title: "Rename Column Brief Lock",
        description: "Renaming a column briefly locks the table during metadata update.",
        message: "rename_column operation locks table briefly",
        recommendation: "Use with caution in large tables; consider aliasing instead",
        metadata: { operation: "rename_column", risk_level: :medium, locking: true }
      }
    end
  end

  risks
end

.detect_unsafe_index_additions(content, migration_name) ⇒ Object

Detect index additions without algorithm: :concurrently or add_index without safe options



28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
 
# File 'lib/query_guard/migrations/migration_risk_detectors.rb', line 28

def self.detect_unsafe_index_additions(content, migration_name)
  risks = []

  # Find all add_index occurrences
  lines = content.lines
  lines.each_with_index do |line, index|
    # Skip commented-out lines
    next if line.strip.start_with?("#")
    next unless line.include?("add_index")

    # Check if line includes algorithm: :concurrently
    unless line.include?("algorithm: :concurrently") || line.include?("algorithm: :concurrent")
      risks << {
        type: :index_not_concurrent,
        severity: :error,
        line_number: index + 1,
        migration_name: migration_name,
        title: "Index Addition Without CONCURRENTLY",
        description: "Adding an index locks the table. Use algorithm: :concurrently for PostgreSQL.",
        message: "add_index without algorithm: :concurrently",
        recommendation: "Add algorithm: :concurrently to allow concurrent queries during index creation",
        metadata: {
          operation: "add_index",
          risk_level: :high,
          locking: true
        }
      }
    end
  end

  risks
end

.detect_unsafe_raw_sql(content, migration_name) ⇒ Object

Detect unsafe raw SQL statements



269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
 
# File 'lib/query_guard/migrations/migration_risk_detectors.rb', line 269

def self.detect_unsafe_raw_sql(content, migration_name)
  risks = []
  lines = content.lines

  lines.each_with_index do |line, index|
    next if line.strip.start_with?("#")
    next if line.strip.empty?

    # Check for dangerous SQL keywords in quoted strings in execute() calls
    if line.include?("execute") && (line.include?('"') || line.include?("'"))
      # Extract the SQL string from the line
      sql_match = line.match(/"([^"]+)"|'([^']+)'/)
      if sql_match
        sql = sql_match[1] || sql_match[2]
        upcase_sql = sql.upcase

        if upcase_sql.include?("TRUNCATE")
          risks << {
            type: :dangerous_raw_sql,
            severity: :error,
            line_number: index + 1,
            migration_name: migration_name,
            title: "TRUNCATE in Migration",
            description: "TRUNCATE operations are dangerous and can cause data loss.",
            message: "TRUNCATE detected in raw SQL",
            recommendation: "Avoid TRUNCATE; use safer alternatives or manual database cleanup",
            metadata: { operation: "raw_sql", risk_level: :critical }
          }
        elsif upcase_sql.include?("DROP")
          risks << {
            type: :dangerous_raw_sql,
            severity: :error,
            line_number: index + 1,
            migration_name: migration_name,
            title: "DROP in Migration",
            description: "DROP operations are dangerous and can cause data loss.",
            message: "DROP detected in raw SQL",
            recommendation: "Avoid DROP in migrations; use Rails schema helpers instead",
            metadata: { operation: "raw_sql", risk_level: :critical }
          }
        elsif upcase_sql.include?("LOCK TABLE")
          risks << {
            type: :explicit_table_lock,
            severity: :warn,
            line_number: index + 1,
            migration_name: migration_name,
            title: "Explicit Table Lock",
            description: "Explicit LOCK TABLE statements block all table access.",
            message: "LOCK TABLE detected in raw SQL",
            recommendation: "Avoid explicit locks; let migrations handle locking implicitly",
            metadata: { operation: "raw_sql", risk_level: :medium }
          }
        elsif (upcase_sql.include?("UPDATE") || upcase_sql.include?("DELETE")) && !upcase_sql.include?("WHERE")
          risks << {
            type: :unsafe_raw_sql_full_table,
            severity: :error,
            line_number: index + 1,
            migration_name: migration_name,
            title: "Full Table SQL Without WHERE",
            description: "UPDATE/DELETE without WHERE clause affects entire table.",
            message: "Full table operation without WHERE clause",
            recommendation: "Always include WHERE clause; batch operations for safety",
            metadata: { operation: "raw_sql", risk_level: :critical }
          }
        end
      end
    end

    # Also check for dangerous keywords on their own line
    upcase_line = line.upcase

    if upcase_line.include?("TRUNCATE") && !line.strip.start_with?("#")
      # Skip if already reported from execute() parsing
      next if line.include?("execute")

      risks << {
        type: :dangerous_raw_sql,
        severity: :error,
        line_number: index + 1,
        migration_name: migration_name,
        title: "TRUNCATE in Migration",
        description: "TRUNCATE operations are dangerous and can cause data loss.",
        message: "TRUNCATE detected",
        recommendation: "Avoid TRUNCATE; use safer alternatives or manual database cleanup",
        metadata: { operation: "raw_sql", risk_level: :critical }
      }
    elsif upcase_line.include?("DROP TABLE") || upcase_line.include?("DROP COLUMN")
      next if line.include?("execute")

      risks << {
        type: :dangerous_raw_sql,
        severity: :error,
        line_number: index + 1,
        migration_name: migration_name,
        title: "DROP in Migration",
        description: "DROP operations are dangerous and can cause data loss.",
        message: "DROP detected",
        recommendation: "Avoid DROP in migrations; use Rails schema helpers instead",
        metadata: { operation: "raw_sql", risk_level: :critical }
      }
    elsif upcase_line.include?("LOCK TABLE")
      next if line.include?("execute")

      risks << {
        type: :explicit_table_lock,
        severity: :warn,
        line_number: index + 1,
        migration_name: migration_name,
        title: "Explicit Table Lock",
        description: "Explicit LOCK TABLE statements block all table access.",
        message: "LOCK TABLE detected",
        recommendation: "Avoid explicit locks; let migrations handle locking implicitly",
        metadata: { operation: "raw_sql", risk_level: :medium }
      }
    end
  end

  risks
end