[Tentacat 2.0] Problems parsing JSON (github response)

[vermaxik]

Hi folks,

I've updated project to 2.0.0 version and got issue with creating new repo:

{400,
 %{
   "documentation_url" => "https://developer.github.com/v3/repos/#create",
   "message" => "Problems parsing JSON"
 },
 %HTTPoison.Response{
   body: %{
     "documentation_url" => "https://developer.github.com/v3/repos/#create",
     "message" => "Problems parsing JSON"
   }
...

Small debugging showed that post request generate request not correctly, it sends (look on body):

request: %HTTPoison.Request{
     body: "[{\"name\":\"test1\"},{\"private\":false}]",
     headers: [
       {"User-agent", "tentacat"},
       {"Authorization", "token token"}
     ],
     method: :post,
     options: [],
     params: %{},
     url: "https://api.github.com/user/repos"
   }

Expected behavior:

request: %HTTPoison.Request{
     body: "{\"name\":\"test1\",\"private\":false}",
     headers: [
       {"User-agent", "tentacat"},
       {"Authorization", "token token"}
     ],
     method: :post,
     options: [],
     params: %{},
     url: "https://api.github.com/user/repos"
   }

@niklaslong @edgurgel would you like to fix it, or I can help with a fix?

Thank you!

[edgurgel]

Hey @vermaxik ,

First thanks for opening this issue!

If you know what is the problem and want to send a PR that would be great. Otherwise I will try to fix this the next day or so as this will definitely impact other users.

[niklaslong]

@vermaxik Thanks for the debugging, that helps a lot. It seems to be an issue caused by keyword lists being passed through directly to the encoder rather than first being converted to a map-like structure.

@edgurgel I'd be happy to help investigate further!

[edgurgel]

Yeah I think the issue is that we were using keyword/lists as you said!

tentacat/lib/tentacat/repositories.ex

Line 113 in dad5e7a

post("orgs/#{org}/repos", client, List.flatten([name: repo], options))

We might need to transform them into maps first if they are Keywords

[niklaslong]

A fairly common pattern in the codebase seems to be accepting either a keyword list or a map as a body; this means that many requests will now be encoded incorrectly when the former is passed in.

I think it may be a good idea to convert keyword lists to maps in each function, thus keeping the current API intact? I also think using maps for the data intended as the body is better than keyword lists as it more accurately represents the JSON (no duplicate keys for instance).

[edgurgel]

Agreed, @niklaslong!

We might be fine by simply calling Map.new(kw_list) if the argument is a keyword list?

I wonder if we might need to use Keyword.keyword?(list) instead of just checking if it's a list ? I'm not sure if there's some GitHub API that accepts a JSON body as a single list/array?

body: ["a", "b", "c"]

[edgurgel]

We might want to use this to ensure that the request body is being matched? https://github.com/parroty/exvcr#matching-against-request-body

[niklaslong]

@edgurgel yes, we should definitely be testing against the request body!

I think we should check if it is a keyword list using Keyword.keyword?(list) just to be safe. After all, encoding a list would be entirely valid.

[niklaslong]

I think #194 is perhaps a good first step but some longer term changes will have to be implemented.

I think we should:

• test against the request body in all tests
• consider only accepting maps instead of keyword lists or maps, when passing in options for the body of the request. If I'm not mistaken, the API isn't consistent in this regard and homogenising it may be a nice refactor?

[edgurgel]

@niklaslong yeah I think it's the right time to break the API and accept only maps. 2.0.0 will be retired anyway as it's a broken release. We can release 2.0.1 having it with just maps?

[edgurgel]

@niklaslong I will see how far I can get changing this and I will open a PR.

[niklaslong]

@edgurgel sounds good. I'd be happy to help if needed, just let me know!

[edgurgel]

2.0.1 is out! Hopefully fixed all the issues. I've manually tested a few commands and they all seem to work fine and if a keyword is used it complains that a map is expected. Thanks, everyone!