Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
Mar 16, 2022
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1c8f8d9
alternate impl_enum_type that owns the enum itself (compiles!)
david-crespo Mar 16, 2022
455f645
now move VpcRouter and VpcRouterKind into views (compiles, tests pass)
david-crespo Mar 16, 2022
127d890
convert DatasetKind, very simple
david-crespo Mar 16, 2022
90c8777
Merge branch 'main' into uncommon-views
david-crespo Mar 16, 2022
636c72d
Into -> From because it's better!
david-crespo Mar 16, 2022
a51793a
macro rename and comments so I can merge the intermediate state
david-crespo Mar 16, 2022
8e2cc6f
Merge main into uncommon-views
david-crespo Mar 16, 2022
12100ed
make diff slightly nicer
david-crespo Mar 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 0 additions & 21 deletions common/src/api/external/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1244,27 +1244,6 @@ impl FromStr for IpNet {
}
}

#[derive(Clone, Copy, Debug, Deserialize, Serialize, PartialEq, JsonSchema)]
#[serde(rename_all = "snake_case")]
pub enum VpcRouterKind {
System,
Custom,
}

/// A VPC router defines a series of rules that indicate where traffic
/// should be sent depending on its destination.
#[derive(ObjectIdentity, Clone, Debug, Deserialize, Serialize, JsonSchema)]
pub struct VpcRouter {
/// common identifying metadata
#[serde(flatten)]
pub identity: IdentityMetadata,

pub kind: VpcRouterKind,

/// The VPC to which the router belongs.
pub vpc_id: Uuid,
}

/// A `RouteTarget` describes the possible locations that traffic matching a
/// route destination can be sent.
#[derive(
Expand Down
86 changes: 66 additions & 20 deletions nexus/src/db/model.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,67 @@ macro_rules! impl_enum_type {
}
}

