openapi macro prototype #1
Conversation
| @@ -0,0 +1,346 @@ | |||
| extern crate proc_macro; | |||
There was a problem hiding this comment.
What do you think about adding a block comment here explaining the purpose, and maybe some comments later in the file about the various pieces?
|
|
||
| use std::collections::HashMap; | ||
|
|
||
| macro_rules! abort { |
There was a problem hiding this comment.
A comment on this macro would be useful. Is it for compile-time errors or runtime? Why is it different under cfg!(test)?
lib/openapi/src/lib.rs
Outdated
| attr: proc_macro::TokenStream, | ||
| item: proc_macro::TokenStream, | ||
| ) -> proc_macro::TokenStream { | ||
| //treeify(0, &attr.clone().into()); |
There was a problem hiding this comment.
Drop the commented-out code? (Also at L26)
lib/openapi/src/lib.rs
Outdated
| ) -> proc_macro::TokenStream { | ||
| //treeify(0, &attr.clone().into()); | ||
| let a = attr.into(); | ||
| let mm = to_map(&a); |
There was a problem hiding this comment.
What about more descriptive names for a and mm? I'm not sure exactly what distinguishes a from attr, but maybe mm could be endpoint_params or something like that?
lib/openapi/src/lib.rs
Outdated
| let method = match mm.get("method") { | ||
| Some(MapEntry::Value(method)) => match method.as_str() { | ||
| "DELETE" | "GET" | "PATCH" | "POST" | "PUT" => method, | ||
| _ => abort!(a, r#"invlid method "{}""#, method), |
There was a problem hiding this comment.
invlid should be invalid
lib/openapi/src/lib.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn parse(tt: &TokenStream) -> MapEntry { |
There was a problem hiding this comment.
Would it make sense to inline this in the 2 places [I found] where it's used? (Otherwise, maybe a more descriptive name?)
| } | ||
|
|
||
| #[allow(dead_code)] | ||
| fn treeify(depth: usize, tt: &TokenStream) { |
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
I love all the tests.
| @@ -39,11 +41,14 @@ pub fn api_register_entrypoints(router: &mut HttpRouter) { | |||
| "/projects", | |||
| HttpRouteHandler::new(api_projects_post), | |||
| ); | |||
| /* | |||
There was a problem hiding this comment.
Putting a note here to remind both of us to delete the commented-out block and apply the macro to the other functions before we integrate.
| @@ -21,3 +21,4 @@ url = "^2.1" | |||
| #url = {version = "1.5", optional = true} | |||
| #percent-encoding = {version = "1.0.0", optional = true} | |||
| #clap = "2.25" | |||
| openapi = { path = "lib/openapi" } | |||
There was a problem hiding this comment.
I didn't know you could define crates within the crate like this but it seems like a good call. I imagine we might make this a general-purpose crate later but it'll be much easier to iterate on it here.
Volumes are "checked out" from Nexus for many reasons, some of which include sending to another service for use in `Volume::construct`. When that service activates the resulting Volume, this will forcibly take over any existing downstairs connections based on the Upstairs' generation number. This is intentional, and was designed so Nexus, in handing out Volumes with increasing generation numbers, can be sure that the resulting Volume works no matter what (for example, even if a previous Upstairs is wedged somehow, even if the service that is running the previous Upstairs is no longer accepting network connections). Up until now, Nexus wouldn't allow checking out a Volume if there is any chance a Propolis could be running that may use that Volume. This meant restricting certain operations, like creating a snapshot when a disk is attached to an instance that is stopped: any action Nexus would take to attempt a snapshot using a Pantry would race with a user's request to start that instance, and if the Volume checkouts occur in the wrong order the Pantry would take over connections from Propolis, resulting in guest OS errors. Nexus _can_ do this safely though: it has all the information required to know when a checkout is safe to do, and when it may not be safe. This commit adds checks to the Volume checkout transaction that are based on the reason that checkout is occurring, and requires call sites that are performing a checkout to say why they are. Because these checks are performed inside a transaction, Nexus can say for sure when it is safe to allow a Volume to be checked out for a certain reason. For example, in the scenario of taking a snapshot of a disk attached to an instance that is stopped, there are two checkout operations that have the possiblity of racing: 1) the one that Nexus will send to a Pantry during a snapshot create saga. 2) the one that Nexus will send to a Propolis during an instance start saga. If oxidecomputer#1 occurs before oxidecomputer#2, then Propolis will take over the downstairs connections that the Pantry has established, and the snapshot create saga will fail, but the guest OS for that Propolis will not see any errors. If oxidecomputer#2 occurs before oxidecomputer#1, then the oxidecomputer#1 checkout will fail due to one of the conditions added in this commit: the checkout is being performed for use with a Pantry, and a Propolis _may_ exist, so reject the checkout attempt. Fixes oxidecomputer#3289.
I think the change to api_http_entrypoints.rs is probably the most interesting part to check out. This just validated how the macro might work; while I'm confidant that we'll be able to extract the macro data, type content, etc. I haven't figured that all the way out (and that's the next step).