Structure for moving views and params structs out of common #374

david-crespo · 2021-11-09T04:10:49Z

Now that we have client generation for communication between the apps inside omicron (#369) we no longer need to put Nexus view and params structs in common so the clients can use them to understand Nexus responses. I call them views here (and in this change) even though we have not actually called them that in the code so far because I think that is what they are: public lenses on the DB models.

We now have this directory structure inside Nexus:

nexus/src/external_api
├── http_entrypoints.rs
├── mod.rs
├── params.rs
└── views.rs
nexus/src/internal_api
├── http_entrypoints.rs
├── mod.rs
└── params.rs

The point is to represent:

a) the distinction between views and params, and
b) the fundamental association of views and params with external or internal endpoints.

The overall picture is this:

params are the public interface for routes (i.e., they deserialize POST bodies)
params get passed along to nexus (I think of this as the "service" layer)
in nexus, params are turned into changesets via into and models via model constructors
models and changesets are the arguments to datastore functions called by nexus
datastore functions return models to nexus
nexus returns models to route handlers
route handlers turn models into views for serialization

request --Params--> handler --Params--> nexus --Changeset/Model
                                                       |
                                                   datastore 
                                                       |
response  <--View-- handler  <--Model-- nexus      <--Model

Even though this will not result in net fewer lines of code (it should be about the same overall), I think this is much easier to understand. The structs that define the input and output format of Nexus's endpoints should be owned by Nexus.

Note that I had to revert #288, which moved api_identity out of its own crate and into common, in order to more easily import it into Nexus. At least I think I had to, I may have misinterpreted the errors I was getting.

There's good discussion on my last attempt at this in #191.

What's here and what's next

I only did Organization and Project related views and params on the external side and OximeterInfo and SledAgentStartupInfo params on the internal side.

I don't think I want to move everything in a single PR, but rather set up the structure and then move the rest in subsequent PRs. With the structure in place, people can make new views and params in the right spot to begin with. There are some complicated ones like instance and disk runtime states that are pretty tightly integrated in both nexus and sled agent, but the principle to follow is that they belong in whichever app defines the relevant API endpoints, and the client will use the type from the generated client.

david-crespo · 2021-11-09T04:14:06Z

nexus/src/params.rs

+pub struct OrganizationUpdate {
+    #[serde(flatten)]
+    pub identity: IdentityMetadataUpdateParams,
+}


Separating params from views feels bad because you can't visually compare the fields anymore, but I think that colocation was actually misleading. Params are, if anything, much more closely related to the models than the views — they are the inputs for creating models or changesets. There’s no necessary relationship between the params and views. By putting them in their own files, we can import params into the models file and know views are not involved.

On top of that you get the nice ergonomic name params::OrganizationCreate.

The OpenAPI spec test failure reminds me that these names go right into the spec at top level, which means we might be stuck with calling these OrganizationCreateParams and OrganizationUpdateParams unless we don't mind *Create and *Update being the convention for params.

Params are, if anything, much more closely related to the models than the views — they are the inputs for creating models or changesets.

My only real question about this change is that I don't really see why that should be true. I can't off the top of my head think of interesting cases where the view would likely differ from the model. But if, say, we translate some field of the model into something different in the view, I think I'd expect a corresponding translation on the create/update side as well.

That said, this is in the bucket of things I'd like to get more experience with before spending a lot of time trying to get right without a concrete example in hand. So I'm fine with doing whatever we want here assuming we can always change it later.

Broadly I think that's right — params, models, and views will all be pretty close in structure — but the chain of relationships goes params -> model -> view. All I'm saying is there's no strong reason to colocate views and params in the same file, since conceptually models sit in the middle there. We could also do it, but I like having a file with just params.

david-crespo · 2021-11-09T04:31:50Z

nexus/src/views/external.rs

+    fn into(self) -> Organization {
+        Organization { identity: self.identity() }
+    }
+}


