Skip to content

Commit

Permalink
[RND-521] - Changes from the current SQL style query to DSL query on …
Browse files Browse the repository at this point in the history
…Opensearch. (#291)

* Changes from the current SQL style query to DSL query on Opensearch.

* Trying to fix opensearch unit tests.

* Fixing test name on Elasticsearch.

* Trying to fix e2e tests.

* Adds query casing scenarios to e2e tests.

* Adds unit tests.
  • Loading branch information
DavidJGapCR authored Aug 30, 2023
1 parent 9670c6f commit 0acfd49
Show file tree
Hide file tree
Showing 5 changed files with 229 additions and 252 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ describe('when querying for students', () => {
expect(queryResult.documents.findIndex((x: any) => x.studentUniqueId === studentUniqueIdTwo)).toBeGreaterThan(-1);
});

it('should have used the correct SQL query', async () => {
it('should have used the correct query', async () => {
expect(spyOnRequest.mock.calls.length).toBe(1);
expect(spyOnRequest.mock.calls[0].length).toBe(1);
const { body } = spyOnRequest.mock.calls[0][0];
Expand Down Expand Up @@ -222,7 +222,7 @@ describe('when querying for students', () => {
expect(queryResult.documents.findIndex((x: any) => x.studentUniqueId === studentUniqueIdTwo)).toBeGreaterThan(-1);
});

it('should have used the correct SQL query', async () => {
it('should have used the correct query', async () => {
expect(spyOnRequest.mock.calls.length).toBe(1);
expect(spyOnRequest.mock.calls[0].length).toBe(1);
const { body } = spyOnRequest.mock.calls[0][0];
Expand Down Expand Up @@ -280,7 +280,7 @@ describe('when querying for students', () => {
expect(queryResult.documents.findIndex((x: any) => x.studentUniqueId === studentUniqueIdTwo)).toBeGreaterThan(-1);
});

it('should have used the correct SQL query', async () => {
it('should have used the correct query', async () => {
expect(spyOnRequest.mock.calls.length).toBe(1);
expect(spyOnRequest.mock.calls[0].length).toBe(1);
const { body } = spyOnRequest.mock.calls[0][0];
Expand All @@ -291,6 +291,34 @@ describe('when querying for students', () => {
mock.clearAll();
});
});

describe('when querying with a case different from the case in the datastore', () => {
const authorizationStrategy: AuthorizationStrategy = { type: 'FULL_ACCESS' };
let queryResult: QueryResult;
const studentUniqueIdOne = 'one';
const studentUniqueIdTwo = 'two';
const birthCity = 'a';
const matches = [
{
match_phrase: { birthCity: birthCity.toUpperCase() },
},
];

beforeAll(async () => {
const client = setupMockRequestHappyPath([studentUniqueIdOne, studentUniqueIdTwo], matches);
const request = setupQueryRequest(authorizationStrategy, { birthCity: birthCity.toUpperCase() }, {});

queryResult = await queryDocuments(request, client);
});

it('should return the two students regardless of the casing', async () => {
expect(queryResult.documents.length).toBe(2);
});

afterAll(() => {
mock.clearAll();
});
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,13 @@ export async function handleOpenSearchError(
const responseException = err as ResponseError;
if (responseException?.message !== undefined) {
if (responseException.message !== 'Response Error') {
let startPosition = responseException?.message?.indexOf('Reason:');
const position = responseException?.message?.indexOf(' Preview');
startPosition = startPosition > -1 ? startPosition : 0;
if (position > -1) {
responseException.message = responseException?.message?.substring(startPosition, position);
} else if (startPosition !== 0) {
responseException.message = responseException?.message?.substring(startPosition);
if (responseException?.message.indexOf('index_not_found_exception') !== -1) {
Logger.warn(`${moduleName} ${documentProcessError} index not found`, traceId);
return {
response: 'QUERY_FAILURE_INVALID_QUERY',
documents: [],
failureMessage: 'IndexNotFoundException',
};
}
Logger.error(
`${moduleName} ${documentProcessError}`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,46 +22,21 @@ export function indexFromResourceInfo(resourceInfo: ResourceInfo): string {
}

/**
* Sanitizes input by escaping single all apostrophes. If \' is present, then someone is trying to evade the sanitization. In
* that case, replace the search term with an empty string.
* DSL querying
*/
function sanitize(input: string): string {
if (input.indexOf(`\\'`) > -1) {
return '';
}

return input.replaceAll(`'`, `\\'`);
}

/**
* Convert query string parameters from http request to OpenSearch
* SQL WHERE conditions. Returns empty string if there are none.
*/
function whereConditionsFrom(queryParameters: object): string {
return Object.entries(queryParameters)
.map(([field, value]) => `${field} = '${sanitize(value)}'`)
.join(' AND ');
}

/**
* This mechanism of SQL querying is specific to OpenSearch (vs Elasticsearch)
*/
async function performSqlQuery(client: Client, query: string): Promise<any> {
async function performDslQuery(client: Client, path: string, query: any, size: number, from: number): Promise<any> {
return client.transport.request({
method: 'POST',
path: '/_opendistro/_sql',
body: { query },
path: `/${path}/_search`,
body: {
from,
query,
size,
sort: [{ _doc: { order: 'asc' } }],
},
});
}

/**
* Returns well-formed WHERE clause from existing where clause and new clause
*/
function appendedWhereClause(existingWhereClause: string, newWhereClause: string): string {
if (existingWhereClause === '') return newWhereClause;
return `${existingWhereClause} AND ${newWhereClause}`;
}

/**
* Entry point for querying with OpenSearch
*/
Expand All @@ -73,35 +48,49 @@ export async function queryDocuments(request: QueryRequest, client: Client): Pro
let documents: any = [];
let recordCount: number;
try {
let query = `SELECT info FROM ${indexFromResourceInfo(resourceInfo)}`;
let whereClause: string = '';
const matches: any[] = [];

// API client requested filters
if (Object.entries(queryParameters).length > 0) {
whereClause = whereConditionsFrom(queryParameters);
Object.entries(queryParameters).forEach(([key, value]) => {
matches.push({
match_phrase: { [key]: value },
});
});
}

// Ownership-based security filter - if the resource is a descriptor we will ignore security
if (request.security.authorizationStrategy.type === 'OWNERSHIP_BASED' && !resourceInfo.isDescriptor) {
const securityWhereClause = `createdBy = '${request.security.clientId}'`;
whereClause = appendedWhereClause(whereClause, securityWhereClause);
matches.push({
match: {
createdBy: request.security.clientId,
},
});
}

if (whereClause !== '') {
query += ` WHERE ${whereClause}`;
}

query += ' ORDER BY _doc';

if (paginationParameters.limit != null) query += ` LIMIT ${paginationParameters.limit}`;
if (paginationParameters.offset != null) query += ` OFFSET ${paginationParameters.offset}`;
const query = {
bool: {
must: matches,
},
};

Logger.debug(`${moduleName}.queryDocuments queryDocuments executing query: ${query}`, traceId);

const { body } = await performSqlQuery(client, query);
recordCount = body.total;

documents = body.datarows.map((datarow) => JSON.parse(datarow));
if (isDebugEnabled()) {
Logger.debug(`${moduleName}.queryDocuments queryDocuments executing query: ${JSON.stringify(query)}`, traceId);
}
const result = await performDslQuery(
client,
indexFromResourceInfo(resourceInfo),
query,
paginationParameters.limit as number,
paginationParameters.offset as number,
);

recordCount = result.body.hits.total.value;

if (recordCount > 0) {
// eslint-disable-next-line no-underscore-dangle
documents = result.body.hits.hits.map((datarow) => JSON.parse(datarow._source.info));
}

if (isDebugEnabled()) {
const idsForLogging: string[] = documents.map((document) => document.id);
Expand Down
Loading

0 comments on commit 0acfd49

Please sign in to comment.