From 6e535f22824ddcd0095f1de214eca602618e1c59 Mon Sep 17 00:00:00 2001 From: "Adam H. Leventhal" Date: Tue, 17 Mar 2020 16:36:37 -0700 Subject: [PATCH 1/3] edge --- src/httpapi/router.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/httpapi/router.rs b/src/httpapi/router.rs index 8a5284356f4..914f2a9c23a 100644 --- a/src/httpapi/router.rs +++ b/src/httpapi/router.rs @@ -148,10 +148,16 @@ pub struct HttpRouter { struct HttpRouterNode { /** Handlers for each of the HTTP methods defined for this node. */ method_handlers: BTreeMap>, - /** Outgoing edges for different literal paths. */ - edges_literals: BTreeMap>, + /** Edges linking to child nodes. */ + edges: HttpRouterEdges, +} + +#[derive(Debug)] +enum HttpRouterEdges { + /** Outgoing edges for literal paths. */ + Literals(BTreeMap>), /** Outgoing edges for variable-named paths. */ - edge_varname: Option, + Variable((String, Box)), } /** From b1d7745bd47541f53cbbebb3d8a4648aa93ec273 Mon Sep 17 00:00:00 2001 From: "Adam H. Leventhal" Date: Tue, 17 Mar 2020 22:27:44 -0700 Subject: [PATCH 2/3] move to union type --- rustfmt.toml | 2 +- src/httpapi/router.rs | 171 ++++++++++++++++++++---------------------- 2 files changed, 83 insertions(+), 90 deletions(-) diff --git a/rustfmt.toml b/rustfmt.toml index e48aca7c1ce..7c364819a87 100644 --- a/rustfmt.toml +++ b/rustfmt.toml @@ -74,7 +74,7 @@ trailing_comma = "Vertical" match_block_trailing_comma = false blank_lines_upper_bound = 1 blank_lines_lower_bound = 0 -edition = "2015" +edition = "2018" version = "One" inline_attribute_width = 0 merge_derives = true diff --git a/src/httpapi/router.rs b/src/httpapi/router.rs index 914f2a9c23a..f6350333e14 100644 --- a/src/httpapi/router.rs +++ b/src/httpapi/router.rs @@ -149,7 +149,7 @@ struct HttpRouterNode { /** Handlers for each of the HTTP methods defined for this node. */ method_handlers: BTreeMap>, /** Edges linking to child nodes. */ - edges: HttpRouterEdges, + edges: Option, } #[derive(Debug)] @@ -226,17 +226,22 @@ pub struct RouterLookupResult<'a> { pub variables: BTreeMap, } +impl HttpRouterNode { + pub fn new() -> Self { + HttpRouterNode { + method_handlers: BTreeMap::new(), + edges: None, + } + } +} + impl HttpRouter { /** * Returns a new `HttpRouter` with no routes configured. */ pub fn new() -> Self { HttpRouter { - root: Box::new(HttpRouterNode { - method_handlers: BTreeMap::new(), - edges_literals: BTreeMap::new(), - edge_varname: None, - }), + root: Box::new(HttpRouterNode::new()), } } @@ -306,51 +311,31 @@ impl HttpRouter { node = match segment { PathSegment::Literal(lit) => { - /* - * We do not allow both literal and variable edges from the - * same node. This could be supported (with some caveats - * about how matching would work), but it seems more likely - * to be a mistake. - */ - if let Some(HttpRouterEdgeVariable(varname, _)) = - &node.edge_varname - { - panic!( - "URI path \"{}\": attempted to register route for \ - literal path segment \"{}\" when a route exists \ - for variable path segment (variable name: \"{}\")", - path, lit, varname - ); - } - - if !node.edges_literals.contains_key(&lit) { - let newnode = Box::new(HttpRouterNode { - method_handlers: BTreeMap::new(), - edges_literals: BTreeMap::new(), - edge_varname: None, - }); - - node.edges_literals.insert(lit.clone(), newnode); + match node.edges.get_or_insert(HttpRouterEdges::Literals( + BTreeMap::new(), + )) { + /* + * We do not allow both literal and variable edges from the + * same node. This could be supported (with some caveats + * about how matching would work), but it seems more likely + * to be a mistake. + */ + HttpRouterEdges::Variable((varname, _)) => { + panic!( + "URI path \"{}\": attempted to register route \ + for literal path segment \"{}\" when a route \ + exists for variable path segment (variable \ + name: \"{}\")", + path, lit, varname + ); + } + HttpRouterEdges::Literals(ref mut literals) => literals + .entry(lit) + .or_insert(Box::new(HttpRouterNode::new())), } - - node.edges_literals.get_mut(&lit).unwrap() } PathSegment::Varname(new_varname) => { - /* - * See the analogous check above about combining literal and - * variable path segments from the same resource. - */ - if !node.edges_literals.is_empty() { - panic!( - "URI path \"{}\": attempted to register route for \ - variable path segment (variable name: \"{}\") \ - when a route already exists for a literal path \ - segment", - path, new_varname - ); - } - /* * Do not allow the same variable name to be used more than * once in the path. Again, this could be supported (with @@ -365,37 +350,42 @@ impl HttpRouter { } varnames.insert(new_varname.clone()); - if node.edge_varname.is_none() { - let newnode = Box::new(HttpRouterNode { - method_handlers: BTreeMap::new(), - edges_literals: BTreeMap::new(), - edge_varname: None, - }); - - node.edge_varname = Some(HttpRouterEdgeVariable( - new_varname.clone(), - newnode, - )); - } else if *new_varname - != *node.edge_varname.as_ref().unwrap().0 - { + match node.edges.get_or_insert(HttpRouterEdges::Variable(( + new_varname.clone(), + Box::new(HttpRouterNode::new()), + ))) { /* - * Don't allow people to use different names for the - * same part of the path. Again, this could be - * supported, but it seems likely to be confusing and - * probably a mistake. + * See the analogous check above about combining literal and + * variable path segments from the same resource. */ - panic!( - "URI path \"{}\": attempted to use variable name \ - \"{}\", but a different name (\"{}\") has \ - already been used for this", - path, - new_varname, - node.edge_varname.as_ref().unwrap().0 - ); + HttpRouterEdges::Literals(_) => panic!( + "URI path \"{}\": attempted to register route for \ + variable path segment (variable name: \"{}\") \ + when a route already exists for a literal path \ + segment", + path, new_varname + ), + + HttpRouterEdges::Variable((varname, ref mut node)) => { + if *new_varname != *varname { + /* + * Don't allow people to use different names for the + * same part of the path. Again, this could be + * supported, but it seems likely to be confusing and + * probably a mistake. + */ + panic!( + "URI path \"{}\": attempted to use \ + variable name \"{}\", but a different \ + name (\"{}\") has already been used for \ + this", + path, new_varname, varname + ); + } + + node + } } - - &mut node.edge_varname.as_mut().unwrap().1 } }; } @@ -434,14 +424,18 @@ impl HttpRouter { for segment in all_segments { let segment_string = segment.to_string(); - if let Some(n) = node.edges_literals.get(&segment_string) { - node = n; - } else if let Some(edge) = &node.edge_varname { - variables.insert(edge.0.clone(), segment_string); - node = &edge.1 - } else { - return Err(HttpError::for_status(StatusCode::NOT_FOUND)); + + node = match &node.edges { + None => None, + Some(HttpRouterEdges::Literals(edges)) => { + edges.get(&segment_string) + } + Some(HttpRouterEdges::Variable((varname, ref node))) => { + variables.insert(varname.clone(), segment_string); + Some(node) + } } + .ok_or(HttpError::for_status(StatusCode::NOT_FOUND))?; } /* @@ -453,14 +447,13 @@ impl HttpRouter { } let methodname = method.as_str().to_uppercase(); - if let Some(handler) = node.method_handlers.get(&methodname) { - Ok(RouterLookupResult { - handler: handler, - variables: variables, + node.method_handlers + .get(&methodname) + .map(|handler| RouterLookupResult { + handler, + variables, }) - } else { - Err(HttpError::for_status(StatusCode::METHOD_NOT_ALLOWED)) - } + .ok_or(HttpError::for_status(StatusCode::METHOD_NOT_ALLOWED)) } } From 11d1f37796b8fce02d8c96d26fd77c773670cf88 Mon Sep 17 00:00:00 2001 From: "Adam H. Leventhal" Date: Wed, 18 Mar 2020 10:08:25 -0700 Subject: [PATCH 3/3] turn router edges into enum type --- src/httpapi/router.rs | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/src/httpapi/router.rs b/src/httpapi/router.rs index f6350333e14..a949a0aab48 100644 --- a/src/httpapi/router.rs +++ b/src/httpapi/router.rs @@ -157,7 +157,7 @@ enum HttpRouterEdges { /** Outgoing edges for literal paths. */ Literals(BTreeMap>), /** Outgoing edges for variable-named paths. */ - Variable((String, Box)), + Variable(String, Box), } /** @@ -276,7 +276,7 @@ impl HttpRouter { * that does not apply here. We could certainly make one up (e.g., * "http://127.0.0.1") and construct a URL whose path matches the path * we were given. However, while it seems natural that our internal - * representaiton would not be percent-encoded, the "url" crate + * representation would not be percent-encoded, the "url" crate * percent-encodes any path that it's given. Further, we probably want * to treat consecutive "/" characters as equivalent to a single "/", * but that crate treats them separately (which is not unreasonable, @@ -311,16 +311,17 @@ impl HttpRouter { node = match segment { PathSegment::Literal(lit) => { - match node.edges.get_or_insert(HttpRouterEdges::Literals( - BTreeMap::new(), - )) { + let edges = node.edges.get_or_insert( + HttpRouterEdges::Literals(BTreeMap::new()), + ); + match edges { /* * We do not allow both literal and variable edges from the * same node. This could be supported (with some caveats * about how matching would work), but it seems more likely * to be a mistake. */ - HttpRouterEdges::Variable((varname, _)) => { + HttpRouterEdges::Variable(varname, _) => { panic!( "URI path \"{}\": attempted to register route \ for literal path segment \"{}\" when a route \ @@ -331,7 +332,7 @@ impl HttpRouter { } HttpRouterEdges::Literals(ref mut literals) => literals .entry(lit) - .or_insert(Box::new(HttpRouterNode::new())), + .or_insert_with(|| Box::new(HttpRouterNode::new())), } } @@ -350,10 +351,12 @@ impl HttpRouter { } varnames.insert(new_varname.clone()); - match node.edges.get_or_insert(HttpRouterEdges::Variable(( - new_varname.clone(), - Box::new(HttpRouterNode::new()), - ))) { + let edges = + node.edges.get_or_insert(HttpRouterEdges::Variable( + new_varname.clone(), + Box::new(HttpRouterNode::new()), + )); + match edges { /* * See the analogous check above about combining literal and * variable path segments from the same resource. @@ -366,7 +369,7 @@ impl HttpRouter { path, new_varname ), - HttpRouterEdges::Variable((varname, ref mut node)) => { + HttpRouterEdges::Variable(varname, ref mut node) => { if *new_varname != *varname { /* * Don't allow people to use different names for the @@ -391,7 +394,7 @@ impl HttpRouter { } let methodname = method.as_str().to_uppercase(); - if let Some(_) = node.method_handlers.get(&methodname) { + if node.method_handlers.get(&methodname).is_some() { panic!( "URI path \"{}\": attempted to create duplicate route for \ method \"{}\"", @@ -430,12 +433,12 @@ impl HttpRouter { Some(HttpRouterEdges::Literals(edges)) => { edges.get(&segment_string) } - Some(HttpRouterEdges::Variable((varname, ref node))) => { + Some(HttpRouterEdges::Variable(varname, ref node)) => { variables.insert(varname.clone(), segment_string); Some(node) } } - .ok_or(HttpError::for_status(StatusCode::NOT_FOUND))?; + .ok_or_else(|| HttpError::for_status(StatusCode::NOT_FOUND))?; } /* @@ -453,7 +456,9 @@ impl HttpRouter { handler, variables, }) - .ok_or(HttpError::for_status(StatusCode::METHOD_NOT_ALLOWED)) + .ok_or_else(|| { + HttpError::for_status(StatusCode::METHOD_NOT_ALLOWED) + }) } }