Skip to content
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-3641 - Update Collections LIST endpoint to query Postgres basic #3681

Merged
merged 27 commits into from
Jun 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
993d72a
Merge branch 'feature/es-phase-1' of https://github.com/nasa/cumulus …
Nnaga1 May 20, 2024
65dfe5e
Merge branch 'feature/es-phase-1' of https://github.com/nasa/cumulus …
Nnaga1 May 23, 2024
e4c1e59
Merge branch 'feature/es-phase-1' of https://github.com/nasa/cumulus …
Nnaga1 May 25, 2024
d7dd6cf
Merge branch 'feature/es-phase-1' of https://github.com/nasa/cumulus …
Nnaga1 May 31, 2024
b557e9e
Merge branch 'feature/es-phase-1' of https://github.com/nasa/cumulus …
Nnaga1 Jun 4, 2024
c89f0f9
reopening PR
Nnaga1 Jun 4, 2024
f19244c
PR feedback
Nnaga1 Jun 5, 2024
43b1cd7
small test fix
Nnaga1 Jun 5, 2024
f386137
small PR feedbacks
Nnaga1 Jun 5, 2024
be810af
adding new tests from match queries
Nnaga1 Jun 5, 2024
682660b
PR feedback/formatting
Nnaga1 Jun 6, 2024
cea3c14
temporary reversion to list endpoint for reconreport tests
Nnaga1 Jun 6, 2024
1e6d54e
reverting changes
Nnaga1 Jun 7, 2024
aaec2a2
adding logging
Nnaga1 Jun 7, 2024
2ee8efd
more logging
Nnaga1 Jun 7, 2024
ca6b106
more logging
Nnaga1 Jun 8, 2024
0d9f057
removing logging + commenting reconrep test temp
Nnaga1 Jun 8, 2024
d0ae579
commenting out failing createReconReport spec
Nnaga1 Jun 10, 2024
b282642
removing comment
Nnaga1 Jun 10, 2024
3ca493d
reverting changes to reconReport test
Nnaga1 Jun 10, 2024
52281e8
reverting previous change
Nnaga1 Jun 10, 2024
db45ecd
adding ts-check
Nnaga1 Jun 11, 2024
9f613ba
PR feedback
Nnaga1 Jun 12, 2024
bc40d57
PR feedback
Nnaga1 Jun 12, 2024
6aeb39f
adding in test
Nnaga1 Jun 12, 2024
e208d5e
PR feedback fix
Nnaga1 Jun 12, 2024
5433023
PR feedback
Nnaga1 Jun 13, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).

### Replace ElasticSearch Phase 1

- **CUMULUS-3641**
- Updated `collections` api endpoint to query postgres instead of elasticsearch except if `includeStats` is in the query parameters
- **CUMULUS-3695**
- Updated `granule` list api endpoint and BaseSearch class to handle sort fields
- **CUMULUS-3688**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ const waitForCollectionRecordsInList = async (stackName, collectionIds, addition
async () => {
// Verify the collection is returned when listing collections
const collsResp = await getCollections({ prefix: stackName,
query: { _id__in: collectionIds.join(','), ...additionalQueryParams, limit: 30 } });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add support for _id__in for collection db query.

query: { _id__in: collectionIds.join(','), ...additionalQueryParams, includeStats: true, limit: 30 } });
const results = get(JSON.parse(collsResp.body), 'results', []);
const ids = results.map((c) => constructCollectionId(c.name, c.version));
return isEqual(ids.sort(), collectionIds.sort());
Expand Down
25 changes: 18 additions & 7 deletions packages/api/endpoints/collections.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
//@ts-check

'use strict';

