Fixes #39465 - Migrate taxonomies.ignore_types from YAML text to JSONB#11046
Fixes #39465 - Migrate taxonomies.ignore_types from YAML text to JSONB#11046adamruzicka wants to merge 2 commits into
Conversation
78370d0 to
c9e767b
Compare
|
The original was merged, we can rebase now once more 🙏 |
PR theforeman#10990 review feedback suggested replacing the fragile YAML-based potentially_ignoring scope. This migration provides: - **Exact matching**: JSONB containment operator (@>) eliminates over-matching (e.g., 'User' no longer matches 'UserGroup') - **No post-filter**: Previously required Ruby .ignore? check is gone - **Indexed queries**: GIN index enables O(1) lookup vs sequential scan - **Format independence**: No coupling to YAML serialization Renamed scope from potentially_ignoring to ignoring since JSONB containment provides exact matching - no "potentially" about it. Added comprehensive test coverage for the scope including exact matching, no over-matching, empty arrays, and multiple types. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Pass User class directly to ignoring scope instead of string 'user' - Replace LIKE queries in cleanup_helper with ignoring scope Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
c9e767b to
2c90353
Compare
ofedoren
left a comment
There was a problem hiding this comment.
Review — Migrate ignore_types from YAML text to JSONB
The migration to jsonb is the right call — it eliminates the LIKE-on-YAML fragility, removes the Ruby post-filter, and enables GIN indexing. Three issues to address before merge.
1. Migration's serialize syntax won't survive a Rails upgrade
The fake model in the migration uses:
serialize :ignore_types_yaml, Array, coder: YAMLOn Rails 7.0, coder: YAML falls into **options and is silently ignored — it works by accident because Array is treated as the positional class_name_or_coder arg. On Rails 7.1, passing Array as a positional arg is deprecated. On Rails 7.2, it's removed entirely. This migration would break on upgrade.
Fix:
serialize :ignore_types_yaml, coder: YAMLThe YAML deserializer returns whatever was stored (an Array), so the type hint is unnecessary.
2. The ignoring scope does unnecessary normalization
scope :ignoring, ->(type) { where("ignore_types @> ?", [type.to_s.classify].to_json) }.to_s.classify silently normalizes lowercase or mismatched input (e.g. 'user' → 'User'). But all call sites pass either a class constant (User) or a properly cased string ('Environment'). This is developer-written code, not user input — silently fixing bad input hides bugs.
Additionally, the call sites are inconsistent: user.rb passes User (class), cleanup_helper.rb passes 'Environment' (string). Pick one convention. Since ignore_types stores strings like 'User', passing strings is more natural and avoids the .to_s indirection:
scope :ignoring, ->(type) { where("ignore_types @> ?", [type].to_json) }And update the user.rb call site from ignoring(User) to ignoring('User').
3. Migration should guard against malformed YAML data
The up migration reads ignore_types_yaml via the YAML deserializer and writes to the jsonb column:
taxonomy.update_column(:ignore_types, taxonomy.ignore_types_yaml || [])If any row has malformed YAML or a non-array value (e.g. a bare string, a hash), the deserializer could return an unexpected type or raise. Adding a guard costs nothing:
value = taxonomy.ignore_types_yaml
value = [] unless value.is_a?(Array)
taxonomy.update_column(:ignore_types, value)Informational (not blocking)
foreman_puppet migration — The existing migration at foreman_puppet/db/migrate/20220208135305_migrate_environment_ignore_type.foreman_puppet.rb uses LIKE '%Environment%' on ignore_types. On new installations this runs before the jsonb migration (by timestamp ordering), so it works. But foreman_puppet should update this migration to use jsonb syntax as a follow-up. The PR author already noted this.
This review was assisted by Claude Code (claude-opus-4-6).
|
Just few notes:
|
Split from #10990