From 4c23bce69fc6331e977229ad9ca3c9a8f1aadc8a Mon Sep 17 00:00:00 2001 From: danny-mhlv Date: Fri, 7 Oct 2022 15:10:18 +0300 Subject: [PATCH 1/5] Fixed problems with reading 'undefined's --- .../controller/papers.controller.ts | 2 +- src/core/domain/enums/page-order.enum.ts | 2 + src/core/interceptors/page.interceptor.ts | 4 +- src/core/services/common/search.service.ts | 14 +-- src/test/search.service.spec.ts | 93 ++++--------------- 5 files changed, 30 insertions(+), 85 deletions(-) diff --git a/src/application/controller/papers.controller.ts b/src/application/controller/papers.controller.ts index bdf507a..60d0f3a 100644 --- a/src/application/controller/papers.controller.ts +++ b/src/application/controller/papers.controller.ts @@ -1,4 +1,4 @@ -import { Controller, Get, HttpCode, Param, ParseUUIDPipe, Query, Req, UseFilters, UseInterceptors } from "@nestjs/common"; +import { Controller, Get, HttpCode, Param, ParseUUIDPipe, Query, UseFilters, UseInterceptors } from "@nestjs/common"; import { SearchService } from "../../core/services/common/search.service"; import { PageInterceptor } from "../../core/interceptors/page.interceptor"; import { ApiExtraModels, ApiGatewayTimeoutResponse, ApiOperation, ApiResponse, ApiTags } from "@nestjs/swagger"; diff --git a/src/core/domain/enums/page-order.enum.ts b/src/core/domain/enums/page-order.enum.ts index 61e5176..9e724bd 100644 --- a/src/core/domain/enums/page-order.enum.ts +++ b/src/core/domain/enums/page-order.enum.ts @@ -19,6 +19,8 @@ export enum Order { * @returns Appropriate enum-member */ export function toOrder(str: string): Order { + if (!str) return Order.DESC; + switch (str) { case 'asc': return Order.ASC; case 'desc': return Order.DESC; diff --git a/src/core/interceptors/page.interceptor.ts b/src/core/interceptors/page.interceptor.ts index 8a78ded..0551733 100644 --- a/src/core/interceptors/page.interceptor.ts +++ b/src/core/interceptors/page.interceptor.ts @@ -1,5 +1,5 @@ import { HttpService } from "@nestjs/axios"; -import { CACHE_MANAGER, CallHandler, ExecutionContext, Inject, Injectable, NestInterceptor } from "@nestjs/common"; +import { BadRequestException, CACHE_MANAGER, CallHandler, ExecutionContext, Inject, Injectable, InternalServerErrorException, NestInterceptor } from "@nestjs/common"; import { Observable, map, take, switchMap, of } from "rxjs"; import { PageDto } from "../domain/dtos"; import { EsTime } from "../domain/enums/es-time.enum"; @@ -63,7 +63,7 @@ export class PageInterceptor implements NestInterceptor { }; // Check if the performed search is a backwards search - let data = res.hits.hits; + let data = res?.hits?.hits; // Omitting the redundant info and leaving only the document data = data.map((el) => el._source); // Change the order if set diff --git a/src/core/services/common/search.service.ts b/src/core/services/common/search.service.ts index b72685e..b037ab5 100644 --- a/src/core/services/common/search.service.ts +++ b/src/core/services/common/search.service.ts @@ -1,5 +1,5 @@ import { HttpService } from "@nestjs/axios"; -import { GatewayTimeoutException, Injectable, NotFoundException } from "@nestjs/common"; +import { GatewayTimeoutException, ImATeapotException, Injectable, NotFoundException } from "@nestjs/common"; import { map, take } from "rxjs"; import { EsResponseDto, SearchQueryDto} from "../../domain/dtos"; import { EsQueryDto } from "../../domain/dtos/elastic/es-query.dto"; @@ -28,7 +28,7 @@ export class SearchService { /** * Finds a paper by its own ID - * @param uuid + * @param uuid String, that represents unique identifier of a paper * @returns Elasticsearch hits or an error object */ async findByID(uuid: string): Promise { // Should I change 'object' to specific DTO? @@ -48,12 +48,12 @@ export class SearchService { })) ?.pipe(take(1), map(axiosRes => axiosRes.data)) .subscribe((res: EsResponseDto) => { - if (!res.hits.hits.length) { - reject(new NotFoundException); - } if (res.timed_out) { reject(new GatewayTimeoutException('Elasticsearch Timed Out')); } + if (!res?.hits?.hits?.length) { + reject(new NotFoundException); + } resolve(res); }); } catch (error) { @@ -65,7 +65,7 @@ export class SearchService { /** * Finds relevant documents by context using the given query string * @param query, - * @returns Elasticsearch hits or an error object + * @returns Elasticsearch response */ async findByContext(query: SearchQueryDto): Promise { // Contruct a body for querying Elasticsearch @@ -86,7 +86,7 @@ export class SearchService { headers: {'Content-Type': 'application/json'}, })) ?.pipe(take(1), map(axiosRes => axiosRes.data)) - .subscribe((res: EsResponseDto) => { + ?.subscribe((res: EsResponseDto) => { if (res.timed_out) { reject(new GatewayTimeoutException('Elasticsearch Timed Out')); } diff --git a/src/test/search.service.spec.ts b/src/test/search.service.spec.ts index 689c43c..2fa1197 100644 --- a/src/test/search.service.spec.ts +++ b/src/test/search.service.spec.ts @@ -3,7 +3,7 @@ import { GatewayTimeoutException, HttpException } from "@nestjs/common"; import { ConfigModule } from "@nestjs/config"; import { Test } from "@nestjs/testing"; import { of } from "rxjs"; -import { EsQueryDto, EsResponseDto } from "src/core/domain"; +import { SearchQueryDto } from "src/core/domain"; import { SearchService } from "src/core/services/common/search.service"; describe('Unit tests for SearchService', () => { @@ -42,26 +42,6 @@ describe('Unit tests for SearchService', () => { expect(httpGetSpy).toHaveBeenCalled(); }); - // it('Should send correct data via HttpService.get() body parameter', () => { - // let httpGetSpy = jest.spyOn(httpService, 'get'); - - // const uuid = 'thisIsUUID_Provided'; - // searchService.findByID(uuid); - // expect(httpGetSpy).toHaveBeenCalledWith<[string, object]>(expect.anything(), { - // data: { - // size: 1, - // query: { - // query_string: { - // query: 'id:' + uuid - // } - // }, - // search_after: undefined, - // sort: undefined, - // }, - // headers: { 'Content-Type': 'application/json' } - // }); - // }); - it('Should call HttpService.get() with correct URI and port number', () => { let httpGetSpy = jest.spyOn(httpService, 'get'); @@ -76,28 +56,6 @@ describe('Unit tests for SearchService', () => { expect(searchService.findByID('')).toBeInstanceOf(Promise); }); - // it('Should return a Promise with EsResponseDto', () => { - // // Axios response mock - // httpService.get = jest.fn().mockReturnValueOnce( - // of({ - // status: undefined, - // statusText: undefined, - // headers: undefined, - // config: undefined, - // data: { - // took: 1, - // timed_out: false, - // hits: { - // total: {}, - // hits: [{}] - // } - // }, - // }) - // ); - - // expect(searchService.findByID('')).resolves.toBeInstanceOf(EsResponseDto) - // }); - // Errors it('Should throw 504 | GatewayTimeoutException', () => { // Axios response mock @@ -136,25 +94,27 @@ describe('Unit tests for SearchService', () => { it('Should touch HttpService.get() method', () => { let httpGetSpy = jest.spyOn(httpService, 'get'); - searchService.findByContext(null); + searchService.findByContext({query: ""}); expect(httpGetSpy).toHaveBeenCalled(); }); it('Should send correct data via HttpService.get() body parameter', () => { let httpGetSpy = jest.spyOn(httpService, 'get'); - let es_query = new EsQueryDto(); - es_query = { - query: { - query_string: { - query: 'thisIsTheQuery!' - } - } - } + const query = new SearchQueryDto('keyword', 1, 32, 'desc'); - // searchService.findByContext(es_query); + searchService.findByContext(query); expect(httpGetSpy).toHaveBeenCalledWith<[string, object]>(expect.anything(), { - data: es_query, + data: { + query: { + query_string: { + query: 'keyword', + default_field: 'content', + } + }, + from: 32, + size: 1, + }, headers: { 'Content-Type': 'application/json' } }); }); @@ -162,7 +122,7 @@ describe('Unit tests for SearchService', () => { it('Should call HttpService.get() with correct URI and port number', () => { let httpGetSpy = jest.spyOn(httpService, 'get'); - searchService.findByContext(null); + searchService.findByContext({query: ""}); expect(httpGetSpy).toHaveBeenCalledWith<[string, object]>( `http://${process.env.ES_CONTAINER_NAME}:${process.env.ES_PORT}/_search`, expect.anything() @@ -170,26 +130,9 @@ describe('Unit tests for SearchService', () => { }); it('Should return a Promise', () => { - expect(searchService.findByContext(null)).toBeInstanceOf(Promise); + expect(searchService.findByContext({query: ""})).toBeInstanceOf(Promise); }); - // it('Should return a Promise with EsResponseDto', () => { - // // Axios response mock - // httpService.get = jest.fn().mockReturnValueOnce( - // of({ - // status: undefined, - // statusText: undefined, - // headers: undefined, - // config: undefined, - // data: { - // dummy: 'dum' - // } - // }) - // ); - - // expect(searchService.findByContext(null)).resolves.toMatchObject(null); - // }); - // Errors it('Should throw 504 | GatewayTimeoutException', () => { // Axios response mock @@ -206,7 +149,7 @@ describe('Unit tests for SearchService', () => { }) ); - searchService.findByContext(null).catch((err) => { + searchService.findByContext({query: ""}).catch((err) => { expect(err).toBeInstanceOf(GatewayTimeoutException); }); }); @@ -216,7 +159,7 @@ describe('Unit tests for SearchService', () => { throw new HttpException({ oops: 'sorry' }, 999); }); - searchService.findByContext(null).catch((err) => { + searchService.findByContext({query: ""}).catch((err) => { expect(err).toBeInstanceOf(HttpException); expect(err.response).toEqual({ oops: 'sorry' }); expect(err.status).toEqual(999); -- 2.39.5 From 3707100826307fd89d00307f5d57b14431e6fa34 Mon Sep 17 00:00:00 2001 From: danny-mhlv Date: Fri, 7 Oct 2022 15:28:32 +0300 Subject: [PATCH 2/5] Fixed relative path to HttpExceptionFilter, that failed the e2e test --- src/application/controller/papers.controller.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/application/controller/papers.controller.ts b/src/application/controller/papers.controller.ts index 60d0f3a..04c6531 100644 --- a/src/application/controller/papers.controller.ts +++ b/src/application/controller/papers.controller.ts @@ -3,7 +3,7 @@ import { SearchService } from "../../core/services/common/search.service"; import { PageInterceptor } from "../../core/interceptors/page.interceptor"; import { ApiExtraModels, ApiGatewayTimeoutResponse, ApiOperation, ApiResponse, ApiTags } from "@nestjs/swagger"; import { EsHitDto, EsResponseDto, PageDto, PaperDto, SearchQueryDto } from "../../core/domain"; -import { HttpExceptionFilter } from "src/core/filters/http-exception.filter"; +import { HttpExceptionFilter } from "../../core/filters/http-exception.filter"; /** * /papers/ route controller -- 2.39.5 From a5b175b2922fcdac659ab3de65c34bd78603d866 Mon Sep 17 00:00:00 2001 From: danny-mhlv Date: Wed, 12 Oct 2022 13:47:52 +0300 Subject: [PATCH 3/5] Fixed insufficient caching problem --- src/core/interceptors/page.interceptor.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/core/interceptors/page.interceptor.ts b/src/core/interceptors/page.interceptor.ts index 0551733..3a409b1 100644 --- a/src/core/interceptors/page.interceptor.ts +++ b/src/core/interceptors/page.interceptor.ts @@ -42,16 +42,17 @@ export class PageInterceptor implements NestInterceptor { async intercept(context: ExecutionContext, next: CallHandler): Promise> { const query = context.switchToHttp().getRequest().query; - // const offset = !query.offset ? 0 : query.offset; const offset = query.offset; - // const limit = !query.limit ? 10 : query.limit; const limit = query.limit; - // const order = !query.order ? Order.DESC : query.order; const order = query.order; + const query_string = query.query; const prev_page = await this.cacheManager.get('prev_page'); if (prev_page) { - if (offset == prev_page[1] && limit == prev_page[2] && order == prev_page[3]) return of(prev_page[0]); + if (offset == prev_page[1] && + limit == prev_page[2] && + order == prev_page[3] && + query_string === prev_page[4]) return of(prev_page[0]); } return next.handle().pipe( @@ -71,7 +72,7 @@ export class PageInterceptor implements NestInterceptor { // Cache and return the page const page: PageDto = new PageDto(data, meta); - await this.cacheManager.set('prev_page', [page, offset, limit, order]); + await this.cacheManager.set('prev_page', [page, offset, limit, order, query_string]); return page; }) ); -- 2.39.5 From f5ad2e0ff79fbf003e2363cfcd8630a1f9693a3c Mon Sep 17 00:00:00 2001 From: Danny Mikhaylov Date: Tue, 8 Nov 2022 05:18:17 +0300 Subject: [PATCH 4/5] Changed 'match' query to 'multi-match'. Allows to search for both 'content' and 'title' of the paper --- .../domain/dtos/elastic/es-multimatch.dto.ts | 68 +++++++++++++++++++ src/core/services/common/search.service.ts | 11 ++- 2 files changed, 72 insertions(+), 7 deletions(-) create mode 100644 src/core/domain/dtos/elastic/es-multimatch.dto.ts diff --git a/src/core/domain/dtos/elastic/es-multimatch.dto.ts b/src/core/domain/dtos/elastic/es-multimatch.dto.ts new file mode 100644 index 0000000..6617fea --- /dev/null +++ b/src/core/domain/dtos/elastic/es-multimatch.dto.ts @@ -0,0 +1,68 @@ +import { ApiExtraModels, ApiProperty, ApiPropertyOptional } from "@nestjs/swagger"; +import { IsArray, IsDefined, IsInt, IsObject, IsOptional } from "class-validator"; +import { EsPit } from "../../interfaces/elastic/es-pit.interface"; +import { EsQuery } from "../../interfaces/elastic/es-query.interface" + +/** + * List of allowed properties in this DTO + */ + const allowedProperties = ['query']; + + /** + * Elasticsearch multi-match query DTO + */ + @ApiExtraModels() + export class EsMultimatchQueryDto { + /** + * Offset from the start of the list of hits + */ + @IsOptional() + @IsInt() + @ApiPropertyOptional({ + description: 'Offset from the start of the list of hits', + example: 5, + }) + public from?: number; + + /** + * Maximum number of elements returned by Elasticsearch + */ + @IsOptional() + @IsInt() + @ApiPropertyOptional({ + description: 'Maximum number of elements returned by Elasticsearch', + example: 30 + }) + public size?: number; + + /** + * The search query object passed to Elasticsearch + */ + @IsDefined() + @IsObject() + @ApiProperty({ + description: 'Search query object passed to Elasticsearch', + example: { + multi_match: { + query: 'Maths', + fields: [ + 'title', + 'content' + ] + } + }, + }) + private readonly query: Object; + + /** + * Constructs a multi-match + */ + constructor(query: string = '', fields: Array = ['content']) { + this.query = { + multi_match: { + query: query, + fields: fields + } + } + } + } \ No newline at end of file diff --git a/src/core/services/common/search.service.ts b/src/core/services/common/search.service.ts index b037ab5..7e5a47c 100644 --- a/src/core/services/common/search.service.ts +++ b/src/core/services/common/search.service.ts @@ -1,6 +1,7 @@ import { HttpService } from "@nestjs/axios"; import { GatewayTimeoutException, ImATeapotException, Injectable, NotFoundException } from "@nestjs/common"; import { map, take } from "rxjs"; +import { EsMultimatchQueryDto } from "src/core/domain/dtos/elastic/es-multimatch.dto"; import { EsResponseDto, SearchQueryDto} from "../../domain/dtos"; import { EsQueryDto } from "../../domain/dtos/elastic/es-query.dto"; @@ -69,13 +70,9 @@ export class SearchService { */ async findByContext(query: SearchQueryDto): Promise { // Contruct a body for querying Elasticsearch - const es_query: EsQueryDto = new EsQueryDto(); - es_query.query = { - query_string: { - query: query.query, - default_field: 'content', - } - }; + const es_query: EsMultimatchQueryDto = new EsMultimatchQueryDto(query.query, [ + 'title', 'content' + ]); es_query.from = query.offset; es_query.size = query.limit; -- 2.39.5 From cbf7938ed738cb441822751736dced3a18850995 Mon Sep 17 00:00:00 2001 From: Danny Date: Wed, 9 Nov 2022 13:27:50 +0300 Subject: [PATCH 5/5] Fixed the test issue --- src/core/domain/dtos/elastic/es-multimatch.dto.ts | 4 +--- src/test/search.service.spec.ts | 13 ++++++++----- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/core/domain/dtos/elastic/es-multimatch.dto.ts b/src/core/domain/dtos/elastic/es-multimatch.dto.ts index 6617fea..81295d0 100644 --- a/src/core/domain/dtos/elastic/es-multimatch.dto.ts +++ b/src/core/domain/dtos/elastic/es-multimatch.dto.ts @@ -1,7 +1,5 @@ import { ApiExtraModels, ApiProperty, ApiPropertyOptional } from "@nestjs/swagger"; -import { IsArray, IsDefined, IsInt, IsObject, IsOptional } from "class-validator"; -import { EsPit } from "../../interfaces/elastic/es-pit.interface"; -import { EsQuery } from "../../interfaces/elastic/es-query.interface" +import { IsDefined, IsInt, IsObject, IsOptional } from "class-validator"; /** * List of allowed properties in this DTO diff --git a/src/test/search.service.spec.ts b/src/test/search.service.spec.ts index 2fa1197..76bc66a 100644 --- a/src/test/search.service.spec.ts +++ b/src/test/search.service.spec.ts @@ -107,13 +107,16 @@ describe('Unit tests for SearchService', () => { expect(httpGetSpy).toHaveBeenCalledWith<[string, object]>(expect.anything(), { data: { query: { - query_string: { - query: 'keyword', - default_field: 'content', + multi_match: { + query: query.query, + fields: [ + 'title', + 'content' + ] } }, - from: 32, - size: 1, + from: query.offset, + size: query.limit }, headers: { 'Content-Type': 'application/json' } }); -- 2.39.5