JensandClaude Opus 5
7c052a339e
Two problems of the same shape: information about *why* something failed being replaced by something vaguer. get_dataset_geojson wrapped the JSON parse, the metadata read, the CRS resolution and the canonicalisation in one try and reported all of it as "Stored dataset is not valid JSON" with a 500. An operator whose dataset had an unusable CRS was sent to inspect a file that parses perfectly well, and the canonicaliser's own AppError — with its code and its status — never reached them. Only the parse is now inside that handler; everything after it keeps the error it raised, and a genuine bug becomes a distinct 500 rather than a mislabelled client error. A guard finds the same shape elsewhere: catching Exception around a call into another component and relabelling what it reported. Wrapping one's own private helper stays legitimate and the guard says so. The redirect policy was split without anyone saying so. Two acquisition services rejected every redirect through a hand-rolled opener, while eight allowed a same-origin one through the shared guard — and only the latter checked where the response came from. Both live in the guard now, and the strict path uses the rejecting handler rather than the guard's after-the-fact check: objecting to response.url means urllib already opened the connection and read the body, which for a metadata endpoint is the whole attack. That was a weakening I introduced in this same commit's first draft. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
126 lines
4.2 KiB
Python
126 lines
4.2 KiB
Python
"""A precise failure must not be reported as a generic one.
|
|
|
|
``get_dataset_geojson`` wrapped the JSON parse, the metadata read, the CRS
|
|
resolution and the canonicalisation in one ``try``, and reported everything as
|
|
"Stored dataset is not valid JSON" with a 500. An operator whose dataset has an
|
|
unusable CRS was sent to inspect a file that parses perfectly well, and the
|
|
specific AppError the canonicaliser raised — with its own code and status —
|
|
never reached them.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import json
|
|
from pathlib import Path
|
|
from uuid import uuid4
|
|
|
|
import pytest
|
|
|
|
from app.core.errors import AppError
|
|
from app.models import Dataset
|
|
from app.services.dataset_service import DatasetService
|
|
from app.services.vector_feature_service import VectorFeatureService
|
|
|
|
|
|
class FakeSession:
|
|
def __init__(self, dataset: Dataset) -> None:
|
|
self.dataset = dataset
|
|
|
|
def get(self, _model, item_id):
|
|
return self.dataset if item_id == self.dataset.id else None
|
|
|
|
|
|
def _dataset(tmp_path: Path, payload: str) -> Dataset:
|
|
path = tmp_path / "features.geojson"
|
|
path.write_text(payload, encoding="utf-8")
|
|
return Dataset(
|
|
id=uuid4(),
|
|
project_id=uuid4(),
|
|
name="features.geojson",
|
|
dataset_type="geojson",
|
|
source="test",
|
|
status="ready",
|
|
storage_path=str(path),
|
|
crs="EPSG:4326",
|
|
)
|
|
|
|
|
|
def test_a_valid_dataset_is_returned(tmp_path: Path) -> None:
|
|
dataset = _dataset(
|
|
tmp_path,
|
|
json.dumps(
|
|
{
|
|
"type": "FeatureCollection",
|
|
"features": [
|
|
{
|
|
"type": "Feature",
|
|
"geometry": {"type": "Point", "coordinates": [5.0, 51.0]},
|
|
"properties": {},
|
|
}
|
|
],
|
|
}
|
|
),
|
|
)
|
|
|
|
result = DatasetService.get_dataset_geojson(FakeSession(dataset), dataset.id)
|
|
|
|
assert result["type"] == "FeatureCollection"
|
|
|
|
|
|
def test_unparseable_bytes_are_reported_as_invalid_json(tmp_path: Path) -> None:
|
|
dataset = _dataset(tmp_path, "{ not json")
|
|
|
|
with pytest.raises(AppError) as exc_info:
|
|
DatasetService.get_dataset_geojson(FakeSession(dataset), dataset.id)
|
|
|
|
assert exc_info.value.code == "INVALID_GEOJSON"
|
|
assert "JSON" in exc_info.value.message
|
|
|
|
|
|
def test_a_parseable_file_that_is_not_a_feature_collection_says_so(tmp_path: Path) -> None:
|
|
"""Valid JSON, wrong shape. Telling the operator it is not JSON sends them
|
|
to inspect a file that parses perfectly well."""
|
|
|
|
dataset = _dataset(tmp_path, json.dumps({"type": "Feature", "geometry": None}))
|
|
|
|
with pytest.raises(AppError) as exc_info:
|
|
DatasetService.get_dataset_geojson(FakeSession(dataset), dataset.id)
|
|
|
|
assert exc_info.value.code == "INVALID_GEOJSON"
|
|
assert "FeatureCollection" in exc_info.value.message
|
|
# The canonicaliser's own status survives; it is a bad request, not a
|
|
# server fault.
|
|
assert exc_info.value.status_code == 400
|
|
|
|
|
|
def test_a_canonicalisation_failure_keeps_its_own_code(tmp_path: Path, monkeypatch) -> None:
|
|
dataset = _dataset(tmp_path, json.dumps({"type": "FeatureCollection", "features": []}))
|
|
|
|
def failing(*_args, **_kwargs):
|
|
raise AppError(code="INVALID_CRS", message="Unusable source CRS", status_code=409)
|
|
|
|
monkeypatch.setattr(VectorFeatureService, "canonicalize_geojson_payload", staticmethod(failing))
|
|
|
|
with pytest.raises(AppError) as exc_info:
|
|
DatasetService.get_dataset_geojson(FakeSession(dataset), dataset.id)
|
|
|
|
assert exc_info.value.code == "INVALID_CRS"
|
|
assert exc_info.value.status_code == 409
|
|
|
|
|
|
def test_an_unexpected_failure_still_becomes_a_server_error(tmp_path: Path, monkeypatch) -> None:
|
|
"""A genuine bug must not masquerade as a client mistake."""
|
|
|
|
dataset = _dataset(tmp_path, json.dumps({"type": "FeatureCollection", "features": []}))
|
|
|
|
def exploding(*_args, **_kwargs):
|
|
raise RuntimeError("pyproj blew up")
|
|
|
|
monkeypatch.setattr(VectorFeatureService, "canonicalize_geojson_payload", staticmethod(exploding))
|
|
|
|
with pytest.raises(AppError) as exc_info:
|
|
DatasetService.get_dataset_geojson(FakeSession(dataset), dataset.id)
|
|
|
|
assert exc_info.value.status_code == 500
|
|
assert exc_info.value.code == "DATASET_GEOJSON_UNREADABLE"
|