oxidecomputer / omicron

Omicron: Oxide control plane
Mozilla Public License 2.0
252 stars 40 forks source link

[type safety] Strongly typed UUIDs #745

Open smklein opened 2 years ago

smklein commented 2 years ago

We use UUIDs for everything in Omicron - identifying internal objects, external objects, and probably just for fun in a couple other spots too.

These all use the uuid::Uuid structure, which is functionally sufficient, but less safe than it could be. For example, in PRs like https://github.com/oxidecomputer/omicron/pull/741 , where a "Disk ID" was replaced by a "Volume ID", the underlying types are both UUIDs, but they're fundamentally different.

Building a simple "newtype" wrapper around a UUID for each object would prevent this misuse, along the lines of:

#[derive(Clone, Copy, Serialize, Deserialize, Debug)]
struct DiskID(pub uuid::Uuid);

impl std::ops::Deref for DiskID {
  type Target = uuid::Uuid;
  fn deref(&self) -> &Self::Target { &self.0 }
}

#[derive(Clone, Copy, Serialize, Deserialize, Debug)]
struct VolumeID(pub uuid::Uuid);

impl std::ops::Deref for VolumeID {
  type Target = uuid::Uuid;
  fn deref(&self) -> &Self::Target { &self.0 }
}

These structs would be usable wherever a standard uuid::Uuid object could be used, but would avoid being interchangeable with each other.

leftwo commented 2 years ago

If we do this, then Crucible should have it as well, as it also has the uuid::Uuid.

davepacheco commented 2 years ago

I'd definitely like this. It'd also be interesting to consider serializing them with prefixes, similar to AWS (where instances start with "i-", etc.)

It feels like this will have a huge blast radius.

(edit: remove my question that was answered in the description! I need to dig deeper into Deref.)

jgallagher commented 2 years ago

I did this just yesterday in MGS work. I didn't implement Deref, but I did have to manually implement slog::Value. I wonder if I could've avoided that via Deref instead?

smklein commented 2 years ago

We chatted about this a bit internally - although the newtype pattern would still definitely be useful here, it sounds like "explicit conversions to uuid::Uuid" may be preferable over abusing deref coercion.

jclulow commented 2 years ago

In buildomat I've been experimenting with a macro for stamping out ID newtypes and it's been a good experience. Implementing ToSql/FromSql allows me to drop them straight in the model structs. I don't think I deref them to the internal Ulid.