fix(metadata): capabilities.write 同时约束 save() —— 可写 datasource loader 必须实现写入与删除两半 (#5654)

.changeset/metadata-loader-save-contract.md

@@ -0,0 +1,50 @@
+---
+"@objectstack/metadata": patch
+---
+
+fix(metadata): `capabilities.write` now also binds `save()` — a writable datasource loader must implement both halves of the write (#5654)
+
+#5276 (shipped in v17.0.0-rc) made `capabilities.write` binding on `delete()`:
+a loader declaring `protocol: 'datasource:'` with `capabilities.write: true`
+and no `delete()` is refused at registration, because `unregister()` used to
+skip it silently and announce the deletion anyway. The gate stopped there, so
+**one declaration was binding at one end of an item's life and decorative at
+the other**.
+
+`MetadataManager.register()` had the identical hole one direction over. Its
+persistence loop read `loader.save &&` first, so a `datasource:` loader
+declaring `capabilities.write: true` **without** a `save()` method was
+**silently skipped** — no warn, no error. `register()` then wrote the in-memory
+registry, invalidated the list cache, announced `created`/`updated` and notified
+watchers, so the caller (Studio/Setup, REST PUT, the CLI, a package publish) was
+told the write succeeded. The item read back correctly for the life of the
+process and was **gone at the next restart**, with nothing to retry it — a
+durability degradation that leaves the system looking entirely healthy.
+
+`registerLoader()`'s gate (renamed `assertWritableLoaderContract`) now requires
+**both** `save()` and `delete()` for that combination, and rejects with one
+message naming which method is missing, the consequence, and both repairs.
+`registerLoader()` is the sole writer of the loader map — the constructor's
+`config.loaders` funnel through it — so the combination can no longer reach the
+runtime and lose a write there. The `save` short-circuit inside `register()`
+survives as defensive code whose unreachability is now guaranteed by
+construction, exactly like `unregister()`'s.
+
+**Does this affect you?** Only if you register a custom metadata loader that
+declares `protocol: 'datasource:'` with `capabilities.write: true`. If it does
+and has no `save()`, registration now throws where it previously succeeded and
+quietly discarded your writes. Two ways to fix it, both stated in the error:
+
+1. implement
+   `async save(type: string, name: string, data: any, options?: MetadataSaveOptions): Promise<MetadataSaveResult>`
+   on the loader, persisting the item into its store (`DatabaseLoader` in this
+   package is the reference implementation); or
+2. if the loader is genuinely read-only, declare `capabilities.write: false` — a
+   read-only `datasource:` loader registers without complaint and is never
+   written to in the first place.
+
+Loaders on the other protocols (`file:`, `memory:`, `http:`, `s3:`) are
+unaffected: `MetadataManager` never persists to them at runtime, so they may
+declare `capabilities.write` without a `save()`/`delete()` exactly as before.
+The one `datasource:` loader shipped in this package, `DatabaseLoader`, has
+always implemented both and is unchanged.

packages/metadata/src/loaders/loader-interface.ts

@@ -73,7 +73,27 @@ export interface MetadataLoader {
   list(type: string): Promise<string[]>;
 
   /**
-   * Save metadata item
+   * Save metadata item into this loader's store.
+   *
+   * [#5654] Optional on the interface, **mandatory for a `datasource:` loader
+   * that declares `capabilities.write`** — `MetadataManager.registerLoader()`
+   * refuses to register such a loader when this method is missing, so the
+   * combination "declared writable, cannot persist" never reaches the runtime.
+   *
+   * The reason it is enforced at registration rather than tolerated at the write
+   * site: `MetadataManager.register()` persists into every writable
+   * `datasource:` loader, and it used to read `loader.save &&` first — a loader
+   * that declares it can be written to but has no `save()` made every write a
+   * silent lie. `register()` would skip it, then write the in-memory registry,
+   * invalidate the list cache, announce a `created`/`updated` event and notify
+   * watchers, so the caller is told the write succeeded; the item reads back
+   * correctly for the life of the process and is **gone at the next restart**,
+   * with nothing to retry it.
+   *
+   * Loaders on the other protocols (`file:`, `memory:`, `http:`, `s3:`) are not
+   * gated: `MetadataManager` never persists to them at runtime — `register()`
+   * filters on `datasource:` — so a missing `save()` there loses nothing.
+   *
    * @param type The metadata type
    * @param name The item name
    * @param data The data to save
@@ -89,8 +109,8 @@ export interface MetadataLoader {
   /**
    * Delete a metadata item from this loader's store.
    *
-   * [#5276] Optional on the interface, **mandatory for a `datasource:` loader
-   * that declares `capabilities.write`** — `MetadataManager.registerLoader()`
+   * [#5276, #5654] Optional on the interface, **mandatory for a `datasource:`
+   * loader that declares `capabilities.write`** — `MetadataManager.registerLoader()`
    * refuses to register such a loader when this method is missing, so the
    * combination "declared writable, cannot delete" never reaches the runtime.
    *
@@ -105,6 +125,12 @@ export interface MetadataLoader {
    * therefore means *both* directions of the write, on both ends of the item's
    * life — declared = enforced.
    *
+   * One gate covers both halves: `assertWritableLoaderContract` in
+   * `metadata-manager.ts` requires `save()` **and** `delete()` for this
+   * combination and names whichever is missing. #5276 built it for `delete`;
+   * #5654 widened it to `save`, which had the identical silent skip in
+   * `register()` — see the note on `save?` above.
+   *
    * Loaders on the other protocols (`file:`, `memory:`, `http:`, `s3:`) are not
    * gated: `MetadataManager` never writes to them at runtime, so it never has a
    * deletion of its own to take back.

packages/metadata/src/metadata-manager-loader-delete-contract.test.ts

@@ -35,6 +35,14 @@
  *      register without a `delete`, because the manager never writes to them;
  *   5. `DatabaseLoader`, the repo's only real `datasource:` loader, passes the
  *      gate unchanged.
+ *
+ * [#5654] The gate this file pins has since been widened — it is
+ * `assertWritableLoaderContract` now, and `capabilities.write` requires `save()`
+ * as well, because `register()` had the identical silent skip one direction
+ * over. Everything below still holds verbatim: these loaders all implement
+ * `save`, so `delete` is the only thing missing and the message is unchanged.
+ * The `save` half is pinned next door in
+ * `metadata-manager-loader-save-contract.test.ts`.
  */
 
 import { describe, it, expect, vi, beforeEach } from 'vitest';