pingora icon indicating copy to clipboard operation
pingora copied to clipboard

pingora_http::ResponseHeader is not exported in pingora::prelude, but pingora_http::RequestHeader is exported

Open PeppyDays opened this issue 10 months ago • 3 comments

Describe the bug

This is a minor bug for import consistency. When using pingora crate to follow examples, most of the required things are imported from pingora::prelude::* including pingora_http::RequestHeader, but not pingora_http::ResponseHeader.

pingora::prelude exports pingora_http::prelude::*, but pingora_http::prelude only includes RequestHeader as follows. See here.

pub mod prelude {
    pub use crate::RequestHeader;
}

I think pub use crate::ResponseHeader; should be included for consistency.

Pingora info

Please include the following information about your environment:

Pingora version: Release 0.4.0, and the latest main as of the time of creating this Rust version: 1.85.0 Operating system version: MacOS 14.6.1 (23G93)

Steps to reproduce

Here is the sample code which I feel the import statements are not consistent.

use std::sync::Arc;
use std::time::Duration;

// This does not include ResponseHeader to implement ProxyHttp::response_filter
use pingora::prelude::*;
// So manual import from pingora_http is required
// But ResponseHeader to implement ProxyHttp::request_filter is included in pingora::prelude
use pingora::http::ResponseHeader;

struct LB(Arc<LoadBalancer<RoundRobin>>);

#[async_trait::async_trait]
impl ProxyHttp for LB {
    type CTX = ();

    fn new_ctx(&self) -> Self::CTX {}

    async fn upstream_peer(
        &self,
        _session: &mut Session,
        _ctx: &mut Self::CTX,
    ) -> Result<Box<HttpPeer>> {
        let upstream = self.0.select(b"", 256).unwrap();
        Ok(Box::from(HttpPeer::new(upstream, false, String::from(""))))
    }

    async fn upstream_request_filter(
        &self,
        _session: &mut Session,
        upstream_request: &mut RequestHeader,
        _ctx: &mut Self::CTX,
    ) -> Result<()>
    where
        Self::CTX: Send + Sync,
    {
        upstream_request
            .insert_header("x-proxy-from", "0.0.0.0:6193")
            .unwrap();

        Ok(())
    }

    async fn request_filter(&self, session: &mut Session, _ctx: &mut Self::CTX) -> Result<bool>
    where
        Self::CTX: Send + Sync,
    {
        if !session.req_header().uri.path().starts_with("/health") {
            let _ = session.respond_error(403).await;
            return Ok(true);
        }

        Ok(false)
    }

    async fn response_filter(
        &self,
        _session: &mut Session,
        upstream_response: &mut ResponseHeader,
        _ctx: &mut Self::CTX,
    ) -> Result<()>
    where
        Self::CTX: Send + Sync,
    {
        upstream_response.insert_header("Name", "Arine").unwrap();
        Ok(())
    }
}

Expected results

use pingora::prelude::RequestHeader;
use pingora::prelude::ResponseHeader;

Import statements for RequestHeader and ResponseHeader have consistent path.

Observed results

N/A

Additional context

N/A

PeppyDays avatar Mar 06 '25 15:03 PeppyDays

I want to make a PR for this without filing this issue because it's very simple and not much to discuss (even though it is expected), but failed to create a branch T.T

PeppyDays avatar Mar 06 '25 15:03 PeppyDays

I want to make a PR for this without filing this issue because it's very simple and not much to discuss (even though it is expected), but failed to create a branch T.T

We'd be happy to accept a PR for this!

Generally, you do so by forking the repository first - due to how GitHub's permission model works, most OSS projects don't allow non-members to create branches on the main repository itself, so forking is the expected workflow for most contributions on GitHub.

Noah-Kennedy avatar Mar 07 '25 17:03 Noah-Kennedy

Thanks for the guide! Let me contribute for the first time in my life. I requested PR and linked this issue :)

PeppyDays avatar Mar 08 '25 06:03 PeppyDays