Handle invalid GitHub token #42
Conversation
chshersh
added
type: feature
chshersh
requested review from
chshersh and
heathhenley
and removed request for
heathhenley
|
@heathhenley @loadre I've added you both as collaborators. Since we're starting to work more on this together, feel free to review PRs of each other as well. It's a nice practice 🙂 I haven't really invested a lot into making this project contributable. I'll add proper Code of Conduct and Contributing guide soon, so there'll be less uncertainty about future of this project and conflict resolution. |
|
@qexat Since you made a contribution, it now applies to you as well 😅 No pressure! |
lib/gh/client.ml
Outdated
| match response with | ||
| | Error (code, msg) -> Error (Curl_error { code = Curl.errno code; msg }) | ||
| | Ok response -> Ok response.body) | ||
| | Ok { body; _ } -> |
There was a problem hiding this comment.
Huh, GitHub sends an Ok response on bad credentials?
There was a problem hiding this comment.
utop # let query token query_body =
Ezcurl.post ~params:[]
~headers:[
("Authorization", "bearer " ^ token);
]
~content:(`String (Printf.sprintf "{ \"query\": %S }" query_body))
~url:"https://api.github.com/graphql" ()
;;
val query :
string -> string -> (Ezcurl.response, Curl.curlCode * string) result = <fun>
utop # query "foo" "query { viewer { login } }";;
- : (Ezcurl_core.response, Curl.curlCode * string) result =
Ok
{Ezcurl_core.code = 401;
...
body =
"{\n \"message\": \"Bad credentials\",\n \"documentation_url\": \"https://docs.github.com/graphql\",\n \"status\": \"401\"\n}\n";
...}
Github API sends 401 with plain HTTP request with curl, I think this might have something to do with how Ezcurl handles responses.
There was a problem hiding this comment.
@loadre Thanks for providing the example! I didn't know this about the internals of ezcurl.
lib/gh/client.ml
Outdated
|
|
||
| type error = | ||
| | No_github_token | ||
| | Bad_credentials |
There was a problem hiding this comment.
My only feedback: let's create the bad_credentials error type and put the message, documentation url and status code inside this type and decode it from JSON:
"{\"message\":\"Bad credentials\",\"documentation_url\":\"https://docs.github.com/graphql\",\"status\":\"401\"}"And then display this information in the UI. Let's try to avoid loosing potentially useful debugging information.
Also, I'm not sure if "Bad credentials" is the only possible message. At least, if we display the message, we'll be able to learn about different error scenarios
|
Pulled it down to test, looks great with no token and with a bad token 🚀 |
Update fmt_error to fit more possible error messages
…ub_token' into handle_invalid_github_token
| | Ok { body; code; _ } -> | ||
| let open Yojson.Basic in | ||
| let json = from_string body in | ||
| if Util.member "data" json <> `Null then Ok json |
There was a problem hiding this comment.
This part assumes that if response.body doesn't contain data, then it must contain message and documentation_url, which I'm not sure is a good decision or not.
Edit:
Just tested with malformed GQL query, got responses in the form of {"errors": [...]}. Maybe it deserves having its own issue (later on)?
There was a problem hiding this comment.
This part assumes that if response.body doesn't contain data, then it must contain message and documentation_url, which I'm not sure is a good decision or not.
Sounds good to me. Let's stick to it for now. We can always refine later.
Just tested with malformed GQL query, got responses in the form of {"errors": [...]}. Maybe it deserves having its own issue (later on)?
Yes, please! Feel free to create a separate issue. It's a good idea to handle such errors as well.
4 participants
closes #24