Skip to content

Commit bf0ebff

Browse files
authored
Merge pull request #1208 from dahlia/bugfix/vocab/clones-share-property-arrays
Isolate property arrays in vocabulary clones
2 parents fd218a8 + cea807f commit bf0ebff

8 files changed

Lines changed: 1704 additions & 1258 deletions

File tree

‎CHANGES.md‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,23 @@ Version 2.0.31
88

99
To be released.
1010

11+
### @fedify/vocab
12+
13+
- Fixed shared property arrays in vocabulary objects so dereferencing a
14+
clone no longer changes its source's properties or serialization.
15+
Constructors and `clone()` also copy supplied plural-value arrays,
16+
allowing frozen arrays and preventing changes to the caller's arrays.
17+
Nested objects and URLs retain their identity. [[#1207], [#1208]]
18+
19+
[#1207]: https://github.com/fedify-dev/fedify/issues/1207
20+
[#1208]: https://github.com/fedify-dev/fedify/pull/1208
21+
22+
### @fedify/vocab-tools
23+
24+
- Fixed generated constructors and `clone()` methods to copy property
25+
arrays, preventing a clone's remote lookups from changing its source and
26+
preserving arrays supplied by callers. [[#1207], [#1208]]
27+
1128

1229
Version 2.0.30
1330
--------------
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
---
2+
links:
3+
'#1207': https://github.com/fedify-dev/fedify/issues/1207
4+
'#1208': https://github.com/fedify-dev/fedify/pull/1208
5+
---
6+
- Fixed generated constructors and `clone()` methods to copy property
7+
arrays, preventing a clone's remote lookups from changing its source and
8+
preserving arrays supplied by callers. [[#1207], [#1208]]
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
links:
3+
'#1207': https://github.com/fedify-dev/fedify/issues/1207
4+
'#1208': https://github.com/fedify-dev/fedify/pull/1208
5+
---
6+
- Fixed shared property arrays in vocabulary objects so dereferencing a
7+
clone no longer changes its source's properties or serialization.
8+
Constructors and `clone()` also copy supplied plural-value arrays,
9+
allowing frozen arrays and preventing changes to the caller's arrays.
10+
Nested objects and URLs retain their identity. [[#1207], [#1208]]

‎packages/vocab-tools/src/__snapshots__/class.test.ts.deno.snap‎

Lines changed: 484 additions & 418 deletions
Large diffs are not rendered by default.

‎packages/vocab-tools/src/__snapshots__/class.test.ts.node.snap‎

Lines changed: 484 additions & 418 deletions
Large diffs are not rendered by default.

‎packages/vocab-tools/src/__snapshots__/class.test.ts.snap‎

Lines changed: 484 additions & 418 deletions
Large diffs are not rendered by default.

‎packages/vocab-tools/src/constructor.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,7 @@ export async function* generateConstructor(
175175
if (Array.isArray(values.${property.pluralName}) &&
176176
values.${property.pluralName}.every(v => ${typeGuards})) {
177177
// @ts-ignore: type is checked above.
178-
this.${fieldName} = values.${property.pluralName};
178+
this.${fieldName} = values.${property.pluralName}.slice();
179179
`;
180180
if (!allScalarTypes) {
181181
yield `
@@ -208,7 +208,8 @@ export async function* generateCloner(
208208
* Clones this instance, optionally updating it with the given values.
209209
* @param values The values to update the clone with.
210210
* @param options The options to use for cloning.
211-
* @returns The cloned instance.
211+
* @returns The cloned instance with its own property arrays. Nested objects
212+
* and URLs are shared with this instance.
212213
*/
213214
${emitOverride(typeUri, types)} clone(
214215
values:
@@ -245,7 +246,7 @@ export async function* generateCloner(
245246
const fieldName = await getFieldName(property.uri);
246247
const trustFieldName = await getFieldName(property.uri, "#_trust");
247248
const allScalarTypes = areAllScalarTypes(property.range, types);
248-
yield `clone.${fieldName} = this.${fieldName};`;
249+
yield `clone.${fieldName} = this.${fieldName}.slice();`;
249250
if (!allScalarTypes) {
250251
yield `clone.${trustFieldName} = new Set(this.${trustFieldName});`;
251252
}
@@ -309,7 +310,7 @@ export async function* generateCloner(
309310
if (Array.isArray(values.${property.pluralName}) &&
310311
values.${property.pluralName}.every(v => ${typeGuards})) {
311312
// @ts-ignore: type is checked above.
312-
clone.${fieldName} = values.${property.pluralName};
313+
clone.${fieldName} = values.${property.pluralName}.slice();
313314
`;
314315
if (!allScalarTypes) {
315316
yield `

‎packages/vocab/src/vocab.test.ts‎

Lines changed: 212 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,13 @@ import {
1212
notDeepStrictEqual,
1313
ok,
1414
rejects,
15+
strictEqual,
1516
throws,
1617
} from "node:assert/strict";
1718
import { assertInstanceOf } from "./utils.ts";
1819
import * as vocab from "./vocab.ts";
1920
import {
21+
Accept,
2022
Activity,
2123
Announce,
2224
Collection,
@@ -1279,6 +1281,216 @@ test("FEP-fe34: Trust tracking in object cloning", () => {
12791281
);
12801282
});
12811283

1284+
for (const property of ["object", "attachment"] as const) {
1285+
for (const fetchOriginal of [false, true]) {
1286+
test(
1287+
`clone() isolates ${property} dereferencing (${
1288+
fetchOriginal ? "original" : "clone"
1289+
} first)`,
1290+
async () => {
1291+
const embedded = {
1292+
id: "https://b.example/notes/1",
1293+
type: "Note",
1294+
content: "embedded",
1295+
};
1296+
const json = {
1297+
"@context": "https://www.w3.org/ns/activitystreams",
1298+
id: "https://a.example/activities/1",
1299+
type: "Create",
1300+
[property]: embedded,
1301+
};
1302+
const original = await Create.fromJsonLd(json, {
1303+
contextLoader: mockDocumentLoader,
1304+
});
1305+
const copy = original.clone();
1306+
const fetching = fetchOriginal ? original : copy;
1307+
const untouched = fetchOriginal ? copy : original;
1308+
let fetches = 0;
1309+
// deno-lint-ignore require-await
1310+
const documentLoader = async (url: string) => {
1311+
fetches++;
1312+
return {
1313+
contextUrl: null,
1314+
documentUrl: url,
1315+
document: {
1316+
"@context": "https://www.w3.org/ns/activitystreams",
1317+
...embedded,
1318+
content: "fetched",
1319+
},
1320+
};
1321+
};
1322+
const options = { documentLoader, contextLoader: mockDocumentLoader };
1323+
const fetched = property === "object"
1324+
? await fetching.getObject(options)
1325+
: (await Array.fromAsync(fetching.getAttachments(options)))[0];
1326+
assertInstanceOf(fetched, Note);
1327+
deepStrictEqual(fetched.content, "fetched");
1328+
1329+
// Inspect the other instance before it fetches anything itself.
1330+
const trusted = { crossOrigin: "trust" as const };
1331+
const preserved = property === "object"
1332+
? await untouched.getObject(trusted)
1333+
: (await Array.fromAsync(untouched.getAttachments(trusted)))[0];
1334+
assertInstanceOf(preserved, Note);
1335+
deepStrictEqual(preserved.content, "embedded");
1336+
const serialized = await original.toJsonLd() as Record<string, unknown>;
1337+
deepStrictEqual(
1338+
serialized[property],
1339+
fetchOriginal ? { ...embedded, content: "fetched" } : embedded,
1340+
);
1341+
const accept = new Accept({ object: untouched });
1342+
const acceptJson = await accept.toJsonLd() as {
1343+
object: Record<string, unknown>;
1344+
};
1345+
const nested = acceptJson.object;
1346+
deepStrictEqual(nested[property], embedded);
1347+
1348+
// Successful lookups and their trust metadata belong to each instance.
1349+
const cached = property === "object"
1350+
? await fetching.getObject(options)
1351+
: (await Array.fromAsync(fetching.getAttachments(options)))[0];
1352+
strictEqual(cached, fetched);
1353+
deepStrictEqual(fetches, 1);
1354+
const independentlyFetched = property === "object"
1355+
? await untouched.getObject(options)
1356+
: (await Array.fromAsync(untouched.getAttachments(options)))[0];
1357+
assertInstanceOf(independentlyFetched, Note);
1358+
deepStrictEqual(independentlyFetched.content, "fetched");
1359+
deepStrictEqual(fetches, 2);
1360+
const warmedClone = fetching.clone();
1361+
strictEqual(
1362+
property === "object"
1363+
? await warmedClone.getObject(options)
1364+
: (await Array.fromAsync(warmedClone.getAttachments(options)))[0],
1365+
fetched,
1366+
);
1367+
deepStrictEqual(fetches, 2);
1368+
},
1369+
);
1370+
}
1371+
}
1372+
1373+
for (const clone of [false, true]) {
1374+
for (const frozen of [false, true]) {
1375+
test(
1376+
`${clone ? "clone()" : "constructor"} copies ${
1377+
frozen ? "frozen" : "mutable"
1378+
} plural arrays`,
1379+
async () => {
1380+
const items = [
1381+
new URL("https://example.com/object"),
1382+
new URL("https://example.com/object"),
1383+
];
1384+
const before = items.slice();
1385+
if (frozen) globalThis.Object.freeze(items);
1386+
const base = new Collection({});
1387+
const collection = clone
1388+
? base.clone({ items })
1389+
: new Collection({ items });
1390+
const sibling = clone
1391+
? base.clone({ items })
1392+
: new Collection({ items });
1393+
const fetched = await Array.fromAsync(collection.getItems({
1394+
documentLoader: mockDocumentLoader,
1395+
contextLoader: mockDocumentLoader,
1396+
}));
1397+
deepStrictEqual(fetched.length, 2);
1398+
deepStrictEqual(fetched.map((item) => item.name), [
1399+
"Fetched object",
1400+
"Fetched object",
1401+
]);
1402+
deepStrictEqual(items, before);
1403+
strictEqual(items[0], before[0]);
1404+
strictEqual(items[1], before[1]);
1405+
const siblingJson = await sibling.toJsonLd() as { items: unknown };
1406+
deepStrictEqual(siblingJson.items, before.map(String));
1407+
if (!frozen) {
1408+
items.push(new URL("https://example.com/extra"));
1409+
deepStrictEqual(collection.itemIds, before);
1410+
deepStrictEqual(sibling.itemIds, before);
1411+
}
1412+
},
1413+
);
1414+
}
1415+
}
1416+
1417+
test("clone() copies scalar arrays without changing sparse inputs", () => {
1418+
const names = ["original"];
1419+
const original = new Object({ names });
1420+
names.push("caller mutation");
1421+
deepStrictEqual(original.names, ["original"]);
1422+
const copy = original.clone();
1423+
copy.names.push("clone mutation");
1424+
deepStrictEqual(original.names, ["original"]);
1425+
deepStrictEqual(copy.names, ["original", "clone mutation"]);
1426+
const replacements = ["replacement"];
1427+
const replaced = original.clone({ names: replacements });
1428+
replacements.push("caller mutation");
1429+
deepStrictEqual(replaced.names, ["replacement"]);
1430+
1431+
const sparseNames = new Array<string>(2);
1432+
sparseNames[1] = "kept";
1433+
const sparse = new Object({ names: sparseNames });
1434+
for (
1435+
const instance of [
1436+
sparse,
1437+
sparse.clone(),
1438+
sparse.clone({ names: sparse.names }),
1439+
]
1440+
) {
1441+
deepStrictEqual(instance.names.length, 2);
1442+
deepStrictEqual(0 in instance.names, false);
1443+
deepStrictEqual(instance.names[1], "kept");
1444+
}
1445+
});
1446+
1447+
test("clone() preserves embedded object identity and trust", async () => {
1448+
const note = new Note({ id: new URL("https://b.example/note") });
1449+
const original = new Create({
1450+
id: new URL("https://a.example/create"),
1451+
object: note,
1452+
});
1453+
// deno-lint-ignore require-await
1454+
const documentLoader = async () => {
1455+
throw new Error("Trusted embedded objects must not be fetched.");
1456+
};
1457+
for (
1458+
const instance of [
1459+
original,
1460+
original.clone(),
1461+
original.clone({ objects: [note] }),
1462+
]
1463+
) {
1464+
strictEqual(await instance.getObject({ documentLoader }), note);
1465+
strictEqual(instance.objectId, note.id);
1466+
}
1467+
});
1468+
1469+
test("clone() isolates crossOrigin trust when fetching", async () => {
1470+
const original = new Create({
1471+
id: new URL("https://a.example/create"),
1472+
object: new URL("https://b.example/note"),
1473+
});
1474+
const copy = original.clone();
1475+
// deno-lint-ignore require-await
1476+
const documentLoader = async (url: string) => ({
1477+
contextUrl: null,
1478+
documentUrl: url,
1479+
document: {
1480+
"@context": "https://www.w3.org/ns/activitystreams",
1481+
id: "https://other.example/note",
1482+
type: "Note",
1483+
content: "cross-origin",
1484+
},
1485+
});
1486+
const options = { documentLoader, contextLoader: mockDocumentLoader };
1487+
const trusted = await copy.getObject({ ...options, crossOrigin: "trust" });
1488+
assertInstanceOf(trusted, Note);
1489+
deepStrictEqual(trusted.content, "cross-origin");
1490+
deepStrictEqual(await original.getObject(options), null);
1491+
deepStrictEqual(original.objectId, new URL("https://b.example/note"));
1492+
});
1493+
12821494
test("FEP-fe34: crossOrigin ignore behavior (default)", async () => {
12831495
// Create a mock document loader that returns objects with different origins
12841496
// deno-lint-ignore require-await

0 commit comments

Comments
 (0)