obsidian-rs / obsidian

Ergonomic async http framework for reliable and efficient web
MIT License
26 stars 4 forks source link

Custom header support for Responder #30

Closed jk-gan closed 4 years ago

jk-gan commented 4 years ago

Currently user can't define custom header for response, make some use cases like signing completely impossible with the framework.

jk-gan commented 4 years ago

I would suggest the following syntax:

fn handler(_ctx: Context) -> impl Responder {
    "Hello World".with_header("Content-Type", "text/plain")
}

Multiple headers:

fn handler(_ctx: Context) -> impl Responder {
    "Hello World"
            .with_header("Content-Type", "text/plain")
            .with_header("X-Custom-Header", "custom-header-value")
}
plwai commented 4 years ago

I think with_header is too long. header should be enough?

fn handler(_ctx: Context) -> impl Responder {
    "Hello World"
            .header("Content-Type", "text/plain")
            .header("X-Custom-Header", "custom-header-value")
}
jk-gan commented 4 years ago

So the syntax for a responder with custom headers and custom status code could be

fn handler(_ctx: Context) -> impl Responder {
    "Hello World"
            .header("Content-Type", "text/plain")
            .header("X-Custom-Header", "custom-header-value")
            .status(StatusCode::OK)
}
plwai commented 4 years ago

I think that is just nice.

jk-gan commented 4 years ago

I'm thinking other possibilities:

Tuple

fn handler(_ctx: Context) -> impl Responder {
    let headers = [
           ("Content-Type", "text/plain"),
           ("X-Custom-Header", "custom-header-value"),
    ];

    (200, headers, "Hello World")
}

Response struct

fn handler(_ctx: Context) -> impl Responder {
    let headers = [
           ("Content-Type", "text/plain"),
           ("X-Custom-Header", "custom-header-value"),
    ];

    Response {
        status: StatusCode::OK,
        headers,
        body: "Hello World",
    }
}
plwai commented 4 years ago

I feel first one is better for extension. Lets say at the beginning user don't want to add header. Later on user can add it easily by extend it.

jk-gan commented 4 years ago

I feel first one is better for extension. Lets say at the beginning user don't want to add header. Later on user can add it easily by extend it.

We have another tuple which only have 2 value inside: (status_code, body)

Btw I prefer the headers generated externally, so that it is more flexible and avoid duplication if they share some of the headers:

fn generate_headers(request_id: &str) -> Vec<(&str, &str)> {
    [
        ("Content-Type", "text/plain"),
        ("X-Request-ID", request_id),
        ("X-Custom-Header", "custom-header-value"),
    ]
}

fn handler_a(_ctx: Context) -> impl Responder {
    let headers = generate_headers("1a");

    (200, headers, "Hello World")
}

fn handler_b(_ctx: Context) -> impl Responder {
    let headers = generate_headers("1b");

    (200, headers, "Hello World")
}

There might be a better way, but for not I can't think of any

plwai commented 4 years ago

I feel first one is better for extension. Lets say at the beginning user don't want to add header. Later on user can add it easily by extend it.

We have another tuple which only have 2 value inside: (status_code, body)

I prefer the headers generated externally, so that it is more flexible and avoid duplication if they share some of the headers:

fn generate_headers(request_id: &str) -> Vec<(&str, &str)> {
    [
        ("Content-Type", "text/plain"),
        ("X-Request-ID", request_id),
        ("X-Custom-Header", "custom-header-value"),
    ]
}

fn handler_a(_ctx: Context) -> impl Responder {
    let headers = generate_headers("1a");

    (200, headers, "Hello World")
}

fn handler_b(_ctx: Context) -> impl Responder {
    let headers = generate_headers("1b");

    (200, headers, "Hello World")
}

There might be a better way, but for not I can't think of any

const fn generate_headers is a good point. There is another approach which is used in http::Request

fn generate_headers(request_id: &str) -> Vec<(&str, &str)> {
    [
        ("Content-Type", "text/plain"),
        ("X-Request-ID", request_id),
        ("X-Custom-Header", "custom-header-value"),
    ]
}

fn handler(_ctx: Context) -> impl Responder {
    let headers = generate_headers("1a");

    "Hello World".headers_mut().add(headers)
}

From the implementation wise, I think the tuple and Response approach will be easier.

plwai commented 4 years ago

Tuple style I feel hard to use as the args location matters and it is not precise for new comers.

Response struct style is good to read but it is too different with the one without status and headers.

For example from

fn handler(_ctx: Context) -> impl Responder {
    let headers = [
           ("Content-Type", "text/plain"),
           ("X-Custom-Header", "custom-header-value"),
    ];

    "Hello World"
}

to

fn handler(_ctx: Context) -> impl Responder {
    let headers = [
           ("Content-Type", "text/plain"),
           ("X-Custom-Header", "custom-header-value"),
    ];

    Response {
        status: StatusCode::OK,
        headers,
        body: "Hello World",
    }
}

So I think maybe this is the better style.

const fn generate_headers() -> Vec<(&str, &str)> {
    [
        ("Content-Type", "text/plain"),
        ("X-Request-ID", request_id),
        ("X-Custom-Header", "custom-header-value"),
    ]
}

fn handler(_ctx: Context) -> impl Responder {
    "Hello World"
            .headers(generate_headers())
}
pickfire commented 4 years ago

What if?

const fn generate_headers() -> Vec<(&str, &str)> {
    [
        ("Content-Type", "text/plain"),
        ("X-Request-ID", request_id),
        ("X-Custom-Header", "custom-header-value"),
        ("X-Custom-Header", "custom-header-value"),
    ]
}

fn handler(_ctx: Context) -> impl Responder {
    "Hello World"
            .headers(generate_headers())
}
jk-gan commented 4 years ago

For current implementation, the later header will overwrite the previous one if they have same key. Does this match your expectation?

plwai commented 4 years ago

For current implementation, the later header will overwrite the previous one if they have same key. Does this match your expectation?

I think that is ok.