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.

Sign up for GitHub

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

Router refactor #307

Merged
merged 4 commits into from
Nov 20, 2022
Merged

Router refactor #307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f08f890
fix(routers): add a Router trait along with some minor improvements
AntoineRR Nov 10, 2022
bc398f3
fix: clippy warnings
AntoineRR Nov 10, 2022
456f872
fix: fixes after review
AntoineRR Nov 11, 2022
c0850a7
fix: rename DynRouter to HttpRouter
AntoineRR Nov 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 56 additions & 78 deletions src/routers/const_router.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,120 +1,65 @@
use std::collections::HashMap;
use std::str::FromStr;
use std::sync::Arc;
use std::sync::RwLock;
// pyo3 modules
use crate::executors::execute_function;
use anyhow::Context;
use log::debug;
use pyo3::prelude::*;
use pyo3::types::PyAny;

use actix_web::http::Method;
use matchit::Router;

use anyhow::{bail, Error, Result};
use anyhow::{Error, Result};

/// Contains the thread safe hashmaps of different routes
use super::Router;

type RouteMap = RwLock<matchit::Router<String>>;

/// Contains the thread safe hashmaps of different routes
pub struct ConstRouter {
get_routes: Arc<RwLock<Router<String>>>,
post_routes: Arc<RwLock<Router<String>>>,
put_routes: Arc<RwLock<Router<String>>>,
delete_routes: Arc<RwLock<Router<String>>>,
patch_routes: Arc<RwLock<Router<String>>>,
head_routes: Arc<RwLock<Router<String>>>,
options_routes: Arc<RwLock<Router<String>>>,
connect_routes: Arc<RwLock<Router<String>>>,
trace_routes: Arc<RwLock<Router<String>>>,
routes: HashMap<Method, Arc<RouteMap>>,
}

impl ConstRouter {
pub fn new() -> Self {
Self {
get_routes: Arc::new(RwLock::new(Router::new())),
post_routes: Arc::new(RwLock::new(Router::new())),
put_routes: Arc::new(RwLock::new(Router::new())),
delete_routes: Arc::new(RwLock::new(Router::new())),
patch_routes: Arc::new(RwLock::new(Router::new())),
head_routes: Arc::new(RwLock::new(Router::new())),
options_routes: Arc::new(RwLock::new(Router::new())),
connect_routes: Arc::new(RwLock::new(Router::new())),
trace_routes: Arc::new(RwLock::new(Router::new())),
}
}

#[inline]
fn get_relevant_map(&self, route: Method) -> Option<Arc<RwLock<Router<String>>>> {
match route {
Method::GET => Some(self.get_routes.clone()),
Method::POST => Some(self.post_routes.clone()),
Method::PUT => Some(self.put_routes.clone()),
Method::PATCH => Some(self.patch_routes.clone()),
Method::DELETE => Some(self.delete_routes.clone()),
Method::HEAD => Some(self.head_routes.clone()),
Method::OPTIONS => Some(self.options_routes.clone()),
Method::CONNECT => Some(self.connect_routes.clone()),
Method::TRACE => Some(self.trace_routes.clone()),
_ => None,
}
}

#[inline]
fn get_relevant_map_str(&self, route: &str) -> Option<Arc<RwLock<Router<String>>>> {
if route != "WS" {
let method = match Method::from_bytes(route.as_bytes()) {
Ok(res) => res,
Err(_) => return None,
};

self.get_relevant_map(method)
} else {
None
}
}

/// Checks if the functions is an async function
/// Inserts them in the router according to their nature(CoRoutine/SyncFunction)
impl Router<String, Method> for ConstRouter {
Copy link
Member

@sansyrox sansyrox Nov 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @AntoineRR ,

I am a bit unfamiliar with traits in rust. Can you please explain this statement to me? And the benefits associated with it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure !

About traits

Traits are a way in Rust to share behavior between structs. There is no inheritance in Rust as in a truly object oriented language like C++, so traits are more similar to interfaces in Java for example.

Here, defining the Router trait allows to specify what methods should be in a router : add_route and get_route. For every router we want to define, we have to implement this trait using the line above. This will force us to provide a definition for the add_route and get_route method that is specific to our struct, which is ConstRouter here.

About the generics

I didn't want to change the code too much so I kept the same return types and parameter types in every router struct. The return type of the get_route method is different across the routers (DynRouter and MiddlewareRouter return an Option<((PyFunction, u8), HashMap<String, String>)> while ConstRouter return an Option<String>), so I used a generic type to unify their behavior in the trait. Similarly, the route_type parameter of the get_route method can either be a MiddlewareRoute or a Method. I needed two different generic types to handle those differences: this is T and U in the definition of the trait : pub trait Router<T, U>. For ConstRouter, T is String and U is Method.

Benefits of using traits

With this PR, DynRouter, ConstRouter and MiddlewareRouter all implement the Router trait (I wanted to include the WebSocketRouter too but it seems too different from the others). This means the compiler is aware those share some behavior. We could now do some cool things like:

