fix(storage): neutral queryConstraints honored by both adapters

Bug: server adapter accepted queryConstraints (orderBy/limit) but ignored them.
Combat log [orderBy('timestamp','desc'), limit(500)] returned ALL logs unordered
in server mode — unbounded growth. No test caught it: shared contract tested
subscribeCollection with 0 constraints; the firebase-only queryConstraint test
never touched the server adapter. Parity gap.

Fix (Plan A — neutral shape):
- index.js: orderBy()/limit() now NEUTRAL builders returning {__type}. Removed
  SDK orderBy/limit re-export.
- firebase.js: subscribeCollection translates neutral -> SDK query constraints.
- server.js: applyConstraints() helper sorts/limits client-side (backend returns
  all). Constraints stored per collection, applied on initial fetch + WS change.
- contract.js: added 3 queryConstraint tests (orderBy desc, limit, no-constraints)
  to SHARED contract — both adapters now run identical assertions.
- firebase.contract.test.js: removed now-redundant firebase-only constraint
  block (covered by shared contract).

Data impact: ZERO. Constraints are query-time only, never stored. Combat log
docs keep timestamp field. No Firebase data migration needed.

208 tests green.
This commit is contained in:
david raistrick
2026-07-04 16:06:28 -04:00
parent e94e1959ff
commit 27b2272ced
5 changed files with 98 additions and 50 deletions
+39
View File
@@ -232,6 +232,45 @@ function runStorageContract(name, factory) {
expect(last).toHaveLength(1);
});
});
// queryConstraints: orderBy + limit. App's combat log uses
// [orderBy('timestamp','desc'), limit(500)] — newest 500 entries.
// Adapter MUST honor these. Constraint shape = neutral {__type} objects
// (what firebase mock produces; what App passes via shared builders).
describe('subscribeCollection queryConstraints', () => {
const orderByC = (field, dir) => ({ __type: 'orderBy', field, dir });
const limitC = (n) => ({ __type: 'limit', n });
beforeEach(async () => {
// seed 5 log docs, timestamps out of order
await storage.setDoc('logs/l1', { msg: 'one', timestamp: 1 });
await storage.setDoc('logs/l2', { msg: 'two', timestamp: 3 });
await storage.setDoc('logs/l3', { msg: 'three', timestamp: 5 });
await storage.setDoc('logs/l4', { msg: 'four', timestamp: 2 });
await storage.setDoc('logs/l5', { msg: 'five', timestamp: 4 });
});
test('orderBy desc sorts newest-first', async () => {
const result = await new Promise((resolve) => {
storage.subscribeCollection('logs', resolve, [orderByC('timestamp', 'desc')]);
});
expect(result.map(d => d.msg)).toEqual(['three', 'five', 'two', 'four', 'one']);
});
test('limit returns only first N after ordering', async () => {
const result = await new Promise((resolve) => {
storage.subscribeCollection('logs', resolve, [orderByC('timestamp', 'desc'), limitC(2)]);
});
expect(result.map(d => d.msg)).toEqual(['three', 'five']);
});
test('no constraints returns all (insertion/id order)', async () => {
const result = await new Promise((resolve) => {
storage.subscribeCollection('logs', resolve, []);
});
expect(result).toHaveLength(5);
});
});
});
}
+11 -3
View File
@@ -130,8 +130,15 @@ export function createFirebaseStorage() {
subscribeCollection(collectionPath, cb, queryConstraints = [], errCb) {
recordAdapterCall({ fn: 'subscribeCollection', path: collectionPath });
const q = queryConstraints.length > 0
? query(collection(db, collectionPath), ...queryConstraints)
// queryConstraints = neutral {__type} builders (from index.js).
// Translate to SDK orderBy/limit.
const sdkConstraints = queryConstraints.map(c => {
if (c.__type === 'orderBy') return orderBy(c.field, c.dir);
if (c.__type === 'limit') return limit(c.n);
return c; // pass-through (forward compat)
});
const q = sdkConstraints.length > 0
? query(collection(db, collectionPath), ...sdkConstraints)
: collection(db, collectionPath);
return onSnapshot(q, (snap) => {
cb(snap.docs.map(d => ({ id: d.id, ...d.data() })));
@@ -146,7 +153,8 @@ export function createFirebaseStorage() {
}
// Re-export SDK pieces App.js uses directly (until full refactor).
// orderBy/limit NOT re-exported: App uses neutral builders from index.js.
export {
doc, setDoc, updateDoc, deleteDoc, addDoc, collection, onSnapshot,
query, orderBy, limit, writeBatch,
query, writeBatch,
};
+10 -2
View File
@@ -1,5 +1,6 @@
// src/storage/index.js — storage factory + SDK re-exports.
// STORAGE=firebase (default): adapter wraps SDK. STORAGE=server: backend.
// orderBy/limit below = NEUTRAL builders (not SDK passthrough).
// App.js imports getStorage() for subscribe; still imports SDK pieces for writes (per-group refactor pending).
import { initializeApp } from 'firebase/app';
@@ -8,7 +9,7 @@ import {
} from 'firebase/auth';
import {
getFirestore, doc, setDoc, getDoc, getDocs, addDoc, collection,
onSnapshot, updateDoc, deleteDoc, query, orderBy, limit, writeBatch,
onSnapshot, updateDoc, deleteDoc, query, writeBatch,
} from 'firebase/firestore';
import { initFirebase, createFirebaseStorage } from './firebase';
import { createServerStorage } from './server';
@@ -38,9 +39,16 @@ export function getStorageMode() {
return process.env.REACT_APP_STORAGE || 'firebase';
}
// Neutral query-constraint builders. App builds constraints with these (not
// raw SDK), so both adapters read the same shape. firebase adapter translates
// neutral -> SDK; server adapter applies client-side. Combat log uses these:
// [orderBy('timestamp','desc'), limit(500)]
export function orderBy(field, dir) { return { __type: 'orderBy', field, dir }; }
export function limit(n) { return { __type: 'limit', n }; }
export {
initializeApp,
getAuth, signInAnonymously, onAuthStateChanged, signInWithCustomToken,
getFirestore, doc, setDoc, getDoc, getDocs, addDoc, collection,
onSnapshot, updateDoc, deleteDoc, query, orderBy, limit, writeBatch,
onSnapshot, updateDoc, deleteDoc, query, writeBatch,
};
+32 -5
View File
@@ -42,8 +42,30 @@ function createServerStorage({ baseUrl, realtimeUrl } = {}) {
return { id, ...data };
}
// Apply neutral {__type} query constraints client-side. Backend returns all
// docs; adapter sorts/limits. Mirrors firebase mock applyConstraints.
function applyConstraints(docs, constraints) {
let out = [...docs];
for (const c of constraints || []) {
if (!c || !c.__type) continue;
if (c.__type === 'orderBy') {
out.sort((a, b) => {
const av = a[c.field];
const bv = b[c.field];
if (av === bv) return 0;
const cmp = av > bv ? 1 : -1;
return c.dir === 'desc' ? -cmp : cmp;
});
} else if (c.__type === 'limit') {
out = out.slice(0, c.n);
}
}
return out;
}
const docSubs = new Map(); // path -> Set<cb>
const collSubs = new Map(); // collPath -> Set<cb>
const collConstraints = new Map(); // collPath -> constraints[] (per collection)
let ws = null;
let wsReady = null;
@@ -130,11 +152,12 @@ function createServerStorage({ baseUrl, realtimeUrl } = {}) {
const doc = await storage.getDoc(c.path);
docCbs.forEach(cb => cb(doc));
}
// collection subscribers at parent path (doc belongs to this collection)
// collection subscribers at parent path (apply their stored constraints)
if (c.parent) {
const collCbs = collSubs.get(c.parent);
if (collCbs) {
const docs = await storage.getCollection(c.parent);
const constraints = collConstraints.get(c.parent) || [];
const docs = applyConstraints(await storage.getCollection(c.parent), constraints);
collCbs.forEach(cb => cb(docs));
}
}
@@ -228,8 +251,12 @@ function createServerStorage({ baseUrl, realtimeUrl } = {}) {
subscribeCollection(rawCollPath, cb, queryConstraints = [], errCb) {
const p = norm(rawCollPath);
// Initial value via REST (independent of WS connect).
storage.getCollection(p).then(cb).catch(() => {});
collConstraints.set(p, queryConstraints || []);
// Initial value via REST (independent of WS connect). Apply constraints
// client-side — backend returns all docs, adapter sorts/limits.
storage.getCollection(p)
.then(docs => cb(applyConstraints(docs, queryConstraints || [])))
.catch(() => {});
// WS only for subsequent change notifications.
ensureWs().then(() => {
ws.send(JSON.stringify({ type: 'subscribe', kind: 'collection', path: p }));
@@ -243,7 +270,7 @@ function createServerStorage({ baseUrl, realtimeUrl } = {}) {
disposed = true;
if (reconnectTimer) { clearTimeout(reconnectTimer); reconnectTimer = null; }
if (ws) ws.close();
docSubs.clear(); collSubs.clear();
docSubs.clear(); collSubs.clear(); collConstraints.clear();
if (typeof cb === 'function') cb();
},
+6 -40
View File
@@ -1,13 +1,15 @@
// Firebase adapter contract test.
// Runs the SAME storage contract as ws (src/storage/contract.js)
// Runs the SAME storage contract as server (src/storage/contract.js)
// against createFirebaseStorage. The SDK is mocked via src/__mocks__/firebase/*,
// so this proves the adapter translates contract calls -> SDK calls correctly
// (path handling, merge semantics, subscribe) without hitting network.
// (path handling, merge semantics, subscribe, queryConstraints) without network.
//
// queryConstraints (orderBy + limit) now tested in shared contract for BOTH
// adapters — no separate firebase-only block needed here.
import { initFirebase, createFirebaseStorage } from '../storage/firebase';
import { runStorageContract } from '../storage/contract';
import { resetMockDb, MOCK_DB } from '../__mocks__/firebase/_mock-db';
import { orderBy, limit } from '../__mocks__/firebase/firestore';
import { resetMockDb } from '../__mocks__/firebase/_mock-db';
// Firebase mock DB is shared global state. Reset before each factory so
// contract tests are isolated.
@@ -17,39 +19,3 @@ runStorageContract('firebase', () => {
if (!ok) throw new Error('initFirebase failed under mocked env');
return createFirebaseStorage();
});
// Firebase-only: queryConstraints (orderBy + limit) must flow through
// subscribeCollection. main App passes LOG_QUERY = [orderBy('timestamp','desc'),
// limit(500)] for the combat log. Adapter forwards them to the SDK; this proves
// the whole chain sorts + caps, not just dumps raw.
describe('firebase adapter: queryConstraints', () => {
let storage;
beforeEach(() => {
resetMockDb();
initFirebase();
storage = createFirebaseStorage();
});
test('subscribeCollection honors orderBy desc', async () => {
MOCK_DB.set('logs/a', { message: 'A', timestamp: 100 });
MOCK_DB.set('logs/b', { message: 'B', timestamp: 300 });
MOCK_DB.set('logs/c', { message: 'C', timestamp: 200 });
const result = await new Promise((resolve) => {
storage.subscribeCollection('logs', resolve, [orderBy('timestamp', 'desc')]);
});
expect(result.map(d => d.message)).toEqual(['B', 'C', 'A']);
});
test('subscribeCollection honors limit', async () => {
for (let i = 0; i < 5; i++) {
MOCK_DB.set(`logs/l${i}`, { message: `L${i}`, timestamp: i });
}
const result = await new Promise((resolve) => {
storage.subscribeCollection('logs', resolve, [orderBy('timestamp', 'desc'), limit(3)]);
});
expect(result).toHaveLength(3);
expect(result.map(d => d.message)).toEqual(['L4', 'L3', 'L2']);
});
});