Reintroduce lidvid not found case when testing that the latest detection script runs #259
Conversation
| @@ -357,7 +357,7 @@ public void setResponse(RestHighLevelClient client, SearchRequest request) throw | |||
| long hitCount = hits.getTotalHits().value; | |||
| if (hitCount == 1L) { | |||
| this.formatters.get(this.format).setResponse(hits.getAt(0), this.fields); | |||
| } else { | |||
| } else if (hitCount > 1L) { | |||
There was a problem hiding this comment.
Unless I'm missing something, the logic change here seems incorrect - the idea of that set of conditionals is "if the response yields a single result, as expected, throw an error, otherwise there is an incorrect number of responses (either zero, or multiple), in which case a descriptive error should be thrown".
If this change addresses a test failure, the test case should be adjusted instead imho.
There was a problem hiding this comment.
0 is not an error since it can happen that the lidvid is not found in the database, in which case we want to return 404 error and not 500 with the message on latest script. The change I made makes the not found lidvid (0 found) return 404 again (tested with integration tests)
I can not be positive though that my approach is the best regarding the overall architecture of the application, @al-niessner would help us on that.
There was a problem hiding this comment.
@tloubrieu-jpl what you're saying makes sense, but let me take a closer look to make sure that this isn't in a context where exactly-one hit is expected - I recall changing this conditional a couple of weeks back and it doesn't seem like a simple/understandable mistake for me to have messed up the logic so badly during that change.
There was a problem hiding this comment.
Here's the commit in question if you want to get in before I do
There was a problem hiding this comment.
@alexdunnjpl it does not look like your commit introduced that error, since before, the else clause was already both referring to cases with 0 or >1 results. The change I am proposing it to exclude the case 0 result from the else.
I am not questioning the behavior since after my PR, we are getting the expected behavior, but there might be a better way, especially more readable way of doing that.
There was a problem hiding this comment.
@al-niessner could you chime in on whether the original logic (see 22a0970 pre-diff version) is a mistake, or correct given some context @tloubrieu-jpl and I are missing?
|
@al-niessner could you review this PR ? |
|
Branch test failure is due to bug fixed in ce237d3 which is already merged to |
|
@alexdunnjpl , since @al-niessner is mostly unavailable this week, I suggest to merge this PR (since it does what I expect). |
🗒️ Summary
Very simple change
⚙️ Test Data and/or Report
The test is part of the standard integration tests found in repository 'registry'
♻️ Related Issues
Fixes #258