Ecto: Where with keyword list and nil

Created on 24 Feb 2017 · 10Comments · Source: elixir-ecto/ecto

Precheck

Do not use the issues tracker for help or support requests (try Stack Overflow, IRC or mailing lists, etc).
For proposing a new feature, please start a discussion on elixir-ecto.
For bugs, do a quick search and make sure the bug has not yet been reported.
Finally, be nice and have fun!

Environment

Elixir version (elixir -v): Elixir 1.3.1
Database and version (PostgreSQL 9.4, MongoDB 3.2, etc.): Postgres 9.6.1
Ecto version (mix reps): 2.1.2
Database adapter and version (mix reps): postgrex 0.13.0
Operating system: OS X 10.11.6

Current behavior

Using where with a keyword list that contains a nil value causes an error.

filters = [user_id: 1, some_attribute: nil]
from q in query, where: ^filters

** (ArgumentError) nil given for :some_attribute. Comparison with nil is forbidden as it is unsafe...

Expected behavior

Automatically deal with nils.

Source

maxsalven

Most helpful comment

Automatically dealing with nils would expose security risks. Imagine you have this a column which is an access_token. Users may have nil access_token if they are revoked or use. Now imagine you write this code:

conditions = [token: params[:token]]

If we automatically handle nil, by simply not passing the token, you have now given users the ability to access as any user in the database that does not have a token, usually the first.

josevalim on 24 Feb 2017

👍2

All 10 comments

conditions = [token: params[:token]]

If we automatically handle nil, by simply not passing the token, you have now given users the ability to access as any user in the database that does not have a token, usually the first.

josevalim on 24 Feb 2017

👍2

I just stumbled on the same issue.
I have to say I do understand both sides but I think that it is the responsibility of the developer to handle nil values.
I think it should be a warning that may be explicitly disabled by the developer.

My use case: I want to allow users filtering a resource with query parameters, for example:
GET /user/:id/posts?last_viewed_at=nil to receive posts that were not viewed before.
Of course, I want to allow this filtering in conjunction with other filters so I build a filter keyword list in the following manner:

  defp parse_value("nil"), do: nil
  defp parse_value(x), do: x

  defp parse_params(params) do
    params
    |> Map.take(["last_viewed_at","title","category"])
    |> Enum.reduce([], fn({k,v}, acc) ->
      Keyword.put(acc, String.to_atom(k), parse_value(v))
    end)
  end

This is an oversimplification of the situation but the idea is to create a filter keyword list and pass it directly to a from Post, where: ^filter_keyword_list to receive the filtered results

This is not achievable currently since nil values won't be accepted as values of the keyword list.
If I am missing an alternative, easy way to achieve that please share it with me, otherwise I think it should be allowed to explicitly ignore that error from being generated, considering this is a very classic use case for RESTful APIs.

Would love to hear your opinion.

erez-rabih on 19 Oct 2017

👍1

My first option would be to decoupled those two. If you want posts that have not been viewed before, have this be its own filter:

?unviewed=true

Then in your code you will do:

query =
  if params["unviewed"] do
    from q in query, where: is_nil(q.last_viewed_at)
  else
    query
  end

You can even keep it as you do today and change your ^filter_keyword_list to ignore any "nil" value and then another pass where you explicitly check for nils.

josevalim on 19 Oct 2017

I agree that's an option, I just think this over complicates a simple and basic operation on RESTful APIs.
On that same model, I have 5 fields marking either time or nil and that makes the query parsing very cumbersome.

Again, I do understand that the intention is good but why make this an error and not a warning?

erez-rabih on 22 Oct 2017

@erez-rabih because when you get it wrong the security implications are very drastic and in some cases may give an attacker access to someone else's account.

If you are doing it for 5 fields, you can still make it dynamic via a function:

query
|> tag_is_nil(:unviewed, params["unviewed"])
|> tag_is_nil(:another, params["another"])


defp tag_is_nil(query, field, nil) do
  from q in query, where: is_nil(field(q, ^field))
end

defp tag_is_nil(query, _field, _value) do
  query
end

Also, you also are not supposed to run on a system with warnings, so I don't see why a warning would make any difference.

josevalim on 22 Oct 2017

The field function you mentioned above may ease the pain here, thanks for taking the time to explain this.

erez-rabih on 22 Oct 2017

@josevalim do you mind if I create a PR that adds a notice about nil handling to Ecto.Query moduledoc? It seems like a common question people are asking and by doing a search by nil on Ecto.Query docs I wasn't able to find any notices on this behavior.