brendon/ranked-model

v0.4.6 Error: ActiveRecord::NotNullViolation: PG::NotNullViolation: ERROR: null value in column "row_order" violates not-null constraint

richardonrails opened this issue · 5 comments

richardonrails commented

I just upgraded to 0.4.6 and got a (not easily reproducible) error from on our my users so far:

ActiveRecord::NotNullViolation: PG::NotNullViolation: ERROR: null value in column "row_order" violates not-null constraint

Which occurred when trying to create a new record in the table.

I have:

ranks :row_order, with_same: [:site_id, :category_id] and t.integer "row_order", null: false

Do we need to make this column support NULL with null: true now?

brendon commented

Hi @richardonrails, that's an interesting one. I'm not aware of there being any issue with having a not null constraint on the column. Are you able to do a bit of digging to see if you can reproduce this with a test?

This could be happening when a rebalance happens which would explain why it's hard to track down. Try cloning the repo and getting the test suite running then set the constraint and see if it triggers in any of the existing tests perhaps?

rcugut commented

I stumbled over the same issue.

TL;DR

Do we need to make this column support NULL with null: true now?

Yes: remove the not-null constraint (i.e. allow NULL values in the DB column).

Between v0.4.5 and v0.4.6, among other changes in pull #164 , this quite important piece was added:

https://github.com/mixonic/ranked-model/pull/164/files#diff-a5a9575b90964f80c71d3536d46a0390R73

Which is then used during rebalance_ranks.

I would say it's a good approach by @janklimo , and it's probably the "lesser evil" between doing way more complex logic in the rebalancing algorithm of ranks, or just removing the not-null constraint from the DB column.

@brendon if you agree to leave things as is, I suggest to put this note (about removing the not-null constraint) very clearly in the README. and I'm offering to do a pull request for it if you want to :-)

FYI, if the not-null constraint exists, it will not trigger an issue/error initially. It happened to us after about 30 inserts in the table, quite well into full production deployment. So it's a bit of a time-bomb 💥 issue! :-)

  • 👍2
brendon commented

Ah I see! Yes that is a bit of a time bomb! I think it's probably best to leave this as-is as you say. The column is internally managed by ranked-model and definitely requires the ability to null the value as you say.

I'd be stoked if you could do up a PR with the warning message. Maybe add it close to the migration code?

Thanks heaps for tracing this one.

rcugut commented

Sure, I'll find some time tis week. Thanks for maintaining this ;-)

  • 👍3
richardonrails commented

Thanks for updating readme, looks like we can just close this now.

  • 👍1