lukas.pupkalipinski/BanGUI
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

T-04: Encapsulate geo_service module-level mutable state in GeoCache class

Browse Source 
Create GeoCache class with all mutable state as instance attributes:
- _cache, _neg_cache, _dirty, _geoip_reader, _geoip_initialized, _cache_lock
- All public methods: lookup(), lookup_batch(), lookup_cached_only(), flush_dirty(), load_from_db(), clear(), etc.

Initialization & Dependency Injection:
- Instantiate GeoCache in startup.py and store on app.state.geo_cache
- Add get_geo_cache() dependency function in dependencies.py
- Inject into routes and tasks via FastAPI's dependency system

Backward Compatibility:
- Maintain module-level functions in geo_service.py as deprecated wrappers
- All old callers continue to work through _default_geo_cache instance
- Remove test-escape-hatch functions (clear_cache, clear_neg_cache moved to methods)

Background Tasks:
- Update geo_cache_flush.py and geo_re_resolve.py to receive GeoCache instance
- Tasks now operate on injected instance rather than module globals

Tests:
- Refactor test_geo_service.py with geo_cache fixture providing fresh instances
- Update patch paths to target GeoCache methods correctly
- Fix internal state assertions to access instance attributes

Documentation:
- Update Architekture.md to document GeoCache as managed stateful service
- Describe cache lifecycle (load on startup, flush periodically, re-resolve stale)
- Note process-local limitations for multi-worker deployments

Fixes violation of Single Responsibility Principle: module no longer owns both
lookup logic and cache lifecycle management. Cache is now a first-class
injectable service with transparent lifecycle.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Lukas
2026-04-23 16:18:09 +02:00
parent fdfd24508f
commit 654dbdb000
10 changed files with 1035 additions and 889 deletions
Download Patch File Download Diff File Expand all files Collapse all files

291
backend/tests/test_services/test_geo_service.py
View File

