# GitHub Issue Comments

## Issue #82: Add HttpMiddleware trait for transport-level HTTP concerns

**Status: RESOLVED ✅**

---

### Closing Comment

This issue has been fully resolved! 🎉

## What Was Delivered

### Core HttpMiddleware Infrastructure

✅ **`HttpMiddleware` trait** with comprehensive lifecycle hooks:
```rust
#[async_trait]
pub trait HttpMiddleware: Send + Sync {
    async fn on_request(&self, request: &mut HttpRequest, context: &HttpMiddlewareContext) -> Result<()>;
    async fn on_response(&self, response: &mut HttpResponse, context: &HttpMiddlewareContext) -> Result<()>;
    async fn on_error(&self, error: &Error, context: &HttpMiddlewareContext) -> Result<()>;
    fn priority(&self) -> i32;
    async fn should_execute(&self, context: &HttpMiddlewareContext) -> bool;
}
```

✅ **`HttpRequest` and `HttpResponse` abstractions**:
- Case-insensitive header operations (HTTP RFC 7230 compliant)
- Clean API: `add_header()`, `get_header()`, `has_header()`, `remove_header()`
- Works with `HashMap<String, String>` internally

✅ **`HttpMiddlewareContext`**:
- Request metadata (url, method, attempt number)
- Shareable metadata store for middleware coordination
- Request ID correlation support

✅ **`HttpMiddlewareChain`**:
- Priority-based execution ordering
- Short-circuit error semantics
- Reverse order for response processing
- Comprehensive error handling with `on_error` hooks

### Integration with StreamableHttpTransport

✅ **Automatic middleware invocation** in `StreamableHttpTransport`:
- `build_request_with_middleware()` - Applied before HTTP send (lines 339-435)
- `apply_response_middleware()` - Applied after HTTP receive (lines 437-470)
- Fast path optimization: Skips middleware when none configured
- OAuth precedence: `auth_provider > HttpMiddleware > extra_headers`

✅ **Configuration API**:
```rust
let mut http_chain = HttpMiddlewareChain::new();
http_chain.add(Arc::new(OAuthClientMiddleware::new(token)));

let config = StreamableHttpTransportConfig {
    url: Url::parse("http://localhost:8080").unwrap(),
    http_middleware_chain: Some(Arc::new(http_chain)), // ← Integration point
    // ...
};

let transport = StreamableHttpTransport::new(config);
let client = ClientBuilder::new(transport).build();

// Middleware runs automatically - no manual calls needed!
```

## Use Cases Demonstrated

### 1. Header Injection ✅
See `examples/40_middleware_demo.rs` - `CorrelationHeaderMiddleware`

### 2. Request/Response Logging ✅
Built-in with comprehensive header tracking

