From bb5f1393558a56f448c3eb2ed5f0397468e74f20 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Sun, 2 Jun 2019 19:27:23 +0200 Subject: [PATCH] fix fragmentId:0 being evaluated as falsy --- .../room/timeline/FragmentIdComparer.js | 52 ++++++++++++++++--- src/matrix/room/timeline/common.js | 3 ++ .../timeline/entries/FragmentBoundaryEntry.js | 5 ++ .../timeline/persistence/TimelineReader.js | 3 +- 4 files changed, 54 insertions(+), 9 deletions(-) create mode 100644 src/matrix/room/timeline/common.js diff --git a/src/matrix/room/timeline/FragmentIdComparer.js b/src/matrix/room/timeline/FragmentIdComparer.js index 15c2bc08..c26aa043 100644 --- a/src/matrix/room/timeline/FragmentIdComparer.js +++ b/src/matrix/room/timeline/FragmentIdComparer.js @@ -22,31 +22,31 @@ until no more fragments */ +import {isValidFragmentId} from "./common.js"; function findBackwardSiblingFragments(current, byId) { const sortedSiblings = []; - while (current.previousId) { + while (isValidFragmentId(current.previousId)) { const previous = byId.get(current.previousId); if (!previous) { - throw new Error(`Unknown previousId ${current.previousId} on ${current.id}`); + break; } if (previous.nextId !== current.id) { throw new Error(`Previous fragment ${previous.id} doesn't point back to ${current.id}`); } byId.delete(current.previousId); - sortedSiblings.push(previous); + sortedSiblings.unshift(previous); current = previous; } - sortedSiblings.reverse(); return sortedSiblings; } function findForwardSiblingFragments(current, byId) { const sortedSiblings = []; - while (current.nextId) { + while (isValidFragmentId(current.nextId)) { const next = byId.get(current.nextId); if (!next) { - throw new Error(`Unknown nextId ${current.nextId} on ${current.id}`); + break; } if (next.previousId !== current.id) { throw new Error(`Next fragment ${next.id} doesn't point back to ${current.id}`); @@ -143,7 +143,7 @@ export default class FragmentIdComparer { } add(fragment) { - this._fragmentsById[fragment.id] = fragment; + this._fragmentsById.set(fragment.id, fragment); this.rebuild(this._fragmentsById.values()); } } @@ -168,6 +168,44 @@ export function tests() { assert.equal(index.compare(1, 1), 0); }, + test_falsy_id(assert) { + const index = new FragmentIdComparer([ + {id: 0, nextId: 1}, + {id: 1, previousId: 0}, + ]); + assert(index.compare(0, 1) < 0); + assert(index.compare(1, 0) > 0); + }, + test_falsy_id_reverse(assert) { + const index = new FragmentIdComparer([ + {id: 1, previousId: 0}, + {id: 0, nextId: 1}, + ]); + assert(index.compare(0, 1) < 0); + assert(index.compare(1, 0) > 0); + }, + test_allow_unknown_id(assert) { + // as we tend to load fragments incrementally + // as events come into view, we need to allow + // unknown previousId/nextId in the fragments that we do load + assert.doesNotThrow(() => { + new FragmentIdComparer([ + {id: 1, previousId: 2}, + {id: 0, nextId: 3}, + ]); + }); + }, + test_throw_on_link_mismatch(assert) { + // as we tend to load fragments incrementally + // as events come into view, we need to allow + // unknown previousId/nextId in the fragments that we do load + assert.throws(() => { + new FragmentIdComparer([ + {id: 1, previousId: 0}, + {id: 0, nextId: 2}, + ]); + }); + }, test_2_island_dont_compare(assert) { const index = new FragmentIdComparer([ {id: 1}, diff --git a/src/matrix/room/timeline/common.js b/src/matrix/room/timeline/common.js new file mode 100644 index 00000000..7565d8a4 --- /dev/null +++ b/src/matrix/room/timeline/common.js @@ -0,0 +1,3 @@ +export function isValidFragmentId(id) { + return typeof id === "number"; +} diff --git a/src/matrix/room/timeline/entries/FragmentBoundaryEntry.js b/src/matrix/room/timeline/entries/FragmentBoundaryEntry.js index ce496364..3182fd2f 100644 --- a/src/matrix/room/timeline/entries/FragmentBoundaryEntry.js +++ b/src/matrix/room/timeline/entries/FragmentBoundaryEntry.js @@ -1,5 +1,6 @@ import BaseEntry from "./BaseEntry.js"; import Direction from "../Direction.js"; +import {isValidFragmentId} from "../common.js"; export default class FragmentBoundaryEntry extends BaseEntry { constructor(fragment, isFragmentStart, fragmentIdComparator) { @@ -77,6 +78,10 @@ export default class FragmentBoundaryEntry extends BaseEntry { } } + get hasLinkedFragment() { + return isValidFragmentId(this.linkedFragmentId); + } + get direction() { if (this.started) { return Direction.Backward; diff --git a/src/matrix/room/timeline/persistence/TimelineReader.js b/src/matrix/room/timeline/persistence/TimelineReader.js index dd57ca39..7d86115b 100644 --- a/src/matrix/room/timeline/persistence/TimelineReader.js +++ b/src/matrix/room/timeline/persistence/TimelineReader.js @@ -25,7 +25,6 @@ export default class TimelineReader { async _readFrom(eventKey, direction, amount, txn) { let entries = []; - const timelineStore = txn.timelineEvents; const fragmentStore = txn.timelineFragments; @@ -47,7 +46,7 @@ export default class TimelineReader { // append or prepend fragmentEntry, reuse func from GapWriter? directionalAppend(entries, fragmentEntry, direction); // don't count it in amount perhaps? or do? - if (fragmentEntry.linkedFragmentId) { + if (fragmentEntry.hasLinkedFragment) { const nextFragment = await fragmentStore.get(this._roomId, fragmentEntry.linkedFragmentId); this._fragmentIdComparer.add(nextFragment); const nextFragmentEntry = new FragmentBoundaryEntry(nextFragment, direction.isForward, this._fragmentIdComparer);