Electron100 / butane

An ORM for Rust with a focus on simplicity and on writing Rust, not SQL
Apache License 2.0
84 stars 12 forks source link

PartialEq for Many and ForeignKey #70

Open jayvdb opened 1 year ago

jayvdb commented 1 year ago

The implementations of PartialEq for Many and ForeignKey currently have no tests that cover them properly. The tests string_pk, fkey_match & rustval requires that a ForeignKey or Many returns true during a comparison of a stored object and its fetched equivalent. i.e. No test currently depends on eq returning false.

The implementation for ForeignKey only checks that the key value is identical.

Consider this, which passes

diff --git a/butane/tests/basic.rs b/butane/tests/basic.rs
index 2cc217f..1018952 100644
--- a/butane/tests/basic.rs
+++ b/butane/tests/basic.rs
@@ -206,6 +206,18 @@ fn string_pk(conn: Connection) {

     let bar2 = Bar::get(&conn, "tarzan".to_string()).unwrap();
     assert_eq!(bar, bar2);
+
+    bar2.foo.load(&conn).unwrap();
+
+    let mut foo2 =  Foo::get(&conn, 1).unwrap();
+    foo2.bam = 1.1;
+    foo2.save(&conn).unwrap();
+
+    let bar3 = Bar::get(&conn, "tarzan".to_string()).unwrap();
+    bar3.foo.load(&conn).unwrap();
+    assert_eq!(bar2, bar3);
+    assert_eq!(bar2.foo.get().unwrap().bam, 0.0);
+    assert_eq!(bar3.foo.get().unwrap().bam, 1.1);
 }
 testall!(string_pk);

Now I understand why this is the current implementation. But it is worth thinking about whether it is correct, as it can be quite surprising. If retained like this, it should be documented very prominently.

The Many implementation similar checks only that they target item_table and owner are the same - the content could wildly differ. This one is worse, because .add or .remove could have been invoked, making the two objects very-very different but they would still say they are equivalent.