feat: make Client.headers case-insensitive

[thomasht86]

Summary

• Add CaseInsensitiveDict wrapper class that allows case-insensitive key access
• Override Client.headers property to return headers wrapped in CaseInsensitiveDict
• client.headers["X-Custom"], client.headers["x-custom"], and client.headers["X-CUSTOM"] all work now

Test plan

• Added test_client_headers_case_insensitive test covering __getitem__, __contains__, and get() methods
• All existing tests pass
• mypy and ruff checks pass

🤖 Generated with Claude Code

[claude]

Code Review: feat: make Client.headers case-insensitive

Summary

This PR adds case-insensitive header access to Client.headers by introducing a CaseInsensitiveDict wrapper class. The implementation is straightforward and aligns with HTTP/2 requirements.

Positive Aspects ✅

1. Good test coverage: The new test test_client_headers_case_insensitive covers the key methods (__getitem__, __contains__, get()) thoroughly.

2. Consistent with existing patterns: The implementation follows the existing codebase pattern where headers are internally lowercased (as seen in the Rust get_headers() method at src/lib.rs:251-259).

3. Minimal changes: The solution is clean and doesn't require modifying the Rust core, keeping changes localized to the Python wrapper.

4. Automatic inheritance: AsyncClient automatically inherits this behavior since it extends Client.

5. Preserves equality checks: The test confirms that dict equality still works with lowercased keys (client.headers == {"x-custom": "value", ...}).

Issues Found 🐛

1. Missing dict methods (Medium Priority)

CaseInsensitiveDict only overrides 3 methods, but Python's dict has many more that accept keys:

• __setitem__: headers["X-Custom"] = "value" won't work case-insensitively
• __delitem__: del headers["X-Custom"] won't work case-insensitively
• pop(), popitem(), setdefault(), update(): All accept keys

Impact: Users might expect to SET headers case-insensitively too, not just read them. However, since the getter creates a new dict each time (line 227), modifications don't affect the underlying client anyway. This is actually correct behavior (prevents confusion), but could be documented.

Recommendation:

