diff --git a/server/lib/matrix-utils/fetch-accessible-rooms.js b/server/lib/matrix-utils/fetch-accessible-rooms.js index c0c3b33..04184db 100644 --- a/server/lib/matrix-utils/fetch-accessible-rooms.js +++ b/server/lib/matrix-utils/fetch-accessible-rooms.js @@ -3,6 +3,7 @@ const assert = require('assert'); const urlJoin = require('url-join'); +const { DIRECTION } = require('matrix-public-archive-shared/lib/reference-values'); const { fetchEndpointAsJson } = require('../fetch-endpoint'); const { traceFunction } = require('../../tracing/trace-utilities'); @@ -43,18 +44,34 @@ async function requestPublicRooms( return publicRoomsRes; } -// eslint-disable-next-line complexity +// eslint-disable-next-line complexity, max-statements async function fetchAccessibleRooms( accessToken, - { server, searchTerm, paginationToken, limit, abortSignal } = {} + { + server, + searchTerm, + // Direction is baked into the pagination token but we're unable to decipher it from + // the opaque token, we also have to pass it in explicitly. + paginationToken, + direction = DIRECTION.forward, + limit, + abortSignal, + } = {} ) { assert(accessToken); + assert([DIRECTION.forward, DIRECTION.backward].includes(direction), 'direction must be [f|b]'); + + // Based off of the matrix.org room directory, only 42% of rooms are world_readable, + // which means our best bet to fill up the results to the limit is to request 2.5 times as many. + const bulkPaginationLimit = Math.ceil(2.5 * limit); let accessibleRooms = []; - let nextPaginationToken = paginationToken; - let prevPaginationToken; - let lastPaginationToken; + let firstResponse; + let lastResponse; + + let loopToken = paginationToken; + let lastLoopToken; let continuationIndex; let currentRequestCount = 0; while ( @@ -66,31 +83,28 @@ async function fetchAccessibleRooms( // Always do the first request (currentRequestCount === 0 || // If we have a next token, we can do another request - (currentRequestCount > 0 && nextPaginationToken)) + (currentRequestCount > 0 && loopToken)) ) { const publicRoomsRes = await requestPublicRooms(accessToken, { server, searchTerm, - paginationToken: nextPaginationToken, - // Based off of the matrix.org room directory, only 42% of rooms are world_readable, - // which means our best bet to fill up the results to the limit is to request 2.5 times as many. - limit: Math.ceil(2.5 * limit), + paginationToken: loopToken, + limit: bulkPaginationLimit, abortSignal, }); - lastPaginationToken = nextPaginationToken; + lastLoopToken = loopToken; + lastResponse = publicRoomsRes; - // We keep track prev_batch token from the first request only as we may be - // paginating over many pages but to the client, it just appears like one they can - // go back to. if (currentRequestCount === 0) { - prevPaginationToken = publicRoomsRes.prev_batch; + firstResponse = publicRoomsRes; } // Keep track of this as we go. For the final pagination token, we return to the // client, we might need to back-track later to get the perfect continuation point // if we find more than the limit of rooms we want to fetch. Alternatively, we could // just not worry about and show more than the limit of rooms. - nextPaginationToken = publicRoomsRes.next_batch; + loopToken = + direction === DIRECTION.forward ? publicRoomsRes.next_batch : publicRoomsRes.prev_batch; // We only want to see world_readable rooms in the archive let index = 0; @@ -114,30 +128,64 @@ async function fetchAccessibleRooms( currentRequestCount += 1; } + // Back-track to get the perfect continuation point + // + // Alternatively, we could just not worry about and show more than the limit of rooms + // // XXX: Since the room directory order is not stable, this is slightly flawed as the // results could have shifted slightly from when we made the last request to get the // `continuationIndex` to now when we're trying to get the actual token to continue // seamlessly. - // - // Alternatively, we could just not worry about and show more than the limit of rooms + let nextPaginationToken; + let prevPaginationToken; if (continuationIndex) { + // let continuationLimit; + // if (direction === DIRECTION.forward) { + // continuationLimit = continuationIndex + 1; + // } else if (direction === DIRECTION.backward) { + // continuationLimit = bulkPaginationLimit - continuationIndex; + // } else { + // throw new Error(`Invalid direction: ${direction}`); + // } + const publicRoomsRes = await requestPublicRooms(accessToken, { server, searchTerm, - // Start from the beginning of the last request - paginationToken: lastPaginationToken, - // Then go out only as far as the continuation index (the point when we filled the limit) + // Start from the last request + paginationToken: lastLoopToken, + // Then only go out as far out as the continuation index (the point when we filled the limit) limit: continuationIndex + 1, abortSignal, }); - nextPaginationToken = publicRoomsRes.next_batch; + // console.log('firstResponse', firstResponse.prev_batch, firstResponse.next_batch); + // console.log('back-track publicRoomsRes', publicRoomsRes.prev_batch, publicRoomsRes.next_batch); + + if (direction === DIRECTION.forward) { + prevPaginationToken = firstResponse.prev_batch; + nextPaginationToken = publicRoomsRes.next_batch; + } else if (direction === DIRECTION.backward) { + prevPaginationToken = publicRoomsRes.prev_batch; + nextPaginationToken = firstResponse.next_batch; + } else { + throw new Error(`Invalid direction: ${direction}`); + } + } else { + if (direction === DIRECTION.forward) { + prevPaginationToken = firstResponse.prev_batch; + nextPaginationToken = lastResponse.next_batch; + } else if (direction === DIRECTION.backward) { + prevPaginationToken = lastResponse.prev_batch; + nextPaginationToken = firstResponse.next_batch; + } else { + throw new Error(`Invalid direction: ${direction}`); + } } return { rooms: accessibleRooms, - nextPaginationToken, prevPaginationToken, + nextPaginationToken, }; } diff --git a/server/routes/room-directory-routes.js b/server/routes/room-directory-routes.js index caedc8e..fde1467 100644 --- a/server/routes/room-directory-routes.js +++ b/server/routes/room-directory-routes.js @@ -6,6 +6,7 @@ const urlJoin = require('url-join'); const express = require('express'); const asyncHandler = require('../lib/express-async-handler'); +const { DIRECTION } = require('matrix-public-archive-shared/lib/reference-values'); const RouteTimeoutAbortError = require('../lib/errors/route-timeout-abort-error'); const UserClosedConnectionAbortError = require('../lib/errors/user-closed-connection-abort-error'); const identifyRoute = require('../middleware/identify-route-middleware'); @@ -33,9 +34,19 @@ router.get( '/', identifyRoute('app-room-directory-index'), asyncHandler(async function (req, res) { - const paginationToken = req.query.page; const searchTerm = req.query.search; const homeserver = req.query.homeserver; + const paginationToken = req.query.page; + const direction = req.query.dir; + + // You must provide both `paginationToken` and `direction` if either is defined + if (paginationToken || direction) { + assert( + [DIRECTION.forward, DIRECTION.backward].includes(direction), + '?dir query parameter must be [f|b]' + ); + assert(paginationToken, '?page query parameter must be defined if ?dir is defined'); + } // It would be good to grab more rooms than we display in case we need // to filter any out but then the pagination tokens with the homeserver @@ -54,6 +65,7 @@ router.get( server: homeserver, searchTerm, paginationToken, + direction, limit, abortSignal: req.abortSignal, } diff --git a/shared/lib/url-creator.js b/shared/lib/url-creator.js index 4285d21..f8b9b3d 100644 --- a/shared/lib/url-creator.js +++ b/shared/lib/url-creator.js @@ -3,7 +3,10 @@ const urlJoin = require('url-join'); const assert = require('matrix-public-archive-shared/lib/assert'); -const { TIME_PRECISION_VALUES } = require('matrix-public-archive-shared/lib/reference-values'); +const { + DIRECTION, + TIME_PRECISION_VALUES, +} = require('matrix-public-archive-shared/lib/reference-values'); function qsToUrlPiece(qs) { if (qs.toString()) { @@ -25,7 +28,16 @@ class URLCreator { return `https://matrix.to/#/${roomIdOrAlias}`; } - roomDirectoryUrl({ searchTerm, homeserver, paginationToken } = {}) { + roomDirectoryUrl({ searchTerm, homeserver, paginationToken, direction } = {}) { + // You must provide both `paginationToken` and `direction` if either is defined + if (paginationToken || direction) { + assert( + [DIRECTION.forward, DIRECTION.backward].includes(direction), + 'direction must be [f|b]' + ); + assert(paginationToken); + } + let qs = new URLSearchParams(); if (searchTerm) { qs.append('search', searchTerm); @@ -36,6 +48,9 @@ class URLCreator { if (paginationToken) { qs.append('page', paginationToken); } + if (direction) { + qs.append('dir', direction); + } return `${this._basePath}${qsToUrlPiece(qs)}`; } diff --git a/shared/viewmodels/RoomDirectoryViewModel.js b/shared/viewmodels/RoomDirectoryViewModel.js index 90c5c13..3268b6b 100644 --- a/shared/viewmodels/RoomDirectoryViewModel.js +++ b/shared/viewmodels/RoomDirectoryViewModel.js @@ -9,6 +9,7 @@ const ModalViewModel = require('matrix-public-archive-shared/viewmodels/ModalVie const HomeserverSelectionModalContentViewModel = require('matrix-public-archive-shared/viewmodels/HomeserverSelectionModalContentViewModel'); const RoomCardViewModel = require('matrix-public-archive-shared/viewmodels/RoomCardViewModel'); const checkTextForNsfw = require('matrix-public-archive-shared/lib/check-text-for-nsfw'); +const { DIRECTION } = require('../lib/reference-values'); const DEFAULT_SERVER_LIST = ['matrix.org', 'gitter.im']; @@ -304,6 +305,7 @@ class RoomDirectoryViewModel extends ViewModel { homeserver: this.homeserverSelection, searchTerm: this.searchTerm, paginationToken: this._nextPaginationToken, + direction: DIRECTION.forward, }); } @@ -316,6 +318,7 @@ class RoomDirectoryViewModel extends ViewModel { homeserver: this.homeserverSelection, searchTerm: this.searchTerm, paginationToken: this._prevPaginationToken, + direction: DIRECTION.backward, }); } diff --git a/test/e2e-tests.js b/test/e2e-tests.js index ec7a9ff..deb142a 100644 --- a/test/e2e-tests.js +++ b/test/e2e-tests.js @@ -2745,7 +2745,7 @@ describe('matrix-public-archive', () => { // doing them serially and the room directory doesn't return the rooms in any // particular order so it doesn't make the test any more clear doing them // serially. - await Promise.all( + const createdRoomsIds = await Promise.all( roomsConfigurationsToCreate.map((roomCreateOptions) => createTestRoom(client, roomCreateOptions) ) @@ -2786,6 +2786,7 @@ describe('matrix-public-archive', () => { const nextPaginationLink = nextLinkElement.getAttribute('href'); return { + archiveUrl, roomsIdsOnPage, previousPaginationLink, nextPaginationLink, @@ -2817,12 +2818,56 @@ describe('matrix-public-archive', () => { const firstPage = await checkRoomsOnPage(archiveUrl); const secondPage = await checkRoomsOnPage(firstPage.nextPaginationLink); const thirdPage = await checkRoomsOnPage(secondPage.nextPaginationLink); + const backtrackSecondPage = await checkRoomsOnPage(thirdPage.previousPaginationLink); + const backtrackFirstPage = await checkRoomsOnPage( + backtrackSecondPage.previousPaginationLink + ); - // TODO: This does not work (pagination tokens are not stable) - assert.strictEqual(firstPage.previousPaginationLink, null); - assert.strictEqual(firstPage.nextPaginationLink, secondPage.previousPaginationLink); - assert.strictEqual(secondPage.nextPaginationLink, thirdPage.previousPaginationLink); - assert.strictEqual(thirdPage.nextPaginationLink, null); + // assert.strictEqual(firstPage.previousPaginationLink, null); + // assert.strictEqual(firstPage.nextPaginationLink, secondPage.previousPaginationLink); + // assert.strictEqual(secondPage.nextPaginationLink, thirdPage.previousPaginationLink); + // assert.strictEqual(thirdPage.nextPaginationLink, null); + + function roomIdToRoomName(expectedRoomId) { + const roomIndex = createdRoomsIds.findIndex((roomId) => { + return roomId === expectedRoomId; + }); + assert( + roomIndex > 0, + `Expected to find expectedRoomId=${expectedRoomId} in the list of created rooms createdRoomsIds=${createdRoomsIds}` + ); + + const roomConfig = roomsConfigurationsToCreate[roomIndex]; + assert( + roomConfig, + `Expected to find room config for roomIndex=${roomIndex} in the list of roomsConfigurationsToCreate (length ${roomsConfigurationsToCreate.length})}` + ); + + return roomConfig.name; + } + + // Ensure that we saw all of the visible rooms paginating through the directory + assert.deepStrictEqual( + [...firstPage.roomsIdsOnPage, ...secondPage.roomsIdsOnPage, ...thirdPage.roomsIdsOnPage] + .map(roomIdToRoomName) + .sort(), + visibleRoomConfigurations.map((roomConfig) => roomConfig.name).sort(), + 'Make sure we saw all visible rooms paginating through the directory' + ); + + // Ensure that we see the same rooms in the same order going backward that we saw going forward + archiveUrl = backtrackSecondPage.archiveUrl; + assert.deepStrictEqual( + backtrackSecondPage.roomsIdsOnPage.map(roomIdToRoomName), + secondPage.roomsIdsOnPage.map(roomIdToRoomName), + 'From the third page, going backward to second page should show the same rooms that we saw on the second page when going forward' + ); + archiveUrl = backtrackFirstPage.archiveUrl; + assert.deepStrictEqual( + backtrackFirstPage.roomsIdsOnPage.map(roomIdToRoomName), + firstPage.roomsIdsOnPage.map(roomIdToRoomName), + 'From the second page, going backward to first page should show the same rooms that we saw on first page when going forward' + ); }); });