From 7e8103e7948413964c4c3f828d8d13ff3bc7bde3 Mon Sep 17 00:00:00 2001 From: Julien Maupetit Date: Mon, 16 Dec 2024 21:58:36 +0100 Subject: [PATCH] =?UTF-8?q?=E2=9A=A1=EF=B8=8F(api)=20cache=20PointDeCharge?= =?UTF-8?q?=20id=20from=20id=5Fpdc=5Fitinerance=20db=20request?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Getting a point of charge identifier from its `id_pdc_itinerance` is now the most frequent database request as it's accessed almost by every dynamic endpoints (create + read / status + session). As the same point of charge may generate multiple events (and dynamic data) within a limited timeline, caching this database request result may save time and database hits in that period for the same point of charge. --- .github/workflows/api.yml | 1 + env.d/api | 1 + src/api/CHANGELOG.md | 1 + src/api/qualicharge/api/v1/routers/dynamic.py | 74 ++++++------- src/api/qualicharge/conf.py | 2 + src/api/tests/api/v1/routers/test_dynamic.py | 101 +++++++++++++++++- src/api/tests/fixtures/app.py | 2 + 7 files changed, 137 insertions(+), 45 deletions(-) diff --git a/.github/workflows/api.yml b/.github/workflows/api.yml index 1fc13ba5..3bd9b095 100644 --- a/.github/workflows/api.yml +++ b/.github/workflows/api.yml @@ -366,6 +366,7 @@ jobs: QUALICHARGE_DB_NAME: test-qualicharge-api QUALICHARGE_TEST_DB_NAME: test-qualicharge-api QUALICHARGE_API_GET_USER_CACHE_INFO: true + QUALICHARGE_API_GET_PDC_ID_CACHE_INFO: true # Speed up tests QUALICHARGE_API_STATIQUE_BULK_CREATE_MAX_SIZE: 10 QUALICHARGE_API_STATUS_BULK_CREATE_MAX_SIZE: 10 diff --git a/env.d/api b/env.d/api index d6da1182..2034f114 100644 --- a/env.d/api +++ b/env.d/api @@ -4,6 +4,7 @@ QUALICHARGE_API_ADMIN_PASSWORD=admin QUALICHARGE_API_ADMIN_USER=admin QUALICHARGE_API_STATIQUE_BULK_CREATE_MAX_SIZE=1000 QUALICHARGE_API_GET_USER_CACHE_INFO=True +QUALICHARGE_API_GET_PDC_ID_CACHE_INFO=True QUALICHARGE_DB_CONNECTION_MAX_OVERFLOW=200 QUALICHARGE_DB_CONNECTION_POOL_SIZE=50 QUALICHARGE_DB_ENGINE=postgresql+psycopg diff --git a/src/api/CHANGELOG.md b/src/api/CHANGELOG.md index 76c8b1f9..dc437cd9 100644 --- a/src/api/CHANGELOG.md +++ b/src/api/CHANGELOG.md @@ -22,6 +22,7 @@ and this project adheres to - CLI: sort groups and operational units alphabetically in the `list-groups` command - Decrease the number of database queries for dynamic endpoints +- Cache the "get PointDeCharge id from its `id_pdc_itinerance`" database query ## [0.16.0] - 2024-12-12 diff --git a/src/api/qualicharge/api/v1/routers/dynamic.py b/src/api/qualicharge/api/v1/routers/dynamic.py index 9ba8bee5..8cdedc3f 100644 --- a/src/api/qualicharge/api/v1/routers/dynamic.py +++ b/src/api/qualicharge/api/v1/routers/dynamic.py @@ -1,9 +1,12 @@ """QualiCharge API v1 dynamique router.""" import logging +from threading import Lock from typing import Annotated, List, cast +from uuid import UUID from annotated_types import Len +from cachetools import LRUCache, cached from fastapi import APIRouter, Depends, HTTPException, Path, Query, Security from fastapi import status as fa_status from pydantic import UUID4, BaseModel, PastDatetime, StringConstraints @@ -59,6 +62,31 @@ class DynamiqueItemsCreatedResponse(BaseModel): items: List[UUID4] +@cached( + LRUCache( + maxsize=settings.API_GET_PDC_ID_CACHE_MAXSIZE, + ), + lock=Lock(), + key=lambda id_pdc_itinerance, session: id_pdc_itinerance, + info=settings.API_GET_PDC_ID_CACHE_INFO, +) +def get_pdc_id(id_pdc_itinerance: str, session: Session) -> UUID | None: + """Get PointDeCharge.id from an `id_pdc_itinerance`.""" + pdc_id = session.exec( + select(PointDeCharge.id).where( + PointDeCharge.id_pdc_itinerance == id_pdc_itinerance + ) + ).one_or_none() + + if pdc_id is not None: + return pdc_id + + raise HTTPException( + status_code=fa_status.HTTP_404_NOT_FOUND, + detail="Point of charge does not exist", + ) + + @router.get("/status/", tags=["Status"]) async def list_statuses( user: Annotated[User, Security(get_user, scopes=[ScopesEnum.DYNAMIC_READ.value])], @@ -196,16 +224,7 @@ async def read_status( raise PermissionDenied("You cannot read the status of this point of charge") # Get target point de charge - pdc_id = session.exec( - select(PointDeCharge.id).where( - PointDeCharge.id_pdc_itinerance == id_pdc_itinerance - ) - ).one_or_none() - if pdc_id is None: - raise HTTPException( - status_code=fa_status.HTTP_404_NOT_FOUND, - detail="Selected point of charge does not exist", - ) + pdc_id = get_pdc_id(id_pdc_itinerance, session) # Get latest status (if any) latest_db_status_stmt = ( @@ -264,16 +283,7 @@ async def read_status_history( if not is_pdc_allowed_for_user(id_pdc_itinerance, user): raise PermissionDenied("You cannot read statuses of this point of charge") - pdc_id = session.exec( - select(PointDeCharge.id).where( - PointDeCharge.id_pdc_itinerance == id_pdc_itinerance - ) - ).one_or_none() - if pdc_id is None: - raise HTTPException( - status_code=fa_status.HTTP_404_NOT_FOUND, - detail="Selected point of charge does not exist", - ) + pdc_id = get_pdc_id(id_pdc_itinerance, session) # Get latest statuses db_statuses_stmt = select(Status).where(Status.point_de_charge_id == pdc_id) @@ -313,16 +323,8 @@ async def create_status( if not is_pdc_allowed_for_user(status.id_pdc_itinerance, user): raise PermissionDenied("You cannot create statuses for this point of charge") - pdc_id = session.exec( - select(PointDeCharge.id).where( - PointDeCharge.id_pdc_itinerance == status.id_pdc_itinerance - ) - ).one_or_none() - if pdc_id is None: - raise HTTPException( - status_code=fa_status.HTTP_404_NOT_FOUND, - detail="Attached point of charge does not exist", - ) + pdc_id = get_pdc_id(status.id_pdc_itinerance, session) + db_status = Status(**status.model_dump(exclude={"id_pdc_itinerance"})) # Store status id so that we do not need to perform another request db_status_id = db_status.id @@ -396,16 +398,8 @@ async def create_session( # # - `db_session` / `Session` refers to the database session, while, # - `session` / `QCSession` / `SessionCreate` refers to qualicharge charging session - pdc_id = db_session.exec( - select(PointDeCharge.id).where( - PointDeCharge.id_pdc_itinerance == session.id_pdc_itinerance - ) - ).one_or_none() - if pdc_id is None: - raise HTTPException( - status_code=fa_status.HTTP_404_NOT_FOUND, - detail="Attached point of charge does not exist", - ) + pdc_id = get_pdc_id(session.id_pdc_itinerance, db_session) + db_qc_session = QCSession(**session.model_dump(exclude={"id_pdc_itinerance"})) # Store session id so that we do not need to perform another request db_qc_session_id = db_qc_session.id diff --git a/src/api/qualicharge/conf.py b/src/api/qualicharge/conf.py index b831d2b7..12ad39e0 100644 --- a/src/api/qualicharge/conf.py +++ b/src/api/qualicharge/conf.py @@ -133,6 +133,8 @@ def PASSWORD_CONTEXT(self) -> CryptContext: API_GET_USER_CACHE_MAXSIZE: int = 256 API_GET_USER_CACHE_TTL: int = 1800 API_GET_USER_CACHE_INFO: bool = False + API_GET_PDC_ID_CACHE_MAXSIZE: int = 5000 + API_GET_PDC_ID_CACHE_INFO: bool = False model_config = SettingsConfigDict( case_sensitive=True, env_nested_delimiter="__", env_prefix="QUALICHARGE_" diff --git a/src/api/tests/api/v1/routers/test_dynamic.py b/src/api/tests/api/v1/routers/test_dynamic.py index d3edbeb7..58018114 100644 --- a/src/api/tests/api/v1/routers/test_dynamic.py +++ b/src/api/tests/api/v1/routers/test_dynamic.py @@ -7,11 +7,12 @@ from urllib.parse import quote_plus import pytest -from fastapi import status +from fastapi import HTTPException, status from sqlalchemy import func from sqlalchemy.schema import Column as SAColumn from sqlmodel import select +from qualicharge.api.v1.routers.dynamic import get_pdc_id from qualicharge.auth.factories import GroupFactory from qualicharge.auth.schemas import GroupOperationalUnit, ScopesEnum, User from qualicharge.conf import settings @@ -37,6 +38,52 @@ from qualicharge.schemas.utils import save_statique, save_statiques +def test_get_pdc_id(db_session): + """Test the get_pdc_id utility.""" + id_pdc_itinerance = "FRALLE0123456" + with pytest.raises(HTTPException, match="Point of charge does not exist"): + get_pdc_id(id_pdc_itinerance, db_session) + + n_pdc = 4 + save_statiques(db_session, StatiqueFactory.batch(n_pdc)) + pdcs = db_session.exec(select(PointDeCharge)).all() + assert len(pdcs) == n_pdc + + for pdc in pdcs: + assert pdc.id == get_pdc_id(pdc.id_pdc_itinerance, db_session) + + +def test_get_pdc_id_cache(db_session): + """Test the get_pdc_id utility cache.""" + n_pdc = 4 + save_statiques(db_session, StatiqueFactory.batch(n_pdc)) + pdcs = db_session.exec(select(PointDeCharge)).all() + assert len(pdcs) == n_pdc + + hits_by_pdc = 9 + for pdc_index in range(n_pdc): + pdc = pdcs[pdc_index] + + # First call: feed the cache + with SAQueryCounter(db_session.connection()) as counter: + pdc_id = get_pdc_id(pdc.id_pdc_itinerance, db_session) + assert pdc_id == pdc.id + cache_info = get_pdc_id.cache_info() # type: ignore[attr-defined] + assert counter.count == 1 + assert cache_info.hits == pdc_index * hits_by_pdc + assert cache_info.currsize == pdc_index + 1 + + # Test cached entry + for hit in range(1, hits_by_pdc + 1): + with SAQueryCounter(db_session.connection()) as counter: + pdc_id = get_pdc_id(pdc.id_pdc_itinerance, db_session) + assert pdc_id == pdc.id + cache_info = get_pdc_id.cache_info() # type: ignore[attr-defined] + assert counter.count == 0 + assert cache_info.hits == (pdc_index * hits_by_pdc) + hit + assert cache_info.currsize == pdc_index + 1 + + @pytest.mark.parametrize( "client_auth", ( @@ -331,7 +378,7 @@ def test_read_status_for_non_existing_point_of_charge(client_auth): """Test the /status/{id_pdc_itinerance} endpoint for unknown point of charge.""" response = client_auth.get("/dynamique/status/FR911E1111ER1") assert response.status_code == status.HTTP_404_NOT_FOUND - assert response.json() == {"detail": "Selected point of charge does not exist"} + assert response.json() == {"detail": "Point of charge does not exist"} def test_read_status_for_non_existing_status(db_session, client_auth): @@ -420,6 +467,50 @@ def test_read_status_for_superuser(db_session, client_auth): assert expected_status.etat_prise_type_ef == response_status.etat_prise_type_ef +def test_read_status_get_pdc_id_cache(db_session, client_auth): + """Test the /status/{id_pdc_itinerance} endpoint's get_pdc_id cache usage.""" + StatusFactory.__session__ = db_session + + # Create the PointDeCharge + id_pdc_itinerance = "FR911E1111ER1" + save_statique( + db_session, StatiqueFactory.build(id_pdc_itinerance=id_pdc_itinerance) + ) + pdc = db_session.exec( + select(PointDeCharge).where( + PointDeCharge.id_pdc_itinerance == id_pdc_itinerance + ) + ).one() + + # Create 20 attached statuses + n_statuses = 20 + StatusFactory.create_batch_sync(n_statuses, point_de_charge_id=pdc.id) + + # Count queries while getting the latest status + with SAQueryCounter(db_session.connection()) as counter: + client_auth.get(f"/dynamique/status/{id_pdc_itinerance}") + cache_info = get_pdc_id.cache_info() # type: ignore[attr-defined] + assert cache_info.hits == 0 + assert cache_info.currsize == 1 + # We expect the following db request: + # 1. User authentication + # 2. get_user injection + # 3. get_pdc_id + # 4. latest db status (sub) queries + # 5. get_pdc_id + expected = 5 + assert counter.count == expected + + for hit in range(1, 10): + # Count queries while getting the latest status + with SAQueryCounter(db_session.connection()) as counter: + client_auth.get(f"/dynamique/status/{id_pdc_itinerance}") + cache_info = get_pdc_id.cache_info() # type: ignore[attr-defined] + assert cache_info.hits == hit + assert cache_info.currsize == 1 + assert counter.count == 1 + + @pytest.mark.parametrize( "client_auth", ( @@ -489,7 +580,7 @@ def test_read_status_history_for_non_existing_point_of_charge(client_auth): """Test the /status/{id_pdc_itinerance}/history endpoint for unknown PDC.""" response = client_auth.get("/dynamique/status/FR911E1111ER1/history") assert response.status_code == status.HTTP_404_NOT_FOUND - assert response.json() == {"detail": "Selected point of charge does not exist"} + assert response.json() == {"detail": "Point of charge does not exist"} def test_read_status_history_for_non_existing_status(db_session, client_auth): @@ -709,7 +800,7 @@ def test_create_status_for_non_existing_point_of_charge(client_auth): "/dynamique/status/", json=json.loads(qc_status.model_dump_json()) ) assert response.status_code == status.HTTP_404_NOT_FOUND - assert response.json() == {"detail": "Attached point of charge does not exist"} + assert response.json() == {"detail": "Point of charge does not exist"} @pytest.mark.parametrize( @@ -1180,7 +1271,7 @@ def test_create_session_for_non_existing_point_of_charge(client_auth): "/dynamique/session/", json=json.loads(qc_session.model_dump_json()) ) assert response.status_code == status.HTTP_404_NOT_FOUND - assert response.json() == {"detail": "Attached point of charge does not exist"} + assert response.json() == {"detail": "Point of charge does not exist"} @pytest.mark.parametrize( diff --git a/src/api/tests/fixtures/app.py b/src/api/tests/fixtures/app.py index 59d0135c..65acc192 100644 --- a/src/api/tests/fixtures/app.py +++ b/src/api/tests/fixtures/app.py @@ -5,6 +5,7 @@ from sqlmodel import Session from qualicharge.api.v1 import app +from qualicharge.api.v1.routers.dynamic import get_pdc_id from qualicharge.auth.factories import GroupFactory, IDTokenFactory, UserFactory from qualicharge.auth.oidc import get_token, get_user_from_db from qualicharge.auth.schemas import UserGroup @@ -67,3 +68,4 @@ def clear_lru_cache(): # Clear the LRU cache. get_user_from_db.cache_clear() + get_pdc_id.cache_clear()