core: Sort constraint definitions before comparing #727
Conversation
When updating a database, we may falsely report schemas as not compatible due to the `ConstraintDef`s coming out in different orders in the proposed and existing schemas. Not sorting them was an oversight in 9bb2b21.
There was a problem hiding this comment.
@kim I am going to stamp this but won't pretend to understand the difference :) May I steal some of your time soon to get a walk-through of the code that gives some context? I've been meaning to ask you, anyways :)
| .filter(|c| c.constraints != Constraints::unset()) | ||
| .collect() | ||
| } | ||
|
|
There was a problem hiding this comment.
Could be better to do this change inside TableDef?:
pub struct TableDef {
pub table_name: String,
pub constraints: BtreeSet<ConstraintDef>,
}
There was a problem hiding this comment.
Do we actually want to store them like that? We might just want this function as a way to compare them. I think I'm going to propose that we merge this and consider that for future work.
There was a problem hiding this comment.
It threw me off a bit to realize that there can be unset constraints in the table def, so we would still need an element-wise comparison if it was a set already.
| .filter(|c| c.constraints != Constraints::unset()) | ||
| .collect() | ||
| } | ||
|
|
There was a problem hiding this comment.
Do we actually want to store them like that? We might just want this function as a way to compare them. I think I'm going to propose that we merge this and consider that for future work.
When updating a database, we may falsely report schemas as not
compatible due to the
ConstraintDefs coming out in different orders inthe proposed and existing schemas.
Not sorting them was an oversight in 9bb2b21.
Expected complexity level and risk
1