Fix comments

Signed-off-by: Vincenzo Scamporlino <me@vinzscam.dev>

plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts

@@ -13,6 +13,8 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+
+import { ConfigReader } from '@backstage/config';
 import {
   AuthorizeDecision,
   AuthorizeResult,
@@ -91,6 +93,7 @@ describe('AuthorizedSearchEngine', () => {
     searchEngine,
     defaultTypes,
     permissionAuthorizer,
+    new ConfigReader({}),
   );
 
   const options = { token: 'token' };
@@ -201,7 +204,6 @@ describe('AuthorizedSearchEngine', () => {
 
     const userToBeReturned = 8;
 
-    // TODO(vinzscam): I am not sure if such authorizer makes sense
     mockedAuthorize.mockImplementation(async queries =>
       queries.map(query => {
         if (
@@ -217,11 +219,7 @@ describe('AuthorizedSearchEngine', () => {
           }
           return {
             result: AuthorizeResult.CONDITIONAL,
-            // TODO(vinzscam): is something like this allowed?
-            // I guess NO, but I am not sure.
-            // if yes, who is responsible for evaluating this?
-            conditions: { not: { rule: 'ok', params: [] } },
-          };
+          } as AuthorizeDecision;
         }
 
         return {
@@ -246,7 +244,6 @@ describe('AuthorizedSearchEngine', () => {
         if (query.resourceRef) {
           return { result: AuthorizeResult.ALLOW };
         }
-        // TODO(vinzscam): again, not sure about this
         return { result: AuthorizeResult.CONDITIONAL } as AuthorizeDecision;
       }),
     );
@@ -255,17 +252,23 @@ describe('AuthorizedSearchEngine', () => {
     const templatesWithAuth = generateSampleResults(typeTemplates, true);
     const servicesWithAuth = generateSampleResults(typeServices, true);
 
+    const allDocuments = [
+      ...usersWithAuth,
+      ...templatesWithAuth,
+      ...servicesWithAuth,
+    ].sort(() => Math.floor(Math.random() * 3 - 1));
+
     mockedQuery
       .mockImplementationOnce(async () => ({
-        results: usersWithAuth,
+        results: allDocuments.slice(0, 10),
         nextPageCursor: encodePageCursor({ page: 1 }),
       }))
       .mockImplementationOnce(async () => ({
-        results: templatesWithAuth,
+        results: allDocuments.slice(10, 20),
         nextPageCursor: encodePageCursor({ page: 2 }),
       }))
       .mockImplementationOnce(async () => ({
-        results: servicesWithAuth,
+        results: allDocuments.slice(20, 30),
       }));
 
     const result = await authorizedSearchEngine.query({ term: '' }, options);
@@ -287,11 +290,7 @@ describe('AuthorizedSearchEngine', () => {
       { token: 'token' },
     );
 
-    const expectedResult = [
-      ...usersWithAuth,
-      ...templatesWithAuth,
-      ...servicesWithAuth,
-    ].slice(0, 25);
+    const expectedResult = allDocuments.slice(0, 25);
 
     const expectedFirstRequestCursor = 'MQ==';
     expect(result).toEqual({
@@ -306,7 +305,6 @@ describe('AuthorizedSearchEngine', () => {
         if (query.resourceRef) {
           return { result: AuthorizeResult.ALLOW };
         }
-        // TODO(vinzscam): again, not sure about this
         return { result: AuthorizeResult.CONDITIONAL } as AuthorizeDecision;
       }),
     );

plugins/search-backend/src/service/AuthorizedSearchEngine.ts

@@ -32,6 +32,7 @@ import {
   SearchResult,
   SearchResultSet,
 } from '@backstage/search-common';
+import { Config } from '@backstage/config';
 import { InputError } from '@backstage/errors';
 
 export function decodePageCursor(pageCursor?: string): { page: number } {
@@ -63,15 +64,18 @@ export type AuthorizedSearchEngineConfig = {
 };
 
 export class AuthorizedSearchEngine implements SearchEngine {
-  private readonly config: AuthorizedSearchEngineConfig;
+  private readonly pageSize = 25;
+  private readonly queryLatencyBudgetMs: number;
 
   constructor(
     private readonly searchEngine: SearchEngine,
     private readonly types: Record<string, DocumentTypeInfo>,
     private readonly permissions: PermissionAuthorizer,
-    config?: Partial<AuthorizedSearchEngineConfig>,
+    config: Config,
   ) {
-    this.config = { queryLatencyBudgetMs: 1000, pageSize: 25, ...config };
+    this.queryLatencyBudgetMs =
+      config.getOptionalNumber('search.permissions.queryLatencyBudgetMs') ??
+      1000;
   }
 
   setTranslator(translator: QueryTranslator): void {
@@ -134,7 +138,7 @@ export class AuthorizedSearchEngine implements SearchEngine {
     }
 
     const { page } = decodePageCursor(query.pageCursor);
-    const targetResults = (page + 1) * this.config.pageSize;
+    const targetResults = (page + 1) * this.pageSize;
 
     let filteredResults: SearchResult[] = [];
     let nextPageCursor: string | undefined;
@@ -152,7 +156,7 @@ export class AuthorizedSearchEngine implements SearchEngine {
 
       nextPageCursor = nextPage.nextPageCursor;
       latencyBudgetExhausted =
-        Date.now() - queryStartTime > this.config.queryLatencyBudgetMs;
+        Date.now() - queryStartTime > this.queryLatencyBudgetMs;
     } while (
       nextPageCursor &&
       filteredResults.length < targetResults &&
@@ -161,8 +165,8 @@ export class AuthorizedSearchEngine implements SearchEngine {
 
     return {
       results: filteredResults.slice(
-        page * this.config.pageSize,
-        (page + 1) * this.config.pageSize,
+        page * this.pageSize,
+        (page + 1) * this.pageSize,
       ),
       previousPageCursor:
         page === 0 ? undefined : encodePageCursor({ page: page - 1 }),

plugins/search-backend/src/service/router.ts

@@ -68,11 +68,7 @@ export async function createRouter(
   });
 
   const engine = config.getOptionalBoolean('permission.enabled')
-    ? new AuthorizedSearchEngine(inputEngine, types, permissions, {
-        queryLatencyBudgetMs: config.getOptionalNumber(
-          'search.permissions.queryLatencyBudgetMs',
-        ),
-      })
+    ? new AuthorizedSearchEngine(inputEngine, types, permissions, config)
     : inputEngine;
 
   const filterResultSet = ({ results, ...resultSet }: SearchResultSet) => ({