update 4xx responses in API spec #252
Conversation
| @@ -681,15 +682,13 @@ responsesPostWallet: &responsesPostWallet | |||
| schema: *ApiWallet | |||
|
|
|||
| responsesGetWallet: &responsesGetWallet | |||
| <<: *responsesErr400 | |||
There was a problem hiding this comment.
no sure about this but I was not able to produce 400 for this
There was a problem hiding this comment.
What if you send an invalid wallet id ?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Hmmm... I'd expect the API to return a 400 for a illed-wallet id 🤔 ..
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ;)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
| <<: *responsesErr404 | ||
| <<: *responsesErr406 | ||
| 200: | ||
| description: Ok | ||
| schema: *ApiWallet | ||
|
|
||
| responsesDeleteWallet: &responsesDeleteWallet | ||
| <<: *responsesErr400 |
specifications/api/swagger.yaml
Outdated
| <<: *responsesErr404 | ||
| <<: *responsesErr406 | ||
| 200: | ||
| description: Ok | ||
| schema: *ApiWallet | ||
|
|
||
| responsesDeleteWallet: &responsesDeleteWallet | ||
| <<: *responsesErr400 | ||
| <<: *responsesErr404 | ||
| <<: *responsesErr406 | ||
| <<: *responsesErr423 |
There was a problem hiding this comment.
also don't know how to produce 423...
There was a problem hiding this comment.
I thought we would be more granular in the wallet error reporting but we are not there yet. I think for now, it'll just hang and wait for the resource to become available again.
There was a problem hiding this comment.
shall we remove it then?
piotr-iohk
force-pushed
the
piotr/220/update_api_spec
branch
from
b315ada to
4bab1e8
Compare
piotr-iohk
force-pushed
the
piotr/220/update_api_spec
branch
from
4bab1e8 to
fa378d7
Compare
Issue Number
Overview
Comments