From 3c67d1911cd6497a54c1aaa1c2e2ce0b3fb9cd96 Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Tue, 5 Dec 2023 12:31:26 +0800 Subject: [PATCH] fix tests --- packages/api/src/resolvers/article/index.ts | 17 +- packages/api/src/services/library_item.ts | 67 +++++--- packages/api/src/utils/search.ts | 19 +-- packages/api/test/resolvers/article.test.ts | 6 +- packages/api/test/utils/search.test.ts | 168 -------------------- 5 files changed, 55 insertions(+), 222 deletions(-) delete mode 100644 packages/api/test/utils/search.test.ts diff --git a/packages/api/src/resolvers/article/index.ts b/packages/api/src/resolvers/article/index.ts index 9340d6d23..d3b598623 100644 --- a/packages/api/src/resolvers/article/index.ts +++ b/packages/api/src/resolvers/article/index.ts @@ -90,7 +90,6 @@ import { generateSlug, isParsingTimeout, libraryItemToArticle, - libraryItemToArticleSavingRequest, libraryItemToSearchItem, titleForFilePath, userDataToUser, @@ -758,6 +757,7 @@ export const updatesSinceResolver = authorized< startDate = new Date(0) } + // create a search query folder = folder || InFilter.ALL const searchQuery = parseSearchQuery( `in:${folder} updated:${startDate.toISOString()}` @@ -827,16 +827,10 @@ export const bulkActionResolver = authorized< }, }) - if (!query) { - return { errorCodes: [BulkActionErrorCode.BadRequest] } - } - - // parse query - const searchQuery = parseSearchQuery(query) - const ids = searchQuery.getValue?.('includes') as string[] - if (!ids || ids.length === 0 || ids.length > 100) { - return { errorCodes: [BulkActionErrorCode.BadRequest] } - } + // the query size is limited to 255 characters + if (!query || query.length > 255) { + return { errorCodes: [BulkActionErrorCode.BadRequest] } + } // get labels if needed let labels = undefined @@ -848,6 +842,7 @@ export const bulkActionResolver = authorized< labels = await findLabelsByIds(labelIds, uid) } + const searchQuery = parseSearchQuery(query) await updateLibraryItems(action, searchQuery, uid, labels, args) return { success: true } diff --git a/packages/api/src/services/library_item.ts b/packages/api/src/services/library_item.ts index 58d5254be..fdd646380 100644 --- a/packages/api/src/services/library_item.ts +++ b/packages/api/src/services/library_item.ts @@ -281,28 +281,37 @@ export const buildQuery = ( throw new Error('Expected a literal expression.') } - const label = expression.value?.toString()?.toLowerCase() - if (!label) { + const value = expression.value?.toString()?.toLowerCase() + if (!value) { throw new Error('Expected a value.') } - const param = `label_${parameters.length}` + const labels = value.split(',') + return ( + labels + .map((label) => { + const param = `label_${parameters.length}` - const hasWildcard = label.includes('*') - if (hasWildcard) { - return escapeQueryWithParameters( - `exists (select 1 from unnest(array_cat(library_item.label_names, library_item.highlight_labels)::text[]) as label where label ILIKE :${param})`, - { - [param]: label.replace(/\*/g, '%'), - } - ) - } + const hasWildcard = label.includes('*') + if (hasWildcard) { + return escapeQueryWithParameters( + `exists (select 1 from unnest(array_cat(library_item.label_names, library_item.highlight_labels)::text[]) as label where label ILIKE :${param})`, + { + [param]: label.replace(/\*/g, '%'), + } + ) + } - return escapeQueryWithParameters( - `:${param} = ANY(lower(array_cat(library_item.label_names, library_item.highlight_labels)::text)::text[])`, - { - [param]: label, - } + return escapeQueryWithParameters( + `:${param} = ANY(lower(array_cat(library_item.label_names, library_item.highlight_labels)::text)::text[])`, + { + [param]: label, + } + ) + }) + .join(' OR ') + // wrap in brackets to avoid precedence issues + .replace(/^(.*)$/, '($1)') ) } case 'sort': { @@ -475,9 +484,13 @@ export const buildQuery = ( } const param = `recommendedBy_${parameters.length}` + if (value === '*') { + // select all if * is provided + return "library_item.recommender_names <> '{}'" + } return escapeQueryWithParameters( - `:recommendedBy = ANY(lower(library_item.recommender_names::text)::text[])`, + `:${param} = ANY(lower(library_item.recommender_names::text)::text[])`, { [param]: value.toLowerCase(), } @@ -582,7 +595,7 @@ export const buildQuery = ( return left } - return `(${left} ${operator} ${right})` + return `${left} ${operator} ${right}` } if (ast.type === 'UnaryOperator') { @@ -885,7 +898,6 @@ export const searchLibraryItems = async ( .createQueryBuilder(LibraryItem, 'library_item') .select(selectColumns) .where('library_item.user_id = :userId', { userId }) - .orderBy(`library_item.${sortField}`, sortOrder, 'NULLS LAST') if (args.searchQuery) { const parameters: ObjectLiteral[] = [] @@ -920,7 +932,11 @@ export const searchLibraryItems = async ( queryBuilder.andWhere("library_item.state <> 'DELETED'") } - const libraryItems = await queryBuilder.skip(from).take(size).getMany() + const libraryItems = await queryBuilder + .addOrderBy(`library_item.${sortField}`, sortOrder, 'NULLS LAST') + .skip(from) + .take(size) + .getMany() const count = await queryBuilder.getCount() @@ -1286,8 +1302,13 @@ export const updateLibraryItems = async ( .createQueryBuilder(LibraryItem, 'library_item') .where('library_item.user_id = :userId', { userId }) - // build the where clause - // buildWhereClause(queryBuilder, searchQuery) + const parameters: ObjectLiteral[] = [] + const whereClause = buildQuery(searchQuery, parameters) + if (whereClause) { + queryBuilder + .andWhere(whereClause) + .setParameters(parameters.reduce((a, b) => ({ ...a, ...b }), {})) + } if (addLabels) { if (!labels) { diff --git a/packages/api/src/utils/search.ts b/packages/api/src/utils/search.ts index f6bd870ac..8c1d82669 100644 --- a/packages/api/src/utils/search.ts +++ b/packages/api/src/utils/search.ts @@ -338,23 +338,8 @@ export const parseSearchQuery = (query: string): LiqeQuery => { .replace(/\W\s":/g, '') .replace('in:subscription', 'has:subscriptions') // compatibility with old search .replace('in:library', 'no:subscription') // compatibility with old search - // const result: SearchFilter = { - // query: searchQuery, - // readFilter: ReadFilter.ALL, - // inFilter: searchQuery ? InFilter.ALL : InFilter.INBOX, - // labelFilters: [], - // hasFilters: [], - // dateFilters: [], - // termFilters: [], - // matchFilters: [], - // ids: [], - // noFilters: [], - // rangeFilters: [], - // } - - // if (!searchQuery) { - // return result - // } + // wrap the value behind colon in quotes if it's not already + .replace(/(\w+):([^"\s]+)/g, '$1:"$2"') return parse(searchQuery) diff --git a/packages/api/test/resolvers/article.test.ts b/packages/api/test/resolvers/article.test.ts index dda47a53f..17b66eff2 100644 --- a/packages/api/test/resolvers/article.test.ts +++ b/packages/api/test/resolvers/article.test.ts @@ -569,14 +569,14 @@ describe('Article API', () => { ).expect(200) // Save a link, then archive it - let allLinks = await graphqlRequest(searchQuery(''), authToken).expect( + let allLinks = await graphqlRequest(searchQuery('in:inbox'), authToken).expect( 200 ) const justSavedId = allLinks.body.data.search.edges[0].node.id await archiveLink(authToken, justSavedId) // test the negative case, ensuring the archive link wasn't returned - allLinks = await graphqlRequest(searchQuery(''), authToken).expect(200) + allLinks = await graphqlRequest(searchQuery('in:inbox'), authToken).expect(200) expect(allLinks.body.data.search.edges[0]?.node?.url).to.not.eq(url) // Now save the link again, and ensure it is returned @@ -585,7 +585,7 @@ describe('Article API', () => { authToken ).expect(200) - allLinks = await graphqlRequest(searchQuery(''), authToken).expect(200) + allLinks = await graphqlRequest(searchQuery('in:inbox'), authToken).expect(200) expect(allLinks.body.data.search.edges[0].node.id).to.eq(justSavedId) expect(allLinks.body.data.search.edges[0].node.url).to.eq(url) }) diff --git a/packages/api/test/utils/search.test.ts b/packages/api/test/utils/search.test.ts deleted file mode 100644 index 5518e84b0..000000000 --- a/packages/api/test/utils/search.test.ts +++ /dev/null @@ -1,168 +0,0 @@ -import 'mocha' -import { expect } from 'chai' -import { InFilter, parseSearchQuery, ReadFilter } from '../../src/utils/search' -import { PageType } from '../../src/generated/graphql' - -describe('undefined query', () => { - it('returns an empty result with read state ALL and no typeFilter', () => { - const result = parseSearchQuery(undefined) - expect(result.query).to.be.undefined - expect(result.readFilter).to.eq(ReadFilter.ALL) - expect(result.typeFilter).to.be.undefined - }) -}) - -describe('empty query', () => { - it('returns an empty result with read state ALL and no typefilter', () => { - const result = parseSearchQuery('') - expect(result.query).to.be.undefined - expect(result.readFilter).to.eq(ReadFilter.ALL) - expect(result.typeFilter).to.be.undefined - }) -}) - -describe('query with READ read state', () => { - it('returns a READ result', () => { - const result = parseSearchQuery('is:read') - expect(result.query).to.be.undefined - expect(result.readFilter).to.eq(ReadFilter.READ) - expect(result.typeFilter).to.be.undefined - }) -}) - -describe('query with UNREAD read state', () => { - it('returns a UNREAD result', () => { - const result = parseSearchQuery('is:unread') - expect(result.query).to.be.undefined - expect(result.readFilter).to.eq(ReadFilter.UNREAD) - expect(result.typeFilter).to.be.undefined - }) -}) - -describe('query with multiple read states', () => { - it('just uses the last one', () => { - const result = parseSearchQuery('is:unread is:read') - expect(result.query).to.be.undefined - expect(result.readFilter).to.eq(ReadFilter.READ) - expect(result.typeFilter).to.be.undefined - }) -}) - -describe('query with invalid read states', () => { - it('returns ALL', () => { - const result = parseSearchQuery('is:invalid') - expect(result.query).to.be.undefined - expect(result.readFilter).to.eq(ReadFilter.ALL) - expect(result.typeFilter).to.be.undefined - }) -}) - -describe('query with read state before search query', () => { - it('sets read state and query', () => { - const result = parseSearchQuery('is:read "machine learning"') - expect(result.query).to.eq(`"machine learning"`) - expect(result.readFilter).to.eq(ReadFilter.READ) - expect(result.typeFilter).to.be.undefined - }) -}) - -describe('query with read state after search query', () => { - it('sets read state and query', () => { - const result = parseSearchQuery('machine learning techniques is:read') - expect(result.query).to.eq(`machine learning techniques`) - expect(result.readFilter).to.eq(ReadFilter.READ) - expect(result.typeFilter).to.be.undefined - }) -}) - -describe('query with quoted text', () => { - it('sets the text as quoted and returns the default read state', () => { - const result = parseSearchQuery('"machine learning" techniques"') - expect(result.query).to.eq(`"machine learning" techniques`) - expect(result.readFilter).to.eq(ReadFilter.ALL) - expect(result.typeFilter).to.be.undefined - }) -}) - -describe('query with a file type', () => { - it('sets the type to the supplied type', () => { - const result = parseSearchQuery('"my string" type:file"') - expect(result.query).to.eq(`"my string"`) - expect(result.readFilter).to.eq(ReadFilter.ALL) - expect(result.typeFilter).to.eq(PageType.File) - }) -}) - -describe('query with an article type', () => { - it('sets the type to the supplied type', () => { - const result = parseSearchQuery('"my string" type:article"') - expect(result.query).to.eq(`"my string"`) - expect(result.readFilter).to.eq(ReadFilter.ALL) - expect(result.typeFilter).to.eq(PageType.Article) - }) -}) - -describe('query with pdf as its type', () => { - it('sets the type to the supplied file type', () => { - const result = parseSearchQuery('"my string" type:pdf"') - expect(result.query).to.eq(`"my string"`) - expect(result.readFilter).to.eq(ReadFilter.ALL) - expect(result.typeFilter).to.eq(PageType.File) - }) -}) - -describe('query without in param set', () => { - it('returns ALL if there is a search term', () => { - const result = parseSearchQuery('my search') - expect(result.query).to.eq(`my search`) - expect(result.inFilter).to.eq(InFilter.ALL) - }) - it('returns INBOX if there is not a search term', () => { - const result = parseSearchQuery('') - expect(result.inFilter).to.eq(InFilter.INBOX) - }) - it('returns INBOX if the search term is undefined', () => { - const result = parseSearchQuery(undefined) - expect(result.inFilter).to.eq(InFilter.INBOX) - }) -}) - -describe('query with in param set', () => { - it('returns set value if there is a search term', () => { - const result = parseSearchQuery('my search in:archive') - expect(result.query).to.eq(`my search`) - expect(result.inFilter).to.eq(InFilter.ARCHIVE) - }) - it('returns set value if there is not a search term', () => { - const result = parseSearchQuery('in:archive') - expect(result.inFilter).to.eq(InFilter.ARCHIVE) - }) -}) - -describe('query with in param set to invalid value', () => { - it('returns all if there is a query', () => { - const result = parseSearchQuery('my search in:blahblah') - expect(result.query).to.eq(`my search`) - expect(result.inFilter).to.eq(InFilter.ALL) - }) - it('returns set value if there is not a search term', () => { - const result = parseSearchQuery('in:blahblah') - expect(result.inFilter).to.eq(InFilter.INBOX) - }) -}) - -describe('query with author set', () => { - it('adds author to the match filters', () => { - const result = parseSearchQuery('author:"Omnivore Blog"') - expect(result.matchFilters[0].field).to.equal('author') - expect(result.matchFilters[0].value).to.equal('omnivore blog') - }) -}) - -describe('query with site set', () => { - it('adds site_name to the match filters', () => { - const result = parseSearchQuery('site:omnivore.app') - expect(result.matchFilters[0].field).to.equal('site_name') - expect(result.matchFilters[0].value).to.equal('omnivore.app') - }) -})