From 87baabacbdbcd7857893970bb8044c3a1e500040 Mon Sep 17 00:00:00 2001 From: psaesha Date: Sat, 14 Nov 2026 01:03:47 +0530 Subject: [PATCH 1/7] feat(data-index): give every upload its own table and switch over safely Table names were built from the resource name and dataset title, and Postgres keeps only the first 63 bytes of a name, so two resources with long similar names shared one table and a re-upload of one replaced the other's data. Re-uploads also dropped the old table before the new one existed, so readers saw empty or half-written data and a failed re-upload lost it. Each upload now gets a table named rdt_. index_resource_data builds the new table in one data-database transaction, then switches the resource over in one main-database commit (resource row locked, stored file re-checked so the newest file wins, column rows rebuilt with errors escaping) and confirms the new row exists; anything short of that drops this build's table and keeps the old data. A post_delete receiver on ResourceDataTable drops the old table after commit, which also frees storage on resource, dataset and admin deletes; it leaves legacy tables two rows still share. update_dataindex no longer wraps each call in a transaction, and the flow refuses to run inside one. Tests run against a real Postgres when TEST_DATA_DB_URL is set; CI adds a postgres service and fails instead of skipping them. Plan: plans/claude/data-index-table-names.md (Phase 1) --- .github/workflows/django-tests.yml | 18 + api/management/commands/update_dataindex.py | 7 +- api/models/Resource.py | 8 +- api/signals/__init__.py | 1 + api/signals/data_table_signals.py | 33 ++ api/utils/data_indexing.py | 405 +++++------------ api/utils/data_table_names.py | 44 ++ api/utils/data_tables.py | 378 ++++++++++++++++ pytest.ini | 4 +- tests/data_index/__init__.py | 0 tests/data_index/conftest.py | 111 +++++ tests/data_index/fixtures/districts_v1.csv | 4 + tests/data_index/fixtures/districts_v2.csv | 5 + tests/data_index/fixtures/long_header.csv | 3 + tests/data_index/fixtures/out_of_range.csv | 3 + tests/data_index/fixtures/percent_header.csv | 4 + tests/data_index/routers.py | 18 + tests/data_index/support.py | 60 +++ tests/data_index/test_delete_cleanup.py | 181 ++++++++ tests/data_index/test_reader_regression.py | 59 +++ tests/data_index/test_reindex_switch.py | 434 +++++++++++++++++++ tests/data_index/test_table_names.py | 93 ++++ tests/data_index/test_upload_mutations.py | 128 ++++++ tests/test_settings.py | 12 + 24 files changed, 1697 insertions(+), 316 deletions(-) create mode 100644 api/signals/data_table_signals.py create mode 100644 api/utils/data_table_names.py create mode 100644 api/utils/data_tables.py create mode 100644 tests/data_index/__init__.py create mode 100644 tests/data_index/conftest.py create mode 100644 tests/data_index/fixtures/districts_v1.csv create mode 100644 tests/data_index/fixtures/districts_v2.csv create mode 100644 tests/data_index/fixtures/long_header.csv create mode 100644 tests/data_index/fixtures/out_of_range.csv create mode 100644 tests/data_index/fixtures/percent_header.csv create mode 100644 tests/data_index/routers.py create mode 100644 tests/data_index/support.py create mode 100644 tests/data_index/test_delete_cleanup.py create mode 100644 tests/data_index/test_reader_regression.py create mode 100644 tests/data_index/test_reindex_switch.py create mode 100644 tests/data_index/test_table_names.py create mode 100644 tests/data_index/test_upload_mutations.py diff --git a/.github/workflows/django-tests.yml b/.github/workflows/django-tests.yml index 6bca8c20..2f49c740 100644 --- a/.github/workflows/django-tests.yml +++ b/.github/workflows/django-tests.yml @@ -16,6 +16,20 @@ jobs: django-tests: runs-on: ubuntu-latest + # The data database: tests marked data_db build real tables in it. + services: + postgres: + image: postgres:16 + env: + POSTGRES_PASSWORD: postgres + ports: + - 5432:5432 + options: >- + --health-cmd pg_isready + --health-interval 5s + --health-timeout 5s + --health-retries 10 + steps: - uses: actions/checkout@v4 @@ -40,6 +54,10 @@ jobs: # tests/object_types/charts and the dataset metadata tests were left # behind by the chart consolidation and fail on main: see #189. - name: Run tests + env: + TEST_DATA_DB_URL: postgres://postgres:postgres@localhost:5432/postgres + # Fail the run instead of skipping the data_db tests if the database is missing. + REQUIRE_DATA_DB_TESTS: "1" run: | pytest tests/ \ --ignore=tests/object_types/charts \ diff --git a/api/management/commands/update_dataindex.py b/api/management/commands/update_dataindex.py index af1190a4..b9647f77 100644 --- a/api/management/commands/update_dataindex.py +++ b/api/management/commands/update_dataindex.py @@ -4,7 +4,6 @@ import structlog from django.core.management.base import BaseCommand, CommandParser -from django.db import transaction from api.models.Resource import Resource, ResourceDataTable from api.utils.data_indexing import INDEXED_FORMATS, index_resource_data @@ -125,9 +124,9 @@ def handle(self, *args: Any, **options: Dict[str, Any]) -> None: f"[{i}/{total_resources}] Processing resource {resource.id} - {resource.name}" ) - # Update dataindex for the resource - with transaction.atomic(): - data_table = index_resource_data(resource) + # Not inside a transaction: the flow commits its own switch and refuses + # to run inside an outer one (a failed resource keeps its old data). + data_table = index_resource_data(resource) if data_table: success_count += 1 diff --git a/api/models/Resource.py b/api/models/Resource.py index 2ed78501..f3f54d3a 100644 --- a/api/models/Resource.py +++ b/api/models/Resource.py @@ -11,6 +11,7 @@ logger = structlog.getLogger(__name__) +from api.utils.data_table_names import data_table_name from api.utils.enums import DataType from api.utils.storage import private_storage @@ -101,11 +102,8 @@ def __str__(self): def save(self, *args, **kwargs): if not self.table_name: - # Generate a unique table name based on resource ID - safe_name = slugify(self.resource.name)[:50] - safe_title = slugify(self.resource.dataset.title)[:50] - resource_id = self.resource.id.hex[:8] # Use shorter ID - self.table_name = f"{safe_name}_{safe_title}_{resource_id}" + # Named from this row's own id: unique, and never cut by Postgres's 63-byte limit. + self.table_name = data_table_name(self.id) super().save(*args, **kwargs) diff --git a/api/signals/__init__.py b/api/signals/__init__.py index 3d504230..141dbf97 100644 --- a/api/signals/__init__.py +++ b/api/signals/__init__.py @@ -1,6 +1,7 @@ # Import signals to register them from api.signals import ( aimodel_signals, + data_table_signals, dataset_signals, event_signals, publication_signals, diff --git a/api/signals/data_table_signals.py b/api/signals/data_table_signals.py new file mode 100644 index 00000000..655ebec0 --- /dev/null +++ b/api/signals/data_table_signals.py @@ -0,0 +1,33 @@ +"""Drop a data table once the row that points at it is deleted. + +Re-uploads, resource deletes, dataset deletes (by cascade) and admin deletes +all delete the resource's ResourceDataTable row, so this one receiver frees the +storage for every one of them. The drop waits for the delete to commit, so a +rolled-back delete keeps its data, and it leaves a legacy table alone while +another row still shares it. + +Registered in api/signals/__init__.py (a test proves it is connected once the +app loads). Docs: ../utils/data_indexing_architecture.md +""" + +from functools import partial +from typing import Any + +from django.db import transaction +from django.db.models.signals import post_delete +from django.dispatch import receiver + +from api.models.Resource import ResourceDataTable +from api.utils.data_tables import drop_data_table_if_unshared + + +@receiver(post_delete, sender=ResourceDataTable) +def drop_table_after_row_delete( + sender: Any, instance: ResourceDataTable, using: str, **kwargs: Any +) -> None: + # Once the delete commits, drop the row's table (unless another row still uses it). + transaction.on_commit( + partial(drop_data_table_if_unshared, instance.table_name, instance.pk), + using=using, + robust=True, + ) diff --git a/api/utils/data_indexing.py b/api/utils/data_indexing.py index 9ac20feb..63080b41 100644 --- a/api/utils/data_indexing.py +++ b/api/utils/data_indexing.py @@ -1,23 +1,49 @@ -from typing import Any, Dict, Generator, List, Optional, Tuple +"""Copying tabular uploads into the data database, and reading them back. + +The upload flow is ``index_resource_data`` below; its helpers live in +./data_tables.py. The rest of this module reads the tables (row count, +preview, column list, the data API's filtered fetch, chart queries); every +reader looks the table up from the resource's ResourceDataTable row. + +Docs: ./data_indexing_architecture.md +""" + +from typing import Any, Dict, List, Optional, Tuple import pandas as pd import structlog from django.db import connections, transaction from django.db.utils import ProgrammingError +from django.utils import timezone from psycopg2 import sql as pg_sql # type: ignore[import-untyped] from api.models.Resource import Resource, ResourceDataTable from api.models.ResourceSchema import ResourceSchema from api.types.type_preview_data import PreviewData -from api.utils.file_utils import load_tabular_data, local_copy +from api.utils.data_tables import ( # noqa: F401 — DATA_DB, INDEXED_FORMATS, get_sql_type re-exported + DATA_DB, + INDEXED_FORMATS, + NewerFileSaved, + build_data_table, + data_table_row_exists, + describe_data_table, + drop_data_table, + error_fields, + get_sql_type, + has_indexable_file, + is_inside_transaction, + load_resource_frame, + lock_resource, + new_data_table_row, + rebuild_resource_schema, + retire_data_table_row, + save_data_table_row, + stored_file_identity, +) +from api.utils.file_utils import local_copy logger = structlog.get_logger("dataspace.data_indexing") -# Use a separate database for data tables -DATA_DB = "data_db" # This should match the connection name in settings.py - -# Formats indexed into ResourceDataTable (queryable via get_row_count) -INDEXED_FORMATS = {"csv", "xls", "xlsx", "ods", "parquet", "feather", "json", "tsv"} # Formats counted by parsing the file directly (not indexed into DB) FILE_COUNT_FORMATS = {"yml", "yaml", "xml"} @@ -43,313 +69,80 @@ } -def get_sql_type(pandas_dtype: str) -> str: - """Convert pandas dtype to SQL type.""" - if "int" in pandas_dtype: - return "BIGINT" - elif "float" in pandas_dtype: - return "DOUBLE PRECISION" - elif "datetime" in pandas_dtype: - return "TIMESTAMP" - elif "bool" in pandas_dtype: - return "BOOLEAN" - else: - return "TEXT" - - -def create_table_for_resource(resource: Resource, df: pd.DataFrame) -> Optional[ResourceDataTable]: - """Create a database table for the resource data and index it.""" - try: - # Create ResourceDataTable entry first to get the table name - data_table = ResourceDataTable.objects.create(resource=resource) - table_name = data_table.table_name - - # Get column types - column_types = {col: get_sql_type(str(df[col].dtype)) for col in df.columns} - - # Create table in the data database - with connections[DATA_DB].cursor() as cursor: - # Drop table if exists - cursor.execute(f'DROP TABLE IF EXISTS "{table_name}"') - - # Create table with proper column types - columns = [f'"{col}" {dtype}' for col, dtype in column_types.items()] - create_table_sql = f'CREATE TABLE "{table_name}" ({", ".join(columns)})' - cursor.execute(create_table_sql) - - # Create temporary table for COPY - temp_table = f"temp_{table_name}" - cursor.execute(f'CREATE TEMP TABLE "{temp_table}" (LIKE "{table_name}")') - - # Copy data to temp table - quoted_columns = [f'"{col}"' for col in df.columns] - # Use StringIO to create a file-like object for the CSV data - from io import StringIO - - csv_data = StringIO() - df.to_csv(csv_data, index=False, header=False) - csv_data.seek(0) - - copy_sql = f'COPY "{temp_table}" ({",".join(quoted_columns)}) FROM STDIN WITH CSV' - cursor.copy_expert(copy_sql, csv_data) - - # Insert from temp to main table with validation - cursor.execute(f'INSERT INTO "{table_name}" SELECT * FROM "{temp_table}"') - cursor.execute(f'DROP TABLE "{temp_table}"') - - return data_table - - except Exception as e: - logger.error(f"Error creating table for resource {resource.id}: {str(e)}") - if "data_table" in locals(): - data_table.delete() - return None - - def index_resource_data(resource: Resource) -> Optional[ResourceDataTable]: - """Index a resource's CSV data into a database table. - - This function handles the indexing process gracefully, catching and logging - specific errors at each stage of the process without crashing. - - Args: - resource: The Resource object to index - - Returns: - Optional[ResourceDataTable]: The created data table or None if indexing failed + """Copy a resource's tabular file into a table of its own in the data database. + + Every upload gets a new table, named from its new ResourceDataTable row. + The table is built in full first; then one main-database commit points the + resource at it and rebuilds the column list. The old table is dropped after + that commit by the row's delete receiver. Readers see the complete old data + until the commit, then the complete new data. If anything fails, the old + data stays as it was and this build's table is dropped. + + Must be called outside any transaction, so "committed" really means + committed. Returns the new ResourceDataTable, or None when the resource + was not switched over. """ - resource_id = getattr(resource, "id", "unknown") + resource_id = str(resource.id) - try: - # Check if resource is a supported tabular file - try: - file_details = resource.resourcefiledetails - if not file_details: - logger.info(f"Resource {resource_id} has no file details, skipping indexing") - return None - except Exception as e: - logger.error(f"Failed to access file details for resource {resource_id}: {str(e)}") - return None - - # Check file format - try: - format = file_details.format.lower() - supported_formats = [ - "csv", - "xls", - "xlsx", - "ods", - "parquet", - "feather", - "json", - "tsv", - ] - if format not in supported_formats: - logger.info( - f"Resource {resource_id} has unsupported format: {format}, skipping indexing" - ) - return None - except Exception as e: - logger.error(f"Failed to determine format for resource {resource_id}: {str(e)}") - return None - - # Load tabular data with timeout protection - try: - import signal - from contextlib import contextmanager - - @contextmanager - def timeout(seconds: int) -> Generator[None, None, None]: - def handler(signum: int, frame: Any) -> None: - raise TimeoutError(f"Loading data timed out after {seconds} seconds") - - # Set the timeout handler - original_handler = signal.getsignal(signal.SIGALRM) - signal.signal(signal.SIGALRM, handler) - signal.alarm(seconds) - try: - yield - finally: - signal.alarm(0) - signal.signal(signal.SIGALRM, original_handler) - - with local_copy(file_details.file) as file_path: - # Apply timeout only on Unix-based systems where signal.SIGALRM is available - try: - with timeout(60): # 60 second timeout for loading data - df = load_tabular_data(file_path, format) - except TimeoutError as te: - logger.error( - f"Timeout while loading data for resource {resource_id}: {str(te)}" - ) - return None - except Exception: - # Fallback without timeout if signal.SIGALRM is not available (e.g., on Windows) - df = load_tabular_data(file_path, format) - - if df is None: - logger.error( - f"Failed to load data for resource {resource_id}: Data loading returned None" - ) - return None - if df.empty: - logger.info(f"Resource {resource_id} has empty data, skipping indexing") - return None - - # Check for problematic column names and sanitize them - try: - # Replace problematic characters in column names - # Create a new list of sanitized column names - sanitized_columns = [ - str(col).replace('"', "").replace("\n", " ").replace("\t", " ") - for col in df.columns - ] - # Assign to df.columns using pandas Index constructor - df.columns = pd.Index(sanitized_columns) - - # Check for duplicate column names after sanitization - if len(df.columns) != len(set(df.columns)): - # Handle duplicate columns by adding suffixes - from collections import Counter - - col_counter = Counter(df.columns) - for col, count in col_counter.items(): - if count > 1: - # Find all occurrences of this column name - indices = [i for i, x in enumerate(df.columns) if x == col] - # Rename all but the first occurrence - for i, idx in enumerate(indices[1:], 1): - df.columns.values[idx] = f"{col}_{i}" - logger.warning(f"Renamed duplicate columns in resource {resource_id}") - except Exception as e: - logger.error( - f"Failed to sanitize column names for resource {resource_id}: {str(e)}" - ) - return None - - except Exception as e: - import traceback - - logger.error( - f"Failed to load data for resource {resource_id}: {str(e)}\n{traceback.format_exc()}" - ) - return None - - # Create and index table within transaction - try: - with transaction.atomic(): - # Delete existing table if any - try: - existing_table = ResourceDataTable.objects.get(resource=resource) - try: - with connections[DATA_DB].cursor() as cursor: - cursor.execute(f'DROP TABLE IF EXISTS "{existing_table.table_name}"') - except Exception as drop_error: - logger.error( - f"Failed to drop existing table for resource {resource_id}: {str(drop_error)}" - ) - # Continue anyway as we'll try to recreate the table - - existing_table.delete() - except ResourceDataTable.DoesNotExist: - pass - except Exception as e: - logger.error( - f"Error handling existing table for resource {resource_id}: {str(e)}" - ) - # Continue with table creation - - # Create new table - data_table = create_table_for_resource(resource, df) - if not data_table: - logger.error(f"Failed to create table for resource {resource_id}") - return None - - # Update resource schema - try: - # Store existing schema descriptions to preserve them during re-indexing - existing_schemas: Dict[str, Dict[str, Optional[str]]] = {} - for schema in ResourceSchema.objects.filter( - resource=resource - ): # type: ResourceSchema - existing_schemas[schema.field_name] = { - "description": schema.description, - "format": schema.format, - } - - # Clear existing schema for this resource - ResourceSchema.objects.filter(resource=resource).delete() - - # Then create new schema entries for each column, preserving descriptions where possible - schema_success_count = 0 - schema_error_count = 0 - - for col in df.columns: - try: - # Determine format - always use the detected format from the data - sql_type = get_sql_type(str(df[col].dtype)) - - # Map SQL types to FieldTypes enum values - if sql_type == "BIGINT": - format_value = "INTEGER" - elif sql_type == "DOUBLE PRECISION": - format_value = "NUMBER" - elif sql_type == "TIMESTAMP": - format_value = "DATE" - elif sql_type == "BOOLEAN": - format_value = "BOOLEAN" - else: # TEXT or any other type - format_value = "STRING" - - # For description, preserve existing if available, otherwise auto-generate - description = f"Description of column {col}" - if col in existing_schemas: - existing_description = existing_schemas[col]["description"] - # Check for None and non-auto-generated descriptions - if existing_description is not None: - description = existing_description - logger.debug(f"Preserved custom description for column {col}") - - # Create the schema entry - ResourceSchema.objects.create( - resource=resource, - field_name=col, - format=format_value, - description=description, - ) - schema_success_count += 1 - except Exception as schema_error: - schema_error_count += 1 - logger.error( - f"Error creating schema for column {col} in resource {resource_id}: {str(schema_error)}" - ) - # Continue with other columns even if one fails - - logger.info( - f"Resource {resource_id} schema creation: {schema_success_count} columns succeeded, {schema_error_count} failed" - ) - except Exception as schema_error: - logger.error( - f"Failed to update schema for resource {resource_id}: {str(schema_error)}" - ) - # Continue and return the data_table even if schema update fails - - return data_table - except Exception as tx_error: - import traceback + # Refuse to run inside someone else's transaction: the switch would only release a savepoint. + if is_inside_transaction(): + logger.error("data_index_refused_inside_transaction", resource_id=resource_id) + return None - logger.error( - f"Transaction error for resource {resource_id}: {str(tx_error)}\n{traceback.format_exc()}" - ) - return None + # Only supported tabular files (csv, xlsx, parquet, ...) get a table. + if not has_indexable_file(resource): + return None - except Exception as e: - import traceback + # Read the file into a frame, remembering exactly which stored file it was. + loaded = load_resource_frame(resource) + if loaded is None: + return None - logger.error( - f"Unexpected error indexing resource {resource_id}: {str(e)}\n{traceback.format_exc()}" + # This upload's own row, and with it its own table name. + new_row = new_data_table_row(resource) + switched = False + try: + # Build the complete new table; readers keep using the old one meanwhile. + build_data_table( + new_row.table_name, loaded.frame, describe_data_table(resource, built_at=timezone.now()) ) + + # Switch the resource over to the new table in one commit. + with transaction.atomic(): + # Hold the resource, so two uploads of it switch one after the other. + lock_resource(resource) + + # If a newer file was saved while we built, that upload wins: back off. + if stored_file_identity(resource) != loaded.file: + raise NewerFileSaved() + + # Retire the old row; its table is dropped once this commits. + previous_created = retire_data_table_row(resource) + + # Point the resource at the new table, keeping its place as the dataset's default. + save_data_table_row(new_row, created=previous_created) + + # Rebuild the column list, keeping the descriptions people wrote. + rebuild_resource_schema(resource, loaded.frame) + + # It only counts as switched if the new row really is there after the commit. + switched = data_table_row_exists(new_row.pk) + if not switched: + logger.error("data_index_switch_not_saved", resource_id=resource_id) + except NewerFileSaved: + logger.info("data_index_skipped_newer_file", resource_id=resource_id) + except Exception as error: + logger.error("data_index_failed", resource_id=resource_id, **error_fields(error)) + finally: + # Anything short of a confirmed switch leaves this build's table unused: drop it. + if not switched: + drop_data_table(new_row.table_name) + + if not switched: return None + logger.info("data_index_switched", resource_id=resource_id, table_name=new_row.table_name) + return new_row def query_resource_data(resource: Resource, query: str) -> Optional[pd.DataFrame]: diff --git a/api/utils/data_table_names.py b/api/utils/data_table_names.py new file mode 100644 index 00000000..7be81964 --- /dev/null +++ b/api/utils/data_table_names.py @@ -0,0 +1,44 @@ +"""How tables in the data database are named. + +Every tabular upload gets its own table, named "rdt_" plus the 32-character +hex of its ResourceDataTable row id (36 characters in all). The name never +depends on a resource or dataset title, so renames don't touch it and two +uploads can never share one. + +Postgres keeps only the first 63 bytes of any name. Names from before this +rule were built from titles (up to 110 characters), so two of them can map to +the same physical table; ``physical_table_name`` gives the name Postgres +actually stored, for the checks that care. + +No imports from models: the ResourceDataTable model uses this module. +Docs: ./data_indexing_architecture.md +""" + +import uuid + +DATA_TABLE_PREFIX = "rdt_" +MAX_IDENTIFIER_BYTES = 63 +# A cut name is at least this long: a cut never drops more than a 4-byte character's first 3 bytes. +SHORTEST_CUT_NAME_BYTES = MAX_IDENTIFIER_BYTES - 3 + + +def data_table_name(row_id: uuid.UUID) -> str: + """The table name for a ResourceDataTable row: "rdt_" + its id in lowercase hex.""" + return f"{DATA_TABLE_PREFIX}{row_id.hex}" + + +def physical_table_name(name: str) -> str: + """The name Postgres stores for ``name``: its first 63 bytes, never half a character.""" + encoded = name.encode("utf-8") + if len(encoded) <= MAX_IDENTIFIER_BYTES: + return name + return encoded[:MAX_IDENTIFIER_BYTES].decode("utf-8", errors="ignore") + + +def may_share_physical_table(name: str) -> bool: + """Whether another, longer name could be cut to the same physical table as this one. + + Only names at least as long as the shortest possible cut can; "rdt_" names + (36 bytes) never can. + """ + return len(name.encode("utf-8")) >= SHORTEST_CUT_NAME_BYTES diff --git a/api/utils/data_tables.py b/api/utils/data_tables.py new file mode 100644 index 00000000..de4603bc --- /dev/null +++ b/api/utils/data_tables.py @@ -0,0 +1,378 @@ +"""Helpers for copying uploaded files into tables in the data database. + +The upload flow (``index_resource_data`` in ./data_indexing.py) reads a +resource's file, builds a brand-new table for it, then switches the resource's +ResourceDataTable row over to that table in one main-database commit. Old tables +are dropped after the commit by the row's delete receiver +(api/signals/data_table_signals.py). Each helper here does one step of that. + +Tables live in the separate ``data_db`` database (its statements autocommit); +the rows that point at them (ResourceDataTable, ResourceSchema) live in the +main database. New SQL composes names and comments with psycopg2's +``sql.Identifier`` / ``sql.Literal``, never with string formatting. + +Logs carry ids, counts and error codes only — never file contents or values. +Docs: ./data_indexing_architecture.md +""" + +import json +import os +import signal +import threading +import traceback +from collections import Counter +from datetime import datetime +from io import StringIO +from typing import Any, Dict, List, NamedTuple, Optional + +import pandas as pd +import structlog +from django.db import connections, transaction +from psycopg2 import sql as pg_sql # type: ignore[import-untyped] + +from api.models.Resource import Resource, ResourceDataTable, ResourceFileDetails +from api.models.ResourceSchema import ResourceSchema +from api.utils.data_table_names import ( + data_table_name, + may_share_physical_table, + physical_table_name, +) +from api.utils.file_utils import load_tabular_data, local_copy + +logger = structlog.get_logger("dataspace.data_tables") + +DATA_DB = "data_db" # the connection name in settings.DATABASES + +# Formats copied into a table (and so readable through the data API). +INDEXED_FORMATS = {"csv", "xls", "xlsx", "ods", "parquet", "feather", "json", "tsv"} + +# How long a drop waits for readers holding the table before giving up. +DROP_LOCK_TIMEOUT = "5s" + +# How long reading a file may take, where the platform lets us stop it. +LOAD_TIMEOUT_SECONDS = 60 + + +class FileIdentity(NamedTuple): + """Which stored file a build read: its storage name and SHA-256.""" + + name: str + sha256: Optional[str] + + +class LoadedFile(NamedTuple): + """A resource's file read into a frame, with the identity of that file.""" + + frame: pd.DataFrame + file: FileIdentity + + +class NewerFileSaved(Exception): + """Another upload saved a newer file while this one was being built.""" + + +def get_sql_type(pandas_dtype: str) -> str: + """The Postgres column type for a pandas dtype name (TEXT when unsure).""" + if "int" in pandas_dtype: + return "BIGINT" + if "float" in pandas_dtype: + return "DOUBLE PRECISION" + if "datetime" in pandas_dtype: + return "TIMESTAMP" + if "bool" in pandas_dtype: + return "BOOLEAN" + return "TEXT" + + +_FIELD_FORMATS = { + "BIGINT": "INTEGER", + "DOUBLE PRECISION": "NUMBER", + "TIMESTAMP": "DATE", + "BOOLEAN": "BOOLEAN", +} + + +def error_fields(error: BaseException) -> Dict[str, Any]: + """Log fields for an error: its class, Postgres error code and where it was raised. + + Never its message: database messages quote the offending value (a COPY + error names the cell). ``where`` is the innermost frame in our own code + ("data_tables.py:231 build_data_table"), which carries no data. + """ + cause = error.__cause__ or error + frames = traceback.extract_tb(error.__traceback__) + ours = [f for f in frames if f"{os.sep}api{os.sep}" in f.filename] or frames + where = ( + f"{os.path.basename(ours[-1].filename)}:{ours[-1].lineno} {ours[-1].name}" if ours else None + ) + return { + "error_type": type(error).__name__, + "error_code": getattr(cause, "pgcode", None), + "where": where, + } + + +def is_inside_transaction() -> bool: + """Whether the main database is already inside a transaction (an atomic block).""" + return bool(transaction.get_connection().in_atomic_block) + + +def has_indexable_file(resource: Resource) -> bool: + """Whether the resource has a stored file in a format that gets a table.""" + file_details = getattr(resource, "resourcefiledetails", None) + if not file_details or not file_details.file: + logger.info("data_index_skipped_no_file", resource_id=str(resource.id)) + return False + file_format = (file_details.format or "").lower() + if file_format not in INDEXED_FORMATS: + logger.info("data_index_skipped_format", resource_id=str(resource.id), format=file_format) + return False + return True + + +def load_resource_frame(resource: Resource) -> Optional[LoadedFile]: + """Read the resource's stored file into a frame and note which file it was. + + Column names are cleaned the way tables have always needed them: double + quotes removed, line breaks and tabs turned into spaces, and names that + then collide get "_1", "_2", ... suffixes. + + Returns None when the file can't be read, takes longer than + LOAD_TIMEOUT_SECONDS, or has no rows (each case logged). + """ + file_details = resource.resourcefiledetails + identity = FileIdentity(file_details.file.name, file_details.sha256) + try: + with local_copy(file_details.file) as path: + frame = _load_with_timeout(path, file_details.format.lower()) + except Exception as error: + logger.error("data_index_load_failed", resource_id=str(resource.id), **error_fields(error)) + return None + if frame is None: + logger.error("data_index_load_failed", resource_id=str(resource.id), error_type="NoFrame") + return None + if frame.empty: + logger.info("data_index_skipped_empty", resource_id=str(resource.id)) + return None + frame.columns = pd.Index(_clean_column_names(list(frame.columns))) + return LoadedFile(frame, identity) + + +def _load_with_timeout(path: str, file_format: str) -> Optional[pd.DataFrame]: + # SIGALRM only works in a Unix process's main thread; elsewhere (a server + # worker thread) the file loads without a limit, as it always has. + if threading.current_thread() is not threading.main_thread() or not hasattr(signal, "SIGALRM"): + return load_tabular_data(path, file_format) + + def on_alarm(signum: int, frame: Any) -> None: + raise TimeoutError(f"loading took longer than {LOAD_TIMEOUT_SECONDS} seconds") + + previous = signal.signal(signal.SIGALRM, on_alarm) + signal.alarm(LOAD_TIMEOUT_SECONDS) + try: + return load_tabular_data(path, file_format) + finally: + signal.alarm(0) + signal.signal(signal.SIGALRM, previous) + + +def _clean_column_names(columns: List[Any]) -> List[str]: + cleaned = [str(c).replace('"', "").replace("\n", " ").replace("\t", " ") for c in columns] + seen: Counter = Counter() + unique = [] + for name in cleaned: + unique.append(f"{name}_{seen[name]}" if seen[name] else name) + seen[name] += 1 + return unique + + +def stored_file_identity(resource: Resource) -> Optional[FileIdentity]: + """The resource's current stored file, read fresh from the database.""" + row = ( + ResourceFileDetails.objects.filter(resource_id=resource.pk) + .values_list("file", "sha256") + .first() + ) + return FileIdentity(*row) if row else None + + +def new_data_table_row(resource: Resource) -> ResourceDataTable: + """An unsaved ResourceDataTable row for this upload, already carrying its table name.""" + row = ResourceDataTable(resource=resource) + row.table_name = data_table_name(row.id) + return row + + +def describe_data_table(resource: Resource, built_at: datetime) -> str: + """The comment stored on a new table, so people browsing the data database know what it holds. + + JSON with the resource id and name, the dataset title and the build time; + the migration command reads ``built_at`` to leave freshly built tables alone. + """ + return json.dumps( + { + "resource_id": str(resource.id), + "resource": resource.name, + "dataset": resource.dataset.title, + "built_at": built_at.isoformat(), + }, + ensure_ascii=False, + ) + + +def build_data_table(table_name: str, frame: pd.DataFrame, comment: str) -> None: + """Create a table holding every row of the frame, in one data-database transaction. + + Creates the table, copies the rows in through a temporary table, and + stores the comment. Raises on any failure, and then nothing is left + behind: a killed process or a bad value mid-copy rolls the whole table back. + """ + table = pg_sql.Identifier(table_name) + staging = pg_sql.Identifier(f"tmp_{table_name}") + columns = pg_sql.SQL(", ").join( + pg_sql.SQL("{} {}").format( + pg_sql.Identifier(str(col)), pg_sql.SQL(get_sql_type(str(frame[col].dtype))) + ) + for col in frame.columns + ) + rows = StringIO() + frame.to_csv(rows, index=False, header=False) + rows.seek(0) + + with transaction.atomic(using=DATA_DB), connections[DATA_DB].cursor() as cursor: + cursor.execute(pg_sql.SQL("CREATE TABLE {} ({})").format(table, columns)) + cursor.execute( + pg_sql.SQL("CREATE TEMP TABLE {} (LIKE {}) ON COMMIT DROP").format(staging, table) + ) + cursor.copy_expert(pg_sql.SQL("COPY {} FROM STDIN WITH CSV").format(staging), rows) + cursor.execute(pg_sql.SQL("INSERT INTO {} SELECT * FROM {}").format(table, staging)) + cursor.execute( + pg_sql.SQL("COMMENT ON TABLE {} IS {}").format(table, pg_sql.Literal(comment)) + ) + + +def lock_resource(resource: Resource) -> None: + """Lock the resource's row until the surrounding transaction ends. + + Two uploads of one resource then switch one after the other. A no-op on + SQLite (tests); Postgres in production. + """ + Resource.objects.select_for_update().only("id").get(pk=resource.pk) + + +def retire_data_table_row(resource: Resource) -> Optional[datetime]: + """Delete the resource's current ResourceDataTable row, returning when it was created. + + Its table is dropped by the delete receiver once the surrounding + transaction commits. Returns None when the resource had no row. + """ + old = ResourceDataTable.objects.filter(resource_id=resource.pk).first() + if old is None: + return None + created = old.created + old.delete() + return created + + +def save_data_table_row(row: ResourceDataTable, created: Optional[datetime]) -> None: + """Save the new row, keeping the retired row's creation time when there was one. + + The data API picks a dataset's default table by creation time, so a + re-upload keeps the dataset's default where it was. + """ + row.save(force_insert=True) + if created is not None: + ResourceDataTable.objects.filter(pk=row.pk).update(created=created) + row.created = created + + +def rebuild_resource_schema(resource: Resource, frame: pd.DataFrame) -> None: + """Replace the resource's column rows with one per frame column. + + Each column's type comes from the frame. Descriptions people wrote are + carried over to columns with the same name; new columns get + "Description of column ". + + Errors escape: inside the switch transaction a swallowed database error + would silently roll the whole switch back. A header longer than the column + row allows is refused up front (ValueError naming its position), the same + outcome Postgres would give, on every database. + """ + max_length = ResourceSchema._meta.get_field("field_name").max_length or 255 + for position, column in enumerate(frame.columns, start=1): + if len(str(column)) > max_length: + raise ValueError(f"column {position} header is longer than {max_length} characters") + + existing = ResourceSchema.objects.filter(resource=resource) + descriptions = { + name: description + for name, description in existing.values_list("field_name", "description") + if description is not None + } + existing.delete() + for column in frame.columns: + sql_type = get_sql_type(str(frame[column].dtype)) + ResourceSchema.objects.create( + resource=resource, + field_name=column, + format=_FIELD_FORMATS.get(sql_type, "STRING"), + description=descriptions.get(column, f"Description of column {column}"), + ) + + +def data_table_row_exists(row_id: Any) -> bool: + """Whether a ResourceDataTable row with this id is saved.""" + return ResourceDataTable.objects.filter(pk=row_id).exists() + + +def drop_data_table(table_name: str) -> bool: + """Drop a table from the data database; never raises. + + Waits at most DROP_LOCK_TIMEOUT for readers holding the table. A failure + is logged with the table name and swallowed — the table is left behind as + an orphan for ``migrate_data_table_names`` to report. Returns whether the + drop went through (a table that was already gone counts as dropped). + """ + try: + with transaction.atomic(using=DATA_DB), connections[DATA_DB].cursor() as cursor: + cursor.execute( + pg_sql.SQL("SET LOCAL lock_timeout = {}").format(pg_sql.Literal(DROP_LOCK_TIMEOUT)) + ) + cursor.execute( + pg_sql.SQL("DROP TABLE IF EXISTS {}").format(pg_sql.Identifier(table_name)) + ) + except Exception as error: + logger.error("data_table_drop_failed", table_name=table_name, **error_fields(error)) + return False + logger.info("data_table_dropped", table_name=table_name) + return True + + +def drop_data_table_if_unshared(table_name: str, row_id: Any) -> None: + """Drop a deleted row's table, unless another row still uses the same physical table. + + Only names from before per-upload tables can share one (Postgres cut two + long names to the same 63 bytes); for those, the table stays until its last + row is gone. Query: ``table_name LIKE '%'`` on the unique + table_name index — a sequential scan on some collations, over one row per + tabular resource. Never raises. + """ + try: + shared = may_share_physical_table(table_name) and _another_row_uses(table_name, row_id) + except Exception as error: + logger.error("data_table_drop_failed", table_name=table_name, **error_fields(error)) + return + if shared: + logger.info("data_table_drop_skipped_shared", table_name=table_name, row_id=str(row_id)) + return + drop_data_table(table_name) + + +def _another_row_uses(table_name: str, row_id: Any) -> bool: + physical = physical_table_name(table_name) + candidates = ( + ResourceDataTable.objects.filter(table_name__startswith=physical) + .exclude(pk=row_id) + .values_list("table_name", flat=True) + ) + return any(physical_table_name(name) == physical for name in candidates) diff --git a/pytest.ini b/pytest.ini index b493e2f9..634db965 100644 --- a/pytest.ini +++ b/pytest.ini @@ -6,4 +6,6 @@ python_functions = test_* testpaths = tests pythonpath = . django_find_project = true -addopts = -v --nomigrations --tb=short \ No newline at end of file +addopts = -v --nomigrations --tb=short +markers = + data_db: builds tables in the Postgres data database; needs TEST_DATA_DB_URL (see tests/data_index/conftest.py) diff --git a/tests/data_index/__init__.py b/tests/data_index/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/data_index/conftest.py b/tests/data_index/conftest.py new file mode 100644 index 00000000..7703b854 --- /dev/null +++ b/tests/data_index/conftest.py @@ -0,0 +1,111 @@ +"""Shared setup for the data-index tests. + +Tests marked ``data_db`` build real tables in a Postgres data database. They +need TEST_DATA_DB_URL (see tests/test_settings.py), for example: + + docker run -d --name dataspace-test-data-db -e POSTGRES_PASSWORD=postgres \ + -p 55432:5432 postgres:16 + export TEST_DATA_DB_URL=postgres://postgres:postgres@localhost:55432/postgres + +Without it they are skipped, unless REQUIRE_DATA_DB_TESTS=1 (set in CI), in +which case the run fails: a green run that skipped them proves nothing. + +The main database stays SQLite, so row locks are no-ops here; the lock order is +proven by the flow tests and its effect by the journey script. +""" + +import os +import types +from typing import Any, Callable, List, Optional + +import pytest +from django.conf import settings +from django.core.files.base import ContentFile +from django.db import connections + +from api.models import Dataset, Organization, Resource, ResourceFileDetails +from authorization.models import User +from tests.data_index.support import ( + DATA_DB, + fixture_bytes, + list_public_tables, + table_comment, + table_exists, + table_rows, +) + + +def pytest_collection_modifyitems(config: pytest.Config, items: List[pytest.Item]) -> None: + if DATA_DB in settings.DATABASES: + return + data_db_items = [item for item in items if item.get_closest_marker("data_db")] + if not data_db_items: + return + if os.environ.get("REQUIRE_DATA_DB_TESTS") == "1": + raise pytest.UsageError( + f"{len(data_db_items)} data_db tests would be skipped: set TEST_DATA_DB_URL " + "(REQUIRE_DATA_DB_TESTS=1 forbids skipping them)." + ) + skip = pytest.mark.skip(reason="needs TEST_DATA_DB_URL (a Postgres data database)") + for item in data_db_items: + item.add_marker(skip) + + +@pytest.fixture(autouse=True) +def empty_data_db_after_test(request: pytest.FixtureRequest, django_db_blocker: Any) -> Any: + """Drop every table a data_db test left behind. + + pytest-django only empties tables of models routed to a database, and none + are routed to the data database, so without this each test would see the + tables of the tests before it. + """ + yield + if not request.node.get_closest_marker("data_db") or DATA_DB not in settings.DATABASES: + return + with django_db_blocker.unblock(): + for name in list_public_tables(): + with connections[DATA_DB].cursor() as cursor: + cursor.execute(f'DROP TABLE IF EXISTS "{name}" CASCADE') + + +@pytest.fixture +def tables() -> types.SimpleNamespace: + """Read-only peeks into the test data database.""" + return types.SimpleNamespace( + names=list_public_tables, exists=table_exists, rows=table_rows, comment=table_comment + ) + + +@pytest.fixture +def owner(db: Any) -> User: + return User.objects.create(username="owner", keycloak_id="owner") + + +@pytest.fixture +def dataset(owner: User) -> Dataset: + org = Organization.objects.create(name="Org", description="o", slug="org") + return Dataset.objects.create( + title="District health indicators", + slug="dhi", + description="d", + user=owner, + organization=org, + ) + + +@pytest.fixture +def add_resource(dataset: Dataset) -> Callable[..., Resource]: + """Create a resource holding a CSV file (fixture file name or raw bytes).""" + + def add(name: str, csv: Any, in_dataset: Optional[Dataset] = None) -> Resource: + content = fixture_bytes(csv) if isinstance(csv, str) else csv + resource = Resource.objects.create(name=name, dataset=in_dataset or dataset) + ResourceFileDetails.objects.create( + resource=resource, + file=ContentFile(content, name=f"{resource.id.hex}.csv"), + size=len(content), + format="csv", + ) + return resource + + return add diff --git a/tests/data_index/fixtures/districts_v1.csv b/tests/data_index/fixtures/districts_v1.csv new file mode 100644 index 00000000..1cb01ba4 --- /dev/null +++ b/tests/data_index/fixtures/districts_v1.csv @@ -0,0 +1,4 @@ +district,cases +Kamrup,10 +Nagaon,20 +Jorhat,30 diff --git a/tests/data_index/fixtures/districts_v2.csv b/tests/data_index/fixtures/districts_v2.csv new file mode 100644 index 00000000..e02f0ba8 --- /dev/null +++ b/tests/data_index/fixtures/districts_v2.csv @@ -0,0 +1,5 @@ +district,cases +Kamrup,11 +Nagaon,22 +Jorhat,33 +Dibrugarh,44 diff --git a/tests/data_index/fixtures/long_header.csv b/tests/data_index/fixtures/long_header.csv new file mode 100644 index 00000000..15305323 --- /dev/null +++ b/tests/data_index/fixtures/long_header.csv @@ -0,0 +1,3 @@ +district,Prevalence of anaemia among women aged 15 to 49 years who were tested during the survey round Prevalence of anaemia among women aged 15 to 49 years who were tested during the survey round Prevalence of anaemia among women aged 15 to 49 years who were tested during the survey round Prevalence of anae +Kamrup,1 +Nagaon,2 diff --git a/tests/data_index/fixtures/out_of_range.csv b/tests/data_index/fixtures/out_of_range.csv new file mode 100644 index 00000000..f8d6615a --- /dev/null +++ b/tests/data_index/fixtures/out_of_range.csv @@ -0,0 +1,3 @@ +district,count +Kamrup,1 +Nagaon,18446744073709551615 diff --git a/tests/data_index/fixtures/percent_header.csv b/tests/data_index/fixtures/percent_header.csv new file mode 100644 index 00000000..1b9e7886 --- /dev/null +++ b/tests/data_index/fixtures/percent_header.csv @@ -0,0 +1,4 @@ +district,Literacy rate (%),year +Kamrup,81.5,2021 +Nagaon,72.0,2021 +Jorhat,85.25,2022 diff --git a/tests/data_index/routers.py b/tests/data_index/routers.py new file mode 100644 index 00000000..56d4dcf7 --- /dev/null +++ b/tests/data_index/routers.py @@ -0,0 +1,18 @@ +"""Keep Django's own tables out of the test data database. + +In production the data database holds only data tables (one per tabular +upload). The router mirrors that in tests, so pytest-django creates no model +tables there and an orphan or "is this the main database?" check sees what +production would see. +""" + +from typing import Any, Optional + + +class DataDbTestRouter: + def allow_migrate( + self, db: str, app_label: str, model_name: Optional[str] = None, **hints: Any + ) -> Optional[bool]: + if db == "data_db": + return False + return None diff --git a/tests/data_index/support.py b/tests/data_index/support.py new file mode 100644 index 00000000..d9a04ef0 --- /dev/null +++ b/tests/data_index/support.py @@ -0,0 +1,60 @@ +"""Helpers shared by the data-index tests (fixtures live in conftest.py).""" + +from pathlib import Path +from typing import Any, List, Optional + +import pytest +from django.core.files.base import ContentFile +from django.db import connections + +from api.models import Resource + +FIXTURES = Path(__file__).parent / "fixtures" +DATA_DB = "data_db" + + +def uses_data_db(test: Any) -> Any: + """Mark a test that builds tables in the data database. + + It needs real commits (to see on_commit drops and the switch) and both + databases. Skipped without TEST_DATA_DB_URL; see conftest.py. + """ + test = pytest.mark.django_db(transaction=True, databases=["default", DATA_DB])(test) + return pytest.mark.data_db(test) + + +def list_public_tables() -> List[str]: + with connections[DATA_DB].cursor() as cursor: + cursor.execute("SELECT tablename FROM pg_tables WHERE schemaname = 'public'") + return sorted(row[0] for row in cursor.fetchall()) + + +def table_exists(name: str) -> bool: + with connections[DATA_DB].cursor() as cursor: + cursor.execute("SELECT to_regclass(%s) IS NOT NULL", [f'public."{name}"']) + return bool(cursor.fetchone()[0]) + + +def table_rows(name: str, order_by: str = "1") -> List[tuple]: + with connections[DATA_DB].cursor() as cursor: + cursor.execute(f'SELECT * FROM "{name}" ORDER BY {order_by}') + return cursor.fetchall() + + +def table_comment(name: str) -> Optional[str]: + with connections[DATA_DB].cursor() as cursor: + cursor.execute("SELECT obj_description(to_regclass(%s), 'pg_class')", [f'public."{name}"']) + return cursor.fetchone()[0] + + +def fixture_bytes(name: str) -> bytes: + return (FIXTURES / name).read_bytes() + + +def replace_file(resource: Resource, csv: Any) -> None: + """Save a new file for the resource, as an upload request does before indexing.""" + content = fixture_bytes(csv) if isinstance(csv, str) else csv + details = resource.resourcefiledetails + details.file = ContentFile(content, name=f"{resource.id.hex}.csv") + details.size = len(content) + details.save() diff --git a/tests/data_index/test_delete_cleanup.py b/tests/data_index/test_delete_cleanup.py new file mode 100644 index 00000000..fd19926c --- /dev/null +++ b/tests/data_index/test_delete_cleanup.py @@ -0,0 +1,181 @@ +"""Deleting a data-table row drops its table once the delete commits. + +Root cause this guards against: deleting a resource or a dataset never removed +its data tables, so the data database kept every table ever built. The one +receiver on the row's delete also covers re-uploads and admin deletes. +""" + +import os +import subprocess +import sys +from functools import partial +from pathlib import Path +from typing import Any +from unittest.mock import patch + +import psycopg2 +from django.db import connections, transaction +from structlog.testing import capture_logs + +from api.models import ResourceDataTable +from api.signals.data_table_signals import drop_table_after_row_delete +from api.utils.data_indexing import index_resource_data +from api.utils.data_tables import drop_data_table_if_unshared +from tests.data_index.support import uses_data_db + +REPO_ROOT = Path(__file__).resolve().parents[2] +SHARED_PREFIX = "district-wise-prevalence-of-anaemia-among-women-aged-15-49-dist" # 63 bytes + + +def create_legacy_table(name: str) -> None: + with connections["data_db"].cursor() as cursor: + cursor.execute(f'CREATE TABLE "{name}" (district text)') + + +@uses_data_db +def test_deleting_a_resource_drops_its_table(add_resource: Any, tables: Any) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + row = index_resource_data(resource) + + resource.delete() + + assert not tables.exists(row.table_name) + + +@uses_data_db +def test_deleting_a_dataset_drops_every_table_of_its_resources( + add_resource: Any, dataset: Any, tables: Any +) -> None: + index_resource_data(add_resource("first.csv", "districts_v1.csv")) + index_resource_data(add_resource("second.csv", "districts_v2.csv")) + assert len(tables.names()) == 2 + + dataset.delete() + + assert tables.names() == [] + + +@uses_data_db +def test_a_rolled_back_delete_keeps_the_table(add_resource: Any, tables: Any) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + row = index_resource_data(resource) + + try: + with transaction.atomic(): + resource.delete() + raise RuntimeError("the request failed after the delete") + except RuntimeError: + pass + + assert ResourceDataTable.objects.filter(pk=row.pk).exists() + assert tables.exists(row.table_name) + + +@uses_data_db +def test_a_legacy_table_two_rows_share_is_dropped_only_with_the_last_row( + add_resource: Any, tables: Any +) -> None: + first = ResourceDataTable.objects.create( + resource=add_resource("first.csv", "districts_v1.csv"), + table_name=SHARED_PREFIX + "rict-health-indicators-nfhs-4_1a2b3c4d", + ) + second = ResourceDataTable.objects.create( + resource=add_resource("second.csv", "districts_v1.csv"), + table_name=SHARED_PREFIX + "rict-health-indicators-nfhs-5_5e6f7a8b", + ) + create_legacy_table(first.table_name) + assert tables.names() == [SHARED_PREFIX] + + with capture_logs() as logs: + first.delete() + assert tables.names() == [SHARED_PREFIX] + assert any(log["event"] == "data_table_drop_skipped_shared" for log in logs) + + second.delete() + assert tables.names() == [] + + +@uses_data_db +def test_dropping_a_table_that_is_already_gone_does_not_fail_the_delete( + add_resource: Any, tables: Any +) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + ResourceDataTable.objects.create(resource=resource, table_name="rdt_never_built") + + resource.delete() + + assert not ResourceDataTable.objects.filter(resource_id=resource.pk).exists() + + +@uses_data_db +def test_a_drop_blocked_by_a_reader_is_logged_and_the_delete_still_succeeds( + add_resource: Any, tables: Any +) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + row = index_resource_data(resource) + settings = connections["data_db"].settings_dict + reader = psycopg2.connect( + dbname=settings["NAME"], + user=settings["USER"], + password=settings["PASSWORD"], + host=settings["HOST"], + port=settings["PORT"], + ) + try: + with reader.cursor() as cursor: + cursor.execute(f'LOCK TABLE "{row.table_name}" IN ACCESS SHARE MODE') + + with patch("api.utils.data_tables.DROP_LOCK_TIMEOUT", "200ms"), capture_logs() as logs: + resource.delete() + finally: + reader.rollback() + reader.close() + + assert not ResourceDataTable.objects.filter(pk=row.pk).exists() + assert tables.exists(row.table_name) + failures = [log for log in logs if log["event"] == "data_table_drop_failed"] + assert failures and failures[0]["table_name"] == row.table_name + + +def test_the_receiver_schedules_one_guarded_drop_per_deleted_row(add_resource: Any) -> None: + first = ResourceDataTable.objects.create(resource=add_resource("a.csv", "districts_v1.csv")) + second = ResourceDataTable.objects.create(resource=add_resource("b.csv", "districts_v1.csv")) + expected = [(first.table_name, first.pk), (second.table_name, second.pk)] + + with patch("api.signals.data_table_signals.transaction.on_commit") as on_commit: + ResourceDataTable.objects.filter(pk__in=[first.pk, second.pk]).delete() + + assert on_commit.call_count == 2 + scheduled = [] + for call in on_commit.call_args_list: + callback = call.args[0] + assert isinstance(callback, partial) + assert callback.func is drop_data_table_if_unshared + assert call.kwargs == {"using": "default", "robust": True} + scheduled.append(callback.args) + assert sorted(scheduled) == sorted(expected) + + +def test_the_receiver_is_connected_once_the_app_loads() -> None: + # A fresh interpreter: an import elsewhere in this test run can't connect it for us. + check = ( + "import django; django.setup()\n" + "from django.db.models.signals import post_delete\n" + "from api.models import ResourceDataTable\n" + "receivers = post_delete._live_receivers(ResourceDataTable)[0]\n" + "names = {(r.__module__, r.__name__) for r in receivers}\n" + "print(('api.signals.data_table_signals', 'drop_table_after_row_delete') in names)\n" + ) + env = {**os.environ, "DJANGO_SETTINGS_MODULE": "tests.test_settings"} + env.pop("TEST_DATA_DB_URL", None) + result = subprocess.run( + [sys.executable, "-c", check], + cwd=REPO_ROOT, + env=env, + capture_output=True, + text=True, + timeout=120, + ) + assert result.returncode == 0, result.stderr[-2000:] + assert result.stdout.strip().splitlines()[-1] == "True" + assert drop_table_after_row_delete.__module__ == "api.signals.data_table_signals" diff --git a/tests/data_index/test_reader_regression.py b/tests/data_index/test_reader_regression.py new file mode 100644 index 00000000..152a8b4a --- /dev/null +++ b/tests/data_index/test_reader_regression.py @@ -0,0 +1,59 @@ +"""Every reader of data tables still works on the new per-upload tables. + +The readers look the table up from the resource's ResourceDataTable row, so +the new names should be invisible to them. This proves it for each reader, +on a header with a percent sign (common in indicator tables). +""" + +from typing import Any + +from api.models import ResourcePreviewDetails, ResourceSchema +from api.utils.data_indexing import ( + fetch_resource_data, + get_preview_data, + get_resource_columns, + get_row_count, + index_resource_data, + query_resource_data, +) +from tests.data_index.support import uses_data_db + +COLUMNS = ["district", "Literacy rate (%)", "year"] + + +@uses_data_db +def test_every_reader_works_on_a_new_table_with_a_percent_header(add_resource: Any) -> None: + resource = add_resource("literacy.csv", "percent_header.csv") + resource.preview_enabled = True + resource.preview_details = ResourcePreviewDetails.objects.create(is_all_entries=True) + resource.save() + row = index_resource_data(resource) + assert row is not None and row.table_name.startswith("rdt_") + + assert get_row_count(resource) == 3 + + preview = get_preview_data(resource) + assert preview is not None + assert preview.columns == COLUMNS + assert len(preview.rows) == 3 + + assert get_resource_columns(resource) == COLUMNS + + fetched = fetch_resource_data( + resource, + filters={"Literacy rate (%)__gte": 80}, + columns=["district", "Literacy rate (%)"], + order_by=["-Literacy rate (%)"], + ) + assert fetched["rows"] == [["Jorhat", 85.25], ["Kamrup", 81.5]] + assert fetched["total"] == 2 + + chart = query_resource_data( + resource, 'SELECT district, "Literacy rate (%)" FROM {{table}} WHERE year = 2022' + ) + assert chart is not None + assert chart.values.tolist() == [["Jorhat", 85.25]] + + # With no column rows, the column list falls back to the table itself. + ResourceSchema.objects.filter(resource=resource).delete() + assert get_resource_columns(resource) == COLUMNS diff --git a/tests/data_index/test_reindex_switch.py b/tests/data_index/test_reindex_switch.py new file mode 100644 index 00000000..52b9f13d --- /dev/null +++ b/tests/data_index/test_reindex_switch.py @@ -0,0 +1,434 @@ +"""Re-uploads build the new table first and switch over in one commit. + +Root causes this guards against: +- The old table was dropped before the new one existed, so readers saw empty or + half-written data, and a failed re-upload lost the data for good. +- The column-row rebuild swallowed errors per column. Inside the switch's + transaction one failure silently rolled everything back while the upload + still reported success. +- Two overlapping uploads of one resource each indexed their own file, and the + slower (older) one could switch in last, so the download and the served rows + disagreed. +""" + +from datetime import datetime, timezone +from typing import Any, List +from unittest.mock import MagicMock, patch + +import pandas as pd +import pytest +from django.db import connections, transaction +from django.db.utils import ProgrammingError +from rest_framework.test import APIClient +from structlog.testing import capture_logs + +from api.models import Resource, ResourceDataTable, ResourceSchema +from api.utils import data_tables +from api.utils.data_indexing import index_resource_data +from api.utils.data_tables import ( + FileIdentity, + LoadedFile, + has_indexable_file, + load_resource_frame, + rebuild_resource_schema, +) +from tests.data_index.support import fixture_bytes, replace_file, uses_data_db + +# --- Layer 1: the real flow against both databases -------------------------- + + +@uses_data_db +def test_first_upload_creates_a_named_table_with_its_columns( + add_resource: Any, tables: Any +) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + + row = index_resource_data(resource) + + assert row is not None and row.table_name == "rdt_" + row.id.hex + assert ResourceDataTable.objects.get(resource=resource).table_name == row.table_name + assert tables.rows(row.table_name) == [("Jorhat", 30), ("Kamrup", 10), ("Nagaon", 20)] + assert list( + ResourceSchema.objects.filter(resource=resource) + .order_by("id") + .values_list("field_name", "format") + ) == [("district", "STRING"), ("cases", "INTEGER")] + assert str(resource.id) in tables.comment(row.table_name) + assert "cases.csv" in tables.comment(row.table_name) + + +@uses_data_db +def test_reupload_switches_to_a_new_table_and_drops_the_old_one( + add_resource: Any, tables: Any +) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + old = index_resource_data(resource) + ResourceSchema.objects.filter(resource=resource, field_name="cases").update( + description="Confirmed cases" + ) + + replace_file(resource, "districts_v2.csv") + new = index_resource_data(resource) + + assert new is not None and new.table_name != old.table_name + assert ResourceDataTable.objects.get(resource=resource).pk == new.pk + assert len(tables.rows(new.table_name)) == 4 + assert not tables.exists(old.table_name) + assert tables.names() == [new.table_name] + assert ( + ResourceSchema.objects.get(resource=resource, field_name="cases").description + == "Confirmed cases" + ) + + +@uses_data_db +def test_readers_see_the_complete_old_table_until_the_switch_commits( + add_resource: Any, tables: Any +) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + old = index_resource_data(resource) + replace_file(resource, "districts_v2.csv") + seen: dict = {} + real_lock = data_tables.lock_resource + real_drop = data_tables.drop_data_table_if_unshared + + def spy_lock(res: Resource) -> None: + new_tables = [t for t in tables.names() if t != old.table_name] + seen["new_rows_at_switch"] = len(tables.rows(new_tables[0])) + seen["old_exists_at_switch"] = tables.exists(old.table_name) + # The lock only holds inside the switch's transaction (Postgres refuses it outside). + seen["lock_inside_transaction"] = transaction.get_connection().in_atomic_block + real_lock(res) + + def spy_drop(table_name: str, row_id: Any) -> None: + seen["dropped"] = table_name + seen["open_transaction_at_drop"] = transaction.get_connection().in_atomic_block + seen["new_row_saved_at_drop"] = ( + ResourceDataTable.objects.get(resource=resource).table_name != old.table_name + ) + real_drop(table_name, row_id) + + with ( + patch("api.utils.data_indexing.lock_resource", spy_lock), + patch("api.signals.data_table_signals.drop_data_table_if_unshared", spy_drop), + ): + new = index_resource_data(resource) + + assert new is not None + assert seen == { + "new_rows_at_switch": 4, + "old_exists_at_switch": True, + "lock_inside_transaction": True, + "dropped": old.table_name, + "open_transaction_at_drop": False, + "new_row_saved_at_drop": True, + } + + +@uses_data_db +def test_a_failed_build_keeps_the_old_data_and_leaves_no_table( + add_resource: Any, tables: Any +) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + old = index_resource_data(resource) + + # A count past the BIGINT range: the copy into the new table fails midway. + replace_file(resource, "out_of_range.csv") + with capture_logs() as logs: + assert index_resource_data(resource) is None + + assert ResourceDataTable.objects.get(resource=resource).pk == old.pk + assert tables.names() == [old.table_name] + assert len(tables.rows(old.table_name)) == 3 + # The failure is logged by its code, never by the offending cell's value. + failures = [log for log in logs if log["event"] == "data_index_failed"] + assert failures and failures[0]["error_code"] == "22003" + assert failures[0]["where"].startswith("data_tables.py:") + assert "18446744073709551615" not in repr(logs) + + +@uses_data_db +def test_a_header_too_long_for_the_column_list_fails_the_switch_and_keeps_the_old_data( + add_resource: Any, tables: Any +) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + old = index_resource_data(resource) + old_columns = list( + ResourceSchema.objects.filter(resource=resource).values_list("field_name", flat=True) + ) + + replace_file(resource, "long_header.csv") + assert index_resource_data(resource) is None + + assert ResourceDataTable.objects.get(resource=resource).pk == old.pk + assert tables.names() == [old.table_name] + assert ( + list(ResourceSchema.objects.filter(resource=resource).values_list("field_name", flat=True)) + == old_columns + ) + + +@uses_data_db +def test_a_reader_still_holding_the_old_name_gets_does_not_exist_not_new_rows( + add_resource: Any, tables: Any +) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + stale = index_resource_data(resource) + replace_file(resource, "districts_v2.csv") + index_resource_data(resource) + + with pytest.raises(ProgrammingError, match="does not exist"): + with connections["data_db"].cursor() as cursor: + cursor.execute(f'SELECT * FROM "{stale.table_name}"') + + +@uses_data_db +def test_a_slower_build_of_an_older_file_backs_off_when_a_newer_file_was_saved( + add_resource: Any, tables: Any +) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + real_build = data_tables.build_data_table + newer: dict = {} + + def build_then_newer_upload_lands(table_name: str, frame: pd.DataFrame, comment: str) -> None: + real_build(table_name, frame, comment) + if newer: + return # the other request's own build + newer["started"] = True + # Meanwhile another request saves a newer file and finishes indexing it. + other_request = Resource.objects.get(pk=resource.pk) + replace_file(other_request, "districts_v2.csv") + newer["row"] = index_resource_data(other_request) + + with patch("api.utils.data_indexing.build_data_table", build_then_newer_upload_lands): + assert index_resource_data(resource) is None + + current = ResourceDataTable.objects.get(resource=resource) + assert current.pk == newer["row"].pk + assert tables.names() == [current.table_name] + assert len(tables.rows(current.table_name)) == 4 + + +@uses_data_db +def test_indexing_inside_an_outer_transaction_is_refused(add_resource: Any, tables: Any) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + + with capture_logs() as logs, transaction.atomic(): + assert index_resource_data(resource) is None + + assert tables.names() == [] + assert not ResourceDataTable.objects.filter(resource=resource).exists() + assert any(log["event"] == "data_index_refused_inside_transaction" for log in logs) + + +@uses_data_db +def test_reupload_keeps_the_datasets_default_table( + add_resource: Any, owner: Any, dataset: Any +) -> None: + first = add_resource("first.csv", "districts_v1.csv") + second = add_resource("second.csv", "districts_v2.csv") + first_row = index_resource_data(first) + index_resource_data(second) + + replace_file(first, "districts_v2.csv") + replaced = index_resource_data(first) + + assert replaced is not None and replaced.created == first_row.created + assert ResourceDataTable.objects.get(pk=replaced.pk).created == first_row.created + client = APIClient() + client.force_authenticate(owner) + response = client.get(f"/api/datasets/{dataset.id}/data/") + assert response.status_code == 200, response.content + assert response.json()["resource_id"] == str(first.id) + + +# --- Layer 1: helpers on the main database only ------------------------------ + + +def test_a_resource_without_a_supported_file_is_not_indexable(add_resource: Any) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + assert has_indexable_file(resource) + + resource.resourcefiledetails.format = "pdf" + assert not has_indexable_file(resource) + + bare = Resource.objects.create(name="empty", dataset=resource.dataset) + assert not has_indexable_file(bare) + + +def test_load_remembers_which_stored_file_it_read(add_resource: Any) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + details = resource.resourcefiledetails + + loaded = load_resource_frame(resource) + + assert loaded is not None + assert loaded.file == FileIdentity(details.file.name, details.sha256) + assert list(loaded.frame.columns) == ["district", "cases"] + + +def test_load_returns_nothing_for_a_file_with_only_a_header(add_resource: Any) -> None: + resource = add_resource("cases.csv", b"district,cases\n") + assert load_resource_frame(resource) is None + + +def test_load_cleans_quotes_line_breaks_and_duplicate_headers(add_resource: Any) -> None: + # pandas renames plain duplicates itself; these two only collide once the quote goes. + resource = add_resource("cases.csv", b'"dis""trict","a\nb","x""",x\nKamrup,1,2,3\n') + loaded = load_resource_frame(resource) + assert loaded is not None + assert list(loaded.frame.columns) == ["district", "a b", "x", "x_1"] + + +def test_rebuild_columns_lets_a_database_error_escape(add_resource: Any) -> None: + from django.db import DatabaseError + + resource = add_resource("cases.csv", "districts_v1.csv") + frame = pd.DataFrame({"district": ["Kamrup"], "cases": [1]}) + + def failing_create(**kwargs: Any) -> None: + raise DatabaseError("value too long for type character varying(255)") + + with patch.object(ResourceSchema.objects, "create", failing_create): + with pytest.raises(DatabaseError): + rebuild_resource_schema(resource, frame) + + +def test_rebuild_columns_refuses_a_header_longer_than_the_column_list_allows( + add_resource: Any, +) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + frame = pd.DataFrame({"district": ["Kamrup"], "x" * 256: [1]}) + with pytest.raises(ValueError, match="column 2"): + rebuild_resource_schema(resource, frame) + assert not ResourceSchema.objects.filter(resource=resource).exists() + + +# --- Layer 3: the flow's order and clean-up (helpers mocked) ----------------- + +FLOW_HELPERS = [ + "is_inside_transaction", + "has_indexable_file", + "load_resource_frame", + "new_data_table_row", + "describe_data_table", + "build_data_table", + "lock_resource", + "stored_file_identity", + "retire_data_table_row", + "save_data_table_row", + "rebuild_resource_schema", + "data_table_row_exists", + "drop_data_table", +] +FILE = FileIdentity("resources/cases.csv", "abc123") + + +@pytest.fixture +def flow() -> Any: + """Every helper the flow calls, mocked on one parent so call order is recorded.""" + parent = MagicMock() + patchers = [ + patch(f"api.utils.data_indexing.{name}", getattr(parent, name)) for name in FLOW_HELPERS + ] + for p in patchers: + p.start() + parent.is_inside_transaction.return_value = False + parent.has_indexable_file.return_value = True + parent.load_resource_frame.return_value = LoadedFile(pd.DataFrame({"a": [1]}), FILE) + parent.new_data_table_row.return_value = MagicMock(table_name="rdt_new", pk="new-id") + parent.stored_file_identity.return_value = FILE + parent.retire_data_table_row.return_value = datetime(2026, 1, 1, tzinfo=timezone.utc) + parent.data_table_row_exists.return_value = True + yield parent + for p in patchers: + p.stop() + + +def called(flow: Any) -> List[str]: + return [c[0] for c in flow.mock_calls if "." not in c[0]] + + +def test_flow_builds_then_locks_before_retiring_the_old_row(flow: Any) -> None: + row = index_resource_data(MagicMock()) + + assert row is flow.new_data_table_row.return_value + assert called(flow) == [ + "is_inside_transaction", + "has_indexable_file", + "load_resource_frame", + "new_data_table_row", + "describe_data_table", + "build_data_table", + "lock_resource", + "stored_file_identity", + "retire_data_table_row", + "save_data_table_row", + "rebuild_resource_schema", + "data_table_row_exists", + ] + flow.save_data_table_row.assert_called_once_with( + flow.new_data_table_row.return_value, created=flow.retire_data_table_row.return_value + ) + + +def raise_error(*args: Any, **kwargs: Any) -> None: + raise RuntimeError("boom") + + +@pytest.mark.parametrize( + "helper", + ["build_data_table", "lock_resource", "retire_data_table_row", "rebuild_resource_schema"], +) +def test_flow_drops_its_table_when_a_step_fails(flow: Any, helper: str) -> None: + getattr(flow, helper).side_effect = raise_error + + assert index_resource_data(MagicMock()) is None + + flow.drop_data_table.assert_called_once_with("rdt_new") + + +def test_flow_drops_its_table_when_a_newer_file_was_saved(flow: Any) -> None: + flow.stored_file_identity.return_value = FileIdentity("resources/newer.csv", "def456") + + assert index_resource_data(MagicMock()) is None + + flow.retire_data_table_row.assert_not_called() + flow.drop_data_table.assert_called_once_with("rdt_new") + + +def test_flow_drops_its_table_when_the_new_row_is_missing_after_the_commit(flow: Any) -> None: + flow.data_table_row_exists.return_value = False + + assert index_resource_data(MagicMock()) is None + + flow.drop_data_table.assert_called_once_with("rdt_new") + + +def test_flow_drops_its_table_even_when_interrupted(flow: Any) -> None: + flow.build_data_table.side_effect = KeyboardInterrupt + + with pytest.raises(KeyboardInterrupt): + index_resource_data(MagicMock()) + + flow.drop_data_table.assert_called_once_with("rdt_new") + + +def test_flow_keeps_the_table_after_a_confirmed_switch(flow: Any) -> None: + index_resource_data(MagicMock()) + flow.drop_data_table.assert_not_called() + + +@pytest.mark.parametrize("stop_at", ["inside_transaction", "no_file", "nothing_loaded"]) +def test_flow_builds_nothing_when_there_is_nothing_to_index(flow: Any, stop_at: str) -> None: + if stop_at == "inside_transaction": + flow.is_inside_transaction.return_value = True + elif stop_at == "no_file": + flow.has_indexable_file.return_value = False + else: + flow.load_resource_frame.return_value = None + + assert index_resource_data(MagicMock()) is None + + flow.build_data_table.assert_not_called() + flow.drop_data_table.assert_not_called() diff --git a/tests/data_index/test_table_names.py b/tests/data_index/test_table_names.py new file mode 100644 index 00000000..d2c98ceb --- /dev/null +++ b/tests/data_index/test_table_names.py @@ -0,0 +1,93 @@ +"""Every tabular upload gets its own table, named from its own row id. + +Root cause this guards against: table names used to be built from the resource +name and dataset title (up to 110 characters). Postgres keeps only the first 63 +bytes of a name, so two resources with long, similar names shared one table and +a re-upload of one replaced the other's data. +""" + +import json +import uuid +from datetime import datetime, timezone + +from api.models import ResourceDataTable +from api.utils.data_indexing import index_resource_data +from api.utils.data_table_names import data_table_name, physical_table_name +from api.utils.data_tables import build_data_table, describe_data_table +from tests.data_index.support import uses_data_db + +LEGACY_NAME = ( + "district-wise-prevalence-of-anaemia-among-women-aged-15-49-" + "district-health-indicators-assam-nfhs-5_1a2b3c4d" +) + + +def test_a_table_name_is_rdt_and_the_row_id_in_lowercase_hex() -> None: + row_id = uuid.UUID("0123456789ABCDEF0123456789ABCDEF") + assert data_table_name(row_id) == "rdt_0123456789abcdef0123456789abcdef" + assert len(data_table_name(row_id)) == 36 + + +def test_physical_name_is_cut_to_63_bytes_like_postgres() -> None: + assert len(LEGACY_NAME) > 100 + assert physical_table_name(LEGACY_NAME) == LEGACY_NAME[:63] + short = data_table_name(uuid.uuid4()) + assert physical_table_name(short) == short + + +def test_physical_name_never_cuts_a_character_in_half() -> None: + # 62 ASCII bytes then a 2-byte character: Postgres keeps 62 bytes, not 63. + name = "a" * 62 + "é" + "tail" + assert physical_table_name(name) == "a" * 62 + + +def test_a_new_row_is_named_from_its_own_id(add_resource: object) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + row = ResourceDataTable.objects.create(resource=resource) + assert row.table_name == "rdt_" + row.id.hex + + +def test_an_existing_row_keeps_its_stored_name_on_save(add_resource: object) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + row = ResourceDataTable.objects.create(resource=resource, table_name=LEGACY_NAME) + row.save() + row.refresh_from_db() + assert row.table_name == LEGACY_NAME + + +@uses_data_db +def test_two_resources_with_long_similar_names_get_their_own_tables( + add_resource: object, tables: object +) -> None: + prefix = "District wise prevalence of anaemia among women aged 15 to 49 " + first = add_resource(prefix + "(NFHS-4)", "districts_v1.csv") + second = add_resource(prefix + "(NFHS-5)", "districts_v2.csv") + + first_row = index_resource_data(first) + second_row = index_resource_data(second) + + assert first_row is not None and second_row is not None + assert first_row.table_name != second_row.table_name + assert [r[1] for r in tables.rows(first_row.table_name)] == [30, 10, 20] + assert len(tables.rows(second_row.table_name)) == 4 + + +@uses_data_db +def test_the_table_comment_is_stored_as_text_not_run_as_sql( + add_resource: object, tables: object +) -> None: + import pandas as pd + + resource = add_resource("cases'; DROP TABLE x; --\"", "districts_v1.csv") + built_at = datetime(2026, 11, 13, 10, 30, tzinfo=timezone.utc) + comment = describe_data_table(resource, built_at=built_at) + + build_data_table("rdt_comment_check", pd.DataFrame({"a": [1]}), comment) + + stored = json.loads(tables.comment("rdt_comment_check")) + assert stored == { + "resource_id": str(resource.id), + "resource": "cases'; DROP TABLE x; --\"", + "dataset": "District health indicators", + "built_at": "2026-11-13T10:30:00+00:00", + } diff --git a/tests/data_index/test_upload_mutations.py b/tests/data_index/test_upload_mutations.py new file mode 100644 index 00000000..c5c9f313 --- /dev/null +++ b/tests/data_index/test_upload_mutations.py @@ -0,0 +1,128 @@ +"""Upload, replace and delete through the GraphQL API, read through the data API. + +Who may call these mutations is unchanged and covered by +tests/schema/test_resource_permissions.py; these tests follow a dataset owner +through the data-table lifecycle end to end. +""" + +import types +from typing import Any, Dict + +from django.core.files.uploadedfile import SimpleUploadedFile +from django.test import RequestFactory +from rest_framework.test import APIClient + +from api.models import Resource, ResourceDataTable +from api.schema.schema import schema +from tests.data_index.support import fixture_bytes, uses_data_db + +CREATE = """mutation ($ds: UUID!, $file: Upload!) { + createFileResources(fileResourceInput: {dataset: $ds, files: [$file]}) { id } +}""" +REPLACE = """mutation ($id: UUID!, $file: Upload!) { + updateFileResource(fileResourceInput: {id: $id, file: $file}) { + __typename ... on OperationInfo { messages { message } } + } +}""" +RESET = """mutation ($id: UUID!) { + resetFileResourceSchema(resourceId: $id) { + __typename ... on OperationInfo { messages { message } } + } +}""" +DELETE_RESOURCE = "mutation ($id: UUID!) { deleteFileResource(resourceId: $id) }" +DELETE_DATASET = "mutation ($id: UUID!) { deleteDataset(datasetId: $id) }" + + +def run(user: Any, query: str, **variables: Any) -> Dict[str, Any]: + request = RequestFactory().post("/api/graphql") + request.user = user + request.context = {} + ctx = types.SimpleNamespace(request=request, user=user, context={}, response=None) + result = schema.execute_sync(query, variable_values=variables, context_value=ctx) + assert result.errors is None, result.errors + payload = next(iter(result.data.values())) + if isinstance(payload, dict): + assert payload.get("__typename") != "OperationInfo", payload + return result.data + + +def csv_upload(name: str) -> SimpleUploadedFile: + return SimpleUploadedFile("cases.csv", fixture_bytes(name), content_type="text/csv") + + +def data_api(owner: Any, resource_id: Any) -> Any: + client = APIClient() + client.force_authenticate(owner) + return client.get(f"/api/resources/{resource_id}/data/?order_by=district") + + +def upload(owner: Any, dataset: Any, name: str) -> Resource: + data = run(owner, CREATE, ds=str(dataset.id), file=csv_upload(name)) + return Resource.objects.get(id=data["createFileResources"][0]["id"]) + + +@uses_data_db +def test_an_uploaded_csv_is_served_from_its_own_table( + owner: Any, dataset: Any, tables: Any +) -> None: + resource = upload(owner, dataset, "districts_v1.csv") + + table = ResourceDataTable.objects.get(resource=resource).table_name + assert table.startswith("rdt_") and tables.exists(table) + response = data_api(owner, resource.id) + assert response.status_code == 200, response.content + assert response.json()["rows"] == [["Jorhat", 30], ["Kamrup", 10], ["Nagaon", 20]] + + +@uses_data_db +def test_a_replaced_file_is_served_and_its_old_table_is_gone( + owner: Any, dataset: Any, tables: Any +) -> None: + resource = upload(owner, dataset, "districts_v1.csv") + old_table = ResourceDataTable.objects.get(resource=resource).table_name + + run(owner, REPLACE, id=str(resource.id), file=csv_upload("districts_v2.csv")) + + assert not tables.exists(old_table) + assert tables.names() == [ResourceDataTable.objects.get(resource=resource).table_name] + assert data_api(owner, resource.id).json()["total"] == 4 + + +@uses_data_db +def test_resetting_the_columns_switches_to_a_fresh_table( + owner: Any, dataset: Any, tables: Any +) -> None: + resource = upload(owner, dataset, "districts_v1.csv") + old_table = ResourceDataTable.objects.get(resource=resource).table_name + + run(owner, RESET, id=str(resource.id)) + + new_table = ResourceDataTable.objects.get(resource=resource).table_name + assert new_table != old_table + assert tables.names() == [new_table] + assert data_api(owner, resource.id).json()["total"] == 3 + + +@uses_data_db +def test_deleting_a_resource_removes_its_table_and_its_data( + owner: Any, dataset: Any, tables: Any +) -> None: + resource = upload(owner, dataset, "districts_v1.csv") + + run(owner, DELETE_RESOURCE, id=str(resource.id)) + + assert tables.names() == [] + assert data_api(owner, resource.id).status_code == 404 + + +@uses_data_db +def test_deleting_a_dataset_removes_every_table_it_had( + owner: Any, dataset: Any, tables: Any +) -> None: + upload(owner, dataset, "districts_v1.csv") + upload(owner, dataset, "districts_v2.csv") + assert len(tables.names()) == 2 + + run(owner, DELETE_DATASET, id=str(dataset.id)) + + assert tables.names() == [] diff --git a/tests/test_settings.py b/tests/test_settings.py index 05375f6d..2159f0b0 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -33,6 +33,18 @@ } } +# Data tables live in a second Postgres database. Tests that build them +# (marked ``data_db``) run only when TEST_DATA_DB_URL points at a Postgres +# server; pytest-django creates and drops its own database there. The router +# keeps every Django model out of it, as in production. +_TEST_DATA_DB_URL = os.environ.get("TEST_DATA_DB_URL") +if _TEST_DATA_DB_URL: + DATABASES["data_db"] = { + **env.db_url_config(_TEST_DATA_DB_URL), # noqa: F405 — from DataSpace.settings + "TEST": {"NAME": "test_dataspace_data_db"}, + } + DATABASE_ROUTERS = ["tests.data_index.routers.DataDbTestRouter"] + # Disable debug mode DEBUG = True From 33c77550db081edbdb97e99ae54bf54cd314289f Mon Sep 17 00:00:00 2001 From: psaesha Date: Sat, 14 Nov 2026 06:01:41 +0530 Subject: [PATCH 2/7] feat(data-index): add migrate_data_table_names for existing tables Existing tables still carry title-based names, and two resources can share one (Postgres cut both names to the same 63 bytes), so one of them serves the other's data. This one-off command moves them to rdt_ names. Short old names can't have collided and are renamed in place, holding the resource row and waiting at most 2 s for readers. Long ones are rebuilt from each resource's own file through the normal upload flow, oldest first and with no read-time limit; the shared table is dropped once its last row has moved. A long table whose file is missing or can no longer become a table is renamed and marked "unverified" in its comment if no other row uses it, and every later report lists it until the resource is re-uploaded. A rerun after a crash finishes half-done renames. It is a dry run by default. The dry run checks each stored file, so its report shows what --apply will do, which tables are shared and which rows need a person. Tables no row uses are listed with size and comment. They are dropped only with --apply --drop-orphans --from-report , and only if still unused at that moment. The command refuses to run when the data database looks like the main one. Every row's outcome is logged as it happens, and an in-progress report is written before any change. The Layer 5 journey (re-upload while polling, racing re-uploads, delete) is added for on-demand runs. Plan: plans/claude/data-index-table-names.md (Phase 2) --- .../commands/migrate_data_table_names.py | 166 +++++ api/utils/data_indexing.py | 12 +- api/utils/data_table_migration.py | 475 ++++++++++++ api/utils/data_tables.py | 113 ++- .../test_migrate_data_table_names.py | 690 ++++++++++++++++++ .../data_index/reupload-and-delete.py | 149 ++++ 6 files changed, 1582 insertions(+), 23 deletions(-) create mode 100644 api/management/commands/migrate_data_table_names.py create mode 100644 api/utils/data_table_migration.py create mode 100644 tests/data_index/test_migrate_data_table_names.py create mode 100644 tests/journeys/data_index/reupload-and-delete.py diff --git a/api/management/commands/migrate_data_table_names.py b/api/management/commands/migrate_data_table_names.py new file mode 100644 index 00000000..d48e6a0d --- /dev/null +++ b/api/management/commands/migrate_data_table_names.py @@ -0,0 +1,166 @@ +"""Move existing data tables to per-upload names, and list the tables nothing uses. + + python manage.py migrate_data_table_names # dry run: write a report, change nothing + python manage.py migrate_data_table_names --apply # rename, rebuild, fix rows + python manage.py migrate_data_table_names --apply --drop-orphans --from-report + + --resource-id act on one resource only + --report where to write the JSON report (default: a timestamped file here) + +Short old names are renamed in place; long ones (which may hold another +resource's data) are rebuilt from each resource's own file. A long one whose +file is missing or unusable is renamed and marked "unverified" (listed in +every report until the resource is re-uploaded), or left alone if another row +shares it. A dry run checks the files too, so it shows what --apply will do. +Safe to rerun: each run picks up whatever is left ("nothing to do" once only +rows needing a person remain), and doubles as an orphan audit. + +Docs (runbook, failure model): ../../utils/data_indexing_architecture.md +""" + +from typing import Any, Dict, List + +from django.core.management.base import BaseCommand, CommandError, CommandParser +from django.utils import timezone + +from api.utils.data_table_migration import ( + access_models_touched, + build_report, + drop_reviewed_orphans, + find_orphans, + find_unverified, + nothing_left_to_do, + plan_table_migration, + point_row_at_its_table, + read_data_table_rows, + read_reviewed_orphans, + rebuild_decision, + rebuild_from_file, + record_outcome, + reindex_decision, + reindex_missing_table, + rename_legacy_table, + same_file, + summary_lines, + write_report, +) +from api.utils.data_tables import ( + DataDbNotSeparate, + assert_data_db_is_separate, + list_data_db_tables, +) + + +class Command(BaseCommand): + help = "Move data tables to per-upload rdt_ names; list (and drop reviewed) orphans." + + def add_arguments(self, parser: CommandParser) -> None: + mode = parser.add_mutually_exclusive_group() + mode.add_argument("--dry-run", action="store_true", help="Report only (the default).") + mode.add_argument("--apply", action="store_true", help="Rename, rebuild and fix rows.") + parser.add_argument( + "--drop-orphans", + action="store_true", + help="With --apply: drop orphans listed in --from-report that are still orphans.", + ) + parser.add_argument("--from-report", help="A dry-run report someone reviewed.") + parser.add_argument("--resource-id", help="Act on this resource only.") + parser.add_argument("--report", help="Where to write this run's JSON report.") + + def handle(self, *args: Any, **options: Any) -> None: + apply, drop_orphans, from_report = ( + options["apply"], + options["drop_orphans"], + options["from_report"], + ) + mode = "apply" if apply else "dry-run" + now = timezone.now() + report_path = options["report"] or f"data_table_migration_{now:%Y%m%dT%H%M%SZ}.json" + + # Orphans are dropped only by an applying run, and only from a reviewed report. + if drop_orphans and not (apply and from_report): + raise CommandError("--drop-orphans needs --apply and --from-report ") + if from_report and same_file(report_path, from_report): + raise CommandError("--report would overwrite the reviewed --from-report file") + + # Never touch anything if the data database might be the main database. + try: + assert_data_db_is_separate() + except DataDbNotSeparate as error: + raise CommandError(f"Refusing to run: {error}") from error + + # Read the reviewed orphan list now, so a bad file stops the run before any change. + reviewed = read_reviewed_orphans(from_report) if drop_orphans else set() + + # Write an in-progress report first: the path works, and a run that dies leaves a trace. + write_report(report_path, build_report(mode, now, status="in_progress")) + + # Read every row and every data table, and work out what each row needs. + plan = plan_table_migration( + read_data_table_rows(), list_data_db_tables(), now, resource_id=options["resource_id"] + ) + entries: List[Dict[str, Any]] = [ + record_outcome(row, "none", "already_migrated") for row in plan.already_migrated + ] + + if apply: + # Finish renames a crashed run left half done: just point the row at its table. + for row in plan.row_updates: + entries.append(record_outcome(row, "row_update", point_row_at_its_table(row))) + + # Rename short old tables in place; they can't have collided. + for row in plan.renames: + entries.append(record_outcome(row, "rename", rename_legacy_table(row))) + + # Rebuild long old tables from each resource's own file, oldest first. + for row, shared in plan.rebuilds: + outcome = rebuild_from_file(row, shared) + entries.append(record_outcome(row, "rebuild", outcome, shared=shared)) + + # Re-index rows whose table is gone, from their file. + for row in plan.missing_tables: + entries.append(record_outcome(row, "reindex", reindex_missing_table(row))) + else: + # A dry run changes nothing, but checks each stored file so the report shows + # what --apply would really do (rebuild, unverified rename, or leave for a person). + for row in plan.row_updates: + entries.append(record_outcome(row, "row_update", "planned_row_update")) + for row in plan.renames: + entries.append(record_outcome(row, "rename", "planned_rename")) + for row, shared in plan.rebuilds: + decision = rebuild_decision(row, shared) + entries.append(record_outcome(row, "rebuild", f"planned_{decision}", shared=shared)) + for row in plan.missing_tables: + entries.append(record_outcome(row, "reindex", f"planned_{reindex_decision(row)}")) + + # Rebuilt resources get new column rows, which resets access models' column choices. + touched = access_models_touched( + [row.resource_id for row, _ in plan.rebuilds] + + [row.resource_id for row in plan.missing_tables] + ) + + dropped: List[str] = [] + if drop_orphans: + # Drop the reviewed orphans that are still orphans right now. + current = find_orphans(read_data_table_rows(), list_data_db_tables(), timezone.now()) + dropped = drop_reviewed_orphans(reviewed, current) + + # List every table still marked unverified, from this run or any earlier one. + unverified = find_unverified(read_data_table_rows(), list_data_db_tables()) + + # Write the full report and print the summary. + report = build_report( + mode, + now, + entries, + plan.orphans, + dropped, + touched, + unverified, + plan.shared_groups, + ) + write_report(report_path, report) + if nothing_left_to_do(entries): + self.stdout.write("nothing to do") + for line in summary_lines(report, report_path): + self.stdout.write(line) diff --git a/api/utils/data_indexing.py b/api/utils/data_indexing.py index 63080b41..6a5af5aa 100644 --- a/api/utils/data_indexing.py +++ b/api/utils/data_indexing.py @@ -23,6 +23,7 @@ from api.utils.data_tables import ( # noqa: F401 — DATA_DB, INDEXED_FORMATS, get_sql_type re-exported DATA_DB, INDEXED_FORMATS, + LOAD_TIMEOUT_SECONDS, NewerFileSaved, build_data_table, data_table_row_exists, @@ -69,7 +70,9 @@ } -def index_resource_data(resource: Resource) -> Optional[ResourceDataTable]: +def index_resource_data( + resource: Resource, load_timeout: Optional[int] = LOAD_TIMEOUT_SECONDS +) -> Optional[ResourceDataTable]: """Copy a resource's tabular file into a table of its own in the data database. Every upload gets a new table, named from its new ResourceDataTable row. @@ -80,8 +83,9 @@ def index_resource_data(resource: Resource) -> Optional[ResourceDataTable]: data stays as it was and this build's table is dropped. Must be called outside any transaction, so "committed" really means - committed. Returns the new ResourceDataTable, or None when the resource - was not switched over. + committed. ``load_timeout`` limits reading the file (seconds; None for no + limit, as the migration command uses). Returns the new ResourceDataTable, + or None when the resource was not switched over. """ resource_id = str(resource.id) @@ -95,7 +99,7 @@ def index_resource_data(resource: Resource) -> Optional[ResourceDataTable]: return None # Read the file into a frame, remembering exactly which stored file it was. - loaded = load_resource_frame(resource) + loaded = load_resource_frame(resource, timeout=load_timeout) if loaded is None: return None diff --git a/api/utils/data_table_migration.py b/api/utils/data_table_migration.py new file mode 100644 index 00000000..0000e231 --- /dev/null +++ b/api/utils/data_table_migration.py @@ -0,0 +1,475 @@ +"""Helpers for ``migrate_data_table_names``: moving old data tables to per-upload names. + +Old tables were named from the resource name and dataset title. Postgres cut +those names to 63 bytes, so two resources could end up sharing one table. The +command works out what each ResourceDataTable row needs, then: + + - a short old name (63 bytes or less) can't have collided, so its table is + renamed in place to "rdt_"; + - a long old name may already hold another resource's data, so the + resource is rebuilt from its own stored file (through the normal upload + flow, with no limit on reading time); the shared table is dropped by the + delete receiver once its last row has moved. If the file is gone or can't + become a table, a table only this row uses is renamed and marked + "unverified" in its comment (listed on every run until the resource is + re-uploaded); a shared one is left for a manual fix; + - a row whose table was renamed but not yet pointed at (a crashed run) just + gets its row updated; + - a row with no table at all is re-indexed from its file; + - tables no row uses are listed as orphans, and dropped only from a report + someone reviewed. + +A dry run checks each stored file too, so its report shows what --apply will +do. Each step is safe to rerun; every row's outcome is logged as it happens. +Logs carry ids, outcomes and counts only. +Docs: ./data_indexing_architecture.md +""" + +import json +import os +from collections import Counter +from dataclasses import dataclass, field +from datetime import datetime, timedelta +from typing import Any, Dict, Iterable, List, NamedTuple, Optional, Set, Tuple + +import structlog +from django.core.management.base import CommandError +from django.db import connections, transaction +from django.db.utils import OperationalError +from psycopg2 import sql as pg_sql # type: ignore[import-untyped] + +from api.models import AccessModelResource, Resource, ResourceDataTable +from api.utils.data_indexing import index_resource_data +from api.utils.data_table_names import ( + DATA_TABLE_PREFIX, + MAX_IDENTIFIER_BYTES, + data_table_name, + physical_table_name, +) +from api.utils.data_tables import ( + DATA_DB, + DataDbTable, + describe_data_table, + django_table_names, + drop_data_table, + error_fields, + has_indexable_file, + lock_resource, + table_built_at, + table_is_unverified, +) + +logger = structlog.get_logger("dataspace.data_table_migration") + +# How long a rename waits for readers holding the table before it is skipped. +RENAME_LOCK_TIMEOUT = "2s" +# Per-upload tables younger than this may belong to an upload that hasn't switched yet. +YOUNG_TABLE_AGE = timedelta(hours=1) +LOCK_NOT_AVAILABLE = "55P03" +# Outcomes that leave nothing for a rerun to do (missing or unusable files need a person). +NEEDS_MANUAL_FIX = { + "missing_file", + "not_indexable", + "planned_missing_file", + "planned_not_indexable", +} +SETTLED_OUTCOMES = NEEDS_MANUAL_FIX | {"already_migrated"} + + +class DataTableRow(NamedTuple): + """The fields of a ResourceDataTable row the migration looks at.""" + + row_id: Any + resource_id: Any + table_name: str + created: datetime + + +@dataclass +class MigrationPlan: + """What each row needs. Rebuilds carry whether another row shares the table.""" + + already_migrated: List[DataTableRow] = field(default_factory=list) + row_updates: List[DataTableRow] = field(default_factory=list) + renames: List[DataTableRow] = field(default_factory=list) + rebuilds: List[Tuple[DataTableRow, bool]] = field(default_factory=list) + missing_tables: List[DataTableRow] = field(default_factory=list) + orphans: List[DataDbTable] = field(default_factory=list) + shared_groups: int = 0 # old tables two or more rows point at + + +def read_data_table_rows() -> List[DataTableRow]: + """Every ResourceDataTable row, oldest first (one row per tabular resource).""" + rows = ( + ResourceDataTable.objects.order_by("created") + .values_list("id", "resource_id", "table_name", "created") + .iterator() + ) + return [DataTableRow(*row) for row in rows] + + +def plan_table_migration( + rows: List[DataTableRow], + tables: List[DataDbTable], + now: datetime, + resource_id: Optional[str] = None, +) -> MigrationPlan: + """Sort rows by what they need, given the tables that exist now. + + ``resource_id`` limits the actions to one resource; orphans are always + worked out against every row. + """ + existing = {table.name for table in tables} + rows_per_table = Counter(physical_table_name(row.table_name) for row in rows) + plan = MigrationPlan( + orphans=find_orphans(rows, tables, now), + shared_groups=sum(1 for count in rows_per_table.values() if count > 1), + ) + for row in rows: + if resource_id and str(row.resource_id) != str(resource_id): + continue + own_name = data_table_name(row.row_id) + physical = physical_table_name(row.table_name) + if row.table_name == own_name: + target = plan.already_migrated if own_name in existing else plan.missing_tables + target.append(row) + elif physical in existing and _is_long(row.table_name): + plan.rebuilds.append((row, rows_per_table[physical] > 1)) + elif physical in existing: + plan.renames.append(row) + elif own_name in existing: + plan.row_updates.append(row) + else: + plan.missing_tables.append(row) + return plan + + +def _is_long(name: str) -> bool: + return len(name.encode("utf-8")) > MAX_IDENTIFIER_BYTES + + +def find_orphans( + rows: List[DataTableRow], tables: List[DataDbTable], now: datetime +) -> List[DataDbTable]: + """Tables no row uses, in the order given. + + Never a Django model's table, never a row's (cut) table or its renamed + "rdt_" table, and never a per-upload table built less than + YOUNG_TABLE_AGE ago (its upload may still be switching over). + """ + in_use = {physical_table_name(row.table_name) for row in rows} + in_use |= {data_table_name(row.row_id) for row in rows} + protected = in_use | django_table_names() + orphans = [] + for table in tables: + if table.name in protected: + continue + built_at = table_built_at(table.comment) + if ( + table.name.startswith(DATA_TABLE_PREFIX) + and built_at + and now - built_at < YOUNG_TABLE_AGE + ): + continue + orphans.append(table) + return orphans + + +def point_row_at_its_table(row: DataTableRow) -> str: + """Finish a rename a crashed run started: point the row at "rdt_".""" + try: + with transaction.atomic(): + lock_resource(Resource.objects.get(pk=row.resource_id)) + updated = ResourceDataTable.objects.filter( + pk=row.row_id, table_name=row.table_name + ).update(table_name=data_table_name(row.row_id)) + except Exception as error: + logger.error("data_table_row_update_failed", row_id=str(row.row_id), **error_fields(error)) + return "failed" + return "row_updated" if updated else "changed" + + +def rename_legacy_table(row: DataTableRow, unverified: bool = False) -> str: + """Rename the row's old table to "rdt_" and point the row at it. + + ``unverified`` stores the "unverified" mark in the table's comment, for a + long-named table kept without a rebuild (it may hold another resource's data). + + Holds the resource row, so an upload can't switch it meanwhile; a row an + upload already replaced is left alone ("changed"). Waits at most + RENAME_LOCK_TIMEOUT for readers ("skipped_lock"). The data database commits + first; if the row update then fails, a rerun finds the renamed table and + updates the row ("row_updated"). Returns "renamed" on success, "failed" + on any other error (logged). + """ + new_name = data_table_name(row.row_id) + try: + with transaction.atomic(): + resource = Resource.objects.select_related("dataset").get(pk=row.resource_id) + lock_resource(resource) + still_legacy = ResourceDataTable.objects.filter( + pk=row.row_id, table_name=row.table_name + ).exists() + if not still_legacy: + return "changed" + comment = describe_data_table(resource, built_at=row.created, unverified=unverified) + _rename_in_data_db(row.table_name, new_name, comment) + ResourceDataTable.objects.filter(pk=row.row_id).update(table_name=new_name) + except OperationalError as error: + if error_fields(error)["error_code"] == LOCK_NOT_AVAILABLE: + logger.warning("data_table_rename_skipped_lock", row_id=str(row.row_id)) + return "skipped_lock" + logger.error("data_table_rename_failed", row_id=str(row.row_id), **error_fields(error)) + return "failed" + except Exception as error: + logger.error("data_table_rename_failed", row_id=str(row.row_id), **error_fields(error)) + return "failed" + logger.info("data_table_renamed", row_id=str(row.row_id), table_name=new_name) + return "renamed" + + +def _rename_in_data_db(old_name: str, new_name: str, comment: str) -> None: + with transaction.atomic(using=DATA_DB), connections[DATA_DB].cursor() as cursor: + cursor.execute( + pg_sql.SQL("SET LOCAL lock_timeout = {}").format(pg_sql.Literal(RENAME_LOCK_TIMEOUT)) + ) + cursor.execute( + pg_sql.SQL("ALTER TABLE {} RENAME TO {}").format( + pg_sql.Identifier(old_name), pg_sql.Identifier(new_name) + ) + ) + cursor.execute( + pg_sql.SQL("COMMENT ON TABLE {} IS {}").format( + pg_sql.Identifier(new_name), pg_sql.Literal(comment) + ) + ) + + +def file_problem(resource: Resource) -> Optional[str]: + """Why the resource's stored file can't become a table, or None if it can. + + "missing_file" (no file, or gone from storage) or "not_indexable" (a format + that never gets a table, e.g. the file was later replaced by a PDF). + Storage errors are raised. + """ + details = getattr(resource, "resourcefiledetails", None) + if not details or not details.file: + return "missing_file" + if not has_indexable_file(resource): + return "not_indexable" + if not details.file.storage.exists(details.file.name): + return "missing_file" + return None + + +def rebuild_decision(row: DataTableRow, shared: bool) -> str: + """What --apply will do with a long-named old table. + + "rebuild" when the stored file can become a table; otherwise + "unverified_rename" for a table only this row uses, or the file problem + ("missing_file" / "not_indexable") for a shared one, which is left alone. + "check_failed" (logged) when the check itself fails. + """ + try: + problem = file_problem(Resource.objects.get(pk=row.resource_id)) + except Exception as error: + logger.error("data_table_file_check_failed", row_id=str(row.row_id), **error_fields(error)) + return "check_failed" + if problem is None: + return "rebuild" + return problem if shared else "unverified_rename" + + +def reindex_decision(row: DataTableRow) -> str: + """What --apply will do with a row whose table is gone: "reindex" or the file problem.""" + try: + problem = file_problem(Resource.objects.get(pk=row.resource_id)) + except Exception as error: + logger.error("data_table_file_check_failed", row_id=str(row.row_id), **error_fields(error)) + return "check_failed" + return problem or "reindex" + + +def rebuild_from_file(row: DataTableRow, shared: bool) -> str: + """Carry out the rebuild decision for a long-named old table. + + Rebuilds go through the normal upload flow with no limit on reading time, + so the resource gets a fresh per-upload table and the old one is dropped + once no row uses it. Returns "rebuilt", "unverified" (renamed and marked), + the rename's other outcomes, a file problem, "check_failed" or "failed". + """ + decision = rebuild_decision(row, shared) + if decision == "unverified_rename": + outcome = rename_legacy_table(row, unverified=True) + return "unverified" if outcome == "renamed" else outcome + if decision != "rebuild": + return decision + return _index_without_time_limit(row) + + +def reindex_missing_table(row: DataTableRow) -> str: + """Re-index a row whose table is gone: "reindexed", a file problem, or a failure.""" + decision = reindex_decision(row) + if decision != "reindex": + return decision + outcome = _index_without_time_limit(row) + return "reindexed" if outcome == "rebuilt" else outcome + + +def _index_without_time_limit(row: DataTableRow) -> str: + try: + resource = Resource.objects.select_related("dataset").get(pk=row.resource_id) + return "rebuilt" if index_resource_data(resource, load_timeout=None) else "failed" + except Exception as error: + logger.error("data_table_rebuild_failed", row_id=str(row.row_id), **error_fields(error)) + return "failed" + + +def find_unverified(rows: List[DataTableRow], tables: List[DataDbTable]) -> List[DataTableRow]: + """Rows whose table carries the "unverified" mark — kept without a rebuild, needing a check.""" + marked = {table.name for table in tables if table_is_unverified(table.comment)} + return [ + row + for row in rows + if physical_table_name(row.table_name) in marked or data_table_name(row.row_id) in marked + ] + + +def access_models_touched(resource_ids: Iterable[Any]) -> List[str]: + """Access models whose column choices a rebuild resets (columns are recreated).""" + ids = ( + AccessModelResource.objects.filter(resource_id__in=list(resource_ids)) + .values_list("access_model_id", flat=True) + .distinct() + ) + return sorted(str(access_model_id) for access_model_id in ids) + + +def read_reviewed_orphans(path: str) -> Set[str]: + """The orphan table names listed in a report someone reviewed. + + Raises CommandError if the file can't be read or isn't a report, so a bad + path stops the run before it changes anything. + """ + try: + with open(path, encoding="utf-8") as report: + return {orphan["name"] for orphan in json.load(report)["orphans"]} + except (OSError, ValueError, KeyError, TypeError) as error: + raise CommandError(f"Can't read the reviewed report {path}: {error}") from error + + +def same_file(first: str, second: str) -> bool: + """Whether two paths name the same file.""" + return os.path.abspath(first) == os.path.abspath(second) + + +def drop_reviewed_orphans(reviewed: Set[str], current_orphans: List[DataDbTable]) -> List[str]: + """Drop tables that are both in the reviewed report and still orphans now.""" + dropped = [] + for table in current_orphans: + if table.name in reviewed and drop_data_table(table.name): + dropped.append(table.name) + return dropped + + +def write_report(path: str, report: Dict[str, Any]) -> None: + """Write the run's report as JSON (ids, names, outcomes and sizes; no data).""" + with open(path, "w", encoding="utf-8") as out: + json.dump(report, out, indent=2, default=str) + + +def record_outcome( + row: DataTableRow, action: str, outcome: str, shared: Optional[bool] = None +) -> Dict[str, Any]: + """Log one row's outcome as it happens, and return its line for the report.""" + entry: Dict[str, Any] = { + "row_id": str(row.row_id), + "resource_id": str(row.resource_id), + "table_name": row.table_name, + "action": action, + "outcome": outcome, + } + if shared is not None: + entry["shared"] = shared + logger.info( + "data_table_migration_row", + row_id=entry["row_id"], + resource_id=entry["resource_id"], + action=action, + outcome=outcome, + ) + return entry + + +def nothing_left_to_do(entries: List[Dict[str, Any]]) -> bool: + """Whether a rerun would change nothing: every row is migrated or needs a person.""" + return all(entry["outcome"] in SETTLED_OUTCOMES for entry in entries) + + +def build_report( + mode: str, + generated_at: datetime, + entries: Optional[List[Dict[str, Any]]] = None, + orphans: Optional[List[DataDbTable]] = None, + orphans_dropped: Optional[List[str]] = None, + access_models: Optional[List[str]] = None, + unverified: Optional[List[DataTableRow]] = None, + shared_groups: int = 0, + status: str = "complete", +) -> Dict[str, Any]: + """The run's JSON report: counts by outcome, per-row lines, orphans, unverified tables. + + Orphans carry their size and comment (resource, dataset, build time) so a + reviewer can tell what each is before approving its drop. + """ + entries = entries or [] + counts = Counter(entry["outcome"] for entry in entries) + return { + "mode": mode, + "status": status, + "generated_at": generated_at.isoformat(), + "counts": dict(sorted(counts.items())), + "shared_groups": shared_groups, + "rows": entries, + "needs_manual_fix": [e for e in entries if e["outcome"] in NEEDS_MANUAL_FIX], + "unverified_tables": [ + {"row_id": str(r.row_id), "resource_id": str(r.resource_id), "table_name": r.table_name} + for r in unverified or [] + ], + "orphans": [ + {"name": t.name, "size_bytes": t.size_bytes, "comment": t.comment} + for t in orphans or [] + ], + "orphans_dropped": orphans_dropped or [], + "access_models_touched": access_models or [], + } + + +def summary_lines(report: Dict[str, Any], report_path: str) -> List[str]: + """A short plain-text summary of a report, for the terminal and the logs.""" + orphan_bytes = sum(orphan["size_bytes"] for orphan in report["orphans"]) + lines = [f"{report['mode']}: report written to {report_path}"] + lines += [f" {outcome}: {count}" for outcome, count in report["counts"].items()] + lines.append(f" old tables shared by several rows: {report['shared_groups']}") + lines.append( + f" rows needing a manual fix (file missing or unusable): {len(report['needs_manual_fix'])}" + ) + lines.append( + f" unverified tables (may hold another resource's data): {len(report['unverified_tables'])}" + ) + lines.append(f" orphans listed: {len(report['orphans'])} ({orphan_bytes} bytes)") + lines.append(f" orphans dropped: {len(report['orphans_dropped'])}") + lines.append( + f" access models with column choices reset: {len(report['access_models_touched'])}" + ) + logger.info( + "data_table_migration_finished", + mode=report["mode"], + shared_groups=report["shared_groups"], + needs_manual_fix=len(report["needs_manual_fix"]), + unverified_tables=len(report["unverified_tables"]), + orphans_listed=len(report["orphans"]), + orphans_dropped=len(report["orphans_dropped"]), + **report["counts"], + ) + return lines diff --git a/api/utils/data_tables.py b/api/utils/data_tables.py index de4603bc..776967f0 100644 --- a/api/utils/data_tables.py +++ b/api/utils/data_tables.py @@ -5,6 +5,8 @@ ResourceDataTable row over to that table in one main-database commit. Old tables are dropped after the commit by the row's delete receiver (api/signals/data_table_signals.py). Each helper here does one step of that. +The last few (listing the data database's tables, the "is this really a +separate database?" check) serve the ``migrate_data_table_names`` command. Tables live in the separate ``data_db`` database (its statements autocommit); the rows that point at them (ResourceDataTable, ResourceSchema) live in the @@ -23,10 +25,11 @@ from collections import Counter from datetime import datetime from io import StringIO -from typing import Any, Dict, List, NamedTuple, Optional +from typing import Any, Dict, List, NamedTuple, Optional, Set import pandas as pd import structlog +from django.apps import apps from django.db import connections, transaction from psycopg2 import sql as pg_sql # type: ignore[import-untyped] @@ -130,21 +133,23 @@ def has_indexable_file(resource: Resource) -> bool: return True -def load_resource_frame(resource: Resource) -> Optional[LoadedFile]: +def load_resource_frame( + resource: Resource, timeout: Optional[int] = LOAD_TIMEOUT_SECONDS +) -> Optional[LoadedFile]: """Read the resource's stored file into a frame and note which file it was. Column names are cleaned the way tables have always needed them: double quotes removed, line breaks and tabs turned into spaces, and names that then collide get "_1", "_2", ... suffixes. - Returns None when the file can't be read, takes longer than - LOAD_TIMEOUT_SECONDS, or has no rows (each case logged). + Returns None when the file can't be read, takes longer than ``timeout`` + seconds (None: no limit), or has no rows (each case logged). """ file_details = resource.resourcefiledetails identity = FileIdentity(file_details.file.name, file_details.sha256) try: with local_copy(file_details.file) as path: - frame = _load_with_timeout(path, file_details.format.lower()) + frame = _load_with_timeout(path, file_details.format.lower(), timeout) except Exception as error: logger.error("data_index_load_failed", resource_id=str(resource.id), **error_fields(error)) return None @@ -158,17 +163,20 @@ def load_resource_frame(resource: Resource) -> Optional[LoadedFile]: return LoadedFile(frame, identity) -def _load_with_timeout(path: str, file_format: str) -> Optional[pd.DataFrame]: +def _load_with_timeout( + path: str, file_format: str, timeout: Optional[int] +) -> Optional[pd.DataFrame]: # SIGALRM only works in a Unix process's main thread; elsewhere (a server # worker thread) the file loads without a limit, as it always has. - if threading.current_thread() is not threading.main_thread() or not hasattr(signal, "SIGALRM"): + in_main_thread = threading.current_thread() is threading.main_thread() + if timeout is None or not in_main_thread or not hasattr(signal, "SIGALRM"): return load_tabular_data(path, file_format) def on_alarm(signum: int, frame: Any) -> None: - raise TimeoutError(f"loading took longer than {LOAD_TIMEOUT_SECONDS} seconds") + raise TimeoutError(f"loading took longer than {timeout} seconds") previous = signal.signal(signal.SIGALRM, on_alarm) - signal.alarm(LOAD_TIMEOUT_SECONDS) + signal.alarm(timeout) try: return load_tabular_data(path, file_format) finally: @@ -203,21 +211,24 @@ def new_data_table_row(resource: Resource) -> ResourceDataTable: return row -def describe_data_table(resource: Resource, built_at: datetime) -> str: +def describe_data_table(resource: Resource, built_at: datetime, unverified: bool = False) -> str: """The comment stored on a new table, so people browsing the data database know what it holds. JSON with the resource id and name, the dataset title and the build time; the migration command reads ``built_at`` to leave freshly built tables alone. + ``unverified`` marks an old table the migration kept without rebuilding it + (its file was missing), which may hold another resource's data; the mark + stays until the resource's next upload replaces the table. """ - return json.dumps( - { - "resource_id": str(resource.id), - "resource": resource.name, - "dataset": resource.dataset.title, - "built_at": built_at.isoformat(), - }, - ensure_ascii=False, - ) + description: Dict[str, Any] = { + "resource_id": str(resource.id), + "resource": resource.name, + "dataset": resource.dataset.title, + "built_at": built_at.isoformat(), + } + if unverified: + description["unverified"] = True + return json.dumps(description, ensure_ascii=False) def build_data_table(table_name: str, frame: pd.DataFrame, comment: str) -> None: @@ -376,3 +387,67 @@ def _another_row_uses(table_name: str, row_id: Any) -> bool: .values_list("table_name", flat=True) ) return any(physical_table_name(name) == physical for name in candidates) + + +class DataDbTable(NamedTuple): + """A table in the data database's public schema.""" + + name: str + size_bytes: int + comment: Optional[str] + + +class DataDbNotSeparate(Exception): + """The data database looks like (or holds tables of) the main database.""" + + +def list_data_db_tables() -> List[DataDbTable]: + """Every ordinary table in the data database's public schema, with its size and comment.""" + with connections[DATA_DB].cursor() as cursor: + cursor.execute( + "SELECT c.relname, pg_total_relation_size(c.oid), obj_description(c.oid, 'pg_class') " + "FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace " + "WHERE n.nspname = 'public' AND c.relkind = 'r' ORDER BY c.relname" + ) + return [DataDbTable(name, int(size), comment) for name, size, comment in cursor.fetchall()] + + +def django_table_names() -> Set[str]: + """The tables Django's own models use (plus its migration log).""" + models_tables = {m._meta.db_table for m in apps.get_models(include_auto_created=True)} + return models_tables | {"django_migrations"} + + +def assert_data_db_is_separate() -> None: + """Raise DataDbNotSeparate unless the data database is clearly not the main one. + + Refused when both connections name the same host, port and database, or + when the data database holds ``django_migrations`` or any table a Django + model uses — either way, every application table would look like an + unused data table to the orphan sweep. + """ + main = connections["default"].settings_dict + data = connections[DATA_DB].settings_dict + if all( + str(main.get(key) or "") == str(data.get(key) or "") for key in ("HOST", "PORT", "NAME") + ): + raise DataDbNotSeparate("the data database connection points at the main database") + found = sorted(django_table_names() & {table.name for table in list_data_db_tables()}) + if found: + raise DataDbNotSeparate(f"the data database holds Django tables: {', '.join(found[:5])}") + + +def table_built_at(comment: Optional[str]) -> Optional[datetime]: + """When a per-upload table was built, read from its comment (None if unreadable).""" + try: + return datetime.fromisoformat(json.loads(comment or "")["built_at"]) + except (ValueError, KeyError, TypeError): + return None + + +def table_is_unverified(comment: Optional[str]) -> bool: + """Whether a table's comment carries the migration's "unverified" mark.""" + try: + return json.loads(comment or "").get("unverified") is True + except (ValueError, AttributeError): + return False diff --git a/tests/data_index/test_migrate_data_table_names.py b/tests/data_index/test_migrate_data_table_names.py new file mode 100644 index 00000000..03f776f2 --- /dev/null +++ b/tests/data_index/test_migrate_data_table_names.py @@ -0,0 +1,690 @@ +"""The one-off command that moves existing data tables to per-upload names. + +Root cause it cleans up after: tables used to be named from the resource name +and dataset title, cut by Postgres to 63 bytes, so two resources could share +one table (one of them serving the other's data). Short names can't have +collided and are renamed in place; long names may already hold another +resource's data and are rebuilt from each resource's own file. +""" + +import json +from datetime import datetime, timedelta +from datetime import timezone as dt_timezone +from io import StringIO +from pathlib import Path +from typing import Any, Dict, List +from unittest.mock import MagicMock, patch + +import pandas as pd +import psycopg2 +import pytest +from django.core.management import CommandError, call_command +from django.db import connections + +from api.models import AccessModel, AccessModelResource, ResourceDataTable +from api.utils.data_table_migration import ( + DataTableRow, + find_orphans, + rename_legacy_table, +) +from api.utils.data_table_names import data_table_name +from api.utils.data_tables import DataDbTable, build_data_table +from tests.data_index.support import uses_data_db + +SHORT_LEGACY = "casescsv_district-health-indicators_1a2b3c4d" +SHARED_PREFIX = "district-wise-prevalence-of-anaemia-among-women-aged-15-49-dist" # 63 bytes +LONG_FIRST = SHARED_PREFIX + "rict-health-indicators-nfhs-4_1a2b3c4d" +LONG_SECOND = SHARED_PREFIX + "rict-health-indicators-nfhs-5_5e6f7a8b" +WRONG_ROWS = [("Somewhere else", 999)] + + +def legacy_table(name: str, rows: List[tuple]) -> None: + """A table as the old code built it (Postgres cuts the name to 63 bytes).""" + with connections["data_db"].cursor() as cursor: + cursor.execute(f'CREATE TABLE "{name}" (district text, cases bigint)') + for row in rows: + cursor.execute(f'INSERT INTO "{name}" VALUES (%s, %s)', list(row)) + + +def legacy_row(resource: Any, name: str) -> ResourceDataTable: + return ResourceDataTable.objects.create(resource=resource, table_name=name) + + +def run_command(*args: str, report: Path) -> str: + out = StringIO() + call_command("migrate_data_table_names", *args, "--report", str(report), stdout=out) + return out.getvalue() + + +def read_report(path: Path) -> Dict[str, Any]: + return json.loads(path.read_text()) + + +def outcomes(report: Dict[str, Any]) -> Dict[str, str]: + return {entry["table_name"]: entry["outcome"] for entry in report["rows"]} + + +def current_table(resource: Any) -> str: + return ResourceDataTable.objects.get(resource=resource).table_name + + +def remove_stored_file(resource: Any) -> None: + details = resource.resourcefiledetails + details.file.storage.delete(details.file.name) + + +# --- Layer 1: the command against both databases ----------------------------- + + +@uses_data_db +def test_a_short_legacy_table_is_renamed_only_on_apply( + add_resource: Any, tables: Any, tmp_path: Path +) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + row = legacy_row(resource, SHORT_LEGACY) + legacy_table(SHORT_LEGACY, [("Kamrup", 10), ("Nagaon", 20)]) + + run_command(report=tmp_path / "dry.json") + assert current_table(resource) == SHORT_LEGACY + assert tables.names() == [SHORT_LEGACY] + assert outcomes(read_report(tmp_path / "dry.json")) == {SHORT_LEGACY: "planned_rename"} + + run_command("--apply", report=tmp_path / "apply.json") + assert current_table(resource) == data_table_name(row.id) + assert tables.names() == [data_table_name(row.id)] + assert tables.rows(data_table_name(row.id)) == [("Kamrup", 10), ("Nagaon", 20)] + assert json.loads(tables.comment(data_table_name(row.id)))["resource_id"] == str(resource.id) + + +@uses_data_db +def test_a_table_two_rows_shared_is_rebuilt_from_each_resources_own_file( + add_resource: Any, tables: Any, tmp_path: Path +) -> None: + first = add_resource("first.csv", "districts_v1.csv") + second = add_resource("second.csv", "districts_v2.csv") + legacy_row(first, LONG_FIRST) + legacy_row(second, LONG_SECOND) + legacy_table(LONG_SECOND, [("Kamrup", 11)]) # the second upload overwrote the first's data + + run_command("--apply", report=tmp_path / "apply.json") + + assert outcomes(read_report(tmp_path / "apply.json")) == { + LONG_FIRST: "rebuilt", + LONG_SECOND: "rebuilt", + } + assert len(tables.rows(current_table(first))) == 3 + assert len(tables.rows(current_table(second))) == 4 + assert sorted(tables.names()) == sorted([current_table(first), current_table(second)]) + + +@uses_data_db +def test_a_long_legacy_table_with_one_row_left_is_rebuilt_not_renamed( + add_resource: Any, tables: Any, tmp_path: Path +) -> None: + resource = add_resource("first.csv", "districts_v1.csv") + legacy_row(resource, LONG_FIRST) + legacy_table(LONG_FIRST, WRONG_ROWS) # its former sharer, since deleted, overwrote it + + run_command("--apply", report=tmp_path / "apply.json") + + assert tables.rows(current_table(resource)) == [("Jorhat", 30), ("Kamrup", 10), ("Nagaon", 20)] + assert tables.names() == [current_table(resource)] + + +@uses_data_db +def test_a_long_legacy_table_whose_file_is_gone_is_renamed_and_marked_unverified( + add_resource: Any, tables: Any, tmp_path: Path +) -> None: + resource = add_resource("first.csv", "districts_v1.csv") + row = legacy_row(resource, LONG_FIRST) + legacy_table(LONG_FIRST, WRONG_ROWS) + remove_stored_file(resource) + + run_command("--apply", report=tmp_path / "apply.json") + + assert outcomes(read_report(tmp_path / "apply.json")) == {LONG_FIRST: "unverified"} + assert current_table(resource) == data_table_name(row.id) + assert tables.rows(data_table_name(row.id)) == WRONG_ROWS + assert json.loads(tables.comment(data_table_name(row.id)))["unverified"] is True + + # Every later run still lists it, though the row itself now looks migrated. + run_command(report=tmp_path / "later.json") + later = read_report(tmp_path / "later.json") + assert outcomes(later) == {data_table_name(row.id): "already_migrated"} + assert [t["resource_id"] for t in later["unverified_tables"]] == [str(resource.id)] + + +@uses_data_db +def test_a_sharing_row_whose_file_is_gone_stays_put_while_the_other_moves( + add_resource: Any, tables: Any, tmp_path: Path +) -> None: + first = add_resource("first.csv", "districts_v1.csv") + second = add_resource("second.csv", "districts_v2.csv") + legacy_row(first, LONG_FIRST) + legacy_row(second, LONG_SECOND) + legacy_table(LONG_SECOND, [("Kamrup", 11)]) + remove_stored_file(first) + + run_command("--apply", report=tmp_path / "apply.json") + + assert outcomes(read_report(tmp_path / "apply.json")) == { + LONG_FIRST: "missing_file", + LONG_SECOND: "rebuilt", + } + assert current_table(first) == LONG_FIRST + assert SHARED_PREFIX in tables.names() # still used by the first row + assert len(tables.rows(current_table(second))) == 4 + + +@uses_data_db +def test_a_rerun_after_a_crash_between_rename_and_row_update_just_updates_the_row( + add_resource: Any, tables: Any, tmp_path: Path +) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + row = legacy_row(resource, SHORT_LEGACY) + legacy_table(SHORT_LEGACY, [("Kamrup", 10)]) + with connections["data_db"].cursor() as cursor: + cursor.execute(f'ALTER TABLE "{SHORT_LEGACY}" RENAME TO "{data_table_name(row.id)}"') + + run_command("--apply", report=tmp_path / "apply.json") + + assert outcomes(read_report(tmp_path / "apply.json")) == {SHORT_LEGACY: "row_updated"} + assert current_table(resource) == data_table_name(row.id) + assert tables.rows(data_table_name(row.id)) == [("Kamrup", 10)] + + +@uses_data_db +def test_a_rename_blocked_by_a_reader_is_skipped_then_finished_on_a_rerun( + add_resource: Any, tables: Any, tmp_path: Path +) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + row = legacy_row(resource, SHORT_LEGACY) + legacy_table(SHORT_LEGACY, [("Kamrup", 10)]) + settings = connections["data_db"].settings_dict + reader = psycopg2.connect( + dbname=settings["NAME"], + user=settings["USER"], + password=settings["PASSWORD"], + host=settings["HOST"], + port=settings["PORT"], + ) + try: + with reader.cursor() as cursor: + cursor.execute(f'LOCK TABLE "{SHORT_LEGACY}" IN ACCESS SHARE MODE') + with patch("api.utils.data_table_migration.RENAME_LOCK_TIMEOUT", "200ms"): + run_command("--apply", report=tmp_path / "blocked.json") + finally: + reader.rollback() + reader.close() + + assert outcomes(read_report(tmp_path / "blocked.json")) == {SHORT_LEGACY: "skipped_lock"} + assert current_table(resource) == SHORT_LEGACY + + run_command("--apply", report=tmp_path / "rerun.json") + assert current_table(resource) == data_table_name(row.id) + + +@uses_data_db +def test_orphans_are_listed_with_their_size_and_dropped_only_from_a_reviewed_report( + add_resource: Any, tables: Any, tmp_path: Path +) -> None: + owned = add_resource("cases.csv", "districts_v1.csv") + legacy_row(owned, SHORT_LEGACY) + legacy_table(SHORT_LEGACY, [("Kamrup", 10)]) + legacy_table("forgotten-table_1", [("Kamrup", 1)]) + legacy_table("forgotten-table_2", [("Kamrup", 2)]) + old_build = (datetime.now(dt_timezone.utc) - timedelta(hours=2)).isoformat() + young_build = datetime.now(dt_timezone.utc).isoformat() + frame = pd.DataFrame({"a": [1]}) + build_data_table("rdt_old_orphan", frame, json.dumps({"built_at": old_build})) + build_data_table("rdt_in_flight", frame, json.dumps({"built_at": young_build})) + + run_command(report=tmp_path / "dry.json") + orphans = read_report(tmp_path / "dry.json")["orphans"] + assert sorted(o["name"] for o in orphans) == [ + "forgotten-table_1", + "forgotten-table_2", + "rdt_old_orphan", + ] + assert all(o["size_bytes"] > 0 for o in orphans) + + # Without a reviewed report nothing is dropped. + with pytest.raises(CommandError, match="--from-report"): + run_command("--apply", "--drop-orphans", report=tmp_path / "x.json") + assert "forgotten-table_1" in tables.names() + + # Since the review, a row started using forgotten-table_2, and a new orphan appeared. + late = add_resource("late.csv", "districts_v1.csv") + legacy_row(late, "forgotten-table_2") + legacy_table("not-reviewed", [("Kamrup", 3)]) + + run_command( + "--apply", + "--drop-orphans", + "--from-report", + str(tmp_path / "dry.json"), + report=tmp_path / "drop.json", + ) + + assert sorted(read_report(tmp_path / "drop.json")["orphans_dropped"]) == [ + "forgotten-table_1", + "rdt_old_orphan", + ] + remaining = tables.names() + assert "forgotten-table_1" not in remaining and "rdt_old_orphan" not in remaining + # The late row's table was renamed by the same run, not dropped. + assert current_table(late).startswith("rdt_") + assert {current_table(late), "not-reviewed", "rdt_in_flight"} <= set(remaining) + + +@uses_data_db +def test_a_second_apply_after_completion_has_nothing_to_do( + add_resource: Any, tables: Any, tmp_path: Path +) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + legacy_row(resource, SHORT_LEGACY) + legacy_table(SHORT_LEGACY, [("Kamrup", 10)]) + run_command("--apply", report=tmp_path / "first.json") + names_after_first = tables.names() + + output = run_command("--apply", report=tmp_path / "second.json") + + assert "nothing to do" in output + assert tables.names() == names_after_first + assert outcomes(read_report(tmp_path / "second.json")) == { + data_table_name(ResourceDataTable.objects.get(resource=resource).id): "already_migrated" + } + + +@uses_data_db +def test_rebuilds_list_the_access_models_whose_column_choices_reset( + add_resource: Any, dataset: Any, tmp_path: Path +) -> None: + resource = add_resource("first.csv", "districts_v1.csv") + legacy_row(resource, LONG_FIRST) + legacy_table(LONG_FIRST, WRONG_ROWS) + access_model = AccessModel.objects.create(name="Partners", dataset=dataset) + AccessModelResource.objects.create(access_model=access_model, resource=resource) + + run_command(report=tmp_path / "dry.json") + + assert read_report(tmp_path / "dry.json")["access_models_touched"] == [str(access_model.id)] + + +@uses_data_db +def test_the_command_refuses_when_the_data_database_is_the_main_database( + tmp_path: Path, +) -> None: + data = connections["data_db"].settings_dict + same = {"HOST": data["HOST"], "PORT": data["PORT"], "NAME": data["NAME"]} + with patch.dict(connections["default"].settings_dict, same): + with pytest.raises(CommandError, match="main database"): + run_command(report=tmp_path / "x.json") + assert not (tmp_path / "x.json").exists() + + +@uses_data_db +def test_the_command_refuses_when_the_data_database_holds_django_tables( + tables: Any, tmp_path: Path +) -> None: + with connections["data_db"].cursor() as cursor: + cursor.execute("CREATE TABLE django_migrations (id int)") + legacy_table("forgotten-table", [("Kamrup", 1)]) + + with pytest.raises(CommandError, match="django_migrations"): + run_command("--apply", report=tmp_path / "x.json") + assert "forgotten-table" in tables.names() + + +@uses_data_db +def test_a_dry_run_shows_what_apply_will_do_with_each_long_table( + add_resource: Any, tables: Any, tmp_path: Path +) -> None: + alone = add_resource("alone.csv", "districts_v1.csv") + first = add_resource("first.csv", "districts_v1.csv") + second = add_resource("second.csv", "districts_v2.csv") + lonely_name = "x" * 70 + legacy_row(alone, lonely_name) + legacy_table(lonely_name, WRONG_ROWS) + legacy_row(first, LONG_FIRST) + legacy_row(second, LONG_SECOND) + legacy_table(LONG_SECOND, [("Kamrup", 11)]) + remove_stored_file(alone) + remove_stored_file(first) + + run_command(report=tmp_path / "dry.json") + + report = read_report(tmp_path / "dry.json") + assert outcomes(report) == { + lonely_name: "planned_unverified_rename", + LONG_FIRST: "planned_missing_file", + LONG_SECOND: "planned_rebuild", + } + assert {e["table_name"]: e["shared"] for e in report["rows"]} == { + lonely_name: False, + LONG_FIRST: True, + LONG_SECOND: True, + } + assert report["shared_groups"] == 1 + assert [e["table_name"] for e in report["needs_manual_fix"]] == [LONG_FIRST] + assert current_table(alone) == lonely_name # a dry run changes nothing + + +@uses_data_db +def test_a_long_table_whose_file_can_no_longer_become_a_table_is_kept_and_marked( + add_resource: Any, tables: Any, tmp_path: Path +) -> None: + alone = add_resource("alone.csv", "districts_v1.csv") + first = add_resource("first.csv", "districts_v1.csv") + second = add_resource("second.csv", "districts_v2.csv") + lonely_name = "x" * 70 + legacy_row(alone, lonely_name) + legacy_table(lonely_name, WRONG_ROWS) + legacy_row(first, LONG_FIRST) + legacy_row(second, LONG_SECOND) + legacy_table(LONG_SECOND, [("Kamrup", 11)]) + for resource in (alone, first): # later replaced by a PDF, which gets no table + resource.resourcefiledetails.format = "pdf" + resource.resourcefiledetails.save() + + run_command("--apply", report=tmp_path / "apply.json") + assert outcomes(read_report(tmp_path / "apply.json")) == { + lonely_name: "unverified", + LONG_FIRST: "not_indexable", # still shared at planning time: left alone + LONG_SECOND: "rebuilt", + } + + # With its sharer gone, the left-behind row is now alone, so a rerun marks it too. + run_command("--apply", report=tmp_path / "rerun.json") + assert outcomes(read_report(tmp_path / "rerun.json"))[LONG_FIRST] == "unverified" + + output = run_command("--apply", report=tmp_path / "last.json") + assert "nothing to do" in output + assert len(read_report(tmp_path / "last.json")["unverified_tables"]) == 2 + + +@uses_data_db +def test_rebuilds_read_files_with_no_time_limit(add_resource: Any, tmp_path: Path) -> None: + resource = add_resource("first.csv", "districts_v1.csv") + legacy_row(resource, LONG_FIRST) + legacy_table(LONG_FIRST, WRONG_ROWS) + + with patch( + "api.utils.data_table_migration.index_resource_data", return_value=MagicMock() + ) as index: + run_command("--apply", report=tmp_path / "apply.json") + + index.assert_called_once() + assert index.call_args.kwargs == {"load_timeout": None} + + +@uses_data_db +def test_a_row_whose_table_is_gone_is_reindexed_or_left_for_a_person( + add_resource: Any, tables: Any, tmp_path: Path +) -> None: + has_file = add_resource("has-file.csv", "districts_v1.csv") + no_file = add_resource("no-file.csv", "districts_v1.csv") + gone = ResourceDataTable.objects.create(resource=has_file) # rdt_ name, table never built + ResourceDataTable.objects.create(resource=no_file) + remove_stored_file(no_file) + + output = run_command("--apply", report=tmp_path / "apply.json") + + report = read_report(tmp_path / "apply.json") + assert sorted(e["outcome"] for e in report["rows"]) == ["missing_file", "reindexed"] + assert current_table(has_file) != gone.table_name + assert len(tables.rows(current_table(has_file))) == 3 + assert "nothing to do" not in output + assert "nothing to do" in run_command("--apply", report=tmp_path / "rerun.json") + + +@uses_data_db +def test_resource_id_limits_the_run_to_that_resource( + add_resource: Any, tables: Any, tmp_path: Path +) -> None: + target = add_resource("target.csv", "districts_v1.csv") + other = add_resource("other.csv", "districts_v1.csv") + legacy_row(target, SHORT_LEGACY) + legacy_table(SHORT_LEGACY, [("Kamrup", 10)]) + legacy_row(other, "other-legacy-table") + legacy_table("other-legacy-table", [("Kamrup", 1)]) + + run_command("--apply", "--resource-id", str(target.id), report=tmp_path / "one.json") + + assert current_table(target).startswith("rdt_") + assert current_table(other) == "other-legacy-table" + + +@uses_data_db +def test_a_rename_backs_off_when_an_upload_replaced_the_row_after_planning( + add_resource: Any, tables: Any +) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + row = legacy_row(resource, SHORT_LEGACY) + legacy_table(SHORT_LEGACY, [("Kamrup", 10)]) + planned = DataTableRow(row.id, resource.id, SHORT_LEGACY, row.created) + ResourceDataTable.objects.filter(pk=row.pk).update(table_name="rdt_" + "f" * 32) + + assert rename_legacy_table(planned) == "changed" + assert SHORT_LEGACY in tables.names() + + +@uses_data_db +def test_a_bad_reviewed_report_stops_the_run_before_any_change( + add_resource: Any, tables: Any, tmp_path: Path +) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + legacy_row(resource, SHORT_LEGACY) + legacy_table(SHORT_LEGACY, [("Kamrup", 10)]) + (tmp_path / "edited.json").write_text('{"orphans": [{"name": "a"},]}') # trailing comma + + for reviewed in ("missing.json", "edited.json"): + with pytest.raises(CommandError, match="reviewed report"): + run_command( + "--apply", + "--drop-orphans", + "--from-report", + str(tmp_path / reviewed), + report=tmp_path / "out.json", + ) + + assert current_table(resource) == SHORT_LEGACY + + +@uses_data_db +def test_the_report_may_not_overwrite_the_reviewed_report(tmp_path: Path) -> None: + run_command(report=tmp_path / "dry.json") + reviewed = (tmp_path / "dry.json").read_text() + + with pytest.raises(CommandError, match="overwrite"): + run_command( + "--apply", + "--drop-orphans", + "--from-report", + str(tmp_path / "dry.json"), + report=tmp_path / "dry.json", + ) + + assert (tmp_path / "dry.json").read_text() == reviewed + + +# --- Layer 2: orphan rules --------------------------------------------------- + +NOW = datetime(2026, 11, 13, 12, 0, tzinfo=dt_timezone.utc) + + +def table(name: str, built_minutes_ago: Any = None) -> DataDbTable: + comment = None + if built_minutes_ago is not None: + built_at = (NOW - timedelta(minutes=built_minutes_ago)).isoformat() + comment = json.dumps({"built_at": built_at}) + return DataDbTable(name=name, size_bytes=8192, comment=comment) + + +def test_orphans_never_include_django_tables_rows_tables_or_fresh_builds() -> None: + row_id = __import__("uuid").uuid4() + rows = [ + DataTableRow(row_id=row_id, resource_id=row_id, table_name=LONG_FIRST, created=NOW), + ] + candidates = [ + table("resource_data_table"), # a Django model's table + table("django_migrations"), + table(SHARED_PREFIX), # the row's (cut) table + table(data_table_name(row_id)), # the row's renamed table, row not yet updated + table("rdt_" + "a" * 32, built_minutes_ago=5), # an upload still switching + table("rdt_" + "b" * 32, built_minutes_ago=90), + table("rdt_" + "c" * 32), # no readable build time + table("forgotten-table"), + ] + + names = [t.name for t in find_orphans(rows, candidates, now=NOW)] + + assert names == ["rdt_" + "b" * 32, "rdt_" + "c" * 32, "forgotten-table"] + + +# --- Layer 3: the command's flow (helpers mocked) ---------------------------- + +COMMAND = "api.management.commands.migrate_data_table_names" + + +@pytest.fixture +def command_helpers() -> Any: + parent = MagicMock(unsafe=True) # one helper's name starts with "assert" + names = [ + "assert_data_db_is_separate", + "read_data_table_rows", + "list_data_db_tables", + "plan_table_migration", + "point_row_at_its_table", + "rename_legacy_table", + "rebuild_from_file", + "rebuild_decision", + "reindex_decision", + "reindex_missing_table", + "find_unverified", + "access_models_touched", + "read_reviewed_orphans", + "find_orphans", + "drop_reviewed_orphans", + "write_report", + ] + patchers = [patch(f"{COMMAND}.{name}", getattr(parent, name)) for name in names] + for p in patchers: + p.start() + row = lambda name: DataTableRow( # noqa: E731 + row_id=name, resource_id=name, table_name=name, created=NOW + ) + plan = MagicMock() + plan.already_migrated = [] + plan.row_updates = [row("crashed")] + plan.renames = [row("short")] + plan.rebuilds = [(row("long_old"), False), (row("long_new"), True)] + plan.missing_tables = [row("missing")] + plan.orphans = [] + plan.shared_groups = 1 + parent.plan_table_migration.return_value = plan + parent.point_row_at_its_table.return_value = "row_updated" + parent.rename_legacy_table.return_value = "renamed" + parent.rebuild_from_file.return_value = "rebuilt" + parent.reindex_missing_table.return_value = "reindexed" + parent.rebuild_decision.return_value = "rebuild" + parent.reindex_decision.return_value = "reindex" + parent.find_unverified.return_value = [] + parent.access_models_touched.return_value = [] + parent.drop_reviewed_orphans.return_value = [] + yield parent + for p in patchers: + p.stop() + + +def top_level_calls(parent: MagicMock) -> List[str]: + return [c[0] for c in parent.mock_calls if c[0] and "." not in c[0] and "(" not in c[0]] + + +def test_a_dry_run_checks_safety_first_and_changes_nothing( + command_helpers: MagicMock, tmp_path: Path +) -> None: + call_command( + "migrate_data_table_names", "--report", str(tmp_path / "r.json"), stdout=StringIO() + ) + + calls = top_level_calls(command_helpers) + assert calls[0] == "assert_data_db_is_separate" + assert "write_report" in calls + for action in [ + "point_row_at_its_table", + "rename_legacy_table", + "rebuild_from_file", + "reindex_missing_table", + "drop_reviewed_orphans", + ]: + assert action not in calls + + +def test_apply_fixes_crashed_rows_then_renames_then_rebuilds_in_order( + command_helpers: MagicMock, tmp_path: Path +) -> None: + call_command( + "migrate_data_table_names", + "--apply", + "--report", + str(tmp_path / "r.json"), + stdout=StringIO(), + ) + + actions = [ + (c[0], c.args[0].table_name) + for c in command_helpers.mock_calls + if c[0] + in { + "point_row_at_its_table", + "rename_legacy_table", + "rebuild_from_file", + "reindex_missing_table", + } + ] + assert actions == [ + ("point_row_at_its_table", "crashed"), + ("rename_legacy_table", "short"), + ("rebuild_from_file", "long_old"), + ("rebuild_from_file", "long_new"), + ("reindex_missing_table", "missing"), + ] + command_helpers.drop_reviewed_orphans.assert_not_called() + + +def test_the_command_stops_before_reading_anything_when_safety_fails( + command_helpers: MagicMock, tmp_path: Path +) -> None: + from api.utils.data_tables import DataDbNotSeparate + + command_helpers.assert_data_db_is_separate.side_effect = DataDbNotSeparate("same database") + + with pytest.raises(CommandError): + call_command("migrate_data_table_names", "--apply", "--report", str(tmp_path / "r.json")) + + assert top_level_calls(command_helpers) == ["assert_data_db_is_separate"] + + +def test_orphans_are_dropped_only_with_apply_and_a_reviewed_report( + command_helpers: MagicMock, tmp_path: Path +) -> None: + with pytest.raises(CommandError): + call_command("migrate_data_table_names", "--drop-orphans", "--from-report", "r.json") + command_helpers.drop_reviewed_orphans.assert_not_called() + + command_helpers.read_reviewed_orphans.return_value = {"forgotten-table"} + call_command( + "migrate_data_table_names", + "--apply", + "--drop-orphans", + "--from-report", + str(tmp_path / "reviewed.json"), + "--report", + str(tmp_path / "r.json"), + stdout=StringIO(), + ) + command_helpers.read_reviewed_orphans.assert_called_once_with(str(tmp_path / "reviewed.json")) + command_helpers.drop_reviewed_orphans.assert_called_once_with( + {"forgotten-table"}, command_helpers.find_orphans.return_value + ) diff --git a/tests/journeys/data_index/reupload-and-delete.py b/tests/journeys/data_index/reupload-and-delete.py new file mode 100644 index 00000000..2055b883 --- /dev/null +++ b/tests/journeys/data_index/reupload-and-delete.py @@ -0,0 +1,149 @@ +""" +Journey: upload a CSV, read its rows, replace the file while polling the data +API (it must never come back empty or half-replaced), fire two replacements at +once (the rows served must match the file the download serves), then delete +the resource and get a 404 from the data API. + +On-demand (Layer 5) — runs against a live backend. Reads from env: + KEYCLOAK_TEST_TOKEN a token for a user who can edit TEST_DATASET_ID + TEST_BASE_URL default http://localhost:8000 + TEST_DATASET_ID a draft dataset to add the resource to +Fails loudly on the first assertion that fails. + +Usage: + KEYCLOAK_TEST_TOKEN=... TEST_DATASET_ID=... TEST_BASE_URL=http://localhost:8000 \ + python tests/journeys/data_index/reupload-and-delete.py + +Afterwards, at least an hour later (the orphan list skips per-upload tables +built within the last hour, as their upload may still be switching), the +operator runs `python manage.py migrate_data_table_names` (a dry run) on the +server and checks the report lists no orphan from the racing uploads. +Docs: api/utils/data_indexing_architecture.md +""" + +import csv +import io +import json +import os +import threading + +import requests + +base = os.environ.get("TEST_BASE_URL", "http://localhost:8000") +token = os.environ["KEYCLOAK_TEST_TOKEN"] +dataset_id = os.environ["TEST_DATASET_ID"] +headers = {"Authorization": f"Bearer {token}"} +graphql = f"{base}/api/graphql" + +CREATE = """mutation ($ds: UUID!, $file: Upload!) { + createFileResources(fileResourceInput: {dataset: $ds, files: [$file]}) { id } +}""" +REPLACE = """mutation ($id: UUID!, $file: Upload!) { + updateFileResource(fileResourceInput: {id: $id, file: $file}) { __typename } +}""" +DELETE = "mutation ($id: UUID!) { deleteFileResource(resourceId: $id) }" + + +def make_csv(label, rows): + """A CSV whose every row carries the label, so a mix of two files is visible.""" + lines = ["district,label,value"] + [f"District {i},{label},{i}" for i in range(rows)] + return ("\n".join(lines) + "\n").encode() + + +def upload(query, variables, file_bytes): + """Post a GraphQL mutation with one file, using the GraphQL multipart spec.""" + operations = {"query": query, "variables": {**variables, "file": None}} + res = requests.post( + graphql, + data={"operations": json.dumps(operations), "map": json.dumps({"0": ["variables.file"]})}, + files={"0": ("journey.csv", file_bytes, "text/csv")}, + headers=headers, + timeout=300, + ) + assert res.status_code == 200, res.text + body = res.json() + assert not body.get("errors"), body["errors"] + return body["data"] + + +def served_labels(resource_id): + """The labels the data API serves for the resource (all rows), or None on an error.""" + res = requests.get( + f"{base}/api/resources/{resource_id}/data/", + params={"limit": 10000, "columns": "label"}, + headers=headers, + timeout=60, + ) + if res.status_code != 200: + return None + return {row[0] for row in res.json()["rows"]}, res.json()["total"] + + +def downloaded_label(resource_id): + """The label in the file the download endpoint serves.""" + res = requests.get(f"{base}/api/download/resource/{resource_id}", headers=headers, timeout=60) + assert res.status_code == 200, res.text + rows = list(csv.DictReader(io.StringIO(res.content.decode()))) + labels = {row["label"] for row in rows} + assert len(labels) == 1, labels + return labels.pop() + + +# 1. Upload a CSV and read its rows back. +created = upload(CREATE, {"ds": dataset_id}, make_csv("first", 500)) +resource_id = created["createFileResources"][0]["id"] +assert served_labels(resource_id) == ({"first"}, 500) + +# 2. Replace it while polling: every read is the complete old or the complete new file. +seen = [] +done = threading.Event() + + +def poll(): + while not done.is_set(): + seen.append(served_labels(resource_id)) + + +poller = threading.Thread(target=poll) +poller.start() +upload(REPLACE, {"id": resource_id}, make_csv("second", 800)) +done.set() +poller.join() +assert seen, "the poller never read anything" +for reading in seen: + assert reading in (({"first"}, 500), ({"second"}, 800)), reading +assert served_labels(resource_id) == ({"second"}, 800) + +# 3. Two replacements at once: whichever file the download serves, the data API serves too. +race_errors = [] + + +def race(label): + try: + upload(REPLACE, {"id": resource_id}, make_csv(label, 300)) + except Exception as error: # surfaced below: a thread's exception is otherwise lost + race_errors.append(error) + + +racers = [threading.Thread(target=race, args=(label,)) for label in ("third", "fourth")] +for racer in racers: + racer.start() +for racer in racers: + racer.join() +assert not race_errors, race_errors +winner = downloaded_label(resource_id) +assert served_labels(resource_id) == ({winner}, 300), (winner, served_labels(resource_id)) + +# 4. Delete the resource: its data is gone. +res = requests.post( + graphql, json={"query": DELETE, "variables": {"id": resource_id}}, headers=headers, timeout=60 +) +assert res.status_code == 200 and not res.json().get("errors"), res.text +gone = requests.get(f"{base}/api/resources/{resource_id}/data/", headers=headers, timeout=60) +assert gone.status_code == 404, gone.text + +print("PASS: reupload-and-delete") +print( + "In an hour, run `python manage.py migrate_data_table_names` and check the report " + "lists no orphans (younger per-upload tables are skipped by design)." +) From 599b8482a9dd52c588c1726eaf03b3a47cb334fe Mon Sep 17 00:00:00 2001 From: psaesha Date: Sat, 14 Nov 2026 06:04:41 +0530 Subject: [PATCH 3/7] docs(data-index): add the data index architecture doc and runbook Covers per-upload table naming, the build -> switch -> drop-after-commit flow, the delete receiver and its shared-table guard, the readers, the migrate_data_table_names command with its runbook and old naming schemes, the Postgres test setup, the failure model, and known limitations. SDK: no. The data API doc links to it. Plan: plans/claude/data-index-table-names.md (Phase 4) --- api/utils/data_indexing_architecture.md | 339 ++++++++++++++++++++++++ docs/dataset_data_api.md | 3 +- 2 files changed, 341 insertions(+), 1 deletion(-) create mode 100644 api/utils/data_indexing_architecture.md diff --git a/api/utils/data_indexing_architecture.md b/api/utils/data_indexing_architecture.md new file mode 100644 index 00000000..0ca820a3 --- /dev/null +++ b/api/utils/data_indexing_architecture.md @@ -0,0 +1,339 @@ +# Data index — per-upload data tables + +> Backend only. Related: [`docs/dataset_data_api.md`](../../docs/dataset_data_api.md) (the public data API that reads these tables). Plan: `plans/claude/data-index-table-names.md`. + +## Overview + +Every tabular resource file (csv, xls, xlsx, ods, parquet, feather, json, tsv) is copied into a table of its own in the separate `data_db` Postgres database. Previews, row counts, the column list, the data API (`/api/resources//data/`, `/api/datasets//data/`) and chart queries read those tables. A resource's `ResourceDataTable` row (main database) records which table is its own; every reader looks the table up from that row, never from a name rule. + +This feature makes each upload's table **unique and safely replaced**: + +- **Names come from the upload's own row id:** `rdt_<32 hex of the ResourceDataTable id>`, 36 characters. They never collide and never change when a resource or dataset is renamed. +- **Re-uploads build first, then switch, then drop.** The new table is built completely, one main-database commit points the resource at it, and the old table is dropped only after that commit. Readers see the complete old data, then the complete new data. A failed re-upload leaves the old data exactly as it was. +- **Deletes free storage.** Deleting a resource, a dataset (by cascade), or a row in the admin drops the table once the delete commits. A rolled-back delete keeps it. +- **Existing tables are migrated by a one-off command** (`migrate_data_table_names`). Its runbook is below. + +Why it was needed: names used to be `slug(resource name)[:50]_slug(dataset title)[:50]_<8 hex>`, up to 110 characters. Postgres keeps only the first 63 bytes of any name, so the id suffix was usually cut off and two resources with long, similar names shared one physical table. A re-upload of one replaced the other's data, and the other's preview, charts and data API served the wrong numbers. Re-uploads also dropped the old table before building the new one: readers saw empty or half-written data for a few seconds, a failed re-upload lost the data for good, and deletes never dropped anything. + +## Submodule map + +| Submodule | Trigger | Code | +|---|---|---| +| Upload / re-index flow | GraphQL `createFileResources`, `updateFileResource` (with a file), `resetFileResourceSchema`, `updateFileResourceSchema` (when a column format changes); `python manage.py update_dataindex` | `index_resource_data` in `api/utils/data_indexing.py`; helpers in `api/utils/data_tables.py` | +| Table names | used by the model and the flow | `api/utils/data_table_names.py` | +| Delete clean-up | `post_delete` of any `ResourceDataTable` row (re-upload, resource/dataset delete, admin delete) | `api/signals/data_table_signals.py` → `drop_data_table_if_unshared` | +| Readers | GraphQL resource fields (preview, row count), data API views, chart generator | `get_row_count`, `get_preview_data`, `get_resource_columns`, `fetch_resource_data`, `query_resource_data` in `api/utils/data_indexing.py` (unchanged) | +| Migration of existing tables | `python manage.py migrate_data_table_names` (one-off, operator-run) | `api/management/commands/migrate_data_table_names.py`; helpers in `api/utils/data_table_migration.py` | + +--- + +## Submodule: Upload / re-index flow + +### Trigger +`index_resource_data(resource, load_timeout=60)` is called synchronously by the resource file mutations in `api/schema/resource_schema.py` (none of them runs inside a transaction), by `update_dataindex` per resource, and by the migration command for rebuilds (with `load_timeout=None`). + +### Business use case +A publisher uploads or replaces a tabular file. Visitors then preview it, chart it and query it through the data API. They must never see another resource's data, an empty table mid-replace, or lose the old data because a new file was bad. + +### Flow (English) +1. **Refuse inside an outer transaction.** If the main database is already in an atomic block, log `data_index_refused_inside_transaction` and return None. Inside someone else's transaction, "committed" would only mean a savepoint was released, and a later rollback would leave a switched table pointing nowhere. +2. **Check the file.** The resource must have a stored file in an indexed format; otherwise return None. Nothing is built. +3. **Load the file** into a frame, and note which stored file it was (storage name + `sha256`). Loading is limited to 60 s where the platform allows it: SIGALRM works only in a process's main thread, so management commands get the limit and web worker threads never have. Column names are cleaned: double quotes removed, line breaks and tabs become spaces, and names that then collide get `_1`, `_2`… An unreadable file, a timeout or an empty frame returns None. +4. **Make this upload's row in memory.** A new `ResourceDataTable` with its UUID, so its table name `rdt_` is known before anything is saved. +5. **Build the new table** in one `data_db` transaction: create it, copy the rows in through a temporary table (`ON COMMIT DROP`), and store a JSON comment (resource id and name, dataset title, `built_at`). A failure, such as a value out of range mid-copy, rolls the whole table back, and the old row and table are untouched. +6. **Switch, in one main-database transaction:** + 1. lock the resource row (`select_for_update`), so two uploads of one resource switch one after the other; + 2. re-read the stored file's identity, and if a newer file was saved while this one was building, back off (`NewerFileSaved`), because the newest file wins; + 3. delete the old `ResourceDataTable` row, if any. Its delete receiver schedules the old table's drop for after the commit; + 4. save the new row, carrying over the old row's `created` (see "Dataset default table" below); + 5. rebuild the `ResourceSchema` column rows (descriptions people wrote are kept by column name). Errors escape, and a header longer than the 255-character `field_name` is refused up front. +7. **Confirm the switch.** It counts only if the new row exists after the transaction. +8. **`finally`: drop this build's table unless step 7 confirmed the switch.** That covers a failed build, a failed switch, a newer file, a missing row, and even `KeyboardInterrupt`. +9. After the commit, the receiver drops the old table (see Delete clean-up). + +Returns the new `ResourceDataTable`, or None when the resource was not switched (each reason logged). + +**Why the switch lets errors escape.** The old column rebuild caught each column's error. Inside an atomic block a database error marks the whole block for rollback, so one bad header silently rolled the switch back while the upload still looked successful. + +**Why the newest file wins.** Each upload request saves its own file before indexing it. Without the re-check, a slow build of an older file could switch in last: the download would serve one file while previews, charts and the data API served another. + +**Dataset default table.** `GET /api/datasets//data/` without `resource_id` serves the dataset's data table with the oldest `created`. The new row inherits the old row's `created`, so replacing a file never moves a dataset's default to another resource. The migration doesn't reshuffle defaults either. + +### Helpers (`api/utils/data_tables.py`, Tier 2 unless noted) +- `is_inside_transaction() -> bool`: whether the main DB is in an atomic block. +- `has_indexable_file(resource) -> bool`: has a stored file whose format is in `INDEXED_FORMATS`; logs why not. +- `load_resource_frame(resource, timeout=60) -> LoadedFile | None`: frame plus `FileIdentity(name, sha256)`; None (logged) when unreadable, too slow, or empty. Depends on Tier 1 `local_copy`, `load_tabular_data` (`api/utils/file_utils.py`). +- `stored_file_identity(resource) -> FileIdentity | None`: the current file, read fresh from the database. +- `new_data_table_row(resource) -> ResourceDataTable`: unsaved, named `rdt_`. +- `describe_data_table(resource, built_at, unverified=False) -> str`: the table comment (JSON). `unverified` is used only by the migration. +- `build_data_table(table_name, frame, comment)`: one `data_db` transaction. Raises on failure, leaving nothing behind. Names via `sql.Identifier`, comment via `sql.Literal`. +- `lock_resource(resource)`: `select_for_update` on the resource row (no-op on SQLite). +- `retire_data_table_row(resource) -> datetime | None`: deletes the current row, returns its `created`. +- `save_data_table_row(row, created)`: inserts, then sets `created` with a queryset update (it's `auto_now_add`). +- `rebuild_resource_schema(resource, frame)`: replaces column rows. Raises `ValueError("column N header is longer than 255 characters")` and lets DB errors escape. +- `data_table_row_exists(row_id) -> bool`. +- `drop_data_table(name) -> bool`: `DROP TABLE IF EXISTS` with `SET LOCAL lock_timeout = '5s'`. Never raises: a failure is logged (`data_table_drop_failed`) and the table is left as an orphan for the migration command to report. +- `error_fields(error) -> dict`: `error_type`, Postgres `error_code`, and `where` (innermost frame in `api/`). **Never the message**, because COPY errors quote the offending cell value. +- `get_sql_type(dtype) -> str`: BIGINT / DOUBLE PRECISION / TIMESTAMP / BOOLEAN / TEXT (moved here; still importable from `data_indexing`, as are `DATA_DB` and `INDEXED_FORMATS`). + +`api/utils/data_table_names.py` (no model imports, because the model uses it): +- `data_table_name(row_id) -> "rdt_" + hex`. +- `physical_table_name(name) -> str`: the first 63 bytes, never half a character (what Postgres stores). +- `may_share_physical_table(name) -> bool`: only names of ≥ 60 bytes can be the cut of a longer one. + +### Model rule +`ResourceDataTable.save()` fills a blank `table_name` with `data_table_name(self.id)`. Existing rows keep their stored names. Nothing else generates names. Rolling the code back is safe: old code only generates a name when the field is blank, and every reader uses the stored name. + +--- + +## Submodule: Delete clean-up + +### Trigger +`post_delete` with `sender=ResourceDataTable`. Django sends it for every row in a cascade or a queryset delete, because a receiver is connected (no fast delete). That covers re-upload (`retire_data_table_row`), `deleteFileResource`, `deleteDataset`, admin deletes of a row, resource or dataset, and the migration's rebuilds. + +### Flow (English) +1. The receiver schedules `drop_data_table_if_unshared(table_name, row_id)` with `transaction.on_commit(..., using=, robust=True)`. +2. After the commit, the drop helper checks whether another row still maps to the same physical table. This is possible only for old long names; `rdt_` names skip the check. + 1. If one does, it logs `data_table_drop_skipped_shared` and keeps the table. + 2. Otherwise it drops the table (5 s lock timeout). Any failure is logged and swallowed. + +A rolled-back delete never runs the callback, so its data is kept. + +**Registration.** The module is imported by `api/signals/__init__.py`. A test checks the receiver is connected in a fresh interpreter, because `collaborative_signals.py` once shipped without being imported and never ran. + +**Lock order.** A switch locks the resource, then its data-table row. A resource delete locks the rows, then the resource. A switch and a delete of the same resource at the same moment can therefore deadlock. Postgres aborts one of them, and nothing is lost: the aborted switch drops its own table, and the aborted delete leaves everything in place. + +--- + +## Submodule: Readers (unchanged) +`get_row_count`, `get_preview_data`, `get_resource_columns` (falls back to `information_schema` by table name when there are no column rows), `fetch_resource_data` (data API; column names via `_column_ident`, which doubles `%`), and `query_resource_data` (charts; `{{table}}` replaced by the quoted table name). They all look up `ResourceDataTable` per call and work unchanged on `rdt_` tables. A reader that fetched the old row just before a switch can hit "relation does not exist" once the old table is dropped. It then takes its existing error path (None / 0 / the data API's 500); it never gets the new file's rows under the old row. Readers still compose the table name with f-strings; `rdt_` names are safe in that position. + +--- + +## Submodule: Migration of existing tables — `migrate_data_table_names` + +### Trigger +Operator-run, once after deploy (and safe to rerun any time as an orphan audit). + +### Business use case +Move every existing table to an `rdt_` name and repair tables two resources were sharing. Then list, and after review drop, tables nothing uses (left behind by deletes from before this feature). + +### Flow (English) +1. Check the options. `--drop-orphans` needs `--apply` and `--from-report`, and `--report` must not name the reviewed file. +2. **Safety:** refuse if `data_db` and the main database name the same host, port and database, or if `data_db` holds `django_migrations` or any Django model's table. Either would make every application table look like an orphan. +3. Read the reviewed orphan list now, if dropping. A missing or malformed file stops the run before any change. +4. Write an "in progress" report. This proves the path is writable, and a run that dies leaves a trace. +5. Read every row (oldest first) and every `data_db` table (size and comment), and sort each row (limited to `--resource-id` if given): + - **already migrated**: `rdt_` exists; + - **row update**: a crashed earlier run renamed the table but didn't update the row (the old name is gone, `rdt_` exists); + - **rename**: an old name of ≤ 63 bytes. It can't have collided; + - **rebuild**: an old name of > 63 bytes, whether one row or several point at the physical table. It may already hold another resource's data, so it is never just renamed while its file is usable. Each rebuild carries whether the table is shared; + - **missing table**: neither name exists. +6. **Dry run (the default):** change nothing, but check each rebuild's and missing table's stored file, and record the planned outcome: + - `planned_rebuild`; + - `planned_unverified_rename`: the file is missing or unusable and the table is not shared; + - `planned_missing_file` / `planned_not_indexable`: the table is shared, so it is left for a person; + - `planned_reindex`. +7. **`--apply`**, in this order: + 1. **Row updates.** + 2. **Renames:** in a main-DB transaction, lock the resource and re-check that the row still has its old name (otherwise `changed`). Then, in `data_db`, `SET LOCAL lock_timeout = '2s'`, rename the table and set its comment. Update the row last. A lock timeout gives `skipped_lock`, and a rerun finishes it. + 3. **Rebuilds:** oldest first, through `index_resource_data(resource, load_timeout=None)`. The old row's delete receiver keeps a shared table until its last row has moved. + - If the file is missing or can no longer become a table (for example, later replaced by a PDF) and no other row uses the table, it is renamed and **marked `"unverified": true` in its comment** (`unverified`). + - If the table is shared, it is left (`missing_file` / `not_indexable`). Once its sharers have moved, the next run finds it alone and marks it unverified. + 4. **Missing tables:** re-indexed from the file, or `missing_file` / `not_indexable`. +8. **`--drop-orphans`:** recompute orphans now, and drop only those that are both in the reviewed report and still orphans. +9. Write the full report and print the summary. Every row's outcome is also logged as it happens (`data_table_migration_row`). "nothing to do" is printed when no row needs a rerun: every row is migrated, or needs a person (missing or unusable file). + +**Orphans:** +- **Never orphans:** a Django model's table, any row's physical table or `rdt_`, and an `rdt_` table whose comment says it was built under an hour ago (its upload may still be switching). +- **Listed:** `rdt_` tables without a readable build time, because every table the new code builds has a comment written in the same transaction as its creation. +- Each listed orphan carries its size and comment. + +**Unverified tables** are listed in every report (`unverified_tables`), dry run or apply, until the resource is re-uploaded (the new table has no mark). They may hold another resource's data. Restore the resource's file and re-upload it, or check the data by hand. + +**Report** (JSON, default `data_table_migration_.json` in the working directory): +- `mode`, `status`; +- `counts` by outcome and `shared_groups`; +- `rows` (ids, table name, action, outcome, `shared`); +- `needs_manual_fix`; +- `unverified_tables`; +- `orphans` (name, size, comment), `orphans_dropped`; +- `access_models_touched`: access models whose column choices are reset because the resource's column rows are recreated. This is existing re-upload behaviour. + +No file contents or values appear in the report. + +### Runbook +1. Deploy. New uploads use `rdt_` names from now on, and old names keep working. +2. Off-peak: `python manage.py migrate_data_table_names --report dry.json`. Review `counts`, `shared_groups`, `needs_manual_fix` (restore files first if you can), `unverified_tables` and `orphans`. +3. `python manage.py migrate_data_table_names --apply --report apply1.json`. Rerun until it prints "nothing to do". + - `skipped_lock` and `failed` rows are retried. + - A shared table whose file is missing is marked on the run after its sharers move. +4. At least an hour later: `python manage.py migrate_data_table_names --report orphans.json`. Review the orphan list and remove from the file any table you want kept. +5. `python manage.py migrate_data_table_names --apply --drop-orphans --from-report orphans.json --report drop.json`. +6. Follow up on `unverified_tables` and `needs_manual_fix` by hand. + +Renames take an exclusive lock briefly and wait up to 2 s behind running reads. Rebuilds hold two copies of a table for a moment. + +**Old naming schemes the command meets:** +- `resource_data_<32 hex>`: March–May 2025, 46 characters, so renamed. +- Raw `--<32 hex>`: May–July 2025. These can be mixed case, contain spaces or non-ASCII, and are usually over 63 bytes, so they are rebuilt. +- `slug(name)[:50]_slug(title)[:50]_<8 hex>`: from July 2025. Short ones are renamed and long ones rebuilt. + +Short names of every scheme are unique, so they can't have collided. + +### Helpers (`api/utils/data_table_migration.py`, plus `list_data_db_tables`, `django_table_names`, `assert_data_db_is_separate`, `table_built_at`, `table_is_unverified` in `data_tables.py`) +- `read_data_table_rows()`: every row, oldest first, as `DataTableRow(row_id, resource_id, table_name, created)`. +- `plan_table_migration(rows, tables, now, resource_id=None) -> MigrationPlan`: pure; the sorting above, plus `orphans` and `shared_groups`. +- `find_orphans(rows, tables, now)`: pure; the orphan rules. +- `file_problem(resource)`: `missing_file`, `not_indexable` or None. Storage errors are raised. +- `rebuild_decision(row, shared)` / `reindex_decision(row)`: what `--apply` will do (`check_failed` if the file check errors). +- `point_row_at_its_table`, `rename_legacy_table(row, unverified=False)`, `rebuild_from_file(row, shared)`, `reindex_missing_table(row)`: the actions. Each returns an outcome string and never raises. +- `find_unverified(rows, tables)`: rows whose table carries the mark. +- `access_models_touched(resource_ids)`, `read_reviewed_orphans(path)` (CommandError when unreadable), `same_file`, `drop_reviewed_orphans(reviewed, current)`, `record_outcome` (logs and returns the report line), `nothing_left_to_do`, `build_report`, `write_report`, `summary_lines`. + +--- + +## Data model +- `ResourceDataTable` (`resource_data_table`): `id` UUID pk, `resource` one-to-one, `table_name` CharField(255) unique, `created`, `modified`. **No schema change and no migration file.** Only the generated value changed. `created` is now carried over on re-upload. +- `ResourceSchema`: rebuilt on every re-index (existing behaviour). `AccessModelResource.fields` (many-to-many to these rows) loses its selections whenever they are rebuilt, as before. +- `ResourceFileDetails.file` and `sha256` are read as the file identity (`sha256` is computed on save when the file changes). +- `data_db` tables: one per tabular resource, named `rdt_`, with a JSON comment: `resource_id`, `resource`, `dataset`, `built_at`, and `unverified` when the migration kept a table without a rebuild. +- No Elasticsearch documents are affected. + +## Indexing & performance +- **Switch:** `Resource` by pk (`select_for_update`); `ResourceDataTable` by `resource_id` (one-to-one unique index); `ResourceFileDetails` by `resource_id` (unique). +- **Receiver guard:** `ResourceDataTable.table_name LIKE '<63-byte prefix>%'`, only for old names of ≥ 60 bytes (`rdt_` names skip it). This uses the unique btree on `table_name`, but may be a sequential scan on non-C collations. That's acceptable at one row per tabular resource (thousands). +- **Command:** one ordered read of `ResourceDataTable` (values only, iterator) and one `pg_class` catalogue query on `data_db`. Each is repeated once at the end, and once more when dropping orphans. +- **`data_db`:** no indexes added. Readers do full-table reads with existing timeouts. +- **Disk:** a re-upload briefly holds two copies of the table. +- No new user-facing query, so no pagination change. + +## Security +- No new endpoint and no permission change. The file mutations keep their `ChangeResourcePermission` (edit rights on the dataset) and the data API keeps its existing check. If a user from another organization sends the same mutation, it is refused exactly as before (`tests/schema/test_resource_permissions.py`). +- Out of scope, and unchanged: the public row API serves rows of any PUBLISHED dataset regardless of `access_type`. +- All new SQL composes table names with `sql.Identifier` and comments with `sql.Literal`. Resource and dataset names reach SQL only as a literal comment. +- The orphan sweep refuses to run against a database that looks like the main one, never drops Django tables, and drops only names a person reviewed that are still unused. +- Logs carry ids, outcomes, counts and error codes. Never file contents, cell values or database error messages. + +## SDK impact +SDK: no. This is storage-internal, and no public API changes shape. + +## Tests + +Tests marked `data_db` run against a real Postgres. Locally: + +``` +docker run -d --name dataspace-test-data-db -e POSTGRES_PASSWORD=postgres -p 55432:5432 postgres:16 +export TEST_DATA_DB_URL=postgres://postgres:postgres@localhost:55432/postgres +pytest tests/data_index +``` + +How the setup works: +- **Database:** `tests/test_settings.py` adds the `data_db` alias only when `TEST_DATA_DB_URL` is set. pytest-django creates and drops `test_dataspace_data_db`, and `tests.data_index.routers.DataDbTestRouter` keeps Django models out of it. +- **Skipping:** without the URL these tests are skipped, unless `REQUIRE_DATA_DB_TESTS=1` (set in CI, which runs a `postgres:16` service). Then the run fails, because a run that skipped them proves nothing. +- **Clean-up:** an autouse fixture drops every `public` table after each such test, since pytest-django only empties tables of models routed there, and there are none. +- **Where it applies:** the skip/fail hook and that fixture live in `tests/data_index/conftest.py`, and `uses_data_db` in `tests/data_index/support.py`. Tests elsewhere that need them must move both into a shared conftest. +- **Main database:** stays in-memory SQLite, so row locks are no-ops there. +- **Locally:** if `drf_yasg` fails with "No module named pkg_resources", install `setuptools<81` (CI installs it). + +### Layer 1 — DB helper tests +- **Naming:** a new row is named `rdt_`, and an existing row keeps its stored name on save. Two resources with names identical for 60+ characters get their own tables with their own rows. +- **Uploads:** a first upload creates the row, the table, the column rows and the comment. A re-upload switches to a new table, drops the old one and keeps custom descriptions. +- **Switch timing:** at the moment of the switch the new table is full and the old one still exists, and the lock is taken inside the switch transaction. The old table is dropped only after commit, with no transaction open and the new row saved. +- **Failures keep the old data:** a value out of range mid-copy keeps the old row and table, leaves no new table, and logs error code 22003 without the value. A 300-character header fails the switch, keeps the old row, table and column rows, and drops the new table. +- **Readers and races:** a reader still holding the old name gets "does not exist", never new rows. A slower build of an older file backs off when a newer file was saved, and the newer one stays. +- **Transaction and default:** indexing inside an outer transaction is refused and builds nothing. A re-upload keeps the row's `created` and the dataset's default table in the data API. +- **Deletes:** deleting a resource drops its table; deleting a dataset drops all its tables; a rolled-back delete keeps the table. Two legacy rows sharing a table: the table is kept until the last one is deleted. A drop of a table that's already gone doesn't fail. A drop blocked past the lock timeout is logged and the delete still succeeds. +- **Readers (regression):** on an `rdt_` table with a `%` header, the row count, preview, column list (including the `information_schema` fallback), filtered and ordered fetch, and a chart query all work. +- **Helpers:** indexable-file checks; load records the file identity, returns None for a header-only file, and cleans headers; the column rebuild lets DB errors escape and refuses over-long headers. +- **Migration — moving tables:** + - a short table is renamed only on `--apply`; + - a shared long table is rebuilt from each resource's own file, and the shared table is dropped after both rows move; + - a single long table is rebuilt, not renamed; + - with its file gone, a single long table is renamed and marked unverified, and later runs still list it; + - with its file gone, a shared row stays while the other row moves; + - a run after a crash just updates the row; + - a rename blocked by a reader is skipped, then finished on a rerun. +- **Migration — orphans:** listed with size; drop needs `--from-report`; only reviewed names that are still orphans are dropped; young `rdt_` tables are never listed. +- **Migration — reruns and reports:** + - a second `--apply` has nothing to do; + - access models are listed; + - the dry run shows the planned outcome per long table, the shared flag and `shared_groups`; + - an unusable file (PDF) is marked unverified, or left while shared, and the runs settle on "nothing to do"; + - rebuilds read with no time limit; + - a missing table is re-indexed or left for a person; + - `--resource-id` limits the run; + - a rename backs off when an upload replaced the row after planning. +- **Migration — refusals:** a bad reviewed report stops the run before any change; the report may not overwrite the reviewed file; the run refuses when the data database is the main one or holds `django_migrations`. + +### Layer 2 — Non-DB helper tests +- **Names:** `rdt_` plus lowercase hex, 36 characters. The physical name is cut at 63 bytes and never mid-character. +- **Comment:** a resource name with quotes and semicolons is stored as text, not run as SQL. +- **Orphan rules:** Django tables, a row's tables, and fresh builds are excluded. +- **Registration:** the receiver is connected after app load (fresh interpreter). + +### Layer 3 — Flow tests +- **Upload flow order:** load → name → build → lock → file check → retire → save → rebuild columns → confirm. +- **Clean-up:** this build's table is dropped when the build, lock, retire or column rebuild fails, on a newer file, when the row is missing after commit, and on `KeyboardInterrupt`. It is never dropped after a confirmed switch. +- **Nothing to index:** nothing is built when inside a transaction, when there is no indexable file, or when nothing loaded. +- **Receiver:** schedules one guarded drop per deleted row, via `on_commit(robust=True)`. +- **Command:** the safety check runs first; a dry run changes nothing and writes the report; `--apply` runs row updates → renames → rebuilds (in plan order) → re-indexes; a failed safety check stops before any read; orphans are dropped only with `--apply` and a reviewed report. + +### Layer 4 — Backend API e2e +As the dataset owner, through the GraphQL schema and the data API: +- `createFileResources` gives an `rdt_` table, and the data API returns its rows; +- `updateFileResource` with a new file serves the new rows and the old table is gone; +- `resetFileResourceSchema` switches to a fresh table; +- `deleteFileResource` drops the table, and the data API returns 404; +- `deleteDataset` drops every table. + +Cross-org denial is unchanged and covered by `tests/schema/test_resource_permissions.py`. + +### Layer 5 — API user journey +`tests/journeys/data_index/reupload-and-delete.py`, on request against a running backend (`KEYCLOAK_TEST_TOKEN`, `TEST_DATASET_ID`, `TEST_BASE_URL`): +1. Upload a CSV and read its rows. +2. Replace it while a thread polls the data API. Every read is the complete old file or the complete new one. +3. Fire two replacements at once. The data API serves the same file the download serves. +4. Delete the resource. The data API returns 404. + +At least an hour later, the operator's dry run should list no orphan from the racing uploads. + +### Layer 6 — Browser e2e +n/a — no UI change. + +### Visual checklist (manual, on request) +n/a — no UI change. + +### LLM-judge points +n/a — all assertions are deterministic. + +--- + +## Failure model + +| What could go wrong | Guarantee | +|---|---| +| Two resources map to one physical table | `rdt_` names are unique by construction. | +| A reader sees an empty or half-filled table | The new table is built fully before the switch, and the old one is dropped only after commit. | +| A failed re-upload wipes the data | The old row and table are untouched until the switch commits. | +| A failed build or switch leaves a stray table | `finally` drops it unless the switch is confirmed. | +| A rolled-back delete loses data | Drops run only `on_commit`. | +| The receiver drops a table another row still uses | Shared-name guard for old long names. | +| The receiver isn't loaded | Imported in `api/signals/__init__.py`, and a fresh-interpreter test checks it. | +| Two re-uploads race | Resource row lock plus the newest-file check. | +| A drop fails (lock, missing table) | Logged, never raised; the table becomes an orphan the command reports. | +| A rename gets stuck behind long reads | 2 s lock timeout; skipped, reported, and finished by a rerun. | +| The command crashes between rename and row update | The rerun updates the row; the "unverified" mark survives in the comment. | +| A shared table is rebuilt from the wrong file | Each row is rebuilt from its own file, and the table is dropped only after the last row moves. | +| A stored file is missing or unusable | Reported per row. An unshared table is marked unverified and listed in every report; a shared one is left for a person. | +| A collided table whose sharer was deleted gets renamed | Long old names are always rebuilt while the file is usable. | +| The orphan sweep deletes application tables or an in-flight upload's table | Same-database and Django-table refusals, the one-hour age guard, and only reviewed names dropped. | +| A swallowed error rolls the switch back silently | Column-row errors escape, the switch is confirmed by the row existing, and indexing refuses to run inside an outer transaction. | +| Test data tables leak between tests | An autouse fixture drops every `public` table after each `data_db` test. | + +## Limitations & future work +- **A failed re-upload still reports success to the uploader.** This is existing behaviour: the mutations ignore the indexing result. Now the old data stays served while the download serves the new file, until a successful re-upload. Surfacing the failure would change the upload API's contract and is left for a decision. +- **Two long headers that differ only after 63 bytes** become one column in Postgres, so the build fails. Devanagari headers hit this at about 21 characters. This existed before and is not fixed here. +- **Access-model column selections are reset** whenever column rows are rebuilt (every re-upload, and migration rebuilds). Carrying them over by column name would be a small follow-up. +- **Readers still use f-string table names.** That is safe for `rdt_` names; they were left as they are deliberately. +- The plan's failure table said 5 s for the rename lock timeout. The design and the code use 2 s for renames and 5 s for drops. diff --git a/docs/dataset_data_api.md b/docs/dataset_data_api.md index 5e970ea9..8857b105 100644 --- a/docs/dataset_data_api.md +++ b/docs/dataset_data_api.md @@ -4,7 +4,8 @@ This document describes the HTTP endpoints and SDK methods for fetching the *indexed tabular data* that DataSpace stores in the `data_db` PostgreSQL database. When a CSV / XLSX / Parquet / JSON resource is uploaded, its rows are indexed into a per-resource table so they can be queried, filtered, and -streamed without re-downloading the source file. +streamed without re-downloading the source file. How those tables are built, +named, replaced and cleaned up: `api/utils/data_indexing_architecture.md`. ## Overview From b4bb21f1f103c735491ff6b9b1e329a38f1c04c8 Mon Sep 17 00:00:00 2001 From: psaesha Date: Sat, 14 Nov 2026 11:00:04 +0530 Subject: [PATCH 4/7] fix(data-index): keep the new table when a run stops after the switch Final QC found that stopping a command (Ctrl+C) while the old table's after-commit drop waited for a reader made the flow's clean-up drop the new table, though the resource already pointed at it. The clean-up now asks the database first: it drops the build's table only if its row isn't saved, and leaves the table for the orphan audit when it can't tell. That also covers a commit that went through but reported an error. The runbook now says a row that fails twice the same way needs a person. Plan: plans/claude/data-index-table-names.md (final QC) --- api/utils/data_indexing.py | 7 ++-- api/utils/data_indexing_architecture.md | 7 +++- api/utils/data_tables.py | 23 ++++++++++ tests/data_index/test_reindex_switch.py | 56 +++++++++++++++++++++---- 4 files changed, 81 insertions(+), 12 deletions(-) diff --git a/api/utils/data_indexing.py b/api/utils/data_indexing.py index 6a5af5aa..22159628 100644 --- a/api/utils/data_indexing.py +++ b/api/utils/data_indexing.py @@ -28,7 +28,7 @@ build_data_table, data_table_row_exists, describe_data_table, - drop_data_table, + drop_unswitched_table, error_fields, get_sql_type, has_indexable_file, @@ -139,9 +139,10 @@ def index_resource_data( except Exception as error: logger.error("data_index_failed", resource_id=resource_id, **error_fields(error)) finally: - # Anything short of a confirmed switch leaves this build's table unused: drop it. + # Short of a confirmed switch, drop this build's table, unless its row was saved + # after all (e.g. a run stopped while the old table's drop was waiting). if not switched: - drop_data_table(new_row.table_name) + drop_unswitched_table(new_row) if not switched: return None diff --git a/api/utils/data_indexing_architecture.md b/api/utils/data_indexing_architecture.md index 0ca820a3..5b82e7d2 100644 --- a/api/utils/data_indexing_architecture.md +++ b/api/utils/data_indexing_architecture.md @@ -48,7 +48,7 @@ A publisher uploads or replaces a tabular file. Visitors then preview it, chart 4. save the new row, carrying over the old row's `created` (see "Dataset default table" below); 5. rebuild the `ResourceSchema` column rows (descriptions people wrote are kept by column name). Errors escape, and a header longer than the 255-character `field_name` is refused up front. 7. **Confirm the switch.** It counts only if the new row exists after the transaction. -8. **`finally`: drop this build's table unless step 7 confirmed the switch.** That covers a failed build, a failed switch, a newer file, a missing row, and even `KeyboardInterrupt`. +8. **`finally`: drop this build's table unless step 7 confirmed the switch.** That covers a failed build, a failed switch, a newer file, a missing row, and even `KeyboardInterrupt`. Before dropping, it checks the database: if the new row was saved after all (a command stopped with Ctrl+C while the old table's after-commit drop waited for a reader, or a commit that went through but reported an error), the table is the resource's live data and stays. If the check itself fails, the table is left for the orphan audit. 9. After the commit, the receiver drops the old table (see Delete clean-up). Returns the new `ResourceDataTable`, or None when the resource was not switched (each reason logged). @@ -72,6 +72,7 @@ Returns the new `ResourceDataTable`, or None when the resource was not switched - `save_data_table_row(row, created)`: inserts, then sets `created` with a queryset update (it's `auto_now_add`). - `rebuild_resource_schema(resource, frame)`: replaces column rows. Raises `ValueError("column N header is longer than 255 characters")` and lets DB errors escape. - `data_table_row_exists(row_id) -> bool`. +- `drop_unswitched_table(row)`: the flow's clean-up. It drops the build's table only if its row isn't saved, and leaves it when the check fails. Never raises. - `drop_data_table(name) -> bool`: `DROP TABLE IF EXISTS` with `SET LOCAL lock_timeout = '5s'`. Never raises: a failure is logged (`data_table_drop_failed`) and the table is left as an orphan for the migration command to report. - `error_fields(error) -> dict`: `error_type`, Postgres `error_code`, and `where` (innermost frame in `api/`). **Never the message**, because COPY errors quote the offending cell value. - `get_sql_type(dtype) -> str`: BIGINT / DOUBLE PRECISION / TIMESTAMP / BOOLEAN / TEXT (moved here; still importable from `data_indexing`, as are `DATA_DB` and `INDEXED_FORMATS`). @@ -167,6 +168,7 @@ No file contents or values appear in the report. 2. Off-peak: `python manage.py migrate_data_table_names --report dry.json`. Review `counts`, `shared_groups`, `needs_manual_fix` (restore files first if you can), `unverified_tables` and `orphans`. 3. `python manage.py migrate_data_table_names --apply --report apply1.json`. Rerun until it prints "nothing to do". - `skipped_lock` and `failed` rows are retried. + - A row that comes back `failed` on two runs in a row needs a person: its file is stored and in a supported format but won't load or switch (it doesn't parse, has only a header, or has a header over 255 characters). Find it under `rows` with outcome `failed`, using the resource id and the `data_index_failed` / `data_index_load_failed` log lines. Its old table keeps serving unmarked, and a long one may hold another resource's data. - A shared table whose file is missing is marked on the run after its sharers move. 4. At least an hour later: `python manage.py migrate_data_table_names --report orphans.json`. Review the orphan list and remove from the file any table you want kept. 5. `python manage.py migrate_data_table_names --apply --drop-orphans --from-report orphans.json --report drop.json`. @@ -241,6 +243,7 @@ How the setup works: - **Uploads:** a first upload creates the row, the table, the column rows and the comment. A re-upload switches to a new table, drops the old one and keeps custom descriptions. - **Switch timing:** at the moment of the switch the new table is full and the old one still exists, and the lock is taken inside the switch transaction. The old table is dropped only after commit, with no transaction open and the new row saved. - **Failures keep the old data:** a value out of range mid-copy keeps the old row and table, leaves no new table, and logs error code 22003 without the value. A 300-character header fails the switch, keeps the old row, table and column rows, and drops the new table. +- **Stopped runs:** a run stopped (KeyboardInterrupt) while the old table's after-commit drop waits keeps the new table the resource already points at. The clean-up keeps a table whose row is saved, drops one whose row isn't, and leaves it when the check fails. - **Readers and races:** a reader still holding the old name gets "does not exist", never new rows. A slower build of an older file backs off when a newer file was saved, and the newer one stays. - **Transaction and default:** indexing inside an outer transaction is refused and builds nothing. A re-upload keeps the row's `created` and the dataset's default table in the data API. - **Deletes:** deleting a resource drops its table; deleting a dataset drops all its tables; a rolled-back delete keeps the table. Two legacy rows sharing a table: the table is kept until the last one is deleted. A drop of a table that's already gone doesn't fail. A drop blocked past the lock timeout is logged and the delete still succeeds. @@ -274,7 +277,7 @@ How the setup works: ### Layer 3 — Flow tests - **Upload flow order:** load → name → build → lock → file check → retire → save → rebuild columns → confirm. -- **Clean-up:** this build's table is dropped when the build, lock, retire or column rebuild fails, on a newer file, when the row is missing after commit, and on `KeyboardInterrupt`. It is never dropped after a confirmed switch. +- **Clean-up:** the unswitched-build clean-up runs when the build, lock, retire or column rebuild fails, on a newer file, when the row is missing after commit, and on `KeyboardInterrupt`. It never runs after a confirmed switch. - **Nothing to index:** nothing is built when inside a transaction, when there is no indexable file, or when nothing loaded. - **Receiver:** schedules one guarded drop per deleted row, via `on_commit(robust=True)`. - **Command:** the safety check runs first; a dry run changes nothing and writes the report; `--apply` runs row updates → renames → rebuilds (in plan order) → re-indexes; a failed safety check stops before any read; orphans are dropped only with `--apply` and a reviewed report. diff --git a/api/utils/data_tables.py b/api/utils/data_tables.py index 776967f0..3e2f8eb4 100644 --- a/api/utils/data_tables.py +++ b/api/utils/data_tables.py @@ -359,6 +359,29 @@ def drop_data_table(table_name: str) -> bool: return True +def drop_unswitched_table(row: ResourceDataTable) -> None: + """Drop the table of a build whose switch wasn't confirmed, unless its row was saved after all. + + The switch can commit and still not be confirmed: a run stopped (Ctrl+C) + while the old table's after-commit drop was waiting, or a commit that went + through but reported an error. So the row is checked first; if it exists, + the table is the resource's live data and stays. If the check itself fails, + the table is left for the migration command's orphan audit. Never raises + an Exception. + """ + try: + saved = data_table_row_exists(row.pk) + except Exception as error: + logger.error( + "data_index_switch_check_failed", table_name=row.table_name, **error_fields(error) + ) + return + if saved: + logger.warning("data_index_switch_saved_after_all", table_name=row.table_name) + return + drop_data_table(row.table_name) + + def drop_data_table_if_unshared(table_name: str, row_id: Any) -> None: """Drop a deleted row's table, unless another row still uses the same physical table. diff --git a/tests/data_index/test_reindex_switch.py b/tests/data_index/test_reindex_switch.py index 52b9f13d..db6ef8e0 100644 --- a/tests/data_index/test_reindex_switch.py +++ b/tests/data_index/test_reindex_switch.py @@ -28,6 +28,8 @@ from api.utils.data_tables import ( FileIdentity, LoadedFile, + build_data_table, + drop_unswitched_table, has_indexable_file, load_resource_frame, rebuild_resource_schema, @@ -182,6 +184,46 @@ def test_a_reader_still_holding_the_old_name_gets_does_not_exist_not_new_rows( cursor.execute(f'SELECT * FROM "{stale.table_name}"') +@uses_data_db +def test_a_run_stopped_while_the_old_table_is_dropped_keeps_the_new_table( + add_resource: Any, tables: Any +) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + old = index_resource_data(resource) + replace_file(resource, "districts_v2.csv") + + def stopped_by_the_operator(table_name: str, row_id: Any) -> None: + raise KeyboardInterrupt # Ctrl+C while the old table's drop waits for a reader + + with patch( + "api.signals.data_table_signals.drop_data_table_if_unshared", stopped_by_the_operator + ): + with pytest.raises(KeyboardInterrupt): + index_resource_data(resource) + + current = ResourceDataTable.objects.get(resource=resource) + assert current.pk != old.pk # the switch had committed + assert tables.exists(current.table_name) + assert len(tables.rows(current.table_name)) == 4 + + +@uses_data_db +def test_dropping_an_unswitched_build_checks_the_row_first(add_resource: Any, tables: Any) -> None: + resource = add_resource("cases.csv", "districts_v1.csv") + saved = index_resource_data(resource) + unsaved = data_tables.new_data_table_row(resource) + build_data_table(unsaved.table_name, pd.DataFrame({"a": [1]}), "{}") + + drop_unswitched_table(saved) + drop_unswitched_table(unsaved) + assert tables.exists(saved.table_name) + assert not tables.exists(unsaved.table_name) + + with patch("api.utils.data_tables.data_table_row_exists", side_effect=RuntimeError("db gone")): + drop_unswitched_table(saved) # can't tell: leave it for the orphan audit + assert tables.exists(saved.table_name) + + @uses_data_db def test_a_slower_build_of_an_older_file_backs_off_when_a_newer_file_was_saved( add_resource: Any, tables: Any @@ -319,7 +361,7 @@ def test_rebuild_columns_refuses_a_header_longer_than_the_column_list_allows( "save_data_table_row", "rebuild_resource_schema", "data_table_row_exists", - "drop_data_table", + "drop_unswitched_table", ] FILE = FileIdentity("resources/cases.csv", "abc123") @@ -385,7 +427,7 @@ def test_flow_drops_its_table_when_a_step_fails(flow: Any, helper: str) -> None: assert index_resource_data(MagicMock()) is None - flow.drop_data_table.assert_called_once_with("rdt_new") + flow.drop_unswitched_table.assert_called_once_with(flow.new_data_table_row.return_value) def test_flow_drops_its_table_when_a_newer_file_was_saved(flow: Any) -> None: @@ -394,7 +436,7 @@ def test_flow_drops_its_table_when_a_newer_file_was_saved(flow: Any) -> None: assert index_resource_data(MagicMock()) is None flow.retire_data_table_row.assert_not_called() - flow.drop_data_table.assert_called_once_with("rdt_new") + flow.drop_unswitched_table.assert_called_once_with(flow.new_data_table_row.return_value) def test_flow_drops_its_table_when_the_new_row_is_missing_after_the_commit(flow: Any) -> None: @@ -402,7 +444,7 @@ def test_flow_drops_its_table_when_the_new_row_is_missing_after_the_commit(flow: assert index_resource_data(MagicMock()) is None - flow.drop_data_table.assert_called_once_with("rdt_new") + flow.drop_unswitched_table.assert_called_once_with(flow.new_data_table_row.return_value) def test_flow_drops_its_table_even_when_interrupted(flow: Any) -> None: @@ -411,12 +453,12 @@ def test_flow_drops_its_table_even_when_interrupted(flow: Any) -> None: with pytest.raises(KeyboardInterrupt): index_resource_data(MagicMock()) - flow.drop_data_table.assert_called_once_with("rdt_new") + flow.drop_unswitched_table.assert_called_once_with(flow.new_data_table_row.return_value) def test_flow_keeps_the_table_after_a_confirmed_switch(flow: Any) -> None: index_resource_data(MagicMock()) - flow.drop_data_table.assert_not_called() + flow.drop_unswitched_table.assert_not_called() @pytest.mark.parametrize("stop_at", ["inside_transaction", "no_file", "nothing_loaded"]) @@ -431,4 +473,4 @@ def test_flow_builds_nothing_when_there_is_nothing_to_index(flow: Any, stop_at: assert index_resource_data(MagicMock()) is None flow.build_data_table.assert_not_called() - flow.drop_data_table.assert_not_called() + flow.drop_unswitched_table.assert_not_called() From 0e339eb53e1b0d31f97ac5dc9139616126e94130 Mon Sep 17 00:00:00 2001 From: psaesha Date: Sat, 14 Nov 2026 15:42:21 +0530 Subject: [PATCH 5/7] docs(data-index): mark the cut-column-name collision as a known TODO Two headers that differ only after Postgres's 63-byte cut become one column, so the file never gets a table. This bug predates per-upload tables and is noted, not fixed, by user decision. --- api/utils/data_indexing_architecture.md | 2 +- api/utils/data_tables.py | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/api/utils/data_indexing_architecture.md b/api/utils/data_indexing_architecture.md index 5b82e7d2..9eab48ed 100644 --- a/api/utils/data_indexing_architecture.md +++ b/api/utils/data_indexing_architecture.md @@ -336,7 +336,7 @@ n/a — all assertions are deterministic. ## Limitations & future work - **A failed re-upload still reports success to the uploader.** This is existing behaviour: the mutations ignore the indexing result. Now the old data stays served while the download serves the new file, until a successful re-upload. Surfacing the failure would change the upload API's contract and is left for a decision. -- **Two long headers that differ only after 63 bytes** become one column in Postgres, so the build fails. Devanagari headers hit this at about 21 characters. This existed before and is not fixed here. +- **TODO — two long headers that differ only after 63 bytes** become one column in Postgres, so the build fails and that file never gets a table (the upload still reports success; see the first item). Devanagari headers hit this at about 21 characters. This existed before and is not fixed yet. A fix would catch names that only clash once cut to 63 bytes during column clean-up, and either shorten them uniquely (keeping the stored column names in step) or refuse the file with a clear reason. A `TODO` comment marks the spot in `_clean_column_names` (`data_tables.py`). - **Access-model column selections are reset** whenever column rows are rebuilt (every re-upload, and migration rebuilds). Carrying them over by column name would be a small follow-up. - **Readers still use f-string table names.** That is safe for `rdt_` names; they were left as they are deliberately. - The plan's failure table said 5 s for the rename lock timeout. The design and the code use 2 s for renames and 5 s for drops. diff --git a/api/utils/data_tables.py b/api/utils/data_tables.py index 3e2f8eb4..9b810f7e 100644 --- a/api/utils/data_tables.py +++ b/api/utils/data_tables.py @@ -185,6 +185,9 @@ def on_alarm(signum: int, frame: Any) -> None: def _clean_column_names(columns: List[Any]) -> List[str]: + # TODO: Postgres keeps only the first 63 bytes of a column name, so two headers that + # differ only after that (about 21 Devanagari characters) become one column and the + # file never gets a table. Known, not fixed yet: data_indexing_architecture.md, Limitations. cleaned = [str(c).replace('"', "").replace("\n", " ").replace("\t", " ") for c in columns] seen: Counter = Counter() unique = [] From da795518c781fee93c702a1d66bed1626aac700c Mon Sep 17 00:00:00 2001 From: psaesha Date: Sat, 14 Nov 2026 15:54:00 +0530 Subject: [PATCH 6/7] docs(data-index): mark the silent failed re-upload as a known TODO A re-upload whose rows can't be loaded still reports success, so the download serves the new file while previews, charts and the data API serve the old one. Marked pending a product decision: enforce the database's limits at upload and flag problem columns in the column-description UI. --- api/schema/resource_schema.py | 6 ++++++ api/utils/data_indexing_architecture.md | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/api/schema/resource_schema.py b/api/schema/resource_schema.py index 4890fd0b..42f189b1 100644 --- a/api/schema/resource_schema.py +++ b/api/schema/resource_schema.py @@ -184,6 +184,12 @@ def _validate_file_details_and_update_format(resource: Resource) -> None: def _create_file_resource_schema(resource: Resource) -> None: """Create file resource schema.""" + # TODO: a failed indexing (the file saved, but its rows couldn't be loaded) still + # reports success, so the download serves the new file while previews, charts and + # the data API keep the old one. Pending a product decision: enforce the database's + # limits at upload (e.g. column names within 255 characters) and flag problem + # columns where people add column descriptions. See + # api/utils/data_indexing_architecture.md, Limitations. # Try to index CSV data if applicable data_table = index_resource_data(resource) diff --git a/api/utils/data_indexing_architecture.md b/api/utils/data_indexing_architecture.md index 9eab48ed..adbc6388 100644 --- a/api/utils/data_indexing_architecture.md +++ b/api/utils/data_indexing_architecture.md @@ -335,7 +335,7 @@ n/a — all assertions are deterministic. | Test data tables leak between tests | An autouse fixture drops every `public` table after each `data_db` test. | ## Limitations & future work -- **A failed re-upload still reports success to the uploader.** This is existing behaviour: the mutations ignore the indexing result. Now the old data stays served while the download serves the new file, until a successful re-upload. Surfacing the failure would change the upload API's contract and is left for a decision. +- **TODO — a failed re-upload still reports success to the uploader.** This is existing behaviour: the upload mutations ignore the indexing result. The file is saved (the download serves it), but if its rows can't be loaded, previews, charts and the data API keep serving the previous file, until a successful re-upload. Nobody is told the two disagree. Direction (pending a talk with the product team, with the DataSpace team): check the file at upload against the database's limits and enforce them on the platform (for example, column names within the 255 characters a column row allows), and flag problem columns where people see column names and add column descriptions. A `TODO` comment marks the spot in `_create_file_resource_schema` (`api/schema/resource_schema.py`). - **TODO — two long headers that differ only after 63 bytes** become one column in Postgres, so the build fails and that file never gets a table (the upload still reports success; see the first item). Devanagari headers hit this at about 21 characters. This existed before and is not fixed yet. A fix would catch names that only clash once cut to 63 bytes during column clean-up, and either shorten them uniquely (keeping the stored column names in step) or refuse the file with a clear reason. A `TODO` comment marks the spot in `_clean_column_names` (`data_tables.py`). - **Access-model column selections are reset** whenever column rows are rebuilt (every re-upload, and migration rebuilds). Carrying them over by column name would be a small follow-up. - **Readers still use f-string table names.** That is safe for `rdt_` names; they were left as they are deliberately. From 0dc798bf09bbe5398ff65033339ae1b0aa4db4eb Mon Sep 17 00:00:00 2001 From: psaesha Date: Sat, 14 Nov 2026 15:54:00 +0530 Subject: [PATCH 7/7] ci(sdk): run the SDK tests on ubuntu-24.04 ubuntu-latest is now Ubuntu 26.04, where setup-python has no Python 3.9 build. The 3.9 job failed at setup and fail-fast cancelled 3.10 and 3.11 (seen on #295). The SDK still declares 3.9 support, so pin the runner instead of dropping it. --- .github/workflows/sdk-tests.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/sdk-tests.yml b/.github/workflows/sdk-tests.yml index 672eec48..b504bc21 100644 --- a/.github/workflows/sdk-tests.yml +++ b/.github/workflows/sdk-tests.yml @@ -8,7 +8,8 @@ on: jobs: test: - runs-on: ubuntu-latest + # Pinned: ubuntu-latest (26.04) has no Python 3.9 build, which the SDK still supports. + runs-on: ubuntu-24.04 strategy: matrix: python-version: ["3.9", "3.10", "3.11"]