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

Replace all REST query empty result HTTP Status codes to 200 #4065

Merged
merged 6 commits into from
Apr 10, 2019
Merged

Replace all REST query empty result HTTP Status codes to 200 #4065

merged 6 commits into from
Apr 10, 2019

Conversation

MarinX
Copy link
Contributor

@MarinX MarinX commented Apr 6, 2019 •
edited
Loading

All query params that are empty will now return success (http.StatusOK) instead of mixed http status codes (http.StatusNoContent/http.StatusBadRequest/http.StatusInternalServerError)

closes: #2007

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added a relevant changelog entry: sdkch add [section] [stanza] [message]

  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@MarinX MarinX changed the title 2007 empty query results Replace all REST query empty result HTTP Status codes to 200 Apr 6, 2019
@codecov
Copy link

codecov bot commented Apr 6, 2019

Codecov Report

Merging #4065 into develop will increase coverage by 0.01%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #4065      +/-   ##
===========================================
+ Coverage    60.01%   60.02%   +0.01%     
===========================================
  Files          212      212              
  Lines        15107    15104       -3     
===========================================
  Hits          9066     9066              
+ Misses        5420     5417       -3     
  Partials       621      621

1 similar comment
@codecov
Copy link

codecov bot commented Apr 6, 2019 •
edited
Loading

Codecov Report

Merging #4065 into develop will increase coverage by 0.01%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #4065      +/-   ##
===========================================
+ Coverage    60.01%   60.02%   +0.01%     
===========================================
  Files          212      212              
  Lines        15107    15104       -3     
===========================================
  Hits          9066     9066              
+ Misses        5420     5417       -3     
  Partials       621      621

fedekunze
fedekunze suggested changes Apr 6, 2019
View reviewed changes
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

thanks @MarinX again for your contribution ! I left some notes that need to be addressed

x/auth/client/rest/query.go Outdated
@@ -88,7 +88,7 @@ func QueryBalancesRequestHandlerFn(

// the query will return empty if there is no data for this account
if len(res) == 0 {
w.WriteHeader(http.StatusNoContent)
w.WriteHeader(http.StatusOK)
Copy link
Collaborator

Choose a reason for hiding this comment

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

curious if this function is a duplicate from the one above ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of it is duplicated, only change is response. Eg. for /auth/accounts/{address} we are returning Account{} which in case /bank/balances/{address} we are returning sdk.Coins

The balance endpoint does not make sense for me, it should be /auth/accounts/{address}/balances and the same function should check the endpoint and return corresponding model. But this is a API breaking change so maybe it should be a separated issue for code cleanup :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree it doesn’t make much sense. Let’s open an issue for it

@alessio alessio requested review from alessio and sabau April 7, 2019 19:00
alexanderbez
alexanderbez suggested changes Apr 8, 2019
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Thanks @MarinX -- changes l look mostly good, but a bunch of return codes have been modified that shouldn't have.

@MarinX
Copy link
Contributor Author

MarinX commented Apr 8, 2019

Summary of change:

  • Reverted back most of statuses for empty query params
  • For empty results, we are returning empty models
  • Added TestAccountBalanceQuery which covers the case for empty results

alexanderbez
alexanderbez suggested changes Apr 9, 2019
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Missing pending change log entry 👍

fedekunze
fedekunze approved these changes Apr 9, 2019
View reviewed changes
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

ACK, missing changelog entry tho

@MarinX
Copy link
Contributor Author

MarinX commented Apr 9, 2019

noted.
Added the changelog entry

@fedekunze fedekunze requested a review from alexanderbez April 10, 2019 09:11
alessio
alessio suggested changes Apr 10, 2019
View reviewed changes
Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

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

Changelog entry is wrong

alexanderbez
alexanderbez suggested changes Apr 10, 2019
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Minor nit but otherwise LGMT.

@MarinX
Copy link
Contributor Author

MarinX commented Apr 10, 2019

Thanks for the suggestion.
I updated it.
It should be ok now :)

@alexanderbez
Copy link
Contributor

@alessio please approve :)

@MarinX MarinX mentioned this pull request Apr 10, 2019
Closed
4 tasks
@alessio alessio merged commit b50b25d into cosmos:develop Apr 10, 2019
@MarinX MarinX deleted the 2007-empty-query-results branch April 10, 2019 17:41
@alexanderbez alexanderbez mentioned this pull request May 3, 2019
Closed
4 tasks
alexanderbez pushed a commit that referenced this pull request May 3, 2019
@MarinX @alexanderbez
Replace all REST query empty result HTTP Status codes to 200 (#4065)
70b8b5d 
All query params that are empty now returns success
(http.StatusOK) instead of mixed http status codes
(http.StatusNoContent/http.StatusBadRequest/
http.StatusInternalServerError)

Closes: #2007
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alessio alessio alessio approved these changes

@alexanderbez alexanderbez alexanderbez approved these changes

@fedekunze fedekunze fedekunze approved these changes

@cwgoes cwgoes Awaiting requested review from cwgoes

@ebuchman ebuchman Awaiting requested review from ebuchman

@rigelrozanski rigelrozanski Awaiting requested review from rigelrozanski

@sabau sabau Awaiting requested review from sabau

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

Successfully merging this pull request may close these issues.
4 participants
@MarinX @alexanderbez @alessio @fedekunze