ForeignKeyTypeChecker duplicates

[pirj]

ForeignKeyTypeChecker seems to report the same problem twice (or more for e.g. polymorphic associations).

1. Duplicate

  create_table "reports", force: :cascade do |t|
    # ...
  end

  create_table "lines", id: :serial, force: :cascade do |t|
    t.integer "report_id"
    # ...
  end

  add_foreign_key "lines", "reports"

class Line < ApplicationRecord
  belongs_to :report, optional: true
  # ...
end

class Report < ApplicationRecord
  has_many :lines, dependent: :destroy
  # ...
end

ForeignKeyTypeChecker fail Line report foreign key (report_id) with type (integer) doesn't cover primary key (id) with type (bigint)
ForeignKeyTypeChecker fail Report lines foreign key (report_id) with type (integer) doesn't cover primary key (id) with type (bigint)

2. Multiplicate (for a polymorphic association, paper_trail)

  create_table "versions", id: :serial, force: :cascade do |t| # PaperTrail gem
    t.string "item_type", null: false
    t.integer "item_id", null: false
    # ...
  end

# Not much to show here, as it's all just from PaperTrail
class Foo < ApplicationRecord
  has_paper_trail
  # ...
end

class Bar < ApplicationRecord
  has_paper_trail
  # ...
end

class Baz < ApplicationRecord
  has_paper_trail
  # ...
end

ForeignKeyTypeChecker fail Foo versions foreign key (item_id) with type (integer) doesn't cover primary key (id) with type (bigint)
ForeignKeyTypeChecker fail Bar versions foreign key (item_id) with type (integer) doesn't cover primary key (id) with type (bigint)
ForeignKeyTypeChecker fail Baz versions foreign key (item_id) with type (integer) doesn't cover primary key (id) with type (bigint)
# ... more of them in my app, one per model with `has_paper_trail`

[djezzzl]

Hi @pirj ,

Thank you for reporting this!

Could you please help me to understand what you think we should do instead?

I see it this way: every has_one association has its own unique pair (model - model) and even though they all share the same foreign key because of belongs_to association, it's still a kind of a separate case, isn't it?

About the combination of has_one and belong_to for both sides, we can keep just one.

P.S. I guess more "noise" makes people fix it quickly or do you think it's better to reduce it?

[pirj]

Hey @djezzzl ! 👋

In the first case, it's a one-to-many.
If ForeignKeyTypeChecker analysed it just from one side, would this be sufficient?

ForeignKeyTypeChecker fail Line report foreign key (report_id) with type (integer) doesn't cover primary key (id) with type (bigint)
- ForeignKeyTypeChecker fail Report lines foreign key (report_id) with type (integer) doesn't cover primary key (id) with type (bigint)

The report_id FK is on the Line/lines table.

This class checks if association's foreign key type covers associated model's primary key (same or bigger)

For polymorphic associations it's mire complicated, as some associated tables may have the same key type, and some - a bigger one.

  create_table "bars", force: :cascade do |t| # bigint! too big for `item_id`
    # ...
  end

  create_table "bazs", id: :serial, force: :cascade do |t| # integer
   # ...
  end

No good ideas how to behave in this case. Show it ust for those that are not covered?

ForeignKeyTypeChecker fail Foo versions foreign key (item_id) with type (integer) doesn't cover primary key (id) with type (bigint)
- ForeignKeyTypeChecker fail Bar versions foreign key (item_id) with type (integer) doesn't cover primary key (id) with type (bigint)
ForeignKeyTypeChecker fail Baz versions foreign key (item_id) with type (integer) doesn't cover primary key (id) with type (bigint)

more "noise" makes people fix it quickly or do you think it's better to reduce it?

I don't have a strong opinion here. It may deter people from fixing if the task looks like it needs more effort.

[djezzzl]

Hi @pirj ,

I've just released 1.3.6 with the improvement. Could you please have a look and say if that's any better?

Feel free to reopen the issue if needed and I wish you a good weekend.

[pirj]

Thank you, @djezzzl

At a glance, there might be a regression that swallows some cases, as the list of ForeignKeyTypeChecker offences went down from 109 entries to 7.