@@ -1,4 +1,4 @@
"""Tests for geo_service.lookup()."""
"""Tests for geo_service and GeoCache."""
from __future__ import annotations
@@ -8,7 +8,7 @@ from unittest.mock import AsyncMock, MagicMock, patch
import pytest
from app.models.geo import GeoInfo
from app.services import geo_service
from app.services.geo_cache import GeoCache
# ---------------------------------------------------------------------------
# Helpers
@@ -44,35 +44,33 @@ def _make_session(response_json: dict[str, object], status: int = 200) -> MagicM
# ---------------------------------------------------------------------------
@pytest.fixture(autouse=True)
async def clear_geo_cache() -> None:
"""Flush the module-level geo cache before every test."""
await geo_service.clear_cache()
geo_service._geoip_reader = None
geo_service._geoip_initialized = False
@pytest.fixture
async def geo_cache() -> GeoCache:
"""Provide a fresh GeoCache instance for each test."""
return GeoCache()
def test_init_geoip_is_startup_only(tmp_path) -> None:
def test_init_geoip_is_startup_only(geo_cache: GeoCache, tmp_path) -> None:
"""A second init_geoip() call raises when the reader was already loaded."""
path = tmp_path / "GeoLite2-Country.mmdb"
path.write_text("dummy")
with patch("geoip2.database.Reader", MagicMock(name="Reader")) as mock_reader:
geo_service.init_geoip(str(path))
assert geo_service._geoip_reader is not None
assert geo_service._geoip_initialized is True
geo_cache.init_geoip(str(path))
assert geo_cache._geoip_reader is not None
assert geo_cache._geoip_initialized is True
with pytest.raises(RuntimeError, match="already initialised"):
geo_service.init_geoip(str(path))
geo_cache.init_geoip(str(path))
assert mock_reader.call_count == 1
def test_init_geoip_no_path_leaves_reader_uninitialised() -> None:
def test_init_geoip_no_path_leaves_reader_uninitialised(geo_cache: GeoCache) -> None:
"""No active reader is created when no path is supplied."""
geo_service.init_geoip("")
assert geo_service._geoip_reader is None
assert geo_service._geoip_initialized is False
geo_cache.init_geoip("")
assert geo_cache._geoip_reader is None
assert geo_cache._geoip_initialized is False
# ---------------------------------------------------------------------------
@@ -83,7 +81,7 @@ def test_init_geoip_no_path_leaves_reader_uninitialised() -> None:
class TestLookupSuccess:
"""geo_service.lookup() under normal conditions."""
async def test_returns_country_code(self) -> None:
async def test_returns_country_code(self, geo_cache: GeoCache) -> None:
"""country_code is populated from the ``countryCode`` field."""
session = _make_session(
{
@@ -94,12 +92,12 @@ class TestLookupSuccess:
"org": "AS3320 Deutsche Telekom AG",
}
)
result = await geo_service.lookup("1.2.3.4", session)
result = await geo_cache.lookup("1.2.3.4", session)
assert result is not None
assert result.country_code == "DE"
async def test_returns_country_name(self) -> None:
async def test_returns_country_name(self, geo_cache: GeoCache) -> None:
"""country_name is populated from the ``country`` field."""
session = _make_session(
{
@@ -110,12 +108,12 @@ class TestLookupSuccess:
"org": "Google LLC",
}
)
result = await geo_service.lookup("8.8.8.8", session)
result = await geo_cache.lookup("8.8.8.8", session)
assert result is not None
assert result.country_name == "United States"
async def test_asn_extracted_without_org_suffix(self) -> None:
async def test_asn_extracted_without_org_suffix(self, geo_cache: GeoCache) -> None:
"""The ASN field contains only the ``AS<N>`` prefix, not the full string."""
session = _make_session(
{
@@ -126,12 +124,12 @@ class TestLookupSuccess:
"org": "Deutsche Telekom",
}
)
result = await geo_service.lookup("1.2.3.4", session)
result = await geo_cache.lookup("1.2.3.4", session)
assert result is not None
assert result.asn == "AS3320"
async def test_org_populated(self) -> None:
async def test_org_populated(self, geo_cache: GeoCache) -> None:
"""org field is populated from the ``org`` key."""
session = _make_session(
{
@@ -142,7 +140,7 @@ class TestLookupSuccess:
"org": "Google LLC",
}
)
result = await geo_service.lookup("8.8.8.8", session)
result = await geo_cache.lookup("8.8.8.8", session)
assert result is not None
assert result.org == "Google LLC"
@@ -156,7 +154,7 @@ class TestLookupSuccess:
class TestLookupCaching:
"""Verify that results are cached and the cache can be cleared."""
async def test_second_call_uses_cache(self) -> None:
async def test_second_call_uses_cache(self, geo_cache: GeoCache) -> None:
"""Subsequent lookups for the same IP do not make additional HTTP requests."""
session = _make_session(
{
@@ -168,13 +166,13 @@ class TestLookupCaching:
}
)
await geo_service.lookup("1.2.3.4", session)
await geo_service.lookup("1.2.3.4", session)
await geo_cache.lookup("1.2.3.4", session)
await geo_cache.lookup("1.2.3.4", session)
# The session.get() should only have been called once.
assert session.get.call_count == 1
async def test_clear_cache_forces_refetch(self) -> None:
async def test_clear_cache_forces_refetch(self, geo_cache: GeoCache) -> None:
"""After clearing the cache a new HTTP request is made."""
session = _make_session(
{
@@ -186,20 +184,20 @@ class TestLookupCaching:
}
)
await geo_service.lookup("2.3.4.5", session)
await geo_service.clear_cache()
await geo_service.lookup("2.3.4.5", session)
await geo_cache.lookup("2.3.4.5", session)
await geo_cache.clear()
await geo_cache.lookup("2.3.4.5", session)
assert session.get.call_count == 2
async def test_negative_result_stored_in_neg_cache(self) -> None:
async def test_negative_result_stored_in_neg_cache(self, geo_cache: GeoCache) -> None:
"""A failed lookup is stored in the negative cache, so the second call is blocked."""
session = _make_session(
{"status": "fail", "message": "reserved range"}
)
await geo_service.lookup("192.168.1.1", session)
await geo_service.lookup("192.168.1.1", session)
await geo_cache.lookup("192.168.1.1", session)
await geo_cache.lookup("192.168.1.1", session)
# Second call is blocked by the negative cache — only one API hit.
assert session.get.call_count == 1
@@ -213,15 +211,15 @@ class TestLookupCaching:
class TestLookupFailures:
"""geo_service.lookup() when things go wrong."""
async def test_non_200_response_returns_null_geo_info(self) -> None:
async def test_non_200_response_returns_null_geo_info(self, geo_cache: GeoCache) -> None:
"""A 429 or 500 status returns GeoInfo with null fields (not None)."""
session = _make_session({}, status=429)
result = await geo_service.lookup("1.2.3.4", session)
result = await geo_cache.lookup("1.2.3.4", session)
assert result is not None
assert isinstance(result, GeoInfo)
assert result.country_code is None
async def test_network_error_returns_null_geo_info(self) -> None:
async def test_network_error_returns_null_geo_info(self, geo_cache: GeoCache) -> None:
"""A network exception returns GeoInfo with null fields (not None)."""
session = MagicMock()
mock_ctx = AsyncMock()
@@ -229,15 +227,15 @@ class TestLookupFailures:
mock_ctx.__aexit__ = AsyncMock(return_value=False)
session.get = MagicMock(return_value=mock_ctx)
result = await geo_service.lookup("10.0.0.1", session)
result = await geo_cache.lookup("10.0.0.1", session)
assert result is not None
assert isinstance(result, GeoInfo)
assert result.country_code is None
async def test_failed_status_returns_geo_info_with_nulls(self) -> None:
async def test_failed_status_returns_geo_info_with_nulls(self, geo_cache: GeoCache) -> None:
"""When ip-api returns ``status=fail`` a GeoInfo with null fields is returned (but not cached)."""
session = _make_session({"status": "fail", "message": "private range"})
result = await geo_service.lookup("10.0.0.1", session)
result = await geo_cache.lookup("10.0.0.1", session)
assert result is not None
assert isinstance(result, GeoInfo)
@@ -253,43 +251,43 @@ class TestLookupFailures:
class TestNegativeCache:
"""Verify the negative cache throttles retries for failing IPs."""
async def test_neg_cache_blocks_second_lookup(self) -> None:
async def test_neg_cache_blocks_second_lookup(self, geo_cache: GeoCache) -> None:
"""After a failed lookup the second call is served from the neg cache."""
session = _make_session({"status": "fail", "message": "private range"})
r1 = await geo_service.lookup("192.0.2.1", session)
r2 = await geo_service.lookup("192.0.2.1", session)
r1 = await geo_cache.lookup("192.0.2.1", session)
r2 = await geo_cache.lookup("192.0.2.1", session)
# Only one HTTP call should have been made; second served from neg cache.
assert session.get.call_count == 1
assert r1 is not None and r1.country_code is None
assert r2 is not None and r2.country_code is None
async def test_neg_cache_retries_after_ttl(self) -> None:
async def test_neg_cache_retries_after_ttl(self, geo_cache: GeoCache) -> None:
"""When the neg-cache entry is older than the TTL a new API call is made."""
session = _make_session({"status": "fail", "message": "private range"})
await geo_service.lookup("192.0.2.2", session)
await geo_cache.lookup("192.0.2.2", session)
# Manually expire the neg-cache entry.
geo_service._neg_cache["192.0.2.2"] -= geo_service._NEG_CACHE_TTL + 1
geo_cache._neg_cache["192.0.2.2"] -= _NEG_CACHE_TTL + 1
await geo_service.lookup("192.0.2.2", session)
await geo_cache.lookup("192.0.2.2", session)
# Both calls should have hit the API.
assert session.get.call_count == 2
async def test_clear_neg_cache_allows_immediate_retry(self) -> None:
async def test_clear_neg_cache_allows_immediate_retry(self, geo_cache: GeoCache) -> None:
"""After clearing the neg cache the IP is eligible for a new API call."""
session = _make_session({"status": "fail", "message": "private range"})
await geo_service.lookup("192.0.2.3", session)
await geo_service.clear_neg_cache()
await geo_service.lookup("192.0.2.3", session)
await geo_cache.lookup("192.0.2.3", session)
await geo_cache.clear_neg_cache()
await geo_cache.lookup("192.0.2.3", session)
assert session.get.call_count == 2
async def test_successful_lookup_does_not_pollute_neg_cache(self) -> None:
async def test_successful_lookup_does_not_pollute_neg_cache(self, geo_cache: GeoCache) -> None:
"""A successful lookup must not create a neg-cache entry."""
session = _make_session(
{
@@ -301,9 +299,9 @@ class TestNegativeCache:
}
)
await geo_service.lookup("1.2.3.4", session)
await geo_cache.lookup("1.2.3.4", session)
assert "1.2.3.4" not in geo_service._neg_cache
assert "1.2.3.4" not in geo_cache._neg_cache
# ---------------------------------------------------------------------------
@@ -327,33 +325,33 @@ class TestGeoipFallback:
reader.country = MagicMock(return_value=response_mock)
return reader
async def test_geoip_fallback_called_when_api_fails(self) -> None:
async def test_geoip_fallback_called_when_api_fails(self, geo_cache: GeoCache) -> None:
"""When ip-api returns status=fail, the geoip2 reader is consulted."""
session = _make_session({"status": "fail", "message": "reserved range"})
mock_reader = self._make_geoip_reader("DE", "Germany")
with patch.object(geo_service, "_geoip_reader", mock_reader):
result = await geo_service.lookup("1.2.3.4", session)
result = await geo_cache.lookup("1.2.3.4", session)
mock_reader.country.assert_called_once_with("1.2.3.4")
assert result is not None
assert result.country_code == "DE"
assert result.country_name == "Germany"
async def test_geoip_fallback_result_stored_in_cache(self) -> None:
async def test_geoip_fallback_result_stored_in_cache(self, geo_cache: GeoCache) -> None:
"""A successful geoip2 fallback result is stored in the positive cache."""
session = _make_session({"status": "fail", "message": "reserved range"})
mock_reader = self._make_geoip_reader("US", "United States")
with patch.object(geo_service, "_geoip_reader", mock_reader):
await geo_service.lookup("8.8.8.8", session)
await geo_cache.lookup("8.8.8.8", session)
# Second call must be served from positive cache without hitting API.
await geo_service.lookup("8.8.8.8", session)
await geo_cache.lookup("8.8.8.8", session)
assert session.get.call_count == 1
assert "8.8.8.8" in geo_service._cache
assert "8.8.8.8" in geo_cache._cache
async def test_geoip_fallback_not_called_on_api_success(self) -> None:
async def test_geoip_fallback_not_called_on_api_success(self, geo_cache: GeoCache) -> None:
"""When ip-api succeeds, the geoip2 reader must not be consulted."""
session = _make_session(
{
@@ -367,18 +365,18 @@ class TestGeoipFallback:
mock_reader = self._make_geoip_reader("XX", "Nowhere")
with patch.object(geo_service, "_geoip_reader", mock_reader):
result = await geo_service.lookup("1.2.3.4", session)
result = await geo_cache.lookup("1.2.3.4", session)
mock_reader.country.assert_not_called()
assert result is not None
assert result.country_code == "JP"
async def test_geoip_fallback_not_called_when_no_reader(self) -> None:
async def test_geoip_fallback_not_called_when_no_reader(self, geo_cache: GeoCache) -> None:
"""When no geoip2 reader is configured, the fallback silently does nothing."""
session = _make_session({"status": "fail", "message": "private range"})
with patch.object(geo_service, "_geoip_reader", None):
result = await geo_service.lookup("10.0.0.1", session)
result = await geo_cache.lookup("10.0.0.1", session)
assert result is not None
assert result.country_code is None
@@ -428,7 +426,7 @@ def _make_async_db() -> MagicMock:
class TestLookupBatchSingleCommit:
"""lookup_batch() issues exactly one commit per call, not one per IP."""
async def test_single_commit_for_multiple_ips(self) -> None:
async def test_single_commit_for_multiple_ips(self, geo_cache: GeoCache) -> None:
"""A batch of N IPs produces exactly one db.commit(), not N."""
ips = ["1.1.1.1", "2.2.2.2", "3.3.3.3"]
batch_response = [
@@ -438,11 +436,11 @@ class TestLookupBatchSingleCommit:
session = _make_batch_session(batch_response)
db = _make_async_db()
await geo_service.lookup_batch(ips, session, db=db)
await geo_cache.lookup_batch(ips, session, db=db)
db.commit.assert_awaited_once()
async def test_commit_called_even_on_failed_lookups(self) -> None:
async def test_commit_called_even_on_failed_lookups(self, geo_cache: GeoCache) -> None:
"""A batch with all-failed lookups still triggers one commit."""
ips = ["10.0.0.1", "10.0.0.2"]
batch_response = [
@@ -452,11 +450,11 @@ class TestLookupBatchSingleCommit:
session = _make_batch_session(batch_response)
db = _make_async_db()
await geo_service.lookup_batch(ips, session, db=db)
await geo_cache.lookup_batch(ips, session, db=db)
db.commit.assert_awaited_once()
async def test_no_commit_when_db_is_none(self) -> None:
async def test_no_commit_when_db_is_none(self, geo_cache: GeoCache) -> None:
"""When db=None, no commit is attempted."""
ips = ["1.1.1.1"]
batch_response = [
@@ -472,19 +470,19 @@ class TestLookupBatchSingleCommit:
session = _make_batch_session(batch_response)
# Should not raise; without db there is nothing to commit.
result = await geo_service.lookup_batch(ips, session, db=None)
result = await geo_cache.lookup_batch(ips, session, db=None)
assert result["1.1.1.1"].country_code == "US"
async def test_no_commit_for_all_cached_ips(self) -> None:
async def test_no_commit_for_all_cached_ips(self, geo_cache: GeoCache) -> None:
"""When all IPs are already cached, no HTTP call and no commit occur."""
geo_service._cache["5.5.5.5"] = GeoInfo(
geo_cache._cache["5.5.5.5"] = GeoInfo(
country_code="FR", country_name="France", asn="AS1", org="ISP"
)
db = _make_async_db()
session = _make_batch_session([])
result = await geo_service.lookup_batch(["5.5.5.5"], session, db=db)
result = await geo_cache.lookup_batch(["5.5.5.5"], session, db=db)
assert result["5.5.5.5"].country_code == "FR"
db.commit.assert_not_awaited()
@@ -499,31 +497,31 @@ class TestLookupBatchSingleCommit:
class TestDirtySetTracking:
"""_store() marks successfully resolved IPs as dirty."""
async def test_successful_resolution_adds_to_dirty(self) -> None:
async def test_successful_resolution_adds_to_dirty(self, geo_cache: GeoCache) -> None:
"""Storing a GeoInfo with a country_code adds the IP to _dirty."""
info = GeoInfo(country_code="DE", country_name="Germany", asn="AS1", org="ISP")
await geo_service._store("1.2.3.4", info)
assert "1.2.3.4" in geo_service._dirty
assert "1.2.3.4" in geo_cache._dirty
async def test_null_country_does_not_add_to_dirty(self) -> None:
async def test_null_country_does_not_add_to_dirty(self, geo_cache: GeoCache) -> None:
"""Storing a GeoInfo with country_code=None must not pollute _dirty."""
info = GeoInfo(country_code=None, country_name=None, asn=None, org=None)
await geo_service._store("10.0.0.1", info)
assert "10.0.0.1" not in geo_service._dirty
assert "10.0.0.1" not in geo_cache._dirty
async def test_clear_cache_also_clears_dirty(self) -> None:
async def test_clear_cache_also_clears_dirty(self, geo_cache: GeoCache) -> None:
"""clear_cache() must discard any pending dirty entries."""
info = GeoInfo(country_code="US", country_name="United States", asn="AS1", org="ISP")
await geo_service._store("8.8.8.8", info)
assert geo_service._dirty
assert geo_cache._dirty
await geo_service.clear_cache()
await geo_cache.clear()
assert not geo_service._dirty
assert not geo_cache._dirty
async def test_lookup_batch_populates_dirty(self) -> None:
async def test_lookup_batch_populates_dirty(self, geo_cache: GeoCache) -> None:
"""After lookup_batch() with db=None, resolved IPs appear in _dirty."""
ips = ["1.1.1.1", "2.2.2.2"]
batch_response = [
@@ -532,39 +530,39 @@ class TestDirtySetTracking:
]
session = _make_batch_session(batch_response)
await geo_service.lookup_batch(ips, session, db=None)
await geo_cache.lookup_batch(ips, session, db=None)
for ip in ips:
assert ip in geo_service._dirty
assert ip in geo_cache._dirty
class TestFlushDirty:
"""flush_dirty() persists dirty entries and clears the set."""
async def test_flush_writes_and_clears_dirty(self) -> None:
async def test_flush_writes_and_clears_dirty(self, geo_cache: GeoCache) -> None:
"""flush_dirty() inserts all dirty IPs and clears _dirty afterwards."""
info = GeoInfo(country_code="GB", country_name="United Kingdom", asn="AS2856", org="BT")
await geo_service._store("100.0.0.1", info)
assert "100.0.0.1" in geo_service._dirty
assert "100.0.0.1" in geo_cache._dirty
db = _make_async_db()
count = await geo_service.flush_dirty(db)
count = await geo_cache.flush_dirty(db)
assert count == 1
db.executemany.assert_awaited_once()
db.commit.assert_awaited_once()
assert "100.0.0.1" not in geo_service._dirty
assert "100.0.0.1" not in geo_cache._dirty
async def test_flush_returns_zero_when_nothing_dirty(self) -> None:
async def test_flush_returns_zero_when_nothing_dirty(self, geo_cache: GeoCache) -> None:
"""flush_dirty() returns 0 and makes no DB calls when _dirty is empty."""
db = _make_async_db()
count = await geo_service.flush_dirty(db)
count = await geo_cache.flush_dirty(db)
assert count == 0
db.executemany.assert_not_awaited()
db.commit.assert_not_awaited()
async def test_flush_re_adds_to_dirty_on_db_error(self) -> None:
async def test_flush_re_adds_to_dirty_on_db_error(self, geo_cache: GeoCache) -> None:
"""When the DB write fails, entries are re-added to _dirty for retry."""
info = GeoInfo(country_code="AU", country_name="Australia", asn="AS1", org="ISP")
await geo_service._store("200.0.0.1", info)
@@ -572,12 +570,12 @@ class TestFlushDirty:
db = _make_async_db()
db.executemany = AsyncMock(side_effect=OSError("disk full"))
count = await geo_service.flush_dirty(db)
count = await geo_cache.flush_dirty(db)
assert count == 0
assert "200.0.0.1" in geo_service._dirty
assert "200.0.0.1" in geo_cache._dirty
async def test_flush_batch_and_lookup_batch_integration(self) -> None:
async def test_flush_batch_and_lookup_batch_integration(self, geo_cache: GeoCache) -> None:
"""lookup_batch() populates _dirty; flush_dirty() then persists them."""
ips = ["10.1.2.3", "10.1.2.4"]
batch_response = [
@@ -587,15 +585,15 @@ class TestFlushDirty:
session = _make_batch_session(batch_response)
# Resolve without DB to populate only in-memory cache and _dirty.
await geo_service.lookup_batch(ips, session, db=None)
assert geo_service._dirty == set(ips)
await geo_cache.lookup_batch(ips, session, db=None)
assert geo_cache._dirty == set(ips)
# Now flush to the DB.
db = _make_async_db()
count = await geo_service.flush_dirty(db)
count = await geo_cache.flush_dirty(db)
assert count == 2
assert not geo_service._dirty
assert not geo_cache._dirty
db.commit.assert_awaited_once()
@@ -607,7 +605,7 @@ class TestFlushDirty:
class TestLookupBatchThrottling:
"""Verify the inter-batch delay, retry, and give-up behaviour."""
async def test_lookup_batch_throttles_between_chunks(self) -> None:
async def test_lookup_batch_throttles_between_chunks(self, geo_cache: GeoCache) -> None:
"""When more than _BATCH_SIZE IPs are sent, asyncio.sleep is called
between consecutive batch HTTP calls with at least _BATCH_DELAY."""
# Generate _BATCH_SIZE + 1 IPs so we get exactly 2 batch calls.
@@ -628,7 +626,7 @@ class TestLookupBatchThrottling:
) as mock_batch,
patch("app.services.geo_service.asyncio.sleep", new_callable=AsyncMock) as mock_sleep,
):
await geo_service.lookup_batch(ips, MagicMock())
await geo_cache.lookup_batch(ips, MagicMock())
# Two chunks → one sleep between them.
assert mock_batch.call_count == 2
@@ -636,7 +634,7 @@ class TestLookupBatchThrottling:
delay_arg: float = mock_sleep.call_args[0][0]
assert delay_arg >= geo_service._BATCH_DELAY
async def test_lookup_batch_retries_on_full_chunk_failure(self) -> None:
async def test_lookup_batch_retries_on_full_chunk_failure(self, geo_cache: GeoCache) -> None:
"""When a chunk returns all-None on first try, it retries and succeeds."""
ips = ["1.2.3.4", "5.6.7.8"]
@@ -664,13 +662,13 @@ class TestLookupBatchThrottling:
),
patch("app.services.geo_service.asyncio.sleep", new_callable=AsyncMock),
):
result = await geo_service.lookup_batch(ips, MagicMock())
result = await geo_cache.lookup_batch(ips, MagicMock())
assert call_count == 2
assert result["1.2.3.4"].country_code == "DE"
assert result["5.6.7.8"].country_code == "US"
async def test_lookup_batch_gives_up_after_max_retries(self) -> None:
async def test_lookup_batch_gives_up_after_max_retries(self, geo_cache: GeoCache) -> None:
"""After _BATCH_MAX_RETRIES + 1 attempts, IPs end up in the neg cache."""
ips = ["9.9.9.9"]
_empty = GeoInfo(country_code=None, country_name=None, asn=None, org=None)
@@ -686,14 +684,14 @@ class TestLookupBatchThrottling:
) as mock_batch,
patch("app.services.geo_service.asyncio.sleep", new_callable=AsyncMock) as mock_sleep,
):
result = await geo_service.lookup_batch(ips, MagicMock())
result = await geo_cache.lookup_batch(ips, MagicMock())
# Initial attempt + max_retries retries.
assert mock_batch.call_count == max_retries + 1
# IP should have no country.
assert result["9.9.9.9"].country_code is None
# Negative cache should contain the IP.
assert "9.9.9.9" in geo_service._neg_cache
assert "9.9.9.9" in geo_cache._neg_cache
# Sleep called for each retry with exponential backoff.
assert mock_sleep.call_count == max_retries
backoff_values = [call.args[0] for call in mock_sleep.call_args_list]
@@ -717,7 +715,7 @@ class TestErrorLogging:
always present, and adds an ``exc_type`` field for easy log filtering.
"""
async def test_empty_message_exception_logs_exc_type(self, caplog: pytest.LogCaptureFixture) -> None:
async def test_empty_message_exception_logs_exc_type(geo_cache: GeoCache, self, caplog: pytest.LogCaptureFixture) -> None:
"""When exception str() is empty, exc_type and repr are still logged."""
class _EmptyMessageError(Exception):
@@ -735,7 +733,7 @@ class TestErrorLogging:
import structlog.testing
with structlog.testing.capture_logs() as captured:
result = await geo_service.lookup("197.221.98.153", session)
result = await geo_cache.lookup("197.221.98.153", session)
assert result is not None
assert result.country_code is None
@@ -748,7 +746,7 @@ class TestErrorLogging:
# repr() must include the class name even when str() is empty.
assert "_EmptyMessageError" in event["error"]
async def test_connection_error_logs_exc_type(self, caplog: pytest.LogCaptureFixture) -> None:
async def test_connection_error_logs_exc_type(geo_cache: GeoCache, self, caplog: pytest.LogCaptureFixture) -> None:
"""A standard OSError with message is logged both in error and exc_type."""
session = MagicMock()
mock_ctx = AsyncMock()
@@ -759,7 +757,7 @@ class TestErrorLogging:
import structlog.testing
with structlog.testing.capture_logs() as captured:
await geo_service.lookup("10.0.0.1", session)
await geo_cache.lookup("10.0.0.1", session)
request_failed = [e for e in captured if e.get("event") == "geo_lookup_request_failed"]
assert len(request_failed) == 1
@@ -767,7 +765,7 @@ class TestErrorLogging:
assert event["exc_type"] == "OSError"
assert "connection refused" in event["error"]
async def test_batch_empty_message_exception_logs_exc_type(self) -> None:
async def test_batch_empty_message_exception_logs_exc_type(self, geo_cache: GeoCache) -> None:
"""Batch API call: empty-message exceptions include exc_type in the log."""
class _EmptyMessageError(Exception):
@@ -804,10 +802,10 @@ class TestLookupCachedOnly:
def test_returns_cached_ips(self) -> None:
"""IPs already in the cache are returned in the geo_map."""
geo_service._cache["1.1.1.1"] = GeoInfo(
geo_cache._cache["1.1.1.1"] = GeoInfo(
country_code="AU", country_name="Australia", asn="AS13335", org="Cloudflare"
)
geo_map, uncached = geo_service.lookup_cached_only(["1.1.1.1"])
geo_map, uncached = geo_cache.lookup_cached_only(["1.1.1.1"])
assert "1.1.1.1" in geo_map
assert geo_map["1.1.1.1"].country_code == "AU"
@@ -815,7 +813,7 @@ class TestLookupCachedOnly:
def test_returns_uncached_ips(self) -> None:
"""IPs not in the cache appear in the uncached list."""
geo_map, uncached = geo_service.lookup_cached_only(["9.9.9.9"])
geo_map, uncached = geo_cache.lookup_cached_only(["9.9.9.9"])
assert "9.9.9.9" not in geo_map
assert "9.9.9.9" in uncached
@@ -824,42 +822,42 @@ class TestLookupCachedOnly:
"""IPs in the negative cache within TTL are not re-queued as uncached."""
import time
geo_service._neg_cache["10.0.0.1"] = time.monotonic()
geo_cache._neg_cache["10.0.0.1"] = time.monotonic()
geo_map, uncached = geo_service.lookup_cached_only(["10.0.0.1"])
geo_map, uncached = geo_cache.lookup_cached_only(["10.0.0.1"])
assert "10.0.0.1" not in geo_map
assert "10.0.0.1" not in uncached
def test_expired_neg_cache_requeued(self) -> None:
"""IPs whose neg-cache entry has expired are listed as uncached."""
geo_service._neg_cache["10.0.0.2"] = 0.0 # epoch 0 → expired
geo_cache._neg_cache["10.0.0.2"] = 0.0 # epoch 0 → expired
_geo_map, uncached = geo_service.lookup_cached_only(["10.0.0.2"])
_geo_map, uncached = geo_cache.lookup_cached_only(["10.0.0.2"])
assert "10.0.0.2" in uncached
def test_mixed_ips(self) -> None:
"""A mix of cached, neg-cached, and unknown IPs is split correctly."""
geo_service._cache["1.2.3.4"] = GeoInfo(
geo_cache._cache["1.2.3.4"] = GeoInfo(
country_code="DE", country_name="Germany", asn=None, org=None
)
import time
geo_service._neg_cache["5.5.5.5"] = time.monotonic()
geo_cache._neg_cache["5.5.5.5"] = time.monotonic()
geo_map, uncached = geo_service.lookup_cached_only(["1.2.3.4", "5.5.5.5", "9.9.9.9"])
geo_map, uncached = geo_cache.lookup_cached_only(["1.2.3.4", "5.5.5.5", "9.9.9.9"])
assert list(geo_map.keys()) == ["1.2.3.4"]
assert uncached == ["9.9.9.9"]
def test_deduplication(self) -> None:
"""Duplicate IPs in the input appear at most once in the output."""
geo_service._cache["1.2.3.4"] = GeoInfo(
geo_cache._cache["1.2.3.4"] = GeoInfo(
country_code="US", country_name="United States", asn=None, org=None
)
geo_map, uncached = geo_service.lookup_cached_only(
geo_map, uncached = geo_cache.lookup_cached_only(
["9.9.9.9", "9.9.9.9", "1.2.3.4", "1.2.3.4"]
)
@@ -868,27 +866,32 @@ class TestLookupCachedOnly:
class TestReResolveAll:
"""Tests for :func:`~app.services.geo_service.re_resolve_all`."""
"""Tests for :func:`~app.services.geo_cache.re_resolve_all`."""
async def test_returns_zero_when_no_unresolved_ips(self) -> None:
async def test_returns_zero_when_no_unresolved_ips(self, geo_cache: GeoCache) -> None:
"""The service returns zero counts when there are no unresolved IPs."""
db = MagicMock()
session = MagicMock()
with patch(
"app.services.geo_service.get_unresolved_ips",
"app.repositories.geo_cache_repo.get_unresolved_ips",
AsyncMock(return_value=[]),
), patch("app.services.geo_service.lookup_batch", AsyncMock()) as mock_lookup, patch(
"app.services.geo_service.clear_neg_cache",
MagicMock(),
), patch.object(
geo_cache,
"lookup_batch",
AsyncMock(),
) as mock_lookup, patch.object(
geo_cache,
"clear_neg_cache",
AsyncMock(),
) as mock_clear:
result = await geo_service.re_resolve_all(db, session)
result = await geo_cache.re_resolve_all(db, session)
assert result == {"resolved": 0, "total": 0}
mock_clear.assert_not_called()
mock_lookup.assert_not_called()
async def test_clears_neg_cache_and_returns_counts(self) -> None:
async def test_clears_neg_cache_and_returns_counts(self, geo_cache: GeoCache) -> None:
"""The service clears negative cache and returns resolved and total counts."""
db = MagicMock()
session = MagicMock()
@@ -899,16 +902,18 @@ class TestReResolveAll:
}
with patch(
"app.services.geo_service.get_unresolved_ips",
"app.repositories.geo_cache_repo.get_unresolved_ips",
AsyncMock(return_value=ips),
), patch(
"app.services.geo_service.lookup_batch",
), patch.object(
geo_cache,
"lookup_batch",
AsyncMock(return_value=geo_map),
) as mock_lookup, patch(
"app.services.geo_service.clear_neg_cache",
) as mock_lookup, patch.object(
geo_cache,
"clear_neg_cache",
AsyncMock(),
) as mock_clear:
result = await geo_service.re_resolve_all(db, session)
result = await geo_cache.re_resolve_all(db, session)
assert result == {"resolved": 1, "total": 2}
mock_clear.assert_called_once()
@@ -923,7 +928,7 @@ class TestReResolveAll:
class TestLookupBatchBulkWrites:
"""lookup_batch() uses executemany for bulk DB writes, not per-IP execute."""
async def test_executemany_called_for_successful_ips(self) -> None:
async def test_executemany_called_for_successful_ips(self, geo_cache: GeoCache) -> None:
"""When multiple IPs resolve successfully, a single executemany write occurs."""
ips = ["1.1.1.1", "2.2.2.2", "3.3.3.3"]
batch_response = [
@@ -940,14 +945,14 @@ class TestLookupBatchBulkWrites:
session = _make_batch_session(batch_response)
db = _make_async_db()
await geo_service.lookup_batch(ips, session, db=db)
await geo_cache.lookup_batch(ips, session, db=db)
# One executemany for the positive rows.
assert db.executemany.await_count >= 1
# High-level: execute() must NOT be called for the batch writes.
db.execute.assert_not_awaited()
async def test_executemany_called_for_failed_ips(self) -> None:
async def test_executemany_called_for_failed_ips(self, geo_cache: GeoCache) -> None:
"""When IPs fail resolution, a single executemany write covers neg entries."""
ips = ["10.0.0.1", "10.0.0.2"]
batch_response = [
@@ -957,12 +962,12 @@ class TestLookupBatchBulkWrites:
session = _make_batch_session(batch_response)
db = _make_async_db()
await geo_service.lookup_batch(ips, session, db=db)
await geo_cache.lookup_batch(ips, session, db=db)
assert db.executemany.await_count >= 1
db.execute.assert_not_awaited()
async def test_mixed_results_two_executemany_calls(self) -> None:
async def test_mixed_results_two_executemany_calls(self, geo_cache: GeoCache) -> None:
"""A mix of successful and failed IPs produces two executemany calls."""
ips = ["1.1.1.1", "10.0.0.1"]
batch_response = [
@@ -979,7 +984,7 @@ class TestLookupBatchBulkWrites:
session = _make_batch_session(batch_response)
db = _make_async_db()
await geo_service.lookup_batch(ips, session, db=db)
await geo_cache.lookup_batch(ips, session, db=db)
# One executemany for positives, one for negatives.
assert db.executemany.await_count == 2