From 0aed4880ff3136ec92f8a86c9e1d204a2be4e474 Mon Sep 17 00:00:00 2001 From: Amit Chaudhary Date: Tue, 13 Oct 2026 00:44:03 +0530 Subject: [PATCH 01/11] add network override for docker hotreload --- docker-compose.hotreload.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docker-compose.hotreload.yml b/docker-compose.hotreload.yml index d8fce17a..a25508f5 100644 --- a/docker-compose.hotreload.yml +++ b/docker-compose.hotreload.yml @@ -7,3 +7,6 @@ services: command: uvicorn DataSpace.asgi:application --host 0.0.0.0 --port 8000 --reload volumes: - .:/code + + networks: !override + - default \ No newline at end of file From 5a5bc26931811e3f34249ef45b673139deaa66a3 Mon Sep 17 00:00:00 2001 From: Amit Chaudhary Date: Tue, 13 Oct 2026 00:44:49 +0530 Subject: [PATCH 02/11] add AGENTS.md --- AGENTS.md | 178 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 178 insertions(+) create mode 100644 AGENTS.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 00000000..78f32026 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,178 @@ +# AGENTS.md + +You are a senior engineer on the DataSpace backend. + +## Persona +- You write clean, reusable, maintainable code that reads like the code around it. +- You understand the codebase and its security risks (authorization, multi-tenant organization data, file uploads, search exposure), and you turn that into clear, actionable insights. +- Your output: small, well-tested changes and reviews that a teammate can merge with confidence, plus a short explanation of *why* when the reason isn't obvious. + +## Commands +- **Run locally (Docker, hot reload):** `docker compose -f docker-compose.yml -f docker-compose.hotreload.yml up --build` +- **Run locally (venv):** copy `.env.example` to `.env`, then `python manage.py runserver` +- **Test:** `pytest tests/` uses SQLite in-memory with `--nomigrations` (see `pytest.ini`). It needs `pytest pytest-django`, which are not in `requirements.txt`. CI runs it with `--ignore=tests/object_types/charts --deselect tests/schema/test_dataset_schema.py::TestAddUpdateDatasetMetadata` (known-broken, see #189). +- **Single test:** `pytest tests/test_slug_collision.py -k usecase` +- **Format:** `black .` and `isort --profile black .` (line length 100, set in `pyproject.toml`; that file is the SDK's packaging config, but black reads it for the whole repo) +- **Type check:** `mypy --config-file=mypy.ini .` +- **All hooks:** `pre-commit run --all-files` (whitespace, black, isort, mypy, `debug-statements`) +- **Migrations:** `python manage.py makemigrations api`, then `python manage.py migrate` +- **Search mappings:** `python manage.py sync_search_mappings` after changing anything in `search/documents/` +- **API testing:** Postman CLI is installed; collections live in `postman/collections/` + +## Definition of done +Before you call a change finished: +1. `pre-commit run --all-files` is clean. +2. The tests pass with the CI flags above, and a bug fix has a regression test. +3. Every new or changed GraphQL query/mutation has the right permission guard (see Rules). +4. If a model changed: its search document, signals, cache invalidation and SDK still match. + +## Project knowledge + +**What it is:** DataSpace is CivicDataLab's platform for publishing and discovering datasets, use cases, AI models, collaboratives and publications. This repo is the backend: a GraphQL API (main surface) plus REST endpoints for search, downloads, auth and activity. It also ships a Python client, `dataspace_sdk`, published to PyPI. + +**Tech stack:** +- Python 3.11, Django 5.0, PostgreSQL +- GraphQL: `strawberry-graphql` 0.235 + `strawberry-graphql-django` 0.42 (endpoint `/api/graphql`) +- REST: Django REST Framework 3.14, `drf-yasg` for API docs +- Auth: Keycloak (`python-keycloak` 5.5); every request needs a real Keycloak JWT, with no dev-mode fallback +- Search: Elasticsearch 8 via `django-elasticsearch-dsl` +- Cache: Redis (`django-redis`, `django-cacheops`) +- Activity feed: `django-activity-stream` +- Observability: OpenTelemetry, `structlog` +- Data: pandas, pyarrow, DVC for dataset versioning +- Runtime: uvicorn/gunicorn in Docker, deployed to AWS (ECS / EC2 via `scripts/ci-*.sh`) + +**File structure:** +- `DataSpace/`: Django project (`settings.py`, `urls.py`, `asgi.py`, `cache_settings.py`) +- `api/`: main app + - `models/`: one model per file (`Dataset.py`, `Organization.py`, `UseCase.py`, …), exported from `models/__init__.py` + - `schema/`: Strawberry `Query`/`Mutation` classes per domain (`*_schema.py`), stitched together in `schema/schema.py`. `base_mutation.py` holds `BaseMutation`/`MutationResponse`. `extensions.py` holds `TrackActivity`/`TrackModelActivity`. + - `types/`: Strawberry output types (`type_*.py`) + - `views/`: REST views (search, downloads, charts, health, auth) + - `services/`: business logic kept out of resolvers (publication, platform import, metadata mapping/export, model clients) + - `activities/`: activity-stream handlers and display per domain (`dataset.py`, `organization.py`, …), wired up in `registry.py` + - `managers/`: `dvc_manager.py` for dataset versioning + - `signals/`: post-save hooks (search indexing, cache invalidation) + - `middleware/`, `utils/middleware.py`: request validation, rate limiting, structured logging, organization/dataspace context from headers + - `utils/`: shared helpers (`graphql_utils.py`, `keycloak_utils.py`, `search_query.py`, `search_cache.py`, `enums.py`, …) + - `management/commands/`: `sync_search_mappings`, `update_dataindex`, `seed_resource_types`, `populate_geographies`, … + - `migrations/`: numbered sequentially (`00NN_*`) +- `authorization/`: Keycloak integration, roles, `OrganizationMembership`, consent/activity middleware + - `permissions.py`: both the DRF permission classes and the Strawberry `*GraphQL` ones + - `schema/`: user/membership GraphQL schema; `schema/permissions.py` holds `HasOrganizationAdminRole` + - `graphql_permissions.py`: legacy duplicate of the GraphQL classes; don't add new uses +- `search/documents/`: Elasticsearch document definitions and analysers +- `dataspace_sdk/`: standalone Python SDK (its own version in `pyproject.toml`; depends only on `requests`) +- `tests/`: pytest suite (`test_*.py`, `tests/schema/`, `tests/journeys/`), settings in `tests/test_settings.py` +- `docs/`: feature docs (Keycloak, unified search, charts, activity stream, SDK) +- `postman/`: Postman collections and fixtures for API testing +- `aws/`, `scripts/`, `.github/workflows/`: infra, deploy scripts, CI +- Root-level `migrate_*.py`, `populate_sdgs.py`, `test_geography_hierarchy.py`: one-off scripts. Leave them alone, and put new code in `api/management/commands/` instead. + +## Standards + +**Naming conventions:** +- Functions, methods, variables: snake_case (`create_organization`, `get_current_user`) +- Classes: PascalCase (`OrganizationInput`, `HasOrganizationRole`) +- Constants: UPPER_SNAKE_CASE (`MAX_RETRIES`) +- Strawberry output types: `Type` in `api/types/type_.py` +- Model files: PascalCase, one model per file (`api/models/UseCase.py`) +- GraphQL fields are snake_case in Python. Strawberry exposes them as camelCase (`create_organization` → `createOrganization`). + +**Rules:** +- Type-annotate every function. mypy runs in pre-commit, so don't add new `# type: ignore` or `mypy.ini` exemptions without a reason in a comment. The existing `# type: ignore[list-item]` on `permission_classes` lists that mix in `HasOrganizationRoleGraphQL` subclasses is the accepted pattern. + - mypy skips `tests/` and is relaxed for `api.models`, `api.activities` and the resource, use case and dataset schemas (see `mypy.ini`). Still annotate new code there. +- Every GraphQL query and mutation that touches non-public data needs `permission_classes`. Don't rely on the ORM to reject anonymous users. + - Import from `authorization.permissions`: `IsAuthenticated`, `IsOrganizationMemberGraphQL`, `HasOrganizationRoleGraphQL`, or a domain class built on them (`CreateDatasetPermission`, `ChangePublicationPermission`, the `Change/DeleteOrganizationPermission` subclasses in `organization_schema.py`, …). + - The plain `IsOrganizationMember` and `HasOrganizationRole` in `authorization/permissions.py` are DRF classes for REST views. They don't work in Strawberry `permission_classes`. Schemas alias the GraphQL versions instead (`HasOrganizationRoleGraphQL as HasOrganizationRole`). + - `HasOrganizationRoleGraphQL` checks the organization from the request header (or an `organization_id` argument), not the object an `input.id` points at. When a mutation loads an object by id, also check the caller's membership in that object's organization inside the resolver. +- Scope querysets to what the user can see (organization membership, dataspace, published status). Superuser bypass goes through `is_superuser(info)` from `api/utils/graphql_utils.py`. +- Keep resolvers thin. Put reusable logic in `api/services/` or `api/utils/`, and reuse existing helpers before writing new ones. +- Wrap multi-step writes in `transaction.atomic()`. +- Return clean client errors (400/404, or `MutationResponse.error_response`) instead of letting `IntegrityError`, `ValueError` or `DoesNotExist` surface as a 500. +- Log with the existing logger and `structlog`, without tokens or PII. +- Comments explain *why* (constraints, past incidents), not *what*. Match the density of the surrounding file. +- Write regression tests in the style of `tests/test_slug_collision.py`: a module docstring with the root cause and the fix, then small focused tests. + +**Code style example:** +```python +from api.schema.extensions import TrackModelActivity +from api.utils.graphql_utils import is_superuser +from authorization.models import OrganizationMembership +from authorization.permissions import HasOrganizationRoleGraphQL as HasOrganizationRole +from authorization.permissions import IsAuthenticated + + +class ChangeOrganizationPermission(HasOrganizationRole): + def __init__(self) -> None: + super().__init__(operation="change") + + +# ✅ Good - permission guard, typed, scoped, atomic, clean error +@strawberry_django.mutation( + handle_django_errors=True, + permission_classes=[IsAuthenticated, ChangeOrganizationPermission], # type: ignore[list-item] + extensions=[ + TrackModelActivity( + verb="updated", + get_data=lambda result, **kwargs: { + "organization_name": result.name, + "organization_id": str(result.id), + }, + ) + ], +) +def update_organization(self, info: Info, input: OrganizationInputPartial) -> TypeOrganization: + """Update an organization the caller administers.""" + try: + organization = Organization.objects.get(id=input.id) + except Organization.DoesNotExist: + raise ValueError(f"Organization with ID {input.id} does not exist.") + + # The permission class checks the organization in the request header, not input.id + is_admin = OrganizationMembership.objects.filter( + user=info.context.user, organization=organization, role__name="admin" + ).exists() + if not is_superuser(info) and not is_admin: + raise PermissionError("You don't have permission to update this organization.") + + with transaction.atomic(): + for field, value in vars(input).items(): + if field != "id" and value is not strawberry.UNSET: + setattr(organization, field, value) + organization.save() + return TypeOrganization.from_django(organization) + + +# ❌ Bad - no auth guard, untyped, unscoped lookup, raw 500 on a missing id +@strawberry_django.mutation +def update_org(self, info, input): + org = Organization.objects.get(id=input.id) + for k, v in vars(input).items(): + setattr(org, k, v) + org.save() + return org +``` + +## Git & PRs +- Branch from `dev` with `fix/…`, `feat/…` or `chore/…`. PRs go into `dev`. Releases go from `dev` to `main`, gated by `pr-gate.yml`. +- Do not make any commits utill asked for it. +- Use Conventional Commits with a scope: `fix(api): …`, `feat(usecase): …`. +- When a merge brings in migrations with clashing numbers, renumber yours after the latest one on `dev` and fix the `dependencies`. + +## Boundaries +- ✅ **Always:** write code in `api/`, `authorization/`, `search/`, `dataspace_sdk/` and `tests/`. Run `pytest` and `pre-commit` before committing. +- ⚠️ **Ask first:** + - model or schema changes and new migrations + - changes to permission classes, Keycloak/auth middleware or rate limits + - changes to Elasticsearch mappings (these need a reindex) + - adding or upgrading dependencies + - bumping the SDK version or publishing it + - touching `.github/workflows/`, `aws/`, `scripts/ci-*.sh`, `Dockerfile` or `docker-compose.yml` + - anything that runs against a deployed environment +- 🚫 **Never:** + - commit `.env`, secrets, Keycloak client secrets or tokens. Use `.env.example` for new variables. + - edit or delete already-merged migrations. Add a new migration instead. + - disable an auth check or a failing test to get green. Fix the cause, or ask. + - edit `.venv/`, `files/` (user uploads) or `logs/` + - force-push to `dev` or `main` From e4c52db4d859a2ec67bd865da410ec1ee88d0cd9 Mon Sep 17 00:00:00 2001 From: Amit Chaudhary Date: Mon, 19 Oct 2026 12:10:37 +0530 Subject: [PATCH 03/11] refactor(api)!: remove DVC dataset versioning DVC never worked: uploaded files were never under DVC_REPO_PATH, so every DVC call raised and createMajorVersion always failed. Remove the manager, the setup command, version-change detection and the post_save signal. createMajorVersion now writes the ResourceVersion row directly inside transaction.atomic(), behind IsAuthenticated plus a check that the caller owns the dataset or may change it. BREAKING CHANGE: the setup_dvc management command and DVC_* settings are gone. --- DataSpace/settings.py | 5 - .../commands/backfill_file_hashes.py | 2 +- api/management/commands/setup_dvc.py | 131 ------- api/managers/dvc_manager.py | 132 ------- api/models/Resource.py | 162 +-------- api/schema/resource_schema.py | 63 ++-- api/signals/dataset_signals.py | 22 +- api/utils/version_detection.py | 327 ------------------ docs/version_detection_system.md | 140 -------- requirements.txt | 1 - 10 files changed, 28 insertions(+), 957 deletions(-) delete mode 100644 api/management/commands/setup_dvc.py delete mode 100644 api/managers/dvc_manager.py delete mode 100644 api/utils/version_detection.py delete mode 100644 docs/version_detection_system.md diff --git a/DataSpace/settings.py b/DataSpace/settings.py index d390827e..63452733 100644 --- a/DataSpace/settings.py +++ b/DataSpace/settings.py @@ -308,11 +308,6 @@ PUBLIC_SITE_URL = os.getenv("PUBLIC_SITE_URL", "https://civicdataspace.in") PUBLIC_API_URL = os.getenv("PUBLIC_API_URL", "https://api.datakeep.civicdays.in") -# DVC settings -DVC_REPO_PATH = os.path.join(BASE_DIR, "dvc") -DVC_REMOTE_NAME = os.getenv("DVC_REMOTE_NAME", None) -DVC_REMOTE_URL = os.getenv("DVC_REMOTE_URL", None) - # Django REST Framework settings REST_FRAMEWORK = { "DEFAULT_PERMISSION_CLASSES": [ diff --git a/api/management/commands/backfill_file_hashes.py b/api/management/commands/backfill_file_hashes.py index 139af5dd..a97e5172 100644 --- a/api/management/commands/backfill_file_hashes.py +++ b/api/management/commands/backfill_file_hashes.py @@ -3,7 +3,7 @@ python manage.py backfill_file_hashes # fill every missing hash python manage.py backfill_file_hashes --dry-run # only count -Writes through a queryset update, so the DVC versioning signal does not run. +Writes through a queryset update, so no model save signals run. """ from django.core.management.base import BaseCommand diff --git a/api/management/commands/setup_dvc.py b/api/management/commands/setup_dvc.py deleted file mode 100644 index 22808721..00000000 --- a/api/management/commands/setup_dvc.py +++ /dev/null @@ -1,131 +0,0 @@ -import os -import subprocess -from typing import Any, Optional, cast - -import structlog -from django.conf import settings -from django.core.management.base import BaseCommand - -logger = structlog.getLogger(__name__) - - -class Command(BaseCommand): - help = "Set up DVC repository for dataset versioning" - - def handle(self, *args: Any, **options: Any) -> None: - try: - repo_path = settings.DVC_REPO_PATH - - # Create directory if needed - if not os.path.exists(repo_path): - os.makedirs(repo_path) - self.stdout.write(f"Created DVC repository directory at {repo_path}") - - # Initialize Git and DVC if not already done - if not os.path.exists(os.path.join(repo_path, ".git")): - subprocess.run(["git", "init"], cwd=repo_path, check=True) - self.stdout.write("Initialized Git repository") - - if not os.path.exists(os.path.join(repo_path, ".dvc")): - subprocess.run(["dvc", "init"], cwd=repo_path, check=True) - self.stdout.write("Initialized DVC repository") - - # Configure chunking for large files - subprocess.run( - ["dvc", "config", "cache.type", "hardlink,symlink"], - cwd=repo_path, - check=True, - ) - subprocess.run( - ["dvc", "config", "cache.shared", "group"], - cwd=repo_path, - check=True, - ) - - # Configure cache size limits to prevent excessive disk usage - subprocess.run( - ["dvc", "config", "cache.size_limit", "10G"], - cwd=repo_path, - check=True, - ) - - self.stdout.write("Configured DVC for large file handling") - - # Set up remote if configured - if ( - hasattr(settings, "DVC_REMOTE_NAME") - and hasattr(settings, "DVC_REMOTE_URL") - and settings.DVC_REMOTE_NAME - and settings.DVC_REMOTE_URL - ): - # Cast to str to satisfy mypy - remote_name = cast(str, settings.DVC_REMOTE_NAME) - remote_url = cast(str, settings.DVC_REMOTE_URL) - - subprocess.run( - [ - "dvc", - "remote", - "add", - remote_name, - remote_url, - ], - cwd=repo_path, - check=True, - ) - - # Set as default remote - subprocess.run( - ["dvc", "remote", "default", remote_name], - cwd=repo_path, - check=True, - ) - - self.stdout.write(f"Configured DVC remote: {remote_name}") - - # Set up Git user if not already configured - try: - # Check if Git user is configured - result = subprocess.run( - ["git", "config", "user.name"], - cwd=repo_path, - capture_output=True, - text=True, - ) - if not result.stdout.strip(): - subprocess.run( - ["git", "config", "user.name", "DataEx Bot"], - cwd=repo_path, - check=True, - ) - subprocess.run( - ["git", "config", "user.email", "dataex@example.com"], - cwd=repo_path, - check=True, - ) - self.stdout.write("Configured Git user for DVC repository") - except subprocess.CalledProcessError: - # If checking fails, set the user anyway - subprocess.run( - ["git", "config", "user.name", "DataEx Bot"], - cwd=repo_path, - check=True, - ) - subprocess.run( - ["git", "config", "user.email", "dataex@example.com"], - cwd=repo_path, - check=True, - ) - self.stdout.write("Configured Git user for DVC repository") - - self.stdout.write(self.style.SUCCESS("DVC repository set up successfully")) - - except Exception as e: - import traceback - - logger.error( - f"Failed to set up DVC repository: {str(e)}, Traceback: {traceback.format_exc()}" - ) - self.stdout.write( - self.style.ERROR(f"Failed to set up DVC repository: {str(e)}") - ) diff --git a/api/managers/dvc_manager.py b/api/managers/dvc_manager.py deleted file mode 100644 index e9c98195..00000000 --- a/api/managers/dvc_manager.py +++ /dev/null @@ -1,132 +0,0 @@ -import subprocess -from pathlib import Path -from typing import List, Union - -import structlog - -logger = structlog.getLogger(__name__) - - -class DVCManager: - def __init__(self, repo_path: Union[str, Path]) -> None: - self.repo_path = Path(repo_path) - - def _run_command(self, command: List[str]) -> subprocess.CompletedProcess: - try: - return subprocess.run( - command, - cwd=str(self.repo_path), - capture_output=True, - text=True, - check=True, - ) - except subprocess.CalledProcessError as e: - # Log the error with details - logger.error(f"Command failed: {' '.join(command)}, Error: {e.stderr}") - # You can choose to re-raise or handle differently - raise - - def track_resource(self, file_path: Union[str, Path], chunked: bool = False) -> str: - """Add a resource file to DVC tracking with optional chunking for large files""" - rel_path = Path(file_path).relative_to(self.repo_path) - cmd = ["dvc", "add"] - if chunked: - cmd.append("--chunked") # Enables chunking for large files - cmd.append(str(rel_path)) - self._run_command(cmd) - return str(rel_path) + ".dvc" - - def commit_version(self, dvc_file: str, message: str) -> None: - """Commit the DVC file to git""" - self._run_command(["git", "add", dvc_file]) - self._run_command(["git", "commit", "-m", message]) - - def tag_version(self, tag_name: str) -> None: - """Add a git tag for this version""" - self._run_command(["git", "tag", tag_name]) - - def push_to_remote(self) -> None: - """Push data to DVC remote and metadata to git""" - self._run_command(["dvc", "push"]) - self._run_command(["git", "push", "--follow-tags"]) - - def get_version(self) -> str: - """Get the current version from DVC""" - result = self._run_command(["dvc", "version"]) - return str(result.stdout.strip()) - - def get_remote(self) -> str: - """Get the current remote from DVC""" - result = self._run_command(["dvc", "remote", "list"]) - return str(result.stdout.strip()) - - def setup_remote(self, remote_name: str, remote_url: str) -> None: - """Configure a DVC remote for storing data""" - self._run_command(["dvc", "remote", "add", remote_name, remote_url]) - self._run_command(["dvc", "remote", "default", remote_name]) - - def verify_file(self, file_path: Union[str, Path]) -> bool: - """Verify file integrity using DVC checksums""" - rel_path = Path(file_path).relative_to(self.repo_path) - try: - result = self._run_command(["dvc", "status", str(rel_path)]) - return "up to date" in str(result.stdout) - except subprocess.CalledProcessError: - return False - - def rollback_to_version( - self, file_path: Union[str, Path], version_tag: str - ) -> None: - """Roll back a file to a specific version using DVC and Git""" - rel_path = Path(file_path).relative_to(self.repo_path) - dvc_file = str(rel_path) + ".dvc" - - # Checkout the specific version of the DVC file - self._run_command(["git", "checkout", version_tag, "--", dvc_file]) - - # Pull the data for that version - self._run_command(["dvc", "checkout", str(rel_path)]) - - def lock_resource(self, file_path: Union[str, Path]) -> None: - """Lock a resource to prevent concurrent modifications""" - rel_path = Path(file_path).relative_to(self.repo_path) - self._run_command(["dvc", "lock", str(rel_path)]) - - def unlock_resource(self, file_path: Union[str, Path]) -> None: - """Unlock a previously locked resource""" - rel_path = Path(file_path).relative_to(self.repo_path) - self._run_command(["dvc", "unlock", str(rel_path)]) - - def add_metric(self, metric_file: Union[str, Path], metric_name: str) -> None: - """Add a file as a DVC metric for tracking""" - rel_path = Path(metric_file).relative_to(self.repo_path) - self._run_command( - ["dvc", "metrics", "add", str(rel_path), "--name", metric_name] - ) - - def show_metrics(self) -> str: - """Show all tracked metrics""" - result = self._run_command(["dvc", "metrics", "show"]) - return str(result.stdout.strip()) - - def gc_cache(self, force: bool = False) -> None: - """Clean up unused cache to save disk space""" - cmd = ["dvc", "gc"] - if force: - cmd.append("-f") - self._run_command(cmd) - - def configure(self, section: str, option: str, value: str) -> None: - """Configure DVC settings""" - self._run_command(["dvc", "config", f"{section}.{option}", value]) - - def has_changes(self, file_path: Union[str, Path]) -> bool: - """Check if a file has uncommitted changes""" - rel_path = Path(file_path).relative_to(self.repo_path) - try: - result = self._run_command(["dvc", "status", str(rel_path)]) - # If there are changes, the output will contain the file path - return str(rel_path) in str(result.stdout) - except subprocess.CalledProcessError: - # If the command fails, assume there are changes - return True diff --git a/api/models/Resource.py b/api/models/Resource.py index 2af8d0a9..c9857edd 100644 --- a/api/models/Resource.py +++ b/api/models/Resource.py @@ -1,23 +1,17 @@ import hashlib -import os -import random import uuid -from pathlib import Path from typing import TYPE_CHECKING, Any, Optional import structlog -from django.conf import settings from django.contrib.auth import get_user_model from django.db import models -from django.db.models.signals import post_save, pre_save +from django.db.models.signals import pre_save from django.dispatch import receiver from django.utils.text import slugify logger = structlog.getLogger(__name__) -from api.managers.dvc_manager import DVCManager from api.utils.enums import DataType -from api.utils.version_detection import detect_version_change_type if TYPE_CHECKING: from api.models.Dataset import Dataset @@ -168,160 +162,6 @@ def hash_resource_file(sender, instance: ResourceFileDetails, **kwargs): instance.sha256 = compute_sha256(instance.file) -@receiver(post_save, sender=ResourceFileDetails) -def version_resource_with_dvc(sender, instance: ResourceFileDetails, created, **kwargs): - """Create a new version using DVC when resource is updated""" - # Initialize DVC manager - dvc = DVCManager(settings.DVC_REPO_PATH) - - # Skip if this is just being created (first version) - if created: - # For first version, we just track it - try: - # Use chunked mode for large files (over 100MB) - file_size = ( - instance.file.size - if hasattr(instance.file, "size") - else os.path.getsize(instance.file.path) - ) - use_chunked = file_size > 100 * 1024 * 1024 # 100MB threshold - - dvc_file = dvc.track_resource(instance.file.path, chunked=use_chunked) - message = f"Add resource: {instance.resource.name} version {instance.resource.version}" - dvc.commit_version(dvc_file, message) - dvc.tag_version(f"{instance.resource.name}-{instance.resource.version}") - - # Create first version record - ResourceVersion.objects.create( - resource=instance.resource, - version_number=instance.resource.version, - change_description=f"Initial version of {instance.resource.name}", - ) - except Exception as e: - logger.error(f"Failed to version resource: {str(e)}") - # Continue without versioning if it fails - # This allows the resource to be created even if DVC fails - pass - else: - # For updates, check if the file has actually changed before creating a new version - try: - # Skip versioning if the file hasn't changed - if dvc.verify_file(instance.file.path): - logger.info( - f"No changes detected for {instance.resource.name}, skipping version creation" - ) - return - - # Get the latest version - last_version: Optional[ResourceVersion] = instance.resource.versions.order_by( - "-created_at" - ).first() - - # Handle case when there are no versions yet - if last_version is None: - new_version = "v1.0.0" - else: - # Determine appropriate version increment type by analyzing changes - try: - # Get the previous version file path - # We need to create a temporary copy of the previous version - import shutil - import tempfile - - from django.core.files.storage import default_storage - - # Create a temporary directory for the previous version - with tempfile.TemporaryDirectory() as temp_dir: - # Get the previous version file path - file_name = instance.file.name - if not file_name: - raise ValueError("File name is missing") - prev_file_name = ( - f"prev_version_{instance.resource.id}.{file_name.split('.')[-1]}" - ) - prev_file_path = os.path.join(temp_dir, prev_file_name) - - # Use DVC to get the previous version - try: - # Try to checkout the previous version using DVC - rel_path = Path(instance.file.path).relative_to(settings.DVC_REPO_PATH) - tag_name = f"{instance.resource.name}-{last_version.version_number}" - - # Save current file to temp location - current_file_path = instance.file.path - temp_current = os.path.join(temp_dir, "current_file") - shutil.copy2(current_file_path, temp_current) - - # Checkout previous version - dvc.rollback_to_version(instance.file.path, tag_name) - - # Copy the previous version to our temp location - shutil.copy2(instance.file.path, prev_file_path) - - # Restore current file - shutil.copy2(temp_current, current_file_path) - - # Detect version change type - increment_type = detect_version_change_type( - prev_file_path, current_file_path - ) - logger.info( - f"Detected version change type: {increment_type} for {instance.resource.name}" - ) - except Exception as e: - logger.warning( - f"Could not analyze version changes: {str(e)}, defaulting to minor version" - ) - increment_type = "minor" - except Exception as e: - logger.warning( - f"Error in version detection: {str(e)}, defaulting to minor version" - ) - increment_type = "minor" - - # Increment version based on detected change type - new_version = _increment_version( - last_version.version_number, increment_type=increment_type - ) - - # Use chunked mode for large files (over 100MB) - file_size = ( - instance.file.size - if hasattr(instance.file, "size") - else os.path.getsize(instance.file.path) - ) - use_chunked = file_size > 100 * 1024 * 1024 # 100MB threshold - - # Update using DVC - dvc_file = dvc.track_resource(instance.file.path, chunked=use_chunked) - message = f"Update resource: {instance.resource.name} to version {new_version}" - dvc.commit_version(dvc_file, message) - dvc.tag_version(f"{instance.resource.name}-{new_version}") - - # Create version record - ResourceVersion.objects.create( - resource=instance.resource, - version_number=new_version, - change_description=f"Updated version of {instance.resource.name}", - ) - - # Update resource version field - instance.resource.version = new_version - instance.resource.save(update_fields=["version"]) - - # Optional: Trigger garbage collection periodically - # This could be moved to a scheduled task instead - if random.random() < 0.05: # 5% chance to run GC on version update - try: - dvc.gc_cache() - except Exception as e: - logger.warning(f"Failed to run garbage collection: {str(e)}") - except Exception as e: - logger.error(f"Failed to update resource version: {str(e)}") - # Continue without versioning if it fails - pass - - def _increment_version(version: str, increment_type: str = "minor") -> str: """Semantic version incrementing logic diff --git a/api/schema/resource_schema.py b/api/schema/resource_schema.py index 85fbc965..bb3c6d2a 100644 --- a/api/schema/resource_schema.py +++ b/api/schema/resource_schema.py @@ -9,11 +9,11 @@ import strawberry import strawberry_django import structlog +from django.db import transaction from django.db.models import QuerySet from strawberry.file_uploads import Upload from strawberry.types import Info -from api.managers.dvc_manager import DVCManager from api.models import ( Dataset, Resource, @@ -29,6 +29,9 @@ from api.utils.data_indexing import index_resource_data from api.utils.file_utils import file_validation from api.utils.graphql_telemetry import trace_resolver +from api.utils.graphql_utils import is_superuser +from authorization.permissions import IsAuthenticated +from authorization.services import AuthorizationService logger = structlog.get_logger("dataspace.resource_schema") @@ -393,6 +396,7 @@ def delete_file_resource(self, info: Info, resource_id: uuid.UUID) -> bool: @strawberry_django.mutation( handle_django_errors=True, + permission_classes=[IsAuthenticated], extensions=[ TrackModelActivity( verb="versioned", @@ -412,58 +416,37 @@ def create_major_version(self, info: Info, input: CreateMajorVersionInput) -> Ty This should be used when significant changes are made to the resource data structure, such as schema changes, column additions/removals, or other breaking changes. """ - import os - - from django.conf import settings - try: - # Get the resource - resource = Resource.objects.get(id=input.resource_id) + resource = Resource.objects.select_related("dataset").get(id=input.resource_id) except Resource.DoesNotExist: raise ValueError(f"Resource with ID {input.resource_id} does not exist") - # Get the latest version - last_version = resource.versions.order_by("-created_at").first() + # Only the dataset's owner or someone who may change it can version its resources. + user = info.context.user + dataset = resource.dataset + can_change = dataset.user_id == user.id or AuthorizationService.check_dataset_permission( + user.id, str(dataset.id), "change" + ) + if not is_superuser(info) and not can_change: + raise PermissionError("You don't have permission to version this resource.") + last_version = resource.versions.order_by("-created_at").first() if not last_version: logger.warning( f"No previous version found for resource {resource.name}, creating initial version" ) new_version = "v1.0.0" else: - # Increment major version new_version = _increment_version(last_version.version_number, increment_type="major") - # Initialize DVC manager - dvc = DVCManager(settings.DVC_REPO_PATH) - - # Get the resource file path - file_path = resource.resourcefiledetails.file.path - - # Determine if file is large and should use chunking - file_size = ( - resource.resourcefiledetails.file.size - if hasattr(resource.resourcefiledetails.file, "size") - else os.path.getsize(file_path) - ) - use_chunked = file_size > 100 * 1024 * 1024 # 100MB threshold - - # Track with DVC - dvc_file = dvc.track_resource(file_path, chunked=use_chunked) - message = f"Major version update for resource: {resource.name} to version {new_version}" - dvc.commit_version(dvc_file, message) - dvc.tag_version(f"{resource.name}-{new_version}") - - # Create version record - ResourceVersion.objects.create( - resource=resource, - version_number=new_version, - change_description=input.description, - ) - - # Update resource version field - resource.version = new_version - resource.save(update_fields=["version"]) + with transaction.atomic(): + ResourceVersion.objects.create( + resource=resource, + version_number=new_version, + change_description=input.description, + ) + resource.version = new_version + resource.save(update_fields=["version"]) logger.info(f"Created major version {new_version} for resource {resource.name}") return TypeResource.from_django(resource) diff --git a/api/signals/dataset_signals.py b/api/signals/dataset_signals.py index 3d4ac987..6f9f88ee 100644 --- a/api/signals/dataset_signals.py +++ b/api/signals/dataset_signals.py @@ -1,11 +1,9 @@ from typing import Any, Optional import structlog -from django.conf import settings from django.db.models.signals import pre_save from django.dispatch import receiver -from api.managers.dvc_manager import DVCManager from api.models.Dataset import Dataset from api.models.Resource import Resource, ResourceVersion, _increment_version from api.utils.enums import DatasetStatus @@ -77,16 +75,13 @@ def handle_dataset_publication(sender: Any, instance: Dataset, **kwargs: Any) -> # Get all resources for this dataset resources = instance.resources.all() - # Initialize DVC manager - dvc = DVCManager(settings.DVC_REPO_PATH) - # Increment version for each resource for resource in resources: # type: Resource try: # Get the latest version - last_version: Optional[ResourceVersion] = ( - resource.versions.order_by("-created_at").first() - ) + last_version: Optional[ResourceVersion] = resource.versions.order_by( + "-created_at" + ).first() if last_version: # Increment minor version (publication is a significant change) @@ -94,17 +89,6 @@ def handle_dataset_publication(sender: Any, instance: Dataset, **kwargs: Any) -> last_version.version_number, increment_type="minor" ) - # Get the resource file path - file_path: str = resource.resourcefiledetails.file.path - - # Track with DVC - dvc_file = dvc.track_resource(file_path) - message: str = ( - f"Publishing resource: {resource.name} to version {new_version}" - ) - dvc.commit_version(dvc_file, message) - dvc.tag_version(f"{resource.name}-{new_version}") - # Create version record ResourceVersion.objects.create( resource=resource, diff --git a/api/utils/version_detection.py b/api/utils/version_detection.py deleted file mode 100644 index 809e9db6..00000000 --- a/api/utils/version_detection.py +++ /dev/null @@ -1,327 +0,0 @@ -import json -import os -import xml.etree.ElementTree as ET -from typing import Any, Dict, List, Set, Tuple, Union - -import pandas as pd -import structlog -from deepdiff import DeepDiff # type: ignore - -logger = structlog.getLogger(__name__) - - -def detect_version_change_type(old_file_path: str, new_file_path: str) -> str: - """ - Analyze changes between two versions of a file and determine the appropriate - version increment type (major, minor, or patch). - - Args: - old_file_path: Path to the previous version of the file - new_file_path: Path to the new version of the file - - Returns: - String indicating the type of version change: "major", "minor", or "patch" - """ - # Get file extensions - _, old_ext = os.path.splitext(old_file_path) - _, new_ext = os.path.splitext(new_file_path) - - # If file format changed, it's a major change - if old_ext.lower() != new_ext.lower(): - logger.info( - f"File format changed from {old_ext} to {new_ext} - major version change" - ) - return "major" - - # Handle different file types - if old_ext.lower() in [".csv", ".tsv"]: - return _detect_tabular_changes(old_file_path, new_file_path) - elif old_ext.lower() in [".json"]: - return _detect_json_changes(old_file_path, new_file_path) - elif old_ext.lower() in [".xml"]: - return _detect_xml_changes(old_file_path, new_file_path) - else: - # For other file types, default to comparing file size - return _detect_generic_changes(old_file_path, new_file_path) - - -def _detect_tabular_changes(old_file_path: str, new_file_path: str) -> str: - """ - Detect changes in tabular data files (CSV, TSV) using pandas - """ - try: - # Read files with pandas - old_df = pd.read_csv(old_file_path, encoding="utf-8") - new_df = pd.read_csv(new_file_path, encoding="utf-8") - - # Check for schema changes - old_columns = set(old_df.columns) - new_columns = set(new_df.columns) - - # If columns were added or removed, it's a major change - if old_columns != new_columns: - added = new_columns - old_columns - removed = old_columns - new_columns - logger.info( - f"Schema changed - added columns: {added}, removed columns: {removed} - major version change" - ) - return "major" - - # Check for data type changes in columns - for col in old_columns: - if old_df[col].dtype != new_df[col].dtype: - logger.info( - f"Data type changed for column {col} - major version change" - ) - return "major" - - # Check for significant row count changes - if len(old_df) != len(new_df): - # Row count changed by more than 10% - change_percentage = ( - abs(len(new_df) - len(old_df)) / max(len(old_df), 1) * 100 - ) - if change_percentage > 10: - logger.info( - f"Row count changed by {change_percentage:.2f}% - minor version change" - ) - return "minor" - - # Use pandas built-in comparison for data changes - # Limit to common rows to avoid index errors - min_rows = min(len(old_df), len(new_df)) - if min_rows > 0: - # Truncate both dataframes to the same length - old_sample = old_df.iloc[:min_rows].reset_index(drop=True) - new_sample = new_df.iloc[:min_rows].reset_index(drop=True) - - # Use pandas compare function (available in pandas >= 1.1.0) - try: - # Get differences between dataframes - diff_df = new_sample.compare(old_sample) - - # Calculate change percentage - total_cells = min_rows * len(old_columns) - changed_cells = diff_df.size # Number of changed cells - - change_percentage = ( - (changed_cells / total_cells) * 100 if total_cells > 0 else 0 - ) - - if change_percentage > 30: - logger.info( - f"Data changed significantly ({change_percentage:.2f}% of cells) - minor version change" - ) - return "minor" - elif change_percentage > 0: - logger.info( - f"Small data changes ({change_percentage:.2f}% of cells) - patch version change" - ) - return "patch" - except Exception as compare_error: - # Fallback if compare fails (older pandas version or other issues) - logger.warning( - f"Pandas compare failed: {str(compare_error)}, using manual comparison" - ) - - # Manual comparison as fallback - changes = 0 - total_cells = min_rows * len(old_columns) - - for col in old_columns: - # Compare column values - unequal_mask = old_sample[col].values != new_sample[col].values - changes += unequal_mask.sum() # type: ignore - - change_percentage = ( - (changes / total_cells) * 100 if total_cells > 0 else 0 - ) - - if change_percentage > 30: - logger.info( - f"Data changed significantly ({change_percentage:.2f}% of cells) - minor version change" - ) - return "minor" - elif change_percentage > 0: - logger.info( - f"Small data changes ({change_percentage:.2f}% of cells) - patch version change" - ) - return "patch" - - # Default to patch for minimal changes - return "patch" - - except Exception as e: - logger.error(f"Error analyzing tabular files: {str(e)}") - # Default to minor if we can't analyze properly - return "minor" - - -def _detect_json_changes(old_file_path: str, new_file_path: str) -> str: - """ - Detect changes in JSON files using DeepDiff - """ - try: - with open(old_file_path, "r", encoding="utf-8") as f: - old_data = json.load(f) - - with open(new_file_path, "r", encoding="utf-8") as f: - new_data = json.load(f) - - # Use DeepDiff for efficient and accurate comparison - diff = DeepDiff(old_data, new_data, ignore_order=True) - - # Check for structure changes (major) - if diff.get("dictionary_item_added") or diff.get("dictionary_item_removed"): - logger.info(f"JSON structure changed - major version change") - return "major" - - # Check for type changes (major) - if diff.get("type_changes"): - logger.info(f"JSON data types changed - major version change") - return "major" - - # Check for significant value changes (minor) - value_changes = len(diff.get("values_changed", {})) - if isinstance(old_data, dict) and isinstance(new_data, dict): - total_values = _count_values(old_data) - change_percentage = ( - (value_changes / total_values) * 100 if total_values > 0 else 0 - ) - - if change_percentage > 30: - logger.info( - f"JSON values changed significantly ({change_percentage:.2f}%) - minor version change" - ) - return "minor" - - # Check for array changes (minor) - if diff.get("iterable_item_added") or diff.get("iterable_item_removed"): - logger.info(f"JSON array items changed - minor version change") - return "minor" - - # If there are any changes at all, it's at least a patch - if diff: - return "patch" - - # No changes detected - return "patch" - - except Exception as e: - logger.error(f"Error analyzing JSON files: {str(e)}") - return "minor" - - -def _detect_xml_changes(old_file_path: str, new_file_path: str) -> str: - """ - Detect changes in XML files - """ - try: - import xml.etree.ElementTree as ET - - old_tree = ET.parse(old_file_path) - new_tree = ET.parse(new_file_path) - - old_root = old_tree.getroot() - new_root = new_tree.getroot() - - # Check for tag changes - old_tags = _get_xml_tags(old_root) - new_tags = _get_xml_tags(new_root) - - if old_tags != new_tags: - logger.info(f"XML structure changed - major version change") - return "major" - - # Check for attribute changes - old_attrs = _get_xml_attributes(old_root) - new_attrs = _get_xml_attributes(new_root) - - if old_attrs != new_attrs: - logger.info(f"XML attributes changed - minor version change") - return "minor" - - # Default to patch for content changes - return "patch" - - except Exception as e: - logger.error(f"Error analyzing XML files: {str(e)}") - return "minor" - - -def _detect_generic_changes(old_file_path: str, new_file_path: str) -> str: - """ - Detect changes in generic files based on size - """ - old_size = os.path.getsize(old_file_path) - new_size = os.path.getsize(new_file_path) - - # Calculate size change percentage - size_change_percentage = abs(new_size - old_size) / max(old_size, 1) * 100 - - if size_change_percentage > 50: - logger.info( - f"File size changed by {size_change_percentage:.2f}% - major version change" - ) - return "major" - elif size_change_percentage > 10: - logger.info( - f"File size changed by {size_change_percentage:.2f}% - minor version change" - ) - return "minor" - else: - logger.info( - f"File size changed by {size_change_percentage:.2f}% - patch version change" - ) - return "patch" - - -def _flatten_dict_keys(d: Dict, parent_key: str = "") -> List[str]: - """ - Flatten nested dictionary keys - """ - items = [] - for k, v in d.items(): - new_key = f"{parent_key}.{k}" if parent_key else k - if isinstance(v, dict): - items.extend(_flatten_dict_keys(v, new_key)) - else: - items.append(new_key) - return items - - -def _count_values(obj: Union[Dict, List, Any], count: int = 0) -> int: - """ - Count the total number of values in a nested structure - """ - if isinstance(obj, dict): - for k, v in obj.items(): - count += 1 # Count the key-value pair itself - if isinstance(v, (dict, list)): - count = _count_values(v, count) - elif isinstance(obj, list): - count += len(obj) # Count the list items - for item in obj: - if isinstance(item, (dict, list)): - count = _count_values(item, count) - return count - - -def _get_xml_tags(element: ET.Element) -> Set[str]: - """ - Get all unique tag names in an XML tree - """ - tags = {element.tag} - for child in element: - tags.update(_get_xml_tags(child)) - return tags - - -def _get_xml_attributes(element: ET.Element) -> Set[Tuple[str, str]]: - """ - Get all attributes in an XML tree as (element_tag, attribute_name) pairs - """ - attrs = {(element.tag, attr) for attr in element.attrib.keys()} - for child in element: - attrs.update(_get_xml_attributes(child)) - return attrs diff --git a/docs/version_detection_system.md b/docs/version_detection_system.md deleted file mode 100644 index 66fbd99f..00000000 --- a/docs/version_detection_system.md +++ /dev/null @@ -1,140 +0,0 @@ -# Intelligent Version Detection System - -## Overview - -The DataSpace platform implements an version detection system that automatically determines the appropriate version increment (major, minor, or patch) when resources are updated. This document explains the technical implementation, triggering mechanisms, and version classification logic. - -## Version Increment Types - -The system follows semantic versioning principles (X.Y.Z): - -1. **Major Version (X)**: Breaking changes or significant structural modifications -2. **Minor Version (Y)**: Publishing Dataset or significant data changes that maintain compatibility -3. **Patch Version (Z)**: Small fixes, corrections, or minimal data changes - -## Triggering Mechanisms - -Version changes are automatically triggered by the following events: - -1. **Resource File Updates**: When a resource file is updated through the API -2. **Dataset Publication**: When a dataset containing resources is published -3. **Manual Triggers**: Through management commands (e.g., `create_major_version`) - -## Technical Implementation - -### Components - -1. **Signal Handlers**: Detect changes to resources and trigger version detection -2. **Version Detection Utility**: Analyzes file changes to determine version increment type -3. **DVC Manager**: Handles version tracking, tagging, and remote storage - -### Signal Flow - -``` -ResourceFileDetails update → post_save signal → version_resource_with_dvc - → detect_version_change_type → _increment_version → Create ResourceVersion -``` - -## Change Detection Logic - -The system uses different strategies based on file type: - -### CSV/Tabular Files - -**Major Version** triggers: -- Schema changes (columns added/removed) -- Data type changes in columns - -**Minor Version** triggers: -- Row count changes > 10% -- Data changes in > 30% of cells - -**Patch Version** triggers: -- Small data changes (< 30% of cells) -- Minimal corrections - -### JSON Files - -**Major Version** triggers: -- Structure changes (keys added/removed) -- Data type changes - -**Minor Version** triggers: -- Significant value changes (> 30% of values) -- Array item additions/removals - -**Patch Version** triggers: -- Small value changes -- Formatting changes - -### XML Files - -**Major Version** triggers: -- Tag structure changes - -**Minor Version** triggers: -- Attribute changes - -**Patch Version** triggers: -- Content changes without structural modifications - -### Generic Files - -For non-structured files, the system uses file size differences: - -**Major Version** triggers: -- Size changes > 50% - -**Minor Version** triggers: -- Size changes between 10% and 50% - -**Patch Version** triggers: -- Size changes < 10% - -## Technical Details - -### Dependencies - -- **pandas**: For efficient tabular data comparison -- **DeepDiff**: For accurate JSON structure comparison -- **DVC**: For version tracking and storage - -### Performance Considerations - -- Large files (>100MB) use chunked processing -- Tabular comparisons use sampling for very large datasets -- Early return logic to avoid unnecessary processing - -## Example - -When a CSV resource is updated: - -1. The system detects the change through a Django signal -2. `detect_version_change_type` loads both versions of the file -3. The function compares schemas, data types, and values -4. Based on the extent of changes, it returns "major", "minor", or "patch" -5. The version is incremented accordingly (e.g., 1.2.3 → 1.3.0 for minor) -6. DVC tracks the new version with appropriate tags -7. A ResourceVersion record is created in the database - -## Management Commands - -The system includes management commands for manual version control: - -- `create_major_version`: Force a major version increment for a resource -- `setup_dvc`: Configure DVC repository and remotes - -## Error Handling - -The version detection system includes robust error handling to ensure that: - -1. Failed comparisons default to "minor" version changes -2. Temporary files are properly cleaned up -3. Errors are logged with detailed context -4. The system continues functioning even if analysis fails - -## Future Improvements - -- Support for more file formats -- Configurable thresholds for different file types -- Performance optimizations for very large datasets diff --git a/requirements.txt b/requirements.txt index ca9b8bb5..3b5da7ea 100644 --- a/requirements.txt +++ b/requirements.txt @@ -11,7 +11,6 @@ django-elasticsearch-dsl==8.0 django-elasticsearch-dsl-drf==0.22.5 django-nine==0.2.7 djangorestframework==3.14.0 -dvc==3.59.1 elastic-transport==8.13.0 elasticsearch==8.13.0 elasticsearch-dsl==8.12.0 From 0473100418cda7632d6eedcefff2aa0b0c30953e Mon Sep 17 00:00:00 2001 From: Amit Chaudhary Date: Mon, 19 Oct 2026 12:10:44 +0530 Subject: [PATCH 04/11] feat(storage): add S3 public/private media storages Media moves to S3 only, through django-storages. Two storages share one bucket under /public and /private: public assets get unsigned, cacheable URLs; private files get 5-minute presigned URLs. The default storage is private, so anything that falls back to it is never world-readable by accident. Empty access keys use the boto3 credential chain (ECS/EC2 task role). boto/urllib3 loggers are capped at WARNING because at DEBUG they log every signed request, Authorization header included. Tests swap all storages for InMemoryStorage and never reach S3. --- .env.example | 17 ++++++----- DataSpace/settings.py | 48 ++++++++++++++++++++++++++++++-- api/utils/storage.py | 35 +++++++++++++++++++++++ requirements.txt | 3 ++ tests/test_publication_blocks.py | 18 ++++-------- tests/test_settings.py | 14 ++++++++++ 6 files changed, 113 insertions(+), 22 deletions(-) create mode 100644 api/utils/storage.py diff --git a/.env.example b/.env.example index 136af89e..3de8e087 100644 --- a/.env.example +++ b/.env.example @@ -23,13 +23,16 @@ GITHUB_TOKEN= PUBLIC_SITE_URL=https://civicdataspace.in PUBLIC_API_URL=https://api.datakeep.civicdays.in -# --- s3 only --- -# STORAGE_BUCKET=dataspace-dev -# STORAGE_REGION=ap-south-1 -# STORAGE_ENV=dev -# STORAGE_PUBLIC_BASE_URL= -# STORAGE_ACCESS_KEY_ID= -# STORAGE_SECRET_ACCESS_KEY= +# Media storage (S3 only; required). STORAGE_ENV prefixes every key: use +# "local" for local development against the dev bucket. Leave the access keys +# empty on ECS/EC2 so the task role is used. STORAGE_PUBLIC_BASE_URL is the +# base for public asset URLs (CDN or bucket URL); empty uses the S3 bucket URL. +STORAGE_BUCKET=dataspace-dev +STORAGE_REGION=ap-south-1 +STORAGE_ENV=local +STORAGE_PUBLIC_BASE_URL= +STORAGE_ACCESS_KEY_ID= +STORAGE_SECRET_ACCESS_KEY= # Per-IP request throttling (api/middleware/rate_limit.py). False on dev only. RATE_LIMIT_ENABLED=True diff --git a/DataSpace/settings.py b/DataSpace/settings.py index 63452733..9922233d 100644 --- a/DataSpace/settings.py +++ b/DataSpace/settings.py @@ -255,8 +255,39 @@ # https://docs.djangoproject.com/en/4.0/howto/static-files/ # This STATIC_URL setting is overridden below +# Uploaded media lives only in S3 (see STORAGES below). MEDIA_URL is kept as the +# prefix of the legacy /public/ redirect route for links issued before S3. MEDIA_URL = "public/" -MEDIA_ROOT = os.path.join(BASE_DIR, "files", "public") + +# S3 media storage. STORAGE_ENV prefixes every key (local, dev, prod) so one +# bucket can hold several environments. Empty keys fall back to the boto3 +# credential chain, which is what ECS/EC2 task roles use. +STORAGE_BUCKET = env("STORAGE_BUCKET") +STORAGE_REGION = env("STORAGE_REGION") +STORAGE_ENV = env("STORAGE_ENV") +STORAGE_PUBLIC_BASE_URL = env("STORAGE_PUBLIC_BASE_URL", default="") + +_S3_OPTIONS = { + "bucket_name": STORAGE_BUCKET, + "region_name": STORAGE_REGION, + "access_key": env("STORAGE_ACCESS_KEY_ID", default="") or None, + "secret_key": env("STORAGE_SECRET_ACCESS_KEY", default="") or None, + "file_overwrite": False, + # The bucket enforces object ownership; access is granted by bucket policy, not ACLs. + "default_acl": None, +} +_S3_PUBLIC_OPTIONS = { + **_S3_OPTIONS, + "location": f"{STORAGE_ENV}/public", + "querystring_auth": False, + "custom_domain": STORAGE_PUBLIC_BASE_URL.split("://", 1)[-1].rstrip("/") or None, +} +_S3_PRIVATE_OPTIONS = { + **_S3_OPTIONS, + "location": f"{STORAGE_ENV}/private", + "querystring_auth": True, + "querystring_expire": 300, +} # Default primary key field type # https://docs.djangoproject.com/en/4.0/ref/settings/#default-auto-field @@ -372,6 +403,9 @@ "level": "DEBUG" if DEBUG else "INFO", "propagate": True, }, + # At DEBUG the AWS SDK logs every signed request, Authorization header + # included, and floods the log on each S3 call. + **{name: {"level": "WARNING"} for name in ("boto3", "botocore", "s3transfer", "urllib3")}, }, } @@ -485,8 +519,16 @@ "whitenoise.middleware.WhiteNoiseMiddleware", ) -# Use the simplest WhiteNoise storage configuration to avoid MIME type issues -STATICFILES_STORAGE = "whitenoise.storage.StaticFilesStorage" +# "public"/"private" are what model fields name through api.utils.storage. The +# default is private so anything that falls back to default_storage is never +# world-readable by accident. +STORAGES = { + "default": {"BACKEND": "storages.backends.s3.S3Storage", "OPTIONS": _S3_PRIVATE_OPTIONS}, + "public": {"BACKEND": "storages.backends.s3.S3Storage", "OPTIONS": _S3_PUBLIC_OPTIONS}, + "private": {"BACKEND": "storages.backends.s3.S3Storage", "OPTIONS": _S3_PRIVATE_OPTIONS}, + # Use the simplest WhiteNoise storage configuration to avoid MIME type issues + "staticfiles": {"BACKEND": "whitenoise.storage.StaticFilesStorage"}, +} # Disable compression to avoid MIME type issues WHITENOISE_ENABLE_COMPRESSION = False diff --git a/api/utils/storage.py b/api/utils/storage.py new file mode 100644 index 00000000..7cdb679e --- /dev/null +++ b/api/utils/storage.py @@ -0,0 +1,35 @@ +"""Media storages for model file fields. + +Every ``FileField``/``ImageField`` names one of these through ``storage=``. +They are callables, not instances, so migrations record a function reference +instead of bucket settings, and tests can swap the backend through STORAGES. +""" + +from django.core.files.storage import Storage, storages + +# Folder prefixes from the on-disk era, when MEDIA_ROOT was files/public and the +# upload_to helpers added their own "files/..." on top. The S3 storages already +# put every key under /public or /private, so these only repeat it. +_LEGACY_PREFIXES = ("files/public/", "files/") + + +def public_storage() -> Storage: + """Storage for files anyone may read: unsigned, cacheable URLs.""" + return storages["public"] + + +def private_storage() -> Storage: + """Storage for gated files: short-lived presigned URLs only.""" + return storages["private"] + + +def normalize_media_name(name: str) -> str: + """Strip the legacy ``files/public/`` or ``files/`` prefix from a stored name. + + ``files/public/organizations/acme/png/logo.png`` -> ``organizations/acme/png/logo.png``. + Names without a legacy prefix are returned unchanged. + """ + for prefix in _LEGACY_PREFIXES: + if name.startswith(prefix): + return name[len(prefix) :] + return name diff --git a/requirements.txt b/requirements.txt index 3b5da7ea..cb69c412 100644 --- a/requirements.txt +++ b/requirements.txt @@ -11,6 +11,9 @@ django-elasticsearch-dsl==8.0 django-elasticsearch-dsl-drf==0.22.5 django-nine==0.2.7 djangorestframework==3.14.0 +# S3 media storage +django-storages[s3]==1.14.4 +boto3==1.43.108 elastic-transport==8.13.0 elasticsearch==8.13.0 elasticsearch-dsl==8.12.0 diff --git a/tests/test_publication_blocks.py b/tests/test_publication_blocks.py index 6364830e..38ee1b98 100644 --- a/tests/test_publication_blocks.py +++ b/tests/test_publication_blocks.py @@ -1,6 +1,5 @@ """Layer 1/3/4 tests for content blocks: add/reorder/remove/replace + download gate.""" -import os from datetime import date import pytest @@ -24,11 +23,6 @@ VIDEO = "https://youtu.be/dQw4w9WgXcQ" -@pytest.fixture(autouse=True) -def media_root(settings, tmp_path): - settings.MEDIA_ROOT = str(tmp_path) - - @pytest.fixture def owner(db): return User.objects.create(username="owner", keycloak_id="owner") @@ -117,27 +111,27 @@ def test_reorder_sets_positions_to_requested_order(self, publication): class TestReplaceFile: def test_replace_swaps_file_and_removes_the_old_one(self, publication): block = add_file_block(publication, _pdf("first.pdf")) - old_path = block.file.path + storage, old_name = block.file.storage, block.file.name old_id = block.id - assert os.path.exists(old_path) + assert storage.exists(old_name) replace_block_file(block, _pdf("second.pdf")) assert block.id == old_id # same row, in-place assert block.file_name == "second.pdf" - assert not os.path.exists(old_path) # old file removed from disk + assert not storage.exists(old_name) # old file removed from storage @pytest.mark.django_db class TestDeleteRemovesFile: def test_deleting_a_block_removes_its_file(self, publication): block = add_file_block(publication, _pdf()) - path = block.file.path - assert os.path.exists(path) + storage, name = block.file.storage, block.file.name + assert storage.exists(name) block.delete() - assert not os.path.exists(path) # post_delete signal cleaned it up + assert not storage.exists(name) # post_delete signal cleaned it up # --------------------------------------------------------------------------- # diff --git a/tests/test_settings.py b/tests/test_settings.py index 6a641855..05375f6d 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -9,8 +9,22 @@ project_root = os.path.abspath(os.path.join(os.path.dirname(__file__), "..")) sys.path.insert(0, project_root) +# The real settings require the S3 storage variables; give the suite harmless +# values so it boots without a .env. STORAGES is swapped for memory below. +os.environ.setdefault("STORAGE_BUCKET", "test-bucket") +os.environ.setdefault("STORAGE_REGION", "ap-south-1") +os.environ.setdefault("STORAGE_ENV", "test") + from DataSpace.settings import * +# Tests never reach S3: both media storages (and the default) live in memory. +STORAGES = { + **STORAGES, # noqa: F405 — imported via ``from DataSpace.settings import *`` + "default": {"BACKEND": "django.core.files.storage.InMemoryStorage"}, + "public": {"BACKEND": "django.core.files.storage.InMemoryStorage"}, + "private": {"BACKEND": "django.core.files.storage.InMemoryStorage"}, +} + # Use an in-memory SQLite database for testing DATABASES = { "default": { From 73f538135c536a6b8532a2cdb61e81a6cc4036be Mon Sep 17 00:00:00 2001 From: Amit Chaudhary Date: Mon, 19 Oct 2026 12:10:51 +0530 Subject: [PATCH 05/11] feat(storage): store model file fields in S3 Every FileField/ImageField now names a storage: logos, cover images, profile pictures and publication block files go to public storage; resource files and chart images go to private storage. The storages are callables, so the migrations record a function reference, not bucket settings. upload_to paths drop their "files/public/" and "files/" prefixes, which only repeated what MEDIA_ROOT used to add; the storages already put every key under /public or /private. --- api/migrations/0054_file_field_storages.py | 108 ++++++++++++++++++ api/models/Catalog.py | 5 +- api/models/Collaborative.py | 13 ++- api/models/DataSpace.py | 5 +- api/models/Organization.py | 11 +- api/models/Publication.py | 2 + api/models/Resource.py | 3 +- api/models/ResourceChartImage.py | 7 +- api/models/UseCase.py | 11 +- api/utils/file_paths.py | 19 +-- .../0005_profile_picture_storage.py | 26 +++++ authorization/models.py | 6 +- 12 files changed, 193 insertions(+), 23 deletions(-) create mode 100644 api/migrations/0054_file_field_storages.py create mode 100644 authorization/migrations/0005_profile_picture_storage.py diff --git a/api/migrations/0054_file_field_storages.py b/api/migrations/0054_file_field_storages.py new file mode 100644 index 00000000..4e8c6f2b --- /dev/null +++ b/api/migrations/0054_file_field_storages.py @@ -0,0 +1,108 @@ +# Generated by Django 5.0.4 on 2026-10-12 20:25 + +from django.db import migrations, models + +import api.utils.file_paths +import api.utils.storage + + +class Migration(migrations.Migration): + + dependencies = [ + ("api", "0053_alter_collaborative_summary"), + ] + + operations = [ + migrations.AlterField( + model_name="catalog", + name="logo", + field=models.ImageField( + blank=True, + null=True, + storage=api.utils.storage.public_storage, + upload_to=api.utils.file_paths._catalog_directory_path, + ), + ), + migrations.AlterField( + model_name="collaborative", + name="cover_image", + field=models.ImageField( + blank=True, + max_length=300, + null=True, + storage=api.utils.storage.public_storage, + upload_to=api.utils.file_paths._use_case_directory_path, + ), + ), + migrations.AlterField( + model_name="collaborative", + name="logo", + field=models.ImageField( + blank=True, + max_length=300, + null=True, + storage=api.utils.storage.public_storage, + upload_to=api.utils.file_paths._use_case_directory_path, + ), + ), + migrations.AlterField( + model_name="dataspace", + name="logo", + field=models.ImageField( + blank=True, + null=True, + storage=api.utils.storage.public_storage, + upload_to=api.utils.file_paths._dataspace_directory_path, + ), + ), + migrations.AlterField( + model_name="organization", + name="logo", + field=models.ImageField( + blank=True, + max_length=300, + null=True, + storage=api.utils.storage.public_storage, + upload_to=api.utils.file_paths._organization_directory_path, + ), + ), + migrations.AlterField( + model_name="publicationblock", + name="file", + field=models.FileField( + blank=True, + max_length=300, + storage=api.utils.storage.public_storage, + upload_to=api.utils.file_paths._publication_block_directory_path, + ), + ), + migrations.AlterField( + model_name="resourcechartimage", + name="image", + field=models.ImageField( + blank=True, + max_length=300, + null=True, + storage=api.utils.storage.private_storage, + upload_to=api.utils.file_paths._chart_image_directory_path, + ), + ), + migrations.AlterField( + model_name="resourcefiledetails", + name="file", + field=models.FileField( + max_length=300, storage=api.utils.storage.private_storage, upload_to="resources/" + ), + ), + migrations.AlterField( + model_name="usecase", + name="logo", + field=models.ImageField( + blank=True, + max_length=300, + null=True, + storage=api.utils.storage.public_storage, + upload_to=api.utils.file_paths._use_case_directory_path, + ), + ), + ] diff --git a/api/models/Catalog.py b/api/models/Catalog.py index 101bb3d7..799a82f6 100644 --- a/api/models/Catalog.py +++ b/api/models/Catalog.py @@ -6,12 +6,15 @@ from django.utils.translation import gettext_lazy as _ from api.utils.file_paths import _catalog_directory_path +from api.utils.storage import public_storage class Catalog(models.Model): name = models.CharField(max_length=255) datasets = models.ManyToManyField("api.Dataset", related_name="catalogs") - logo = models.ImageField(upload_to=_catalog_directory_path, blank=True, null=True) + logo = models.ImageField( + upload_to=_catalog_directory_path, storage=public_storage, blank=True, null=True + ) created = models.DateTimeField(auto_now_add=True) modified = models.DateTimeField(auto_now=True) homepage = models.URLField(blank=True) diff --git a/api/models/Collaborative.py b/api/models/Collaborative.py index 3b9a38a7..f39f04a2 100644 --- a/api/models/Collaborative.py +++ b/api/models/Collaborative.py @@ -13,6 +13,7 @@ from api.utils.django_utils import retry_on_slug_collision from api.utils.enums import CollaborativeStatus, OrganizationRelationshipType from api.utils.file_paths import _use_case_directory_path +from api.utils.storage import public_storage slug_validator = RegexValidator( regex=r"^[a-z0-9]+(?:-[a-z0-9]+)*$", @@ -25,10 +26,18 @@ class Collaborative(models.Model): title = models.CharField(max_length=200, unique=True, blank=True, null=True) summary = models.TextField(max_length=200_000, blank=True, null=True) logo = models.ImageField( - upload_to=_use_case_directory_path, max_length=300, blank=True, null=True + upload_to=_use_case_directory_path, + storage=public_storage, + max_length=300, + blank=True, + null=True, ) cover_image = models.ImageField( - upload_to=_use_case_directory_path, max_length=300, blank=True, null=True + upload_to=_use_case_directory_path, + storage=public_storage, + max_length=300, + blank=True, + null=True, ) created = models.DateTimeField(auto_now_add=True) modified = models.DateTimeField(auto_now=True) diff --git a/api/models/DataSpace.py b/api/models/DataSpace.py index 7430065f..1cb9a8fc 100644 --- a/api/models/DataSpace.py +++ b/api/models/DataSpace.py @@ -4,12 +4,15 @@ from django.utils.text import slugify from api.utils.file_paths import _dataspace_directory_path +from api.utils.storage import public_storage class DataSpace(models.Model): name = models.CharField(max_length=200) description = models.CharField(max_length=1000) - logo = models.ImageField(upload_to=_dataspace_directory_path, blank=True, null=True) + logo = models.ImageField( + upload_to=_dataspace_directory_path, storage=public_storage, blank=True, null=True + ) created = models.DateTimeField(auto_now_add=True) modified = models.DateTimeField(auto_now=True) homepage = models.URLField(blank=True) diff --git a/api/models/Organization.py b/api/models/Organization.py index 977f041a..96131f2d 100644 --- a/api/models/Organization.py +++ b/api/models/Organization.py @@ -5,21 +5,24 @@ from api.utils.enums import OrganizationTypes from api.utils.file_paths import _organization_directory_path +from api.utils.storage import public_storage class Organization(models.Model): name = models.CharField(max_length=200) description = models.CharField(max_length=1000) logo = models.ImageField( - upload_to=_organization_directory_path, blank=True, null=True, max_length=300 + upload_to=_organization_directory_path, + storage=public_storage, + blank=True, + null=True, + max_length=300, ) created = models.DateTimeField(auto_now_add=True) modified = models.DateTimeField(auto_now=True) homepage = models.URLField(blank=True) contact_email = models.EmailField(blank=True, null=True) - organization_types = models.CharField( - max_length=100, choices=OrganizationTypes.choices - ) + organization_types = models.CharField(max_length=100, choices=OrganizationTypes.choices) parent = models.ForeignKey( "api.Organization", unique=False, diff --git a/api/models/Publication.py b/api/models/Publication.py index 2acd2c5d..aea63f46 100644 --- a/api/models/Publication.py +++ b/api/models/Publication.py @@ -7,6 +7,7 @@ from api.utils.enums import DatasetLicense, PublicationBlockType, PublicationStatus from api.utils.file_paths import _publication_block_directory_path +from api.utils.storage import public_storage if TYPE_CHECKING: from api.models.Organization import Organization @@ -149,6 +150,7 @@ class PublicationBlock(models.Model): # FILE block fields. file = models.FileField( upload_to=_publication_block_directory_path, + storage=public_storage, max_length=300, blank=True, ) diff --git a/api/models/Resource.py b/api/models/Resource.py index c9857edd..9d20d934 100644 --- a/api/models/Resource.py +++ b/api/models/Resource.py @@ -12,6 +12,7 @@ logger = structlog.getLogger(__name__) from api.utils.enums import DataType +from api.utils.storage import private_storage if TYPE_CHECKING: from api.models.Dataset import Dataset @@ -67,7 +68,7 @@ def __str__(self) -> str: class ResourceFileDetails(models.Model): resource = models.OneToOneField(Resource, on_delete=models.CASCADE, null=False, blank=False) - file = models.FileField(upload_to="resources/", max_length=300) + file = models.FileField(upload_to="resources/", storage=private_storage, max_length=300) size = models.FloatField(blank=True, null=True) sha256 = models.CharField( max_length=64, diff --git a/api/models/ResourceChartImage.py b/api/models/ResourceChartImage.py index 16812dbb..9c19256d 100644 --- a/api/models/ResourceChartImage.py +++ b/api/models/ResourceChartImage.py @@ -6,6 +6,7 @@ # Import the Dataset model indirectly to avoid duplicate import error from api.utils.file_paths import _chart_image_directory_path +from api.utils.storage import private_storage class ResourceChartImage(models.Model): @@ -13,7 +14,11 @@ class ResourceChartImage(models.Model): name = models.CharField(max_length=50, unique=False, blank=True) description = models.CharField(max_length=1000, unique=False, blank=True) image = models.ImageField( - upload_to=_chart_image_directory_path, blank=True, null=True, max_length=300 + upload_to=_chart_image_directory_path, + storage=private_storage, + blank=True, + null=True, + max_length=300, ) dataset = models.ForeignKey( "api.Dataset", diff --git a/api/models/UseCase.py b/api/models/UseCase.py index 321bfdfc..0ea9b67d 100644 --- a/api/models/UseCase.py +++ b/api/models/UseCase.py @@ -16,6 +16,7 @@ UseCaseStatus, ) from api.utils.file_paths import _use_case_directory_path +from api.utils.storage import public_storage class UseCase(models.Model): @@ -23,7 +24,11 @@ class UseCase(models.Model): title = models.CharField(max_length=200, unique=True, blank=True, null=True) summary = models.TextField(max_length=200_000, blank=True, null=True) logo = models.ImageField( - upload_to=_use_case_directory_path, max_length=300, blank=True, null=True + upload_to=_use_case_directory_path, + storage=public_storage, + max_length=300, + blank=True, + null=True, ) created = models.DateTimeField(auto_now_add=True) modified = models.DateTimeField(auto_now=True) @@ -71,9 +76,7 @@ def disambiguate(attempt: int) -> None: self.title = f"{base_title} ({attempt})" self.slug = slugify(cast(str, self.title)) - retry_on_slug_collision( - lambda: super(UseCase, self).save(*args, **kwargs), disambiguate - ) + retry_on_slug_collision(lambda: super(UseCase, self).save(*args, **kwargs), disambiguate) @property def is_individual_usecase(self): diff --git a/api/utils/file_paths.py b/api/utils/file_paths.py index aa1ecaa2..7cddf28b 100644 --- a/api/utils/file_paths.py +++ b/api/utils/file_paths.py @@ -2,6 +2,9 @@ import uuid from typing import Any +# Names are relative to the storage, which already prefixes every key with +# /public or /private; see api/utils/storage.py. + def _organization_directory_path(org: Any, filename: str) -> str: """ @@ -11,7 +14,7 @@ def _organization_directory_path(org: Any, filename: str) -> str: org_name = org.name _, extension = os.path.splitext(filename) - return f"files/public/organizations/{org_name}/{extension[1:]}/{filename}" + return f"organizations/{org_name}/{extension[1:]}/{filename}" def _chart_image_directory_path(chart: Any, filename: str) -> str: @@ -22,7 +25,7 @@ def _chart_image_directory_path(chart: Any, filename: str) -> str: chart_name = chart.name _, extension = os.path.splitext(filename) - return f"files/chart_images/{chart_name}/{extension[1:]}/{filename}" + return f"chart_images/{chart_name}/{extension[1:]}/{filename}" def _dataspace_directory_path(ds: Any, filename: str) -> str: @@ -33,7 +36,7 @@ def _dataspace_directory_path(ds: Any, filename: str) -> str: ds_name = ds.name _, extension = os.path.splitext(filename) - return f"files/public/dataspace/{ds_name}/{extension[1:]}/{filename}" + return f"dataspace/{ds_name}/{extension[1:]}/{filename}" def _use_case_directory_path(uc: Any, filename: str) -> str: @@ -44,7 +47,7 @@ def _use_case_directory_path(uc: Any, filename: str) -> str: uc_name = uc.title _, extension = os.path.splitext(filename) - return f"files/use_case/{uc_name}/logo/{filename}" + return f"use_case/{uc_name}/logo/{filename}" def _user_profile_image_directory_path(user: Any, filename: str) -> str: @@ -54,7 +57,7 @@ def _user_profile_image_directory_path(user: Any, filename: str) -> str: """ user_name = user.keycloak_id _, extension = os.path.splitext(filename) - return f"files/user_profile/{user_name}/{extension[1:]}/{filename}" + return f"user_profile/{user_name}/{extension[1:]}/{filename}" def _organization_file_directory_path(org: Any, filename: str) -> str: @@ -65,7 +68,7 @@ def _organization_file_directory_path(org: Any, filename: str) -> str: org_name = org.name _, extension = os.path.splitext(filename) - return f"files/resources/{org_name}/sample_data/{extension[1:]}/{filename}" + return f"resources/{org_name}/sample_data/{extension[1:]}/{filename}" def _catalog_directory_path(catalog: Any, filename: str) -> str: @@ -74,7 +77,7 @@ def _catalog_directory_path(catalog: Any, filename: str) -> str: """ catalog_name = catalog.name _, extension = os.path.splitext(filename) - return f"files/catalog/{catalog_name}/{extension[1:]}/{filename}" + return f"catalog/{catalog_name}/{extension[1:]}/{filename}" def _publication_block_directory_path(block: Any, filename: str) -> str: @@ -86,4 +89,4 @@ def _publication_block_directory_path(block: Any, filename: str) -> str: """ publication_id = block.publication_id unique_name = f"{uuid.uuid4().hex}_{filename}" - return f"files/public/publications/{publication_id}/{unique_name}" + return f"publications/{publication_id}/{unique_name}" diff --git a/authorization/migrations/0005_profile_picture_storage.py b/authorization/migrations/0005_profile_picture_storage.py new file mode 100644 index 00000000..61d9507e --- /dev/null +++ b/authorization/migrations/0005_profile_picture_storage.py @@ -0,0 +1,26 @@ +# Generated by Django 5.0.4 on 2026-10-12 20:25 + +from django.db import migrations, models + +import api.utils.file_paths +import api.utils.storage + + +class Migration(migrations.Migration): + + dependencies = [ + ("authorization", "0004_userconsent"), + ] + + operations = [ + migrations.AlterField( + model_name="user", + name="profile_picture", + field=models.ImageField( + blank=True, + null=True, + storage=api.utils.storage.public_storage, + upload_to=api.utils.file_paths._user_profile_image_directory_path, + ), + ), + ] diff --git a/authorization/models.py b/authorization/models.py index 7a295002..85927475 100644 --- a/authorization/models.py +++ b/authorization/models.py @@ -3,6 +3,7 @@ from django.utils.text import slugify from api.utils.file_paths import _user_profile_image_directory_path +from api.utils.storage import public_storage from authorization.consent import UserConsent @@ -46,7 +47,10 @@ class User(AbstractUser): # Additional user profile fields bio = models.TextField(blank=True, null=True) profile_picture = models.ImageField( - upload_to=_user_profile_image_directory_path, blank=True, null=True + upload_to=_user_profile_image_directory_path, + storage=public_storage, + blank=True, + null=True, ) github_profile = models.URLField(blank=True, null=True) linkedin_profile = models.URLField(blank=True, null=True) From 85e183368d591552762d08fd7e23f20d8c0ca9d7 Mon Sep 17 00:00:00 2001 From: Amit Chaudhary Date: Mon, 19 Oct 2026 12:10:57 +0530 Subject: [PATCH 06/11] fix(api): read and serve media without local file paths S3 has no FieldFile.path, so every reader that used it would raise NotImplementedError once media left local disk. - local_copy() yields a temporary local copy (keeping the extension) for pandas, libmagic and the YAML/XML parsers; data indexing, entry counts and MIME validation use it. - The lru_cache on the loaders is dropped: it keyed on temp paths that are deleted on exit, so it only held DataFrames in memory. - Downloads stream with FileResponse instead of reading whole objects. - Charts only check that the file exists; they read the indexed table. - Search documents index the storage URL instead of a rewritten disk path. --- api/schema/resource_schema.py | 4 +- api/types/type_resource_chart.py | 9 ++-- api/utils/data_indexing.py | 58 +++++++++++----------- api/utils/file_utils.py | 43 ++++++++++------ api/views/download_view.py | 20 +++++--- api/views/publication_download_view.py | 9 ++-- search/documents/collaborative_document.py | 4 +- search/documents/usecase_document.py | 2 +- 8 files changed, 86 insertions(+), 63 deletions(-) diff --git a/api/schema/resource_schema.py b/api/schema/resource_schema.py index bb3c6d2a..143fd637 100644 --- a/api/schema/resource_schema.py +++ b/api/schema/resource_schema.py @@ -1,4 +1,3 @@ -import copy import uuid from enum import Enum @@ -135,8 +134,7 @@ def _validate_file_details_and_update_format(resource: Resource) -> None: raise ValueError("Resource has no file details") file = file_details.file - deep_copy_file = copy.deepcopy(file) - mime_type = file_validation(deep_copy_file, file, FORMAT_MAPPING) + mime_type = file_validation(file, file, FORMAT_MAPPING) if not mime_type: raise ValueError("Unsupported file format.") diff --git a/api/types/type_resource_chart.py b/api/types/type_resource_chart.py index c4236ac5..17593365 100644 --- a/api/types/type_resource_chart.py +++ b/api/types/type_resource_chart.py @@ -1,7 +1,9 @@ # mypy: disable-error-code="valid-type" import json + from pyecharts.globals import CurrentConfig + CurrentConfig.ONLINE_HOST = "file:///code/" import uuid from datetime import datetime @@ -20,7 +22,6 @@ from api.types.type_dataset import TypeDataset from api.types.type_resource import TypeResource, TypeResourceSchema from api.utils.enums import ChartStatus, ChartTypes -from api.utils.file_utils import load_csv logger = structlog.get_logger(__name__) T = TypeVar("T", bound="TypeResourceChart") @@ -41,8 +42,10 @@ def chart_base(chart_details: ResourceChartDetails) -> Optional[Chart]: logger.error("invalid resource format") return None - data = load_csv(file_details.file.path) - except (AttributeError, FileNotFoundError): + # Charts read the indexed data table, not the file; only require that it exists. + if not file_details.file.storage.exists(file_details.file.name): + return None + except AttributeError: return None chart_class = CHART_REGISTRY.get(chart_details.chart_type) diff --git a/api/utils/data_indexing.py b/api/utils/data_indexing.py index 8d56da54..16201045 100644 --- a/api/utils/data_indexing.py +++ b/api/utils/data_indexing.py @@ -9,7 +9,7 @@ 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 +from api.utils.file_utils import load_tabular_data, local_copy logger = structlog.get_logger("dataspace.data_indexing") @@ -173,16 +173,19 @@ def handler(signum: int, frame: Any) -> None: signal.alarm(0) signal.signal(signal.SIGALRM, original_handler) - # 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_details.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_details.file.path, format) + 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( @@ -410,29 +413,28 @@ def get_entry_count_from_file(resource: Resource) -> int: return 0 fmt = file_details.format.lower() - filepath = file_details.file.path - - if fmt in ("yml", "yaml"): - import yaml - - with open(filepath, "r") as f: - data = yaml.safe_load(f) - if isinstance(data, list): - return len(data) - if isinstance(data, dict): - for v in data.values(): - if isinstance(v, list): - return len(v) - return len(data) + if fmt not in FILE_COUNT_FORMATS: return 0 - if fmt == "xml": + with local_copy(file_details.file) as filepath: + if fmt in ("yml", "yaml"): + import yaml + + with open(filepath, "r") as f: + data = yaml.safe_load(f) + if isinstance(data, list): + return len(data) + if isinstance(data, dict): + for v in data.values(): + if isinstance(v, list): + return len(v) + return len(data) + return 0 + import xml.etree.ElementTree as ET root = ET.parse(filepath).getroot() return len(list(root)) - - return 0 except Exception as e: logger.error(f"Error counting entries from file for resource {resource.id}: {str(e)}") return 0 diff --git a/api/utils/file_utils.py b/api/utils/file_utils.py index 9028f799..b3ae56a3 100644 --- a/api/utils/file_utils.py +++ b/api/utils/file_utils.py @@ -1,8 +1,9 @@ import mimetypes import os -from functools import lru_cache +import tempfile +from contextlib import contextmanager from pathlib import Path -from typing import Any, Dict, Optional, Union +from typing import Any, Dict, Iterator, Optional, Union import magic import pandas as pd @@ -16,18 +17,36 @@ def check_ext(file_object: Any) -> Optional[str]: return ext_type +@contextmanager +def local_copy(fieldfile: Any) -> Iterator[str]: + """Yield the path of a temporary local copy of a stored file. + + Media lives in S3, which has no local path, but pandas, libmagic and the + YAML/XML parsers need one. The copy keeps the file's extension (some + readers sniff it) and is removed on exit, even if the caller raises. + """ + suffix = os.path.splitext(fieldfile.name)[1] + with tempfile.NamedTemporaryFile(suffix=suffix) as tmp: + fieldfile.open("rb") + try: + for chunk in fieldfile.chunks(): + tmp.write(chunk) + finally: + fieldfile.close() + tmp.flush() + yield tmp.name + + def check_mime_type(file_object: Any) -> Any: # Prefer from_file: libmagic 5.46 misidentifies zip buffers as # application/octet-stream via from_buffer, while from_file works. - file_path = getattr(file_object, "path", None) - if file_path: - return magic.from_file(file_path, mime=True) + if hasattr(file_object, "storage"): + with local_copy(file_object) as file_path: + return magic.from_file(file_path, mime=True) return magic.from_buffer(file_object.read(), mime=True) -def file_validation( - file_object: Any, ext_object: Any, mapping: Dict[str, str] -) -> Optional[Any]: +def file_validation(file_object: Any, ext_object: Any, mapping: Dict[str, str]) -> Optional[Any]: ext_type = check_ext(ext_object) print("ext--", ext_type) mime_type = check_mime_type(file_object) @@ -78,7 +97,6 @@ def normalize_path(path: Union[str, Path]) -> str: return os.path.normpath(str(path)) -@lru_cache() def load_csv(filepath: str) -> pd.DataFrame: """Load CSV file into a pandas DataFrame. @@ -91,7 +109,6 @@ def load_csv(filepath: str) -> pd.DataFrame: return pd.read_csv(filepath) -@lru_cache() def load_excel(filepath: str) -> pd.DataFrame: """Load Excel file into a pandas DataFrame. @@ -104,7 +121,6 @@ def load_excel(filepath: str) -> pd.DataFrame: return pd.read_excel(filepath) -@lru_cache() def load_ods(filepath: str) -> pd.DataFrame: """Load ODS file into a pandas DataFrame. @@ -117,7 +133,6 @@ def load_ods(filepath: str) -> pd.DataFrame: return pd.read_excel(filepath, engine="odf") -@lru_cache() def load_parquet(filepath: str) -> pd.DataFrame: """Load Parquet file into a pandas DataFrame. @@ -130,7 +145,6 @@ def load_parquet(filepath: str) -> pd.DataFrame: return pd.read_parquet(filepath) -@lru_cache() def load_feather(filepath: str) -> pd.DataFrame: """Load Feather file into a pandas DataFrame. @@ -143,7 +157,6 @@ def load_feather(filepath: str) -> pd.DataFrame: return pd.read_feather(filepath) -@lru_cache() def load_json(filepath: str) -> pd.DataFrame: """Load JSON file into a pandas DataFrame. @@ -156,7 +169,6 @@ def load_json(filepath: str) -> pd.DataFrame: return pd.read_json(filepath) -@lru_cache() def load_tsv(filepath: str) -> pd.DataFrame: """Load TSV file into a pandas DataFrame. @@ -169,7 +181,6 @@ def load_tsv(filepath: str) -> pd.DataFrame: return pd.read_csv(filepath, sep="\t") -@lru_cache() def load_tabular_data(filepath: str, format: str) -> Optional[pd.DataFrame]: """Load tabular data from various file formats into a pandas DataFrame. diff --git a/api/views/download_view.py b/api/views/download_view.py index 5d106483..d6668bae 100644 --- a/api/views/download_view.py +++ b/api/views/download_view.py @@ -6,7 +6,14 @@ from asgiref.sync import sync_to_async from django.core.exceptions import ObjectDoesNotExist from django.core.files.uploadedfile import UploadedFile -from django.http import HttpRequest, HttpResponse, HttpResponseRedirect, JsonResponse +from django.http import ( + FileResponse, + HttpRequest, + HttpResponse, + HttpResponseRedirect, + JsonResponse, +) +from django.http.response import HttpResponseBase from pyecharts.charts.chart import Chart from pyecharts.render import make_snapshot from selenium import webdriver @@ -46,7 +53,7 @@ def get_chart_image(id: uuid.UUID) -> ResourceChartImage: @sync_to_async def get_resource_response( resource: Resource, request: Optional[HttpRequest] = None -) -> HttpResponse: +) -> HttpResponseBase: """Get file response for a resource.""" # Link-only (platform-imported) resources: we hold no bytes, send the # user to the file on the source platform. Still counts as a download. @@ -62,7 +69,8 @@ def get_resource_response( if not file_details or not file_details.file: return JsonResponse({"error": "File not found"}, status=404) - response = HttpResponse(file_details.file.read(), content_type="application/octet-stream") + # Stream from storage instead of reading the whole object into memory. + response = FileResponse(file_details.file.open("rb"), content_type="application/octet-stream") # Handle filename and basename explicitly default_name = f"resource_{resource.name}.csv" @@ -98,12 +106,12 @@ def _track_download(resource: Resource, request: Optional[HttpRequest]) -> None: @sync_to_async -def get_chart_image_response(chart_image: ResourceChartImage) -> HttpResponse: +def get_chart_image_response(chart_image: ResourceChartImage) -> HttpResponseBase: """Get file response for a chart image.""" if not chart_image.image: return JsonResponse({"error": "File not found"}, status=404) - response = HttpResponse(chart_image.image.read(), content_type="application/octet-stream") + response = FileResponse(chart_image.image.open("rb"), content_type="application/octet-stream") # Handle filename and basename explicitly default_name = f"chart_{chart_image.id}.png" @@ -119,7 +127,7 @@ def get_chart_image_response(chart_image: ResourceChartImage) -> HttpResponse: return response -async def download(request: HttpRequest, type: str, id: uuid.UUID) -> HttpResponse: +async def download(request: HttpRequest, type: str, id: uuid.UUID) -> HttpResponseBase: """Handle download requests for resources, chart images, and charts.""" try: if type == "resource": diff --git a/api/views/publication_download_view.py b/api/views/publication_download_view.py index deac2435..8f076909 100644 --- a/api/views/publication_download_view.py +++ b/api/views/publication_download_view.py @@ -15,7 +15,8 @@ from typing import Any from django.db.models import F -from django.http import HttpRequest, HttpResponse, JsonResponse +from django.http import FileResponse, HttpRequest, JsonResponse +from django.http.response import HttpResponseBase from api.models import Publication, PublicationBlock from api.services.publication_blocks import can_access_block_file @@ -25,7 +26,7 @@ _INLINE_CONTENT_TYPES = {".pdf": "application/pdf"} -def publication_block_download(request: HttpRequest, block_id: Any) -> HttpResponse: +def publication_block_download(request: HttpRequest, block_id: Any) -> HttpResponseBase: """Serve a block's file if the caller may see its resource, else 404.""" # Find the block and its parent resource. try: @@ -52,7 +53,7 @@ def publication_block_download(request: HttpRequest, block_id: Any) -> HttpRespo return _build_file_response(block) -def _build_file_response(block: PublicationBlock) -> HttpResponse: +def _build_file_response(block: PublicationBlock) -> FileResponse: """Build the HTTP file response with the right content type and disposition.""" stored_name = block.file.name or "" extension = os.path.splitext(stored_name)[1].lower() @@ -60,6 +61,6 @@ def _build_file_response(block: PublicationBlock) -> HttpResponse: disposition = "inline" if extension in _INLINE_CONTENT_TYPES else "attachment" filename = block.file_name or os.path.basename(stored_name) - response = HttpResponse(block.file.read(), content_type=content_type) + response = FileResponse(block.file.open("rb"), content_type=content_type) response["Content-Disposition"] = f'{disposition}; filename="{filename}"' return response diff --git a/search/documents/collaborative_document.py b/search/documents/collaborative_document.py index ef1ec70f..1fedaca3 100644 --- a/search/documents/collaborative_document.py +++ b/search/documents/collaborative_document.py @@ -246,12 +246,12 @@ def prepare_organizations(self, instance: Collaborative) -> List[Dict[str, str]] def prepare_logo(self, instance: Collaborative) -> str: if instance.logo: - return str(instance.logo.path.replace("/code/files/", "")) + return str(instance.logo.url) return "" def prepare_cover_image(self, instance: Collaborative) -> str: if instance.cover_image: - return str(instance.cover_image.path.replace("/code/files/", "")) + return str(instance.cover_image.url) return "" def prepare_dataset_count(self, instance: Collaborative) -> int: diff --git a/search/documents/usecase_document.py b/search/documents/usecase_document.py index b2b9e023..9849d753 100644 --- a/search/documents/usecase_document.py +++ b/search/documents/usecase_document.py @@ -226,7 +226,7 @@ def prepare_organizations(self, instance: UseCase) -> List[Dict[str, str]]: def prepare_logo(self, instance: UseCase) -> str: """Prepare logo URL for indexing.""" if instance.logo: - return str(instance.logo.path.replace("/code/files/", "")) + return str(instance.logo.url) return "" def should_index_object(self, obj: UseCase) -> bool: From 1eb56a3602bfa27d0fdc3887901303187e2af8a7 Mon Sep 17 00:00:00 2001 From: Amit Chaudhary Date: Mon, 19 Oct 2026 12:11:03 +0530 Subject: [PATCH 07/11] feat(api): redirect legacy /public media links to S3 Links issued before the move pointed at /public/ on disk. That route now redirects to the file's public S3 URL, after stripping the old "files/public/" or "files/" prefix from the name. Only public files redirect. The old static() route also served resources and drafts whenever DEBUG was on; anything outside public storage, or a name that tries to traverse paths, is a 404 here. --- DataSpace/urls.py | 16 +++++++++++----- api/views/legacy_media.py | 36 ++++++++++++++++++++++++++++++++++++ search/urls.py | 2 +- 3 files changed, 48 insertions(+), 6 deletions(-) create mode 100644 api/views/legacy_media.py diff --git a/DataSpace/urls.py b/DataSpace/urls.py index 281b717e..85f149c5 100644 --- a/DataSpace/urls.py +++ b/DataSpace/urls.py @@ -23,7 +23,7 @@ from drf_yasg import openapi from drf_yasg.views import get_schema_view -from api.views import health +from api.views import health, legacy_media # Type alias for URL patterns URLPatternsList = List[Union[URLPattern, URLResolver]] @@ -49,16 +49,22 @@ # Health check endpoint path("health/", health.health_check, name="health_check"), # API documentation - path( - "swagger/", schema_view.without_ui(cache_timeout=0), name="schema-json" - ), + path("swagger/", schema_view.without_ui(cache_timeout=0), name="schema-json"), path( "swagger/", schema_view.with_ui("swagger", cache_timeout=0), name="schema-swagger-ui", ), path("redoc/", schema_view.with_ui("redoc", cache_timeout=0), name="schema-redoc"), -] + static(settings.MEDIA_URL, document_root=settings.MEDIA_ROOT) + # Links issued before media moved to S3; see api/views/legacy_media.py. + path( + # Django prefixes a relative MEDIA_URL with the script name ("/public/"), + # which a URL pattern never matches. + f"{settings.MEDIA_URL.lstrip('/')}", + legacy_media.legacy_media_redirect, + name="legacy_media", + ), +] # In debug mode, add static URL patterns and debug toolbar if settings.DEBUG: diff --git a/api/views/legacy_media.py b/api/views/legacy_media.py new file mode 100644 index 00000000..e8900a24 --- /dev/null +++ b/api/views/legacy_media.py @@ -0,0 +1,36 @@ +""" +legacy_media +──────────── +Keeps links issued before the move to S3 working. Media used to be served from +disk at ``/public/``; that path now redirects to the file's S3 URL. + +Names are normalised first: rows created on disk carried a ``files/public/`` or +``files/`` prefix that S3 keys no longer have, so ``/public/files/public/x.png`` +redirects to the file now stored as ``x.png``. + +Only public files redirect. The old route also exposed resources and draft +files whenever DEBUG was on; anything not in public storage is a 404 here, so +this route can never hand out a private object. +""" + +import posixpath + +from django.http import HttpRequest, HttpResponse, HttpResponseRedirect, JsonResponse + +from api.utils.storage import normalize_media_name, public_storage + + +def legacy_media_redirect(request: HttpRequest, name: str) -> HttpResponse: + """Redirect an old ``/public/`` link to the public file's S3 URL, else 404.""" + normalised = posixpath.normpath(name) + if normalised != name or normalised.startswith(("/", "..")): + return JsonResponse({"error": "Not found"}, status=404) + + storage = public_storage() + current = normalize_media_name(name) + if not storage.exists(current): + return JsonResponse({"error": "Not found"}, status=404) + + response = HttpResponseRedirect(storage.url(current)) + response["Cache-Control"] = "public, max-age=86400" + return response diff --git a/search/urls.py b/search/urls.py index 7e663078..0b8ad4c4 100644 --- a/search/urls.py +++ b/search/urls.py @@ -32,4 +32,4 @@ # urlpatterns = [ # path('', include(router.urls)), -# ] + static(settings.MEDIA_URL, document_root=settings.MEDIA_ROOT) +# ] From a1ef6f9c18b280e06369e3337edaddc05c5e30d1 Mon Sep 17 00:00:00 2001 From: Amit Chaudhary Date: Mon, 19 Oct 2026 12:11:03 +0530 Subject: [PATCH 08/11] feat(api): add migrate_media_to_s3 command Rows created on disk carry a "files/public/" or "files/" prefix that the S3 keys no longer use. For each stored name the command puts the file at the normalised key in the storage its field names, taken from the old S3 key or, with --source, from the old disk tree, then repoints the row. Dry run by default; --execute copies and renames. Safe to re-run (names already in place are skipped) and deletes nothing. --- .../commands/migrate_media_to_s3.py | 144 ++++++++++++++++++ 1 file changed, 144 insertions(+) create mode 100644 api/management/commands/migrate_media_to_s3.py diff --git a/api/management/commands/migrate_media_to_s3.py b/api/management/commands/migrate_media_to_s3.py new file mode 100644 index 00000000..a85bf0d7 --- /dev/null +++ b/api/management/commands/migrate_media_to_s3.py @@ -0,0 +1,144 @@ +"""Bring every stored file into S3 under its current, prefix-free name. + + python manage.py migrate_media_to_s3 # dry run + python manage.py migrate_media_to_s3 --execute # copy + rename + python manage.py migrate_media_to_s3 --source /code/files/public --execute + +Rows created on disk carry a ``files/public/`` or ``files/`` prefix that the S3 +keys no longer use (see ``normalize_media_name``). For each stored name this +puts the file at the normalised key in the storage its field names, taking it +from the old S3 key or, with ``--source``, from the old disk tree, then points +the row at the new name. Safe to re-run: names already in place are skipped. +Nothing is deleted, neither local files nor old S3 objects. +""" + +from collections import Counter +from pathlib import Path +from typing import Any, Iterator, List, Optional, Set, Tuple, Type + +import structlog +from django.apps import apps +from django.core.files import File +from django.core.files.storage import Storage +from django.core.management.base import BaseCommand, CommandError, CommandParser +from django.db import models + +from api.utils.storage import normalize_media_name + +logger = structlog.get_logger("dataspace.commands.migrate_media_to_s3") + + +def iter_file_fields() -> Iterator[Tuple[Type[models.Model], models.FileField]]: + """Yield every concrete file field across installed models.""" + for model in apps.get_models(): + for field in model._meta.get_fields(): + if isinstance(field, models.FileField): + yield model, field + + +def stored_names(model: Type[models.Model], field: models.FileField) -> List[str]: + """Distinct non-empty names stored in ``field``.""" + return list( + model._default_manager.exclude(**{field.name: ""}) + .exclude(**{f"{field.name}__isnull": True}) + .values_list(field.name, flat=True) + .distinct() + ) + + +def copy_to(storage: Storage, target: str, fh: Any) -> None: + """Save ``fh`` under exactly ``target``; the row will point at that name.""" + saved = storage.save(target, File(fh)) + if saved != target: + storage.delete(saved) + raise RuntimeError(f"storage renamed {target} to {saved}") + + +class Command(BaseCommand): + help = "Copy media into S3 under prefix-free names and point DB rows at them." + + def add_arguments(self, parser: CommandParser) -> None: + parser.add_argument( + "--source", + help="Old MEDIA_ROOT that legacy names are relative to (e.g. /code/files/public)", + ) + parser.add_argument( + "--execute", action="store_true", help="Copy and rename; without it, only report." + ) + + def handle(self, *args: Any, **options: Any) -> None: + source: Optional[Path] = None + if options["source"]: + source = Path(options["source"]).resolve() + if not source.is_dir(): + raise CommandError(f"Source directory not found: {source}") + execute: bool = options["execute"] + + counts: Counter = Counter() + seen: Set[str] = set() + for model, field in iter_file_fields(): + storage = field.storage + for name in stored_names(model, field): + target = normalize_media_name(name) + seen.add(target) + try: + present = self._place(storage, name, target, source, execute, counts) + if present and target != name: + counts["rows_renamed"] += 1 + if execute: + model._default_manager.filter(**{field.name: name}).update( + **{field.name: target} + ) + except Exception as exc: + counts["failed"] += 1 + logger.error("media_migration_failed", name=name, error=str(exc)) + + if source: + counts["orphaned"] = sum( + 1 + for p in source.rglob("*") + # Disk files keep their legacy names; rows may already use the clean ones. + if p.is_file() + and normalize_media_name(p.relative_to(source).as_posix()) not in seen + ) + + summary = ", ".join(f"{key}={counts[key]}" for key in sorted(counts)) + logger.info("media_migration_summary", execute=execute, **dict(counts)) + self.stdout.write(f"{'Done' if execute else 'Dry run'}: {summary}") + if counts["failed"]: + raise CommandError(f"{counts['failed']} file(s) failed") + + def _place( + self, + storage: Storage, + name: str, + target: str, + source: Optional[Path], + execute: bool, + counts: Counter, + ) -> bool: + """Make sure ``target`` exists in ``storage``; return whether it does (or would).""" + if storage.exists(target): + counts["in_place"] += 1 + return True + + if target != name and storage.exists(name): + counts["moved_in_s3"] += 1 + if execute: + with storage.open(name, "rb") as fh: + copy_to(storage, target, fh) + return True + + if source is not None: + local = (source / name).resolve() + # Names come from the DB; never read outside the source tree. + if local.is_relative_to(source) and local.is_file(): + counts["copied_from_disk"] += 1 + if execute: + with local.open("rb") as fh: + copy_to(storage, target, fh) + return True + + counts["missing"] += 1 + logger.warning("media_source_missing", name=name) + return False From df06b6a97ecd24b196af6bdab62527d4396b6159 Mon Sep 17 00:00:00 2001 From: Amit Chaudhary Date: Mon, 19 Oct 2026 12:11:17 +0530 Subject: [PATCH 09/11] test(api): cover the move to S3-only media Regression tests for the local-disk/DVC to S3 migration: S3 storage options (unsigned public URLs, 5-minute presigned private URLs, private default), which storage each file field names, local_copy() and MIME checks on storage without paths, prefix-free upload names, the legacy /public redirect (public files only, no traversal), createMajorVersion's owner-only version write, and migrate_media_to_s3 (dry run, disk and old S3 sources, missing files, failed copies). --- tests/test_s3_storage.py | 290 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 290 insertions(+) create mode 100644 tests/test_s3_storage.py diff --git a/tests/test_s3_storage.py b/tests/test_s3_storage.py new file mode 100644 index 00000000..27c0ce58 --- /dev/null +++ b/tests/test_s3_storage.py @@ -0,0 +1,290 @@ +"""Regression tests for the move from local disk (and DVC) to S3-only media. + +Root cause: uploads lived on local disk under MEDIA_ROOT and several code paths +read them through ``FieldFile.path`` (data indexing, upload MIME validation, +search documents, DVC versioning). S3 has no local path, so each of those +would raise ``NotImplementedError`` once media moved to a bucket. DVC never +worked at all: files were never under ``DVC_REPO_PATH``, so every DVC call +raised and ``createMajorVersion`` always failed. + +Fix: media goes through two S3 storages (``public``/``private``, keys prefixed +by ``STORAGE_ENV``); readers that need a real file use ``local_copy()``; old +``/public/`` links redirect to public files only; DVC is removed and +``createMajorVersion`` writes the version row directly behind a permission +check. Stored names drop the ``files/public/``/``files/`` prefix left from the +disk layout (S3 keys already sit under ``/public``); ``migrate_media_to_s3`` +moves existing files to those names and repoints the rows. +""" + +import io +import types +from pathlib import Path +from unittest.mock import patch + +import pytest +from django.apps import apps +from django.core.files.base import ContentFile +from django.core.management import call_command +from django.core.management.base import CommandError +from django.db import models +from django.test import RequestFactory +from django.urls import resolve +from storages.backends.s3 import S3Storage + +from api.models import Dataset, Resource, ResourceFileDetails, ResourceVersion +from api.models.Organization import Organization +from api.schema.schema import schema +from api.utils.file_paths import ( + _chart_image_directory_path, + _organization_directory_path, + _publication_block_directory_path, +) +from api.utils.file_utils import check_mime_type, local_copy +from api.utils.storage import normalize_media_name, private_storage, public_storage +from api.views.legacy_media import legacy_media_redirect +from authorization.models import User +from DataSpace import settings as project_settings + +FAKE_KEYS = {"access_key": "AKIAEXAMPLEKEY000000", "secret_key": "not-a-real-secret"} + + +def _s3(options): + # URL building and presigning are local operations; no request reaches AWS. + return S3Storage(**{**options, **FAKE_KEYS}) + + +class TestS3StorageOptions: + def test_public_urls_are_unsigned_under_env_public_prefix(self): + url = _s3(project_settings._S3_PUBLIC_OPTIONS).url("organizations/a/logo.png") + assert url.endswith(f"/{project_settings.STORAGE_ENV}/public/organizations/a/logo.png") + assert "X-Amz-Signature" not in url + + def test_private_urls_are_presigned_for_five_minutes(self): + url = _s3(project_settings._S3_PRIVATE_OPTIONS).url("resources/data.csv") + assert f"/{project_settings.STORAGE_ENV}/private/resources/data.csv" in url + assert "X-Amz-Signature" in url + assert "X-Amz-Expires=300" in url + + def test_default_storage_is_private(self): + assert project_settings.STORAGES["default"] == project_settings.STORAGES["private"] + + def test_s3_has_no_local_path(self): + with pytest.raises(NotImplementedError): + _s3(project_settings._S3_PRIVATE_OPTIONS).path("resources/data.csv") + + +# A new file field that forgets storage= silently lands in the private default, +# so a public image would 403. Keep this list in step with the models. +PRIVATE_FILE_FIELDS = { + ("api", "resourcefiledetails", "file"), + ("api", "resourcechartimage", "image"), +} + + +class TestFileFieldStorages: + def _file_fields(self): + for model in apps.get_models(): + for field in model._meta.get_fields(): + if isinstance(field, models.FileField): + yield (model._meta.app_label, model._meta.model_name, field.name), field + + def test_every_file_field_names_public_or_private_storage(self): + for key, field in self._file_fields(): + assert field._storage_callable in (public_storage, private_storage), key + + def test_only_resource_files_and_chart_images_are_private(self): + private = { + key for key, field in self._file_fields() if field._storage_callable is private_storage + } + assert private == PRIVATE_FILE_FIELDS + + +@pytest.mark.django_db +class TestLocalCopy: + def _stored(self, name, content): + dataset = Dataset.objects.create(title="Local copy") + resource = Resource.objects.create(dataset=dataset, name="r", slug=f"r-{name}") + details = ResourceFileDetails(resource=resource, format="csv") + details.file.save(name, ContentFile(content), save=False) + return details.file + + def test_yields_a_local_file_with_the_stored_bytes_and_extension(self): + fieldfile = self._stored("data.csv", b"a,b\n1,2\n") + with local_copy(fieldfile) as path: + assert path.endswith(".csv") + assert Path(path).read_bytes() == b"a,b\n1,2\n" + + def test_removes_the_temp_file_even_when_the_caller_raises(self): + fieldfile = self._stored("boom.csv", b"x") + with pytest.raises(RuntimeError): + with local_copy(fieldfile) as path: + raise RuntimeError("boom") + assert not Path(path).exists() + + def test_mime_check_reads_a_stored_file_without_a_path(self): + fieldfile = self._stored("report.pdf", b"%PDF-1.7\n%\xe2\xe3\xcf\xd3\n") + assert check_mime_type(fieldfile) == "application/pdf" + + +class TestLegacyMediaRedirect: + def _get(self, name): + return legacy_media_redirect(RequestFactory().get(f"/public/{name}"), name) + + def test_route_points_at_the_redirect_view(self): + assert resolve("/public/files/public/x.png").func is legacy_media_redirect + + def test_public_file_redirects_to_its_storage_url(self): + name = public_storage().save("organizations/acme/png/logo.png", ContentFile(b"png")) + response = self._get(name) + assert response.status_code == 302 + assert response["Location"] == public_storage().url(name) + + def test_old_prefixed_link_redirects_to_the_renamed_file(self): + name = public_storage().save("organizations/old/png/logo.png", ContentFile(b"png")) + response = self._get(f"files/public/{name}") + assert response.status_code == 302 + assert response["Location"] == public_storage().url(name) + + def test_private_file_is_never_redirected(self): + name = private_storage().save("resources/secret.csv", ContentFile(b"a,b")) + assert self._get(name).status_code == 404 + + @pytest.mark.parametrize("name", ["../etc/passwd", "files/../../etc/passwd", "/etc/passwd"]) + def test_traversal_is_rejected(self, name): + assert self._get(name).status_code == 404 + + +MAJOR_VERSION = """ +mutation($input: CreateMajorVersionInput!) { + createMajorVersion(input: $input) { ... on TypeResource { id } } +} +""" + + +@pytest.mark.django_db +class TestCreateMajorVersionWithoutDvc: + @pytest.fixture + def owner(self): + return User.objects.create(username="owner", keycloak_id="owner") + + @pytest.fixture + def resource(self, owner): + dataset = Dataset.objects.create(title="Versioned", user=owner) + return Resource.objects.create(dataset=dataset, name="data", slug="data-versioned") + + def _run(self, user, resource): + return schema.execute_sync( + MAJOR_VERSION, + variable_values={"input": {"resourceId": str(resource.id), "description": "schema"}}, + context_value=types.SimpleNamespace(user=user, context={}), + ) + + def test_owner_gets_a_version_row_and_bump(self, owner, resource): + result = self._run(owner, resource) + assert result.errors is None, result.errors + resource.refresh_from_db() + assert resource.version == "v1.0.0" + assert ResourceVersion.objects.filter(resource=resource, version_number="v1.0.0").exists() + + def test_a_stranger_cannot_version_someone_elses_resource(self, resource): + stranger = User.objects.create(username="stranger", keycloak_id="stranger") + result = self._run(stranger, resource) + assert result.errors + assert not ResourceVersion.objects.filter(resource=resource).exists() + + +class TestNameLayout: + def test_upload_names_carry_no_legacy_prefix(self): + org = types.SimpleNamespace(name="Acme") + chart = types.SimpleNamespace(name="Rain") + block = types.SimpleNamespace(publication_id="p1") + assert _organization_directory_path(org, "logo.png") == "organizations/Acme/png/logo.png" + assert _chart_image_directory_path(chart, "c.png") == "chart_images/Rain/png/c.png" + assert _publication_block_directory_path(block, "r.pdf").startswith("publications/p1/") + + @pytest.mark.parametrize( + "old, new", + [ + ("files/public/organizations/a/png/l.png", "organizations/a/png/l.png"), + ("files/use_case/Health/logo/l.png", "use_case/Health/logo/l.png"), + ("resources/data.csv", "resources/data.csv"), + ], + ) + def test_normalize_media_name(self, old, new): + assert normalize_media_name(old) == new + + +@pytest.mark.django_db +class TestMigrateMediaToS3: + OLD = "files/public/organizations/acme/png/logo.png" + NEW = "organizations/acme/png/logo.png" + + @pytest.fixture + def storage(self): + storage = Organization._meta.get_field("logo").storage + for name in (self.OLD, self.NEW): + if storage.exists(name): + storage.delete(name) + return storage + + @pytest.fixture + def source(self, tmp_path): + logo = tmp_path / self.OLD + logo.parent.mkdir(parents=True) + logo.write_bytes(b"logo-bytes") + (tmp_path / "stray.txt").write_text("no row points here") + return tmp_path + + @pytest.fixture + def org(self, storage): + return Organization.objects.create(name="Acme", slug="acme", logo=self.OLD) + + def _run(self, *args): + out = io.StringIO() + call_command("migrate_media_to_s3", *args, stdout=out) + return out.getvalue() + + def _read(self, storage, name): + with storage.open(name) as fh: + return fh.read() + + def test_dry_run_writes_nothing(self, source, storage, org): + output = self._run("--source", str(source)) + assert "copied_from_disk=1" in output and "rows_renamed=1" in output + assert not storage.exists(self.NEW) + org.refresh_from_db() + assert org.logo.name == self.OLD + + def test_disk_file_lands_under_the_clean_name_and_row_follows(self, source, storage, org): + assert "copied_from_disk=1" in self._run("--source", str(source), "--execute") + assert self._read(storage, self.NEW) == b"logo-bytes" + org.refresh_from_db() + assert org.logo.name == self.NEW + assert "in_place=1" in self._run("--source", str(source), "--execute") + + def test_old_s3_key_is_copied_to_the_clean_name(self, storage, org): + storage.save(self.OLD, ContentFile(b"from-s3")) + assert "moved_in_s3=1" in self._run("--execute") + assert self._read(storage, self.NEW) == b"from-s3" + assert storage.exists(self.OLD) # nothing is deleted + org.refresh_from_db() + assert org.logo.name == self.NEW + + def test_counts_missing_and_orphaned_files(self, source, storage, org): + Organization.objects.create(name="Gone", slug="gone", logo="files/public/gone.png") + output = self._run("--source", str(source)) + assert "missing=1" in output + assert "orphaned=1" in output + + def test_a_missing_file_keeps_its_row_name(self, storage): + gone = Organization.objects.create(name="Gone", slug="gone", logo="files/public/gone.png") + self._run("--execute") + gone.refresh_from_db() + assert gone.logo.name == "files/public/gone.png" + + def test_a_failed_copy_exits_non_zero(self, source, storage, org): + with patch.object(storage, "save", side_effect=OSError("denied")): + with pytest.raises(CommandError): + self._run("--source", str(source), "--execute") + org.refresh_from_db() + assert org.logo.name == self.OLD From 062eed7bcf62023ce234ff5377760258039d9b60 Mon Sep 17 00:00:00 2001 From: Amit Chaudhary Date: Mon, 19 Oct 2026 12:11:40 +0530 Subject: [PATCH 10/11] docs: describe S3-only media storage Replace the DVC references in SECURITY.md and AGENTS.md with the S3 storage layout and access rules, and add the no-co-author commit rule to AGENTS.md. --- AGENTS.md | 5 +++-- SECURITY.md | 5 ++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 78f32026..01684c19 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -39,7 +39,8 @@ Before you call a change finished: - Cache: Redis (`django-redis`, `django-cacheops`) - Activity feed: `django-activity-stream` - Observability: OpenTelemetry, `structlog` -- Data: pandas, pyarrow, DVC for dataset versioning +- Data: pandas, pyarrow +- Media: S3 only, via `django-storages` (`public`/`private` storages in `api/utils/storage.py`; keys prefixed by `STORAGE_ENV`) - Runtime: uvicorn/gunicorn in Docker, deployed to AWS (ECS / EC2 via `scripts/ci-*.sh`) **File structure:** @@ -51,7 +52,6 @@ Before you call a change finished: - `views/`: REST views (search, downloads, charts, health, auth) - `services/`: business logic kept out of resolvers (publication, platform import, metadata mapping/export, model clients) - `activities/`: activity-stream handlers and display per domain (`dataset.py`, `organization.py`, …), wired up in `registry.py` - - `managers/`: `dvc_manager.py` for dataset versioning - `signals/`: post-save hooks (search indexing, cache invalidation) - `middleware/`, `utils/middleware.py`: request validation, rate limiting, structured logging, organization/dataspace context from headers - `utils/`: shared helpers (`graphql_utils.py`, `keycloak_utils.py`, `search_query.py`, `search_cache.py`, `enums.py`, …) @@ -158,6 +158,7 @@ def update_org(self, info, input): - Branch from `dev` with `fix/…`, `feat/…` or `chore/…`. PRs go into `dev`. Releases go from `dev` to `main`, gated by `pr-gate.yml`. - Do not make any commits utill asked for it. - Use Conventional Commits with a scope: `fix(api): …`, `feat(usecase): …`. +- Never use claude co-author while doing commit. - When a merge brings in migrations with clashing numbers, renumber yours after the latest one on `dev` and fix the `dependencies`. ## Boundaries diff --git a/SECURITY.md b/SECURITY.md index e3caa67a..f3afb381 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -134,21 +134,20 @@ When contributing to this project, please follow these security guidelines: - **GraphQL**: Implements query complexity limits and depth restrictions - **REST API**: Rate limiting and throttling enabled - **File Uploads**: Validated for type, size, and content -- **Data Versioning**: DVC integration for data versioning and tracking ### Data Protection - **Encryption**: All data encrypted in transit (TLS 1.2+) - **Database**: PostgreSQL with proper access controls - **Search**: Elasticsearch with authentication enabled - **Caching**: Redis with secure configuration -- **File Storage**: Secure file handling and validation +- **File Storage**: Media in S3; public assets via unsigned URLs, private files via presigned URLs or gated views ### Third-Party Services - **Keycloak**: Ensure proper realm and client configuration - **Elasticsearch**: Enable security features and authentication - **Redis**: Use password protection and disable dangerous commands - **OpenTelemetry**: Configure to exclude sensitive data -- **DVC**: Secure data versioning and storage +- **S3**: Block public access except the `*/public/*` prefix; least-privilege task role ## Security Checklist for Pull Requests From fa89bfa70595760581fca46aa4d93c06f4d54797 Mon Sep 17 00:00:00 2001 From: Amit Chaudhary Date: Mon, 19 Oct 2026 12:17:21 +0530 Subject: [PATCH 11/11] docs: expand AGENTS.md with setup, checklists and media rules Agents kept tripping over setup the file never mentioned (.env and logs/ are needed before tests boot; pre-commit pins python3.11) and over S3-era rules that lived only in code review. Add a quick start, a commands table with the SDK test command, task checklists for mutations, model changes and bug fixes, a files-and-media section (no FieldFile.path, local_copy, FileResponse, public vs private storage), and git safety rules (self-contained commits, no AI co-author trailer, never stash the user's work). Existing rules are unchanged. --- AGENTS.md | 241 ++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 172 insertions(+), 69 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 01684c19..3f7f4bea 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,40 +1,71 @@ # AGENTS.md -You are a senior engineer on the DataSpace backend. +Instructions for AI coding agents (and humans) working on the DataSpace backend. +Read this file first; the rules here override general habits. -## Persona +You are a senior engineer on this codebase: - You write clean, reusable, maintainable code that reads like the code around it. -- You understand the codebase and its security risks (authorization, multi-tenant organization data, file uploads, search exposure), and you turn that into clear, actionable insights. -- Your output: small, well-tested changes and reviews that a teammate can merge with confidence, plus a short explanation of *why* when the reason isn't obvious. +- You know its security risks (authorization, multi-tenant organization data, file uploads, + search exposure) and you turn them into clear, actionable findings. +- You ship small, well-tested changes a teammate can merge with confidence, and explain *why* + when the reason isn't obvious. + +## Quick start + +```bash +cp .env.example .env # settings fail without it (e.g. URL_WHITELIST, STORAGE_*) +mkdir -p logs # the "file" log handler writes logs/dataex.log +pip install -r requirements.txt +pip install pytest pytest-django # test-only, not in requirements.txt +``` + +- **Run (Docker, hot reload):** `docker compose -f docker-compose.yml -f docker-compose.hotreload.yml up --build` +- **Run (venv):** `python manage.py runserver` ## Commands -- **Run locally (Docker, hot reload):** `docker compose -f docker-compose.yml -f docker-compose.hotreload.yml up --build` -- **Run locally (venv):** copy `.env.example` to `.env`, then `python manage.py runserver` -- **Test:** `pytest tests/` uses SQLite in-memory with `--nomigrations` (see `pytest.ini`). It needs `pytest pytest-django`, which are not in `requirements.txt`. CI runs it with `--ignore=tests/object_types/charts --deselect tests/schema/test_dataset_schema.py::TestAddUpdateDatasetMetadata` (known-broken, see #189). -- **Single test:** `pytest tests/test_slug_collision.py -k usecase` -- **Format:** `black .` and `isort --profile black .` (line length 100, set in `pyproject.toml`; that file is the SDK's packaging config, but black reads it for the whole repo) -- **Type check:** `mypy --config-file=mypy.ini .` -- **All hooks:** `pre-commit run --all-files` (whitespace, black, isort, mypy, `debug-statements`) -- **Migrations:** `python manage.py makemigrations api`, then `python manage.py migrate` -- **Search mappings:** `python manage.py sync_search_mappings` after changing anything in `search/documents/` -- **API testing:** Postman CLI is installed; collections live in `postman/collections/` + +| Task | Command | +|---|---| +| Tests (as CI runs them) | `pytest tests/ --ignore=tests/object_types/charts --deselect tests/schema/test_dataset_schema.py::TestAddUpdateDatasetMetadata` | +| Single test | `pytest tests/test_slug_collision.py -k usecase` | +| SDK tests (no Django) | `pytest tests/test_auth.py tests/test_base.py tests/test_client.py tests/test_datasets.py tests/test_aimodels.py tests/test_usecases.py tests/test_exceptions.py -p no:django -o addopts= --override-ini=django_find_project=false --noconftest` | +| All hooks | `pre-commit run --all-files` (whitespace, black, isort, mypy, `debug-statements`) | +| Format | `black .` and `isort --profile black .` (line length 100, from `pyproject.toml`) | +| Type check | `mypy --config-file=mypy.ini .` | +| Migrations | `python manage.py makemigrations api`, then `python manage.py migrate` | +| Search mappings | `python manage.py sync_search_mappings` after any change in `search/documents/` | +| API testing | Postman CLI; collections live in `postman/collections/` | + +Notes: +- Tests use SQLite in memory with `--nomigrations` (`pytest.ini`, settings in + `tests/test_settings.py`). All media storages are `InMemoryStorage`, so tests never reach S3. +- The CI exclusions above are known-broken tests (#189). Don't add to them. +- pre-commit hooks pin `python3.11`. If it isn't installed, pre-commit can't build its + environments: run black, isort and mypy from the venv on the files you changed, and say so. +- mypy results depend on its cache. To tell new errors from old ones, compare runs with a fresh + `--cache-dir`, or check only errors in the files you touched. ## Definition of done -Before you call a change finished: -1. `pre-commit run --all-files` is clean. -2. The tests pass with the CI flags above, and a bug fix has a regression test. -3. Every new or changed GraphQL query/mutation has the right permission guard (see Rules). -4. If a model changed: its search document, signals, cache invalidation and SDK still match. + +A change is finished only when: +1. `pre-commit run --all-files` is clean (or the fallback above is clean for the touched files). +2. Tests pass with the CI flags, and a bug fix has a regression test. +3. Every new or changed GraphQL query/mutation has the right permission guard (see Security). +4. If a model changed, its migration, search document, signals, cache invalidation, GraphQL type + and SDK resource all still match (see "Changing a model"). ## Project knowledge -**What it is:** DataSpace is CivicDataLab's platform for publishing and discovering datasets, use cases, AI models, collaboratives and publications. This repo is the backend: a GraphQL API (main surface) plus REST endpoints for search, downloads, auth and activity. It also ships a Python client, `dataspace_sdk`, published to PyPI. +**What it is:** DataSpace is CivicDataLab's platform for publishing and discovering datasets, +use cases, AI models, collaboratives and publications. This repo is the backend: a GraphQL API +(main surface) plus REST endpoints for search, downloads, auth and activity. It also ships a +Python client, `dataspace_sdk`, published to PyPI. **Tech stack:** - Python 3.11, Django 5.0, PostgreSQL - GraphQL: `strawberry-graphql` 0.235 + `strawberry-graphql-django` 0.42 (endpoint `/api/graphql`) - REST: Django REST Framework 3.14, `drf-yasg` for API docs -- Auth: Keycloak (`python-keycloak` 5.5); every request needs a real Keycloak JWT, with no dev-mode fallback +- Auth: Keycloak (`python-keycloak` 5.5). Every request needs a real Keycloak JWT; there is no dev-mode fallback. - Search: Elasticsearch 8 via `django-elasticsearch-dsl` - Cache: Redis (`django-redis`, `django-cacheops`) - Activity feed: `django-activity-stream` @@ -43,58 +74,121 @@ Before you call a change finished: - Media: S3 only, via `django-storages` (`public`/`private` storages in `api/utils/storage.py`; keys prefixed by `STORAGE_ENV`) - Runtime: uvicorn/gunicorn in Docker, deployed to AWS (ECS / EC2 via `scripts/ci-*.sh`) -**File structure:** +**Layout:** - `DataSpace/`: Django project (`settings.py`, `urls.py`, `asgi.py`, `cache_settings.py`) - `api/`: main app - - `models/`: one model per file (`Dataset.py`, `Organization.py`, `UseCase.py`, …), exported from `models/__init__.py` - - `schema/`: Strawberry `Query`/`Mutation` classes per domain (`*_schema.py`), stitched together in `schema/schema.py`. `base_mutation.py` holds `BaseMutation`/`MutationResponse`. `extensions.py` holds `TrackActivity`/`TrackModelActivity`. + - `models/`: one model per file (`Dataset.py`, `UseCase.py`, …), exported from `models/__init__.py` + - `schema/`: Strawberry `Query`/`Mutation` classes per domain (`*_schema.py`), merged with + `merge_types` in `schema/schema.py`. `base_mutation.py` holds `BaseMutation`/`MutationResponse`; + `extensions.py` holds `TrackActivity`/`TrackModelActivity`. - `types/`: Strawberry output types (`type_*.py`) - - `views/`: REST views (search, downloads, charts, health, auth) + - `views/`: REST views (search, downloads, charts, health, auth, legacy media redirect) - `services/`: business logic kept out of resolvers (publication, platform import, metadata mapping/export, model clients) - - `activities/`: activity-stream handlers and display per domain (`dataset.py`, `organization.py`, …), wired up in `registry.py` - - `signals/`: post-save hooks (search indexing, cache invalidation) + - `activities/`: activity-stream handlers and display per domain, wired up in `registry.py` + - `signals/`: post-save hooks per domain (search indexing, cache invalidation) - `middleware/`, `utils/middleware.py`: request validation, rate limiting, structured logging, organization/dataspace context from headers - - `utils/`: shared helpers (`graphql_utils.py`, `keycloak_utils.py`, `search_query.py`, `search_cache.py`, `enums.py`, …) - - `management/commands/`: `sync_search_mappings`, `update_dataindex`, `seed_resource_types`, `populate_geographies`, … + - `utils/`: shared helpers (`graphql_utils.py`, `keycloak_utils.py`, `storage.py`, `file_utils.py`, `file_paths.py`, `search_query.py`, `search_cache.py`, `enums.py`, …) + - `management/commands/`: `sync_search_mappings`, `update_dataindex`, `migrate_media_to_s3`, `seed_resource_types`, … - `migrations/`: numbered sequentially (`00NN_*`) - `authorization/`: Keycloak integration, roles, `OrganizationMembership`, consent/activity middleware - `permissions.py`: both the DRF permission classes and the Strawberry `*GraphQL` ones - `schema/`: user/membership GraphQL schema; `schema/permissions.py` holds `HasOrganizationAdminRole` - `graphql_permissions.py`: legacy duplicate of the GraphQL classes; don't add new uses - `search/documents/`: Elasticsearch document definitions and analysers -- `dataspace_sdk/`: standalone Python SDK (its own version in `pyproject.toml`; depends only on `requests`) -- `tests/`: pytest suite (`test_*.py`, `tests/schema/`, `tests/journeys/`), settings in `tests/test_settings.py` +- `dataspace_sdk/`: standalone Python SDK (`client.py`, `resources/.py`; own version in `pyproject.toml`; depends only on `requests`) +- `tests/`: pytest suite (`test_*.py`, `tests/schema/`, `tests/journeys/`). `test_auth.py`, `test_base.py`, `test_client.py`, `test_datasets.py`, `test_aimodels.py`, `test_usecases.py` and `test_exceptions.py` test the SDK. - `docs/`: feature docs (Keycloak, unified search, charts, activity stream, SDK) -- `postman/`: Postman collections and fixtures for API testing +- `postman/`: Postman collections and fixtures - `aws/`, `scripts/`, `.github/workflows/`: infra, deploy scripts, CI -- Root-level `migrate_*.py`, `populate_sdgs.py`, `test_geography_hierarchy.py`: one-off scripts. Leave them alone, and put new code in `api/management/commands/` instead. - -## Standards - -**Naming conventions:** -- Functions, methods, variables: snake_case (`create_organization`, `get_current_user`) -- Classes: PascalCase (`OrganizationInput`, `HasOrganizationRole`) -- Constants: UPPER_SNAKE_CASE (`MAX_RETRIES`) -- Strawberry output types: `Type` in `api/types/type_.py` -- Model files: PascalCase, one model per file (`api/models/UseCase.py`) -- GraphQL fields are snake_case in Python. Strawberry exposes them as camelCase (`create_organization` → `createOrganization`). - -**Rules:** -- Type-annotate every function. mypy runs in pre-commit, so don't add new `# type: ignore` or `mypy.ini` exemptions without a reason in a comment. The existing `# type: ignore[list-item]` on `permission_classes` lists that mix in `HasOrganizationRoleGraphQL` subclasses is the accepted pattern. - - mypy skips `tests/` and is relaxed for `api.models`, `api.activities` and the resource, use case and dataset schemas (see `mypy.ini`). Still annotate new code there. -- Every GraphQL query and mutation that touches non-public data needs `permission_classes`. Don't rely on the ORM to reject anonymous users. - - Import from `authorization.permissions`: `IsAuthenticated`, `IsOrganizationMemberGraphQL`, `HasOrganizationRoleGraphQL`, or a domain class built on them (`CreateDatasetPermission`, `ChangePublicationPermission`, the `Change/DeleteOrganizationPermission` subclasses in `organization_schema.py`, …). - - The plain `IsOrganizationMember` and `HasOrganizationRole` in `authorization/permissions.py` are DRF classes for REST views. They don't work in Strawberry `permission_classes`. Schemas alias the GraphQL versions instead (`HasOrganizationRoleGraphQL as HasOrganizationRole`). - - `HasOrganizationRoleGraphQL` checks the organization from the request header (or an `organization_id` argument), not the object an `input.id` points at. When a mutation loads an object by id, also check the caller's membership in that object's organization inside the resolver. -- Scope querysets to what the user can see (organization membership, dataspace, published status). Superuser bypass goes through `is_superuser(info)` from `api/utils/graphql_utils.py`. -- Keep resolvers thin. Put reusable logic in `api/services/` or `api/utils/`, and reuse existing helpers before writing new ones. +- Root-level `migrate_*.py`, `populate_sdgs.py`, `test_geography_hierarchy.py`: one-off scripts. + Leave them alone; new scripts go in `api/management/commands/`. + +## Code standards + +**Naming:** +- Functions, methods, variables: snake_case. Classes: PascalCase. Constants: UPPER_SNAKE_CASE. +- Strawberry output types: `Type` in `api/types/type_.py`. +- Model files: PascalCase, one model per file (`api/models/UseCase.py`). +- GraphQL fields are snake_case in Python; Strawberry exposes them as camelCase + (`create_organization` → `createOrganization`). + +**Style:** +- Type-annotate every function. Don't add `# type: ignore` or `mypy.ini` exemptions without a + reason in a comment. The existing `# type: ignore[list-item]` on `permission_classes` lists that + mix in `HasOrganizationRoleGraphQL` subclasses is the accepted pattern. + mypy skips `tests/` and is relaxed for `api.models`, `api.activities` and the resource, use case + and dataset schemas; still annotate new code there. +- Keep resolvers thin. Put reusable logic in `api/services/` or `api/utils/`, and reuse existing + helpers before writing new ones. - Wrap multi-step writes in `transaction.atomic()`. -- Return clean client errors (400/404, or `MutationResponse.error_response`) instead of letting `IntegrityError`, `ValueError` or `DoesNotExist` surface as a 500. -- Log with the existing logger and `structlog`, without tokens or PII. -- Comments explain *why* (constraints, past incidents), not *what*. Match the density of the surrounding file. -- Write regression tests in the style of `tests/test_slug_collision.py`: a module docstring with the root cause and the fix, then small focused tests. +- Return clean client errors (400/404, or `MutationResponse.error_response`). Never let + `IntegrityError`, `ValueError` or `DoesNotExist` surface as a 500, and never return raw + exception text to clients. +- Log with the module's existing `structlog` logger. Never log tokens, secrets or PII. +- Comments explain *why* (constraints, past incidents), not *what*. Match the density of the + surrounding file. +- Make the smallest change that solves the problem. Don't reformat, rename or refactor + unrelated code in the same change. + +## Security + +- Every GraphQL query and mutation that touches non-public data needs `permission_classes`. + Don't rely on the ORM to reject anonymous users. + - Import from `authorization.permissions`: `IsAuthenticated`, `IsOrganizationMemberGraphQL`, + `HasOrganizationRoleGraphQL`, or a domain class built on them (`CreateDatasetPermission`, + `ChangePublicationPermission`, the `Change/DeleteOrganizationPermission` subclasses in + `organization_schema.py`, …). + - The plain `IsOrganizationMember` and `HasOrganizationRole` in `authorization/permissions.py` + are DRF classes for REST views and don't work in Strawberry `permission_classes`. Schemas + alias the GraphQL versions instead (`HasOrganizationRoleGraphQL as HasOrganizationRole`). + - `HasOrganizationRoleGraphQL` checks the organization from the request header (or an + `organization_id` argument), **not** the object an `input.id` points at. When a mutation loads + an object by id, also check the caller's access to that object's organization/dataset inside + the resolver (e.g. `AuthorizationService.check_dataset_permission`). +- Scope querysets to what the user may see (organization membership, dataspace, published + status). Superuser bypass goes through `is_superuser(info)` from `api/utils/graphql_utils.py`. +- Search must not expose non-public objects. Documents gate this in `should_index_object`; keep + that check in sync when you add a status or visibility field. + +## Files and media + +- Media lives only in S3. Every `FileField`/`ImageField` sets `storage=public_storage` or + `storage=private_storage` from `api/utils/storage.py`. Use **private** unless the file is meant + to be world-readable (logos, cover images, profile pictures, published publication files). +- Never use `FieldFile.path`: S3 has no local path (and tests use `InMemoryStorage`). Use + `file.open("rb")`/`file.chunks()`, `file.storage.exists(file.name)`, or + `api.utils.file_utils.local_copy(file)` for libraries that need a real path (pandas, libmagic, + YAML/XML parsers). +- Serve downloads with `FileResponse(file.open("rb"))`, not `HttpResponse(file.read())`. +- `upload_to` helpers in `api/utils/file_paths.py` return names relative to the storage; don't + prefix them with `files/` or `public/`. +- Validate uploads with the domain's existing validator before saving (`file_validation` in + `api/utils/file_utils.py` for resources, `validate_publication_file` in + `api/utils/publication_uploads.py` for publication blocks). Don't trust the extension alone. + +## Task checklists + +**Adding or changing a GraphQL mutation** +1. Put it in the domain's `api/schema/_schema.py`; logic in `api/services/` if non-trivial. +2. Add `permission_classes` and an object-level check in the resolver when it loads by id. +3. Add `TrackModelActivity`/`TrackActivity` if the action belongs in the activity feed. +4. Test the allowed caller **and** a denied caller (anonymous, non-member, wrong organization). + +**Changing a model** (ask first: see Boundaries) +1. Edit `api/models/.py`, then `python manage.py makemigrations api` and review the file. +2. Update the `Type` in `api/types/` and any input types in the schema. +3. Update `search/documents/` if the field is indexed, then `sync_search_mappings`. +4. Check `api/signals/` and cache invalidation still cover the change. +5. Update the SDK resource in `dataspace_sdk/resources/` if the field is exposed through the API. + +**Fixing a bug** +1. Reproduce it with a failing test first. +2. Write the regression test in the style of `tests/test_slug_collision.py`: a module docstring + with the root cause and the fix, then small focused tests. +3. Fix the cause, not the symptom. + +## Code style example -**Code style example:** ```python from api.schema.extensions import TrackModelActivity from api.utils.graphql_utils import is_superuser @@ -154,26 +248,35 @@ def update_org(self, info, input): return org ``` -## Git & PRs -- Branch from `dev` with `fix/…`, `feat/…` or `chore/…`. PRs go into `dev`. Releases go from `dev` to `main`, gated by `pr-gate.yml`. -- Do not make any commits utill asked for it. -- Use Conventional Commits with a scope: `fix(api): …`, `feat(usecase): …`. -- Never use claude co-author while doing commit. -- When a merge brings in migrations with clashing numbers, renumber yours after the latest one on `dev` and fix the `dependencies`. +## Git and PRs + +- Don't commit until asked. +- Branch from `dev` with `fix/…`, `feat/…` or `chore/…`. PRs go into `dev`. Releases go from + `dev` to `main`, gated by `pr-gate.yml`. +- Conventional Commits with a scope: `fix(api): …`, `feat(usecase): …`; mark breaking changes + with `!` and a `BREAKING CHANGE:` footer. The body says *why*, not a list of files. +- Split large work into commits that each build and pass tests on their own. +- Never add a Claude (or any AI) `Co-Authored-By` trailer to commits. +- Never `git stash`, `reset` or `checkout --` the user's uncommitted work. To compare against a + baseline, use `git worktree add HEAD` (copy `.env` and create `logs/` there). +- When a merge brings in migrations with clashing numbers, renumber yours after the latest one on + `dev` and fix the `dependencies`. ## Boundaries -- ✅ **Always:** write code in `api/`, `authorization/`, `search/`, `dataspace_sdk/` and `tests/`. Run `pytest` and `pre-commit` before committing. + +- ✅ **Always:** work in `api/`, `authorization/`, `search/`, `dataspace_sdk/` and `tests/`. Run + the tests and hooks before saying a change is done. - ⚠️ **Ask first:** - - model or schema changes and new migrations + - model or schema changes, and new migrations - changes to permission classes, Keycloak/auth middleware or rate limits - changes to Elasticsearch mappings (these need a reindex) - - adding or upgrading dependencies + - adding, removing or upgrading dependencies - bumping the SDK version or publishing it - touching `.github/workflows/`, `aws/`, `scripts/ci-*.sh`, `Dockerfile` or `docker-compose.yml` - - anything that runs against a deployed environment + - anything that runs against a deployed environment or a real S3 bucket - 🚫 **Never:** - commit `.env`, secrets, Keycloak client secrets or tokens. Use `.env.example` for new variables. - edit or delete already-merged migrations. Add a new migration instead. - - disable an auth check or a failing test to get green. Fix the cause, or ask. + - disable an auth check, skip or delete a failing test to get green. Fix the cause, or ask. - edit `.venv/`, `files/` (user uploads) or `logs/` - force-push to `dev` or `main`