diff --git a/acs-data-access/lib/api-v1.js b/acs-data-access/lib/api-v1.js index 0e298c02e..49f7ed05a 100644 --- a/acs-data-access/lib/api-v1.js +++ b/acs-data-access/lib/api-v1.js @@ -79,23 +79,94 @@ export class APIv1 { } + /** Finds every dataset whose stored config points at the given dataset. + * + * Reads the structure apps straight from ConfigDB rather than using the + * derived dataset map, so datasets that are currently invalid are still + * checked. An invalid dataset keeps its config document, so it can still + * be left holding a dangling reference. + * + * @param dataset_uuid The dataset that is about to be deleted. + * @returns {Promise>} + */ + async find_referrers(dataset_uuid) { + const referrers = []; + + for (const [structure, handler] of Object.entries(this.handlers)) { + const configs = await rx.firstValueFrom(this.cdb.search_app(structure)); + if (!configs) continue; + + for (const [uuid, config] of configs) { + if (uuid === dataset_uuid) continue; + + if (handler.references(config, dataset_uuid)) + referrers.push({ dataset: uuid, structure }); + } + } + + return referrers; + } + + /** GET. Deletes a dataset. + * + * Refuses with 409 while another dataset still references this one, and + * returns the list of referrers so the caller can deal with them. See the + * comment below for why we refuse rather than rewrite the referrers. + */ async delete_dataset(req, res){ const dataset_uuid = req.params.uuid; if(!dataset_uuid) return fail(this.log, 422, `No req.params.uuid`); if(!valid_uuid(dataset_uuid)) return fail(this.log, 422, `Invalid uuid ${dataset_uuid}`); - + const ok = await this.auth.check_acl( req.auth, Constants.Perm.DeleteDataset, dataset_uuid, true, ); - + if (!ok) return fail(this.log, 403, `You don't have DELETE permissions for ${dataset_uuid}`); this.log(`Delete dataset called by ${req.auth} for ${dataset_uuid}`); - // remove all subclass relationships before deleting + /* Refuse the delete while anything still points here. + * + * The alternative is to edit the referrers. We do not, for three + * reasons. The caller holds DeleteDataset on this dataset only, so + * editing other datasets would change data they may have no permission + * to touch. A UnionComponents list can lose one entry and still mean + * something, but a SessionLimits dataset is a time window over its + * source, so removing the source leaves a window over nothing and there + * is no sensible repair. And refusing writes nothing at all, so a failed + * delete cannot leave the graph half updated. + * + * Callers delete from the top down: remove the union or session first, + * then its components. */ + const referrers = await this.find_referrers(dataset_uuid); + + if (referrers.length > 0) { + this.log(`Refusing to delete ${dataset_uuid}: referenced by %o`, referrers); + + return res.status(409).json({ + error: "dataset_in_use", + dataset: dataset_uuid, + message: `Dataset ${dataset_uuid} is still referenced by ${referrers.length} other dataset(s). Delete or update them first.`, + referrers, + }); + } + + /* Remove the links this dataset owns. The handler knows which way round + * its links point: a union is the superclass of its components, a + * session is a subclass of its source. */ + const datasets = await rx.firstValueFrom(this.data.datasets); + const dataset = datasets.get(dataset_uuid); + + if (dataset?.config && this.handlers[dataset.structure]) { + await this.handlers[dataset.structure] + .remove_subclass_relationships(dataset_uuid, dataset.config); + } + + // remove any remaining subclass relationships before deleting const subclasses = await this.cdb.class_direct_subclasses(dataset_uuid); if(subclasses){ for(let s of subclasses){ diff --git a/acs-data-access/lib/base-structure-handler.js b/acs-data-access/lib/base-structure-handler.js index 84f6b4224..10e82d797 100644 --- a/acs-data-access/lib/base-structure-handler.js +++ b/acs-data-access/lib/base-structure-handler.js @@ -11,4 +11,16 @@ export class BaseStructureHandler { resolve(ctx) {} create_subclass_relationships(datasetUuid, config) {} remove_subclass_relationships(datasetUuid, config) {} + + /** Does this config point at the given dataset? + * + * Used by the delete path to find datasets that would be left with a + * dangling reference. Structures that only point at non-dataset objects + * return false. + * + * @param config The stored config document of another dataset. + * @param target_uuid The dataset UUID we are about to delete. + * @returns {boolean} + */ + references(config, target_uuid) { return false; } } \ No newline at end of file diff --git a/acs-data-access/lib/session-limits-handler.js b/acs-data-access/lib/session-limits-handler.js index 382cf685f..3fdcaf49f 100644 --- a/acs-data-access/lib/session-limits-handler.js +++ b/acs-data-access/lib/session-limits-handler.js @@ -77,7 +77,13 @@ export class SessionLimitsHandler extends BaseStructureHandler { ); } + references(config, target_uuid) { + return config?.source === target_uuid; + } + async remove_subclass_relationships(dataset_uuid, config) { + if(!config?.source) return; + await this.cdb.class_remove_subclass(config.source, dataset_uuid); this.log(`Removed ${dataset_uuid} from ${config.source} subclasses.`) } diff --git a/acs-data-access/lib/sparkplug-sources-handler.js b/acs-data-access/lib/sparkplug-sources-handler.js index cdbed1308..12670f005 100644 --- a/acs-data-access/lib/sparkplug-sources-handler.js +++ b/acs-data-access/lib/sparkplug-sources-handler.js @@ -48,6 +48,12 @@ export class SparkplugSourcesHandler extends BaseStructureHandler { }]; } + /* A SparkplugSrc points at a device, not at another dataset, so it can + * never hold a dangling dataset reference. */ + references() { + return false; + } + async create_subclass_relationships() { return; } diff --git a/acs-data-access/lib/unions-components-handler.js b/acs-data-access/lib/unions-components-handler.js index c5328f100..2e74345e6 100644 --- a/acs-data-access/lib/unions-components-handler.js +++ b/acs-data-access/lib/unions-components-handler.js @@ -74,8 +74,14 @@ export class UnionComponentsHandler extends BaseStructureHandler { } } + references(config, target_uuid) { + if(!Array.isArray(config)) return false; + + return config.includes(target_uuid); + } + async remove_subclass_relationships(dataset_uuid, config) { - if(config.length <= 0) return; + if(!Array.isArray(config) || config.length <= 0) return; for (const src of config){ await this.cdb.class_remove_subclass(dataset_uuid, src); diff --git a/acs-data-access/package.json b/acs-data-access/package.json index 28e573c8f..b4c9da985 100644 --- a/acs-data-access/package.json +++ b/acs-data-access/package.json @@ -18,6 +18,7 @@ "@influxdata/influxdb-client": "^1.35.0", "deep-equal": "^2.2.3", "express": "^5.0.1", + "immutable": "^5.0.0", "p-limit": "^7.3.0", "rxjs": "^7.8.2", "stream": "^0.0.3", diff --git a/acs-data-access/tests/current/delete_dataset_referrers.test.js b/acs-data-access/tests/current/delete_dataset_referrers.test.js new file mode 100644 index 000000000..6c532a624 --- /dev/null +++ b/acs-data-access/tests/current/delete_dataset_referrers.test.js @@ -0,0 +1,281 @@ +/* + * ACS Data Access Service + * Unit tests for the delete path's referential integrity check. + * + * These run against fakes, so they need no cluster. + */ + +import { Map as IMap } from "immutable"; +import * as rx from "rxjs"; +import { describe, expect, test } from "vitest"; + +import { APIv1 } from "../../lib/api-v1.js"; +import { DataAccess as Constants } from "../../lib/constants.js"; + +const SRC_A = "11111111-1111-1111-1111-111111111111"; +const SRC_B = "22222222-2222-2222-2222-222222222222"; +const UNION = "33333333-3333-3333-3333-333333333333"; +const SESSION = "44444444-4444-4444-4444-444444444444"; +const DEVICE = "55555555-5555-5555-5555-555555555555"; + +/* Builds an APIv1 wired to fakes. + * + * `datasets` maps a dataset UUID to { structure, config }, the same shape + * DataFlow produces. The fake ConfigDB derives its per-app search results + * from that map, so the tests only describe the graph once. + */ +function make_api(datasets, raw_configs) { + const calls = { + deleted: [], + removed_subclasses: [], + }; + + const cdb = { + search_app(app) { + /* raw_configs lets a test give ConfigDB a config document that the + * derived dataset map does not carry, which is what happens when a + * dataset is invalid. */ + if (raw_configs) + return rx.of(IMap(raw_configs[app] ?? {})); + + let configs = IMap(); + + for (const [uuid, def] of Object.entries(datasets)) { + if (def.structure === app && def.config != null) + configs = configs.set(uuid, def.config); + } + + return rx.of(configs); + }, + + async class_direct_subclasses() { return []; }, + + async class_remove_subclass(klass, sub) { + calls.removed_subclasses.push([klass, sub]); + }, + + async delete_object(uuid) { calls.deleted.push(uuid); }, + }; + + const api = new APIv1({ + data: { datasets: rx.of(IMap(datasets)) }, + auth: { async check_acl() { return true; } }, + cdb, + debug: { bound: () => () => {} }, + influxReader: {}, + }); + + return { api, calls }; +} + +/* Minimal express response double. */ +function make_res() { + return { + code: null, + body: null, + status(code) { this.code = code; return this; }, + json(body) { this.body = body; return this; }, + }; +} + +function make_req(uuid) { + return { params: { uuid }, auth: "tester" }; +} + +describe("delete_dataset referential integrity", () => { + test("deletes a dataset with no referrers", async () => { + const { api, calls } = make_api({ + [SRC_A]: { + structure: Constants.App.SparkplugSrc, + config: { source: DEVICE }, + }, + }); + + const res = make_res(); + await api.delete_dataset(make_req(SRC_A), res); + + expect(res.code).toBe(200); + expect(res.body).toBe(SRC_A); + expect(calls.deleted).toEqual([SRC_A]); + }); + + test("refuses to delete a dataset listed by a union", async () => { + const { api, calls } = make_api({ + [SRC_A]: { + structure: Constants.App.SparkplugSrc, + config: { source: DEVICE }, + }, + [UNION]: { + structure: Constants.App.UnionComponents, + config: [SRC_A, SRC_B], + }, + }); + + const res = make_res(); + await api.delete_dataset(make_req(SRC_A), res); + + expect(res.code).toBe(409); + expect(res.body.error).toBe("dataset_in_use"); + expect(res.body.referrers).toEqual([ + { dataset: UNION, structure: Constants.App.UnionComponents }, + ]); + expect(calls.deleted).toEqual([]); + expect(calls.removed_subclasses).toEqual([]); + }); + + test("refuses to delete a dataset used as a session source", async () => { + const { api, calls } = make_api({ + [SRC_A]: { + structure: Constants.App.SparkplugSrc, + config: { source: DEVICE }, + }, + [SESSION]: { + structure: Constants.App.SessionLimits, + config: { + source: SRC_A, + from: "2025-01-01T00:00:00.000Z", + to: "2025-01-02T00:00:00.000Z", + }, + }, + }); + + const res = make_res(); + await api.delete_dataset(make_req(SRC_A), res); + + expect(res.code).toBe(409); + expect(res.body.referrers).toEqual([ + { dataset: SESSION, structure: Constants.App.SessionLimits }, + ]); + expect(calls.deleted).toEqual([]); + }); + + test("lists every referrer, not only the first", async () => { + const { api } = make_api({ + [SRC_A]: { + structure: Constants.App.SparkplugSrc, + config: { source: DEVICE }, + }, + [UNION]: { + structure: Constants.App.UnionComponents, + config: [SRC_A], + }, + [SESSION]: { + structure: Constants.App.SessionLimits, + config: { + source: SRC_A, + from: "2025-01-01T00:00:00.000Z", + to: "2025-01-02T00:00:00.000Z", + }, + }, + }); + + const res = make_res(); + await api.delete_dataset(make_req(SRC_A), res); + + expect(res.code).toBe(409); + expect(res.body.referrers.map(r => r.dataset).sort()) + .toEqual([SESSION, UNION].sort()); + }); + + test("deletes a union once nothing points at it", async () => { + const { api, calls } = make_api({ + [SRC_A]: { + structure: Constants.App.SparkplugSrc, + config: { source: DEVICE }, + }, + [UNION]: { + structure: Constants.App.UnionComponents, + config: [SRC_A], + }, + }); + + const res = make_res(); + await api.delete_dataset(make_req(UNION), res); + + expect(res.code).toBe(200); + expect(calls.deleted).toEqual([UNION]); + /* The union is the superclass of its components. */ + expect(calls.removed_subclasses).toEqual([[UNION, SRC_A]]); + }); + + test("drops the link a session owns on its source", async () => { + const { api, calls } = make_api({ + [SRC_A]: { + structure: Constants.App.SparkplugSrc, + config: { source: DEVICE }, + }, + [SESSION]: { + structure: Constants.App.SessionLimits, + config: { + source: SRC_A, + from: "2025-01-01T00:00:00.000Z", + to: "2025-01-02T00:00:00.000Z", + }, + }, + }); + + const res = make_res(); + await api.delete_dataset(make_req(SESSION), res); + + expect(res.code).toBe(200); + expect(calls.deleted).toEqual([SESSION]); + /* A session is a subclass of its source, so the link points the other + * way and the old direct-subclasses sweep never saw it. */ + expect(calls.removed_subclasses).toEqual([[SRC_A, SESSION]]); + }); + + test("finds referrers whose own dataset is invalid", async () => { + /* A dataset with two structural definitions is invalid, but it keeps + * its config documents and can still hold a dangling reference. */ + const { api, calls } = make_api( + { + [SRC_A]: { + structure: Constants.App.SparkplugSrc, + config: { source: DEVICE }, + }, + /* DataFlow reports the union as invalid with a null config. */ + [UNION]: { + structure: Constants.Special.InvalidDataset, + config: null, + }, + }, + { + /* ConfigDB still holds the list. */ + [Constants.App.UnionComponents]: { [UNION]: [SRC_A] }, + }, + ); + + const res = make_res(); + await api.delete_dataset(make_req(SRC_A), res); + + expect(res.code).toBe(409); + expect(res.body.referrers).toEqual([ + { dataset: UNION, structure: Constants.App.UnionComponents }, + ]); + expect(calls.deleted).toEqual([]); + }); + + test("a dataset does not count as its own referrer", async () => { + const { api, calls } = make_api({ + [UNION]: { + structure: Constants.App.UnionComponents, + config: [UNION], + }, + }); + + const res = make_res(); + await api.delete_dataset(make_req(UNION), res); + + expect(res.code).toBe(200); + expect(calls.deleted).toEqual([UNION]); + }); + + test("rejects an invalid uuid before touching ConfigDB", async () => { + const { api, calls } = make_api({}); + + await expect(api.delete_dataset(make_req("xxx"), make_res())) + .rejects.toMatchObject({ status: 422 }); + + expect(calls.deleted).toEqual([]); + }); +}); diff --git a/docs/services/data-access.md b/docs/services/data-access.md index ac5ee7874..d4cd6059b 100644 --- a/docs/services/data-access.md +++ b/docs/services/data-access.md @@ -203,14 +203,33 @@ the structural app the way `POST` does. ### `GET v1/delete/:uuid` Deletes a dataset. Note this is a `GET`, not a `DELETE`, request. -Requires `Delete dataset` on `:uuid`. Before deleting the ConfigDB -object, it removes every direct subclass relationship in which this -dataset is the *superclass* (e.g. session/union children recorded -against it). It does **not** clean up relationships in which this -dataset is itself a subclass — e.g. a session's link to its source, or -a union's link to its members — so other structural configs can be left -referencing a UUID that no longer exists. Returns the deleted UUID as a -JSON string, or an empty string if the dataset didn't exist. +Requires `Delete dataset` on `:uuid`. + +The delete is refused with `409 Conflict` while any other dataset still +points at this one, that is while a `Union components` list contains it +or a `Session limits` config names it as its `source`. The response body +lists the referrers: + +```json +{ + "error": "dataset_in_use", + "dataset": "", + "message": "...", + "referrers": [ { "dataset": "", "structure": "" } ] +} +``` + +Nothing is written when the delete is refused. Delete from the top of +the graph down: remove the union or session first, then its components. +Invalid datasets are checked too, because they keep their config +documents. + +When there are no referrers, the service removes the subclass +relationships this dataset owns in both directions. That covers the +links where the dataset is the superclass (a union's members) and the +links where it is the subclass (a session's link to its source). It then +deletes the ConfigDB object. Returns the deleted UUID as a JSON string, +or an empty string if the dataset didn't exist. ## Permissions