Actix-web: Compile time type-safe Data extractor

[pickfire]

Expected Behavior

It should error on compile time, or at least error when starting the application but not when user sends the request (this is probably harder).

Current Behavior

It errors on run time, and it is hard to figure out why does it fail if a more complex type is involved, users might get frustrated since it compiles but it errors when the user sends the request. Rocket have this issue too, tide does not have it now but might soon have it.

Possible Solution

• add a single state to AppState in addition to current hashmap data which may improve performance and provides compile time guarantee
• use a different type for the state, maybe State rather than Data to only allow an instance of type-safe data?
• better error message and suggestion, suggest how to fix by using the correct data and type, this is probably the hardest method

Steps to Reproduce (for bugs)

use actix_web::*;

#[get("/")]
async fn index(_data: web::Data<String>) -> impl Responder {
    "Hello world"
}

#[actix_web::main]
async fn main() -> std::io::Result<()> {
    HttpServer::new(|| App::new().service(index))
        .bind("[::]:8080")?
        .run()
        .await
}

$ curl 127.1:8080
App data is not configured, to configure use App::data()

Even if I did App::new().data("aoeu"), it still shows App data is not configured, to configure use App::data() which is not true at all.

Context

This simple example is easy to figure out what went wrong, since we have no data assigned for the application. Last time (during an interview T_T) I was dealing with some types that involves Arc and is more complicated and I took some time to figure out why was the application failing when it compiled successfully and is not as straightforward, IIRC I was changing from data to app_data.

Your Environment

• Rust Version (I.e, output of rustc -V): rustc 1.46.0 (04488afe3 2020-08-24)
• Actix Web Version: 3.0.0

[fakeshadow]

As we talked on gitter you can have static typed state if you go a bit lower into the framework. Below is an example on how to do that on a service level.

use std::task::{Context, Poll};

use actix_service::{Service, ServiceFactory};
use actix_web::{App, Error, HttpServer, HttpResponse};
use futures::future::{ok, Ready};
use actix_web::dev::{ServiceRequest, ServiceResponse, HttpServiceFactory, AppService, ResourceDef};
use actix_web::web::Data;

struct MyService {
    state: usize,
}

impl Service for MyService {
    type Request = ServiceRequest;
    type Response = ServiceResponse;
    type Error = Error;
    type Future = Ready<Result<Self::Response, Self::Error>>;

    #[inline]
    fn poll_ready(&mut self, _: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
        Poll::Ready(Ok(()))
    }

    fn call(&mut self, req: ServiceRequest) -> Self::Future {
        let (req, _) = req.into_parts();

        let state = req.app_data::<Data<usize>>().unwrap();

        assert_eq!(123usize, ***state);

        self.state += 1;

        ok(ServiceResponse::new(req, HttpResponse::Ok().body::<String>(self.state.to_string())))

    }
}

#[derive(Clone)]
struct MyServiceFactory;

impl ServiceFactory for MyServiceFactory {
    type Request = ServiceRequest;
    type Response = ServiceResponse;
    type Error = Error;
    type Config = ();
    type Service = MyService;
    type InitError = ();
    type Future = Ready<Result<Self::Service, Self::InitError>>;

    fn new_service(&self, _: ()) -> Self::Future {
        ok(MyService { state: 0 })
    }
}

impl HttpServiceFactory for MyServiceFactory {
    fn register(self, config: &mut AppService) {
        config.register_service(ResourceDef::new("/"), None,self, None);
    }
}

#[actix_web::main]
async fn main() -> std::io::Result<()> {
    HttpServer::new(|| App::new().data(123usize).service(MyServiceFactory))
        .bind("127.0.0.1:8080")?
        .run()
        .await
}

[pickfire]

Yes, it is possible to take it to the lower level, the example itself does still seemed okay (neither good nor bad) but that was not what others used, so most people that goes through the examples provided will first get hit by footgun either until they realized it as it is not a common method to do in the lower level. I was quite surprised what you meant lower level just includes a bunch (still quite a lot IMHO) of boilerplates, but not as hard as I imagined.

[fakeshadow]

After some tests I find a static typed app level state would not only affect the Request types. And it would also mess up the signature response types. Basically for an App level state we need extra type signature for these types. In process order:
App - AppInit - AppInitResult - AppInitService - HttpRequest - FromRequest and/or ServiceRequest - Response and/or ResponseBuilder - ServiceResponse.

TLDR: A static typed state would break all middlewares and all type extractors. ResponseBuilder would also require extra type signature. Too much of a price to pay and the gain is little in comparison.

[pickfire]

TLDR: A static typed state would break all middlewares and all type extractors. ResponseBuilder would also require extra type signature. Too much of a price to pay and the gain is little in comparison.

But the price is compile-time cost and higher complexity but however the current one not only incur some runtime cost for data type lookup in hashmap but also results in safety cost, if that part of code was not tested it might suddenly just break, it does not have the safety rust provides such that "if it compiles, it works".

[fakeshadow]

The cost is not complexity, it's that it will break lots of existing code.

Runtime type check is safe.

There is no such thing called "if it compiles, it works", at least not for Rust. You need a lot of tests and even with those you would still get thing wrong here and there.

[pickfire]

The cost is not complexity, it's that it will break lots of existing code.

Then could this be part of a major release since it is a breaking change? I mean it is a breaking change since it should not allow broken type in the first place, I know it is hard since it may be hard to not use Any type but rely only on features available in the rust compiler.

Runtime type check is safe.

Yes, it is safe. But since I saw actix-web keep putting post to remove the use of unsafe but why not remove foot guns which affects the users? Even other languages like python is safe, I believe it would be a great deal breaker to use a library that does not have foot gun here and there, I personally prefer rust over python due to less runtime error.

There is no such thing called "if it compiles, it works", at least not for Rust. You need a lot of tests and even with those you would still get thing wrong here and there.

It is almost there, I mean excluding logic errors. At least one does not need to invoke a debugger to check out what is wrong. I never said to skip test but for quick prototyping, it would still be better for the compiler to warn me what is wrong rather than having to cargo run (and here requires another curl to check if the data extractor is working) instead of just cargo check (most online articles mention that this is enough, I also personally found this very sufficient frequently).

At first, I wonder if this is feasible or not, but now I know that you built a prototype means it is working, I thought it may not be possible to do so. So now at least I know that there is a breaking change but not as bad as I thought it would, like harming the ergonomics or performance or having extra complexity.