Add support for newtype-uuid?

[sunshowers]

Hi there,

I'm the primary maintainer of newtype-uuid, a crate which provides type-level annotations to UUIDs via a TypedUuid wrapper, so that we can avoid mixing them up. It's a relatively new crate, but one that we've had a lot of success with at Oxide. There's nothing else quite like it in the Rust ecosystem (the closest crate is typed-uuid which is different in certain ways) and I think newtype-uuid is a broadly useful crate.

We plan to keep newtype-uuid at v1 forever, adding new features to it in an append-only fashion.

Ideally we'd be able to use TypedUuid instances directly in Diesel structs. But as you're aware, Rust's orphan rules get in the way of that. I can think of four possible solutions to this, though I'm happy to hear of more ideas:

1. Make newtype-uuid implement Diesel's traits
2. Make Diesel support newtype-uuid, just like it supports uuid today.
3. Make another newtype, say DbTypedUuid<T>(pub TypedUuid<T>), which impls Diesel's traits.
4. Maintain a patchset on top of Diesel which supports newtype-uuid.

Going through each of the possible options:

• The biggest problem with 1 is that it is committal: it will not be possible to change this in the future without making a breaking change to either newtype-uuid or Diesel, to the best of my knowledge. Also, it means that none of Diesel's dependencies would ever be able to pull in newtype-uuid in the future -- the position of newtype-uuid "above" Diesel and its dependencies will be solidified forever.

  Logically, newtype-uuid is a thin wrapper around uuid, and so is "small" in a similar way, while Diesel is "large". It would generally be nice for "large" dependencies to pull in "small" ones rather than the other way round. None of newtype-uuid's dependencies are ever realistically going to pull in Diesel, but the other way round is quite possible.

• 2 is what I'm proposing here. The largest issue is just figuring out whether the crate is broadly useful enough, which is certainly a judgment call -- but one that I submit has a clear answer in this case.

• 3 is what we've been trying to do at Oxide so far and it's certainly possible, but has had some serious issues along the way. It ends up being quite awkward to convert back and forth. For example, it's not just structs that define DB records -- queries need to convert back and forth from DbTypedUuid to TypedUuid. It's ranged from mildly annoying to outright terrible in practice -- so much so that in a few spots we've given up on having typed UUIDs.

• 4 is definitely an option we've considered, but maintaining a patchset in perpetuity can be a lot of work. In addition to that, we'd want to use a [patch] directive to pull it in, so that we can leverage the rest of the Diesel ecosystem while making this append-only change. This means that it isn't possible to publish code that depends on this to crates.io. ([patch.crates-io] requires that the package be named the same -- there's an open issue for this).

So based on the above description, it would be nice if Diesel grew support for newtype-uuid. I'm happy to do the work here if this sounds good.

---

This, and other similar issues, have solidified my general opinion that the current way orphan rules play out in the ecosystem is, by some margin, the worst part of Rust. I have some thoughts on how to relax the orphan rules in a principled way that keeps API compat and avoids the failure mode of Haskell's {-# LANGUAGE UndecidableInstances #-}, and hope to publish and start advocating for them soon.

[weiznich]

Thanks for opening this discussion.

We usually use the following points to decide whether to add support for a certain rust <-> sql type mapping in diesel itself:

• The mapping should work in both directions
• There shouldn't be any value on one side that couldn't be represented on the other side
• There is a single canonical mapping
• Both the rust side and the sql side type should be used commonly

I think the first three points a fulfilled given that this seems to be just a wrapper around the uuid crate. That said I'm not sure that I would call a crate with ~1000 daily downloads as widely used (compared to the > 100.000 daily downloads for uuid).
Are you aware of any other project that use (or want to use) new type-uuid in combination with diesel?

I personally would tend to the first variant from your suggestion list. My main concern here is maintainer capacity. There is only that much code that I can maintaining the limited amount of time I have for this project and I would rather spend that code budget on widely used features.
Diesel is comparable to serde in that way (although it is slightly less popular) and I see that you already added a serde feature there. As for the stability argument: That applies in both directions. Even if we would add support in diesel that might break if you publish another major version of newtype-uuid (which might be required for a hypothetical uuid version 2). Now changing the major version on one side does not necessarily mean that you need to change it on the other side as well as long as the API you actually used did not change in an incompatible way.

I agree on the general observation that the orphan rule sometimes gets in your way, but I fear there is no easy solution to this problem. (As far as I'm aware other solutions do not have that specific limitation, but will result into other equally bad problems).

[sunshowers]

Thanks. I definitely understand the concern about supporting what at this point is a relatively small library (to be clear, I didn't claim it's widely used yet -- I said it is broadly useful). However it's a common pattern to use newtypes to distinguish different kinds of UUIDs and I know of at least one other place that has started using newtype-uuid specifically (though I don't know if it uses Diesel). Would there be a threshold at which you'd consider changing your opinion?

I'm just really uncomfortable with 1 for the reasons outlined in the OP. The circular dependency issue is real and it can show up at the most inopportune times, typically via a chain of optional dependencies. This even shows up with serde -- one example is rustwasm/wasm-bindgen#2770, which required a lot of work to fix and ended up with an extra copy of some code being made.

re orphan rules, I've been chatting with some people and I do believe we've come up with a pretty solid solution that avoids known pitfalls, and would completely decouple the circular dep issue from the question of who takes the burden of maintenance. Hope to write more about this soon.

[sunshowers]

Actually I realized that 1 is unworkable for us -- due to the way Cargo feature unification works, a lot of our non-Diesel-related code will end up pulling in Diesel as a dependency, because it lives in the same workspace as our Diesel code. It's either that or disabling hakari for much worse compile times :(

[sunshowers]

OK, I think we're going to stick with 3 for now, maybe consider 4 if 3 gets too annoying, and see where it goes from there. Thanks so much for your response!

[weiznich]

I just had another look at this and I think I can outline at least a partial solution 5. It is possible to implement a third party trait for a third party type in your downstream crate if the trait is generic and the first generic argument of your trait is a local type. (See the re-rebalance coherence RFC for details). That change was original motivated by diesel needs, although for different traits.

We wan use that to implement deserialization support for third party rust types in a downstream crate by defining our own Sql type. (This might be something that's generally useful for your use-case with newtype-uuid as that would allow to enforce that you only can insert a uuid for your user type into the user table (by having different sql types for different tables). On the other hand it does require change the generated schema.rs file. I've put together a small demonstration here, that just does this for the uuid crate and the sqlite backend (which doesn't support the uuid types out of the box).

The largest downside of this approach is that you are not able to write the necessary trait impl for serialization that way as you run into the conflicting implementations error there. I'm not user if that can be fixed, but I'm open to accept improvements on that side.