Skip to content
Open
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
15 changes: 14 additions & 1 deletion horizon/facts/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
from horizon.startup.api_keys import get_env_api_key
from horizon.startup.remote_config import get_remote_config

CONSISTENT_UPDATE_HEADER = "X-Permit-Consistent-Update"
# Backend compares this value case-sensitively (`value == "true"`); keep coupled to the header name.
CONSISTENT_UPDATE_HEADER_VALUE = "true"


class FactsClient:
def __init__(self):
Expand All @@ -38,16 +42,21 @@ async def build_forward_request(
path: str,
*,
query_params: dict[str, Any] | None = None,
is_consistent_update: bool = False,
) -> HttpxRequest:
"""
Build an HTTPX request from a FastAPI request to forward to the facts service.
:param request: FastAPI request
:param path: Backend facts service path to forward to
:param is_consistent_update: if True, marks the request as a consistent update so the
backend skips the control-plane delta update (the PDP handles propagation locally).
:return: HTTPX request
"""
forward_headers = {
key: value for key, value in request.headers.items() if key.lower() in {"authorization", "content-type"}
}
if is_consistent_update:
forward_headers[CONSISTENT_UPDATE_HEADER] = CONSISTENT_UPDATE_HEADER_VALUE
remote_config = get_remote_config()
project_id = remote_config.context.get("project_id")
environment_id = remote_config.context.get("env_id")
Expand Down Expand Up @@ -77,14 +86,18 @@ async def send_forward_request(
path: str,
*,
query_params: dict[str, Any] | None = None,
is_consistent_update: bool = False,
) -> HttpxResponse:
"""
Send a forward request to the facts service.
:param request: FastAPI request
:param path: Backend facts service path to forward to
:param is_consistent_update: see build_forward_request.
:return: HTTPX response
"""
forward_request = await self.build_forward_request(request, path, query_params=query_params)
forward_request = await self.build_forward_request(
request, path, query_params=query_params, is_consistent_update=is_consistent_update
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

send_forward_request kwarg passthrough is untested

The kwarg plumbing from send_forward_request into build_forward_request has no direct test. The PR's purpose is propagating this flag across that boundary, so the boundary itself should be covered.

Suggestion: Add a test that patches FactsClient.send and asserts the built request carries X-Permit-Consistent-Update: true when send_forward_request(..., is_consistent_update=True) is called.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added test_send_forward_request_propagates_consistent_update_kwarg which patches FactsClient.send, calls send_forward_request(..., is_consistent_update=True), and asserts the built HttpxRequest carries X-Permit-Consistent-Update: true. Fixed in d02054a.

return await self.send(forward_request)

@staticmethod
Expand Down
2 changes: 1 addition & 1 deletion horizon/facts/router.py
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ async def forward_request_then_wait_for_update(
query_params: dict[str, Any] | None = None,
) -> Response:
_update_id = update_id or uuid4()
response = await client.send_forward_request(request, path, query_params=query_params)
response = await client.send_forward_request(request, path, query_params=query_params, is_consistent_update=True)
Comment thread
omer9564 marked this conversation as resolved.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Router integration is untested

The only place is_consistent_update=True is wired in is here, and nothing tests that the wait-for-update path passes True while the fallback at forward_remaining_requests (router.py:391-392) does not. A future refactor could drop the kwarg silently and reintroduce the exact duplicate-update bug this PR prevents — all unit tests would still pass.

Suggestion: Add a router-level test that mocks FactsClient.send_forward_request, exercises forward_request_then_wait_for_update, and asserts the call received is_consistent_update=True. Add the mirror test for forward_remaining_requests asserting the header is absent on the built request.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added router-level coverage in horizon/tests/test_facts_router.py: test_forward_request_then_wait_for_update_sets_consistent_update_flag pins is_consistent_update=True on the wait-for-update path, and test_forward_remaining_requests_does_not_set_consistent_update_header asserts the header is absent on the fallback proxy's built request. Fixed in d02054a.

body = client.extract_body(response)
if body is None:
return client.convert_response(response)
Expand Down
81 changes: 81 additions & 0 deletions horizon/tests/test_facts_client.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
from unittest.mock import AsyncMock, MagicMock, patch

import pytest
from horizon.facts.client import CONSISTENT_UPDATE_HEADER, FactsClient
from starlette.requests import Request as FastApiRequest


def _make_request(headers: dict[str, str] | None = None) -> FastApiRequest:
scope = {
"type": "http",
"method": "POST",
"path": "/facts/users",
"raw_path": b"/facts/users",
"query_string": b"",
"headers": [(k.lower().encode(), v.encode()) for k, v in (headers or {}).items()],
}

async def receive():
return {"type": "http.request", "body": b"", "more_body": False}

return FastApiRequest(scope, receive)


@pytest.mark.asyncio
async def test_build_forward_request_adds_header_when_consistent_update():
"""When is_consistent_update=True, request should carry the X-Permit-Consistent-Update header with value 'true'."""
client = FactsClient()

mock_remote_config = MagicMock()
mock_remote_config.context = {"project_id": "proj1", "env_id": "env1"}

with (
patch("horizon.facts.client.get_remote_config", return_value=mock_remote_config),
patch("horizon.facts.client.get_env_api_key", return_value="test_api_key"),
):
request = _make_request(headers={"authorization": "Bearer user_token", "content-type": "application/json"})
forward_request = await client.build_forward_request(request, "/users", is_consistent_update=True)

# Check the literal header name (not the constant) so a constant rename is caught by tests.
assert "X-Permit-Consistent-Update" in forward_request.headers
assert forward_request.headers["X-Permit-Consistent-Update"] == "true"


@pytest.mark.asyncio
async def test_build_forward_request_omits_header_by_default():
"""By default (fallback proxy path), the request should NOT carry the consistent-update header."""
client = FactsClient()

mock_remote_config = MagicMock()
mock_remote_config.context = {"project_id": "proj1", "env_id": "env1"}

with (
patch("horizon.facts.client.get_remote_config", return_value=mock_remote_config),
patch("horizon.facts.client.get_env_api_key", return_value="test_api_key"),
):
request = _make_request(headers={"authorization": "Bearer user_token", "content-type": "application/json"})
forward_request = await client.build_forward_request(request, "/anything")

assert forward_request.headers.get(CONSISTENT_UPDATE_HEADER) is None


@pytest.mark.asyncio
async def test_send_forward_request_propagates_consistent_update_kwarg():
"""send_forward_request must plumb is_consistent_update into the built request's headers."""
client = FactsClient()

mock_remote_config = MagicMock()
mock_remote_config.context = {"project_id": "proj1", "env_id": "env1"}

with (
patch("horizon.facts.client.get_remote_config", return_value=mock_remote_config),
patch("horizon.facts.client.get_env_api_key", return_value="test_api_key"),
patch.object(FactsClient, "send", new_callable=AsyncMock) as mock_send,
):
request = _make_request(headers={"authorization": "Bearer user_token", "content-type": "application/json"})
await client.send_forward_request(request, "/users", is_consistent_update=True)

assert mock_send.await_count == 1
assert mock_send.call_args is not None
sent_request = mock_send.call_args.args[0]
assert sent_request.headers.get("X-Permit-Consistent-Update") == "true"
74 changes: 74 additions & 0 deletions horizon/tests/test_facts_router.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
from unittest.mock import AsyncMock, MagicMock, patch

import pytest
from horizon.facts.client import FactsClient
from horizon.facts.router import forward_remaining_requests, forward_request_then_wait_for_update
from httpx import Response as HttpxResponse
from starlette.requests import Request as FastApiRequest


def _make_request(headers: dict[str, str] | None = None) -> FastApiRequest:
scope = {
"type": "http",
"method": "POST",
"path": "/facts/users",
"raw_path": b"/facts/users",
"query_string": b"",
"headers": [(k.lower().encode(), v.encode()) for k, v in (headers or {}).items()],
}

async def receive():
return {"type": "http.request", "body": b"", "more_body": False}

return FastApiRequest(scope, receive)


@pytest.mark.asyncio
async def test_forward_request_then_wait_for_update_sets_consistent_update_flag():
"""The wait-for-update proxy path MUST pass is_consistent_update=True to the client."""
client = MagicMock(spec=FactsClient)
client.send_forward_request = AsyncMock(return_value=HttpxResponse(status_code=204))
client.extract_body = MagicMock(return_value=None)
client.convert_response = MagicMock(return_value=MagicMock())

update_subscriber = MagicMock()
request = _make_request(headers={"authorization": "Bearer token"})

await forward_request_then_wait_for_update(
client,
request,
update_subscriber,
wait_timeout=0,
path="/users",
entries_callback=lambda _r, _body, _update_id: [],
)

assert client.send_forward_request.await_count == 1
_, kwargs = client.send_forward_request.await_args
assert kwargs.get("is_consistent_update") is True


@pytest.mark.asyncio
async def test_forward_remaining_requests_does_not_set_consistent_update_header():
"""The fallback proxy route MUST NOT mark the request as a consistent update."""
client = FactsClient()

mock_remote_config = MagicMock()
mock_remote_config.context = {"project_id": "proj1", "env_id": "env1"}

captured = {}

async def fake_send(request, *, stream=False): # noqa: ARG001
captured["headers"] = dict(request.headers)
return HttpxResponse(status_code=204)

with (
patch("horizon.facts.client.get_remote_config", return_value=mock_remote_config),
patch("horizon.facts.client.get_env_api_key", return_value="test_api_key"),
patch.object(FactsClient, "send", side_effect=fake_send),
):
request = _make_request(headers={"authorization": "Bearer token", "content-type": "application/json"})
await forward_remaining_requests(request, client, full_path="some/other/path")

assert "X-Permit-Consistent-Update" not in captured["headers"]
assert "x-permit-consistent-update" not in captured["headers"]
Loading