search

Electron100 / butane

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

Add missing Clone trait #24

Closed jayvdb closed 1 year ago

Electron100 commented 1 year ago

Thanks for the PR! Clone on Many makes sense, but Default on ForeignKey doesn't seem quite right. An empty default isn't really meaningful. Today ForeignKey enforces the internal invariant that either its val or valpk field is set. Initializing both of them to empty/None will result in an invalid state on which several methods will panic. Can you help me understand what you're hoping to achieve? We may be able to find a tweak that meets the same goal without putting the structure into a panic-prone state.

jayvdb commented 1 year ago

I would like some implementation of Default on ForeignKey, as I need Default on the structs which use a ForeignKey for one of the members, so all members type need Default. This requirement is not under my control - the structs are used by other tools which require Default. Using #[derive(..)] did the trick for me, but I can appreciate that the simplistic implementation from #[derive(..)] may cause panics which I haven't found because I have limited use for it unrelated to butane.

Electron100 commented 1 year ago

Thanks for the explanation. I understand the constraint from another library -- the problem though is that a ForeignKey initialized without any information is semantically invalid: it's essentially uninitialized and you can't do anything with it. I think you have a couple options within your code.

  1. You could use an Option<ForeignKey<T>>. Butane should handle that fine (if you see issues, I'll treat it as a bug) and semantically expresses the reality that you may or may not have a valid ForeignKey. Of course when used in a #[model], it also makes the column in your database nullable -- that may or may not be what you want here.
  2. Your impl of Default could call ForeignKey::from_pk and pass 0, or empty string, or zero'd UUID, or whatever's most appropriate for the primary key you're using. Of course you still have an invalid ForeignKey that you can't do anything with, but now you've put that concept only into your application code where you can ensure you account for it judiciously, rather than pushing it into Butane where it could easily become a footgun.
jayvdb commented 1 year ago

ok, I've removed the Default. Thanks for your thoughts about that. I am going to need mandatory fkeys, so I'll chew on that problem for a bit.