[O2B-1544] Fix pagination for filtered envs and add a test (#2096)

* Replaced the two-query pattern with a single queryBuilder in GetAllEnvironmentsUseCase. The previous approach was redundant following Sequelize performance improvements; furthermore, the original implementation's logic was flawed which resulted in the pagination bug.

Author: isaachilly

lib/usecases/environment/GetAllEnvironmentsUseCase.js

@@ -69,18 +69,11 @@ class GetAllEnvironmentsUseCase {
         const { filter, page = {} } = query;
         const { limit = ApiConfig.pagination.limit, offset = 0 } = page;
 
-        /**
-         * Prepare a query builder with ordering, limit and offset
-         *
-         * @return {QueryBuilder} the created query builder
-         */
-        const prepareQueryBuilder = () => dataSource.createQueryBuilder()
+        const queryBuilder = dataSource.createQueryBuilder()
             .orderBy('updatedAt', 'desc')
             .limit(limit)
             .offset(offset);
 
-        const fetchQueryBuilder = prepareQueryBuilder();
-
         if (filter) {
             const {
                 ids: idsExpression,
@@ -90,38 +83,36 @@ class GetAllEnvironmentsUseCase {
                 created,
             } = filter;
 
-            const filterQueryBuilder = prepareQueryBuilder();
-
             if (created) {
                 const from = created.from !== undefined ? created.from : 0;
                 const to = created.to !== undefined ? created.to : Date.now();
-                filterQueryBuilder.where('createdAt').between(from, to);
+                queryBuilder.where('createdAt').between(from, to);
             }
 
             if (idsExpression) {
                 const filters = idsExpression.split(',').map((id) => id.trim());
 
                 // Filter should be like with only one filter
                 if (filters.length === 1) {
-                    filterQueryBuilder.where('id').substring(filters[0]);
+                    queryBuilder.where('id').substring(filters[0]);
                 }
 
                 // Filters should be exact with more than one filter
                 if (filters.length > 1) {
-                    filterQueryBuilder.andWhere({ id: { [Op.in]: filters } });
+                    queryBuilder.andWhere({ id: { [Op.in]: filters } });
                 }
             }
 
             if (currentStatusExpression) {
                 const filters = currentStatusExpression.split(',').map((status) => status.trim());
 
                 // Filter the environments by current status using the subquery
-                filterQueryBuilder.literalWhere(
+                queryBuilder.literalWhere(
                     `${ENVIRONMENT_LATEST_HISTORY_ITEM_SUBQUERY} IN (:filters)`,
                     { filters },
                 );
 
-                filterQueryBuilder.includeAttribute({
+                queryBuilder.includeAttribute({
                     query: ENVIRONMENT_LATEST_HISTORY_ITEM_SUBQUERY,
                     alias: 'currentStatus',
                 });
@@ -157,7 +148,7 @@ class GetAllEnvironmentsUseCase {
                      * Use OR condition to match subsequences ending with either DESTROYED or DONE
                      * Filter the environments by using LIKE for subsequence matching
                      */
-                    filterQueryBuilder.literalWhere(
+                    queryBuilder.literalWhere(
                         `(${ENVIRONMENT_STATUS_HISTORY_SUBQUERY} LIKE :statusFiltersWithDestroyed OR ` +
                         `${ENVIRONMENT_STATUS_HISTORY_SUBQUERY} LIKE :statusFiltersWithDone)`,
                         {
@@ -166,17 +157,17 @@ class GetAllEnvironmentsUseCase {
                         },
                     );
 
-                    filterQueryBuilder.includeAttribute({
+                    queryBuilder.includeAttribute({
                         query: ENVIRONMENT_STATUS_HISTORY_SUBQUERY,
                         alias: 'statusHistory',
                     });
                 } else {
-                    filterQueryBuilder.literalWhere(
+                    queryBuilder.literalWhere(
                         `${ENVIRONMENT_STATUS_HISTORY_SUBQUERY} LIKE :statusFilters`,
                         { statusFilters: `%${statusFilters.join(',')}%` },
                     );
 
-                    filterQueryBuilder.includeAttribute({
+                    queryBuilder.includeAttribute({
                         query: ENVIRONMENT_STATUS_HISTORY_SUBQUERY,
                         alias: 'statusHistory',
                     });
@@ -190,30 +181,20 @@ class GetAllEnvironmentsUseCase {
 
                 // Check that the final run numbers list contains at least one valid run number
                 if (finalRunNumberList.length > 0) {
-                    filterQueryBuilder.include({
+                    queryBuilder.include({
                         association: 'runs',
                         where: {
                             // Filter should be like with only one filter and exact with more than one filter
                             runNumber: { [finalRunNumberList.length === 1 ? Op.substring : Op.in]: finalRunNumberList },
                         },
                     });
                 }
-            };
-
-            const filteredEnvironmentsIds = (await EnvironmentRepository.findAll(filterQueryBuilder)).map(({ id }) => id);
-            // If no environments match the filter, return an empty result
-            if (filteredEnvironmentsIds.length === 0) {
-                return {
-                    count: 0,
-                    environments: [],
-                };
             }
-            fetchQueryBuilder.where('id').oneOf(filteredEnvironmentsIds);
         }
 
-        fetchQueryBuilder.include({ association: 'runs' });
-        fetchQueryBuilder.include({ association: 'historyItems' });
-        const { count, rows } = await EnvironmentRepository.findAndCountAll(fetchQueryBuilder);
+        queryBuilder.include({ association: 'runs' });
+        queryBuilder.include({ association: 'historyItems' });
+        const { count, rows } = await EnvironmentRepository.findAndCountAll(queryBuilder);
         return {
             count,
             environments: rows.map((environment) => environmentAdapter.toEntity(environment)),

test/lib/usecases/environment/GetAllEnvironmentsUseCase.test.js

@@ -225,4 +225,40 @@ module.exports = () => {
         expect(environments).to.be.an('array');
         expect(environments.length).to.be.equal(0); // Environments from seeders
     });
+
+    it('should return correct total count and all filtered results across pages', async () => {
+        const totalMatchingFilter = 6; // 'RUNNING, ERROR' matches 6 environments at this point
+        const limit = 2;
+
+        // First page
+        getAllEnvsDto.query = { page: { limit, offset: 0 }, filter: { currentStatus: 'RUNNING, ERROR' } };
+        const page1 = await new GetAllEnvironmentsUseCase().execute(getAllEnvsDto);
+
+        expect(page1.count).to.be.equal(totalMatchingFilter);
+        expect(page1.environments).to.be.an('array');
+        expect(page1.environments.length).to.be.equal(limit);
+
+        // Second page
+        getAllEnvsDto.query = { page: { limit, offset: 2 }, filter: { currentStatus: 'RUNNING, ERROR' } };
+        const page2 = await new GetAllEnvironmentsUseCase().execute(getAllEnvsDto);
+
+        expect(page2.count).to.be.equal(totalMatchingFilter);
+        expect(page2.environments).to.be.an('array');
+        expect(page2.environments.length).to.be.equal(limit);
+
+        // Third page
+        getAllEnvsDto.query = { page: { limit, offset: 4 }, filter: { currentStatus: 'RUNNING, ERROR' } };
+        const page3 = await new GetAllEnvironmentsUseCase().execute(getAllEnvsDto);
+
+        expect(page3.count).to.be.equal(totalMatchingFilter);
+        expect(page3.environments).to.be.an('array');
+        expect(page3.environments.length).to.be.equal(limit);
+
+        // Collect all environment IDs and verify no duplicates and all present
+        const allIds = [page1, page2, page3].flatMap(({ environments })=> environments.map(({ id }) => id));
+
+        expect(allIds.length).to.be.equal(totalMatchingFilter);
+        expect(new Set(allIds).size).to.be.equal(totalMatchingFilter);
+        expect(allIds).to.have.members(['SomeId', 'newId', 'CmCvjNbg', 'EIDO13i3D', '8E4aZTjY', 'Dxi029djX']);
+    });
 };