Closed pirj closed 1 year ago
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?
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.
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.
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.
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?
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.
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?
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.
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?
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
.
Extended output is on the way: https://github.com/djezzzl/database_consistency/pull/150.
ForeignKeyTypeChecker
seems to report the same problem twice (or more for e.g. polymorphic associations).paper_trail
)