const router = require('express-promise-router')();
Expand All @@ -16,6 +18,7 @@ const {
isCollisionError,
translateApiCollectionToPostgresCollection,
translatePostgresCollectionToApiCollection,
CollectionSearch,
} = require('@cumulus/db');
const CollectionConfigStore = require('@cumulus/collection-config-store');
const { getEsClient, Search } = require('@cumulus/es-client/search');
Expand Down Expand Up @@ -43,14 +46,22 @@ const log = new Logger({ sender: '@cumulus/api/collections' });
* @returns {Promise<Object>} the promise of express response object
*/
async function list(req, res) {
log.trace(`list query ${JSON.stringify(req.query)}`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: we almost never use trace over .info. A decision to start doing that should be deliberate

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jenny added it to granules during the granule endpoint work: https://github.com/nasa/cumulus/pull/3648/files#diff-624123a7dc5bb5513e3769b27fe85821577087609a9376829676b7edbeb3a5aeR104 so we were trying to be consistent

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sure, that comment applies to both PRs then - this is in theory a change to how we're logging. We should broadcast (arch meting) and decide to do it, else efforts outside this insular work won't be consistent with the approach you're choosing.

It's minor and shouldn't block the PR (nit), but it's notable, particularly if we ever get around to log-level squelching.

@jennyhliu

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add a comment to the feature branch, so we can revisit when Jenny is back and when we are thinking of merging the feature branch.

const { getMMT, includeStats, ...queryStringParameters } = req.query;
const collection = new Collection(
{ queryStringParameters },
undefined,
process.env.ES_INDEX,
includeStats === 'true'
);
let result = await collection.query();
let dbSearch;
if (includeStats === 'true') {
dbSearch = new Collection(
{ queryStringParameters },
undefined,
process.env.ES_INDEX,
includeStats === 'true'
);
} else {
dbSearch = new CollectionSearch(
{ queryStringParameters }
);
}
let result = await dbSearch.query();
if (getMMT === 'true') {
result = await insertMMTLinks(result);
}
Expand Down
8 changes: 4 additions & 4 deletions packages/api/tests/app/test-launchpadAuth.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const { createBucket, putJsonS3Object } = require('@cumulus/aws-client/S3');
const launchpad = require('@cumulus/launchpad-auth');
const { randomId } = require('@cumulus/common/test-utils');

const EsCollection = require('@cumulus/es-client/collections');
const { CollectionSearch } = require('@cumulus/db');
const models = require('../../models');
const { createJwtToken } = require('../../lib/token');
const { fakeAccessTokenFactory } = require('../../lib/testUtils');
Expand Down Expand Up @@ -72,7 +72,7 @@ test.after.always(async () => {

test.serial('API request with a valid Launchpad token stores the access token', async (t) => {
const stub = sinon.stub(launchpad, 'validateLaunchpadToken').returns(validateTokenResponse);
const collectionStub = sinon.stub(EsCollection.prototype, 'query').returns([]);
const collectionStub = sinon.stub(CollectionSearch.prototype, 'query').returns([]);

try {
await request(app)
Expand Down Expand Up @@ -113,7 +113,7 @@ test.serial('API request with an invalid Launchpad token returns a 403 unauthori

test.serial('API request with a stored non-expired Launchpad token record returns a successful response', async (t) => {
let stub = sinon.stub(launchpad, 'validateLaunchpadToken').resolves(validateTokenResponse);
const collectionStub = sinon.stub(EsCollection.prototype, 'query').returns([]);
const collectionStub = sinon.stub(CollectionSearch.prototype, 'query').returns([]);

try {
await request(app)
Expand Down Expand Up @@ -143,7 +143,7 @@ test.serial('API request with a stored non-expired Launchpad token record return
});

test.serial('API request with an expired Launchpad token returns a 401 response', async (t) => {
const collectionStub = sinon.stub(EsCollection.prototype, 'query').returns([]);
const collectionStub = sinon.stub(CollectionSearch.prototype, 'query').returns([]);

try {
await accessTokenModel.create({
Expand Down
68 changes: 57 additions & 11 deletions packages/api/tests/endpoints/collections/list-collections.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
const test = require('ava');
const request = require('supertest');
const sinon = require('sinon');
const range = require('lodash/range');
const awsServices = require('@cumulus/aws-client/services');
const {
recursivelyDeleteS3Bucket,
Expand All @@ -11,6 +12,7 @@ const { randomString } = require('@cumulus/common/test-utils');
const { bootstrapElasticSearch } = require('@cumulus/es-client/bootstrap');
const EsCollection = require('@cumulus/es-client/collections');
const { getEsClient } = require('@cumulus/es-client/search');
const { randomId } = require('@cumulus/common/test-utils');

const models = require('../../../models');
const {
Expand All @@ -20,10 +22,25 @@ const {
} = require('../../../lib/testUtils');
const assertions = require('../../../lib/assertions');

const testDbName = randomId('collection');

const {
destroyLocalTestDb,
generateLocalTestDb,
CollectionPgModel,
fakeCollectionRecordFactory,
migrationDir,
localStackConnectionEnv,
} = require('../../../../db/dist');

process.env.PG_HOST = randomId('hostname');
process.env.PG_USER = randomId('user');
process.env.PG_PASSWORD = randomId('password');
process.env.TOKEN_SECRET = randomString();

process.env.AccessTokensTable = randomString();
process.env.stackName = randomString();
process.env.system_bucket = randomString();
process.env.TOKEN_SECRET = randomString();

// import the express app after setting the env variables
const { app } = require('../../../app');
Expand All @@ -34,7 +51,13 @@ let esClient;
let jwtAuthToken;
let accessTokenModel;

test.before(async () => {
process.env = {
...process.env,
...localStackConnectionEnv,
PG_DATABASE: testDbName,
};

test.before(async (t) => {
const esAlias = randomString();
process.env.ES_INDEX = esAlias;
await bootstrapElasticSearch({
Expand All @@ -52,16 +75,45 @@ test.before(async () => {

jwtAuthToken = await createFakeJwtAuthToken({ accessTokenModel, username });
esClient = await getEsClient('fakehost');
const { knexAdmin, knex } = await generateLocalTestDb(
testDbName,
migrationDir
);

t.context.knexAdmin = knexAdmin;
t.context.knex = knex;

t.context.collectionPgModel = new CollectionPgModel();
const collections = [];

range(40).map((num) => (
collections.push(fakeCollectionRecordFactory({
name: num % 2 === 0 ? `testCollection__${num}` : `fakeCollection__${num}`,
version: `${num}`,
cumulus_id: num,
updated_at: new Date(1579352700000 + (num % 2) * 1000),
}))
));

t.context.collections = collections;
await t.context.collectionPgModel.insert(
t.context.knex,
collections
);
});

test.beforeEach((t) => {
t.context.testCollection = fakeCollectionFactory();
});

test.after.always(async () => {
test.after.always(async (t) => {
await accessTokenModel.deleteTable();
await recursivelyDeleteS3Bucket(process.env.system_bucket);
await esClient.client.indices.delete({ index: esIndex });
await destroyLocalTestDb({
...t.context,
testDbName,
});
});

test('CUMULUS-911 GET without pathParameters and without an Authorization header returns an Authorization Missing response', async (t) => {
Expand All @@ -86,21 +138,15 @@ test('CUMULUS-912 GET without pathParameters and with an invalid access token re
test.todo('CUMULUS-912 GET without pathParameters and with an unauthorized user returns an unauthorized response');

test.serial('default returns list of collections from query', async (t) => {
const stub = sinon.stub(EsCollection.prototype, 'query').returns({ results: [t.context.testCollection] });
const spy = sinon.stub(EsCollection.prototype, 'addStatsToCollectionResults');

const response = await request(app)
.get('/collections')
.set('Accept', 'application/json')
.set('Authorization', `Bearer ${jwtAuthToken}`)
.expect(200);

const { results } = response.body;
t.is(results.length, 1);
t.is(results[0].name, t.context.testCollection.name);
t.true(spy.notCalled);
stub.restore();
spy.restore();
t.is(results.length, 10);
t.is(results[0].name, t.context.collections[0].name);
});

test.serial('returns list of collections with stats when requested', async (t) => {
Expand Down
3 changes: 3 additions & 0 deletions packages/db/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,9 @@ export {
export {
StatsSearch,
} from './search/StatsSearch';
export {
CollectionSearch,
} from './search/CollectionSearch';

export { AsyncOperationPgModel } from './models/async_operation';
export { BasePgModel } from './models/base';
Expand Down
2 changes: 1 addition & 1 deletion packages/db/src/search/BaseSearch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,7 @@ class BaseSearch {
* @param testKnex - knex for testing
* @returns search result
*/
async query(testKnex: Knex | undefined) {
async query(testKnex?: Knex) {
const knex = testKnex ?? await getKnexClient();
const { countQuery, searchQuery } = this.buildSearch(knex);
try {
Expand Down
86 changes: 86 additions & 0 deletions packages/db/src/search/CollectionSearch.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
import { Knex } from 'knex';
import pick from 'lodash/pick';

import Logger from '@cumulus/logger';
import { CollectionRecord } from '@cumulus/types/api/collections';
import { BaseSearch } from './BaseSearch';
import { DbQueryParameters, QueryEvent } from '../types/search';
import { translatePostgresCollectionToApiCollection } from '../translate/collections';
import { PostgresCollectionRecord } from '../types/collection';

const log = new Logger({ sender: '@cumulus/db/CollectionSearch' });

/**
* There is no need to declare an ApiCollectionRecord type since
* CollectionRecord contains all the same fields from the api
*/

/**
* Class to build and execute db search query for collection
*/
export class CollectionSearch extends BaseSearch {
constructor(event: QueryEvent) {
super(event, 'collection');
}

/**
* Build basic query
*
* @param knex - DB client
* @returns queries for getting count and search result
*/
protected buildBasicQuery(knex: Knex)
: {
countQuery: Knex.QueryBuilder,
searchQuery: Knex.QueryBuilder,
} {
const countQuery = knex(this.tableName)
.count(`${this.tableName}.cumulus_id`);

const searchQuery = knex(this.tableName)
.select(`${this.tableName}.*`);
return { countQuery, searchQuery };
}

/**
* 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,
}) {
const { countQuery, searchQuery, dbQueryParameters } = params;
const { infix, prefix } = dbQueryParameters ?? this.dbQueryParameters;
if (infix) {
[countQuery, searchQuery].forEach((query) => query.whereLike(`${this.tableName}.name`, `%${infix}%`));
}
if (prefix) {
[countQuery, searchQuery].forEach((query) => query.whereLike(`${this.tableName}.name`, `%${prefix}%`));
}
}

/**
* Translate postgres records to api records
*
* @param pgRecords - postgres records returned from query
* @returns translated api records
*/
protected translatePostgresRecordsToApiRecords(pgRecords: PostgresCollectionRecord[])
: Partial<CollectionRecord>[] {
log.debug(`translatePostgresRecordsToApiRecords number of records ${pgRecords.length} `);
const apiRecords = pgRecords.map((item) => {
const apiRecord = translatePostgresCollectionToApiCollection(item);

return this.dbQueryParameters.fields
? pick(apiRecord, this.dbQueryParameters.fields)
: apiRecord;
});
return apiRecords;
}
}
12 changes: 12 additions & 0 deletions packages/db/src/search/field-mapping.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,18 @@ const collectionMapping : { [key: string]: Function } = {
updatedAt: (value?: string) => ({
updated_at: value && new Date(Number(value)),
}),
reportToEms: (value?: string) => ({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, keep forgetting to add this comment. The feature branch has a test file packages/db/tests/search/test-field-mapping.js. As you are adding some field mappings, do you not need to add any tests to that file for these?

Copy link
Contributor Author

@Nnaga1 Nnaga1 Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not each time, I think based on how Jenny made that file/test, as long as that test-field-mapping test passes, and the correct response/correct format is returned, it should be fine. the reason to add field-mappings in the first place is so term search can work for the specific table, and we test the term search in the respective Search tests

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I missed the actual test before, but it is in that file I referenced on line 104. I think that test should include some of these fields you added, like report_to_ems, sample_file_name, and url_path

report_to_ems: (value === 'true'),
}),
process: (value?: string) => ({
process: value,
}),
sampleFileName: (value?: string) => ({
sample_file_name: value,
}),
urlPath: (value?: string) => ({
url_path: value,
}),
};

const executionMapping : { [key: string]: Function } = {
Expand Down
Loading