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

update 4xx responses in API spec #252

Merged
merged 1 commit into from
May 10, 2019
Merged

update 4xx responses in API spec #252

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions specifications/api/swagger.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,7 @@ responseErr423: &responsesErr423
schema: *responsesErr

responsesListWallets: &responsesListWallets
<<: *responsesErr406
200:
description: Ok
schema:
Expand All @@ -681,18 +682,15 @@ responsesPostWallet: &responsesPostWallet
schema: *ApiWallet

responsesGetWallet: &responsesGetWallet
<<: *responsesErr400
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 sure about this but I was not able to produce 400 for this

Copy link
Member

Choose a reason for hiding this comment

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

What if you send an invalid wallet id ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it is not valid/existing wallet's id - it is always 404...
e.g. some wallet ids I'm testing -> https://github.com/input-output-hk/cardano-wallet/pull/251/files#diff-907b72a217c31fa13b9144cb7ba43cf3R834

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm... I'd expect the API to return a 400 for a illed-wallet id 🤔 ..

Copy link
Member

Choose a reason for hiding this comment

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

Okay, so, because this is a path parameter, Servant actually ignores the route if it can't parse it and tries the next one that would match. Since there's none, it throws a 404. Hence not getting a 400. Not a big fan of this behavior to be honest, I don't think it's the most user-friendly but well.. I'll see if we have maybe a setting we can set to change that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is kind of inconsistent with what CLI returns:

$ stack exec -- cardano-wallet wallet get 617963656a409b8a6828dc3a09001de22af9040
wallet id should be an hex-encoded string of 40 characters

$ stack exec -- cardano-wallet wallet get 617963656a409b8a6828dc3a09001de22af90401
Wallet not found.

404 is okey(ish) from my perspective though (in the end it is Not Found whether id is valid or not ;)

Copy link
Member

Choose a reason for hiding this comment

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

So, I've found this -> haskell-servant/servant#1155
A PR was made on the 27th of March, so it's rather new and not-yet-included in the last servant's release (made last feb). So, I'd rather stick that to the blackboard and consider improving the API once servant 0.17 is available!

In the meantime, we can remove the 400 from the API specification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

^ added to https://github.com/input-output-hk/cardano-wallet/wiki/Blackboard

<<: *responsesErr404
<<: *responsesErr406
200:
description: Ok
schema: *ApiWallet

responsesDeleteWallet: &responsesDeleteWallet
<<: *responsesErr400
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here

<<: *responsesErr404
<<: *responsesErr406
<<: *responsesErr423
204:
description: No Content

Expand Down