actix / actix-web

Actix Web is a powerful, pragmatic, and extremely fast web framework for Rust.
https://actix.rs
Apache License 2.0
21.61k stars 1.67k forks source link

Support url_for without requests #1583

Open zizhengtai opened 4 years ago

zizhengtai commented 4 years ago

Currently, url_for[_static] is defined on HttpRequest. This works fine most of the time, but I recently found a case when using Tera that makes this unusable.

In a Tera template, we can call custom functions which are under the hood implemented as Rust functions. For example, this Tera function url_for(name="foo") could be implemented in Rust as:

fn tera_url_for(args: &HashMap<String, tera::Value>) -> Result<Value, tera::Error> {
  // Here there will be an entry ("name", "foo") in `args`
  ...
}

Now the problem is that tera_url_for needs to be registered as a global function, so it doesn't have access to the current request.

I'm thinking it'd be very helpful to expose an API that looks something like this:

// New type with `url_for` and `url_for_static` methods,
// could be a light wrapper around `ResourceMap`
use actix_web::Routes;

static MY_ROUTES: OnceCell<Routes> = OnceCell::new();

fn tera_url_for(args: &HashMap<String, tera::Value>) -> Result<Value, tera::Error> {
  let name = /* get name from args */;
  let url = MY_ROUTES.url_for(name);
  ...
}

fn main() -> io::Result<()> {
  HttpServer::new(...)
    .inspect_routes(|routes: Routes| {  // New method on `App` or `HttpServer`
      MY_ROUTES.set(routes).unwrap();
    })
    .bind(...)?
    .run()
    .await
}

This will also enable us to do cool things like print out the full route table for inspection/documentation purpose at build time, similar to what Phoenix supports.

robjtede commented 4 years ago

could this be achieved by having the custom tera function take a HttpRequest reference also?

stdrc commented 3 years ago

I've write code like this:

static ROUTES: Arc<Mutex<OnceCell<ResourceMap>>> = Arc::new(Mutex::new(OnceCell::new()));

pub fn serve(instance: Instance, host: &str, port: u16) -> PressResult<()> {
    let addr = format!("{}:{}", host, port);
    Ok(actix_web::rt::System::new("main").block_on(async move {
        HttpServer::new(move || {
            let mut tera = Tera::new(
                instance
                    .template_folder
                    .join("**")
                    .join("*.html")
                    .to_str()
                    .unwrap(),
            )
            .expect("Failed to parse templates.");

            // let routes = Arc::new(Mutex::new(OnceCell::new()));
            // tera.register_function("url_for", make_url_for());
            tera.register_function(
                "url_for",
                |args: &HashMap<String, tera::Value>| -> Result<tera::Value, tera::Error> {
                    println!("args: {:?}", args);
                    let name = args["name"]
                        .as_str()
                        .ok_or(tera::Error::msg("`name` should be a string"))?;
                    let empty_elements = tera::Value::Array(vec![]);
                    let elements_iter = args
                        .get("elements")
                        .unwrap_or(&empty_elements)
                        .as_array()
                        .ok_or(tera::Error::msg("`elements` should be an array"))?
                        .iter();
                    let mut elements = vec![];
                    for elem in elements_iter {
                        elements.push(elem.as_str().ok_or(tera::Error::msg(
                            "`elements` array should contain only strings",
                        ))?);
                    }
                    let routes = ROUTES.lock().unwrap().get().ok_or(tera::Error::msg(
                        "`url_for` should only be called in request context",
                    ))?;
                    let fake_req = TestRequest::default().to_http_request();
                    let url = routes
                        .url_for(&fake_req, name, elements)
                        .or(Err(tera::Error::msg("resource not found")))?;
                    println!("url: {:?}", url);
                    Ok(tera::Value::String(url.path().to_string()))
                },
            );

            App::new()
                .app_data(web::Data::new(State {
                    instance: instance.clone(),
                    templates: tera,
                }))
                .wrap_fn(move |req, srv| {
                    ROUTES
                        .lock()
                        .unwrap()
                        .get_or_init(|| req.resource_map().clone());
                    srv.call(req)
                })
                .service(index)
                // ...
        })
        .bind(addr)?
        .run()
        .await
    })?)
}

But the code won't compile, because the parent: RefCell<Weak<ResourceMap>> and patterns: Vec<(ResourceDef, Option<Rc<ResourceMap>>)> fields of ResourceMap cannot be sent between threads.

could this be achieved by having the custom tera function take a HttpRequest reference also?

So I'm afraid it can't be achieved by simply keeping a HttpRequest reference in tera function.

fakeshadow commented 3 years ago

actix-web in general does not use any thread safe types. The basic requirement for any thread safe smart pointer is at least Send bound and you can not make anything thread safe without it.

try use thread_local to store it in a thread local static or use Box::leak to get hold of 'static reference. They are not good practice though and in general you are doing it wrong if you encounter thread safe problem in actix-web and a rethink of the design could be better

stdrc commented 3 years ago

@fakeshadow Thanks! It works now, my code:

thread_local! {
    static ROUTES_KEY: OnceCell<ResourceMap> = OnceCell::new();
}

fn tera_url_for(args: &HashMap<String, tera::Value>) -> Result<tera::Value, tera::Error> {
    let name = args["name"]
        .as_str()
        .ok_or(tera::Error::msg("`name` should be a string"))?;
    let empty_elements = tera::Value::Array(vec![]);
    let elements_iter = args
        .get("elements")
        .unwrap_or(&empty_elements)
        .as_array()
        .ok_or(tera::Error::msg("`elements` should be an array"))?
        .iter();
    let mut elements = vec![];
    for elem in elements_iter {
        elements.push(elem.as_str().ok_or(tera::Error::msg(
            "`elements` array should contain only strings",
        ))?);
    }
    ROUTES_KEY.with(|routes| {
        let routes = routes.get().ok_or(tera::Error::msg(
            "`url_for` should only be called in request context",
        ))?;
        let fake_req = TestRequest::default().to_http_request();
        let url = routes
            .url_for(&fake_req, name, elements)
            .or(Err(tera::Error::msg("resource not found")))?;
        Ok(tera::Value::String(url.path().to_string())) // TODO: prepend url root
    })
}

pub fn serve(instance: Instance, host: &str, port: u16) -> PressResult<()> {
    let addr = format!("{}:{}", host, port);
    Ok(actix_web::rt::System::new("main").block_on(async move {
        HttpServer::new(move || {
            let mut tera = Tera::new(
                instance
                    .template_folder
                    .join("**")
                    .join("*.html")
                    .to_str()
                    .unwrap(),
            )
            .expect("Failed to parse templates.");

            tera.register_function("url_for", tera_url_for);

            App::new()
                .app_data(web::Data::new(State {
                    instance: instance.clone(),
                    templates: tera,
                }))
                .wrap_fn(move |req, srv| {
                    ROUTES_KEY.with(|routes| {
                        routes.get_or_init(|| req.resource_map().clone());
                    });
                    srv.call(req)
                })
                // ...
        })
        .bind(addr)?
        .run()
        .await
    })?)
}
fakeshadow commented 3 years ago

You probably can use RefCell<Option<ResourceMap>> instead of OnceCell. It would save you a little bit cost on the atomic operation

stdrc commented 3 years ago

@fakeshadow Thanks, I will try it