A random example of what went missing:

ForeignKeyTypeChecker fail Alert user associated model key (id) with type (integer) mismatches key (user_id) with type (bigint)

class User < ApplicationRecord
  has_many :alerts, dependent: :nullify
end

class Alert < ApplicationRecord
  belongs_to :user
end

  create_table "users", id: :serial, force: :cascade do |t|
  end

  create_table "alerts", force: :cascade do |t|
    t.bigint "user_id"
  end

  add_foreign_key "alerts", "users"

The ones with versions described above all collapsed to a single offence, i.e.

ForeignKeyTypeChecker fail Foo versions foreign key (item_id) with type (integer) doesn't cover primary key (id) with type (bigint)
- ForeignKeyTypeChecker fail Bar versions foreign key (item_id) with type (integer) doesn't cover primary key (id) with type (bigint)
- ForeignKeyTypeChecker fail Baz versions foreign key (item_id) with type (integer) doesn't cover primary key (id) with type (bigint)

instead of:

ForeignKeyTypeChecker fail Foo versions foreign key (item_id) with type (integer) doesn't cover primary key (id) with type (bigint)
- ForeignKeyTypeChecker fail Bar versions foreign key (item_id) with type (integer) doesn't cover primary key (id) with type (bigint)
ForeignKeyTypeChecker fail Baz versions foreign key (item_id) with type (integer) doesn't cover primary key (id) with type (bigint)

Please let me know if you need more.

[djezzzl]

A random example of what went missing:

Could you please share the before output and after? Without that, unfortunately, I can't say if that's expected or not.

The ones with versions described above all collapsed to a single offense, i.e.

Why it should be collapsed only to two cases rather than to a single one? As soon as item_id will be updated on versions on the table to bigint, there will be no errors at all, right? So, it's expected to have only a single output then. Could you please say if I'm missing anything?

[pirj]

Why it should be collapsed only to two cases rather than to a single one? As soon as item_id will be updated on versions on the table to bigint, there will be no errors at all, right? So, it's expected to have only a single output then.

One association is picked while others are hidden.
Collapsing might be fine, but.
Imagine you have two tables on the loose side of the polymorphic association, e.g. articles and comments.
articles are at 1M records, and are unlikely to hit the integer limit anytime soon.
comments are at 2'000'000'000 records, and are about to hit the limit.
Both of those tables' id is bigint, while authorable table has content_id pointing at articles and comments has integer type.
If you (randomly) pick Articles to show as an offence, users might glance over and decide that it's not worth it to change content_id to bigint, just not worth the effort, as articles are quite unlikely to hit the limit, and integer content_id would do just fine.
But the picture for comments is different, and content_id wouldn't cover comments.id quite soon, but we hide this.

[djezzzl]

And what should we do in this case? I don't think that we should be too smart on this one and anyhow analyze the persisted data amount.

What do you think?

Looking at this now, I think when we remove duplicates, we may highlight how many offenses we grouped. What do you think a good message would look like in this case?

[pirj]

If it could list the offending tables that were collapsed, that would be sufficient:

- ForeignKeyTypeChecker fail Foo versions foreign key (item_id) with type (integer) doesn't cover primary key (id) with type (bigint)
- ForeignKeyTypeChecker fail Bar versions foreign key (item_id) with type (integer) doesn't cover primary key (id) with type (bigint)
- ForeignKeyTypeChecker fail Baz versions foreign key (item_id) with type (integer) doesn't cover primary key (id) with type (bigint)
+ ForeignKeyTypeChecker fail (Models Foo, Baz) versions foreign key item_id with type integer doesn't cover primary key id with type bigint

Indeed, we can't have an idea of the number of production records when running database_consistency on a local dev machine.

[djezzzl]

Could you please help me understand why you're missing Bar? Doesn't it also have inconsistency in types?

I like listing all the models.

BTW, have you found more missing cases?

[pirj]

As in the example above, one of those tables has an integer id, and doesn't have to be listed. My bad, in that example bazs has it, not bars.

[djezzzl]

Extended output is on the way: #150.