• Document that the returned dict is read-only (modifications don't affect client)
• If mutation is needed, users should use client.headers = {...} instead
• Consider whether __setitem__/__delitem__ should raise informative errors

2. Property getter creates new dict on every access (Performance)

Line 227: return CaseInsensitiveDict(super().headers)

This creates a new wrapper dict on EVERY access. While unlikely to be a bottleneck, repeated access in loops could be inefficient:

for _ in range(1000):
    val = client.headers["Content-Type"]  # Creates 1000 dict wrappers

Recommendation: Document this behavior or consider caching if performance becomes an issue.

3. Missing type annotations (Low Priority)

CaseInsensitiveDict methods lack type hints:

def __getitem__(self, key):  # Should be: (self, key: str) -> str

Recommendation: Add type annotations for IDE support and type checking consistency.

4. No test for items(), keys(), values() (Low Priority)

While these methods work (they inherit from dict), the test doesn't verify that iterating over headers works correctly.

Recommendation: Add test cases:

assert list(client.headers.keys()) == ["x-custom", "content-type"]
assert list(client.headers.items()) == [("x-custom", "value"), ...]

Security Considerations 🔒

No security concerns identified. The case-insensitive lookup matches HTTP/2 spec requirements.

Performance Considerations ⚡

• Property creates new dict wrapper on each access (see Issue Improve CI strategy #2)
• For typical usage patterns (occasional header access), overhead is negligible
• Underlying Rust implementation remains unchanged and fast

Suggestions for Improvement 💡

1. Document read-only nature in the docstring:

@property
def headers(self) -> dict[str, str]:
    """
    Headers configured for this client (case-insensitive access).
    
    Note: Returns a read-only view. To modify headers, assign a new dict:
        client.headers = {"X-Custom": "value"}
    
    Case-insensitive access:
        client.headers["content-type"]  # Works
        client.headers["Content-Type"]  # Also works
    """

2. Add type hints to CaseInsensitiveDict:

def __getitem__(self, key: str) -> str:
    return super().__getitem__(key.lower())

def __contains__(self, key: str) -> bool:  # type: ignore[override]
    return super().__contains__(key.lower())

def get(self, key: str, default: str | None = None) -> str | None:
    return super().get(key.lower(), default)

3. Consider raising NotImplementedError for mutation methods (optional):

def __setitem__(self, key: str, value: str) -> None:
    raise TypeError(
        "Cannot modify headers directly. Use 'client.headers = {...}' instead."
    )

Test Coverage ✅

• ✅ Case-insensitive __getitem__
• ✅ Case-insensitive __contains__
• ✅ Case-insensitive get()
• ✅ Dict equality with lowercased keys
• ✅ Integration with existing test_client_setters
• ⚠️ Missing: keys(), values(), items() iteration
• ⚠️ Missing: Behavior when trying to mutate (should document or test)

Verdict

Approved with minor suggestions ✅

The implementation is solid and solves the stated problem. The issues identified are mostly about edge cases and documentation rather than core functionality. The PR is ready to merge after considering the suggestions above (particularly adding type hints and improving documentation).

Great work maintaining backward compatibility while improving the API ergonomics!

---

Review generated by Claude Code

[thomasht86]

but @claude as CaseInsensitiveDict subclasses dict all the other methods are supported still, right..?

[claude]

Claude finished @thomasht86's task in 54s —— View job

---

Clarifying the CaseInsensitiveDict Implementation

Great question! You're absolutely right - I need to clarify my earlier review comment.

The Reality

Yes, since CaseInsensitiveDict subclasses dict, all dict methods ARE supported and work correctly. The class inherits everything from dict:

• ✅ __setitem__ works: headers["X-Custom"] = "value"
• ✅ __delitem__ works: del headers["X-Custom"]
• ✅ pop(), popitem(), setdefault(), update() all work
• ✅ keys(), values(), items() all work

The Important Nuance

However, only the 3 overridden methods have case-insensitive behavior:

# Case-insensitive (overridden methods):
headers["Content-Type"]  # ✅ Works - __getitem__ is overridden
headers["content-type"]  # ✅ Works - lowercases the key
"Content-Type" in headers  # ✅ Works - __contains__ is overridden
headers.get("Content-Type")  # ✅ Works - get() is overridden

# NOT case-insensitive (inherited methods):
headers["Content-Type"] = "text/html"  # ⚠️ Sets key as "content-type" (dict stores lowercased)
del headers["Content-Type"]  # ⚠️ KeyError - looking for "Content-Type", but stored as "content-type"
headers.pop("Content-Type")  # ⚠️ KeyError - same issue

Why This is Actually Fine

Looking at line 227 in httpr/__init__.py:

@property
def headers(self) -> dict[str, str]:
    """Headers configured for this client (case-insensitive access)."""
    return CaseInsensitiveDict(super().headers)

The property creates a new dict wrapper on every access. This means:

1. Mutations don't affect the client anyway - any modification to the returned dict is lost
2. The returned dict already has lowercased keys - coming from Rust's get_headers() which lowercases them (see src/lib.rs:251-259)
3. This is intentionally read-only behavior - users should use client.headers = {...} to modify headers

My Original Review Was Unclear

My concern about "missing dict methods" wasn't quite right. The methods aren't missing - they're just not case-insensitive. But since:

• The underlying dict already has lowercased keys
• The property returns a new wrapper each time (preventing mutations from affecting the client)
• The intended use case is reading headers, not modifying them

...the implementation is correct as-is!

Minor Suggestion: Documentation

The only improvement I'd suggest is adding a note to the docstring to make the read-only nature explicit:

@property
def headers(self) -> dict[str, str]:
    """
    Headers configured for this client (case-insensitive access).
    
    Note: Returns a read-only view. Modifications to the returned dict 
    won't affect the client. To modify headers, assign a new dict:
        client.headers = {"X-Custom": "value"}
    
    Case-insensitive access works for reading:
        client.headers["content-type"]  # Works
        client.headers["Content-Type"]  # Also works
    """
    return CaseInsensitiveDict(super().headers)

Conclusion

You're right - my review comment was misleading. The implementation is solid! The only thing "missing" is explicit documentation about the read-only nature, but the code itself works correctly for its intended purpose.

Verdict: Approved ✅ The PR is good to merge as-is, with optional documentation enhancement.

---

[claude]

PR Review: Make Client.headers Case-Insensitive

Summary

This PR adds a CaseInsensitiveDict wrapper class and overrides the Client.headers property to enable case-insensitive header access. The implementation is well-tested and aligns with the goal of providing a more user-friendly API.

---

✅ Strengths

1. Good test coverage: The new test test_client_headers_case_insensitive covers all major dictionary operations (__getitem__, __contains__, get(), pop(), setdefault()).

2. Clear purpose: Makes the API more intuitive since HTTP headers are case-insensitive by spec (RFC 7230).

3. Backwards compatible: Existing code will continue to work since the underlying storage is already lowercased.

---

🔍 Issues & Concerns

1. Inconsistency with existing architecture

The codebase already has CaseInsensitiveHeaderMap in Rust (used for response headers at src/response.rs:28). Creating a separate Python-only CaseInsensitiveDict creates architectural inconsistency:

• Response headers: Use CaseInsensitiveHeaderMap (Rust)
• Client headers: Would use CaseInsensitiveDict (Python)

Recommendation: Consider either:

• Using the existing CaseInsensitiveHeaderMap for client headers too
• Or document why a separate implementation is needed

2. get() method return type mismatch

def get(self, key, default=None):
    return super().get(key.lower(), default)

Issue: If a header doesn't exist and default=None, this returns None. But the super().get() call could potentially return the default value even when the key exists if the value stored is None.

Example edge case:

d = CaseInsensitiveDict({"x-test": None})
result = d.get("X-Test")  # Returns None (correct)
result = d.get("X-Missing")  # Also returns None (correct)
# But can't distinguish between "key exists with None value" vs "key missing"

This is probably acceptable since HTTP header values shouldn't be None, but worth noting.

3. Type annotations missing

The CaseInsensitiveDict class lacks type hints:

class CaseInsensitiveDict(dict):
    def __getitem__(self, key):  # Should be: def __getitem__(self, key: str) -> str
        return super().__getitem__(key.lower())

Recommendation: Add type annotations for better IDE support and type checking consistency.

4. AttributeError risk on non-string keys

def __getitem__(self, key):
    return super().__getitem__(key.lower())  # Will crash if key is not a string

While HTTP header keys should always be strings, defensive programming would suggest either:

• Adding type validation
• Or documenting that keys must be strings

Edge case:

headers = client.headers
headers[123] = "value"  # Would crash with AttributeError: 'int' object has no attribute 'lower'

5. Performance consideration

Every access to client.headers creates a new CaseInsensitiveDict instance:

@property
def headers(self) -> dict[str, str]:
    return CaseInsensitiveDict(super().headers)  # Creates new dict every time

Example:

client.headers["x-test"]  # Creates new dict
client.headers["x-other"]  # Creates another new dict

This is probably fine for typical use cases, but could be optimized by caching or documenting that users should store the reference:

# Less efficient
client.headers["x-test"]
client.headers["x-other"]

# More efficient
headers = client.headers
headers["x-test"]
headers["x-other"]

6. Mutation behavior might be confusing

The test shows that mutating the returned dict doesn't affect the client:

headers = client.headers
headers["X-New"] = "new-value"
# This doesn't actually set the header on the client!

This is technically correct (you're mutating a copy), but could be surprising to users who expect:

client.headers["X-Test"] = "value"  # Doesn't actually work as expected

To set headers, users must still do:

client.headers = {"X-Test": "value"}

Recommendation: Add a comment or docstring clarifying this behavior.

---

📝 Minor Issues

1. Test assertion could be more precise: Line 341 in test file:

assert client.headers == {"x-custom": "value", "content-type": "application/json"}

This works because dict equality compares keys/values, but it's comparing a CaseInsensitiveDict to a regular dict. Consider making this explicit in a comment.

2. Missing test for setter interaction: The test doesn't verify that the case-insensitive access still works after using the setter:

client.headers = {"X-New": "value"}
assert client.headers["x-new"] == "value"  # Should test this

---

🔒 Security

No security concerns identified.

---

🎯 Recommendations

High Priority:

1. Add type annotations to CaseInsensitiveDict
2. Document the architectural decision (why not reuse CaseInsensitiveHeaderMap?)
3. Add docstring to explain mutation behavior

Medium Priority:
4. Add defensive check for non-string keys or document the requirement
5. Consider adding a test for setter + case-insensitive getter interaction

Low Priority:
6. Document the performance characteristic of creating new dict instances on each access

---

✅ Verdict

The PR is functional and well-tested, but would benefit from better type safety and documentation. The code achieves its goal, though the architectural inconsistency with CaseInsensitiveHeaderMap is worth discussing.

Suggested Action: Address high-priority items before merging, consider medium-priority improvements for better robustness.

[claude]

Code Review: PR #54 - Make Client.headers case-insensitive

Summary

This PR adds case-insensitive header access to Client.headers by introducing a CaseInsensitiveDict wrapper. The implementation is functional and well-tested, but there are several important issues to address.

---

🔴 Critical Issues

1. Breaking change: Lost original casing

The current implementation lowercases ALL keys when wrapping, which loses the original casing information from the Rust layer.

When super().headers is called, it returns headers that are already lowercased by Rust (per HTTP/2 spec). The CaseInsensitiveDict then stores them with lowercase keys.

Problem: While response headers use CaseInsensitiveHeaderMap (which preserves original casing via dual-map structure), client headers don't preserve casing at all.

Impact:

• client.headers will always show lowercase keys in iteration/repr
• Inconsistent with response.headers behavior which preserves original casing
• Test at line 342 expects lowercase, confirming this behavior

Recommendation: Document this behavior clearly, or consider whether client headers should preserve original casing like response headers do.

2. Performance: Creates new dict wrapper on every access

Every time client.headers is accessed, a new CaseInsensitiveDict is created. This means unnecessary allocations in hot paths, especially problematic if users check headers frequently.

Recommendations:

1. Cache the wrapper dict (but invalidate on client.headers = new_headers)
2. Document that users should store reference: headers = client.headers then use headers["foo"]
3. Consider if the Rust layer should return CaseInsensitiveHeaderMap instead of plain dict

---

⚠️ Design Concerns

3. Mutation behavior is confusing

The test shows mutations to the returned dict don't affect the client. This is correct given the current implementation (returns a copy), but could surprise users expecting reference semantics.

Recommendation: Document this clearly in docstring. Consider whether mutations should raise an error (make it a read-only view) to fail fast instead of silently ignoring.

4. Inconsistency with Rust's CaseInsensitiveHeaderMap

The Rust implementation (src/response.rs:33-133) uses a sophisticated dual-map structure to preserve original casing. The Python CaseInsensitiveDict simply lowercases everything, losing the original casing.

Question: Why not use CaseInsensitiveHeaderMap from Rust for client headers too? This would preserve original casing, avoid Python overhead, and unify the implementation.

5. Missing type annotations

The CaseInsensitiveDict methods lack type hints, reducing IDE support and type safety.

---

✅ Strengths

1. Excellent test coverage: The test thoroughly exercises all case-insensitive methods
2. Consistent interface: Matches CaseInsensitiveHeaderMap API surface area
3. Non-invasive: Doesn't change Rust code or break existing API
4. Solves real UX issue: Case-sensitive client headers was a papercut

---

📝 Suggested Improvements

1. Add type hints to CaseInsensitiveDict methods
2. Update type stubs (httpr.pyi) to document case-insensitive behavior and that returned dict is a copy
3. Consider caching the wrapper to avoid repeated allocations
4. Document that keys are stored lowercase and mutations don't affect the client

---

🎯 Verdict

Approve with reservations. The implementation works and adds value, but please consider:

1. Document the casing behavior (keys are lowercase)
2. Consider performance implications of creating wrappers on every access
3. Add type hints to CaseInsensitiveDict
4. Evaluate whether to use Rust's CaseInsensitiveHeaderMap for consistency

The test coverage is excellent. Nice work! 👍

Questions:

1. Was using CaseInsensitiveHeaderMap from Rust considered?
2. Is the performance overhead of creating a new dict on every access acceptable?
3. Should mutations to the returned dict raise errors instead of being silently ignored?

[claude]

Code Review

Thank you for implementing case-insensitive access for Client.headers! This is a useful feature that improves the API usability. However, I've identified several issues that need to be addressed before this can be merged.

Critical Issues

1. Architectural Inconsistency - Duplicating Existing Rust Implementation

The codebase already has a CaseInsensitiveHeaderMap class implemented in Rust (src/response.rs:27-91) that provides:

• Case-insensitive __getitem__
• Case-insensitive __contains__
• Case-insensitive get()
• Preserves original header casing for iteration
• Keys(), values(), items() methods

Problem: This PR introduces a second, separate Python implementation (CaseInsensitiveDict) that duplicates this functionality. This creates:

• Code duplication and maintenance burden
• Two different implementations that could behave differently
• Confusion about which to use

Recommendation: Modify the Rust RClient.get_headers() method (src/lib.rs:251-259) to return a CaseInsensitiveHeaderMap instead of a plain IndexMapSSR. This would:

• Reuse the existing, battle-tested implementation
• Keep all header handling logic in one place
• Ensure consistency between Response.headers and Client.headers

---

2. Performance Issue - Creating New Dict on Every Access

@property
def headers(self) -> dict[str, str]:
    return CaseInsensitiveDict(super().headers)

Problem: Every time client.headers is accessed, a new CaseInsensitiveDict is created and all headers are copied. For code that accesses headers in a loop, this creates unnecessary allocations.

Example:

# This creates TWO separate CaseInsensitiveDict instances
auth_header = client.headers.get("Authorization")
content_type = client.headers.get("Content-Type")

Impact: If using the Rust CaseInsensitiveHeaderMap approach, this issue is avoided since the Rust object can be returned directly without copying.

---

3. Incomplete Implementation - Missing Methods

The CaseInsensitiveDict implements some dict methods but not all:

Missing:

• __len__
• __repr__
• __eq__
• clear()
• copy()

Example broken behavior:

headers = client.headers
len(headers)  # Works (inherited from dict)
print(headers)  # Shows lowercased keys only (confusing\!)
headers == {"X-Custom": "value"}  # False (keys are lowercase internally)

The Rust CaseInsensitiveHeaderMap already handles these correctly.

---

Minor Issues

4. Type Stub Missing

The CaseInsensitiveDict class should be added to httpr.pyi for proper IDE support. However, if using the existing Rust class, only the return type annotation needs updating.

---

5. Setter Behavior Unclear

@headers.setter
def headers(self, value: dict[str, str] | None) -> None:
    RClient.headers.__set__(self, value)

The setter uses RClient.headers.__set__ which:

1. Is accessing a private descriptor protocol method directly (unusual pattern)
2. Doesn't lowercase keys before setting (inconsistent with getter)

Test this scenario:

client.headers = {"X-Custom": "value"}
print(client.headers["x-custom"])  # Should this work? Currently might not.

---

Testing Gaps

The test coverage is good for the happy path, but missing:

• Edge cases: empty headers, None values
• Setter behavior validation
• Performance implications (multiple accesses)
• Integration with actual requests

---

Recommended Approach

Instead of this PR's approach, I recommend:

1. Modify the Rust getter in src/lib.rs:251-259:

   #[getter]
   pub fn get_headers(&self) -> PyResult<CaseInsensitiveHeaderMap> {
       let headers = self.headers.lock()
           .map_err(|e| map_anyhow_error(anyhow\!("Failed to acquire headers lock: {}", e)))?;
       let mut header_map = CaseInsensitiveHeaderMap::create();
       for (k, v) in headers.iter() {
           if k \!= COOKIE {  // Exclude Cookie header as before
               header_map.insert(k.as_str().to_owned(), 
                                v.to_str().unwrap_or_default().to_owned());
           }
       }
       Ok(header_map)
   }

2. Remove the Python wrapper property - no override needed, the Rust getter provides case-insensitive access directly

3. Update type stubs in httpr.pyi to show return type as CaseInsensitiveHeaderMap

This approach:

• ✅ Reuses existing, tested code
• ✅ No performance overhead
• ✅ Consistent with Response.headers behavior
• ✅ All dict methods work correctly
• ✅ Simpler implementation

---

Summary

While the intent of this PR is excellent, the implementation creates technical debt by duplicating functionality that already exists in the Rust layer. Please consider refactoring to use the existing CaseInsensitiveHeaderMap class instead.

Let me know if you'd like help implementing the recommended approach!