I prefer defining this next to the view rather than the model. The model does not care if it has a view or not.

smklein

Some comments below - definitely like the direction of moving Nexus-specific bits out of common, and into Nexus. I'm hopeful this'll continue to make the codebase easier to grok.

smklein · 2021-11-11T12:58:33Z

nexus/src/params.rs

@@ -0,0 +1,57 @@
+/*!


Any reason why this isn't part of the external_api/ subdirectory, alongside views? My mental model for that subdirectory was "does this structure appear in the inputs or outputs of a dropshot endpoint", and params fit that bill. Frankly, from my perspective, I figured this was enough to make them "views", but I hear you on wanting to separate model imports from views as much as possible.

It should go in external_api, you're right (see other comment about my commit in progress) because params are the post bodies of endpoints. I was trying to keep that really clean single use crate::params import, but oh well.

The way I think of views vs. params is that views are functions from the private representation to the public representation, and params go in the other direction: they're the public representation we get that we turn into a private representation.

This is kind of interesting and it's been tripping me up while I decide how to organize this — we actually have two roles for params structs at present. One is defining post bodies, the other is as the input to the model constructor (create params, example) or a changeset constructor (update params, example). I think I would like to get *Params out of datastore completely by instead calling the model or changeset constructors in nexus.rs and passing the result into the datastore functions.

The main thing that's been holding me up from wrapping up this PR is figuring out the right order to make these mostly-orthogonal changes in so I don't have a 4000 line PR that's impossible to review. I also want to workshop the approach before committing.

made a terrible diagram

request --Params--> handler --Params--> nexus --Changeset/Model | datastore | response <--View-- handler <--Model-- nexus <--Model

smklein · 2021-11-11T13:05:34Z

nexus/src/params.rs

+}
+
+/*
+ * PROJECTS


Some day, it would be nice to get to a point where we can represent something like:

nexus/src/db/model/organization.rs nexus/src/db/model/project.rs nexus/src/external_api/view/organization.rs nexus/src/external_api/view/project.rs nexus/src/external_api/param/organization.rs nexus/src/external_api/param/project.rs ... (etc)

Doesn't need to happen now though.

Yep, I was thinking about doing it almost exactly that way. I have a commit I'm working on that looks like this:

├── external_api │ ├── http_entrypoints.rs │ ├── mod.rs │ ├── params.rs │ └── views.rs ├── internal_api │ ├── http_entrypoints.rs │ └── mod.rs │ ├── params.rs │ └── views.rs

We certainly could. I didn't put a ton of thought into it, but I thought of a few ways you could do it and couldn't choose:

nexus/src/external_api ├── projects │ ├── http_entrypoints.rs │ ├── params.rs │ └── views.rs └── organizations ├── http_entrypoints.rs ├── params.rs └── views.rs

nexus/src/external_api ├── projects.rs <-- contains handlers, views, and params └── organizations.rs

nexus/src/external_api ├── http_entrypoints │ ├── project.rs │ └── organization.rs ├── params │ ├── project.rs │ └── organization.rs └── views ├── project.rs └── organization.rs

The last one is the closest to Sean's suggestion.

One small advantage of the way I have it in this PR is that importing the params module and using particular ones is clean. params::ProjectCreate vs. params::project::ProjectCreate. But that's not a very high priority. It really doesn't matter if we have a big list of explicit params struct imports at the top of db/model.rs.

If we also break down model.rs then that somewhat reduces the import problem.

My preference is 2, 1, 3 respectively.

It does feel natural to organize first by resource and second by controllers, views, params, models, but I have a bad feeling about it because it is generally not how it is done by full-stack frameworks like Rails, Django, Laravel, Phoenix, etc., and I suspect there's a good reason for that. The main one I can think of is testing. When you have all your models together, you also have all your model tests together and you can think of it as a single layer of the call stack. Another reason is that while request and response bodies mostly correspond one-to-one with resources, that is not always going to be the case, e.g., login params, and there are probably other examples.

We definitely still want to group the models together.

So src/db/models/organization.rs, etc. We should preserve the boundary between views/etc and model for sure.

As for things in external_api I think of it less as organized by resource and more as organized by concept. Most concepts will have a consistent pattern of route, params, view but some may have a less complete set.

It's possible that we could grow out of this, but in the short term it means when we make a change, to organizations for example, we're doing so in mostly one spot (without having a giant file with everything in it).

smklein · 2021-11-11T13:06:29Z

openapi/nexus.json

@@ -231,7 +231,7 @@
          "content": {
            "application/json": {
              "schema": {
-                "$ref": "#/components/schemas/OrganizationCreateParams"


The generated names definitely look nicer!

smklein · 2021-11-11T13:11:09Z

nexus/src/external_api/views.rs

+/**
+ * Client view of an [`Organization`]
+ */
+#[derive(ObjectIdentity, Clone, Debug, Deserialize, Serialize, JsonSchema)]


