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.

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

fix: header in const request bug and improve code architecture #333

Merged
merged 1 commit into from
Dec 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions integration_tests/base_routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,16 @@ async def const_request_json():
return jsonify({"hello": "world"})


@app.get("/const_request_headers", const=True)
async def const_request_headers():
return {
"status_code": "200",
"body": "",
"type": "text",
"headers": jsonify({"Header": "header_value"}),
}


@app.get("/404")
def return_404():
return {"status_code": "404", "body": "hello", "type": "text"}
Expand Down
7 changes: 7 additions & 0 deletions integration_tests/test_get_requests.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,10 @@ def test_const_request_json(session):
r = requests.get(f"{BASE_URL}/const_request_json")
assert r.status_code == 200
assert r.json() == {"hello": "world"}


def test_const_request_headers(session):
r = requests.get(f"{BASE_URL}/const_request_headers")
assert r.status_code == 200
assert "Header" in r.headers
assert r.headers["Header"] == "header_value"
6 changes: 3 additions & 3 deletions robyn/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ def __init__(self, file_object: str) -> None:
self.router = Router()
self.middleware_router = MiddlewareRouter()
self.web_socket_router = WebSocketRouter()
self.request_headers = [] # This needs a better type
self.request_headers = [] # This needs a better type
self.directories = []
self.event_handlers = {}
load_vars(project_root=directory_path)
Expand Down Expand Up @@ -83,7 +83,7 @@ def add_directory(
):
self.directories.append((route, directory_path, index_file, show_files_listing))

def add_header(self, key: str, value: str) -> None:
def add_request_header(self, key: str, value: str) -> None:
self.request_headers.append((key, value))

def add_web_socket(self, endpoint: str, ws: WS) -> None:
Expand Down Expand Up @@ -140,7 +140,7 @@ def init_processpool(socket):
target=spawn_process,
args=(
self.directories,
self.headers,
self.request_headers,
self.router.get_routes(),
self.middleware_router.get_routes(),
self.web_socket_router.get_routes(),
Expand Down
2 changes: 1 addition & 1 deletion robyn/processpool.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ def spawn_process(
route, directory_path, index_file, show_files_listing = directory
server.add_directory(route, directory_path, index_file, show_files_listing)

for key, val in headers:
for key, val in request_headers:
server.add_request_header(key, val)

for route in routes:
Expand Down
18 changes: 6 additions & 12 deletions src/executors/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/// This is the module that has all the executor functions
/// i.e. the functions that have the responsibility of parsing and executing functions.
use crate::io_helpers::read_file;
use crate::types::Request;
use crate::types::{FunctionInfo, Request, Response};

use std::collections::HashMap;
use std::sync::Arc;
Expand All @@ -10,7 +10,6 @@ use anyhow::{Context, Result};
use log::debug;
use pyo3_asyncio::TaskLocals;
// pyO3 module
use crate::types::FunctionInfo;
use pyo3::prelude::*;

fn get_function_output<'a>(
Expand Down Expand Up @@ -56,14 +55,8 @@ pub async fn execute_middleware_function<'a>(
}
}

// Change this!
#[inline]
pub async fn execute_http_function(
request: &Request,
function: FunctionInfo,
// need to change this to return a response struct
// create a custom struct for this
) -> Result<HashMap<String, String>> {
pub async fn execute_http_function(request: &Request, function: FunctionInfo) -> Result<Response> {
if function.is_async {
let output = Python::with_gil(|py| {
let function_output = get_function_output(&function, py, request);
Expand All @@ -79,10 +72,11 @@ pub async fn execute_http_function(
let contents = read_file(file_path).unwrap();
res.insert("body".to_owned(), contents);
}
Ok(res)
Response::from_hashmap(res)
} else {
Python::with_gil(|py| get_function_output(&function, py, request)?.extract())
.context("Failed to execute handler function")
Response::from_hashmap(Python::with_gil(|py| {
get_function_output(&function, py, request)?.extract()
})?)
}
}

Expand Down
18 changes: 16 additions & 2 deletions src/io_helpers/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,30 @@ use std::io::Read;

use actix_web::HttpResponseBuilder;
use anyhow::Result;
use dashmap::DashMap;

// this should be something else
// probably inside the submodule of the http router
#[inline]
pub fn apply_headers(response: &mut HttpResponseBuilder, headers: &HashMap<String, String>) {
for (key, val) in (headers).iter() {
pub fn apply_hashmap_headers(
response: &mut HttpResponseBuilder,
headers: &HashMap<String, String>,
) {
for (key, val) in headers.iter() {
response.insert_header((key.clone(), val.clone()));
}
}

#[inline]
pub fn apply_dashmap_headers(
response: &mut HttpResponseBuilder,
headers: &DashMap<String, String>,
) {
for h in headers.iter() {
response.insert_header((h.key().clone(), h.value().clone()));
}
}

Comment on lines +12 to +30
Copy link
Member

Choose a reason for hiding this comment

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

This is very smart! Good job! 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks ;) This avoids merging the headers and then applying them, which creates an unnecessary copy of both headers list. I wanted to make only one function that works with either a hashmap or a dashmap but it doesn't seem to be easy

/// A function to read lossy files and serve it as a html response
///
/// # Arguments
Expand Down
1 change: 0 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
mod executors;
mod io_helpers;
mod request_handler;
mod routers;
mod server;
mod shared_socket;
Expand Down
79 changes: 0 additions & 79 deletions src/request_handler/mod.rs

This file was deleted.

13 changes: 5 additions & 8 deletions src/routers/const_router.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use std::sync::Arc;
use std::sync::RwLock;

use crate::executors::execute_http_function;
use crate::types::Response;
use crate::types::{FunctionInfo, Request};
use anyhow::Context;
use log::debug;
Expand All @@ -16,14 +17,14 @@ use anyhow::{Error, Result};

use super::Router;

type RouteMap = RwLock<MatchItRouter<String>>;
type RouteMap = RwLock<MatchItRouter<Response>>;

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

impl Router<String, Method> for ConstRouter {
impl Router<Response, Method> for ConstRouter {
/// Doesn't allow query params/body/etc as variables cannot be "memoized"/"const"ified
fn add_route(
&self,
Expand All @@ -46,18 +47,14 @@ impl Router<String, Method> for ConstRouter {
.await
.unwrap();
debug!("This is the result of the output {:?}", output);
table
.write()
.unwrap()
.insert(route, output.get("body").unwrap().to_string())
.unwrap();
table.write().unwrap().insert(route, output).unwrap();
Ok(())
})?;

Ok(())
}

fn get_route(&self, route_method: &Method, route: &str) -> Option<String> {
fn get_route(&self, route_method: &Method, route: &str) -> Option<Response> {
let table = self.routes.get(route_method)?;
let route_map = table.read().ok()?;

Expand Down
Loading