### 3. OAuth Token Injection ✅
`OAuthClientMiddleware` (see issue #83)

### 4. Status Code Handling ✅
`on_response` hook with access to status codes

## Testing

✅ **23 comprehensive integration tests**:
- 14 HTTP middleware integration tests (`tests/http_middleware_integration.rs`)
  - Middleware ordering/priority
  - OAuth flows (no provider, expired token, duplicate headers)
  - Concurrency and state isolation
  - Error handling and short-circuit behavior
  - Header case-insensitivity
  - Double-retry protection

- 4 SSE middleware tests (`tests/sse_middleware_integration.rs`)
  - Middleware on SSE GET requests
  - Last-Event-ID header tracking
  - Multiple HTTP methods
  - Response status tracking

- 5 OAuth integration tests (`tests/streamable_http_oauth_integration.rs`)
  - Real client-server interaction
  - Token injection and precedence
  - Token expiry error handling

## Documentation

✅ **Chapter 11: Middleware** in pmcp-book
- HTTP-Level Middleware section with architecture diagram
- Two-layer middleware system (Protocol vs HTTP)
- API examples and use cases

✅ **Chapter 10.3: Streamable HTTP**
- HTTP middleware configuration examples
- OAuth middleware integration guide

✅ **API documentation**
- Complete rustdoc with examples
- Type-level documentation for all public APIs

## Extra Features Beyond Original Request

🎁 **Fast path optimization**: Avoids overhead when no middleware configured
🎁 **OAuth precedence policy**: Ensures `auth_provider` takes precedence
🎁 **Metadata propagation**: Allows middleware coordination (e.g., retry protection)
🎁 **Case-insensitive headers**: HTTP standard compliance
🎁 **Error hooks**: Comprehensive error handling with `on_error`

## Files Changed

**Core Implementation:**
- `src/client/http_middleware.rs` - HttpMiddleware trait, chain, abstractions (356 lines)
- `src/shared/streamable_http.rs` - Integration with transport (lines 333-470)
- `src/client/oauth_middleware.rs` - OAuth client middleware (244 lines)

**Tests:**
- `tests/http_middleware_integration.rs` (764 lines)
- `tests/sse_middleware_integration.rs` (421 lines)
- `tests/streamable_http_oauth_integration.rs` (361 lines)

**Documentation:**
- `pmcp-book/src/ch11-middleware.md` - HTTP middleware section
- `pmcp-book/src/ch10-03-streamable-http.md` - Integration guide

**Examples:**
- `examples/40_middleware_demo.rs` - Full demonstration

## Commits

- Phase 1 Foundation: d024af8
- OAuth Integration Tests: 57349e4
- Phase 2 Complete: 9036e5a
- Quality Gate Fixes: 4dc71df

## Related Issues

- ✅ Resolves #82 (this issue)
- ✅ Resolves #83 (OAuth client middleware - see separate comment)
- ⚠️ Partial progress on #80 (HTTP middleware integration complete, ClientBuilder API still pending)

---

This provides a solid foundation for HTTP-level concerns in the PMCP SDK and achieves feature parity with the TypeScript SDK's fetch-wrapping middleware! 🚀

---

## Issue #83: Add OAuth client-side middleware with token injection and auto-refresh

**Status: CORE FEATURES COMPLETE ✅ (Future enhancements noted)**

---

### Closing Comment

The core OAuth client middleware features have been implemented! 🎉

## What Was Delivered

### OAuthClientMiddleware Implementation

✅ **Automatic bearer token injection**:
```rust
pub struct OAuthClientMiddleware {
    token: Arc<RwLock<BearerToken>>,
}

pub struct BearerToken {
    pub token: String,
    pub token_type: String,
    pub expires_at: Option<SystemTime>,
}

impl BearerToken {
    pub fn new(token: String) -> Self;
    pub fn with_expiry(token: String, duration: Duration) -> Self;
    pub fn is_expired(&self) -> bool;
    pub fn to_header_value(&self) -> String;
}
```

✅ **Token lifecycle features**:
- Expiry tracking with `expires_at` field
- Proactive expiry checking in `on_request`
- Returns `Error::Authentication` when token expired
- `is_expired()` method for manual checking

✅ **401/403 detection and metadata**:
- Detects authentication failures in `on_response`
- Sets `auth_failure: true` metadata for retry coordination
- Sets `status_code` metadata for debugging
- Logs warnings on auth failure after retry

✅ **Smart precedence handling**:
- Skips injection if `auth_provider` present (set via metadata)
- Skips if Authorization header already exists (case-insensitive)
- Prevents duplicate authentication headers
- Clean integration with transport-level auth

✅ **Retry coordination**:
- Sets `oauth.retry_used` metadata for double-retry protection
- Prevents infinite retry loops
- Coordinates with external retry middleware
- Uses `on_error` hook for proper error-chain handling

### Integration

✅ **Works with StreamableHttpTransport**:
```rust
use pmcp::client::http_middleware::HttpMiddlewareChain;
use pmcp::client::oauth_middleware::{OAuthClientMiddleware, BearerToken};
use std::time::Duration;

// Create OAuth middleware
let token = BearerToken::with_expiry(
    "api-token-12345".to_string(),
    Duration::from_secs(3600)
);
let mut http_chain = HttpMiddlewareChain::new();
http_chain.add(Arc::new(OAuthClientMiddleware::new(token)));

// Configure transport
let config = StreamableHttpTransportConfig {
    url: Url::parse("http://localhost:8080").unwrap(),
    http_middleware_chain: Some(Arc::new(http_chain)),
    // ...
};

let transport = StreamableHttpTransport::new(config);
let mut client = ClientBuilder::new(transport).build();

// Token automatically injected into all requests!
let info = client.initialize(ClientCapabilities::minimal()).await?;
```

## Testing

✅ **5 OAuth integration tests** (`tests/streamable_http_oauth_integration.rs`):
- `test_oauth_middleware_injects_token` - Verifies automatic token injection
- `test_auth_provider_takes_precedence_over_oauth` - Validates precedence policy
- `test_oauth_token_expiry_triggers_error` - Token expiry handling
- `test_multiple_requests_with_oauth` - Sequential requests with same token
- `test_oauth_with_case_insensitive_header_check` - Duplicate detection

✅ **3 OAuth retry coordination tests** (`tests/http_middleware_integration.rs`):
- `test_oauth_retry_coordination_with_metadata` - Metadata-based coordination
- `test_double_retry_protection` - Prevents infinite loops
- `test_oauth_error_hook_logging` - Error hook integration

✅ **Real client-server integration**:
- Tests use actual `StreamableHttpServer` instances
- Validates end-to-end token flow
- Confirms server receives injected headers

## Documentation

✅ **Chapter 11: Middleware** - OAuth middleware section
✅ **Chapter 10.3: Streamable HTTP** - OAuth configuration examples
✅ **API documentation** - Complete rustdoc with examples

## What's Not Implemented (Future Enhancements)

The following features from the original issue are **not yet implemented** but can be added in future PRs:

### 1. Automatic Token Refresh ⚠️

**Current behavior**: Returns `Error::Authentication` when token expires
**Desired behavior**: Automatically refresh token and retry request

**Future implementation approach**:
```rust
pub trait TokenProvider: Send + Sync {
    async fn get_token(&self) -> Result<AccessToken>;
    async fn refresh_token(&self, current: &AccessToken) -> Result<AccessToken>;
}

pub struct OAuthClientMiddleware {
    token_provider: Arc<dyn TokenProvider>,
    // Automatically refreshes on expiry or 401
}
```

### 2. Multiple OAuth Flows ⚠️

**Current support**: Bearer token only
**Future flows**:
- Client credentials (client_id + client_secret → token)
- Refresh token (refresh_token → new access_token)
- Device code flow
- Custom token providers

### 3. Automatic Retry After 401 ⚠️

**Current behavior**: Sets metadata, app handles retry
**Desired behavior**: Middleware automatically retries after refresh

**Workaround**: Use external retry middleware that checks `auth_failure` metadata

### 4. Token Storage Strategies ⚠️

**Current support**: In-memory only
**Future strategies**:
- Persistent storage (file, keychain)
- Encrypted token storage
- Shared token cache across clients

### 5. Proactive Token Refresh ⚠️

**Partial support**: Can check expiry before request
**Enhancement**: Automatically refresh N seconds before expiry

## Why Close This Issue?

The **core value proposition** of this issue has been delivered:
- ✅ OAuth tokens are automatically injected
- ✅ Token expiry is tracked and validated
- ✅ 401/403 responses are detected
- ✅ Retry coordination infrastructure is in place

The missing features (auto-refresh, multiple flows) are valuable enhancements but can be added incrementally without blocking other work. The current implementation provides a solid foundation that users can build upon.

## Recommendation

**Close this issue** and open separate enhancement issues for:
1. "Add automatic token refresh to OAuthClientMiddleware" (#TODO)
2. "Support multiple OAuth flows (client credentials, refresh token)" (#TODO)
3. "Add TokenProvider trait for custom OAuth flows" (#TODO)

This allows tracking specific enhancements independently while acknowledging the core feature is complete.

## Files Changed

**Implementation:**
- `src/client/oauth_middleware.rs` (244 lines)
- `src/client/http_middleware.rs` (trait definition)

**Tests:**
- `tests/streamable_http_oauth_integration.rs` (361 lines)
- `tests/http_middleware_integration.rs` (OAuth sections)

**Documentation:**
- `pmcp-book/src/ch11-middleware.md`
- `pmcp-book/src/ch10-03-streamable-http.md`

## Commits

- OAuth Middleware Implementation: 57349e4
- Phase 2 Integration Tests: 9036e5a
- Quality Gate Fixes: 4dc71df

## Related Issues

- ✅ Resolves #83 (this issue - core features)
- ✅ Resolves #82 (HttpMiddleware trait)
- ⚠️ Partial progress on #80 (HTTP middleware integration)

---

Great work positioning PMCP as the best MCP SDK! The OAuth middleware provides essential client-side authentication capabilities. 🚀

---

## Issue #80: Add first-class middleware integration to Client/Protocol/Transport

**Status: IN PROGRESS ⚠️ (HTTP layer complete, Client/Protocol layer pending)**

---

### Progress Update Comment

Significant progress has been made on this issue! The HTTP transport layer now has full first-class middleware integration.

## ✅ What's Been Completed

### HTTP-Level Middleware Integration (COMPLETE)

✅ **Transport-level configuration**:
```rust
use pmcp::client::http_middleware::HttpMiddlewareChain;
use pmcp::shared::streamable_http::{StreamableHttpTransport, StreamableHttpTransportConfig};

let mut http_chain = HttpMiddlewareChain::new();
http_chain.add(Arc::new(LoggingMiddleware::default()));
http_chain.add(Arc::new(OAuthClientMiddleware::new(token)));

let config = StreamableHttpTransportConfig {
    url: Url::parse("http://localhost:8080").unwrap(),
    http_middleware_chain: Some(Arc::new(http_chain)),  // ← First-class integration!
    // ...
};

let transport = StreamableHttpTransport::new(config);
let mut client = ClientBuilder::new(transport).build();

// Middleware runs automatically on ALL operations - zero manual calls!
let tools = client.list_tools(None).await?;  // Middleware runs transparently
```

✅ **Automatic invocation** - No manual `chain.process_request()` calls needed:
- `build_request_with_middleware()` - Called automatically before HTTP send
- `apply_response_middleware()` - Called automatically after HTTP receive
- Short-circuit error handling with `on_error` hooks
- Priority-based execution ordering

✅ **Clean architecture**:
- Configuration via `StreamableHttpTransportConfig::http_middleware_chain`
- Middleware chain is `Option<Arc<HttpMiddlewareChain>>` - zero overhead when unused
- Fast path optimization when no middleware configured

✅ **Production-ready features**:
- Error handling with `on_error` lifecycle
- Context metadata for middleware coordination
- Priority-based ordering (lower runs first)
- Conditional execution via `should_execute()`

### Test Coverage

✅ **23 integration tests** demonstrating automatic middleware invocation:
- Real client-server interactions
- Multiple middleware chaining
- Error scenarios
- Retry coordination

### Documentation

✅ **Chapter 11: Middleware** - Complete HTTP middleware section
✅ **Chapter 10.3: Streamable HTTP** - Integration guide
✅ **examples/40_middleware_demo.rs** - Full demonstration

## ⚠️ What's Still Pending

### 1. ClientBuilder API (HIGH PRIORITY)

**Desired API**:
```rust
let client = Client::builder()
    .capabilities(ClientCapabilities::default())
    .with_middleware_chain(chain)  // ← Doesn't exist yet
    .streamable_http_transport("http://localhost:8080")
    .await?
    .build()
    .await?;
```

**Current workaround**: Configure via transport config (works but less ergonomic)

**Implementation needed**:
- Add `middleware_chain: Option<Arc<MiddlewareChain>>` field to `ClientBuilder`
- Add `.with_middleware_chain()` builder method
- Pass middleware to transport during client construction
- Update all transport builders to accept middleware

### 2. Protocol-Level Middleware Auto-Invocation (MEDIUM PRIORITY)

**Current state**: Protocol middleware (`AdvancedMiddleware`) requires manual calls:
```rust
// Manual invocation required
let mut request = create_request("tools/list", None);
middleware_chain.process_request(&mut request).await?;
let response = client.call_method(request).await?;
middleware_chain.process_response(&mut response).await?;
```

**Desired state**: Automatic invocation like HTTP middleware

**Implementation needed**:
- Add middleware chain to `Protocol` struct
- Wrap all request/response paths with automatic middleware invocation
- Ensure backward compatibility

### 3. Unified Middleware Configuration (LOW PRIORITY)

**Desired**: Single configuration point for both HTTP and Protocol middleware
```rust
let client = Client::builder()
    .with_http_middleware(http_chain)      // HTTP layer
    .with_protocol_middleware(proto_chain) // Protocol layer
    .build();
```

**Benefit**: Clear separation of concerns, easier to understand

## Progress Summary

| Component | Status | Notes |
|-----------|--------|-------|
| HTTP middleware integration | ✅ **COMPLETE** | Full automatic invocation |
| HTTP middleware config API | ✅ **COMPLETE** | Via transport config |
| HTTP middleware tests | ✅ **COMPLETE** | 23 integration tests |
| HTTP middleware docs | ✅ **COMPLETE** | Chapter 11 + examples |
| ClientBuilder API | ❌ **TODO** | No `.with_middleware_chain()` |
| Protocol middleware auto-invoke | ❌ **TODO** | Still requires manual calls |
| Unified middleware config | ❌ **TODO** | Separate HTTP/Protocol APIs |

## Recommendation

**Keep this issue open** but update the title or description to reflect progress:

**Option 1**: Update title to focus on remaining work:
"Add ClientBuilder middleware API and Protocol-level auto-invocation"

**Option 2**: Create separate issues:
- New Issue: "Add ClientBuilder::with_middleware_chain() API" (#TODO)
- New Issue: "Add automatic Protocol middleware invocation" (#TODO)
- Close #80 noting HTTP layer is complete

**Option 3**: Use milestones:
- Milestone 1: HTTP-Level Integration ✅ **COMPLETE**
- Milestone 2: ClientBuilder API ⚠️ **In Progress**
- Milestone 3: Protocol-Level Integration ⚠️ **Pending**

## What Works Today

Users can **immediately** use HTTP middleware with this pattern:
```rust
use pmcp::client::http_middleware::HttpMiddlewareChain;
use pmcp::client::oauth_middleware::{OAuthClientMiddleware, BearerToken};
use pmcp::shared::streamable_http::{StreamableHttpTransport, StreamableHttpTransportConfig};
use std::sync::Arc;

let mut http_chain = HttpMiddlewareChain::new();
http_chain.add(Arc::new(OAuthClientMiddleware::new(token)));

let config = StreamableHttpTransportConfig {
    url: Url::parse("http://localhost:8080")?,
    http_middleware_chain: Some(Arc::new(http_chain)),
    // ... other config
};

let transport = StreamableHttpTransport::new(config);
let mut client = ClientBuilder::new(transport).build();

// All HTTP operations now have middleware applied automatically!
client.initialize(ClientCapabilities::minimal()).await?;
client.list_tools(None).await?;
```

This provides **immediate value** while we continue improving the ergonomics.

## Files Changed (HTTP Layer)

- `src/client/http_middleware.rs` (356 lines)
- `src/shared/streamable_http.rs` (integration code)
- `tests/http_middleware_integration.rs` (764 lines)
- `tests/sse_middleware_integration.rs` (421 lines)
- `tests/streamable_http_oauth_integration.rs` (361 lines)

## Next Steps

1. **Add ClientBuilder::with_middleware_chain()** - Improves ergonomics
2. **Add Protocol middleware auto-invocation** - Parity with HTTP layer
3. **Update documentation** - Show both patterns

## Related Issues

- ✅ #82 - HttpMiddleware trait (RESOLVED)
- ✅ #83 - OAuth client middleware (RESOLVED)
- ⚠️ #80 - This issue (IN PROGRESS - HTTP complete, Client/Protocol pending)

---

The HTTP middleware integration is production-ready and provides immediate value. The remaining work (ClientBuilder API, Protocol auto-invocation) will further improve ergonomics. 🚀

---

## Summary

**Issues ready to close:**
- ✅ **#82** - HttpMiddleware trait (fully implemented)
- ✅ **#83** - OAuth client middleware (core features complete)

**Issues to keep open:**
- ⚠️ **#80** - First-class integration (HTTP done, ClientBuilder/Protocol pending)

**New issues to create (optional):**
1. "Add automatic token refresh to OAuthClientMiddleware"
2. "Support multiple OAuth flows (client credentials, refresh token)"
3. "Add ClientBuilder::with_middleware_chain() API"
4. "Add automatic Protocol middleware invocation"
