Skip to content

Commit d66f2c5

Browse files
pubkeyclaude
andauthored
Fix localstorage remove() not deleting attachment data (#8343)
* FIX localstorage storage remove() leaking attachment data The remove() method deleted documents and indexes from localStorage but did not delete attachment data entries, causing orphaned attachment keys to remain after the storage instance was removed. Fix: when iterating documents for deletion, also read each document's _attachments metadata and remove all corresponding attachment entries from localStorage. Also: - Add count() contract test verifying count() == query().length - Add localstorage-specific tests (cleanup, persistence, index updates) - Update RxStorageInstance.count() interface doc to specify that count() must return the same as query().documents.length https://claude.ai/code/session_01LFMtmsH1mvSJzbWmptDemc * Trim localstorage test file to only essential tests Drop redundant localstorage-specific tests that duplicate coverage already provided by the shared rx-storage-implementations and rx-storage-query-correctness suites (cleanup, persistence, index updates after mutation). Keep only the two tests that cannot be expressed as unified tests: - remove() must delete all localStorage keys including attachments (the bug being fixed in this branch) - count() must return the same amount as query().documents.length with a limit set (the generic query-correctness suite skips count checks with limit/skip because other storages implement count() with a different contract) https://claude.ai/code/session_01LFMtmsH1mvSJzbWmptDemc --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 9151ea3 commit d66f2c5

5 files changed

Lines changed: 180 additions & 2 deletions

File tree

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
- FIX localstorage storage `remove()` not deleting attachment data from localStorage, causing orphaned attachment entries to remain after the storage instance is removed

src/plugins/storage-localstorage/rx-storage-instance-localstorage.ts

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -473,6 +473,10 @@ export class RxStorageInstanceLocalstorage<RxDocType> implements RxStorageInstan
473473
});
474474
}
475475

