marceloboeira commented 2 years ago

The uniqueness constraint of flag.key is a known part of flag. Validation errors that rely on client-logic (uniqueness of a field) is responsibility of the client to implement, thus the server should return a 400-ish (e.g.: 400 Bad Request, 406 Not Acceptable, ...) and not a 500 Internal Server Error, since that is not the case.

More info: #41

Description

Capture flag uniqueness errors to respond with 400 and not 500 since it is a client-side problem

Motivation and Context

41

How Has This Been Tested?

Added a test in the controller, but probably not need an integration one...

Types of changes

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

Checklist:

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

Updates

This can also happen on PUT /api/v1/flag/:id Key=AlreadyExistent.

TODO

[x] Add integration tests for invalid POST
[x] Add integration tests for invalid PUT
[x] Add unit tests for invalid POST
[x] Add uni tests for invalid PUT
[x] Fix POST
[x] Fix PUT

codecov-commenter commented 2 years ago

Codecov Report

Merging #42 (2aa5d2d) into main (f6a8dd2) will increase coverage by 0.05%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #42      +/-   ##
==========================================
+ Coverage   81.55%   81.60%   +0.05%     
==========================================
  Files          27       27              
  Lines        2174     2180       +6     
==========================================
+ Hits         1773     1779       +6     
  Misses        314      314              
  Partials       87       87

Impacted Files	Coverage Δ
pkg/handler/crud_flag_creation.go	`78.04% <100.00%> (+1.73%)`	:arrow_up:

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 f6a8dd2...2aa5d2d. Read the comment docs.

marceloboeira commented 2 years ago

@zhouzhuojie given the breaking changes with check/flagr this won't pass the integration tests for the new 400 vs 500 checks...

POST checkr_flagr_with_sqlite:18000/api/v1/flags
1062
 ✘ status 400 (actual: 500)

github-actions[bot] commented 2 years ago

Stale pull request message

zhouzhuojie commented 2 years ago

might need to rebase and fix the CI

openflagr / flagr-archived

Return 400 when flag.key is not unique #42