  • Creating a Vec<dyn Router> containing the routers
  • Create generic methods that take a type implementing the Router trait as parameter -> fn method<T: Router>(router: T)
  • Eventually provide a default implementation for the add_route and get_route methods if we can make it the same for all structs (which I may try to do later)
  • If a new method needs to be added for every router, its implementation can be in the Router trait instead of in each struct
  • Maybe other things I forgot?

I hope this makes things clearer! In this PR I didn't take advantage of the trait for now, I wanted to do small iterations on the code instead of a really big one. I will try to improve things further if I have some time 😉

Copy link
Member

Choose a reason for hiding this comment

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

Thank you @AntoineRR . This an excellent explanation 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're welcome ! 😉

/// Doesn't allow query params/body/etc as variables cannot be "memoized"/"const"ified
pub fn add_route(
fn add_route(
&self,
route_type: &str, // we can just have route type as WS
route: &str,
function: Py<PyAny>,
is_async: bool,
number_of_params: u8,
event_loop: &PyAny,
event_loop: Option<&PyAny>,
) -> Result<(), Error> {
let table = match self.get_relevant_map_str(route_type) {
Some(table) => table,
None => bail!("No relevant map"),
};
let table = self
.get_relevant_map_str(route_type)
.context("No relevant map")?
.clone();

let route = route.to_string();
let event_loop =
event_loop.context("Event loop must be provided to add a route to the const router")?;

pyo3_asyncio::tokio::run_until_complete(event_loop, async move {
let output = execute_function(function, number_of_params, is_async)
.await
.unwrap();
debug!("This is the result of the output {:?}", output);
table
.clone()
.write()
.unwrap()
.insert(route, output.get("body").unwrap().to_string())
.unwrap();

Ok(())
})
.unwrap();
})?;

Ok(())
}

// Checks if the functions is an async function
// Inserts them in the router according to their nature(CoRoutine/SyncFunction)
pub fn get_route(
&self,
route_method: Method,
route: &str, // check for the route method here
) -> Option<String> {
// need to split this function in multiple smaller functions
let table = self.get_relevant_map(route_method)?;
fn get_route(&self, route_method: Method, route: &str) -> Option<String> {
let table = self.routes.get(&route_method)?;
let route_map = table.read().ok()?;

match route_map.at(route) {
Expand All @@ -123,3 +68,36 @@ impl ConstRouter {
}
}
}

impl ConstRouter {
pub fn new() -> Self {
let mut routes = HashMap::new();
routes.insert(Method::GET, Arc::new(RwLock::new(matchit::Router::new())));
routes.insert(Method::POST, Arc::new(RwLock::new(matchit::Router::new())));
routes.insert(Method::PUT, Arc::new(RwLock::new(matchit::Router::new())));
routes.insert(
Method::DELETE,
Arc::new(RwLock::new(matchit::Router::new())),
);
routes.insert(Method::PATCH, Arc::new(RwLock::new(matchit::Router::new())));
routes.insert(Method::HEAD, Arc::new(RwLock::new(matchit::Router::new())));
routes.insert(
Method::OPTIONS,
Arc::new(RwLock::new(matchit::Router::new())),
);
routes.insert(
Method::CONNECT,
Arc::new(RwLock::new(matchit::Router::new())),
);
routes.insert(Method::TRACE, Arc::new(RwLock::new(matchit::Router::new())));
Self { routes }
}

#[inline]
fn get_relevant_map_str(&self, route: &str) -> Option<&Arc<RouteMap>> {
match route {
"WS" => None,
_ => self.routes.get(&Method::from_str(route).ok()?),
}
}
}
93 changes: 93 additions & 0 deletions src/routers/http_router.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
use std::sync::RwLock;
use std::{collections::HashMap, str::FromStr};
// pyo3 modules
use crate::types::PyFunction;
use pyo3::prelude::*;
use pyo3::types::PyAny;

use actix_web::http::Method;
use matchit::Router as MatchItRouter;

use anyhow::{Context, Result};

use super::Router;

type RouteMap = RwLock<MatchItRouter<(PyFunction, u8)>>;

/// Contains the thread safe hashmaps of different routes
pub struct HttpRouter {
routes: HashMap<Method, RouteMap>,
}

impl Router<((PyFunction, u8), HashMap<String, String>), Method> for HttpRouter {
fn add_route(
&self,
route_type: &str, // We can just have route type as WS
route: &str,
handler: Py<PyAny>,
is_async: bool,
number_of_params: u8,
_event_loop: Option<&PyAny>,
) -> Result<()> {
let table = self
.get_relevant_map_str(route_type)
.context("No relevant map")?;

let function = match is_async {
true => PyFunction::CoRoutine(handler),
false => PyFunction::SyncFunction(handler),
};

// try removing unwrap here
table
.write()
.unwrap()
.insert(route.to_string(), (function, number_of_params))?;

Ok(())
}

fn get_route(
&self,
route_method: Method,
route: &str,
) -> Option<((PyFunction, u8), HashMap<String, String>)> {
let table = self.routes.get(&route_method)?;

let table_lock = table.read().ok()?;
let res = table_lock.at(route).ok()?;
let mut route_params = HashMap::new();
for (key, value) in res.params.iter() {
route_params.insert(key.to_string(), value.to_string());
}

Some((res.value.to_owned(), route_params))
}
}

impl HttpRouter {
pub fn new() -> Self {
let mut routes = HashMap::new();
routes.insert(Method::GET, RwLock::new(MatchItRouter::new()));
routes.insert(Method::POST, RwLock::new(MatchItRouter::new()));
routes.insert(Method::PUT, RwLock::new(MatchItRouter::new()));
routes.insert(Method::DELETE, RwLock::new(MatchItRouter::new()));
routes.insert(Method::PATCH, RwLock::new(MatchItRouter::new()));
routes.insert(Method::HEAD, RwLock::new(MatchItRouter::new()));
routes.insert(Method::OPTIONS, RwLock::new(MatchItRouter::new()));
routes.insert(Method::CONNECT, RwLock::new(MatchItRouter::new()));
routes.insert(Method::TRACE, RwLock::new(MatchItRouter::new()));
Self { routes }
}

#[inline]
fn get_relevant_map_str(
&self,
route: &str,
) -> Option<&RwLock<MatchItRouter<(PyFunction, u8)>>> {
match route {
"WS" => None,
_ => self.routes.get(&Method::from_str(route).ok()?),
}
}
}
86 changes: 38 additions & 48 deletions src/routers/middleware_router.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,57 +5,52 @@ use crate::types::PyFunction;
use pyo3::prelude::*;
use pyo3::types::PyAny;

use matchit::Router;

use anyhow::{bail, Error, Result};
use anyhow::{Context, Error, Result};

use crate::routers::types::MiddlewareRoute;

/// Contains the thread safe hashmaps of different routes
use super::Router;

type RouteMap = RwLock<matchit::Router<(PyFunction, u8)>>;

/// Contains the thread safe hashmaps of different routes
pub struct MiddlewareRouter {
before_request: RwLock<Router<(PyFunction, u8)>>,
after_request: RwLock<Router<(PyFunction, u8)>>,
routes: HashMap<MiddlewareRoute, RouteMap>,
}

impl MiddlewareRouter {
pub fn new() -> Self {
Self {
before_request: RwLock::new(Router::new()),
after_request: RwLock::new(Router::new()),
}
}

#[inline]
fn get_relevant_map(
&self,
route: MiddlewareRoute,
) -> Option<&RwLock<Router<(PyFunction, u8)>>> {
match route {
MiddlewareRoute::BeforeRequest => Some(&self.before_request),
MiddlewareRoute::AfterRequest => Some(&self.after_request),
}
let mut routes = HashMap::new();
routes.insert(
MiddlewareRoute::BeforeRequest,
RwLock::new(matchit::Router::new()),
);
routes.insert(
MiddlewareRoute::AfterRequest,
RwLock::new(matchit::Router::new()),
);
Self { routes }
}
}

// Checks if the functions is an async function
// Inserts them in the router according to their nature(CoRoutine/SyncFunction)
pub fn add_route(
impl Router<((PyFunction, u8), HashMap<String, String>), MiddlewareRoute> for MiddlewareRouter {
fn add_route(
&self,
route_type: MiddlewareRoute,
route_type: &str,
route: &str,
handler: Py<PyAny>,
is_async: bool,
number_of_params: u8,
_event_loop: Option<&PyAny>,
) -> Result<(), Error> {
let table = match self.get_relevant_map(route_type) {
Some(table) => table,
None => bail!("No relevant map"),
};
let table = self
.routes
.get(&MiddlewareRoute::from_str(route_type))
.context("No relevant map")?;

let function = if is_async {
PyFunction::CoRoutine(handler)
} else {
PyFunction::SyncFunction(handler)
let function = match is_async {
true => PyFunction::CoRoutine(handler),
false => PyFunction::SyncFunction(handler),
};

table
Expand All @@ -66,25 +61,20 @@ impl MiddlewareRouter {
Ok(())
}

pub fn get_route(
fn get_route(
&self,
route_method: MiddlewareRoute,
route: &str, // check for the route method here
route: &str,
) -> Option<((PyFunction, u8), HashMap<String, String>)> {
// need to split this function in multiple smaller functions
let table = self.get_relevant_map(route_method)?;
let table = self.routes.get(&route_method)?;

match table.read().unwrap().at(route) {
Ok(res) => {
let mut route_params = HashMap::new();

for (key, value) in res.params.iter() {
route_params.insert(key.to_string(), value.to_string());
}

Some((res.value.clone(), route_params))
}
Err(_) => None,
let table_lock = table.read().ok()?;
let res = table_lock.at(route).ok()?;
let mut route_params = HashMap::new();
for (key, value) in res.params.iter() {
route_params.insert(key.to_string(), value.to_string());
}

Some((res.value.to_owned(), route_params))
}
}
Loading