diff --git a/docs/api.md b/docs/api.md index 12bb9635..b8549d01 100644 --- a/docs/api.md +++ b/docs/api.md @@ -46,6 +46,7 @@ POST /projects/{project_id}/schema/versions GET /projects/{project_id}/schema/versions GET /projects/{project_id}/schema/versions/{version} GET /projects/{project_id}/schema/compare ?from=&to= +POST /projects/{project_id}/schema/preview would this publish? POST /projects/{project_id}/sources/images multipart POST /projects/{project_id}/sources/video multipart GET /projects/{project_id}/sources @@ -587,6 +588,59 @@ in `server/errors.py`, which `tests/server/test_errors.py` holds in exact corres absent from both the detail and the message: on the ingest path it is an absolute path inside a directory the operator, not the client, pointed at. +### The two narrowing refusals + +They share a status, and only one of them has a way forward, so each carries the actionable half of +itself as structure rather than as prose: + +```json +{ "code": "DESTRUCTIVE_SCHEMA_CHANGE", + "detail": { "classes": ["lane"] } } + +{ "code": "SCHEMA_CHANGE_WOULD_ORPHAN", + "detail": { "blockers": [ { "label_class": "lane", "annotations": 12, "assets": 3 } ] } } +``` + +`classes` is the blast radius a confirmation has to name. It carries **no counts**, and that is the +difference between the two: this refusal is about intent and is raised before anything on disk is +consulted, so attaching counts would put a walk over every asset in the project in front of the one +refusal that does not need it. A client that wants them asks the preview below. + +`blockers` is why no flag helps, counted two ways — a thousand labels over a thousand images and the +same thousand over ten are the same `annotations` and a very different problem. + +Neither is available by parsing `message`, and neither should be: `message`'s own field description +says the wording is not part of the contract. + +### Asking before you are refused + +``` +POST /projects/{project_id}/schema/preview +``` + +The body is the **same document** `POST .../schema/versions` takes, so a client previews and +publishes without reshaping anything (`description` and `provenance` are accepted and ignored — +neither enters a diff). It writes nothing. + +```json +{ "diff": { "is_destructive": true, "destructive_classes": ["lane"], "changes": [] }, + "blockers": [ { "label_class": "lane", "annotations": 12, "assets": 3 } ], + "is_refused": true } +``` + +`diff.is_destructive` decides whether the publish needs `allow_destructive=true`. **`is_refused` +decides whether any flag would help** — and `blockers` is byte-for-byte the structure +`SCHEMA_CHANGE_WOULD_ORPHAN` puts in its `detail`, so one renderer serves the warning and the +refusal. + +It is **advisory**. Nothing is locked and nothing is reserved: somebody can label a class between +the preview and the publish, in which case the publish refuses and that refusal is the +authoritative one. What the preview removes is the round trip that was doomed before it was sent, +not the need to handle being refused. + +A POST because the proposal is a whole class list, which does not belong in a query string. It is +still a read. + --- ## For contributors diff --git a/docs/schemas.md b/docs/schemas.md index 22718ae4..da416d74 100644 --- a/docs/schemas.md +++ b/docs/schemas.md @@ -264,16 +264,28 @@ Within one version, class names must be unique ignoring case, for the same reaso beside `car` is two classes that read as one to everybody except the code. The classifier lives in `kernel/domain/schema_diff.py` and is pure - two sequences in, a -verdict out. `preview` runs it against the active version without writing, so a surface can +verdict out. `preview` runs it against the active version without writing, and adds the half the +classifier cannot know - which of the classes being dropped already carry labels - so a surface can warn before it asks: ```python -diff = schemas.preview(project.id, proposed) -diff.is_destructive # True -diff.destructive_classes # frozenset({'lane'}) -diff.describe(ChangeKind.DESTRUCTIVE) # "class 'lane' removed" +preview = schemas.preview(project.id, proposed) +preview.diff.is_destructive # True - needs allow_destructive +preview.diff.destructive_classes # frozenset({'lane'}) +preview.is_refused # False - and no flag would change that if True +preview.blockers # () - or (ClassCount(label_class='lane', ...),) ``` +**`is_destructive` and `is_refused` are different questions**, and conflating them is the loop +`SchemaChangeWouldOrphan` sits outside `DestructiveSchemaChange`'s hierarchy to prevent: the first +is answered by passing a flag, the second by nothing at all. `blockers` is the same structure +`SCHEMA_CHANGE_WOULD_ORPHAN` puts in its `detail`, so one renderer serves the warning and the +refusal. + +The preview is advisory: nothing is locked, so a label written between the preview and the publish +makes the publish refuse, and that refusal is the authoritative one. What it removes is the round +trip that was doomed before it was sent. + `compare(project_id, from_version, to_version)` does the same between two stored versions, in either direction. @@ -405,6 +417,8 @@ refusals are **both 409** with only one override between them, so it branches on `code` and shows "Save anyway" for `DESTRUCTIVE_SCHEMA_CHANGE` and nothing but "Close" for `SCHEMA_CHANGE_WOULD_ORPHAN`. -It has no preview of the change being drafted, because `SchemaService.preview` is unrouted; -`compare` is routed, and the version navigator uses it to show what two *published* versions did -to each other. See [ui.md](ui.md#the-schema-editor-and-the-two-409s). +`POST /projects/{id}/schema/preview` now routes `SchemaService.preview`, so a client can ask +both questions about a *draft* before it publishes; `compare` remains the question about two +*published* versions, which is what the version navigator asks. See +[ui.md](ui.md#the-schema-editor-and-the-two-409s) and +[api.md](api.md#asking-before-you-are-refused). diff --git a/frontend/ui-core/src/generated/api.ts b/frontend/ui-core/src/generated/api.ts index 2cf7e1f1..1fc38fd2 100644 --- a/frontend/ui-core/src/generated/api.ts +++ b/frontend/ui-core/src/generated/api.ts @@ -1672,6 +1672,51 @@ export interface paths { patch?: never; trace?: never; }; + "/projects/{project_id}/schema/preview": { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + get?: never; + put?: never; + /** + * Preview Schema Change + * @description Say what publishing these classes would do, without publishing anything. + * + * Writes nothing, and answers both gates at once. `diff` is the classification + * `GET /compare` returns — whether this narrows the contract, and over which + * classes — so `diff.is_destructive` decides whether the publish needs + * `allow_destructive=true`. + * + * **`is_refused` is the answer no flag changes.** True means annotations already + * exist under a class this proposal drops, so `POST /versions` answers 409 + * `SCHEMA_CHANGE_WOULD_ORPHAN` however it is called, and `blockers` names each + * such class with how many annotations and how many assets carry it. That is the + * **same structure** the refusal itself puts in `detail`, so one renderer serves + * the warning and the refusal. Retrying with `allow_destructive=true` against a + * refused preview is the loop `code` exists to prevent. + * + * A POST because the proposal is the whole class list and a class list does not + * belong in a query string. It is still a read: nothing is written, nothing is + * locked, and nothing is reserved. Somebody can label a class between this call + * and the publish, in which case the publish refuses and **that** refusal is the + * authoritative one — this removes the round trip that was doomed before it was + * sent, not the need to handle being refused. + * + * The body is the same shape `POST /versions` takes, so a client previews and + * publishes the identical document. `description` and `provenance` are accepted + * and ignored: neither enters a diff, and requiring a client to strip them would + * make the two calls differ for no reason. + */ + post: operations["preview_schema_change"]; + delete?: never; + options?: never; + head?: never; + patch?: never; + trace?: never; + }; "/projects/{project_id}/schema/versions": { parameters: { query?: never; @@ -3696,6 +3741,17 @@ export interface components { /** Label Class */ label_class: string; }; + /** + * SchemaChangePreviewOut + * @description What publishing a proposed version would do, and what would stop it. + */ + SchemaChangePreviewOut: { + /** Blockers */ + blockers: components["schemas"]["ClassCountOut"][]; + diff: components["schemas"]["SchemaDiffOut"]; + /** Is Refused */ + is_refused: boolean; + }; /** * SchemaDiffOut * @description Every difference between two schema versions, and the verdict on them. @@ -8419,6 +8475,77 @@ export interface operations { }; }; }; + preview_schema_change: { + parameters: { + query?: never; + header?: never; + path: { + project_id: string; + }; + cookie?: never; + }; + requestBody: { + content: { + "application/json": components["schemas"]["SchemaVersionCreate"]; + }; + }; + responses: { + /** @description Successful Response */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["SchemaChangePreviewOut"]; + }; + }; + /** @description Missing or invalid bearer token */ + 401: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description No such resource */ + 404: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description The request payload is not processable */ + 422: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description Unhandled server error, with an incident id */ + 500: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description The workspace is busy; retry after the header says */ + 503: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + }; + }; list_schema_versions: { parameters: { query?: never; diff --git a/frontend/ui-core/src/generated/checks.ts b/frontend/ui-core/src/generated/checks.ts index fa6c3387..fc0048e5 100644 --- a/frontend/ui-core/src/generated/checks.ts +++ b/frontend/ui-core/src/generated/checks.ts @@ -257,6 +257,9 @@ export const checkSchemaChangeOut: Check = export const checkSchemaDiffOut: Check = /*#__PURE__*/ object({ "changes": [true, arrayOf(checkSchemaChangeOut)], "destructive_classes": [true, arrayOf(isString)], "is_destructive": [true, isBoolean] } as const); +export const checkSchemaChangePreviewOut: Check = + /*#__PURE__*/ object({ "blockers": [true, arrayOf(checkClassCountOut)], "diff": [true, checkSchemaDiffOut], "is_refused": [true, isBoolean] } as const); + export const checkAttributeBody: Check = /*#__PURE__*/ object({ "default": [false, either([isBoolean, isNumber, isString, isNull] as const)], "kind": [true, oneOf(["string", "number", "boolean", "select"] as const)], "name": [true, isString], "options": [false, either([arrayOf(isString), isNull] as const)], "required": [true, isBoolean] } as const); @@ -379,6 +382,7 @@ export const checkListReleases = checkReleasePage; export const checkListSchemaVersions = checkSchemaVersionPage; export const checkListSources = checkSourcePage; export const checkNextPendingAssets = checkAssetPage; +export const checkPreviewSchemaChange = checkSchemaChangePreviewOut; export const checkPromoteBatch = checkAssetPage; export const checkPublishRelease = checkReleaseOut; export const checkRegisterImageSource = checkSourceOut; diff --git a/openapi.json b/openapi.json index 91f6e2d5..10ea4316 100644 --- a/openapi.json +++ b/openapi.json @@ -3475,6 +3475,32 @@ "title": "SchemaChangeOut", "type": "object" }, + "SchemaChangePreviewOut": { + "description": "What publishing a proposed version would do, and what would stop it.", + "properties": { + "blockers": { + "items": { + "$ref": "#/components/schemas/ClassCountOut" + }, + "title": "Blockers", + "type": "array" + }, + "diff": { + "$ref": "#/components/schemas/SchemaDiffOut" + }, + "is_refused": { + "title": "Is Refused", + "type": "boolean" + } + }, + "required": [ + "diff", + "blockers", + "is_refused" + ], + "title": "SchemaChangePreviewOut", + "type": "object" + }, "SchemaDiffOut": { "description": "Every difference between two schema versions, and the verdict on them.", "properties": { @@ -10058,6 +10084,105 @@ ] } }, + "/projects/{project_id}/schema/preview": { + "post": { + "description": "Say what publishing these classes would do, without publishing anything.\n\nWrites nothing, and answers both gates at once. `diff` is the classification\n`GET /compare` returns \u2014 whether this narrows the contract, and over which\nclasses \u2014 so `diff.is_destructive` decides whether the publish needs\n`allow_destructive=true`.\n\n**`is_refused` is the answer no flag changes.** True means annotations already\nexist under a class this proposal drops, so `POST /versions` answers 409\n`SCHEMA_CHANGE_WOULD_ORPHAN` however it is called, and `blockers` names each\nsuch class with how many annotations and how many assets carry it. That is the\n**same structure** the refusal itself puts in `detail`, so one renderer serves\nthe warning and the refusal. Retrying with `allow_destructive=true` against a\nrefused preview is the loop `code` exists to prevent.\n\nA POST because the proposal is the whole class list and a class list does not\nbelong in a query string. It is still a read: nothing is written, nothing is\nlocked, and nothing is reserved. Somebody can label a class between this call\nand the publish, in which case the publish refuses and **that** refusal is the\nauthoritative one \u2014 this removes the round trip that was doomed before it was\nsent, not the need to handle being refused.\n\nThe body is the same shape `POST /versions` takes, so a client previews and\npublishes the identical document. `description` and `provenance` are accepted\nand ignored: neither enters a diff, and requiring a client to strip them would\nmake the two calls differ for no reason.", + "operationId": "preview_schema_change", + "parameters": [ + { + "in": "path", + "name": "project_id", + "required": true, + "schema": { + "format": "uuid", + "title": "Project Id", + "type": "string" + } + } + ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SchemaVersionCreate" + } + } + }, + "required": true + }, + "responses": { + "200": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SchemaChangePreviewOut" + } + } + }, + "description": "Successful Response" + }, + "401": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "Missing or invalid bearer token" + }, + "404": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "No such resource" + }, + "422": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "The request payload is not processable" + }, + "500": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "Unhandled server error, with an incident id" + }, + "503": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "The workspace is busy; retry after the header says" + } + }, + "security": [ + { + "HTTPBearer": [] + } + ], + "summary": "Preview Schema Change", + "tags": [ + "schemas" + ] + } + }, "/projects/{project_id}/schema/versions": { "get": { "description": "Every version, oldest first. An empty page is the ordinary starting state.", diff --git a/src/visionset/cli/_errors.py b/src/visionset/cli/_errors.py index da0cc05c..f5d7e630 100644 --- a/src/visionset/cli/_errors.py +++ b/src/visionset/cli/_errors.py @@ -31,7 +31,13 @@ import typer -from visionset.kernel import LossyExportNotConsented, NotAWorkspace, VisionSetError +from visionset.kernel import ( + DestructiveSchemaChange, + LossyExportNotConsented, + NotAWorkspace, + SchemaChangeWouldOrphan, + VisionSetError, +) from visionset.kernel.services import WORKSPACE_ENV_VAR EXIT_DOMAIN_ERROR: Final = 1 @@ -69,6 +75,24 @@ "Re-run with --allow-lossy to accept the loss, or with --check to see " "exactly what it costs, class by class." ), + # The kernel's sentence says "pass allow_destructive=True", which is the + # *service* keyword — a person at a terminal types `--allow-destructive`, and + # no amount of reading the message tells them so. `LossyExportNotConsented` + # exactly, one refusal over. + DestructiveSchemaChange: ( + "Re-run with --allow-destructive if narrowing the schema is what you meant." + ), + # The opposite hint, and the reason it is worth its own entry: this refusal + # has **no** flag. Without a line saying so, the neighbouring + # `--allow-destructive` reads as the obvious next thing to try — and it is + # precisely the loop `SchemaChangeWouldOrphan` is declared outside + # `DestructiveSchemaChange`'s hierarchy to prevent. The message already names + # the classes and their counts; what it cannot say is that there is nothing + # to pass. + SchemaChangeWouldOrphan: ( + "There is no flag for this one. Delete or relabel those annotations " + "first, or keep the class and change something else." + ), } """A remedy a *terminal* can act on, printed under the error's own sentence. diff --git a/src/visionset/kernel/domain/__init__.py b/src/visionset/kernel/domain/__init__.py index e1912ef1..71da134c 100644 --- a/src/visionset/kernel/domain/__init__.py +++ b/src/visionset/kernel/domain/__init__.py @@ -167,6 +167,7 @@ from visionset.kernel.domain.schema_diff import ( ChangeKind, SchemaChange, + SchemaChangePreview, SchemaDiff, diff_classes, ) @@ -357,6 +358,7 @@ "ReleasePublished", "ReleaseVerification", "SchemaChange", + "SchemaChangePreview", "SchemaDiff", "SchemaProvenance", "SingleJob", diff --git a/src/visionset/kernel/domain/schema_diff.py b/src/visionset/kernel/domain/schema_diff.py index 9559b54c..d8b5fec2 100644 --- a/src/visionset/kernel/domain/schema_diff.py +++ b/src/visionset/kernel/domain/schema_diff.py @@ -27,8 +27,9 @@ from collections.abc import Iterator, Sequence from enum import StrEnum -from pydantic import BaseModel, ConfigDict +from pydantic import BaseModel, ConfigDict, field_validator +from visionset.kernel.domain.dataset import ClassCount from visionset.kernel.domain.schema import Attribute, LabelClass @@ -74,6 +75,53 @@ def describe(self, kind: ChangeKind) -> str: return "; ".join(change.detail for change in self.changes if change.kind is kind) +class SchemaChangePreview(BaseModel): + """A proposed version's verdict, and what stands in the way of publishing it. + + :class:`SchemaDiff` answers whether the change narrows the contract, which is + a question about two class lists and nothing else. This adds the half no + caller can compute for itself — which of the classes being dropped already + carry labels, and how many — so a surface can say *this will be refused, over + these* **before** it asks rather than after. That is what + ``SchemaService.preview`` has promised since it was written and what nothing + on the wire could answer. + + ``blockers`` is the same report :class:`SchemaChangeWouldOrphan` carries, and + deliberately so: one shape for the warning and for the refusal means a client + renders both with one piece of code, and the two cannot drift into + disagreeing about a question they are both answering. + + Empty ``blockers`` under a destructive ``diff`` is the ordinary safe + narrowing — the change removes something nobody has used — and is exactly the + case ``allow_destructive`` exists to confirm rather than refuse. + + Sorted by class name in a validator, on ``Manifest``'s terms: a report two + callers may compare has one order, and it is not the order a dict happened to + iterate in. + """ + + model_config = ConfigDict(frozen=True, extra="forbid") + + diff: SchemaDiff + blockers: tuple[ClassCount, ...] = () + + @field_validator("blockers") + @classmethod + def _ordered(cls, value: tuple[ClassCount, ...]) -> tuple[ClassCount, ...]: + return tuple(sorted(value, key=lambda count: count.label_class)) + + @property + def is_refused(self) -> bool: + """Whether ``create_version`` would refuse this outright, flag or no flag. + + Published rather than left to the caller, for the reason ``is_destructive`` + is: a client deciding whether to offer a way forward must not re-derive + the rule from ``blockers`` and get it subtly wrong — that is the + hand-mirrored table the ``ui-capabilities`` contract bans, in miniature. + """ + return bool(self.blockers) + + def diff_classes(previous: Sequence[LabelClass], proposed: Sequence[LabelClass]) -> SchemaDiff: """Judge what ``proposed`` does to ``previous``. diff --git a/src/visionset/kernel/errors.py b/src/visionset/kernel/errors.py index 1d85794a..77967732 100644 --- a/src/visionset/kernel/errors.py +++ b/src/visionset/kernel/errors.py @@ -229,6 +229,26 @@ class DestructiveSchemaChange(VisionSetError): is always a decision somebody made, never a side effect of an edit. """ + #: Which classes the change narrows — a ``tuple[str, ...]``, the blast radius + #: a confirmation has to name. + #: + #: Names only, with no counts, and that is the whole difference from + #: ``SchemaChangeWouldOrphan.blockers``. This refusal is about *intent* and is + #: raised before anything on disk is consulted, so attaching counts here would + #: put a walk over every asset in the project in front of the one refusal that + #: does not need it — and would invert the "intent first, then facts on disk" + #: ordering the two gates are built on. A caller who wants the counts asks + #: ``SchemaService.preview``, which is what it is for. + #: + #: A class attribute rather than a constructor parameter, for the reason + #: ``SchemaChangeWouldOrphan.blockers`` gives. + classes: object | None = None + + def __init__(self, message: str, *, classes: object | None = None) -> None: + super().__init__(message) + if classes is not None: + self.classes = classes + class SchemaChangeWouldOrphan(VisionSetError): """A destructive change was refused because annotations already depend on it. @@ -241,6 +261,26 @@ class SchemaChangeWouldOrphan(VisionSetError): longer describes. """ + #: Which classes stand in the way, and how many labels each carries — a + #: ``tuple[ClassCount, ...]``. + #: + #: A class attribute with a ``None`` default and **not** a constructor + #: parameter, exactly as ``VisionSetError.index`` and + #: ``LossyExportNotConsented.compatibility`` are — so this error is still + #: constructible from one message, ``ERROR_RULES``' exact-correspondence test + #: is untouched, and ``test_every_mapped_error_can_be_constructed_with_one_argument`` + #: still holds. The service sets it as a keyword. + #: + #: Typed ``object`` because this module may not import a domain model; the + #: type comes back at the boundary through an ``isinstance`` narrowing, which + #: is the same round trip ``compatibility`` makes. + blockers: object | None = None + + def __init__(self, message: str, *, blockers: object | None = None) -> None: + super().__init__(message) + if blockers is not None: + self.blockers = blockers + class InvalidTransition(VisionSetError): """A state machine was asked to make a move that is not in its table. diff --git a/src/visionset/kernel/services/batch_service.py b/src/visionset/kernel/services/batch_service.py index 9bcc58a6..39e67d51 100644 --- a/src/visionset/kernel/services/batch_service.py +++ b/src/visionset/kernel/services/batch_service.py @@ -52,6 +52,7 @@ BatchCompleted, BatchState, ChangeKind, + ClassCount, MembershipChange, Partition, Project, @@ -559,7 +560,8 @@ def _refuse_narrowing( raise DestructiveSchemaChange( f"re-pinning batch {batch.name!r} onto the active schema version narrows what " f"it allows ({diff.describe(ChangeKind.DESTRUCTIVE)}); pass " - f"allow_destructive=True to proceed" + f"allow_destructive=True to proceed", + classes=tuple(sorted(diff.destructive_classes)), ) def _refuse_orphaning(self, uow: UnitOfWork, batch: Batch, guarded: frozenset[str]) -> NoReturn: @@ -572,13 +574,14 @@ def _refuse_orphaning(self, uow: UnitOfWork, batch: Batch, guarded: frozenset[st annotated = _annotated_classes(uow, batch) affected = sorted(guarded & annotated.keys()) or sorted(guarded) counted = ", ".join( - f"{name!r} ({annotated[name]})" if name in annotated else repr(name) + f"{name!r} ({annotated[name].annotations})" if name in annotated else repr(name) for name in affected ) raise SchemaChangeWouldOrphan( f"cannot re-pin batch {batch.name!r}: it already holds annotations under " f"{counted}. Migrating them onto a new version is not supported yet, and " - f"the kernel will not orphan them" + f"the kernel will not orphan them", + blockers=tuple(annotated[name] for name in affected if name in annotated), ) # --- the transition table, consulted rather than restated --------------- @@ -651,8 +654,8 @@ def _subject(batch: Batch) -> str: return f"batch {batch.name!r}" -def _annotated_classes(uow: UnitOfWork, batch: Batch) -> dict[str, int]: - """How many annotations each label class has *inside this batch*. +def _annotated_classes(uow: UnitOfWork, batch: Batch) -> dict[str, ClassCount]: + """How much of each label class this batch holds. ``SchemaService._annotated_classes`` over one batch's membership rather than a whole project, and N + 1 for the same reason: ``Repository.list`` takes a @@ -663,11 +666,16 @@ def _annotated_classes(uow: UnitOfWork, batch: Batch) -> dict[str, int]: asset rows themselves are not wanted, only their annotations, and a missing membership row would refuse a re-pin over a fact this question does not need. """ - counts: dict[str, int] = {} + annotations: dict[str, int] = {} + assets: dict[str, set[UUID]] = {} for asset_id in batch.asset_ids: for annotation in uow.annotations.list(asset_id): - counts[annotation.label_class] = counts.get(annotation.label_class, 0) + 1 - return counts + annotations[annotation.label_class] = annotations.get(annotation.label_class, 0) + 1 + assets.setdefault(annotation.label_class, set()).add(asset_id) + return { + name: ClassCount(label_class=name, annotations=count, assets=len(assets[name])) + for name, count in annotations.items() + } def _already_labeled(uow: UnitOfWork, asset_ids: Iterable[UUID]) -> set[UUID]: diff --git a/src/visionset/kernel/services/schema_service.py b/src/visionset/kernel/services/schema_service.py index 5874bf55..06ffc036 100644 --- a/src/visionset/kernel/services/schema_service.py +++ b/src/visionset/kernel/services/schema_service.py @@ -42,9 +42,11 @@ class the contract no longer describes. IMPLEMENTED_GEOMETRIES, AnnotationSchema, ChangeKind, + ClassCount, GeometryType, LabelClass, Project, + SchemaChangePreview, SchemaDiff, SchemaProvenance, diff_classes, @@ -145,12 +147,30 @@ def compare(self, project_id: UUID, from_version: int, to_version: int) -> Schem self._require_version(versions, project_id, to_version).classes, ) - def preview(self, project_id: UUID, classes: Sequence[LabelClass]) -> SchemaDiff: + def preview(self, project_id: UUID, classes: Sequence[LabelClass]) -> SchemaChangePreview: """How ``create_version`` would judge these classes, without writing. - The same diff the gates in :meth:`create_version` are built on, so a - surface can warn before it asks — "this removes 2 classes, continue?" — - instead of asking and then reporting a refusal. + Both gates, asked and not enforced. ``diff`` is what + :meth:`_refuse_narrowing` decides on — whether this needs + ``allow_destructive`` — and ``blockers`` is what the guarded insert would + refuse over: the classes being dropped that already carry labels. A + surface can therefore say *this removes 2 classes* **or** *this cannot be + published, 12 labels use 'lane'* before it asks, instead of asking and + then translating a refusal. + + ``blockers`` is the same report :class:`SchemaChangeWouldOrphan` carries, + so a client renders the warning and the refusal with one piece of code. + + **Advisory, and it says so.** Nothing is locked and nothing is reserved: + somebody can label a class between this call and the publish, in which + case the publish refuses and *that* refusal is the authoritative one. The + guard inside the insert is what makes the answer safe to act on — not + this. What this removes is the round trip that was doomed before it was + sent, which is a question about the interface rather than about + correctness. + + Empty ``blockers`` under a destructive diff is the ordinary safe + narrowing, and is what ``allow_destructive`` confirms. Raises: ProjectNotFound: no such project in this workspace. @@ -158,7 +178,10 @@ def preview(self, project_id: UUID, classes: Sequence[LabelClass]) -> SchemaDiff with self._workspace.unit_of_work() as uow: self._require_project(uow, project_id) active = self.active(uow, project_id) - return diff_classes(() if active is None else active.classes, classes) + diff = diff_classes(() if active is None else active.classes, classes) + return SchemaChangePreview( + diff=diff, blockers=_blockers(uow, project_id, diff.destructive_classes) + ) # --- writing: the only door -------------------------------------------- @@ -279,9 +302,10 @@ def _refuse_narrowing( """ if not allow_destructive: raise DestructiveSchemaChange( - f"this version narrows the schema of project {project_id} " + f"this version narrows the schema " f"({diff.describe(ChangeKind.DESTRUCTIVE)}); pass allow_destructive=True " - f"to proceed" + f"to proceed", + classes=tuple(sorted(diff.destructive_classes)), ) def _refuse_orphaning( @@ -305,13 +329,14 @@ def _refuse_orphaning( annotated = _annotated_classes(uow, project_id) affected = sorted(guarded & annotated.keys()) or sorted(guarded) counted = ", ".join( - f"{name!r} ({annotated[name]})" if name in annotated else repr(name) + f"{name!r} ({annotated[name].annotations})" if name in annotated else repr(name) for name in affected ) raise SchemaChangeWouldOrphan( - f"cannot narrow project {project_id}: annotations already exist under " + f"cannot narrow this schema: annotations already exist under " f"{counted}. Migrating them onto a new version is not supported yet, and " - f"the kernel will not orphan them" + f"the kernel will not orphan them", + blockers=tuple(annotated[name] for name in affected if name in annotated), ) # --- lookups shared by the operations above ---------------------------- @@ -426,8 +451,27 @@ def _require_coherent(classes: Sequence[LabelClass]) -> None: seen[folded] = label_class.name -def _annotated_classes(uow: UnitOfWork, project_id: UUID) -> dict[str, int]: - """How many annotations each label class currently has in this project. +def _blockers(uow: UnitOfWork, project_id: UUID, guarded: frozenset[str]) -> tuple[ClassCount, ...]: + """Which of ``guarded`` already carry labels, counted — the report, not a gate. + + The read half of what the guarded insert decides, shared by + :meth:`SchemaService.preview` (which asks in advance) and by + :meth:`SchemaService._refuse_orphaning` (which asks afterwards, to say what it + refused over), so the warning and the refusal cannot report different things + about the same project. + + Empty ``guarded`` short-circuits: a change that removes nothing has nothing + to block it, and walking every asset to establish that is a cost with no + question behind it. + """ + if not guarded: + return () + annotated = _annotated_classes(uow, project_id) + return tuple(annotated[name] for name in sorted(guarded & annotated.keys())) + + +def _annotated_classes(uow: UnitOfWork, project_id: UUID) -> dict[str, ClassCount]: + """How much of each label class this project currently holds. Walks the project's assets and reads each one's annotations, because the persistence port has no cross-table query: ``Repository.list`` takes a single @@ -436,9 +480,21 @@ def _annotated_classes(uow: UnitOfWork, project_id: UUID) -> dict[str, int]: is worth more at M1 scale than the round trips cost. When it does start to cost, the fix is a method on the port (``annotations.list_for_project``) implemented in the adapter, never a SQLAlchemy import in a service. + + ``ClassCount`` rather than a bare count, and reused rather than re-spelled: + both numbers mean here exactly what they mean for the trunk, and a class + carrying the same two fields is how two counts of one thing start to + disagree. The second number is what turns "12 annotations" into "12 + annotations across 3 images", which is the difference between a blast radius + somebody can judge and a number they cannot. """ - counts: dict[str, int] = {} + annotations: dict[str, int] = {} + assets: dict[str, set[UUID]] = {} for asset in uow.assets.list(project_id): for annotation in uow.annotations.list(asset.id): - counts[annotation.label_class] = counts.get(annotation.label_class, 0) + 1 - return counts + annotations[annotation.label_class] = annotations.get(annotation.label_class, 0) + 1 + assets.setdefault(annotation.label_class, set()).add(asset.id) + return { + name: ClassCount(label_class=name, annotations=count, assets=len(assets[name])) + for name, count in annotations.items() + } diff --git a/src/visionset/mcp/schemas.py b/src/visionset/mcp/schemas.py index 84c4015b..e8b472fd 100644 --- a/src/visionset/mcp/schemas.py +++ b/src/visionset/mcp/schemas.py @@ -123,12 +123,22 @@ def preview_schema_change(project: ProjectRef, classes: ClassesParam) -> dict[st changing an existing schema rather than creating the first one — it is the only way to find out that a change is destructive without attempting it. - `is_destructive` true means the proposal narrows the contract: a class or an - attribute is gone, or a geometry moved. `destructive_classes` names them. - Applying it then needs `allow_destructive=true` — unless annotations already - exist under one of those classes, in which case `create_schema_version` - refuses outright and no flag overrides it. Adding classes or optional - attributes is additive and needs no flag. + `diff.is_destructive` true means the proposal narrows the contract: a class or + an attribute is gone, or a geometry moved. `diff.destructive_classes` names + them, and applying it then needs `allow_destructive=true`. Adding classes or + optional attributes is additive and needs no flag. + + **`is_refused` is the answer no flag changes.** True means annotations already + exist under a class this proposal drops, so `create_schema_version` refuses + outright however you call it; `blockers` names each such class with how many + annotations and how many assets it carries. Do not retry with + `allow_destructive=true` — that flag answers a different refusal, and retrying + is a loop. Either keep the class, or delete the annotations `blockers` counts + and preview again. + + Advisory: nothing is locked. Somebody can label a class between this call and + the publish, in which case the publish refuses and that refusal is the + authoritative one. Each entry of `classes` must carry `geometry` as one of the declared geometry types; matching against the current version is by exact class name, so @@ -136,8 +146,8 @@ def preview_schema_change(project: ProjectRef, classes: ClassesParam) -> dict[st """ with opened_workspace() as workspace: resolved = resolve_project(workspace, project) - diff = SchemaService(workspace).preview(resolved.id, classes) - return wire.schema_diff(diff) + preview = SchemaService(workspace).preview(resolved.id, classes) + return wire.schema_change_preview(preview) def create_schema_version( diff --git a/src/visionset/server/errors.py b/src/visionset/server/errors.py index d200be8d..938237d1 100644 --- a/src/visionset/server/errors.py +++ b/src/visionset/server/errors.py @@ -121,8 +121,8 @@ WorkspaceNotEmpty, WorkspaceSchemaMismatch, ) -from visionset.kernel.domain import ExportCompatibility -from visionset.server.models import ExportCompatibilityOut +from visionset.kernel.domain import ClassCount, ExportCompatibility +from visionset.server.models import ClassCountOut, ExportCompatibilityOut _logger = logging.getLogger(__name__) """Never call ``logging.basicConfig`` here — records propagate to root, which @@ -484,6 +484,30 @@ def _detail_for(exc: BaseException) -> dict[str, Any] | None: return { "compatibility": ExportCompatibilityOut.of(exc.compatibility).model_dump(mode="json") } + if isinstance(exc, SchemaChangeWouldOrphan) and isinstance(exc.blockers, tuple): + # The per-class report, on the refusal itself — `LossyExportNotConsented`'s + # bargain, and the same one: a client that gets this 409 has everything it + # needs to say *what* is in the way and *how much of it* without a second + # round trip. It is the same structure `POST .../schema/preview` returns, + # so one renderer serves the warning and the refusal. + # + # The `isinstance` is not defensive padding: `blockers` is typed + # `object | None` because `kernel/errors.py` may not import a domain + # model, so this is where the type comes back. + return { + "blockers": [ + ClassCountOut.of(count).model_dump(mode="json") + for count in exc.blockers + if isinstance(count, ClassCount) + ] + } + if isinstance(exc, DestructiveSchemaChange) and isinstance(exc.classes, tuple): + # Names only. This refusal is raised before anything on disk is consulted + # — see the field's own note — so there are no counts to publish, and a + # client wanting them asks `POST .../schema/preview`, which is what it is + # for. What a confirmation needs from *here* is the blast radius: which + # classes go, in an order that does not depend on a set's iteration. + return {"classes": [name for name in exc.classes if isinstance(name, str)]} if isinstance(exc, VisionSetError) and exc.index is not None: # Which item of a bulk request was refused. The kernel sets this on the # way out of a per-item loop; everything else leaves it ``None``, so the diff --git a/src/visionset/server/models.py b/src/visionset/server/models.py index b4b6a715..8b00d902 100644 --- a/src/visionset/server/models.py +++ b/src/visionset/server/models.py @@ -108,6 +108,7 @@ ResumeKind, ResumeTarget, SchemaChange, + SchemaChangePreview, SchemaDiff, SchemaProvenance, SingleJob, @@ -412,6 +413,30 @@ def of(cls, diff: SchemaDiff) -> Self: ) +class SchemaChangePreviewOut(BaseModel): + """What publishing a proposed version would do, and what would stop it.""" + + # `diff` answers whether this needs `allow_destructive`; `blockers` answers + # whether any flag would help. The second is the half a client could not ask + # for before and had to discover by being refused — and it is the *same* + # structure `SCHEMA_CHANGE_WOULD_ORPHAN` puts in its `detail`, so one renderer + # serves the warning and the refusal and the two cannot drift. + diff: SchemaDiffOut + blockers: tuple[ClassCountOut, ...] + # A domain `@property` materialized here, on `SchemaDiffOut`'s terms: a client + # deciding whether to offer a way forward must not re-derive the rule from + # `blockers`. That is the hand-mirrored table the capabilities contract bans. + is_refused: bool + + @classmethod + def of(cls, preview: SchemaChangePreview) -> Self: + return cls( + diff=SchemaDiffOut.of(preview.diff), + blockers=tuple(ClassCountOut.of(count) for count in preview.blockers), + is_refused=preview.is_refused, + ) + + class SchemaVersionCreate(BaseModel): """The whole proposed version. There is no partial edit of a schema.""" diff --git a/src/visionset/server/routes/schemas.py b/src/visionset/server/routes/schemas.py index 77c964d3..f0c0fa58 100644 --- a/src/visionset/server/routes/schemas.py +++ b/src/visionset/server/routes/schemas.py @@ -24,6 +24,7 @@ from visionset.server.errors import documented from visionset.server.models import ( DestructiveQuery, + SchemaChangePreviewOut, SchemaDiffOut, SchemaVersionCreate, SchemaVersionOut, @@ -100,6 +101,41 @@ class while labeling. It is stored exactly as sent and never inferred, so a return SchemaVersionOut.of(created) +@router.post("/preview", responses=documented(404)) +def preview_schema_change( + workspace: WorkspaceDep, project_id: UUID, body: SchemaVersionCreate +) -> SchemaChangePreviewOut: + """Say what publishing these classes would do, without publishing anything. + + Writes nothing, and answers both gates at once. `diff` is the classification + `GET /compare` returns — whether this narrows the contract, and over which + classes — so `diff.is_destructive` decides whether the publish needs + `allow_destructive=true`. + + **`is_refused` is the answer no flag changes.** True means annotations already + exist under a class this proposal drops, so `POST /versions` answers 409 + `SCHEMA_CHANGE_WOULD_ORPHAN` however it is called, and `blockers` names each + such class with how many annotations and how many assets carry it. That is the + **same structure** the refusal itself puts in `detail`, so one renderer serves + the warning and the refusal. Retrying with `allow_destructive=true` against a + refused preview is the loop `code` exists to prevent. + + A POST because the proposal is the whole class list and a class list does not + belong in a query string. It is still a read: nothing is written, nothing is + locked, and nothing is reserved. Somebody can label a class between this call + and the publish, in which case the publish refuses and **that** refusal is the + authoritative one — this removes the round trip that was doomed before it was + sent, not the need to handle being refused. + + The body is the same shape `POST /versions` takes, so a client previews and + publishes the identical document. `description` and `provenance` are accepted + and ignored: neither enters a diff, and requiring a client to strip them would + make the two calls differ for no reason. + """ + classes = [label_class.to_domain() for label_class in body.classes] + return SchemaChangePreviewOut.of(SchemaService(workspace).preview(project_id, classes)) + + @router.get("/versions", responses=documented(404)) def list_schema_versions(workspace: WorkspaceDep, project_id: UUID) -> SchemaVersionPage: """Every version, oldest first. An empty page is the ordinary starting state.""" diff --git a/src/visionset/wire/__init__.py b/src/visionset/wire/__init__.py index 1153376d..c60f8f75 100644 --- a/src/visionset/wire/__init__.py +++ b/src/visionset/wire/__init__.py @@ -90,6 +90,7 @@ Release, ReleaseVerification, SchemaChange, + SchemaChangePreview, SchemaDiff, Source, SplitRecipe, @@ -205,6 +206,29 @@ def schema_diff(value: SchemaDiff) -> dict[str, Any]: } +def schema_change_preview(value: SchemaChangePreview) -> dict[str, Any]: + """What publishing these classes would do, and what would stop it. + + ``diff`` answers whether the change needs ``allow_destructive``; ``blockers`` + answers whether any flag would help, which is the question a caller could not + ask before this existed and had to discover by being refused. + + ``is_refused`` is the domain ``@property`` materialized here, on + ``schema_diff``'s terms and for the same reason: a caller deciding whether to + offer a way forward must not re-derive the rule from ``blockers`` and get it + subtly wrong. + + ``blockers`` reuses :func:`class_count` rather than a shape of its own — it is + the same two numbers about the same classes, and a second spelling is how two + reports of one thing start to disagree. + """ + return { + "diff": schema_diff(value.diff), + "blockers": [class_count(c) for c in value.blockers], + "is_refused": value.is_refused, + } + + # --- sources, ingest and assets ---------------------------------------------- diff --git a/tests/cli/test_json_contract.py b/tests/cli/test_json_contract.py index f92ce19f..82d73213 100644 --- a/tests/cli/test_json_contract.py +++ b/tests/cli/test_json_contract.py @@ -46,6 +46,7 @@ POLYGON, PROJECT, RELEASE, + SCHEMA_CHANGE_PREVIEW, SCHEMA_DIFF, SCHEMA_VERSION, SOURCE, @@ -68,6 +69,11 @@ ("dataset", wire.dataset(DATASET), models.DatasetOut), ("schema_version", wire.schema_version(SCHEMA_VERSION), models.SchemaVersionOut), ("schema_diff", wire.schema_diff(SCHEMA_DIFF), models.SchemaDiffOut), + ( + "schema_change_preview", + wire.schema_change_preview(SCHEMA_CHANGE_PREVIEW), + models.SchemaChangePreviewOut, + ), # ``changes[1]`` rather than ``[0]``: it is the one carrying a non-null # ``attribute``, and a sample holding ``None`` there would leave that half of # the projection unchecked. The diff pair above covers both, since it diff --git a/tests/fixtures/samples.py b/tests/fixtures/samples.py index 526e4d5d..de9994fd 100644 --- a/tests/fixtures/samples.py +++ b/tests/fixtures/samples.py @@ -53,6 +53,7 @@ Release, ReleaseVerification, SchemaChange, + SchemaChangePreview, SchemaDiff, Source, SourceKind, @@ -117,6 +118,14 @@ ), ) +#: ``blockers`` is deliberately non-empty, per this module's rule: an empty tuple +#: here would leave the nested ``ClassCount`` projection unchecked, which is the +#: half of the payload a client actually renders. +SCHEMA_CHANGE_PREVIEW = SchemaChangePreview( + diff=SCHEMA_DIFF, + blockers=(ClassCount(label_class="sign", annotations=5, assets=2),), +) + SOURCE = Source( project_id=PROJECT.id, kind=SourceKind.VIDEO, diff --git a/tests/kernel/test_schema_service.py b/tests/kernel/test_schema_service.py index f8134bde..f58a756f 100644 --- a/tests/kernel/test_schema_service.py +++ b/tests/kernel/test_schema_service.py @@ -598,19 +598,67 @@ def test_preview_reports_what_create_version_would_gate_on_without_writing( project = projects.create("signs") schemas.create_version(project.id, [SIGN, LANE]) - diff = schemas.preview(project.id, [SIGN]) - - assert diff.destructive_classes == frozenset({"lane"}) + preview = schemas.preview(project.id, [SIGN]) + + assert preview.diff.destructive_classes == frozenset({"lane"}) + # Nothing is labeled, so the change is destructive and still publishable — + # which is the distinction `blockers` exists to draw and `is_destructive` + # cannot. + assert preview.blockers == () + assert preview.is_refused is False assert [s.version for s in schemas.list_versions(project.id)] == [1] - with pytest.raises(DestructiveSchemaChange, match=diff.describe(diff.changes[0].kind)): + with pytest.raises( + DestructiveSchemaChange, match=preview.diff.describe(preview.diff.changes[0].kind) + ): schemas.create_version(project.id, [SIGN]) workspace.close() +def test_preview_names_the_classes_that_no_flag_would_get_past(tmp_path: Path) -> None: + """The half `SchemaDiff` cannot answer: destructive, and refused outright. + + A caller holding only the diff sees `is_destructive` and reaches for + `allow_destructive`, which is the loop `SchemaChangeWouldOrphan` sits outside + `DestructiveSchemaChange`'s hierarchy to prevent. `is_refused` is what says so + before the attempt rather than after it. + """ + workspace, projects, schemas = _services(tmp_path) + project = projects.create("signs") + schemas.create_version(project.id, [SIGN, LANE]) + _annotate(workspace, project.id, "lane") + + preview = schemas.preview(project.id, [SIGN]) + + assert preview.is_refused is True + assert [(c.label_class, c.annotations, c.assets) for c in preview.blockers] == [("lane", 1, 1)] + + # And the preview agreed with the refusal, which is the whole point of one + # shape serving both. + with pytest.raises(SchemaChangeWouldOrphan) as caught: + schemas.create_version(project.id, [SIGN], allow_destructive=True) + assert caught.value.blockers == preview.blockers + workspace.close() + + +def test_preview_counts_nothing_for_a_change_that_removes_nothing(tmp_path: Path) -> None: + """An additive proposal has no blockers, and does not walk the project to say so.""" + workspace, projects, schemas = _services(tmp_path) + project = projects.create("signs") + schemas.create_version(project.id, [SIGN]) + _annotate(workspace, project.id, "sign") + + preview = schemas.preview(project.id, [SIGN, LANE]) + + assert preview.diff.is_destructive is False + assert preview.blockers == () + assert preview.is_refused is False + workspace.close() + + def test_preview_on_a_project_with_no_schema_is_all_additive(tmp_path: Path) -> None: workspace, projects, schemas = _services(tmp_path) project = projects.create("signs") - assert schemas.preview(project.id, [SIGN, LANE]).is_destructive is False + assert schemas.preview(project.id, [SIGN, LANE]).diff.is_destructive is False workspace.close() diff --git a/tests/mcp/test_schema_tools.py b/tests/mcp/test_schema_tools.py index 71b9255a..86e90c26 100644 --- a/tests/mcp/test_schema_tools.py +++ b/tests/mcp/test_schema_tools.py @@ -63,8 +63,9 @@ def test_adding_a_class_is_additive_and_needs_no_flag( ) -> None: named = schema(monkeypatch, tmp_path) preview = payload(call("preview_schema_change", project=named, classes=BOTH)) - assert preview["is_destructive"] is False - assert preview["destructive_classes"] == [] + assert preview["diff"]["is_destructive"] is False + assert preview["diff"]["destructive_classes"] == [] + assert preview["is_refused"] is False assert payload(call("create_schema_version", project=named, classes=BOTH))["version"] == 2 @@ -73,8 +74,13 @@ def test_preview_names_what_a_change_would_remove_without_writing_anything( ) -> None: named = schema(monkeypatch, tmp_path) preview = payload(call("preview_schema_change", project=named, classes=CAR_ONLY)) - assert preview["is_destructive"] is True - assert preview["destructive_classes"] == ["sign"] + assert preview["diff"]["is_destructive"] is True + assert preview["diff"]["destructive_classes"] == ["sign"] + # Destructive and still publishable — nothing is labeled — which is the + # distinction `is_destructive` alone cannot draw and an agent otherwise + # discovers by being refused. + assert preview["is_refused"] is False + assert preview["blockers"] == [] # Writes nothing: still one version afterwards. That is the whole reason # `SchemaService.preview` finally has a caller. assert payload(call("get_schema", project=named))["available_versions"] == [1] diff --git a/tests/server/test_schema_refusals.py b/tests/server/test_schema_refusals.py new file mode 100644 index 00000000..4b2e33ef --- /dev/null +++ b/tests/server/test_schema_refusals.py @@ -0,0 +1,199 @@ +"""What the two narrowing refusals publish, and what the preview publishes first. + +The pair is the point. `DESTRUCTIVE_SCHEMA_CHANGE` is retryable with a flag and +`SCHEMA_CHANGE_WOULD_ORPHAN` is retryable with nothing at all, they share a +status, and before this the only thing telling them apart was `code` — with the +*actionable* half of each buried in a sentence whose own field description says +the wording is not part of the contract. + +So every test here asserts structure rather than prose, and the last of them +asserts the structure a client gets **before** the attempt is the structure it +gets from the refusal, because one shape serving both is the whole design. +""" + +from collections.abc import Iterator +from pathlib import Path +from typing import Any + +import pytest +from fastapi.testclient import TestClient +from tests.server._api import api_client +from tests.server._flow import a_box, asset_ids, batch_from_ingest +from tests.server._jobs import InlineDispatcher + + +@pytest.fixture() +def runner() -> InlineDispatcher: + return InlineDispatcher() + + +@pytest.fixture() +def client(tmp_path: Path, runner: InlineDispatcher) -> Iterator[TestClient]: + with api_client(tmp_path / "ws", dispatcher=runner) as made: + yield made + + +def a_class(name: str = "sign", **overrides: Any) -> dict[str, Any]: + return {"name": name, "geometry": "bbox", **overrides} + + +#: `sign` exactly as the fixture declares it, attribute included. Re-sending it +#: unchanged is what makes a proposal narrow *only* `lane` — dropping the +#: attribute would make `sign` destructive too, which is correct and is not the +#: thing these tests are measuring. +SIGN = a_class("sign", attributes=[{"name": "occluded", "kind": "boolean", "required": True}]) + + +def post_version(client: TestClient, project: str, *classes: dict[str, Any], **query: Any) -> Any: + return client.post( + f"/projects/{project}/schema/versions", json={"classes": list(classes)}, params=query + ) + + +def preview(client: TestClient, project: str, *classes: dict[str, Any]) -> Any: + return client.post(f"/projects/{project}/schema/preview", json={"classes": list(classes)}) + + +@pytest.fixture() +def project(client: TestClient) -> str: + """A project on version 1, declaring `sign` and `lane`. Nothing labeled yet.""" + project_id: str = client.post("/projects", json={"name": "road-signs"}).json()["id"] + response = post_version(client, project_id, SIGN, a_class("lane")) + assert response.status_code == 201, response.text + return project_id + + +@pytest.fixture() +def labeled(client: TestClient, runner: InlineDispatcher, tmp_path: Path, project: str) -> str: + """The same project, with two `sign` labels drawn across two assets. + + Real annotations through the real routes rather than rows planted underneath, + because the counts are what these tests are about and a planted row would let + a broken walk agree with a broken fixture. + """ + batch_id = batch_from_ingest(client, runner, tmp_path, project, images=2) + client.post(f"/batches/{batch_id}/approve") + client.post(f"/batches/{batch_id}/start") + job_id: str = client.get(f"/batches/{batch_id}/jobs").json()["items"][0]["id"] + client.post(f"/jobs/{job_id}/start") + response = client.post( + f"/jobs/{job_id}/annotations", + json=[a_box(asset_id) for asset_id in asset_ids(client, batch_id)], + ) + assert response.status_code == 201, response.text + return project + + +# --- the refusals carry their report ------------------------------------------ + + +def test_a_narrowing_refusal_names_the_classes_it_would_remove( + client: TestClient, project: str +) -> None: + """The blast radius, as data. A confirmation dialog cannot count a sentence.""" + response = post_version(client, project, SIGN) + + assert response.status_code == 409 + body = response.json() + assert body["code"] == "DESTRUCTIVE_SCHEMA_CHANGE" + assert body["detail"] == {"classes": ["lane"]} + + +def test_an_orphan_refusal_carries_a_count_per_class(client: TestClient, labeled: str) -> None: + """Both numbers, because "12 labels" and "12 labels across 2 images" differ.""" + response = post_version(client, labeled, a_class("lane"), allow_destructive=True) + + assert response.status_code == 409 + body = response.json() + assert body["code"] == "SCHEMA_CHANGE_WOULD_ORPHAN" + assert body["detail"] == {"blockers": [{"label_class": "sign", "annotations": 2, "assets": 2}]} + + +def test_the_orphan_refusal_does_not_put_the_project_id_in_its_sentence( + client: TestClient, labeled: str +) -> None: + """It was there, and a UUID in prose is unreadable at a terminal and in a dialog. + + The caller already holds the id — it is in the URL it just called — so the + message spends its length on what is wrong instead. + """ + response = post_version(client, labeled, a_class("lane"), allow_destructive=True) + + assert labeled not in response.json()["message"] + + +@pytest.mark.parametrize("allow_destructive", [True, False]) +def test_the_flag_never_gets_a_labeled_class_removed( + client: TestClient, labeled: str, allow_destructive: bool +) -> None: + """Pinning the audit's Q2 finding as a contract rather than an observation. + + With the flag the refusal is the orphan one; without it, the flag refusal + fires first. Neither publishes anything — which is the half worth asserting, + because a client that read only the status would see 409 twice and could not + tell that one of them has no way forward. + """ + response = post_version(client, labeled, a_class("lane"), allow_destructive=allow_destructive) + + assert response.status_code == 409 + expected = "SCHEMA_CHANGE_WOULD_ORPHAN" if allow_destructive else "DESTRUCTIVE_SCHEMA_CHANGE" + assert response.json()["code"] == expected + assert client.get(f"/projects/{labeled}/schema").json()["version"] == 1 + + +# --- the preview says it first ------------------------------------------------ + + +def test_a_preview_writes_nothing(client: TestClient, project: str) -> None: + response = preview(client, project, SIGN) + + assert response.status_code == 200 + assert client.get(f"/projects/{project}/schema").json()["version"] == 1 + assert client.get(f"/projects/{project}/schema/versions").json()["total"] == 1 + + +def test_a_preview_separates_needs_a_flag_from_no_flag_will_help( + client: TestClient, project: str +) -> None: + """Destructive and publishable — the case `is_destructive` alone cannot name.""" + body = preview(client, project, SIGN).json() + + assert body["diff"]["is_destructive"] is True + assert body["diff"]["destructive_classes"] == ["lane"] + assert body["is_refused"] is False + assert body["blockers"] == [] + + +def test_a_preview_of_an_additive_change_is_refused_by_nothing( + client: TestClient, labeled: str +) -> None: + body = preview(client, labeled, SIGN, a_class("lane"), a_class("pole")).json() + + assert body["diff"]["is_destructive"] is False + assert body["is_refused"] is False + assert body["blockers"] == [] + + +def test_a_preview_and_the_refusal_report_the_same_blockers( + client: TestClient, labeled: str +) -> None: + """The contract this whole change exists for. + + A client renders the warning and the refusal with one piece of code, so the + two must not be able to disagree about the same project. Asserted as equality + of the structures rather than of two hand-written literals: a shape that + drifted on one side and not the other fails here and nowhere else. + """ + previewed = preview(client, labeled, a_class("lane")).json() + refused = post_version(client, labeled, a_class("lane"), allow_destructive=True).json() + + assert previewed["is_refused"] is True + assert refused["code"] == "SCHEMA_CHANGE_WOULD_ORPHAN" + assert previewed["blockers"] == refused["detail"]["blockers"] + + +def test_a_preview_of_an_unknown_project_is_404(client: TestClient) -> None: + response = preview(client, "0f4f0f8e-0000-4000-8000-000000000000", a_class("sign")) + + assert response.status_code == 404 + assert response.json()["code"] == "PROJECT_NOT_FOUND"