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.

Sign up for GitHub

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

Jump to bottom

Reintroduce lidvid not found case when testing that the latest detection script runs #259

Merged
merged 2 commits into from
Mar 7, 2023
Merged

Reintroduce lidvid not found case when testing that the latest detection script runs #259

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7cca27c
exclude not found case for 500 error on multiple lidvids returns when…
Feb 24, 2023
c37c718
remove local configuration which should not have been pushed in the f…
Mar 2, 2023
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: 1 addition & 1 deletion service/src/main/java/gov/nasa/pds/api/registry/model/RequestAndResponseContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@tloubrieu-jpl tloubrieu-jpl Mar 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's the commit in question if you want to get in before I do

22a0970?diff=split

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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?

String basicErrMsg =
"Got " + hitCount + " hits for a query which should have returned a singular result. "
+ "Is provenance metadata present and up-to-date?";
Expand Down
4 changes: 2 additions & 2 deletions service/src/main/resources/application.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
springfox.documentation.swagger.v2.path=/api-docs
alexdunnjpl marked this conversation as resolved.
Show resolved Hide resolved
server.contextPath=/
server.port=8080
server.port=8081
server.forward-headers-strategy=framework

#spring.jackson.date-format=io.swagger.RFC3339DateFormat
Expand All @@ -18,7 +18,7 @@ server.ssl.key-store=classpath:keystore.p12
server.ssl.key-store-type=PKCS12

# note the port is mandatory even when it is default :80 or :443
openSearch.host=localhost:9200
openSearch.host=elasticsearch:9200
openSearch.registryIndex=registry
openSearch.registryRefIndex=registry-refs
openSearch.timeOutSeconds=60
Expand Down