[RND-161] enable limit/offset query parameters (#10)

* [RND-161] enable limit/offset query parameters * [RND-161] Test http script for remote deploy, small dockerfile fixup * [RND-161] PR feedback * Mention docker desktop for Windows Co-authored-by: Stephen Fuqua <[email protected]>
Ed-Fi-Exchange-OSS · Dec 17, 2021 · 93d5f60 · 93d5f60
1 parent dd4668b
commit 93d5f60
Show file tree

Hide file tree

Showing 7 changed files with 211 additions and 147 deletions.
diff --git a/Meadowlark-js/docker-meadowlark/readme.md b/Meadowlark-js/docker-meadowlark/readme.md
@@ -3,7 +3,8 @@
 (!) This solution should only be used on localhost with proper firewalls around
 external network access to the workstation. Not appropriate for production use.
 
-This compose file provisions:
+This compose file requires [Docker Compose v2](https://github.com/docker/compose)
+(which comes with Docker Desktop for Windows users). It provisions:
 
 * OpenSearch
 * OpenSearch Dashboard at [http://localhost:5601/](http://localhost:5601/)
@@ -15,6 +16,7 @@ Ensure that you have sufficient resources allocated to Docker:
 * macOS: set RAM to at least 4 GB [user manual](https://docs.docker.com/desktop/mac/).
 * Linux, including Windows Subsystem for Linux (WSL/WSL2): Ensure vm.max_map_count is set to at least 262144 as [per the
   documentation](https://opensearch.org/docs/opensearch/install/important-settings/).
+  * On Linux: ```sudo sysctl -w vm.max_map_count=262144```
   * Permanently setting this on WSL is more challenging than expected. Recipe:
 
     ```powershell

diff --git a/Meadowlark-js/src/handler/CrudHandler.ts b/Meadowlark-js/src/handler/CrudHandler.ts
@@ -5,7 +5,6 @@
 
 /* eslint-disable-next-line import/no-unresolved */
 import { APIGatewayProxyEvent, APIGatewayProxyResult, Context } from 'aws-lambda';
-import R from 'ramda';
 import { validateResource } from './RequestValidator';
 import {
   createEntity,
@@ -19,7 +18,7 @@ import {
 
 import { Logger } from '../helpers/Logger';
 import { PathComponents } from '../model/PathComponents';
-import { getById, list, query } from './GetResolvers';
+import { getById, query } from './GetResolvers';
 import { isDocumentIdValid, documentIdForEntityInfo } from '../helpers/DocumentId';
 import { NoEntityInfo } from '../model/EntityInfo';
 import { validateJwt } from '../helpers/JwtValidator';
@@ -172,8 +171,6 @@ export async function create(event: APIGatewayProxyEvent, context: Context): Pro
   }
 }
 
-const removeDisallowedQueryParameters = R.omit(['offset', 'limit', 'totalCount']);
-
 /**
  * Entry point for all API GET requests
  *
@@ -212,17 +209,7 @@ export async function getResolver(event: APIGatewayProxyEvent, context: Context)
         clientName: jwtStatus.subject,
       });
 
-    const cleanQueryParameters: object = removeDisallowedQueryParameters(event.queryStringParameters) ?? {};
-
-    if (Object.keys(cleanQueryParameters).length === 0) return await list(pathComponents, context);
-
-    return await query(pathComponents, cleanQueryParameters, context, {
-      edOrgIds,
-      studentIds,
-      throughAssociation,
-      isOwnershipEnabled: jwtStatus.isOwnershipEnabled,
-      clientName: jwtStatus.subject,
-    });
+    return await query(pathComponents, event.queryStringParameters ?? {}, context);
   } catch (e) {
     writeErrorToLog(context, event, 'getResolver', 500, e);
     return { body: '', statusCode: 500 };

diff --git a/Meadowlark-js/src/handler/GetResolvers.ts b/Meadowlark-js/src/handler/GetResolvers.ts
@@ -8,12 +8,13 @@
 
 // eslint-disable-next-line import/no-unresolved
 import { APIGatewayProxyResult, Context } from 'aws-lambda';
+import R from 'ramda';
 
 import { Logger } from '../helpers/Logger';
 import { PathComponents } from '../model/PathComponents';
 import { Security } from '../model/Security';
 import { getEntityById, getEntityList } from '../repository/DynamoEntityRepository';
-import { queryEntityList } from '../repository/ElasticsearchRepository';
+import { PaginationParameters, queryEntityList } from '../repository/ElasticsearchRepository';
 import { validateResource } from './RequestValidator';
 
 function writeDebugStatusToLog(context: Context, method: string, status: number, message: string = ''): void {
@@ -97,6 +98,9 @@ export async function getById(
   };
 }
 
+const removeDisallowedQueryParameters = R.omit(['offset', 'limit', 'totalCount']);
+const onlyPaginationParameters = R.pick(['offset', 'limit']);
+
 /**
  * Handler for API GET requests that represent queries
  *
@@ -106,18 +110,20 @@ export async function query(
   pathComponents: PathComponents,
   queryStringParameters: object,
   context: Context,
-  security: Security,
 ): Promise<APIGatewayProxyResult> {
   const { entityInfo, errorBody, metaEdProjectHeaders } = await validateResource(pathComponents, null);
   if (errorBody !== null) {
     writeDebugStatusToLog(context, 'query', 404, errorBody);
     return { body: errorBody, statusCode: 404, headers: metaEdProjectHeaders };
   }
 
+  const cleanQueryParameters: object = removeDisallowedQueryParameters(queryStringParameters);
+  const paginationParameters: PaginationParameters = onlyPaginationParameters(queryStringParameters);
+
   const { success, results } = await queryEntityList(
     entityInfo,
-    queryStringParameters ?? {},
-    security,
+    cleanQueryParameters,
+    paginationParameters,
     context.awsRequestId,
   );
 

diff --git a/Meadowlark-js/src/repository/ElasticsearchRepository.ts b/Meadowlark-js/src/repository/ElasticsearchRepository.ts
@@ -3,22 +3,28 @@
 // The Ed-Fi Alliance licenses this file to you under the Apache License, Version 2.0.
 // See the LICENSE and NOTICES files in the project root for more information.
 
-import R from 'ramda';
 import { ClientRequestArgs, request } from 'http';
 import { Client, Connection } from '@elastic/elasticsearch';
 import { ConnectionOptions } from '@elastic/elasticsearch/lib/Connection';
 import { defaultProvider as AWSCredentialProvider } from '@aws-sdk/credential-provider-node';
 import type { Credentials } from '@aws-sdk/types';
 import { sign } from 'aws4';
 import { EntityInfo, entityTypeStringFrom } from '../model/EntityInfo';
-import { Security } from '../model/Security';
 import { Logger } from '../helpers/Logger';
 
 export type GetResult = {
   success: boolean;
   results: Array<object>;
 };
 
+/**
+ * The special query parameters used to drive pagination
+ */
+export type PaginationParameters = {
+  limit?: string;
+  offset?: string;
+};
+
 /**
  * A replacement for @acuris/aws-es-connection's "createAWSConnection", using aws4.
  * Allows us to use @aws-sdk/credential-provider-node instead of aws-sdk v2 dependency.
@@ -53,7 +59,7 @@ export function indexFromEntityInfo(entityInfo: EntityInfo): string {
   return indexFromEntityTypeString(entityTypeStringFrom(entityInfo));
 }
 
-// TODO: unsafe for SQL injection
+// TODO: RND-203 unsafe for SQL injection
 /**
  * Convert query string parameters from http request to Elasticsearch
  * SQL WHERE conditions.
@@ -96,63 +102,35 @@ async function performSqlQuery(client: Client, query: string): Promise<any> {
   });
 }
 
-/**
- * For Education Organization and Student security, combine WHERE clauses
- * if necessary. Assumes at least one is not empty.
- */
-function fullDirectWare(edOrgDirectWhere: string, studentDirectWhere: string): string {
-  if (edOrgDirectWhere === '') return studentDirectWhere;
-  if (studentDirectWhere === '') return edOrgDirectWhere;
-  return `(${edOrgDirectWhere} OR ${studentDirectWhere})`;
-}
-
 /**
  * Entry point for querying with Elasticsearch, given entity info, the original http
  * request query parameters, and a Security object for possible filtering
  */
 export async function queryEntityList(
   entityInfo: EntityInfo,
   queryStringParameters: object,
-  security: Security,
+  paginationParameters: PaginationParameters,
   awsRequestId: string,
 ): Promise<GetResult> {
-  Logger.debug(`ElasticsearchRepository.queryEntityList security info: ${JSON.stringify(security)}`, awsRequestId);
-
   const client = await getElasticsearchClient(awsRequestId);
 
-  let directResults: any = {};
-  const whereConditionsFromQueryString = whereConditionsFrom(queryStringParameters);
   Logger.debug(`Building query`, awsRequestId);
+
+  let results: any = {};
   try {
-    // direct relationship
-    let query = `SELECT info FROM ${indexFromEntityInfo(entityInfo)} WHERE `;
-
-    // no security if no claims (for demo/testing reasons)
-    if (security.edOrgIds.length === 0 && security.studentIds.length === 0) {
-      query += whereConditionsFromQueryString;
-    } else {
-      let edOrgDirectWhere = '';
-      let studentDirectWhere = '';
-
-      // ed org direct security
-      if (security.edOrgIds.length > 0) {
-        edOrgDirectWhere = `extractedEdOrgId IN (${security.edOrgIds.map((edOrgId) => `'${edOrgId}'`).join(', ')})`;
-      }
-      // student direct security
-      if (security.studentIds.length > 0) {
-        studentDirectWhere += `extractedStudentId IN (${security.studentIds.map((edOrgId) => `'${edOrgId}'`).join(', ')})`;
-      }
-
-      const directWhere = fullDirectWare(edOrgDirectWhere, studentDirectWhere);
-      query += `${whereConditionsFromQueryString} AND ${directWhere}`;
+    let query = `SELECT info FROM ${indexFromEntityInfo(entityInfo)}`;
+    if (Object.entries(queryStringParameters).length > 0) {
+      query += ` WHERE ${whereConditionsFrom(queryStringParameters)} ORDER BY _doc`;
     }
+    if (paginationParameters.limit != null) query += ` LIMIT ${paginationParameters.limit}`;
+    if (paginationParameters.offset != null) query += ` OFFSET ${paginationParameters.offset}`;
 
     Logger.debug(`ElasticsearchRepository.queryEntityList executing query: ${query}`, awsRequestId);
 
     const { body } = await performSqlQuery(client, query);
 
     Logger.debug(`Result: ${JSON.stringify(body)}`, awsRequestId);
-    directResults = body.datarows.map((datarow) => JSON.parse(datarow));
+    results = body.datarows.map((datarow) => JSON.parse(datarow));
   } catch (e) {
     const body = JSON.parse(e.meta.body);
 
@@ -170,31 +148,5 @@ export async function queryEntityList(
     }
   }
 
-  if (security.edOrgIds.length > 0 && security.throughAssociation != null) {
-    Logger.debug(`Filtering by EdOrg`, awsRequestId);
-    let throughResults: any = {};
-    try {
-      // through relationship - student indirect security
-
-      // TODO: currently assumes Ed-Fi 3.3b for simplicity of header directives
-      const query = `SELECT entity.info FROM ${indexFromEntityInfo(
-        entityInfo,
-      )} entity JOIN ed-fi$3-3-1-b$${security.throughAssociation?.toLowerCase()} through ON entity.extractedStudentId = through.extractedStudentId WHERE through.extractedEdOrgId IN (${security.edOrgIds
-        .map((edOrgId) => `'${edOrgId}'`)
-        .join(', ')}) AND ${whereConditionsFromQueryString}`;
-
-      Logger.debug(`ElasticsearchRepository.queryEntityList executing query: ${query}`, awsRequestId);
-
-      const { body } = await (client as any).awsSql({ query });
-      throughResults = body.datarows.map((datarow) => JSON.parse(datarow));
-      Logger.debug(`ElasticsearchRepository.queryEntityList result: ${JSON.stringify(body)}`, awsRequestId);
-    } catch (e) {
-      Logger.error(`ElasticsearchRepository.queryEntityList filtering by edorg`, awsRequestId, 'n/a', e);
-      return { success: false, results: [] };
-    }
-
-    return { success: true, results: R.uniqBy((result) => result.id, [...directResults, ...throughResults]) };
-  }
-
-  return { success: true, results: directResults };
+  return { success: true, results };
 }
diff --git a/Meadowlark-js/test/handler/CrudHandler/GetResolver.test.ts b/Meadowlark-js/test/handler/CrudHandler/GetResolver.test.ts
@@ -217,45 +217,6 @@ describe('given a valid list request', () => {
     mockQuery.mockRestore();
   });
 
-  it('called list', () => {
-    expect(mockList).toHaveBeenCalled();
-  });
-
-  it('did not call getById or query', () => {
-    expect(mockGetById).not.toHaveBeenCalled();
-    expect(mockQuery).not.toHaveBeenCalled();
-  });
-});
-
-describe('given a valid query request', () => {
-  let mockGetById;
-  let mockList;
-  let mockQuery;
-
-  beforeAll(async () => {
-    mockGetById = jest.spyOn(GetResolvers, 'getById').mockImplementation((() => null) as any);
-    mockList = jest.spyOn(GetResolvers, 'list').mockImplementation((() => null) as any);
-    mockQuery = jest.spyOn(GetResolvers, 'query').mockImplementation((() => null) as any);
-
-    const event: APIGatewayProxyEvent = {
-      body: '',
-      headers: '',
-      requestContext: { requestId: 'ApiGatewayRequestId' },
-      path: '/v3.3b/ed-fi/students/',
-      queryStringParameters: { lastSurname: 'World' },
-    } as any;
-    const context = { awsRequestId: 'LambdaRequestId' } as Context;
-
-    // Act
-    await getResolver(event, context);
-  });
-
-  afterAll(() => {
-    mockGetById.mockRestore();
-    mockList.mockRestore();
-    mockQuery.mockRestore();
-  });
-
   it('called query', () => {
     expect(mockQuery).toHaveBeenCalled();
   });
@@ -266,26 +227,22 @@ describe('given a valid query request', () => {
   });
 });
 
-describe('given a valid query request with disallowed limit/offset/totalCount parameters', () => {
+describe('given a valid query request', () => {
   let mockGetById;
   let mockList;
   let mockQuery;
 
-  let sentQueryParameters;
-
   beforeAll(async () => {
     mockGetById = jest.spyOn(GetResolvers, 'getById').mockImplementation((() => null) as any);
     mockList = jest.spyOn(GetResolvers, 'list').mockImplementation((() => null) as any);
-    mockQuery = jest.spyOn(GetResolvers, 'query').mockImplementation(((_, queryParameters) => {
-      sentQueryParameters = queryParameters;
-    }) as any);
+    mockQuery = jest.spyOn(GetResolvers, 'query').mockImplementation((() => null) as any);
 
     const event: APIGatewayProxyEvent = {
       body: '',
       headers: '',
       requestContext: { requestId: 'ApiGatewayRequestId' },
       path: '/v3.3b/ed-fi/students/',
-      queryStringParameters: { lastSurname: 'World', limit: '10', offset: '11', totalCount: '100' },
+      queryStringParameters: { lastSurname: 'World' },
     } as any;
     const context = { awsRequestId: 'LambdaRequestId' } as Context;
 
@@ -299,9 +256,8 @@ describe('given a valid query request with disallowed limit/offset/totalCount pa
     mockQuery.mockRestore();
   });
 
-  it('called query without limit/offset/totalCount', () => {
+  it('called query', () => {
     expect(mockQuery).toHaveBeenCalled();
-    expect(sentQueryParameters).toEqual({ lastSurname: 'World' });
   });
 
   it('did not call getById or list', () => {

diff --git a/Meadowlark-js/test/handler/CrudHandler/Query.test.ts b/Meadowlark-js/test/handler/CrudHandler/Query.test.ts
@@ -7,18 +7,9 @@
 import { APIGatewayProxyResult, Context } from 'aws-lambda';
 
 import * as RequestValidator from '../../../src/handler/RequestValidator';
-import { Security } from '../../../src/model/Security';
 import * as ElasticsearchRepository from '../../../src/repository/ElasticsearchRepository';
 import { query } from '../../../src/handler/GetResolvers';
 
-const mockSecurity: Security = {
-  edOrgIds: [],
-  studentIds: [],
-  throughAssociation: '',
-  isOwnershipEnabled: false,
-  clientName: null,
-};
-
 describe('given the endpoint is not in the MetaEd model', () => {
   let response: APIGatewayProxyResult;
   let mockRequestValidator: any;
@@ -44,7 +35,7 @@ describe('given the endpoint is not in the MetaEd model', () => {
     );
 
     // Act
-    response = await query(pathComponents, {}, context, {} as Security);
+    response = await query(pathComponents, {}, context);
   });
 
   afterAll(() => {
@@ -92,7 +83,7 @@ describe('given persistence fails', () => {
     );
 
     // Act
-    response = await query(pathComponents, {}, context, mockSecurity);
+    response = await query(pathComponents, {}, context);
   });
 
   afterAll(() => {
@@ -143,7 +134,7 @@ describe('given successful fetch from persistence', () => {
     );
 
     // Act
-    response = await query(pathComponents, {}, context, mockSecurity);
+    response = await query(pathComponents, {}, context);
   });
 
   afterAll(() => {