diff --git a/docs/schemas.md b/docs/schemas.md index 08cb36b1..22718ae4 100644 --- a/docs/schemas.md +++ b/docs/schemas.md @@ -69,13 +69,41 @@ was created under, so a version read three milestones from now still says exactl said when the label was made. A version that could be edited would make that record a guess. -Creating a version identical to the active one is allowed. Versions are cheap, and refusing -a no-op would need an equality rule that then has to defend itself against reordered -attributes and changed colors. - Schema rows are deleted only as part of deleting their project, through the database's `ON DELETE CASCADE` - see [projects.md](projects.md). +## Publishing the contract already in force writes nothing + +`create_version` compares the classes it is given against the active version, and when they +are identical it returns that version and inserts no row: + +```python +first = schemas.create_version(project.id, [SIGN]) +again = schemas.create_version(project.id, [SIGN]) +assert again == first # one version, not two +``` + +It is not a refusal - the call succeeds, and the version the caller holds afterwards is the +one in force, which is the only thing it asked for. Over HTTP the answer is `201` either +way: the API declares one 2xx response per operation, and a client that branched on "did +this succeed" would see no difference in any case. + +**Identical means the classes compare equal** - names, geometries, colours, attributes and +order. That is deliberately *not* the same question as an empty diff: +[additive versus destructive](#additive-versus-destructive) classifies whether existing +annotations survive and ignores `color` on purpose, so gating this on the diff would answer +"saved" to somebody who changed a swatch and then throw the swatch away. Equality implies an +empty diff and never the reverse, so the diff stays the one definition of *changed in a way +that matters*. + +Only the **active** version is compared. Re-publishing an older version's classes is a real +change - it is what a revert is - and answering it with that old version would leave the +newer one in force. + +`description` and `provenance` are not part of the comparison, because they are not part of +the contract. A save that changes only the commit message is a no-op, and the message is not +recorded: there is no version for it to describe. + ## A version says why it exists, and when ```python diff --git a/frontend/ui-core/src/generated/api.ts b/frontend/ui-core/src/generated/api.ts index ddd0bf5f..2cf7e1f1 100644 --- a/frontend/ui-core/src/generated/api.ts +++ b/frontend/ui-core/src/generated/api.ts @@ -1691,6 +1691,13 @@ export interface paths { * * The body is the whole proposed version; versions are never edited in place. * + * **Sending the classes that are already in force writes nothing.** The answer + * is the version that was already active, and it is not an error: the version + * a client holds afterwards is the one in force either way, which is the only + * thing it asked for. Identical means the classes match exactly — names, + * geometries, colours, attributes and order — so a colour change is a change + * and does publish a version. + * * `description` is this version's commit message — written once, here, and * never afterwards, because a version is immutable and there is no route that * edits one. Blank is legal and comes back as null. `created_at` is stamped by diff --git a/frontend/ui-core/src/screens/SchemaEditor.tsx b/frontend/ui-core/src/screens/SchemaEditor.tsx index 3a4dbca9..dccfc21f 100644 --- a/frontend/ui-core/src/screens/SchemaEditor.tsx +++ b/frontend/ui-core/src/screens/SchemaEditor.tsx @@ -168,9 +168,41 @@ export interface SchemaEditorProps { readonly onDraftChange: (draft: SchemaDraft | null) => void; } -/** The comparison `dirty` has always used — deep, and cheap at a schema's size. */ +/** + * Two sets of classes describing the same contract, compared as contracts. + * + * Not `JSON.stringify` over the objects, which is what this used to be. The draft + * and the wire describe one class with differently *shaped* objects: a class + * added here is a literal in this file's key order, and one that came off the + * wire carries every optional field `LabelClassBody` declares — a hand-added + * attribute has no `options` key at all where the server sends `null`. Stringify + * calls those unequal, and a comparison that answers "changed" about an + * unchanged contract is exactly the bug this comparison exists to prevent. + * + * So the projection, in `LabelClassBody`'s own field order, with every optional + * field defaulted the way the wire defaults it. Cheap at a schema's size. + */ function same(a: readonly LabelClassBody[], b: readonly LabelClassBody[]): boolean { - return JSON.stringify(a) === JSON.stringify(b); + return canonical(a) === canonical(b); +} + +function canonical(classes: readonly LabelClassBody[]): string { + return JSON.stringify( + classes.map((declared) => [ + declared.name, + declared.geometry, + declared.color ?? null, + (declared.attributes ?? []).map((attribute) => [ + attribute.name, + attribute.kind, + attribute.required ?? false, + // Order is authored and part of the contract, so options are compared as + // given rather than sorted. + attribute.options ?? null, + attribute.default ?? null, + ]), + ]), + ); } export function SchemaEditor({ @@ -223,7 +255,24 @@ export function SchemaEditor({ const classes = showing.classes; const note = showing.note; const failure = publish.isError ? asApiError(publish.error) : null; - const dirty = !same(classes, showing.seed); + /** + * Whether saving would change anything — measured against **the version in + * force**, not against the snapshot the draft was seeded from. + * + * That is the same question `SchemaService.create_version` now answers, so the + * two cannot disagree: what the client declines to send is exactly what the + * kernel would decline to write. + * + * Measuring it against `seed` was the defect. The re-base that refreshes + * `seed` after a save rides on the callback passed to `publish.mutate`, and + * TanStack drops those when the observer's component unmounts — which is what + * happens on a project that had no schema, because the invalidated 404 goes + * back to `pending` and `SchemaSection` swaps this editor for a loading state + * while the refetch flies. The draft came back holding an empty `seed`, read + * as dirty, and a second press published a version identical to the first. + * `active` is a prop and cannot be missed that way. + */ + const dirty = !same(classes, active?.classes ?? []); /** * The version that arrived while this draft was being written, or `null`. * diff --git a/frontend/ui-core/src/screens/schemaDraft.test.tsx b/frontend/ui-core/src/screens/schemaDraft.test.tsx index b47527ac..efd8b1f6 100644 --- a/frontend/ui-core/src/screens/schemaDraft.test.tsx +++ b/frontend/ui-core/src/screens/schemaDraft.test.tsx @@ -28,6 +28,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { JSX, ReactNode } from "react"; import { ApiProvider } from "../data/ApiProvider"; +import { Toaster } from "../primitives/Feedback"; import { writeToken } from "../data/session"; import { ProjectScreen } from "./ProjectScreen"; @@ -45,7 +46,7 @@ let activeSchema: { project_id: string; version: number; classes: unknown[] }; /** How many times it was asked, so "the refetch fired" is observed rather than assumed. */ let schemaReads = 0; -type Answer = { status: number; body?: unknown }; +type Answer = { status: number; body?: unknown; delay?: number }; let handlers: ((request: Request) => Answer | undefined)[] = []; beforeEach(() => { @@ -115,11 +116,18 @@ const DATASET = { }; function respond(answer: Answer): Promise { - return Promise.resolve( + if (answer.delay !== undefined) { + return new Promise((resolve) => setTimeout(() => resolve(built(answer)), answer.delay)); + } + return Promise.resolve(built(answer)); +} + +function built(answer: Answer): Response { + return ( new Response(answer.status === 204 ? null : JSON.stringify(answer.body ?? null), { status: answer.status, headers: { "content-type": "application/json" }, - }), + }) ); } @@ -291,5 +299,143 @@ describe("the schema draft survives a version published underneath", () => { }); }); +/** + * The third way a draft goes wrong, and the only one that reaches the wire. + * + * The two above are about a draft being *lost*. This one is about a draft whose + * baseline is stale: the editor believes there is still something to save, so a + * second press of Save publishes a version identical to the one it just made. + * + * It reproduces only on a project that had **no** schema, and the mechanism is + * why: `useActiveSchema` answers 404 there and therefore holds no data, so the + * invalidation the save triggers puts that query back into `pending` rather than + * leaving it in `error` (TanStack's `fetchState` resets the status whenever + * `data === undefined`), `SchemaSection` swaps the editor for a `LoadingState`, + * and the `mutate()`-level `onSuccess` that re-bases the draft is dropped with + * the unmounted observer. On a project that already had a version the query has + * data, nothing unmounts, and the re-base fires — which is the whole of why this + * needs its own fixture rather than a line in the block above. + * + * **The `delay` on that stub is load-bearing, and it is the honest model rather + * than a contrivance.** A stubbed `fetch` that resolves in the same microtask + * never lets React commit the pending render, so the editor never unmounts and + * the defect vanishes — measured: with an instant stub this test passed against + * the unfixed code. Every real request takes longer than zero. + */ +describe("saving twice with nothing edited in between", () => { + it("issues one request on a project that had no schema", async () => { + let published: { project_id: string; version: number; classes: unknown[] } | null = null; + let posts = 0; + // Stateful on purpose: the 404 is the *state* this defect needs, and a frozen + // stub would answer 404 forever and never let the save land. + handlers.push((request) => { + const path = new URL(request.url).pathname; + if (request.method === "GET" && /\/schema$/.test(path)) { + schemaReads += 1; + return published === null + ? { status: 404, body: { code: "SCHEMA_NOT_FOUND", message: "no schema yet" } } + : { status: 200, body: published, delay: 5 }; + } + if (request.method === "POST" && /\/schema\/versions$/.test(path)) { + posts += 1; + published = { + project_id: PROJECT, + version: (published?.version ?? 0) + 1, + classes: [PEDESTRIAN], + }; + return { status: 201, body: published }; + } + return undefined; + }); + + render( + mount( + <> + + + , + ), + ); + await screen.findByTestId("schema-editor"); + // Class zero, not two: a project with no schema seeds an empty draft, so + // `draftAClass` above — which counts from the fixture's two — cannot be used. + await userEvent.click(screen.getByTestId("add-class")); + await userEvent.type(screen.getByTestId("class-name-0"), "pedestrian"); + + await userEvent.click(screen.getByTestId("save-schema")); + await waitFor(() => + expect(screen.getByTestId("schema-status").textContent).toContain("Version 1 active"), + ); + + await userEvent.click(screen.getByTestId("save-schema")); + + // `DESIGN.md`: pressing Save with nothing to save answers, and issues no + // request. The count is the claim; the toast is what the person sees. + expect(posts).toBe(1); + expect(screen.getByTestId("schema-status").textContent).not.toContain("unsaved"); + expect(await screen.findByText("No changes to save")).toBeDefined(); + }); + + /** + * The same defect through the comparison rather than through the baseline. + * + * A class added here is a literal in `SchemaEditor`'s own key order and a + * *new attribute* has no `options` key at all, where the wire sends every + * optional field `AttributeBody` declares. `JSON.stringify` over those two + * objects is unequal for one identical contract — so the draft reads as dirty + * against the version it just published, and presses Save again. That is why + * the comparison is a projection and not a stringify, and this is the test + * that notices if it goes back. + */ + it("compares a hand-built attribute with the wire's own spelling of it", async () => { + let published: { project_id: string; version: number; classes: unknown[] } | null = null; + let posts = 0; + handlers.push((request) => { + const path = new URL(request.url).pathname; + if (request.method === "GET" && /\/schema$/.test(path)) { + schemaReads += 1; + return published === null + ? { status: 404, body: { code: "SCHEMA_NOT_FOUND", message: "no schema yet" } } + : { status: 200, body: published, delay: 5 }; + } + if (request.method === "POST" && /\/schema\/versions$/.test(path)) { + posts += 1; + published = { + project_id: PROJECT, + version: (published?.version ?? 0) + 1, + // What the server actually sends back: `AttributeBody` in full, with + // the `options` the editor's own literal never carries. + classes: [ + { + ...PEDESTRIAN, + attributes: [ + { name: "occluded", kind: "string", required: false, options: null, default: null }, + ], + }, + ], + }; + return { status: 201, body: published }; + } + return undefined; + }); + + render(mount()); + await screen.findByTestId("schema-editor"); + await userEvent.click(screen.getByTestId("add-class")); + await userEvent.type(screen.getByTestId("class-name-0"), "pedestrian"); + await userEvent.click(screen.getByTestId("add-attribute-0")); + await userEvent.type(screen.getByTestId("attr-name-0-0"), "occluded"); + + await userEvent.click(screen.getByTestId("save-schema")); + await waitFor(() => + expect(screen.getByTestId("schema-status").textContent).toContain("Version 1 active"), + ); + + await userEvent.click(screen.getByTestId("save-schema")); + + expect(posts).toBe(1); + }); +}); + const PEDESTRIAN = { name: "pedestrian", geometry: "bbox", color: null, attributes: [] }; const TRAFFIC_LIGHT = { name: "traffic light", geometry: "bbox", color: null, attributes: [] }; diff --git a/openapi.json b/openapi.json index bee8dc96..91f6e2d5 100644 --- a/openapi.json +++ b/openapi.json @@ -10147,7 +10147,7 @@ ] }, "post": { - "description": "Append the next version of the project's schema.\n\nThe body is the whole proposed version; versions are never edited in place.\n\n`description` is this version's commit message \u2014 written once, here, and\nnever afterwards, because a version is immutable and there is no route that\nedits one. Blank is legal and comes back as null. `created_at` is stamped by\nthe server, so it is a response field and not a request one.\n\n`provenance` says which kind of work is publishing: `curated` for a version\nauthored in a schema editor, `annotation` for one that fell out of adding a\nclass while labeling. It is stored exactly as sent and never inferred, so a\nclient with no opinion omits it and the version records null \u2014 which readers\ngroup with `curated`. It gates nothing and changes no behaviour; it exists so\na version history can separate the milestones from the runs.\n\nRemoving a class or an attribute answers 409 `DESTRUCTIVE_SCHEMA_CHANGE`\nuntil `allow_destructive=true` says so deliberately. If annotations already\nexist under an affected class it answers 409 `SCHEMA_CHANGE_WOULD_ORPHAN`\ninstead, and **no flag overrides that one** \u2014 which is why a client branches\non `code` and not on the status.", + "description": "Append the next version of the project's schema.\n\nThe body is the whole proposed version; versions are never edited in place.\n\n**Sending the classes that are already in force writes nothing.** The answer\nis the version that was already active, and it is not an error: the version\na client holds afterwards is the one in force either way, which is the only\nthing it asked for. Identical means the classes match exactly \u2014 names,\ngeometries, colours, attributes and order \u2014 so a colour change is a change\nand does publish a version.\n\n`description` is this version's commit message \u2014 written once, here, and\nnever afterwards, because a version is immutable and there is no route that\nedits one. Blank is legal and comes back as null. `created_at` is stamped by\nthe server, so it is a response field and not a request one.\n\n`provenance` says which kind of work is publishing: `curated` for a version\nauthored in a schema editor, `annotation` for one that fell out of adding a\nclass while labeling. It is stored exactly as sent and never inferred, so a\nclient with no opinion omits it and the version records null \u2014 which readers\ngroup with `curated`. It gates nothing and changes no behaviour; it exists so\na version history can separate the milestones from the runs.\n\nRemoving a class or an attribute answers 409 `DESTRUCTIVE_SCHEMA_CHANGE`\nuntil `allow_destructive=true` says so deliberately. If annotations already\nexist under an affected class it answers 409 `SCHEMA_CHANGE_WOULD_ORPHAN`\ninstead, and **no flag overrides that one** \u2014 which is why a client branches\non `code` and not on the status.", "operationId": "create_schema_version", "parameters": [ { diff --git a/src/visionset/cli/schemas.py b/src/visionset/cli/schemas.py index 3aa139d0..28d60748 100644 --- a/src/visionset/cli/schemas.py +++ b/src/visionset/cli/schemas.py @@ -96,9 +96,10 @@ def schema_apply( ) -> None: """Add the next schema version from a JSON file. - Versions are 1..N and none of them ever changes, so this always *adds* one — - there is no edit and no rollback, and applying an unchanged document still - makes a new version. + Versions are 1..N and none of them ever changes, so this *adds* one — there + is no edit and no rollback. Applying the document already in force adds + nothing and prints the version that was already there, so re-running this in + a script is free. A change that removes a class or an attribute, or narrows one, is refused until `--allow-destructive`. A change that would orphan annotations already diff --git a/src/visionset/kernel/services/schema_service.py b/src/visionset/kernel/services/schema_service.py index a38883b1..bc267761 100644 --- a/src/visionset/kernel/services/schema_service.py +++ b/src/visionset/kernel/services/schema_service.py @@ -176,6 +176,26 @@ def create_version( 1..N with no gaps and no reuse. Nothing is edited: this always inserts, which is what makes an old version safe to read forever. + **Publishing the contract that is already in force writes nothing** and + returns the active version unchanged. A history whose entries include + "and then nothing changed" is a history somebody has to read past, and a + schema editor that answers "saved" is impossible to tell from one that + answered "already saved" — so the two are made the same thing. + + Identical means the classes compare equal, which — the models being + frozen — covers names, geometries, colours, attributes and order. That is + deliberately *not* the same question as an empty ``diff_classes``: + ``domain/schema_diff.py`` classifies whether existing annotations survive + and ignores ``color`` on purpose, so gating this on the diff would answer + "saved" to somebody who changed a swatch and then discard the swatch. + Equality implies an empty diff, never the other way round, so the diff + remains the one definition of *changed in a way that matters* and no + second one is written here. + + Only the **active** version is compared. Re-publishing an older + version's contract is a real change — it is what a revert is — and + answering it with that old version would leave the newer one in force. + ``description`` is the version's **commit message**: written here and never afterwards, because there is no ``update`` on this service and the model is frozen. Blank is legal and stored as ``None`` — an empty commit @@ -216,6 +236,9 @@ def create_version( _require_coherent(proposed) active = self.active(uow, project_id) + if active is not None and proposed == active.classes: + return active + diff = diff_classes(() if active is None else active.classes, proposed) if diff.is_destructive: self._refuse_narrowing(uow, project_id, diff, allow_destructive) diff --git a/src/visionset/mcp/schemas.py b/src/visionset/mcp/schemas.py index 795c197b..84c4015b 100644 --- a/src/visionset/mcp/schemas.py +++ b/src/visionset/mcp/schemas.py @@ -176,10 +176,12 @@ def create_schema_version( ) -> dict[str, Any]: """Create the next schema version from a complete list of classes. - Versions are 1..N and never edited or deleted; this always inserts a new one - and the highest becomes active. Batches already approved keep the version - they pinned, so this does not retroactively change how existing work is - judged. + Versions are 1..N and never edited or deleted; this inserts a new one and the + highest becomes active. Batches already approved keep the version they + pinned, so this does not retroactively change how existing work is judged. + + Sending the classes already in force adds nothing and returns the version + that was already active, so calling this to be sure is free. Send the whole contract every time — a class omitted is a class removed. Call `preview_schema_change` first if you are not creating the first version. diff --git a/src/visionset/server/routes/schemas.py b/src/visionset/server/routes/schemas.py index c0988548..77c964d3 100644 --- a/src/visionset/server/routes/schemas.py +++ b/src/visionset/server/routes/schemas.py @@ -64,6 +64,13 @@ def create_schema_version( The body is the whole proposed version; versions are never edited in place. + **Sending the classes that are already in force writes nothing.** The answer + is the version that was already active, and it is not an error: the version + a client holds afterwards is the one in force either way, which is the only + thing it asked for. Identical means the classes match exactly — names, + geometries, colours, attributes and order — so a colour change is a change + and does publish a version. + `description` is this version's commit message — written once, here, and never afterwards, because a version is immutable and there is no route that edits one. Blank is legal and comes back as null. `created_at` is stamped by diff --git a/tests/cli/test_schema_commands.py b/tests/cli/test_schema_commands.py index e0d9f9a4..2847b0f5 100644 --- a/tests/cli/test_schema_commands.py +++ b/tests/cli/test_schema_commands.py @@ -53,12 +53,16 @@ def test_apply_creates_version_one(root: Path, tmp_path: Path) -> None: assert ok(root, "schema", "apply", str(schema_file(tmp_path)), "-p", "road-signs") == "1" -def test_applying_again_creates_the_next_version(root: Path, tmp_path: Path) -> None: - # Versions are 1..N and none of them changes, so an unchanged document still - # adds one — there is no edit and no rollback. +def test_applying_the_same_document_again_adds_nothing(root: Path, tmp_path: Path) -> None: + """Which is what makes this safe to leave in a provisioning script. + + The version already in force is what comes back, so the caller reads the same + number twice rather than a history of versions that changed nothing. + """ file = schema_file(tmp_path) ok(root, "schema", "apply", str(file), "-p", "road-signs") - assert ok(root, "schema", "apply", str(file), "-p", "road-signs") == "2" + assert ok(root, "schema", "apply", str(file), "-p", "road-signs") == "1" + assert len(payload(root, "schema", "list", "-p", "road-signs")["items"]) == 1 def test_the_classes_survive_the_round_trip(root: Path, tmp_path: Path) -> None: diff --git a/tests/kernel/test_schema_service.py b/tests/kernel/test_schema_service.py index fc973a96..38d91328 100644 --- a/tests/kernel/test_schema_service.py +++ b/tests/kernel/test_schema_service.py @@ -108,8 +108,10 @@ def test_the_first_version_of_a_schema_is_one(tmp_path: Path) -> None: def test_versions_are_numbered_one_past_the_highest_stored(tmp_path: Path) -> None: workspace, projects, schemas = _services(tmp_path) project = projects.create("signs") - for expected in (1, 2, 3): - assert schemas.create_version(project.id, [SIGN, LANE]).version == expected + # A different contract each time, because publishing the one already in force + # is a no-op — see `test_an_identical_version_is_a_no_op`. + for expected, classes in enumerate(([SIGN], [SIGN, LANE], [SIGN, LANE, RICH]), start=1): + assert schemas.create_version(project.id, classes).version == expected assert [s.version for s in schemas.list_versions(project.id)] == [1, 2, 3] workspace.close() @@ -144,16 +146,72 @@ def test_creating_a_version_never_rewrites_an_earlier_one(tmp_path: Path) -> Non workspace.close() -def test_an_identical_version_is_still_a_new_version(tmp_path: Path) -> None: - """Versions are cheap, and refusing a no-op would need an equality rule we - would then have to defend against reordering and colors.""" +def test_an_identical_version_is_a_no_op(tmp_path: Path) -> None: + """Publishing the contract that is already in force writes nothing. + + Replaces `test_an_identical_version_is_still_a_new_version`, which asserted + the behaviour reported as a defect: a schema editor that saved twice with no + edits in between left a v2 the version panel itself described as "nothing + changed". That test's docstring worried an equality rule would have to be + defended against reordering and colors — it does not, because the rule is + equality of the stored classes and therefore *includes* both. + """ workspace, projects, schemas = _services(tmp_path) project = projects.create("signs") first = schemas.create_version(project.id, [SIGN]) again = schemas.create_version(project.id, [SIGN]) - assert (again.version, again.classes) == (2, first.classes) - assert again.id != first.id + assert again == first + assert [s.version for s in schemas.list_versions(project.id)] == [1] + workspace.close() + + +def test_an_identical_version_is_a_no_op_only_against_the_active_one(tmp_path: Path) -> None: + """Only the version in force is compared — an earlier one does not match. + + Otherwise reverting to v1's contract from v2 would silently answer v1 and + leave v2 active, which is the opposite of what the caller asked for. + """ + workspace, projects, schemas = _services(tmp_path) + project = projects.create("signs") + schemas.create_version(project.id, [SIGN]) + schemas.create_version(project.id, [SIGN, LANE]) + back = schemas.create_version(project.id, [SIGN], allow_destructive=True) + + assert back.version == 3 + workspace.close() + + +def test_a_colour_only_change_is_a_change(tmp_path: Path) -> None: + """The boundary of the no-op rule, and the reason it is equality not the diff. + + `diff_classes` deliberately ignores `color` — it classifies whether existing + annotations survive, and a swatch does not decide that. So an empty diff is + *not* the same question as identical content, and gating the no-op on the + diff would answer "saved" to somebody who changed a colour and then throw the + colour away. + """ + workspace, projects, schemas = _services(tmp_path) + project = projects.create("signs") + schemas.create_version(project.id, [SIGN]) + recoloured = schemas.create_version( + project.id, [LabelClass(name="sign", geometry=GeometryType.BBOX, color="#eb5a47")] + ) + + assert recoloured.version == 2 + assert recoloured.classes[0].color == "#eb5a47" + workspace.close() + + +def test_reordering_the_classes_is_a_change(tmp_path: Path) -> None: + """Order is part of what a version stores — it is the palette's own order.""" + workspace, projects, schemas = _services(tmp_path) + project = projects.create("signs") + schemas.create_version(project.id, [SIGN, LANE]) + swapped = schemas.create_version(project.id, [LANE, SIGN]) + + assert swapped.version == 2 + assert [c.name for c in swapped.classes] == ["lane", "sign"] workspace.close() @@ -740,10 +798,16 @@ def test_provenance_is_not_part_of_what_a_version_declares(tmp_path: Path) -> No workspace, projects, schemas = _services(tmp_path) project = projects.create("roads") schemas.create_version(project.id, [SIGN], provenance=SchemaProvenance.CURATED) - schemas.create_version(project.id, [SIGN], provenance=SchemaProvenance.ANNOTATION) + # Two versions declaring the same classes have to be reached the long way + # round now: republishing the active contract writes nothing, so v3 gets back + # to v1's classes through a v2 that differs. + schemas.create_version(project.id, [SIGN, LANE], provenance=SchemaProvenance.CURATED) + schemas.create_version( + project.id, [SIGN], provenance=SchemaProvenance.ANNOTATION, allow_destructive=True + ) - assert schemas.compare(project.id, 1, 2).changes == () - assert not schemas.compare(project.id, 1, 2).is_destructive + assert schemas.compare(project.id, 1, 3).changes == () + assert not schemas.compare(project.id, 1, 3).is_destructive workspace.close() @@ -753,7 +817,7 @@ def test_each_version_carries_its_own_provenance(tmp_path: Path) -> None: project = projects.create("roads") schemas.create_version(project.id, [SIGN], provenance=SchemaProvenance.CURATED) schemas.create_version(project.id, [SIGN, LANE], provenance=SchemaProvenance.ANNOTATION) - schemas.create_version(project.id, [SIGN, LANE]) + schemas.create_version(project.id, [SIGN, LANE, RICH]) assert [v.provenance for v in schemas.list_versions(project.id)] == [ SchemaProvenance.CURATED, diff --git a/tests/server/test_schemas.py b/tests/server/test_schemas.py index 84fd1100..5b649bea 100644 --- a/tests/server/test_schemas.py +++ b/tests/server/test_schemas.py @@ -82,6 +82,39 @@ def test_the_next_version_is_numbered_one_higher(client: TestClient, project: st assert response.json()["version"] == 2 +def test_sending_the_classes_already_in_force_writes_nothing( + client: TestClient, project: str +) -> None: + """Not an error, and not a second version. + + The status is 201 either way, deliberately: the contract declares one 2xx + response per operation — `scripts/generate_client.mjs` enforces it — and the + version a client holds afterwards is the one in force whichever branch it + took, which is the only thing it asked for. + """ + first = post_version(client, project, a_class()) + assert first.status_code == 201 + + again = post_version(client, project, a_class()) + + assert again.status_code == 201 + assert again.json() == first.json() + assert client.get(f"/projects/{project}/schema/versions").json()["total"] == 1 + + +def test_a_colour_only_change_is_a_change_and_publishes_a_version( + client: TestClient, project: str +) -> None: + """Identity is the stored classes, not the diff — `color` is inside one and + deliberately outside the other.""" + post_version(client, project, a_class()) + + response = post_version(client, project, a_class(color="#eb5a47")) + + assert response.status_code == 201 + assert response.json()["version"] == 2 + + def test_the_active_version_is_the_highest_one(client: TestClient, project: str) -> None: post_version(client, project, a_class()) post_version(client, project, a_class(), a_class("lane")) @@ -534,7 +567,9 @@ def test_the_listing_carries_each_versions_own_provenance(client: TestClient, pr f"/projects/{project}/schema/versions", json={"classes": [a_class("sign"), a_class("lane")], "provenance": "annotation"}, ) - post_version(client, project, a_class("sign"), a_class("lane")) + # A third contract rather than a repeat of the second: republishing the + # classes already in force answers 200 and writes no version. + post_version(client, project, a_class("sign"), a_class("lane"), a_class("kiosk")) listed = client.get(f"/projects/{project}/schema/versions").json()["items"]