This whole api_identity kerfufle is funny (it provides ObjectIdentity) - the proc macro only generates a helper function for accessing identity as a non-mut reference...

... but identity is pub anyway, so it doesn't seem necessary? Anywhere we call organization.identity() we could just call organization.identity.

Anyway. I guess I see why it was necessary to revert the change that tried to make api_identity more isolated, but frankly I'm not sure that macro needs to exist.

I was trying to leave the code as little changed as possible, but it could be we don't need it! I'll try it.

I tried and it looks like it would be a bit of work to rip it out, so we should think about it separately.

…ron-common (#288)" This reverts commit ec737d3.

david-crespo · 2021-11-12T21:11:34Z

oximeter/collector/src/lib.rs

-                .json(&OximeterInfo {
-                    address: server.local_addr(),
+                .json(&nexus_client::types::OximeterInfo {
+                    address: server.local_addr().to_string(),


this is the dream. use the generated client type!

david-crespo · 2021-11-12T21:13:18Z

nexus/src/db/model.rs

@@ -6,6 +6,8 @@ use crate::db::schema::{
    console_session, disk, instance, metric_producer, network_interface,
    organization, oximeter, project, rack, sled, vpc, vpc_router, vpc_subnet,
 };
+use crate::external_api::params;
+use crate::internal_api;


this asymmetry is a little janky, but there are so few internal ones that it kind of works. could do external_params and internal_params, but again the vast majority are external so would be unnecessarily verbose

I don't think it's that bad. In particular having to be explicit about internal params is definitely good. We really do want to see that from the call site.

david-crespo · 2021-11-12T21:19:53Z

I've updated the PR description and this now represents my considered opinion about what to do.

ahl · 2021-11-17T22:39:18Z

nexus/src/external_api/mod.rs

+pub mod params;
+pub mod views;


why pull these apart?

In the way I picture it, models know about params (they are the arguments to the model constructor) but not views (views are functions from model to serializable form). So in the models file we can import the params and refer to them as, e.g., params::ProjectCreate and not know anything about views. Obviously we could have them all in the same file and import only the ones we want, but the simplicity of the import exhibits the relationship. Even in the entrypoints file, it's cute to be able to do params::ProjectCreate.

Also discussed here and here.

Oh yeah, the PR description gives a more long-winded version of the picture. I wrote that last, so it's the most polished.

ahl · 2021-11-17T22:42:27Z

common/src/api/internal/nexus.rs

@@ -45,13 +45,6 @@ pub struct InstanceRuntimeState {
    pub time_updated: DateTime<Utc>,
 }



I think the goal should be to move all this content out of common and into nexus, is that right?

yes. but it's a big, ugly diff

ahl · 2021-11-17T22:43:25Z

nexus/src/external_api/http_entrypoints.rs

@@ -74,7 +70,7 @@ type NexusApiDescription = ApiDescription<Arc<ServerContext>>;
 /**
 * Returns a description of the external nexus API
 */
-pub fn external_api() -> NexusApiDescription {
+pub fn api() -> NexusApiDescription {


I'd leave this as is for clarity, but I don't feel that strongly

That was driven 90% by my feeling that this was weird:

use external_api::http_entrypoints::external_api; use internal_api::http_entrypoints::internal_api;

but it's not like this is any better

use external_api::http_entrypoints::api as external_api; use internal_api::http_entrypoints::api as internal_api;

I'll change it back.

smklein

Looks good to me, I'm definitely happy with this migration of things outside of common.

david-crespo commented Nov 9, 2021

View reviewed changes

david-crespo requested review from smklein, davepacheco and ahl November 9, 2021 04:17

david-crespo commented Nov 9, 2021

View reviewed changes

Base automatically changed from openapi to main November 10, 2021 17:41

david-crespo force-pushed the views-and-params branch 2 times, most recently from 5584286 to c07c97a Compare November 10, 2021 19:08

david-crespo mentioned this pull request Nov 10, 2021

Add .../routers/{router_name}/routes and friends #359

Merged

smklein reviewed Nov 11, 2021

View reviewed changes

david-crespo mentioned this pull request Nov 11, 2021

Use models/changesets, not *Params, as datastore function args #383

Merged

david-crespo added 5 commits November 12, 2021 13:21

Revert "Relocate 'api_identity' derive macro - it's a helper for omic…

4b4e932

…ron-common (#288)" This reverts commit ec737d3.

create external_api and internal_api modules, put entrypoints inside

858281e

pull project and org views and params out of common

0914192

do an example of internal params

818041b

update openapi spec fixture

665a9eb

david-crespo force-pushed the views-and-params branch from c07c97a to 665a9eb Compare November 12, 2021 20:12

david-crespo commented Nov 12, 2021

View reviewed changes

david-crespo marked this pull request as ready for review November 12, 2021 21:19

comment symmetry on params files

02e0afe

david-crespo requested review from smklein and zephraph November 12, 2021 21:35

david-crespo changed the title ~~Move views and params structs out of common~~ Structure for moving views and params structs out of common Nov 12, 2021

david-crespo removed request for davepacheco and ahl November 13, 2021 03:48

david-crespo mentioned this pull request Nov 14, 2021

Tracking issue: move views and params out of common #388

Closed

9 tasks

Merge main into views-and-params

ff05c28

ahl reviewed Nov 17, 2021

View reviewed changes

smklein approved these changes Nov 17, 2021

View reviewed changes

smklein mentioned this pull request Nov 17, 2021

Sled agent views/params shuffling #386

Merged

david-crespo and others added 3 commits November 17, 2021 21:39

update comment

2fc08d4

Merge branch 'main' into views-and-params

02d17ac

address comments

9da2fd7

david-crespo merged commit d93e35f into main Nov 18, 2021

david-crespo deleted the views-and-params branch November 18, 2021 04:18

david-crespo mentioned this pull request Oct 16, 2023

network API re-org trial #4276

Closed

		@@ -45,13 +45,6 @@ pub struct InstanceRuntimeState {
		pub time_updated: DateTime<Utc>,
		}

Structure for moving views and params structs out of common #374

Structure for moving views and params structs out of common #374

Uh oh!

Conversation

david-crespo commented Nov 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's here and what's next

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smklein left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo Nov 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zephraph Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo commented Nov 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smklein left a comment

david-crespo commented Nov 9, 2021 •

edited

Loading

david-crespo Nov 11, 2021 •

edited

Loading

david-crespo Nov 14, 2021 •

edited

Loading

zephraph Nov 16, 2021 •

edited

Loading

david-crespo Nov 17, 2021 •

edited

Loading

david-crespo Nov 18, 2021 •

edited

Loading