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

fix: Change successful schema add status to 200 #1106

Merged
merged 2 commits into from
Feb 15, 2023

Conversation

fredcarle
Copy link
Collaborator

Relevant issue(s)

Resolves #1100

Description

This PR changes the returned status code from the http api on a schema add call to be 200 instead of 404. It also changes a few other statuses from 400 to 500 to be more appropriate.

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

manual CLI call

Specify the platform(s) on which this was tested:

  • MacOS

@fredcarle fredcarle added bug Something isn't working area/api Related to the external API component labels Feb 14, 2023
@fredcarle fredcarle added this to the DefraDB v0.5 milestone Feb 14, 2023
@fredcarle fredcarle requested a review from a team February 14, 2023 22:54
@fredcarle fredcarle self-assigned this Feb 14, 2023
Copy link
Contributor

@orpheuslummis orpheuslummis left a comment

Choose a reason for hiding this comment

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

LGTM. That said I thought a few of the places where it was StatusBadRequest and now StatusInternalServerError could be one of the other depending on the situation, although it seems to better to err on the side of StatusInternalServerError for perhaps a bit more clarity to the user. "The server errs with this broken request".

@codecov
Copy link

codecov bot commented Feb 14, 2023

Codecov Report

Merging #1106 (fc99cb4) into develop (e2160eb) will decrease coverage by 0.02%.
The diff coverage is 85.71%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1106      +/-   ##
===========================================
- Coverage    67.41%   67.39%   -0.02%     
===========================================
  Files          178      178              
  Lines        16635    16635              
===========================================
- Hits         11214    11211       -3     
- Misses        4474     4476       +2     
- Partials       947      948       +1     
Impacted Files Coverage Δ
api/http/handlerfuncs.go 85.44% <85.71%> (ø)
net/server.go 59.87% <0.00%> (-0.98%) ⬇️

@source-devs
Copy link

Benchmark Results

Summary

  • 0 Benchmarks successfully compared.
  • 0 Benchmarks were ✅ Better.
  • 0 Benchmarks were ❌ Worse .
  • 0 Benchmarks were ✨ Unchanged.
✅ See Better Results...
time/opdelta
 
❌ See Worse Results...
time/opdelta
 
✨ See Unchanged Results...
time/opdelta
 
🐋 See Full Results...

@fredcarle
Copy link
Collaborator Author

LGTM. That said I thought a few of the places where it was StatusBadRequest and now StatusInternalServerError could be one of the other depending on the situation, although it seems to better to err on the side of StatusInternalServerError for perhaps a bit more clarity to the user. "The server errs with this broken request".

Where I changed it it's because I thought a 500 error was more appropriate but if you think it's not the case in a given situation, let me know :)

@orpheuslummis
Copy link
Contributor

LGTM. That said I thought a few of the places where it was StatusBadRequest and now StatusInternalServerError could be one of the other depending on the situation, although it seems to better to err on the side of StatusInternalServerError for perhaps a bit more clarity to the user. "The server errs with this broken request".

Where I changed it it's because I thought a 500 error was more appropriate but if you think it's not the case in a given situation, let me know :)

I think it's appropriate. My comment was just that there seems to be a more ideal world in which we classify perfectly between user error and server error but it'd require more work and it's not worth it at this point.

@shahzadlone shahzadlone added the action/no-benchmark Skips the action that runs the benchmark. label Feb 15, 2023
@fredcarle fredcarle merged commit 467cbe8 into develop Feb 15, 2023
@fredcarle fredcarle deleted the fredcarle/fix/I1100-loadschema-status branch February 15, 2023 15:43
shahzadlone pushed a commit that referenced this pull request Apr 13, 2023
Relevant issue(s)
Resolves #1100

Description
This PR changes the returned status code from the http api on a schema add call to be 200 instead of 404. It also changes a few other statuses from 400 to 500 to be more appropriate.
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
Relevant issue(s)
Resolves sourcenetwork#1100

Description
This PR changes the returned status code from the http api on a schema add call to be 200 instead of 404. It also changes a few other statuses from 400 to 500 to be more appropriate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. area/api Related to the external API component bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return proper HTTP error code on Schema add
4 participants