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

Add better error checking around 404s and actual database errors #318

Merged
merged 3 commits into from
Mar 19, 2020

Conversation

wildefires
Copy link

Description

This change does an explicit check for a RecordNotFound error and returns a 404 only in that case; returning 500 in other cases

Motivation and Context

Fixes #317

Currently, in the case that the database is removed from behind a running instance of flagr (network connection, password change, etc) the application returns a 404 to a GET request on localhost:13480/api/v1/flags/1000

How Has This Been Tested?

Tested locally by starting up an instance of flagr, verifying functionality, removing the backing database and then attempting to get the same flag again.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

I don't believe there are tests covering this particular kind of failure case in crud_tests.go but I might have missed them - please do point it out and I'll add them if so

@codecov-io
Copy link

codecov-io commented Jan 29, 2020

Codecov Report

Merging #318 into master will decrease coverage by 0.15%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #318      +/-   ##
==========================================
- Coverage   81.88%   81.72%   -0.16%     
==========================================
  Files          26       26              
  Lines        1535     1538       +3     
==========================================
  Hits         1257     1257              
- Misses        209      211       +2     
- Partials       69       70       +1
Impacted Files Coverage Δ
pkg/handler/crud.go 89.68% <50%> (-0.78%) ⬇️

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 c131ad0...b390b52. Read the comment docs.

pkg/handler/crud.go Outdated Show resolved Hide resolved
@wildefires
Copy link
Author

Do you have any advice on writing a test case for this? Is it possible to remove access to the database during the test runs?

@zhouzhuojie
Copy link
Collaborator

Do you have any advice on writing a test case for this? Is it possible to remove access to the database during the test runs?

I think similar to https://github.com/checkr/flagr/blob/master/pkg/handler/crud_test.go#L145, you can have a subtest that stub again for getDB(). Not looking close enough though, stub and mock gorm may be tricky.

@zhouzhuojie zhouzhuojie mentioned this pull request Mar 19, 2020
8 tasks
@zhouzhuojie
Copy link
Collaborator

LGTM, I will merge this and I can add a simple test in the following PR #337

@zhouzhuojie zhouzhuojie merged commit 0febaaf into openflagr:master Mar 19, 2020
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.

When flagr encounters database errors during the GetFlag func it returns 404
3 participants