fix ordered by-query call

Signed-off-by: Fredrik Adelöw <freben@gmail.com>

.changeset/tough-peaches-kneel.md

@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-catalog-backend': patch
+---
+
+Fixed an issue with the by-query call, where ordering by a field that does not exist on all entities led to not all results being returned

plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts

@@ -1648,6 +1648,43 @@ describe('DefaultEntitiesCatalog', () => {
         expect(response5.totalItems).toBe(6);
       },
     );
+
+    it.each(databases.eachSupportedId())(
+      'should sort properly for fields that do not exist on all entities, %p',
+      async databaseId => {
+        await createDatabase(databaseId);
+
+        await Promise.all([
+          addEntityToSearch(entityFrom('AA', { uid: 'id1' })),
+          addEntityToSearch(entityFrom('BB', { uid: 'id2', title: 'YY' })),
+          addEntityToSearch(entityFrom('CC', { uid: 'id3', title: 'XX' })),
+        ]);
+
+        const catalog = new DefaultEntitiesCatalog({
+          database: knex,
+          logger: mockServices.logger.mock(),
+          stitcher,
+        });
+
+        await expect(
+          catalog
+            .queryEntities({
+              orderFields: [{ field: 'metadata.title', order: 'asc' }],
+              credentials: mockCredentials.none(),
+            })
+            .then(r => r.items.map(e => e.metadata.name)),
+        ).resolves.toEqual(['CC', 'BB', 'AA']); // 'AA' has no title, ends up last
+
+        await expect(
+          catalog
+            .queryEntities({
+              orderFields: [{ field: 'metadata.title', order: 'desc' }],
+              credentials: mockCredentials.none(),
+            })
+            .then(r => r.items.map(e => e.metadata.name)),
+        ).resolves.toEqual(['BB', 'CC', 'AA']); // 'AA' has no title, ends up last
+      },
+    );
   });
 
   describe('removeEntityByUid', () => {

plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts

@@ -379,12 +379,20 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog {
     const [prevItemOrderFieldValue, prevItemUid] =
       cursor.orderFieldValues || [];
 
-    const dbQuery = db('search')
-      .join('final_entities', 'search.entity_id', 'final_entities.entity_id')
-      .where('search.key', sortField.field);
+    const dbQuery = db('final_entities').leftOuterJoin('search', qb =>
+      qb
+        .on('search.entity_id', 'final_entities.entity_id')
+        .andOnVal('search.key', sortField.field),
+    );
 
     if (cursor.filter) {
-      parseFilter(cursor.filter, dbQuery, db, false, 'search.entity_id');
+      parseFilter(
+        cursor.filter,
+        dbQuery,
+        db,
+        false,
+        'final_entities.entity_id',
+      );
     }
 
     const normalizedFullTextFilterTerm = cursor.fullTextFilter?.term?.trim();
@@ -410,7 +418,7 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog {
               `%${normalizedFullTextFilterTerm.toLocaleLowerCase('en-US')}%`,
             );
           });
-        dbQuery.andWhere('search.entity_id', 'in', matchQuery);
+        dbQuery.andWhere('final_entities.entity_id', 'in', matchQuery);
       }
     }
 
@@ -427,32 +435,59 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog {
         )
           .orWhere('value', '=', prevItemOrderFieldValue)
           .andWhere(
-            'search.entity_id',
+            'final_entities.entity_id',
             isFetchingBackwards !== isOrderingDescending ? '<' : '>',
             prevItemUid,
           );
       });
     }
 
-    dbQuery
-      .orderBy([
+    if (db.client.config.client === 'pg') {
+      // pg correctly orders by the column value and handling nulls in one go
+      dbQuery.orderBy([
         {
-          column: 'value',
+          column: 'search.value',
+          order: isFetchingBackwards
+            ? invertOrder(sortField.order)
+            : sortField.order,
+          nulls: 'last',
+        },
+        {
+          column: 'final_entities.entity_id',
+          order: isFetchingBackwards
+            ? invertOrder(sortField.order)
+            : sortField.order,
+        },
+      ]);
+    } else {
+      // sqlite and mysql translate the above statement ONLY into "order by (value is null) asc"
+      // no matter what the order is, for some reason, so we have to manually add back the statement
+      // that translates to "order by value <order>" while avoiding to give an order
+      dbQuery.orderBy([
+        {
+          column: 'search.value',
+          order: undefined,
+          nulls: 'last',
+        },
+        {
+          column: 'search.value',
           order: isFetchingBackwards
             ? invertOrder(sortField.order)
             : sortField.order,
         },
         {
-          column: 'search.entity_id',
+          column: 'final_entities.entity_id',
           order: isFetchingBackwards
             ? invertOrder(sortField.order)
             : sortField.order,
         },
-      ])
-      // fetch an extra item to check if there are more items.
-      .limit(isFetchingBackwards ? limit : limit + 1);
+      ]);
+    }
 
-    countQuery.count('search.entity_id', { as: 'count' });
+    // fetch an extra item to check if there are more items.
+    dbQuery.limit(isFetchingBackwards ? limit : limit + 1);
+
+    countQuery.count('final_entities.entity_id', { as: 'count' });
 
     const [rows, [{ count }]] = await Promise.all([
       limit > 0 ? dbQuery : [],