From c6acbfcc30479b7dc29accbac0010ce64fcb088f Mon Sep 17 00:00:00 2001 From: Nestor Carvantes Date: Wed, 21 Apr 2021 00:32:37 -0700 Subject: [PATCH 1/4] feat: allow sorting by date type parameters --- src/QueryBuilder/index.ts | 2 + src/QueryBuilder/sort.test.ts | 68 +++++++++++++++++++ src/QueryBuilder/sort.ts | 63 +++++++++++++++++ .../elasticSearchService.test.ts.snap | 10 ++- src/constants.ts | 2 + src/elasticSearchService.test.ts | 2 +- src/elasticSearchService.ts | 20 +++++- 7 files changed, 162 insertions(+), 5 deletions(-) create mode 100644 src/QueryBuilder/sort.test.ts create mode 100644 src/QueryBuilder/sort.ts diff --git a/src/QueryBuilder/index.ts b/src/QueryBuilder/index.ts index 4cf91e7..d3c3248 100644 --- a/src/QueryBuilder/index.ts +++ b/src/QueryBuilder/index.ts @@ -111,3 +111,5 @@ export const buildQueryForAllSearchParameters = ( }, }; }; + +export { buildSortClause } from './sort'; diff --git a/src/QueryBuilder/sort.test.ts b/src/QueryBuilder/sort.test.ts new file mode 100644 index 0000000..73fd250 --- /dev/null +++ b/src/QueryBuilder/sort.test.ts @@ -0,0 +1,68 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +import { InvalidSearchParameterError } from 'fhir-works-on-aws-interface'; +import { buildSortClause, parseSortParameter } from './sort'; +import { FHIRSearchParametersRegistry } from '../FHIRSearchParametersRegistry'; + +const fhirSearchParametersRegistry = new FHIRSearchParametersRegistry('4.0.1'); + +describe('parseSortParameter', () => { + test('status,-date,category', () => { + expect(parseSortParameter('status,-date,category')).toMatchInlineSnapshot(` + Array [ + Object { + "order": "asc", + "searchParam": "status", + }, + Object { + "order": "desc", + "searchParam": "date", + }, + Object { + "order": "asc", + "searchParam": "category", + }, + ] + `); + }); +}); + +describe('buildSortClause', () => { + test('valid date params', () => { + expect(buildSortClause(fhirSearchParametersRegistry, 'Patient', '-_lastUpdated,birthdate')) + .toMatchInlineSnapshot(` + Array [ + Object { + "meta.lastUpdated": Object { + "order": "desc", + "unmapped_type": "long", + }, + }, + Object { + "birthDate": Object { + "order": "asc", + "unmapped_type": "long", + }, + }, + ] + `); + }); + + test('invalid params', () => { + [ + 'notAPatientParam', + '_lastUpdated,notAPatientParam', + '+birthdate', + '#$%/., symbols and stuff', + 'valid params must match a param name from fhirSearchParametersRegistry, so most strings are invalid...', + 'name', // This is actually a valid param but right now we only allow sorting by date params + ].forEach(p => + expect(() => buildSortClause(fhirSearchParametersRegistry, 'Patient', p)).toThrow( + InvalidSearchParameterError, + ), + ); + }); +}); diff --git a/src/QueryBuilder/sort.ts b/src/QueryBuilder/sort.ts new file mode 100644 index 0000000..6ddaf0d --- /dev/null +++ b/src/QueryBuilder/sort.ts @@ -0,0 +1,63 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + * + */ + +import { InvalidSearchParameterError } from 'fhir-works-on-aws-interface'; +import { FHIRSearchParametersRegistry } from '../FHIRSearchParametersRegistry'; + +interface SortParameter { + order: 'asc' | 'desc'; + searchParam: string; +} + +export const parseSortParameter = (param: string): SortParameter[] => { + const parts = param.split(','); + return parts.map(s => { + const order = s.startsWith('-') ? 'desc' : 'asc'; + return { + order, + searchParam: s.replace(/^-/, ''), + }; + }); +}; + +// eslint-disable-next-line import/prefer-default-export +export const buildSortClause = ( + fhirSearchParametersRegistry: FHIRSearchParametersRegistry, + resourceType: string, + sortQueryParam: string | string[], + // request: TypeSearchRequest, +): any => { + // const sortQueryParam = request.queryParams[SORT_PARAMETER]; + + if (Array.isArray(sortQueryParam)) { + throw new InvalidSearchParameterError('_sort parameter cannot be used multiple times on a search query'); + } + const sortParams = parseSortParameter(sortQueryParam); + + return sortParams.flatMap(sortParam => { + const searchParameter = fhirSearchParametersRegistry.getSearchParameter(resourceType, sortParam.searchParam); + if (searchParameter === undefined) { + throw new InvalidSearchParameterError( + `Unknown _sort parameter value: ${sortParam.searchParam}. Sort parameters values must use a valid Search Parameter`, + ); + } + if (searchParameter.type !== 'date') { + throw new InvalidSearchParameterError( + `Invalid _sort parameter: ${sortParam.searchParam}. Only date type parameters can be used for sorting`, + ); + } + return searchParameter.compiled.map(compiledParam => { + return { + [compiledParam.path]: { + order: sortParam.order, + // unmapped_type makes queries more fault tolerant. Since we are using dynamic mapping there's no guarantee + // that the mapping exists at query time. This ignores the unmapped field instead of failing + unmapped_type: 'long', + }, + }; + }); + }); +}; diff --git a/src/__snapshots__/elasticSearchService.test.ts.snap b/src/__snapshots__/elasticSearchService.test.ts.snap index 6cb1fb8..c1b3d42 100644 --- a/src/__snapshots__/elasticSearchService.test.ts.snap +++ b/src/__snapshots__/elasticSearchService.test.ts.snap @@ -1466,7 +1466,7 @@ Array [ ] `; -exports[`typeSearch query snapshots for simple queryParams; with ACTIVE filter queryParams={"_count":10,"_getpagesoffset":2} 1`] = ` +exports[`typeSearch query snapshots for simple queryParams; with ACTIVE filter queryParams={"_count":10,"_getpagesoffset":2,"_sort":"_lastUpdated"} 1`] = ` Array [ Array [ Object { @@ -1483,6 +1483,14 @@ Array [ "must": Array [], }, }, + "sort": Array [ + Object { + "meta.lastUpdated": Object { + "order": "asc", + "unmapped_type": "long", + }, + }, + ], }, "from": 2, "index": "patient", diff --git a/src/constants.ts b/src/constants.ts index c7fcd1c..2e9532b 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -12,7 +12,9 @@ export const enum SEARCH_PAGINATION_PARAMS { export const SEPARATOR: string = '_'; export const ITERATIVE_INCLUSION_PARAMETERS = ['_include:iterate', '_revinclude:iterate']; +export const SORT_PARAMETER = '_sort'; export const NON_SEARCHABLE_PARAMETERS = [ + SORT_PARAMETER, SEARCH_PAGINATION_PARAMS.PAGES_OFFSET, SEARCH_PAGINATION_PARAMS.COUNT, '_format', diff --git a/src/elasticSearchService.test.ts b/src/elasticSearchService.test.ts index f9324bf..0d8e4a8 100644 --- a/src/elasticSearchService.test.ts +++ b/src/elasticSearchService.test.ts @@ -40,7 +40,7 @@ describe('typeSearch', () => { describe('query snapshots for simple queryParams; with ACTIVE filter', () => { each([ [{}], - [{ _count: 10, _getpagesoffset: 2 }], + [{ _count: 10, _getpagesoffset: 2, _sort: '_lastUpdated' }], [{ gender: 'female', name: 'Emily' }], [{ gender: 'female', birthdate: 'gt1990' }], [{ gender: 'female', identifier: 'http://acme.org/patient|2345' }], diff --git a/src/elasticSearchService.ts b/src/elasticSearchService.ts index 6df3b83..e8997e0 100644 --- a/src/elasticSearchService.ts +++ b/src/elasticSearchService.ts @@ -19,10 +19,15 @@ import { FhirVersion, } from 'fhir-works-on-aws-interface'; import { ElasticSearch } from './elasticSearch'; -import { DEFAULT_SEARCH_RESULTS_PER_PAGE, SEARCH_PAGINATION_PARAMS, ITERATIVE_INCLUSION_PARAMETERS } from './constants'; +import { + DEFAULT_SEARCH_RESULTS_PER_PAGE, + SEARCH_PAGINATION_PARAMS, + ITERATIVE_INCLUSION_PARAMETERS, + SORT_PARAMETER, +} from './constants'; import { buildIncludeQueries, buildRevIncludeQueries } from './searchInclusions'; import { FHIRSearchParametersRegistry } from './FHIRSearchParametersRegistry'; -import { buildQueryForAllSearchParameters } from './QueryBuilder'; +import { buildQueryForAllSearchParameters, buildSortClause } from './QueryBuilder'; const MAX_INCLUDE_ITERATIVE_DEPTH = 5; @@ -84,7 +89,7 @@ export class ElasticSearchService implements Search { ]); const query = buildQueryForAllSearchParameters(this.fhirSearchParametersRegistry, request, filter); - const params = { + const params: any = { index: resourceType.toLowerCase(), from, size, @@ -92,6 +97,15 @@ export class ElasticSearchService implements Search { query, }, }; + + if (request.queryParams[SORT_PARAMETER]) { + params.body.sort = buildSortClause( + this.fhirSearchParametersRegistry, + resourceType, + request.queryParams[SORT_PARAMETER], + ); + } + const { total, hits } = await this.executeQuery(params); const result: SearchResult = { numberOfResults: total, From 0d0a34656451a15960183ac17abc72e4217e54f7 Mon Sep 17 00:00:00 2001 From: Nestor Carvantes Date: Wed, 21 Apr 2021 00:45:36 -0700 Subject: [PATCH 2/4] remove extra comments --- src/QueryBuilder/sort.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/QueryBuilder/sort.ts b/src/QueryBuilder/sort.ts index 6ddaf0d..79e3454 100644 --- a/src/QueryBuilder/sort.ts +++ b/src/QueryBuilder/sort.ts @@ -28,10 +28,7 @@ export const buildSortClause = ( fhirSearchParametersRegistry: FHIRSearchParametersRegistry, resourceType: string, sortQueryParam: string | string[], - // request: TypeSearchRequest, ): any => { - // const sortQueryParam = request.queryParams[SORT_PARAMETER]; - if (Array.isArray(sortQueryParam)) { throw new InvalidSearchParameterError('_sort parameter cannot be used multiple times on a search query'); } From c4b12b88df07fe9df69e1d47854ffbe5e492f243 Mon Sep 17 00:00:00 2001 From: Nestor Carvantes Date: Wed, 21 Apr 2021 22:35:36 -0700 Subject: [PATCH 3/4] take int account fields of type Period --- src/QueryBuilder/sort.test.ts | 12 +++++++ src/QueryBuilder/sort.ts | 32 +++++++++++++------ .../elasticSearchService.test.ts.snap | 6 ++++ 3 files changed, 40 insertions(+), 10 deletions(-) diff --git a/src/QueryBuilder/sort.test.ts b/src/QueryBuilder/sort.test.ts index 73fd250..79bcf73 100644 --- a/src/QueryBuilder/sort.test.ts +++ b/src/QueryBuilder/sort.test.ts @@ -41,12 +41,24 @@ describe('buildSortClause', () => { "unmapped_type": "long", }, }, + Object { + "meta.lastUpdated.end": Object { + "order": "desc", + "unmapped_type": "long", + }, + }, Object { "birthDate": Object { "order": "asc", "unmapped_type": "long", }, }, + Object { + "birthDate.start": Object { + "order": "asc", + "unmapped_type": "long", + }, + }, ] `); }); diff --git a/src/QueryBuilder/sort.ts b/src/QueryBuilder/sort.ts index 79e3454..22b6433 100644 --- a/src/QueryBuilder/sort.ts +++ b/src/QueryBuilder/sort.ts @@ -23,12 +23,21 @@ export const parseSortParameter = (param: string): SortParameter[] => { }); }; +const elasticsearchSort = (field: string, order: 'asc' | 'desc') => ({ + [field]: { + order, + // unmapped_type makes queries more fault tolerant. Since we are using dynamic mapping there's no guarantee + // that the mapping exists at query time. This ignores the unmapped field instead of failing + unmapped_type: 'long', + }, +}); + // eslint-disable-next-line import/prefer-default-export export const buildSortClause = ( fhirSearchParametersRegistry: FHIRSearchParametersRegistry, resourceType: string, sortQueryParam: string | string[], -): any => { +): any[] => { if (Array.isArray(sortQueryParam)) { throw new InvalidSearchParameterError('_sort parameter cannot be used multiple times on a search query'); } @@ -46,15 +55,18 @@ export const buildSortClause = ( `Invalid _sort parameter: ${sortParam.searchParam}. Only date type parameters can be used for sorting`, ); } - return searchParameter.compiled.map(compiledParam => { - return { - [compiledParam.path]: { - order: sortParam.order, - // unmapped_type makes queries more fault tolerant. Since we are using dynamic mapping there's no guarantee - // that the mapping exists at query time. This ignores the unmapped field instead of failing - unmapped_type: 'long', - }, - }; + return searchParameter.compiled.flatMap(compiledParam => { + return [ + elasticsearchSort(compiledParam.path, sortParam.order), + + // Date search params may target fields of type Period, so we add a sort clause for them. + // The FHIR spec does not fully specify how to sort by Period, but it makes sense that the most recent + // record is the one with the most recent "end" date and vice versa. + elasticsearchSort( + sortParam.order === 'desc' ? `${compiledParam.path}.end` : `${compiledParam.path}.start`, + sortParam.order, + ), + ]; }); }); }; diff --git a/src/__snapshots__/elasticSearchService.test.ts.snap b/src/__snapshots__/elasticSearchService.test.ts.snap index c1b3d42..394d538 100644 --- a/src/__snapshots__/elasticSearchService.test.ts.snap +++ b/src/__snapshots__/elasticSearchService.test.ts.snap @@ -1490,6 +1490,12 @@ Array [ "unmapped_type": "long", }, }, + Object { + "meta.lastUpdated.start": Object { + "order": "asc", + "unmapped_type": "long", + }, + }, ], }, "from": 2, From 6428d80989610625e525d7e7ef1f8611c87bdf56 Mon Sep 17 00:00:00 2001 From: Nestor Carvantes Date: Thu, 22 Apr 2021 13:42:32 -0700 Subject: [PATCH 4/4] update error message --- src/QueryBuilder/sort.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/QueryBuilder/sort.ts b/src/QueryBuilder/sort.ts index 22b6433..fdf4912 100644 --- a/src/QueryBuilder/sort.ts +++ b/src/QueryBuilder/sort.ts @@ -52,7 +52,7 @@ export const buildSortClause = ( } if (searchParameter.type !== 'date') { throw new InvalidSearchParameterError( - `Invalid _sort parameter: ${sortParam.searchParam}. Only date type parameters can be used for sorting`, + `Invalid _sort parameter: ${sortParam.searchParam}. Only date type parameters can currently be used for sorting`, ); } return searchParameter.compiled.flatMap(compiledParam => {