476+
/**
477+
* Returns the same count as query().documents.length
478+
* to fulfill the RxStorageInstance interface contract.
479+
*/
476480
async count(
477481
preparedQuery: PreparedQuery<RxDocType>
478482
): Promise<RxStorageCountResult> {
@@ -568,12 +572,20 @@ export class RxStorageInstanceLocalstorage<RxDocType> implements RxStorageInstan
568572
this.changeStreamSub.unsubscribe();
569573
this.localStorage.removeItem(this.changestreamStorageKey);
570574

571-
// delete documents
575+
// delete documents and their attachments
572576
const firstIndex = Object.values(this.internals.indexes)[0];
573577
const indexedDocs = this.getIndex(firstIndex.index);
574578
indexedDocs.forEach(row => {
575579
const docId = row[1];
580+
const doc = this.getDoc(docId);
576581
this.localStorage.removeItem(this.docsKey + '-' + docId);
582+
if (doc && doc._attachments) {
583+
Object.keys(doc._attachments).forEach(attachmentId => {
584+
this.localStorage.removeItem(
585+
this.attachmentsKey + '-' + docId + '||' + attachmentId
586+
);
587+
});
588+
}
577589
});
578590

579591
// delete indexes

src/types/rx-storage.interface.d.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,10 @@ export interface RxStorageInstance<
212212
/**
213213
* Returns the amount of non-deleted documents
214214
* that match the given query.
215-
* Sort, skip and limit of the query must be ignored!
215+
* Must return the same amount as if the equivalent query()
216+
* was run and the result array length measured.
217+
* For example with .count({limit: 3}) must return the same
218+
* as .query({limit: 3}).documents.length .
216219
*/
217220
count(
218221
preparedQuery: PreparedQuery<RxDocType>

test/unit.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import './unit/rx-storage-query-correctness.test.ts';
2525
import './unit/rx-storage-helper.test.ts';
2626

2727
import './unit/rx-storage-dexie.test.ts';
28+
import './unit/rx-storage-localstorage.test.ts';
2829
import './unit/rx-storage-remote.test.ts';
2930
import './unit/instance-of-check.test.ts';
3031
import './unit/rx-schema.test.ts';
Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
import assert from 'assert';
2+
import config from './config.ts';
3+
import {
4+
createRxDatabase,
5+
randomToken,
6+
addRxPlugin,
7+
prepareQuery,
8+
normalizeMangoQuery,
9+
fillWithDefaultSettings,
10+
clone,
11+
RxJsonSchema
12+
} from '../../plugins/core/index.mjs';
13+
import {
14+
getRxStorageLocalstorage,
15+
getLocalStorageMock
16+
} from '../../plugins/storage-localstorage/index.mjs';
17+
import { wrappedValidateAjvStorage } from '../../plugins/validate-ajv/index.mjs';
18+
import { RxDBAttachmentsPlugin } from '../../plugins/attachments/index.mjs';
19+
20+
addRxPlugin(RxDBAttachmentsPlugin);
21+
22+
/**
23+
* Tests specific to the localstorage storage where the behavior
24+
* cannot be observed through the generic rx-storage-implementations
25+
* test suite.
26+
*/
27+
describe('rx-storage-localstorage.test.ts', () => {
28+
if (config.storage.name !== 'localstorage') {
29+
return;
30+
}
31+
it('remove() must delete all localStorage keys for the database, including attachments', async () => {
32+
const mock = getLocalStorageMock();
33+
const storage = wrappedValidateAjvStorage({
34+
storage: getRxStorageLocalstorage({ localStorage: mock })
35+
});
36+
const dbName = randomToken(10);
37+
const db = await createRxDatabase({
38+
name: dbName,
39+
storage,
40+
eventReduce: false
41+
});
42+
43+
const cols = await db.addCollections({
44+
docs: {
45+
schema: {
46+
version: 0,
47+
primaryKey: 'id',
48+
type: 'object',
49+
properties: {
50+
id: { type: 'string', maxLength: 100 },
51+
name: { type: 'string', maxLength: 100 }
52+
},
53+
required: ['id', 'name'],
54+
attachments: {}
55+
}
56+
}
57+
});
58+
59+
const doc = await cols.docs.insert({ id: 'att-test', name: 'Test' });
60+
await doc.putAttachment({
61+
id: 'myfile.txt',
62+
data: new Blob(['hello attachment'], { type: 'text/plain' }),
63+
type: 'text/plain'
64+
});
65+
66+
function getDbKeys(): string[] {
67+
const keys: string[] = [];
68+
for (let i = 0; i < mock.length; i++) {
69+
const key = mock.key(i);
70+
if (key && key.includes(dbName)) {
71+
keys.push(key);
72+
}
73+
}
74+
return keys;
75+
}
76+
77+
const keysBefore = getDbKeys();
78+
assert.ok(
79+
keysBefore.some(k => k.includes('attachment')),
80+
'should have attachment keys before remove'
81+
);
82+
83+
await db.remove();
84+
85+
const keysAfter = getDbKeys();
86+
assert.strictEqual(
87+
keysAfter.length,
88+
0,
89+
'remove() must clean up all keys, but found leaked: ' + keysAfter.join(', ')
90+
);
91+
});
92+
93+
/**
94+
* count() must respect skip and limit and return the
95+
* same amount as query().documents.length .
96+
* The generic query-correctness suite skips count checks
97+
* when limit/skip is present because some storages implement
98+
* count() with a different contract, so this regression test
99+
* lives here.
100+
*/
101+
it('count() must return the same amount as query().documents.length (with limit)', async () => {
102+
const dbName = randomToken(10);
103+
const db = await createRxDatabase({
104+
name: dbName,
105+
storage: config.storage.getStorage(),
106+
eventReduce: false
107+
});
108+
const rawSchema: RxJsonSchema<{ id: string; age: number; }> = {
109+
version: 0,
110+
primaryKey: 'id',
111+
type: 'object',
112+
properties: {
113+
id: { type: 'string', maxLength: 100 },
114+
age: {
115+
type: 'integer',
116+
minimum: 0,
117+
maximum: 150,
118+
multipleOf: 1
119+
}
120+
},
121+
required: ['id', 'age'],
122+
indexes: ['age']
123+
};
124+
const cols = await db.addCollections({ humans: { schema: rawSchema } });
125+
const col = cols.humans;
126+
127+
await col.bulkInsert(
128+
new Array(10).fill(0).map((_, i) => ({ id: 'doc-' + i, age: 20 + i }))
129+
);
130+
131+
const schema = fillWithDefaultSettings(clone(rawSchema));
132+
const preparedQuery = prepareQuery(
133+
schema,
134+
normalizeMangoQuery(schema, {
135+
selector: {
136+
_deleted: { $eq: false },
137+
age: { $gte: 20 }
138+
} as any,
139+
sort: [
140+
{ _deleted: 'asc' as const },
141+
{ age: 'asc' as const },
142+
{ id: 'asc' as const }
143+
],
144+
skip: 0,
145+
limit: 3
146+
})
147+
);
148+
149+
const queryResult = await col.storageInstance.query(preparedQuery);
150+
const countResult = await col.storageInstance.count(preparedQuery);
151+
152+
assert.strictEqual(
153+
countResult.count,
154+
queryResult.documents.length,
155+
'count() must equal query().documents.length'
156+
);
157+
assert.strictEqual(countResult.count, 3, 'count should be limited to 3');
158+
159+
await db.close();
160+
});
161+
});

0 commit comments

Comments
 (0)