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

Improve Search API Error Handling #343

Merged
merged 2 commits into from
Feb 28, 2022

Conversation

MRichards99
Copy link
Collaborator

Since I've emailed Rolf discussing the datasets relationship in the PaNOSC data model, I will keep #319 open. I will close it once I've got a response and we know a way forward with the data model (whether to raise it further or to follow it as it is).

Description

This PR improves the error signaling for the search API. In #319 (comment), there's a request that showed a 400. Since this is an issue with the validation of the data model, this should really be a 500. My changes fix that for Pydantic's ValidationErrors.

I've also made some general improvements - I've added KeyError to the tuple of standard Python exceptions to catch and moved the assignment of a status_code attribute into its own function (to avoid code duplication).

The version bump will be a fix bump.

Testing Instructions

When using a local instance pointing at ISIS dev ICAT, the following request should return a 500 with a more meaningful error message (about the validation error):
http://{{datagateway-api}}/search-api/documents?filter={"limit": 10}&filter={ "include": [ { "relation": "members", "scope": { "where": { "role": { "neq": "principal_experimenter" } }, "include": [ { "relation": "person" } ] } }, { "relation": "datasets" } ], "limit": 1, "skip": 3

  • Review code
  • Check GitHub Actions build
  • If icatdb Generator Script Consistency Test CI job fails, is this because of a deliberate change made to the script to change generated data (which isn't actually a problem) or is here an underlying issue with the changes made?
  • Review changes to test coverage
  • Does this change mean a new patch, minor or major version should be made? If so, does one of the commit messages feature fix:, feat: or BREAKING CHANGE: so a release is automatically made via GitHub Actions upon merge?

@MRichards99 MRichards99 requested a review from VKTB February 24, 2022 17:09
@codecov
Copy link

codecov bot commented Feb 24, 2022

Codecov Report

Merging #343 (1e42981) into master (dd8bc51) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #343      +/-   ##
==========================================
+ Coverage   94.45%   94.46%   +0.01%     
==========================================
  Files          39       39              
  Lines        3065     3073       +8     
  Branches      306      307       +1     
==========================================
+ Hits         2895     2903       +8     
  Misses        144      144              
  Partials       26       26              
Impacted Files Coverage Δ
datagateway_api/src/search_api/helpers.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd8bc51...1e42981. Read the comment docs.

@MRichards99 MRichards99 merged commit 982518a into master Feb 28, 2022
@MRichards99 MRichards99 deleted the bugfix/validation-error-code-#319 branch February 28, 2022 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants