PoC - Absorbing views and params into Nexus

[david-crespo]

Here's one way it could look to move more of what's currently in common into Nexus, using Project and its create and update params as a demo. This is one of the few models where we don't have to choose between client gen and literal code sharing because Project is not used by internal clients. Doing this piecemeal view by view is probably not a good idea, so this is more meant to show what it looks like so we can think about doing them all this way, or at least picture how it will be when we have client gen.

Why

Short version: I want the imports to mirror the logical flow of data in the application. In this case that means that because we go from model to view, models should not "know about" views, which means the model file should not import view structs in order to define the conversions. Instead we define the conversions where the views are defined. This requires the views to be defined in nexus and not in common.

In my view, some architectural complication in omicron comes from the "view" structs living in omicron-common so they can be shared by the different apps. For example, the Instance view would be used in Nexus to serialize a JSON response, and would be used in another app to deserialize it from JSON. To me this is mostly a workaround for the fact that we're not yet using generated clients — once we have those, we do not need to directly share these structs. They can be defined only in the app that uses them as its responses, and the clients will use (presumably near-identical) generated versions.

In #188 we made it so the DB mostly doesn't know about the view structs, but the views were left in omicron common and the conversions from model to view were still defined in the db models file. This PR shows how it would look to move the views closer to their context of use.

Features of note

• Project view and params entirely contained in nexus
• Project view imports db::model and defines From, not the other way around. db::model doesn't know about views
• db::model doesn't know about params either. conversion from params to model is done next to params definition. this is optional but I think I like it

[david-crespo]

probably don't need to be public, I just don't know how to make the tests that use these work otherwise

[smklein]

I do wanna give the caveat - which I think you're already aware of - we should only make this move if we can do "everything or nothing", because having the Nexus API split between the omicron-common api module and an nexus view module puts things in an awkward split state I'd like to avoid.

That being said, assuming we could move everything from api::external + api::internal::nexus into omicron-nexus...

(Things I like)

• It definitely makes sense for Nexus to own the types it exposes. In a way, "where the conversion between model/view lives" is less important when both types are in the same crate - it makes the translation easy to do.
• In a similar vein, avoiding having these structures in omicron-common makes the "private-by-default" nature a little easier.

(Questions/Concerns)

• I think the distinction between internal and external views is still worthwhile preserving. As an example, moving ProjectCreateParams from external into params obfuscates where it is being used. Since Nexus exposes both an internal and external API, this is an especially important property to consider. Similarly, omicron-nexus/src/view.rs doesn't indicate internal/external-ness.
• Why distinguish "params" from "views"? Neither need to be directly correlated with a model, so I considered both "a particular lens at some data, exposed through the API".
• I noticed this type isn't used by nexus_client.rs, which itself normally has a dependency on these types. It's important that, for example, the sled agent can access client bindings without depending directly on the omicron-nexus crate. As we've discussed, this seems possible if we're generating the client bindings as a new crate, but that might be a prerequisite for pulling this off fully.

TL;DR: I like this direction, but I think good client generation + preserving the internal/external boundary will be a prereq.

[david-crespo]

I think the distinction between internal and external views is still worthwhile preserving.

Good point, I forgot about that because Project is only external. Something about that distinction still rubs me the wrong way, but I can't put my finger on it. Maybe I've just seen too much api::external:: everywhere because those views are referenced in too many places. After refactoring, the models would be what's used everywhere and the views would only get called in the entrypoints.

Maybe we can go for a dir structure like this:

• external/
  • http_entrypoints.rs
  • views.rs
• internal/
  • http_entrypoints.rs
  • views.rs

Why distinguish "params" from "views"?

Params are the inputs to post and put requests and views are the responses. They can go in the same file (maybe they should be grouped by resource anyway), I just thought it was cute using params:: to get the same effect as *Params. Concretely, I assumed you always go model::Project -> views::Project and never the other way, and always go params::ProjectCreate -> model::Project, never model::Project -> params::ProjectCreate. That assumption may not actually hold, but it does make things easy to think about if they each go one direction only, and it's how I've seen this done in the past.

this type isn't used by nexus_client.rs

Yeah, looks like almost nothing is in there, which is funny. Good case for the client generation.

[david-crespo]

Not sure I like my approach, but 3877675 is an attempt at bringing external/internal back.

The weirdest part (good example of why these experiments are useful) is putting params in external. This weirdness is helping me see where your question above about params vs. views was coming from. On one hand params represent the expected shape of a request body, but they are currently also used as args for most of the create and update functions in datastore. ("Most" because datastore.project_create takes a Project model which we made out of params::ProjectCreate one level up in nexus.rs.) So it's not quite right to think of them as only or even primarily part of the external API, when they also define the interface to these internal functions that talk to the DB.

I think this collapsing of the distinction between API params and db function params is going to make less sense as we go forward because the API params are going to stop mapping directly to the model properties. For example, right now on instance create we take memory in bytes and pass that straight through to the database. But whatever the outcome of RFD 190 is, we surely don't expect the public API to specify memory in bytes forever. Presumably we will take it in GiB or MiB and convert to bytes (assuming we don't store it as MiB in the DB). A more dramatic version of this is the API taking an instance type name as a string and converting that to the right ncpus and memory and whatever. In any case, the API instance create endpoint params end up distinct from the DB instance create function params and there has to be a translation step (which doesn't have to be formalized into a From necessarily, it could just be inline in the endpoint handler or a function in nexus.rs).

A rather maximalist design that goes along with the existing setup would be that every PUT and POST endpoint has a corresponding params struct representing its request body, and then the datastore functions all have their own structs that presumably line up with the db representation and can be passed more or less straight through.

The scrappier lightweight version enabled by a query builder would be that the datastore functions don't have interfaces because datastore doesn't exist, and where we currently call it

let instance = osagactx
    .datastore()
    .project_create_instance(
        &instance_id?,
        &params.project_id,
        &params.create_params,
        &runtime.into(),
    )

we call the query builder directly:

let instance = models::Instance { blah blah blah };
diesel::insert_into(instances::table)
    .values(&instance)
    .on_conflict_do_nothing()
    .execute(&*conn)?;

I like this version better. Structs representing request params still exist, of course, but as they are only used for parsing request bodies, they don't have to be imported anywhere besides the entrypoints file. They could even be defined inline next to their http entrypoint functions.

Some useful examples from crates.io

• PUT /crates/:crate_id/follow
• PUT /crates/new
  • note how they construct a DB model right in the handler and pass it to a query. nothing analogous to datastore
  • EncodableCrateUpload is analogous to one of our CreateParams structs. It's used to parse the request body.
• PUT /crates/:crateId/owners
  • helper function analogous to a datastore function in our code is a function defined on the crate model that takes an entire User model as its argument, no special params struct
  • Request params struct is defined inline

[david-crespo]

We can pick this sort of thing back up after #192