From 27b2272cedbe217b86a253a216230b57ded3cd5d Mon Sep 17 00:00:00 2001 From: david raistrick <1108844+keen99@users.noreply.github.com> Date: Sat, 4 Jul 2026 16:06:28 -0400 Subject: [PATCH] fix(storage): neutral queryConstraints honored by both adapters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/storage/contract.js | 39 ++++++++++++++++++++++++ src/storage/firebase.js | 14 +++++++-- src/storage/index.js | 12 ++++++-- src/storage/server.js | 37 +++++++++++++++++++---- src/tests/firebase.contract.test.js | 46 ++++------------------------- 5 files changed, 98 insertions(+), 50 deletions(-) diff --git a/src/storage/contract.js b/src/storage/contract.js index c1fe705..4394448 100644 --- a/src/storage/contract.js +++ b/src/storage/contract.js @@ -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); + }); + }); }); } diff --git a/src/storage/firebase.js b/src/storage/firebase.js index 385d9ce..a6c1a34 100644 --- a/src/storage/firebase.js +++ b/src/storage/firebase.js @@ -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, }; diff --git a/src/storage/index.js b/src/storage/index.js index f8ab9a0..145e810 100644 --- a/src/storage/index.js +++ b/src/storage/index.js @@ -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, }; diff --git a/src/storage/server.js b/src/storage/server.js index 4500a55..5cbae3d 100644 --- a/src/storage/server.js +++ b/src/storage/server.js @@ -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 const collSubs = new Map(); // collPath -> Set + 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(); }, diff --git a/src/tests/firebase.contract.test.js b/src/tests/firebase.contract.test.js index 10e329c..b542a01 100644 --- a/src/tests/firebase.contract.test.js +++ b/src/tests/firebase.contract.test.js @@ -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']); - }); -});