-
Notifications
You must be signed in to change notification settings - Fork 107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CUMULUS-3694: Update granules List endpoints to query postgres - filter by field value #3656
Changes from all commits
0772a4e
dfdf68f
4efe990
38f588d
5cc833c
883e65f
cd20bd4
08d635a
020ccb6
e3926c7
935641d
8d971e9
c1e724a
c31a9d9
421a2dd
87b3cdf
9942fac
39922be
fb9c51f
0f36350
a655b7d
facd22f
8eeb5e2
ae0e085
a68a67a
73504c4
e571757
2610483
31c1ec3
c36a7b4
a1a58f7
7ce65a7
cef467b
e6339db
e899aca
b66a580
ee593d1
c8ae699
b4e56b0
d522b24
470f354
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -288,6 +288,7 @@ test.beforeEach(async (t) => { | |
const granuleId1 = t.context.createGranuleId(); | ||
const granuleId2 = t.context.createGranuleId(); | ||
const granuleId3 = t.context.createGranuleId(); | ||
const timestamp = new Date(); | ||
|
||
// create fake Postgres granule records | ||
t.context.fakePGGranules = [ | ||
|
@@ -299,29 +300,33 @@ test.beforeEach(async (t) => { | |
cmr_link: | ||
'https://cmr.uat.earthdata.nasa.gov/search/granules.json?concept_id=A123456789-TEST_A', | ||
duration: 47.125, | ||
timestamp: new Date(Date.now()), | ||
timestamp, | ||
updated_at: timestamp, | ||
}), | ||
fakeGranuleRecordFactory({ | ||
granule_id: granuleId2, | ||
status: 'failed', | ||
collection_cumulus_id: t.context.collectionCumulusId, | ||
duration: 52.235, | ||
timestamp: new Date(Date.now()), | ||
timestamp, | ||
updated_at: timestamp, | ||
}), | ||
fakeGranuleRecordFactory({ | ||
granule_id: granuleId3, | ||
status: 'failed', | ||
collection_cumulus_id: t.context.collectionCumulusId, | ||
duration: 52.235, | ||
timestamp: new Date(Date.now()), | ||
timestamp, | ||
updated_at: timestamp, | ||
}), | ||
// granule with same granule_id as above but different collection_cumulus_id | ||
fakeGranuleRecordFactory({ | ||
granule_id: granuleId3, | ||
status: 'failed', | ||
collection_cumulus_id: t.context.collectionCumulusId2, | ||
duration: 52.235, | ||
timestamp: new Date(Date.now()), | ||
timestamp, | ||
updated_at: timestamp, | ||
}), | ||
]; | ||
|
||
|
@@ -456,7 +461,7 @@ test.serial('default lists and paginates correctly from querying database', asyn | |
const { meta, results } = response.body; | ||
t.is(results.length, 4); | ||
t.is(meta.stack, process.env.stackName); | ||
t.is(meta.table, 'granule'); | ||
t.is(meta.table, 'granules'); | ||
t.is(meta.count, 4); | ||
results.forEach((r) => { | ||
t.true(granuleIds.includes(r.granuleId)); | ||
|
@@ -487,6 +492,41 @@ test.serial('default lists and paginates correctly from querying database', asyn | |
t.not(results[0].granuleId, newResults[0].granuleId); | ||
}); | ||
|
||
test.serial('LIST endpoint returns search result correctly', async (t) => { | ||
const granuleIds = t.context.fakePGGranules.map((i) => i.granule_id); | ||
const searchParams = new URLSearchParams({ | ||
granuleId: granuleIds[3], | ||
}); | ||
const response = await request(app) | ||
.get(`/granules?limit=1&page=2&${searchParams}`) | ||
.set('Accept', 'application/json') | ||
.set('Authorization', `Bearer ${jwtAuthToken}`) | ||
.expect(200); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't really understand this test, when you add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are 2 granules with same granule Id but different collections in the setup There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ohhhhhh ok, I didn't know it could work like that, thanks 👍 |
||
const { meta, results } = response.body; | ||
t.is(meta.count, 2); | ||
t.is(results.length, 1); | ||
t.true([granuleIds[2], granuleIds[3]].includes(results[0].granuleId)); | ||
|
||
const newSearchParams = new URLSearchParams({ | ||
collectionId: t.context.collectionId, | ||
status: 'failed', | ||
duration: 52.235, | ||
timestamp: t.context.fakePGGranules[0].timestamp.getTime(), | ||
}); | ||
const newResponse = await request(app) | ||
.get(`/granules?${newSearchParams}`) | ||
.set('Accept', 'application/json') | ||
.set('Authorization', `Bearer ${jwtAuthToken}`) | ||
.expect(200); | ||
|
||
const { meta: newMeta, results: newResults } = newResponse.body; | ||
t.is(newMeta.count, 2); | ||
t.is(newResults.length, 2); | ||
const newResultIds = newResults.map((g) => g.granuleId); | ||
t.deepEqual([granuleIds[1], granuleIds[2]].sort(), newResultIds.sort()); | ||
}); | ||
|
||
test.serial('CUMULUS-911 GET without pathParameters and without an Authorization header returns an Authorization Missing response', async (t) => { | ||
const response = await request(app) | ||
.get('/granules') | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,11 @@ | ||
import { Knex } from 'knex'; | ||
import Logger from '@cumulus/logger'; | ||
import { getKnexClient } from '../connection'; | ||
|
||
import { BaseRecord } from '../types/base'; | ||
import { getKnexClient } from '../connection'; | ||
import { TableNames } from '../tables'; | ||
import { DbQueryParameters, QueryEvent, QueryStringParameters } from '../types/search'; | ||
import { convertQueryStringToDbQueryParameters } from './queries'; | ||
|
||
const log = new Logger({ sender: '@cumulus/db/BaseSearch' }); | ||
|
||
|
@@ -15,32 +18,35 @@ export type Meta = { | |
count?: number, | ||
}; | ||
|
||
const typeToTable: { [key: string]: string } = { | ||
asyncOperation: TableNames.asyncOperations, | ||
collection: TableNames.collections, | ||
execution: TableNames.executions, | ||
granule: TableNames.granules, | ||
pdr: TableNames.pdrs, | ||
provider: TableNames.providers, | ||
rule: TableNames.rules, | ||
}; | ||
|
||
/** | ||
* Class to build and execute db search query | ||
*/ | ||
class BaseSearch { | ||
readonly type?: string; | ||
readonly type: string; | ||
readonly queryStringParameters: QueryStringParameters; | ||
// parsed from queryStringParameters for query build | ||
dbQueryParameters: DbQueryParameters = {}; | ||
|
||
constructor(event: QueryEvent, type?: string) { | ||
constructor(event: QueryEvent, type: string) { | ||
this.type = type; | ||
this.queryStringParameters = event?.queryStringParameters ?? {}; | ||
this.dbQueryParameters.page = Number.parseInt( | ||
(this.queryStringParameters.page) ?? '1', | ||
10 | ||
); | ||
this.dbQueryParameters.limit = Number.parseInt( | ||
(this.queryStringParameters.limit) ?? '10', | ||
10 | ||
this.dbQueryParameters = convertQueryStringToDbQueryParameters( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
this.type, this.queryStringParameters | ||
); | ||
this.dbQueryParameters.offset = (this.dbQueryParameters.page - 1) | ||
* this.dbQueryParameters.limit; | ||
} | ||
|
||
/** | ||
* build the search query | ||
* Build the search query | ||
* | ||
* @param knex - DB client | ||
* @returns queries for getting count and search result | ||
|
@@ -51,27 +57,32 @@ class BaseSearch { | |
searchQuery: Knex.QueryBuilder, | ||
} { | ||
const { countQuery, searchQuery } = this.buildBasicQuery(knex); | ||
if (this.dbQueryParameters.limit) searchQuery.limit(this.dbQueryParameters.limit); | ||
if (this.dbQueryParameters.offset) searchQuery.offset(this.dbQueryParameters.offset); | ||
this.buildTermQuery({ countQuery, searchQuery }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so for here, you need to specify 2 different types of queries in case the query might be a term one or an infix one? I don't quite understand how that works, or do both queries (count and search) need to be executed and parsed for the api response There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unlike es, the search query doesn't return count, and only returns limited number of records, so we need a separate query to get the total count which satisfy the search parameters. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A search can have infix, term and other supported query types (sort, range etc.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yea I understood this after looking at your GranuleSearch class, it was kinda hard to understand how you did it based on my initial understanding of the stats endpoint (since stats mainly does count, aggregation, avg, not actually returning records), I understand now the meta thing needs a count and then the actual body is the granules records actually returned 👍 |
||
this.buildInfixPrefixQuery({ countQuery, searchQuery }); | ||
|
||
const { limit, offset } = this.dbQueryParameters; | ||
if (limit) searchQuery.limit(limit); | ||
if (offset) searchQuery.offset(offset); | ||
|
||
log.debug(`_buildSearch returns countQuery: ${countQuery.toSQL().sql}, searchQuery: ${searchQuery.toSQL().sql}`); | ||
return { countQuery, searchQuery }; | ||
} | ||
|
||
/** | ||
* metadata template for query result | ||
* Get metadata template for query result | ||
* | ||
* @returns metadata template | ||
*/ | ||
private _metaTemplate(): Meta { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 nice, this is something I can use |
||
return { | ||
name: 'cumulus-api', | ||
stack: process.env.stackName, | ||
table: this.type, | ||
table: this.type && typeToTable[this.type], | ||
}; | ||
} | ||
|
||
/** | ||
* build basic query | ||
* Build basic query | ||
* | ||
* @param knex - DB client | ||
* @throws - function is not implemented | ||
|
@@ -84,6 +95,46 @@ class BaseSearch { | |
throw new Error('buildBasicQuery is not implemented'); | ||
} | ||
|
||
/** | ||
* Build queries for infix and prefix | ||
* | ||
* @param params | ||
* @param params.countQuery - query builder for getting count | ||
* @param params.searchQuery - query builder for search | ||
* @param [params.dbQueryParameters] - db query parameters | ||
*/ | ||
protected buildInfixPrefixQuery(params: { | ||
countQuery: Knex.QueryBuilder, | ||
searchQuery: Knex.QueryBuilder, | ||
dbQueryParameters?: DbQueryParameters, | ||
}) { | ||
log.debug(`buildInfixPrefixQuery is not implemented ${Object.keys(params)}`); | ||
throw new Error('buildInfixPrefixQuery is not implemented'); | ||
} | ||
|
||
/** | ||
* Build queries for term fields | ||
* | ||
* @param params | ||
* @param params.countQuery - query builder for getting count | ||
* @param params.searchQuery - query builder for search | ||
* @param [params.dbQueryParameters] - db query parameters | ||
*/ | ||
protected buildTermQuery(params: { | ||
countQuery: Knex.QueryBuilder, | ||
searchQuery: Knex.QueryBuilder, | ||
dbQueryParameters?: DbQueryParameters, | ||
}) { | ||
const table = typeToTable[this.type]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so this is for what we were talking about before, where the query can search by ANY field in the table and query by it based on an exact param (i.e. published = true) |
||
const { countQuery, searchQuery, dbQueryParameters } = params; | ||
const { term = {} } = dbQueryParameters || this.dbQueryParameters; | ||
|
||
Object.entries(term).forEach(([name, value]) => { | ||
countQuery.where(`${table}.${name}`, value); | ||
searchQuery.where(`${table}.${name}`, value); | ||
}); | ||
} | ||
|
||
/** | ||
* Translate postgres records to api records | ||
* | ||
|
@@ -96,7 +147,7 @@ class BaseSearch { | |
} | ||
|
||
/** | ||
* build and execute search query | ||
* Build and execute search query | ||
* | ||
* @param testKnex - knex for testing | ||
* @returns search result | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