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.

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

nexus crate shattering 1 of 3: Extract a subset of internal_api/external_api types #1476

Merged
merged 3 commits into from
Jul 25, 2022

Conversation

jgallagher
Copy link
Contributor

This is "nexus crate shattering" PR 1 of 3 leading toward extracting the database model into its own crate. This PR moves nexus::external_api::{views, params, shared}, nexus::internal_api::params, and nexus::db::identity into a new nexus-types crate.

Other minor changes I made to make this work:

  • nexus::cidata::MAX_USER_DATA_BYTES moved to live alongside external_api::params::InstanceCreate (where it's needed to deserialize user_data)
  • Conversion implementations (primarily of the impl From<db::model::T> for <params::T> variety) moved into the relevant db::model::* submodule instead of moving to nexus-types (which doesn't depend on the db model and therefore can't hold the conversion impls).
  • DeviceAuthResponse::from_model() became DeviceAuthRequest::into_response() (for similar reasons as moving the From impls).
  • The derived Resource method name() now returns an omicron_common::api::external::Name instead of a db::model::Name. The latter is a newtype of the former, so this mostly required either adding or removing a .0 in a handful of places.

@jgallagher jgallagher changed the title nexus crate shattering 1 of 3: Extract a subset of internal_api/external_api typtes nexus crate shattering 1 of 3: Extract a subset of internal_api/external_api types Jul 25, 2022
Copy link
Collaborator

@davepacheco davepacheco left a comment

Choose a reason for hiding this comment

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

This looks good to me.

I'm a little on the fence about the fact that db::model knows about views. We discussed this a bit offline and noted:

  • The database layer needs to know about params in many cases because it's not necessarily possible (or desirable) for a consumer to construct a full db::model type for whatever operation it's doing.
  • On the other hand, It doesn't seem like the database layer intrinsically needs to know about views. I think consumers always get back db::model types, not views types, and they can have whatever view(s) they want.
  • The shared types throw a wrench into this. They exist because for some types (especially low-level primitive types), the input and output types are exactly the same. With two different types, we'd probably want some mechanism to ensure they stay in sync and probably some conversion functions. This increasingly feels wrong for things that are simple extensions of primitive types (e.g., Name or IP address). It's not clear to me how to draw the line between "basically a primitive type [that we're okay using for both input and output]" and "something that must be either an input or an output".
  • An alternative might be to keep view-only types out of both nexus-types and db::model. It would take some work even just to see if this would work. It would also make them weirdly separate from params and shared. It's not clear this would be really better.

I'm definitely past the point of caring either way on this and I'm happy to defer to you and @smklein here.

@@ -0,0 +1,7 @@
// This Source Code Form is subject to the terms of the Mozilla Public
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it'd help to have some module-level docs for nexus::types to explain what goes here and why.

@david-crespo
Copy link
Contributor

Dave's comments about the shared types get at why we were not able to complete the extraction from omicron_common. So annoying. I agree about models not knowing about views. Technically it doesn't really matter and it's just about where the impl From lives, but conceptually I do like thinking of the view layer as knowing about models but models not knowing about views.

@jgallagher jgallagher force-pushed the shatter-nexus-types branch from 91a79a4 to 008b21b Compare July 25, 2022 18:08
@jgallagher
Copy link
Contributor Author

I added a doc comment in 008b21b that tries to both summarize what types should be in this crate and why views is present. The TL;DR is that largely due to the existence of shared, I think leaving views here is the most reasonable option, even though it does have the tradeoff of leaving conversion impls between model and view in the model layer.

Copy link
Collaborator

@smklein smklein left a comment

Choose a reason for hiding this comment

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

IMO having the models be aware of views is kinda a bummer, but IMO if it's limited to conversion functions, it's not the worst thing in the world. The most important reason to separate the two is that "models should be able to evolve independently from the API", and that continues to be true.

It does sorta seem like orphan rules force our hand here - either views need to know about models, or models need to know about views, in order to implement the conversion functions.

@jgallagher jgallagher merged commit 2e8d674 into main Jul 25, 2022
@jgallagher jgallagher deleted the shatter-nexus-types branch July 25, 2022 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants