CUMULUS-3641 - Update Collections LIST endpoint to query Postgres basic #3681

Nnaga1 · 2024-06-04T20:56:13Z

Summary: Summary of changes

Addresses CUMULUS-XX: Develop amazing new feature

Changes

Detailed list or prose of changes
...

PR Checklist

Update CHANGELOG
Unit tests
Ad-hoc testing - Deploy changes and test manually
Integration tests

…into feature/es-phase-1

charleshuang80

Some comments/suggested changes. Mostly minor, but one might not be. Still have to go through the tests, so need to continue with that.

packages/api/endpoints/collections.js

packages/db/src/search/CollectionSearch.ts

packages/api/endpoints/collections.js

charleshuang80

review for first test, one more to go

CHANGELOG.md

packages/db/src/search/field-mapping.ts

packages/api/tests/endpoints/collections/list-collections.js

packages/db/tests/search/test-CollectionSearch.js

Jkovarik · 2024-06-06T15:58:01Z

packages/api/endpoints/collections.js

@@ -43,14 +44,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)}`);


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

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

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

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.

packages/api/endpoints/collections.js

Jkovarik

Please enable ts-check on the endpoints logic!

Nnaga1 · 2024-06-11T14:24:17Z

Please enable ts-check on the endpoints logic!

I don't agree with this, this creates errors in virtually every other function when it does getEsClient() (put, del, etc.) except for list pretty much (the fix for that is easy), I don't think I should add something that makes me change work that isn't mine or even related to mine and doesn't really have much value outside of a temporary fix/check

Jkovarik · 2024-06-11T15:22:02Z

Please enable ts-check on the endpoints logic!

I don't agree with this, this creates errors in virtually every other function when it does getEsClient() (put, del, etc.) except for list pretty much (the fix for that is easy), I don't think I should add something that makes me change work that isn't mine or even related to mine and doesn't really have much value outside of a temporary fix/check

Respectfully, it's required per our team ADRs, and something we should be doing in all of our other PRs. It's not expected that we fix all of the errors noted, but all code in the scope of the updates should be properly type annotated.

We should have a team conversation about this - if the team wants to change that stance, we should make that a deliberate decision.

Until this is resolved this cannot be merged.

Nnaga1 · 2024-06-11T19:26:16Z

Please enable ts-check on the endpoints logic!

I don't agree with this, this creates errors in virtually every other function when it does getEsClient() (put, del, etc.) except for list pretty much (the fix for that is easy), I don't think I should add something that makes me change work that isn't mine or even related to mine and doesn't really have much value outside of a temporary fix/check

Respectfully, it's required per our team ADRs, and something we should be doing in all of our other PRs. It's not expected that we fix all of the errors noted, but all code in the scope of the updates should be properly type annotated.

We should have a team conversation about this - if the team wants to change that stance, we should make that a deliberate decision.

Until this is resolved this cannot be merged.

I think an arch meeting topic would be good, I think there is still something I am misunderstanding as to the why/what that you can probably explain better through words 😆 , and it doesnt seem like itll be too magnitudal of a change anyways so. I just dont want to mess with the rest of the file (which does have ts errors, a good amount) if this one spot is going to be a temporary change/ doesnt even give issues/wasn't like that before.

Jkovarik · 2024-06-12T14:03:14Z

@Nnaga1 the changes in cea3c14 and db45ecd look good re: the collection endpoint typings. Thank you!

Concerns have been addressed, thank you!

charleshuang80 · 2024-06-12T18:18:23Z

packages/db/tests/search/test-CollectionSearch.js

+  t.is(response2.results?.length, 20);
+});
+
+test('CollectionSearch supports term search for number field', async (t) => {


this is really minor, and if you want to push off into the feature branch then that's fine, but I think you should try and order these tests to align with the ordering in test-GranuleSearch. That way comparisons and additions/subtractions of the tests will be easier to deal with.

you can also move around the tests in GranuleSearch to get the ordering to match better if that helps

charleshuang80 · 2024-06-12T18:21:22Z

packages/db/tests/search/test-CollectionSearch.js

+  t.is(response.results?.length, 1);
+});
+
+test('CollectionSearch supports search which granule field does not match the given value', async (t) => {


should this description say collection field not granule field?

charleshuang80 · 2024-06-12T22:09:51Z

packages/db/src/search/field-mapping.ts

@@ -123,6 +123,18 @@ const collectionMapping : { [key: string]: Function } = {
  updatedAt: (value?: string) => ({
    updated_at: value && new Date(Number(value)),
  }),
+  reportToEms: (value?: string) => ({


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?

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

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

charleshuang80 · 2024-06-12T22:12:56Z

packages/db/tests/search/test-CollectionSearch.js

+  const response = await dbSearch.query(knex);
+  t.is(response.meta.count, 100);
+  t.is(response.results?.length, 10);
+  response.results.forEach((granule) => t.deepEqual(Object.keys(granule), fields.split(',')));


should this be collection instead of granule? or am I misunderstanding what the results are here?

you're right I copied it incorrectly, changed👍

charleshuang80

looks good! sorry to be a stickler for small things, but thanks for making the changes. I think they'll help with maintainability and when others have to deal with the code.

jennyhliu · 2024-06-17T14:21:21Z

example/spec/parallel/createReconciliationReport/CreateReconciliationReportSpec.js

@@ -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 } });


We should add support for _id__in for collection db query.

* CUMULUS-3692: Update granules List endpoints to query postgres for basic queries (#3637) * CUMULUS-3692:Granule list endpoint for basic postgres query * CUMULUS-3694: Update granules List endpoints to query postgres - filter by field value (#3656) * CUMULUS-3692:Granule list endpoint for basic postgres query * refactor * refactor * typing * add changelog entry * skip search_after * skip searchafter unit tests * add granule list test * rename * refactor * build query parameters * update comment * add field-mapping * update jsdoc * use type over interface,add log * update test description * build term/terms * buildDbQueryParameters * add unit test no terms search * add doc * rename * add unit test * add fields test * add more unit tests * support error.Error search * fix lint * rename functions * ignore files * add convert query unit tests * add all types * add unit test for fieldmapping types fix timestamp * update timestamp test * add multiple term field test * ignore execution in granule list record * CUMULUS-3689: Update Stats/Summary and Stats/Aggregate endpoints to use psql (#3659) * first commit on new branch * CHANGELOG change * small fix * PR feedback * adding jsdoc + fixing spelling/grammar * CUMULUS-3693: Update granules List endpoints to query postgres - range (#3660) * add range query support * CUMULUS-3695 - Update Granules endpoint to handle SortFields (#3663) * first committ * CHANGELOG change * fixing sortQueryMethod * simplifying code * PR feedback * merge conflicts + improving code * small jsdoc fix * PR feedback * PR feedback * PR feedback * fixing test * PR feedback * PR feedback * CUMULUS-3696: Update granules List endpoints to query postgres - match (#3674) * add methods to convert terms,not,exists * CUMULUS-3641 - Update Collections LIST endpoint to query Postgres basic (#3681) * reopening PR * PR feedback * small test fix * small PR feedbacks * adding new tests from match queries * PR feedback/formatting * temporary reversion to list endpoint for reconreport tests * reverting changes * adding logging * more logging * more logging * removing logging + commenting reconrep test temp * commenting out failing createReconReport spec * removing comment * reverting changes to reconReport test * reverting previous change * adding ts-check * PR feedback * PR feedback * adding in test * PR feedback fix * PR feedback * CUMULUS-3699 - Update collection List endpoints to query postgres - includeStats (#3688) * first commit * CHANGELOG * fixing small things * changes + fixes * PR feedback * splitting queries separately * PR feedback * PR feedback * PR feedback * CUMULUS-3639: Add support to db/CollectionSearch to retrieve active collections (#3693) * CUMULUS-3639:Add support to db/CollectionSearch to retrieve active collections * add test active collections * add ts-check * update /collections/active unit test * test snyk * fix field mapping * parallel search and fix urlPath * add cumulus-lp stack * add limit 1 to subquery * CUMULUS-3239 - Update Executions LIST endpoint to query Postgres basic (#3684) * first commit * adding changes * storing changes * updating progress * linting + small fixes * small fix * changing timestamp to string in tests * fixing timestamp * commenting out tests failing in CI but not locally * saving changes * collection support * adding async_ops support * changing endpoint + tests * fixing test * uncommenting tests + adding var * commenting out tests failing in CI but not locally * adding parentArn support + changing tests * added parentArn support + fixing tests * small endpoint test fix * Pr feedback + code improvements * small CHANGELOG fix * PR feedback * PR feedback + linting * PR feedback * PR feedback * fixing test * fixing execution tests after removing asyncCumulusOPId from mapping * PR feedback * removed includeFullRecord from search classes * PR feedback * PR feedback * reverting change * fix changelog * Cumulus 3640/3242- Update granule non-LIST endpoints and other granule related es queries to query postgres (#3727) * removing granules * fixing lint * fixing test * small change * adding back in some deleted things * removing more * lint fix * removing tests * skipping execution search-by-granules tests * skipping execution tests * removing tests * more removing * adding in deleted test * removing more * adding back in needed code * removing ES_HOST, query, and index from bulk_ops * fixing bulk_ops tests * adding back in ELK stack refs * changing reconreports test to skip/adding back in getGranulesByPayload * PR feedback * adding back in skipped tests * CHANGELOG * PR feedback * PR feedback' . ; * PR feedback + syntax check * adding back sort to write-granules test * CUMULUS-3240: Remove Elasticsearch dependency from executions endpoints (#3723) * CUMULUS-3240:Remove ElasticSearch dependency from Executions endpoints * update test-executions * remove es dependencies for execution * update changelog * fix lint and warning * address PR feedback * remove esClient from createExecutionRecords * CUMULUS-3642: postgres query adjustment (#3731) * estimate table row count * add more indexes * use count * * fix test * fix test * fix lint and add file index * add more index * fix lint * fix lint update test * execution asyncOperationId is optional * defautl stats last day * fix granule patchByGranuleId put logic back * vaccum tables * update changelog * remove sql script to another pr * update changelog * update active collection query * Revert "update active collection query" This reverts commit 88024c2. * CUMULUS-3238: Remove ElasticSearch dependency from Collection POST, PUT, and DEL endpoints (#3746) * first commit * CHANGELOG change * PR feedback * PR feedback * CUMULUS-3792: Add db indexes to improve search performance (#3751) * CUMULUS-3792:Add table indexes to improve search performance * fix changelog --------- Co-authored-by: Naga Nages <66387215+Nnaga1@users.noreply.github.com> Co-authored-by: Nate Pauzenga <npauzenga@gmail.com>

Nnaga1 added 6 commits May 20, 2024 10:10

Merge branch 'feature/es-phase-1' of https://github.com/nasa/cumulus …

993d72a

…into feature/es-phase-1

Merge branch 'feature/es-phase-1' of https://github.com/nasa/cumulus …

65dfe5e

…into feature/es-phase-1

Merge branch 'feature/es-phase-1' of https://github.com/nasa/cumulus …

e4c1e59

…into feature/es-phase-1

Merge branch 'feature/es-phase-1' of https://github.com/nasa/cumulus …

d7dd6cf

…into feature/es-phase-1

Merge branch 'feature/es-phase-1' of https://github.com/nasa/cumulus …

b557e9e

…into feature/es-phase-1

reopening PR

c89f0f9

charleshuang80 reviewed Jun 5, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

packages/db/src/search/field-mapping.ts Show resolved Hide resolved

packages/api/tests/endpoints/collections/list-collections.js Show resolved Hide resolved

packages/api/tests/endpoints/collections/list-collections.js Show resolved Hide resolved

Nnaga1 added 3 commits June 5, 2024 13:21

PR feedback

f19244c

small test fix

43b1cd7

small PR feedbacks

f386137

Nnaga1 commented Jun 5, 2024

View reviewed changes

packages/db/tests/search/test-CollectionSearch.js Show resolved Hide resolved

charleshuang80 reviewed Jun 5, 2024

View reviewed changes

packages/db/tests/search/test-CollectionSearch.js Outdated Show resolved Hide resolved

packages/db/tests/search/test-CollectionSearch.js Outdated Show resolved Hide resolved

adding new tests from match queries

be810af

Nnaga1 added the in review label Jun 5, 2024

Jkovarik reviewed Jun 6, 2024

View reviewed changes

packages/api/endpoints/collections.js Show resolved Hide resolved

Nnaga1 added 10 commits June 6, 2024 12:46

PR feedback/formatting

682660b

temporary reversion to list endpoint for reconreport tests

cea3c14

reverting changes

1e6d54e

adding logging

aaec2a2

more logging

2ee8efd

more logging

ca6b106

removing logging + commenting reconrep test temp

0d9f057

commenting out failing createReconReport spec

d0ae579

removing comment

b282642

reverting changes to reconReport test

3ca493d

Jkovarik previously requested changes Jun 10, 2024

View reviewed changes

reverting previous change

52281e8

Nnaga1 changed the title ~~CUMULUS-3641 - Update Collections endpoint to query Postgres basic~~ CUMULUS-3641 - Update Collections LIST endpoint to query Postgres basic Jun 11, 2024

Jkovarik added the pr feedback label Jun 11, 2024

adding ts-check

db45ecd

Jkovarik requested review from Jkovarik and removed request for Jkovarik June 12, 2024 14:04

charleshuang80 reviewed Jun 12, 2024

View reviewed changes

Nnaga1 added 3 commits June 12, 2024 15:03

PR feedback

9f613ba

PR feedback

bc40d57

adding in test

6aeb39f

charleshuang80 reviewed Jun 12, 2024

View reviewed changes

Nnaga1 added 2 commits June 12, 2024 18:28

PR feedback fix

e208d5e

PR feedback

5433023

charleshuang80 approved these changes Jun 13, 2024

View reviewed changes

Nnaga1 merged commit e73059d into feature/es-phase-1 Jun 13, 2024
3 checks passed

jennyhliu reviewed Jun 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CUMULUS-3641 - Update Collections LIST endpoint to query Postgres basic #3681

CUMULUS-3641 - Update Collections LIST endpoint to query Postgres basic #3681

Nnaga1 commented Jun 4, 2024

charleshuang80 left a comment

charleshuang80 left a comment

Jkovarik Jun 6, 2024

Nnaga1 Jun 6, 2024

Jkovarik Jun 6, 2024

charleshuang80 Jun 6, 2024

Jkovarik left a comment

Nnaga1 commented Jun 11, 2024 •

edited

Loading

Jkovarik commented Jun 11, 2024

Nnaga1 commented Jun 11, 2024 •

edited

Loading

Jkovarik commented Jun 12, 2024

charleshuang80 Jun 12, 2024 •

edited

Loading

charleshuang80 Jun 12, 2024

charleshuang80 Jun 12, 2024

charleshuang80 Jun 12, 2024

Nnaga1 Jun 12, 2024 •

edited

Loading

charleshuang80 Jun 13, 2024

charleshuang80 Jun 12, 2024

Nnaga1 Jun 12, 2024

charleshuang80 left a comment

jennyhliu Jun 17, 2024

CUMULUS-3641 - Update Collections LIST endpoint to query Postgres basic #3681

CUMULUS-3641 - Update Collections LIST endpoint to query Postgres basic #3681

Conversation

Nnaga1 commented Jun 4, 2024

Changes

PR Checklist

charleshuang80 left a comment

Choose a reason for hiding this comment

charleshuang80 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jkovarik left a comment

Choose a reason for hiding this comment

Nnaga1 commented Jun 11, 2024 • edited Loading

Jkovarik commented Jun 11, 2024

Nnaga1 commented Jun 11, 2024 • edited Loading

Jkovarik commented Jun 12, 2024

charleshuang80 Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nnaga1 Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charleshuang80 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nnaga1 commented Jun 11, 2024 •

edited

Loading

Nnaga1 commented Jun 11, 2024 •

edited

Loading

charleshuang80 Jun 12, 2024 •

edited

Loading

Nnaga1 Jun 12, 2024 •

edited

Loading