CUMULUS-3697: Remove searchContext from granules LIST endpoint ( Jk/cumulus 3697 update api) #3863
Conversation
There was a problem hiding this comment.
Looks great @Jkovarik! Very glad we didn't actually need a replacement for searchContext.
One broader question about scope on this ticket: can/should we remove the searchContext param from es-client? I don't think it's used elsewhere so maybe we can remove it everywhere?
| const response = await request(app) | ||
| .get('/granules?limit=100&page=101') | ||
| .set('Accept', 'application/json') | ||
| .expect(200); |
There was a problem hiding this comment.
What would this return if you specified a page that doesn't exist? e.g. get('/granules?limit=100&page=99999999')?
Wondering if the API would happily return a successful response for a blank, out-of-range page, or if it'd return something different. If something different, 👍. If there's a possibility you could get a 200 but no results or something, maybe we validate results?
There was a problem hiding this comment.
If, for example, you ask for page 102, it returns no results.
I suppose we could return something more intelligent on pagination (we're not currently in that endpoint as delivered). If you think it's worthwhile I can look into what ES does, but I'd expect it's consistent given our implementation strategy.
There was a problem hiding this comment.
My only concern here is the test itself. We're requesting page 101, which presumably contains results and returns a 200. What if it didn't have results? I... assume... it would return something other than 200?
In other words, is a 200 response indicative of actual results returned?
There was a problem hiding this comment.
I think the 200 assertion here just is a catch in case the API throws to make the test easier to read if it does not. We could remove it to the same effect, just would make the test more unclear if the API unexpectedly fails.
The functional test is that it returned the last page, with 1 granule (t.is(response.body.results.length, 1);)
There was a problem hiding this comment.
OHHHHHHH ha
Ok, so waaaaaaayyyyy down after all the removals you have the assertion you point out t.is(response.body.results.length, 1);. That makes a lot more sense 😆. I didn't see that and thought we were saying the 200 proved that it's returning results.
Ok, I'm good.
|
@npauzenga "One broader question about scope on this ticket: can/should we remove the searchContext param from es-client? I don't think it's used elsewhere so maybe we can remove it everywhere?" We could, but I believe that's the subject of a broader ticket to remove it entirely. |
Meaning, that ticket already exists? I don't see one but I'm also not looking super hard 😆. I'm thinking it should be an easy thing to do if it's not used anywhere else. That could be grossly optimistic and if a ticket already exists, that's cool. |
|
@npauzenga @charleshuang80 has CUMULUS-3847, which I thought would cover it. |
Summary: Summary of changes
Addresses CUMULUS-3697
Changes
searchContextfrom API granules GET/granulesendpoint.PR Checklist