nadia icon indicating copy to clipboard operation
nadia copied to clipboard

Fixes and addition in InputMessegeContent and Parser modules

Open anronin opened this issue 9 years ago • 19 comments

  1. Bug fix in Nadia.Model.InputMessegeContent module where optional fields must be string instead of nil

  2. Fixes and additions in Nadia.Parser after the new models were introduced

anronin avatar May 30 '16 07:05 anronin

What's the problem if optional fields of Nadia.Model.InputMessegeContent are nil?

zhyu avatar May 31 '16 06:05 zhyu

If its nil and not used then you get error message like that: "Bad request: Field "last_name" must be of type String" from telegram server

anronin avatar May 31 '16 06:05 anronin

this is response from server when it's nil

{:ok, %HTTPoison.Response{body: "{\"ok\":false,\"error_code\":400,\"description\":\"Bad request: Field \\\"last_name\\\" must be of type String\"}", headers: [{"Server", "nginx/1.10.0"}, {"Date", "Tue, 31 May 2016 07:07:08 GMT"}, {"Content-Type", "application/json"}, {"Content-Length", "101"}, {"Connection", "keep-alive"}, {"Access-Control-Allow-Origin", "*"}, {"Access-Control-Expose-Headers", "Content-Length,Content-Type,Date,Server,Connection"}], status_code: 400}}

and params are like that [inline_query_id: "13947328571332811", results: "[{\"type\":\"article\",\"title\":\"Test\",\"input_message_content\":{\"phone_number\":\"19232114412\",\"last_name\":null,\"first_name\":\"Arbuz\"},\"id\":\"1\"},{\"type\":\"article\",\"title\":\"Test2\",\"input_message_content\":{\"phone_number\":\"19232114412\",\"last_name\":null,\"first_name\":\"Dyna\"},\"id\":\"2\"}]"]

anronin avatar May 31 '16 07:05 anronin

Well, instead of using empty string for optional field, I prefer to remove nil fields in request params.

zhyu avatar May 31 '16 07:05 zhyu

I do not quite understand where to do it: in Nadia.answer_inline_query or Nadia.API.build_request ?

anronin avatar May 31 '16 08:05 anronin

I think this issue relates to this line: https://github.com/zhyu/nadia/blob/master/lib/nadia.ex#L635

Only nil field in the top level has been removed, a nil field in a non-nil field will not been removed. e.g.,

%{nil_field: nil, non_nil_field: %{nil_field: nil, non_nil_field: "something"}}

will be changed to

%{non_nil_field: %{nil_field: nil, non_nil_field: "something"}}

if we change it to

%{non_nil_field: %{non_nil_field: "something"}}

there won't be any problem.

So I think removing nil fields recursively will be a solution. And since nil fields are very common, I think it's better to do it in Nadia.API.build_request.

zhyu avatar May 31 '16 08:05 zhyu

not sure that is right solution) EDIT: just a moment, this is not right solution EDIT2: still not sure that is right solution, but at least it works. i'm think so

anronin avatar May 31 '16 10:05 anronin

wdyt?

anronin avatar Jun 02 '16 09:06 anronin

The result should look something like this?

defp drop_nil_fields(params) when is_list(params), do: Enum.map(params, &drop_nil_fields/1)
defp drop_nil_fields(params) when is_map(params) do
    params
    |> Map.from_struct
    |> Enum.filter(fn {_, v} -> v != nil end)
    |> Enum.into(%{})
    |> Poison.encode!
  end
defp drop_nil_fields(params), do: to_string(params)

EDIT: no, this is not working

anronin avatar Jun 02 '16 10:06 anronin

in drop_nil_fields for a map, you should call drop_nil_fields on values of the map, not just return the non-nil value.

drop_nil_fields should remove any nil fields recursively, it means you should call recursively on every item of a list, and every value of a map.

EDIT: so just add |> Enum.map(&drop_nil_fields/1) after this line: |> Enum.filter(fn {_, v} -> v != nil end), or you can use filter_map

zhyu avatar Jun 02 '16 10:06 zhyu

so like this? test go fine, but i have a problem with this

defp drop_nil_fields(params) when is_list(params), do: Enum.map(params, &drop_nil_fields/1)
defp drop_nil_fields(params) when is_map(params) do
    params
    |> Map.from_struct
    |> Enum.filter_map(fn {_, v} -> v end, fn {k, v} -> {k, drop_nil_fields(v)} end)
    |> Enum.into(%{})
    |> Poison.encode!
  end
defp drop_nil_fields(params), do: to_string(params)

anronin avatar Jun 02 '16 10:06 anronin

if i get it right |> Poison.encode! must be in drop_nil_fields for list

anronin avatar Jun 02 '16 11:06 anronin

how about that?

anronin avatar Jun 02 '16 12:06 anronin

only problem is that reply_markup: nil not removed

anronin avatar Jun 03 '16 06:06 anronin

i could add |> Keyword.delete(:reply_markup, nil) after |> Keyword.update(:reply_markup, nil, &(Poison.encode!(&1))) in build_request of course

anronin avatar Jun 03 '16 10:06 anronin

any thoughts?

anronin avatar Jun 09 '16 06:06 anronin

Sorry for keeping you waiting 😿 However, this p-r is outdated. I may check and merge it when conflicts are resolved.

zhyu avatar Mar 05 '18 06:03 zhyu

np, i'm glad you are back :) will look at this asap

anronin avatar Mar 05 '18 07:03 anronin

Thanks for your reply, please take your time 😺 After resolving conflicts, please use the formatter to format your code too, you can find details in #60. I'll update .travis.yml to check it 🔜

zhyu avatar Mar 06 '18 06:03 zhyu