From 7592a0413493eb78595c816a836b1f9eac961200 Mon Sep 17 00:00:00 2001 From: Valentin Date: Sun, 11 Dec 2022 16:13:31 +0100 Subject: [PATCH] fixed issues concerning API Cache refresh --- README.md | 2 +- .../commands/check_and_update_cache.py | 30 +++++++++++++---- overfastapi/handlers/api_request_handler.py | 6 ++++ overfastapi/main.py | 2 +- overfastapi/parsers/api_parser.py | 1 - overfastapi/routers/players.py | 5 ++- pyproject.toml | 2 +- tests/commands/test_check_and_update_cache.py | 33 +++++++++++++++++-- 8 files changed, 68 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 629a02c4..876ce122 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ ## ⚠️ Disclaimer concerning career pages ⚠️ -Players statistics are cached for performance purposes, as Blizzard pages take ~2-3 seconds to load. Since the pages are back, I noticed it's very unstable on their side, we often have a "504 Gateway Time-out" error, either on players search or career pages, sometimes a "404 Page Not Found" error even if the player exists and its profile is public. +Players statistics are cached for performance purposes, as Blizzard pages take ~2-3 seconds to load. Since the pages are back, I noticed it's very unstable on their side, we often have a "504 Gateway Time-out" error, either on players search or career pages, sometimes a "404 Page Not Found" error even if the player exists and its profile is public, resulting in a "404 Player Not Found" response from the API. As a consequence, I configured my cache system in order to prevent (in most cases) any issue regarding pages load : - Career data is cached for ~2 hours diff --git a/overfastapi/commands/check_and_update_cache.py b/overfastapi/commands/check_and_update_cache.py index 107fea6c..d3710f1b 100644 --- a/overfastapi/commands/check_and_update_cache.py +++ b/overfastapi/commands/check_and_update_cache.py @@ -1,6 +1,8 @@ """Command used in order to check and update Redis API Cache depending on the expired cache refresh limit configuration. It can be run in the background. """ +import re + from overfastapi.common.cache_manager import CacheManager from overfastapi.common.enums import HeroKey from overfastapi.common.logging import logger @@ -14,6 +16,8 @@ from overfastapi.handlers.list_heroes_request_handler import ListHeroesRequestHandler from overfastapi.handlers.list_roles_request_handler import ListRolesRequestHandler +# Mapping of cache_key prefixes to the associated +# request handler used for cache refresh PREFIXES_HANDLERS_MAPPING = { "/heroes": ListHeroesRequestHandler, "/heroes/roles": ListRolesRequestHandler, @@ -22,18 +26,32 @@ "/players": GetPlayerCareerRequestHandler, } +# Regular expressions for keys we don't want to refresh the cache explicitely +# from here (either will be done in another process or not at all because not +# relevant) +EXCEPTION_KEYS_REGEX = { + r"^\/players\/[^\/]+\/(summary|stats)$", # players summary or search + r"^\/players$", # players search +} + def get_soon_expired_cache_keys() -> set[str]: """Get a set of URIs for values in API Cache which will expire soon without taking subroutes and query parameters""" cache_manager = CacheManager() - return { + + expiring_keys = set() + for key in cache_manager.get_soon_expired_api_cache_keys(): # api-cache:/heroes?role=damage => /heroes?role=damage => /heroes - key.split(":")[1].split("?")[0] - for key in cache_manager.get_soon_expired_api_cache_keys() - # Avoid players subroutes - if key.split("/")[-1].split("?")[0] not in ("summary", "stats") - } + cache_key = key.split(":")[1].split("?")[0] + # Avoid keys we don't want to refresh from here + if any( + re.match(exception_key, cache_key) for exception_key in EXCEPTION_KEYS_REGEX + ): + continue + # Add the key to the set + expiring_keys.add(cache_key) + return expiring_keys def get_request_handler_class_and_kwargs(cache_key: str) -> tuple[type, dict]: diff --git a/overfastapi/handlers/api_request_handler.py b/overfastapi/handlers/api_request_handler.py index a8e82e68..b640ab45 100644 --- a/overfastapi/handlers/api_request_handler.py +++ b/overfastapi/handlers/api_request_handler.py @@ -136,6 +136,12 @@ def update_all_api_cache(self, parsers: list[APIParser], **kwargs) -> None: # page and use the kwargs to generate the appropriate URL try: parser = parser_class(**kwargs) + except ParserInitError as error: + logger.error( + "Failed to instanciate Parser when refreshing : {}", + error.message, + ) + return except HTTPException: return diff --git a/overfastapi/main.py b/overfastapi/main.py index 7e188ade..795ab0e6 100644 --- a/overfastapi/main.py +++ b/overfastapi/main.py @@ -24,7 +24,7 @@ ## ⚠️ Disclaimer concerning career pages ⚠️ -Players statistics are cached for performance purposes, as Blizzard pages take ~2-3 seconds to load. Since the pages are back, I noticed it's very unstable on their side, we often have a "504 Gateway Time-out" error, either on players search or career pages, sometimes a "404 Page Not Found" error even if the player exists and its profile is public. +Players statistics are cached for performance purposes, as Blizzard pages take ~2-3 seconds to load. Since the pages are back, I noticed it's very unstable on their side, we often have a "504 Gateway Time-out" error, either on players search or career pages, sometimes a "404 Page Not Found" error even if the player exists and its profile is public, resulting in a "404 Player Not Found" response from the API. As a consequence, I configured my cache system in order to prevent (in most cases) any issue regarding pages load : - Career data is cached for ~2 hours diff --git a/overfastapi/parsers/api_parser.py b/overfastapi/parsers/api_parser.py index d836e139..f464e59f 100644 --- a/overfastapi/parsers/api_parser.py +++ b/overfastapi/parsers/api_parser.py @@ -78,7 +78,6 @@ def parse(self) -> None: self.cache_key, self.hash ) if parser_cache_data: - # Parser cache is already valid, no need to do anything logger.info("Parser Cache found !") self.data = json.loads(parser_cache_data) diff --git a/overfastapi/routers/players.py b/overfastapi/routers/players.py index 6bf512a1..cd3e1cd7 100644 --- a/overfastapi/routers/players.py +++ b/overfastapi/routers/players.py @@ -37,7 +37,10 @@ async def get_player_common_parameters( player_id: str = Path( title="Player unique name", - description='Identifier of the player : BattleTag (with "#" replaced by "-")', + description=( + 'Identifier of the player : BattleTag (with "#" replaced by "-"). ' + "Be careful, letter case (capital/non-capital letters) is important !" + ), example="TeKrop-2217", ), ): diff --git a/pyproject.toml b/pyproject.toml index 28deb7fb..3b0eb953 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [tool.poetry] name = "overfast-api" -version = "2.3.1" +version = "2.3.2" description = "Overwatch API giving data about heroes, maps, and players statistics." authors = ["TeKrop "] license = "MIT" diff --git a/tests/commands/test_check_and_update_cache.py b/tests/commands/test_check_and_update_cache.py index 77a7405e..295ee49d 100644 --- a/tests/commands/test_check_and_update_cache.py +++ b/tests/commands/test_check_and_update_cache.py @@ -288,7 +288,7 @@ def test_check_and_update_specific_player_to_update( ) -def test_check_error_from_blizzard(cache_manager: CacheManager): +def test_check_internal_error_from_blizzard(cache_manager: CacheManager): # Add some data (to update and not to update) cache_manager.update_api_cache("/heroes/ana", "{}", EXPIRED_CACHE_REFRESH_LIMIT - 5) @@ -326,7 +326,7 @@ def test_check_timeout_from_blizzard(cache_manager: CacheManager): @pytest.mark.parametrize("player_html_data", ["TeKrop-2217"], indirect=True) -def test_check_parsing_error(cache_manager: CacheManager, player_html_data: str): +def test_check_parser_parsing_error(cache_manager: CacheManager, player_html_data: str): # Add some data (to update and not to update) cache_manager.update_api_cache( "/players/TeKrop-2217", "{}", EXPIRED_CACHE_REFRESH_LIMIT - 5 @@ -348,3 +348,32 @@ def test_check_parsing_error(cache_manager: CacheManager, player_html_data: str) "https://overwatch.blizzard.com/en-us/career/TeKrop-2217/", "AttributeError(\"'NoneType' object has no attribute 'find'\")", ) + + +@pytest.mark.parametrize("player_html_data", ["Unknown-1234"], indirect=True) +def test_check_parser_init_error(cache_manager: CacheManager, player_html_data: str): + # Add some data (to update and not to update) + cache_manager.update_api_cache( + "/players/TeKrop-2217", "{}", EXPIRED_CACHE_REFRESH_LIMIT - 5 + ) + + logger_error_mock = Mock() + with patch( + "requests.get", + return_value=Mock(status_code=200, text=player_html_data), + ), patch("overfastapi.common.logging.logger.error", logger_error_mock): + check_and_update_cache_main() + + logger_error_mock.assert_any_call( + "Failed to instanciate Parser when refreshing : {}", "Player not found" + ) + + +def test_check_players_search_never_to_refresh(cache_manager: CacheManager): + # The key will expire soon but shouldn't be in the list of keys to refresh + cache_manager.update_api_cache( + "/players?name=TeKrop", "{}", EXPIRED_CACHE_REFRESH_LIMIT - 5 + ) + # Another one that should be + cache_manager.update_api_cache("/heroes", "{}", EXPIRED_CACHE_REFRESH_LIMIT - 5) + assert get_soon_expired_cache_keys() == {"/heroes"}