macro_rules! impl_enum_type2 {
(
$(#[$enum_meta:meta])*
pub struct $diesel_type:ident;

$(#[$model_meta:meta])*
pub enum $model_type:ident;
$($enum_item:ident => $sql_value:literal)+
) => {

$(#[$enum_meta])*
pub struct $diesel_type;

$(#[$model_meta])*
pub enum $model_type {
$(
$enum_item,
)*
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);


impl<DB> ToSql<$diesel_type, DB> for $model_type
where
DB: Backend,
{
fn to_sql<W: std::io::Write>(
&self,
out: &mut serialize::Output<W, DB>,
) -> serialize::Result {
match self {
$(
$model_type::$enum_item => {
out.write_all($sql_value)?
}
)*
}
Ok(IsNull::No)
}
}

impl<DB> FromSql<$diesel_type, DB> for $model_type
where
DB: Backend + for<'a> BinaryRawValue<'a>,
{
fn from_sql(bytes: RawValue<DB>) -> deserialize::Result<Self> {
match DB::as_bytes(bytes) {
$(
$sql_value => {
Ok($model_type::$enum_item)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

}
)*
_ => {
Err(concat!("Unrecognized enum variant for ",
stringify!{$model_type})
.into())
}
}
}
}
}
}

Copy link
Contributor Author

@david-crespo david-crespo Mar 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
                     }
                     )*
                     _ => {

/// Newtype wrapper around [external::Name].
#[derive(
Clone,
Expand Down Expand Up @@ -1584,14 +1645,14 @@ impl From<params::VpcSubnetUpdate> for VpcSubnetUpdate {
}
}

impl_enum_type!(
impl_enum_type2!(
#[derive(SqlType, Debug)]
#[postgres(type_name = "vpc_router_kind", type_schema = "public")]
pub struct VpcRouterKindEnum;

#[derive(Clone, Debug, AsExpression, FromSqlRow)]
#[derive(Clone, Copy, Debug, AsExpression, FromSqlRow, PartialEq)]
#[sql_type = "VpcRouterKindEnum"]
pub struct VpcRouterKind(pub external::VpcRouterKind);
pub enum VpcRouterKind;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Enum values
System => b"system"
Expand All @@ -1613,16 +1674,11 @@ impl VpcRouter {
pub fn new(
router_id: Uuid,
vpc_id: Uuid,
kind: external::VpcRouterKind,
kind: VpcRouterKind,
params: params::VpcRouterCreate,
) -> Self {
let identity = VpcRouterIdentity::new(router_id, params.identity);
Self {
identity,
vpc_id,
kind: VpcRouterKind(kind),
rcgen: Generation::new(),
}
Self { identity, vpc_id, kind, rcgen: Generation::new() }
}
}

Expand All @@ -1633,16 +1689,6 @@ impl DatastoreCollection<RouterRoute> for VpcRouter {
type CollectionIdColumn = router_route::dsl::router_id;
}

impl Into<external::VpcRouter> for VpcRouter {
fn into(self) -> external::VpcRouter {
external::VpcRouter {
identity: self.identity(),
vpc_id: self.vpc_id,
kind: self.kind.0,
}
}
}

#[derive(AsChangeset)]
#[table_name = "vpc_router"]
pub struct VpcRouterUpdate {
Expand Down
7 changes: 3 additions & 4 deletions nexus/src/external_api/http_entrypoints.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ use crate::ServerContext;
use super::{
console_api, params,
views::{
Organization, Project, Rack, Role, Sled, Snapshot, User, Vpc, VpcSubnet,
Organization, Project, Rack, Role, Sled, Snapshot, User, Vpc,
VpcRouter, VpcSubnet,
},
};
use crate::context::OpContext;
Expand Down Expand Up @@ -59,8 +60,6 @@ use omicron_common::api::external::RouterRouteUpdateParams;
use omicron_common::api::external::Saga;
use omicron_common::api::external::VpcFirewallRuleUpdateParams;
use omicron_common::api::external::VpcFirewallRules;
use omicron_common::api::external::VpcRouter;
use omicron_common::api::external::VpcRouterKind;
use ref_cast::RefCast;
use schemars::JsonSchema;
use serde::Deserialize;
Expand Down Expand Up @@ -1973,7 +1972,7 @@ async fn vpc_routers_post(
&path.organization_name,
&path.project_name,
&path.vpc_name,
&VpcRouterKind::Custom,
&db::model::VpcRouterKind::Custom,
&create_params.into_inner(),
)
.await?;
Expand Down
4 changes: 2 additions & 2 deletions nexus/src/external_api/params.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,14 +222,14 @@ pub struct VpcSubnetUpdate {
* VPC ROUTERS
*/

/// Create-time parameters for a [`VpcRouter`](omicron_common::api::external::VpcRouter)
/// Create-time parameters for a [`VpcRouter`](crate::external_api::views::VpcRouter)
#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
pub struct VpcRouterCreate {
#[serde(flatten)]
pub identity: IdentityMetadataCreateParams,
}

/// Updateable properties of a [`VpcRouter`](omicron_common::api::external::VpcRouter)
/// Updateable properties of a [`VpcRouter`](crate::external_api::views::VpcRouter)
#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
pub struct VpcRouterUpdate {
#[serde(flatten)]
Expand Down
40 changes: 40 additions & 0 deletions nexus/src/external_api/views.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,46 @@ impl Into<VpcSubnet> for model::VpcSubnet {
}
}

#[derive(Clone, Copy, Debug, Deserialize, Serialize, PartialEq, JsonSchema)]
#[serde(rename_all = "snake_case")]
pub enum VpcRouterKind {
System,
Custom,
}

impl Into<VpcRouterKind> for model::VpcRouterKind {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, definitely. We should change them all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

fn into(self) -> VpcRouterKind {
match self {
model::VpcRouterKind::Custom => VpcRouterKind::Custom,
model::VpcRouterKind::System => VpcRouterKind::System,
}
}
}

/// A VPC router defines a series of rules that indicate where traffic
/// should be sent depending on its destination.
#[derive(ObjectIdentity, Clone, Debug, Deserialize, Serialize, JsonSchema)]
pub struct VpcRouter {
/// common identifying metadata
#[serde(flatten)]
pub identity: IdentityMetadata,

pub kind: VpcRouterKind,

/// The VPC to which the router belongs.
pub vpc_id: Uuid,
}

impl Into<VpcRouter> for model::VpcRouter {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above

fn into(self) -> VpcRouter {
VpcRouter {
identity: self.identity(),
vpc_id: self.vpc_id,
kind: self.kind.into(),
}
}
}

/*
* RACKS
*/
Expand Down
7 changes: 3 additions & 4 deletions nexus/src/nexus.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ use omicron_common::api::external::RouterRouteKind;
use omicron_common::api::external::RouterRouteUpdateParams;
use omicron_common::api::external::UpdateResult;
use omicron_common::api::external::VpcFirewallRuleUpdateParams;
use omicron_common::api::external::VpcRouterKind;
use omicron_common::api::internal::nexus;
use omicron_common::api::internal::nexus::DiskRuntimeState;
use omicron_common::api::internal::nexus::UpdateArtifact;
Expand Down Expand Up @@ -1778,7 +1777,7 @@ impl Nexus {
let router = db::model::VpcRouter::new(
system_router_id,
vpc_id,
VpcRouterKind::System,
db::model::VpcRouterKind::System,
params::VpcRouterCreate {
identity: IdentityMetadataCreateParams {
name: "system".parse().unwrap(),
Expand Down Expand Up @@ -2301,7 +2300,7 @@ impl Nexus {
organization_name: &Name,
project_name: &Name,
vpc_name: &Name,
kind: &VpcRouterKind,
kind: &db::model::VpcRouterKind,
params: &params::VpcRouterCreate,
) -> CreateResult<db::model::VpcRouter> {
let vpc = self
Expand Down Expand Up @@ -2332,7 +2331,7 @@ impl Nexus {
router_name,
)
.await?;
if router.kind.0 == VpcRouterKind::System {
if router.kind == db::model::VpcRouterKind::System {
return Err(Error::MethodNotAllowed {
internal_message: "Cannot delete system router".to_string(),
});
Expand Down
5 changes: 3 additions & 2 deletions nexus/test-utils/src/resource_helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,11 @@ use omicron_common::api::external::Disk;
use omicron_common::api::external::IdentityMetadataCreateParams;
use omicron_common::api::external::Instance;
use omicron_common::api::external::InstanceCpuCount;
use omicron_common::api::external::VpcRouter;
use omicron_nexus::crucible_agent_client::types::State as RegionState;
use omicron_nexus::external_api::params;
use omicron_nexus::external_api::views::{Organization, Project, Vpc};
use omicron_nexus::external_api::views::{
Organization, Project, Vpc, VpcRouter,
};
use omicron_sled_agent::sim::SledAgent;
use std::sync::Arc;
use uuid::Uuid;
Expand Down
3 changes: 1 addition & 2 deletions nexus/tests/integration_tests/vpc_routers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,8 @@ use http::method::Method;
use http::StatusCode;
use omicron_common::api::external::IdentityMetadataCreateParams;
use omicron_common::api::external::IdentityMetadataUpdateParams;
use omicron_common::api::external::VpcRouter;
use omicron_common::api::external::VpcRouterKind;
use omicron_nexus::external_api::params;
use omicron_nexus::external_api::views::{VpcRouter, VpcRouterKind};

use dropshot::test_util::object_get;
use dropshot::test_util::objects_list_page;
Expand Down
4 changes: 2 additions & 2 deletions openapi/nexus.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6666,7 +6666,7 @@
]
},
"VpcRouterCreate": {
"description": "Create-time parameters for a [`VpcRouter`](omicron_common::api::external::VpcRouter)",
"description": "Create-time parameters for a [`VpcRouter`](crate::external_api::views::VpcRouter)",
"type": "object",
"properties": {
"description": {
Expand Down Expand Up @@ -6710,7 +6710,7 @@
]
},
"VpcRouterUpdate": {
"description": "Updateable properties of a [`VpcRouter`](omicron_common::api::external::VpcRouter)",
"description": "Updateable properties of a [`VpcRouter`](crate::external_api::views::VpcRouter)",
"type": "object",
"properties": {
"description": {
Expand Down