Skip to content

fix(toolbox-core)!: Simplify add_headers to make it sync in async client #231

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
May 16, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/toolbox-core/src/toolbox_core/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -308,11 +308,11 @@ async def load_toolset(

return tools

async def add_headers(
def add_headers(
self, headers: Mapping[str, Union[Callable, Coroutine, str]]
) -> None:
"""
Asynchronously Add headers to be included in each request sent through this client.
Add headers to be included in each request sent through this client.

Args:
headers: Headers to include in each request sent through this client.
Expand Down
9 changes: 3 additions & 6 deletions packages/toolbox-core/src/toolbox_core/sync_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ def load_toolset(
if not self.__loop or not self.__thread:
raise ValueError("Background loop or thread cannot be None.")

async_tools = asyncio.run_coroutine_threadsafe(coro, self.__loop).result() # type: ignore
async_tools = asyncio.run_coroutine_threadsafe(coro, self.__loop).result()
return [
ToolboxSyncTool(async_tool, self.__loop, self.__thread)
for async_tool in async_tools
Expand All @@ -154,18 +154,15 @@ def add_headers(
self, headers: Mapping[str, Union[Callable, Coroutine, str]]
) -> None:
"""
Synchronously Add headers to be included in each request sent through this client.
Add headers to be included in each request sent through this client.

Args:
headers: Headers to include in each request sent through this client.

Raises:
ValueError: If any of the headers are already registered in the client.
"""
coro = self.__async_client.add_headers(headers)

# We have already created a new loop in the init method in case it does not already exist
asyncio.run_coroutine_threadsafe(coro, self.__loop).result() # type: ignore
self.__async_client.add_headers(headers)

def __enter__(self):
"""Enter the runtime context related to this client instance."""
Expand Down
2 changes: 1 addition & 1 deletion packages/toolbox-core/tests/test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -1443,7 +1443,7 @@ async def test_add_headers_success(
)

async with ToolboxClient(TEST_BASE_URL) as client:
await client.add_headers(static_header)
client.add_headers(static_header)
assert client._ToolboxClient__client_headers == static_header

tool = await client.load_tool(tool_name)
Expand Down
44 changes: 30 additions & 14 deletions packages/toolbox-core/tests/test_sync_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

import inspect
from typing import Any, Callable, Mapping, Optional
from unittest.mock import AsyncMock, patch
from unittest.mock import AsyncMock, Mock, patch

import pytest
from aioresponses import CallbackResult, aioresponses
Expand Down Expand Up @@ -44,8 +44,12 @@ def sync_client_environment():
# This ensures any client created will start a new loop/thread.

# Ensure no loop/thread is running from a previous misbehaving test or setup
assert original_loop is None or not original_loop.is_running()
assert original_thread is None or not original_thread.is_alive()
if original_loop and original_loop.is_running():
original_loop.call_soon_threadsafe(original_loop.stop)
if original_thread and original_thread.is_alive():
original_thread.join(timeout=5)
ToolboxSyncClient._ToolboxSyncClient__loop = None
ToolboxSyncClient._ToolboxSyncClient__thread = None

ToolboxSyncClient._ToolboxSyncClient__loop = None
ToolboxSyncClient._ToolboxSyncClient__thread = None
Expand Down Expand Up @@ -408,20 +412,32 @@ def post_callback(url, **kwargs):
result = tool(param1="test")
assert result == expected_payload["result"]

@pytest.mark.usefixtures("sync_client_environment")
def test_sync_add_headers_duplicate_fail(self):
"""
Tests that adding a duplicate header via add_headers raises ValueError.
Manually create client to control initial headers.
"""
"""Tests that adding a duplicate header via add_headers raises ValueError (from async client)."""
initial_headers = {"X-Initial-Header": "initial_value"}
mock_async_client = AsyncMock(spec=ToolboxClient)

with ToolboxSyncClient(TEST_BASE_URL, client_headers=initial_headers) as client:
with pytest.raises(
ValueError,
match="Client header\\(s\\) `X-Initial-Header` already registered",
):
client.add_headers({"X-Initial-Header": "another_value"})
# Configure add_headers to simulate the ValueError from ToolboxClient
def mock_add_headers(headers):
# Simulate ToolboxClient's check
if "X-Initial-Header" in headers:
raise ValueError(
"Client header(s) `X-Initial-Header` already registered"
)

mock_async_client.add_headers = Mock(side_effect=mock_add_headers)

with patch(
"toolbox_core.sync_client.ToolboxClient", return_value=mock_async_client
):
with ToolboxSyncClient(
TEST_BASE_URL, client_headers=initial_headers
) as client:
with pytest.raises(
ValueError,
match="Client header\\(s\\) `X-Initial-Header` already registered",
):
client.add_headers({"X-Initial-Header": "another_value"})


class TestSyncAuth:
Expand Down