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

Catch exceptions in GetAsync and treat the same as not founds (+ include exception) #459

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

Enkidu93
Copy link
Collaborator

@Enkidu93 Enkidu93 commented Aug 22, 2024

Fixes #453.

I'm not sure if this is the place we want to catch this, but this solution seemed the cleanest to me among the options.


This change is Reviewable

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 56.69%. Comparing base (661eb1a) to head (cfb7dda).

Files Patch % Lines
...al/src/Serval.Shared/Services/EntityServiceBase.cs 50.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #459      +/-   ##
==========================================
+ Coverage   56.67%   56.69%   +0.01%     
==========================================
  Files         275      275              
  Lines       14115    14120       +5     
  Branches     1895     1895              
==========================================
+ Hits         8000     8005       +5     
- Misses       5529     5531       +2     
+ Partials      586      584       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@johnml1135 johnml1135 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ddaspit)

@johnml1135
Copy link
Collaborator

:lgtm:

@johnml1135 johnml1135 merged commit 1e08f4f into main Aug 22, 2024
3 checks passed
@johnml1135 johnml1135 deleted the no_500_on_serialization_failure branch August 22, 2024 20:35
Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion


src/Serval/src/Serval.Shared/Services/EntityServiceBase.cs line 15 at r1 (raw file):

            entity = await Entities.GetAsync(id, cancellationToken);
        }
        catch (Exception e)

This should be handled in the MongoRepository class. Also, catching Exception might hide other errors that should return a 500.

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.

"500 Internal Server Error" for invalid "files/{id}" request
4 participants