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.

Sign up for GitHub

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

Jump to bottom

Adding HTTP Error responses #666

Merged
merged 8 commits into from
Oct 25, 2021
Merged

Adding HTTP Error responses #666

merged 8 commits into from
Oct 25, 2021

Conversation

LorenzHW
Copy link
Contributor

Fixes #200

@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2021 •
edited
Loading

Codecov Report

Merging #666 (aee96df) into develop (25058e5) will increase coverage by 0.48%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #666      +/-   ##
===========================================
+ Coverage    53.77%   54.25%   +0.48%     
===========================================
  Files           23       23              
  Lines         3669     3723      +54     
===========================================
+ Hits          1973     2020      +47     
- Misses        1434     1435       +1     
- Partials       262      268       +6
Impacted Files Coverage Δ
api/handlers.go 15.03% <0.00%> (+0.57%) ⬆️
idb/postgres/internal/writer/writer.go 77.97% <0.00%> (-5.60%) ⬇️
idb/postgres/postgres.go 46.60% <0.00%> (+0.23%) ⬆️
idb/postgres/postgres_migrations.go 35.16% <0.00%> (+0.38%) ⬆️
idb/postgres/internal/encoding/encode.go 88.54% <0.00%> (+1.26%) ⬆️
...ernal/ledger_for_evaluator/ledger_for_evaluator.go 67.13% <0.00%> (+1.43%) ⬆️
idb/postgres/internal/encoding/decode.go 64.00% <0.00%> (+2.60%) ⬆️

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 25058e5...aee96df. Read the comment docs.

@LorenzHW LorenzHW force-pushed the issue_200_incomplete_responses branch from e39aa39 to 8421343 Compare September 21, 2021 20:11
@LorenzHW LorenzHW force-pushed the issue_200_incomplete_responses branch from 8421343 to dab9588 Compare September 26, 2021 19:50
@LorenzHW
Copy link
Contributor Author

Are the tests flaky sometimes? On my forked Travis the tests are green.

@algojack
Copy link
Contributor

algojack commented Oct 6, 2021

does this fix #369?

@LorenzHW
Copy link
Contributor Author

@algojack It seems like that #369 has already been fixed. I updated the OpenAPI definition accordingly.

@winder Can we get this merged?

tolikzinovyev
tolikzinovyev reviewed Oct 19, 2021
View reviewed changes
api/indexer.oas2.json Outdated
@@ -223,6 +244,12 @@
"responses": {
"200": {
"$ref": "#/responses/ApplicationsResponse"
},
"404": {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I removed it.

api/indexer.oas2.json
@@ -2295,6 +2356,12 @@
}
}
},
"ErrorResponse": {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this change needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Take a look at how error responses were defined before:
Screen Shot 2021-10-24 at 14 00 54

Adding a response in the #/responses section helps us to get rid of duplicate code because we don't need to define schema anymore. This makes the API spec more concise and readable.

tolikzinovyev
tolikzinovyev approved these changes Oct 25, 2021
View reviewed changes
Copy link
Contributor

@tolikzinovyev tolikzinovyev left a comment

Choose a reason for hiding this comment

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

Thanks!

@tolikzinovyev tolikzinovyev merged commit c81b364 into algorand:develop Oct 25, 2021
@LorenzHW LorenzHW mentioned this pull request Oct 25, 2021
Open
1 task
@algobarb algobarb mentioned this pull request Nov 30, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tolikzinovyev tolikzinovyev tolikzinovyev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incomplete OpenAPI Response Codes
4 participants
@LorenzHW @codecov-commenter @algojack @tolikzinovyev