PoC for moving enums from common -> Nexus db::model

[david-crespo]

After @smklein revived the dream of #388 (moving as much as possible out of common and into the appropriate service) in #774, I wanted to see what the remaining obstacles were. Unfortunately we've moved almost all the easy stuff — everything left is kind of a pain. I want to get consensus on an approach before putting up a 1000 line PR.

A bunch of important enums like VpcRouterKind (system or custom) are defined in common and then wrapped in a struct in Nexus's model.rs. The models depend on the views/params, which seems to me backwards. Instead, the enum should be defined in the model file, and then the view re-implements the enum with a dumb boilerplate Into to convert between them. Theoretically you could use the model enum directly in the view, but I don't think it's a good idea to blur the line between models and views — that's precisely what we're trying to stop doing with these changes.

The annoying part for the purposes of this PR is that the impl_enum_type! macro needs to be changed (here copied with modification to avoid changing everything at once) to make pub enum VpcRouterKind instead of pub struct VpcRouterKind(pub external::VpcRouterKind). The rest of the changes very straightforward fall out of that.

Possible fanciness

There's a diesel-derive-enum proc macro that would let us do this

#[derive(DbEnum)]
pub enum VpcRouterKind {
    System,
    Custom,
}

and basically be done with it. I feel this is a lot nicer than what we have, but I couldn't get it to work (though I didn't try very hard). I think if we wanted to go this route we'd probably be better off making our own version of that macro that does exactly what we want and nothing more.

[david-crespo]

This is the analogous line in the original. Instead of defining the enum, it wrapped the imported external one.

pub struct $model_type(pub $ext_type);

[david-crespo]

The original did this, i.e., Ok(VpcRouterKind(external::VpcRouterKind::Custom)) as opposed to the simple Ok(VpcRouterKind::Custom).

Ok($model_type(<$ext_type>::$enum_item))

[david-crespo]

Complete diff with the existing macro.

@@ -50,7 +50,7 @@ macro_rules! impl_enum_type {
         pub struct $diesel_type:ident;
 
         $(#[$model_meta:meta])*
-        pub struct $model_type:ident(pub $ext_type:ty);
+        pub enum $model_type:ident;
         $($enum_item:ident => $sql_value:literal)+
     ) => {
 
@@ -58,7 +58,11 @@ macro_rules! impl_enum_type {
         pub struct $diesel_type;
 
         $(#[$model_meta])*
-        pub struct $model_type(pub $ext_type);
+        pub enum $model_type {
+            $(
+                $enum_item,
+            )*
+        }
 
         impl<DB> ToSql<$diesel_type, DB> for $model_type
         where
@@ -68,9 +72,9 @@ macro_rules! impl_enum_type {
                 &self,
                 out: &mut serialize::Output<W, DB>,
             ) -> serialize::Result {
-                match self.0 {
+                match self {
                     $(
-                    <$ext_type>::$enum_item => {
+                    $model_type::$enum_item => {
                         out.write_all($sql_value)?
                     }
                     )*
@@ -87,7 +91,7 @@ macro_rules! impl_enum_type {
                 match DB::as_bytes(bytes) {
                     $(
                     $sql_value => {
-                        Ok($model_type(<$ext_type>::$enum_item))
+                        Ok($model_type::$enum_item)
                     }
                     )*
                     _ => {

[smklein]

This direction looks right to me. I share the intuition that "conversion functions should, when possible, live alongside the view/params".

I think the distinction of "where is the enum defined" to be kinda arbitrary (is the "real" one in models and the derivative one in views, or vice versa) but your approach of making them both "just enums/structures, that can be converted to one another" seems balanced.

I think the original intent of having "the structure in one layer be a newtype wrapper around the other" was just to do what we could to reduce boilerplate, but there are other ways of accomplishing that goal. The confusion caused by the nesting probably wasn't worth the LoC reduction.

[smklein]

I do like the idea of this thing actually being an enum now; that's way more intuitive

[smklein]

Is it possible to impl From<model::VpcRouterKind> for VpcRouterKind?

I think it's functionally identical to this, but it's preferable to impl From over Into when given the option:

One should always prefer implementing From over Into because implementing From automatically provides one with an implementation of Into thanks to the blanket implementation in the standard library.
Only implement Into when targeting a version prior to Rust 1.41 and converting to a type outside the current crate. From was not able to do these types of conversions in earlier versions because of Rust’s orphaning rules.

[david-crespo]

Yep, definitely. We should change them all.

[david-crespo]

Done in 636c72d. I'll change the others in a separate PR to reduce noise.

[smklein]

Same comment as above

[david-crespo]

I share the intuition that "conversion functions should, when possible, live alongside the view/params".

Generally I agree, though so far we've been following a slightly more nuanced pattern where conversion from model to view is defined next to the view, but the conversion from params to model is defined with the model because

1. It's usually the model constructor
2. I think of views as "knowing about" models because they're lenses onto them, but params don't "know about" models because they're really just HTTP request bodies, they don't care what they are turned into

[david-crespo]

Decided to tweak names so this could be merged as-is without having to convert all the enums affected by this macro, which it turns out would be a very complex undertaking.