Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions datastore/services/write.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
from __future__ import annotations

import asyncio
import json
from datetime import datetime, timezone
from typing import TYPE_CHECKING, Any

from frictionless import Resource, Schema
from frictionless.exception import FrictionlessException

from datastore.core.exceptions import ValidationError
from datastore.infrastructure.engines import get_datastore_engine
from datastore.schemas.responses import (
DatastoreCreateResponse,
Expand All @@ -21,6 +26,50 @@ def _utc_now_iso() -> str:
return datetime.now(timezone.utc).replace(tzinfo=None).isoformat()


def _validate_records(
resource_id: str,
schema: dict[str, Any],
records: list[dict[str, Any]],
) -> None:
"""Validate in-memory records against a stored Frictionless schema.

This deliberately lives in the service layer so every storage backend gets
the same row validation behavior. Backends only provide the canonical
schema through `InfoResult` and remain responsible for persistence.
"""
if not records or not schema.get("fields"):
return

try:
schema_obj = Schema.from_descriptor(schema)
resource = Resource(data=records, schema=schema_obj, format="inline")
report = resource.validate(
parallel=False,
limit_errors=1000,
)
except FrictionlessException as exc:
raise ValidationError(
f"Stored schema for resource {resource_id!r} failed validation: {exc}"
) from exc

if report.valid:
return

report_dict = report.to_dict()
messages: list[str] = []
for task in report_dict.get("tasks", []):
for error in task.get("errors", []):
message = error.get("message") or "Invalid record"
messages.append(str(message))

error_count = report.stats.get("errors", len(messages))
raise ValidationError(
f"Records failed Frictionless validation for resource {resource_id!r}: "
f"{error_count} error(s)",
fields={"records": messages or [json.dumps(report_dict)]},
)


async def _sync_resource_to_ckan(
context: RequestContext,
resource_id: str,
Expand Down Expand Up @@ -66,6 +115,7 @@ async def create_datastore(
include_total = bool(data_dict.get("include_total", False))

fields, primary_key = frictionless_schema_to_fields(schema)
_validate_records(resource_id=str(resource or "new-resource"), schema=schema, records=records)

if isinstance(resource, dict):
# Endpoint gates this branch on AUTH_TYPE=ckan, so context.ckan is
Expand Down Expand Up @@ -122,6 +172,9 @@ async def upsert_datastore(
include_total = bool(data_dict.get("include_total", False))

engine = get_datastore_engine(context, mode="rw")
info = await asyncio.to_thread(engine.info, resource_id)
_validate_records(resource_id=resource_id, schema=info.schema, records=records)

Comment on lines +175 to +177

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the service read/write boundary.
rg -n -C 8 'engine\.info|engine\.upsert|async def upsert_datastore' datastore/services/write.py

# Inspect engine implementations for transactions or shared synchronization
# that cover schema reads, schema changes, and upserts.
fd -t f -e py . datastore/infrastructure/engines | while IFS= read -r file; do
  rg -n -C 5 'def (info|upsert|create|delete)\b|transaction|Lock|lock|mutex|semaphore' "$file" || true
done

Repository: datopian/datastore

Length of output: 7561


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the full service boundary and engine implementation around the relevant methods.
sed -n '140,195p' datastore/services/write.py
printf '\n--- BigQuery engine relevant methods ---\n'
sed -n '428,490p' datastore/infrastructure/engines/bigquery.py
sed -n '865,925p' datastore/infrastructure/engines/bigquery.py
sed -n '280,340p' datastore/infrastructure/engines/bigquery.py

printf '\n--- Shared lock/mutex candidates ---\n'
rg -n "\bclass .*Lock\b|\b[Aa]syncIO[Aa]ll\w*Lock\b|\bLock\(|\bMutex|threading\..*Lock|asyncio\.(Lock|Semaphore)|Semaphore|condition|Event|contextlib\." datastore/infrastructure datastore/services datastore/core datastore/api datastore/main.py || true

printf '\n--- Other upsert calls to get schema ---\n'
rg -n -C 5 'engine\.info\(|_validate_records\(|\.upsert\(' datastore/services/write.py datastore/api datastore/main.py datastore/tests 2>/dev/null || true

Repository: datopian/datastore

Length of output: 2181


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf 'Files:\n'
git ls-files 'datastore/infrastructure/engines/*.py' 'datastore/services/write.py'

printf '\n--- BigQuery-related files ---\n'
fd -t f -e py 'bigquery|engine|engines' datastore/infrastructure datastore/core datastore/api datastore/services

printf '\n--- Upstream engine info/upsert definitions ---\n'
rg -n -C 8 'def (info|upsert|delete|create)\b|transaction|Lock|lock|mutex|semaphore|BEGIN|COMMIT|rollback' datastore/infrastructure/**/*.py datastore/services/*.py || true

printf '\n--- write.py relevant area ---\n'
sed -n '160,200p' datastore/services/write.py

printf '\n--- upsert calls and info usages ---\n'
rg -n -C 5 'engine\.info\(|\.upsert\(|_validate_records\(' datastore/services datastore/api datastore/main.py datastore/tests 2>/dev/null || true

Repository: datopian/datastore

Length of output: 12810


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '--- backend.py methods (info/upsert/metadata/schema alters) ---\n'
rg -n -C 10 'def (info|upsert|create|delete|_.*schema|_alter|_apply_schema|_validate|BEGIN|COMMIT|transaction|DELETE_COLUMN|SCHEMA|ALTER TABLE)' datastore/infrastructure/engines/bigquery/backend.py

printf '\n--- backend.py line count ---\n'
wc -l datastore/infrastructure/engines/bigquery/backend.py

printf '\n--- relevant backend slices ---\n'
sed -n '1,160p' datastore/infrastructure/engines/bigquery/backend.py
sed -n '340,470p' datastore/infrastructure/engines/bigquery/backend.py
sed -n '720,870p' datastore/infrastructure/engines/bigquery/backend.py
sed -n '870,950p' datastore/infrastructure/engines/bigquery/backend.py

Repository: datopian/datastore

Length of output: 247


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- backend.py methods (info/upsert/metadata/schema alters) ---'
rg -n -C 10 'def (info|upsert|create|delete|_.*schema|_alter|_apply_schema|_validate|BEGIN|COMMIT|transaction|DELETE_COLUMN|SCHEMA|ALTER TABLE)' datastore/infrastructure/engines/bigquery/backend.py || true

printf '%s\n' '--- backend.py line count ---'
wc -l datastore/infrastructure/engines/bigquery/backend.py

printf '%s\n' '--- relevant backend slices ---'
sed -n '1,160p' datastore/infrastructure/engines/bigquery/backend.py
sed -n '340,470p' datastore/infrastructure/engines/bigquery/backend.py
sed -n '720,950p' datastore/infrastructure/engines/bigquery/backend.py

printf '%s\n' '--- global lock/mutex/transaction references ---'
rg -n -C 5 'Lock\(|asyncio\.Lock|Semaphore|contextlib\.|threading\.[A-Za-z]*Lock|transaction|BEGIN|COMMIT|ROLLBACK|rollback|_lock|lock' datastore/infrastructure/engines datastore/services datastore/main.py || true

Repository: datopian/datastore

Length of output: 35854


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- BigQueryBackend.upsert full implementation ---'
sed -n '460,570p' datastore/infrastructure/engines/bigquery/backend.py

printf '%s\n' '--- BigQueryBackend.create relevant implementation ---'
sed -n '428,460p' datastore/infrastructure/engines/bigquery/backend.py

printf '%s\n' '--- merge DML/schema helpers ---'
sed -n '280,330p' datastore/infrastructure/engines/bigquery/lib.py
sed -n '950,1030p' datastore/infrastructure/engines/bigquery/lib.py

Repository: datopian/datastore

Length of output: 7356


Make the upsert schema read and the BigQuery write atomic.

datastore.services.write.upsert_datastore validates records with engine.info(resource_id).schema, but BigQueryBackend.upsert() reads the schema again before running MERGE/INSERT. A concurrent schema change can make the service validator apply to an old schema while the write succeeds with a different schema, or reject a row that would be valid under the current backend schema. Keep the schema read and the BigQuery DML in one backend-managed atomic block.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@datastore/services/write.py` around lines 175 - 177, Update upsert_datastore
and BigQueryBackend.upsert so the schema is read once within a backend-managed
atomic operation and reused for both _validate_records and the subsequent
MERGE/INSERT DML. Remove the separate pre-validation engine.info read, and pass
the validated records or validation callback into the atomic backend flow while
preserving existing validation behavior.

write_result = await asyncio.to_thread(
engine.upsert,
resource_id=resource_id,
Expand Down
69 changes: 69 additions & 0 deletions tests/test_write_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

import pytest
from datastore.core.config import Config
from datastore.core.exceptions import ValidationError
from datastore.infrastructure.engines.base import InfoResult
from datastore.services.write import (
create_datastore,
delete_datastore,
Expand Down Expand Up @@ -77,6 +79,29 @@ def test_existing_resource_skips_resource_create() -> None:
assert ctx.ckan.created == [] # no CKAN call


def test_create_rejects_invalid_records_before_creating_resource() -> None:
ctx = _ctx()
data_dict = {
"package": {"id": "pkg-1"},
"resource": {"package_id": "pkg-1", "name": "foo"},
"schema": {
"fields": [
{
"name": "status",
"type": "string",
"constraints": {"enum": ["active", "inactive"]},
}
]
},
"records": [{"status": "pending"}],
}

with pytest.raises(ValidationError, match="Frictionless validation"):
asyncio.run(create_datastore(ctx, data_dict))

assert ctx.ckan.created == []


def test_new_resource_creates_via_ckan() -> None:
ctx = _ctx()
data_dict = {
Expand Down Expand Up @@ -288,6 +313,50 @@ def test_upsert_returns_typed_result() -> None:
assert result.total is None


def test_upsert_rejects_invalid_records_before_engine_write(
monkeypatch: pytest.MonkeyPatch,
) -> None:
from datastore.infrastructure.engines.bigquery import BigQueryBackend

schema = {
"fields": [
{
"name": "amount",
"type": "number",
"constraints": {"minimum": 0, "maximum": 100},
}
]
}
writes: list[list[dict[str, Any]]] = []

monkeypatch.setattr(
BigQueryBackend,
"info",
lambda self, resource_id: InfoResult(schema=schema, meta={}),
)

def fake_upsert(self: Any, **kwargs: Any) -> dict[str, Any]:
writes.append(kwargs["records"])
return {"total": 1}

monkeypatch.setattr(BigQueryBackend, "upsert", fake_upsert)

with pytest.raises(ValidationError) as exc_info:
asyncio.run(
upsert_datastore(
_ctx(),
{
"resource_id": "res-1",
"records": [{"amount": 101}],
"method": "upsert",
},
)
)

assert "maximum" in exc_info.value.fields["records"][0]
assert writes == []


def test_upsert_default_method_is_upsert() -> None:
"""`method` is optional; absence resolves to 'upsert' inside the service."""
ctx = _ctx()
Expand Down
Loading