From 4f36022443d76e845a0a1c174125dfc69c5d843b Mon Sep 17 00:00:00 2001 From: Colin Maudry Date: Fri, 10 Jul 2026 14:44:14 +0200 Subject: [PATCH] =?UTF-8?q?fix(tableau):=20vues=20sauvegard=C3=A9es=20en?= =?UTF-8?q?=20AST=20canonique,=20cache=20du=20comptage,=20synchro=20visibi?= =?UTF-8?q?lit=C3=A9=20colonnes=20(#41)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/pages/tableau.py | 37 ++++-- src/utils/grid.py | 15 ++- src/utils/query_ast.py | 135 +++++++++++++++++++++ tests/saved_views/test_apply_saved_view.py | 69 +++++++++-- tests/test_grid.py | 51 ++++++++ tests/test_query_ast.py | 101 +++++++++++++++ 6 files changed, 389 insertions(+), 19 deletions(-) diff --git a/src/pages/tableau.py b/src/pages/tableau.py index 9a11642..3d7b1b6 100644 --- a/src/pages/tableau.py +++ b/src/pages/tableau.py @@ -25,6 +25,12 @@ from src.saved_views import db as saved_views_db from src.saved_views import ui as saved_views_ui from src.utils import get_data_update_timestamp, logger from src.utils.grid import fetch_grid_page, grid_column_defs +from src.utils.query_ast import ( + ast_from_dict, + ast_to_dict, + ast_to_filtermodel, + filtermodel_to_ast, +) from src.utils.seo import META_CONTENT from src.utils.table import ( COLUMNS, @@ -577,9 +583,11 @@ def save_view(_n, name, filter_model, column_state): clean_name, error = saved_views_ui.prepare_view_to_save(has_sub, name) if error: return True, html.Span(error, style={"color": "red"}), no_update - query = json.dumps( - {"filterModel": filter_model or {}, "columnState": column_state or []} - ) + # On stocke l'AST canonique (indépendant de l'UI), pas le filterModel brut + # d'AG Grid : cf. spec de conception, "l'AST (JSON) + columnState, + # indépendant de l'UI". + ast = filtermodel_to_ast(filter_model, schema) + query = json.dumps({"ast": ast_to_dict(ast), "columnState": column_state or []}) saved_views_db.upsert(current_user.id, "tableau", clean_name, query) return ( False, @@ -603,6 +611,7 @@ def populate_saved_views_menu(_pathname, _refresh): @callback( Output("tableau_grid", "filterModel"), Output("tableau_grid", "columnState"), + Output("tableau-hidden-columns", "data", allow_duplicate=True), Input({"type": "saved-view-item", "index": ALL}, "n_clicks"), State({"type": "saved-view-item", "index": ALL}, "id"), prevent_initial_call=True, @@ -610,13 +619,19 @@ def populate_saved_views_menu(_pathname, _refresh): def apply_saved_view(n_clicks, ids): triggered = ctx.triggered_id if not triggered or not any(n_clicks): - return no_update, no_update + return no_update, no_update, no_update row = saved_views_db.get(triggered["index"], current_user.id) if not row: - return no_update, no_update + return no_update, no_update, no_update try: view = json.loads(row["query"]) - filter_model = view.get("filterModel") or {} + # L'AST canonique est stocké (pas le filterModel brut d'AG Grid) : + # cf. save_view. `ast_from_dict(None)` -> None et + # `ast_to_filtermodel(None, schema)` -> {} si la vue est d'un ancien + # format (sans clé "ast") : dégradation propre, la vue se rappelle + # sans filtre plutôt que de planter. + ast = ast_from_dict(view.get("ast")) + filter_model = ast_to_filtermodel(ast, schema) column_state = view.get("columnState") or [] except (json.JSONDecodeError, TypeError, AttributeError): # Vue enregistrée avant la migration vers AG Grid (Task 10) : row["query"] @@ -627,8 +642,14 @@ def apply_saved_view(n_clicks, ids): "Vue sauvegardée au format pré-migration, impossible de l'appliquer : " f"id={row['id']!r} name={row['name']!r}" ) - return no_update, no_update - return filter_model, column_state + return no_update, no_update, no_update + # tableau-hidden-columns pilote les cases à cocher du sélecteur de colonnes + # (update_checkboxes_from_hidden_columns) et la régénération des + # columnDefs (apply_hidden_columns) ; sans cette sortie, ce store restait + # désynchronisé du columnState rappelé (revue finale #41). Même extraction + # que download_data. + hidden_columns = [c["colId"] for c in column_state if c.get("hide")] + return filter_model, column_state, hidden_columns @callback( diff --git a/src/utils/grid.py b/src/utils/grid.py index c3e125b..9f48924 100644 --- a/src/utils/grid.py +++ b/src/utils/grid.py @@ -4,10 +4,23 @@ import polars as pl from src.db import count_marches, query_marches, schema from src.figures import DATA_SCHEMA +from src.utils.cache import cache from src.utils.query_ast import ast_to_sql, filtermodel_to_ast, sort_model_to_sql from src.utils.table import postprocess_page +@cache.memoize() +def _cached_count(where_sql: str, params: tuple) -> int: + """Cache le COUNT(*) sur (where_sql, params). + + AG Grid envoie une requête par bloc de défilement infini ; pour un même + filtre, tous les blocs partagent le même (where_sql, params) et donc le + même total — inutile de recompter un COUNT(*) sur ~1,5M lignes à chaque + bloc chargé (cf. `src.utils.table._fetch_page_sql`, même schéma). + """ + return count_marches(where_sql, params) + + def fetch_grid_page( filter_model, sort_model, @@ -23,7 +36,7 @@ def fetch_grid_page( params = [*base_params, *filter_params] order_by = sort_model_to_sql(sort_model, schema) or None - total = count_marches(where_sql, params) + total = _cached_count(where_sql, tuple(params)) limit = max(0, end_row - start_row) page = query_marches( diff --git a/src/utils/query_ast.py b/src/utils/query_ast.py index f36a4aa..f035f12 100644 --- a/src/utils/query_ast.py +++ b/src/utils/query_ast.py @@ -214,6 +214,141 @@ def filtermodel_to_ast(filter_model, schema): return And(children) if children else None +_TEXT_TYPE_INV = {v: k for k, v in _TEXT_TYPE.items()} +_NUM_TYPE_INV = {v: k for k, v in _NUM_TYPE.items()} + + +def _condition_to_filterspec(cond: Condition, schema: pl.Schema) -> dict | None: + """Convertit une Condition en spec de filtre AG Grid unitaire (une colonne). + + Inverse de `_leaf`. Renvoie None (avec warning) si la colonne est inconnue + ou si l'opérateur n'a pas d'équivalent AG Grid — ne devrait pas arriver + pour un AST produit par `filtermodel_to_ast`, mais on reste défensif. + """ + if cond.column not in schema.names(): + logger.warning( + f"Colonne inconnue ignorée (ast_to_filtermodel) : {cond.column!r}" + ) + return None + + col_type = schema[cond.column] + if col_type.is_numeric(): + filter_type = "number" + elif col_type == pl.Date: + filter_type = "date" + else: + filter_type = "text" + + if filter_type in ("number", "date") and cond.operator == "range": + if filter_type == "number": + return { + "filterType": "number", + "type": "inRange", + "filter": cond.value, + "filterTo": cond.value2, + } + return { + "filterType": "date", + "type": "inRange", + "dateFrom": cond.value, + "dateTo": cond.value2, + } + + if filter_type == "date": + ag_type = _NUM_TYPE_INV.get(cond.operator) + if ag_type is None: + logger.warning( + f"Opérateur sans équivalent AG Grid ignoré : {cond.operator!r} " + f"(colonne {cond.column!r})" + ) + return None + return {"filterType": "date", "type": ag_type, "dateFrom": cond.value} + + if filter_type == "number": + ag_type = _NUM_TYPE_INV.get(cond.operator) + if ag_type is None: + logger.warning( + f"Opérateur sans équivalent AG Grid ignoré : {cond.operator!r} " + f"(colonne {cond.column!r})" + ) + return None + return {"filterType": "number", "type": ag_type, "filter": cond.value} + + # texte + ag_type = _TEXT_TYPE_INV.get(cond.operator) + if ag_type is None: + logger.warning( + f"Opérateur sans équivalent AG Grid ignoré : {cond.operator!r} " + f"(colonne {cond.column!r})" + ) + return None + return {"filterType": "text", "type": ag_type, "filter": cond.value} + + +def _child_to_filterspec(child, schema: pl.Schema): + """Convertit un enfant du And de haut niveau en (colonne, spec filterModel). + + Ne sait inverser que les deux formes produites par `filtermodel_to_ast` : + une Condition seule, ou un And/Or à 2 enfants portant sur la MÊME colonne + (filtre AG Grid natif à deux conditions). Toute autre forme (Not, And/Or + imbriqué plus profondément, colonnes différentes, plus de 2 enfants) est + ignorée avec un warning : un filterModel est par nature par-colonne et ne + peut représenter une expression booléenne arbitraire (cf. #97). + """ + if isinstance(child, Condition): + spec = _condition_to_filterspec(child, schema) + if spec is None: + return None + return child.column, spec + + if isinstance(child, (And, Or)) and len(child.children) == 2: + c1, c2 = child.children + if ( + isinstance(c1, Condition) + and isinstance(c2, Condition) + and c1.column == c2.column + ): + spec1 = _condition_to_filterspec(c1, schema) + spec2 = _condition_to_filterspec(c2, schema) + if spec1 is None or spec2 is None: + return None + operator = "AND" if isinstance(child, And) else "OR" + return c1.column, { + "filterType": spec1["filterType"], + "operator": operator, + "condition1": spec1, + "condition2": spec2, + } + + logger.warning(f"Nœud AST non représentable en filterModel, ignoré : {child!r}") + return None + + +def ast_to_filtermodel(node: Node, schema: pl.Schema) -> dict: + """Traduit un AST en filterModel AG Grid. Inverse de `filtermodel_to_ast`. + + Seules les formes que `filtermodel_to_ast` peut effectivement produire sont + garanties d'être inversées correctement (voir `_child_to_filterspec`). + """ + if node is None: + return {} + if isinstance(node, And) and not node.children: + return {} + + # Le niveau supérieur est normalement un And multi-enfants (un enfant par + # colonne filtrée) ; on tolère aussi un nœud "nu" (une seule colonne). + children = node.children if isinstance(node, And) else [node] + + filter_model: dict = {} + for child in children: + entry = _child_to_filterspec(child, schema) + if entry is None: + continue + column, spec = entry + filter_model[column] = spec + return filter_model + + def sort_model_to_sql(sort_model: list | None, schema: pl.Schema) -> str: """Traduit un sortModel AG Grid en clause ORDER BY DuckDB (adapte à sort_by_to_sql).""" if not sort_model: diff --git a/tests/saved_views/test_apply_saved_view.py b/tests/saved_views/test_apply_saved_view.py index 47f8d6c..0ebabaf 100644 --- a/tests/saved_views/test_apply_saved_view.py +++ b/tests/saved_views/test_apply_saved_view.py @@ -1,10 +1,16 @@ """Régression revue finale #41 : apply_saved_view (callback qui RAPPELLE une vue sauvegardée) ne doit pas planter si row["query"] est encore au format pré-migration (query string, ex. "filtres=a&tris=b"), stocké par l'ancienne -build_view_query avant que Task 10 ne migre save_view vers du JSON -{"filterModel": ..., "columnState": ...}. +build_view_query avant que Task 10 ne migre save_view vers du JSON. + +Depuis le round 2 de la revue finale, le format JSON stocké est +{"ast": ..., "columnState": ...} (AST canonique, indépendant de l'UI) plutôt +que {"filterModel": ..., "columnState": ...} (filterModel brut d'AG Grid) : +cf. spec de conception. apply_saved_view doit aussi resynchroniser +tableau-hidden-columns à partir du columnState rappelé. """ +import json from unittest.mock import patch import dash @@ -13,6 +19,7 @@ import src.app # noqa: F401 # instancie l'app → register_page() des pages from src.auth import db as auth_db from src.pages import tableau from src.saved_views import db as saved_views_db +from src.utils.query_ast import And, Condition, ast_to_dict def _make_user(email="u@ex.fr"): @@ -41,21 +48,26 @@ def test_apply_saved_view_old_format_returns_no_update(monkeypatch, users_db_pat monkeypatch.setattr(tableau, "ctx", _Ctx) with patch.object(tableau, "current_user", _fake_user(uid)): - filter_model, column_state = tableau.apply_saved_view( + filter_model, column_state, hidden_columns = tableau.apply_saved_view( [1], [{"type": "saved-view-item", "index": view_id}] ) assert filter_model is dash.no_update assert column_state is dash.no_update + assert hidden_columns is dash.no_update def test_apply_saved_view_new_format_returns_view(monkeypatch, users_db_path): + """row["query"] au format post-round-2 : {"ast": ..., "columnState": ...}, + AST canonique plutôt que filterModel brut d'AG Grid.""" saved_views_db.init_schema() uid = _make_user() - query = ( - '{"filterModel": {"objet": {"filterType": "text", "filter": "route"}}, ' - '"columnState": [{"colId": "montant", "sort": "desc"}]}' - ) + ast = And([Condition("objet", "contains", "route")]) + column_state = [ + {"colId": "montant", "sort": "desc"}, + {"colId": "acheteur_nom", "hide": True}, + ] + query = json.dumps({"ast": ast_to_dict(ast), "columnState": column_state}) saved_views_db.upsert(uid, "tableau", "Vue récente", query) view_id = saved_views_db.list_views(uid, "tableau")[0]["id"] @@ -63,9 +75,46 @@ def test_apply_saved_view_new_format_returns_view(monkeypatch, users_db_path): monkeypatch.setattr(tableau, "ctx", _Ctx) with patch.object(tableau, "current_user", _fake_user(uid)): - filter_model, column_state = tableau.apply_saved_view( + filter_model, returned_column_state, hidden_columns = tableau.apply_saved_view( [1], [{"type": "saved-view-item", "index": view_id}] ) - assert filter_model == {"objet": {"filterType": "text", "filter": "route"}} - assert column_state == [{"colId": "montant", "sort": "desc"}] + assert filter_model == { + "objet": {"filterType": "text", "type": "contains", "filter": "route"} + } + assert returned_column_state == column_state + # tableau-hidden-columns doit être resynchronisé à partir du columnState + # rappelé (revue finale #41, round 2) : seules les colonnes avec hide=True. + assert hidden_columns == ["acheteur_nom"] + + +def test_apply_saved_view_missing_ast_key_degrades_gracefully( + monkeypatch, users_db_path +): + """Vue stockée dans un format intermédiaire (sans clé "ast", ex. l'ancien + format {"filterModel": ..., "columnState": ...} produit avant le round 2) : + ast_from_dict(None) -> None, ast_to_filtermodel(None, schema) -> {} — la + vue se rappelle sans filtre plutôt que de planter le callback.""" + saved_views_db.init_schema() + uid = _make_user() + column_state = [{"colId": "montant", "sort": "desc"}] + query = json.dumps( + { + "filterModel": {"objet": {"filterType": "text", "filter": "route"}}, + "columnState": column_state, + } + ) + saved_views_db.upsert(uid, "tableau", "Vue ancien format", query) + view_id = saved_views_db.list_views(uid, "tableau")[0]["id"] + + _Ctx.triggered_id = {"type": "saved-view-item", "index": view_id} + monkeypatch.setattr(tableau, "ctx", _Ctx) + + with patch.object(tableau, "current_user", _fake_user(uid)): + filter_model, returned_column_state, hidden_columns = tableau.apply_saved_view( + [1], [{"type": "saved-view-item", "index": view_id}] + ) + + assert filter_model == {} + assert returned_column_state == column_state + assert hidden_columns == [] diff --git a/tests/test_grid.py b/tests/test_grid.py index 89abf48..70cc5e9 100644 --- a/tests/test_grid.py +++ b/tests/test_grid.py @@ -1,10 +1,38 @@ from unittest.mock import patch +import pytest + import src.app # noqa: F401 # instancie l'app → register_page() des pages from src.pages.tableau import get_rows_tableau +from src.utils import grid as grid_module from src.utils.grid import export_dataframe, fetch_grid_page, grid_column_defs +@pytest.fixture(scope="module") +def flask_app(): + """Minimal Flask app with SimpleCache so @cache.memoize() works in tests + (même pattern que tests/test_table.py).""" + from flask import Flask + + from src.utils.cache import cache + + app = Flask(__name__) + cache.init_app(app, config={"CACHE_TYPE": "SimpleCache"}) + return app + + +@pytest.fixture(autouse=True) +def reset_cache(flask_app): + from src.utils.cache import cache + + with flask_app.app_context(): + try: + cache.clear() + except (RuntimeError, AttributeError): + pass + yield + + def test_column_defs_have_field_and_filter(): defs = grid_column_defs(hidden_columns=[]) by_field = {d["field"]: d for d in defs} @@ -68,3 +96,26 @@ def test_get_rows_tableau_tracks_search_once_per_filter_not_per_scroll_block(): get_rows_tableau({"filterModel": fm, "startRow": 100, "endRow": 200}) get_rows_tableau({"filterModel": fm, "startRow": 200, "endRow": 300}) mocked.assert_called_once() + + +def test_fetch_grid_page_caches_count_across_scroll_blocks(flask_app, monkeypatch): + """Régression revue finale #41 : count_marches ne doit être appelé qu'une + fois pour des blocs de défilement successifs partageant le même + where_sql/params (même filtre, start_row différent).""" + call_count = {"n": 0} + real_count_marches = grid_module.count_marches + + def counting_count_marches(where_sql, params): + call_count["n"] += 1 + return real_count_marches(where_sql, params) + + monkeypatch.setattr(grid_module, "count_marches", counting_count_marches) + + fm = {"objet": {"filterType": "text", "type": "contains", "filter": "route"}} + with flask_app.app_context(): + _, total1 = fetch_grid_page(fm, None, 0, 20) + _, total2 = fetch_grid_page(fm, None, 20, 40) + _, total3 = fetch_grid_page(fm, None, 40, 60) + + assert call_count["n"] == 1 + assert total1 == total2 == total3 diff --git a/tests/test_query_ast.py b/tests/test_query_ast.py index 60f4a4e..3c27e4e 100644 --- a/tests/test_query_ast.py +++ b/tests/test_query_ast.py @@ -7,6 +7,7 @@ from src.utils.query_ast import ( Or, ast_from_dict, ast_to_dict, + ast_to_filtermodel, ast_to_sql, filtermodel_to_ast, sort_model_to_sql, @@ -235,3 +236,103 @@ def test_ast_dict_roundtrip(): def test_ast_dict_none(): assert ast_to_dict(None) is None assert ast_from_dict(None) is None + + +def _roundtrip_sql(fm): + """Compile fm -> ast -> filterModel -> ast à nouveau, renvoie (sql, params) + de la première et de la seconde compilation, pour vérifier l'équivalence + sémantique du round-trip (pas l'égalité dict-à-dict).""" + ast1 = filtermodel_to_ast(fm, SCHEMA) + rebuilt_fm = ast_to_filtermodel(ast1, SCHEMA) + ast2 = filtermodel_to_ast(rebuilt_fm, SCHEMA) + return ast_to_sql(ast1, SCHEMA), ast_to_sql(ast2, SCHEMA) + + +def test_ast_to_filtermodel_roundtrip_text_contains(): + fm = {"objet": {"filterType": "text", "type": "contains", "filter": "voirie"}} + original, rebuilt = _roundtrip_sql(fm) + assert original == rebuilt + + +def test_ast_to_filtermodel_roundtrip_number_greaterthan(): + fm = {"montant": {"filterType": "number", "type": "greaterThan", "filter": 40000}} + original, rebuilt = _roundtrip_sql(fm) + assert original == rebuilt + + +def test_ast_to_filtermodel_roundtrip_number_inrange(): + fm = { + "montant": { + "filterType": "number", + "type": "inRange", + "filter": 100, + "filterTo": 200, + } + } + original, rebuilt = _roundtrip_sql(fm) + assert original == rebuilt + + +def test_ast_to_filtermodel_roundtrip_date_greaterthan(): + fm = { + "dateNotification": { + "filterType": "date", + "type": "greaterThan", + "dateFrom": "2022-01-01", + } + } + original, rebuilt = _roundtrip_sql(fm) + assert original == rebuilt + + +def test_ast_to_filtermodel_roundtrip_two_conditions_or(): + fm = { + "objet": { + "filterType": "text", + "operator": "OR", + "condition1": {"filterType": "text", "type": "contains", "filter": "beton"}, + "condition2": { + "filterType": "text", + "type": "contains", + "filter": "ciment", + }, + } + } + original, rebuilt = _roundtrip_sql(fm) + assert original == rebuilt + + +def test_ast_to_filtermodel_roundtrip_multiple_columns(): + fm = { + "objet": {"filterType": "text", "type": "contains", "filter": "voirie"}, + "montant": {"filterType": "number", "type": "greaterThan", "filter": 1000}, + } + original, rebuilt = _roundtrip_sql(fm) + assert original == rebuilt + + +def test_ast_to_filtermodel_none_and_empty_and(): + assert ast_to_filtermodel(None, SCHEMA) == {} + assert ast_to_filtermodel(And([]), SCHEMA) == {} + + +def test_ast_to_filtermodel_skips_not_with_warning(): + node = And([Not(Condition("objet", "contains", "x"))]) + assert ast_to_filtermodel(node, SCHEMA) == {} + + +def test_ast_to_filtermodel_skips_mismatched_columns_with_warning(): + node = And( + [Or([Condition("objet", "contains", "a"), Condition("montant", "gt", 1)])] + ) + assert ast_to_filtermodel(node, SCHEMA) == {} + + +def test_ast_to_filtermodel_bare_single_condition(): + """filtermodel_to_ast enveloppe toujours dans And, mais on tolère un nœud + non enveloppé (Condition seule) en entrée, défensivement.""" + node = Condition("objet", "contains", "voirie") + fm = ast_to_filtermodel(node, SCHEMA) + assert fm == { + "objet": {"filterType": "text", "type": "contains", "filter": "voirie"} + }