opened
sdogruyol
username_0
Work in progress. Don't merge yet.
[WIP] Where IN Array support
issue
created
sdogruyol
username_0
The specs pass on Crystal `0.14.2` but seems to fail in an unrelated place in `0.15.0`
comment
created
sdogruyol
username_0
The specs passed but actually it's not working in a real app with Postgresql. It's related to query building with params so i converted the `Array(T)` generator like below.

```crystal
def _generate(query : ::Array(T), param_count = 0)
  query = query.map do |value|
     "#{value}"
  end.join(", ")
  # This is later being sent to _generate(query : ::ActiveRecord::Criteria, param_count = 0)
  Query.new("(#{query})")
end
```
comment
created
waterlink
username_1
Injecting values there directly is very "bad idea". Expect SQL injections.

Failures with Postgres adapter should be fixed by overriding the `_generate(...)` for special cases, that need different query generation on Postgres in the `postgres_adapter.cr`.
comment
created
waterlink
username_1
Oh, now it fails on `0.15.0` because we can't override now behavior of `!`. So we simply have to use `(...).not` now instead of `!(...)`
comment
created
waterlink
username_1
I will upgrade it to `0.15.0` in separate branch and make a release for it, when everything passes. After that you should rebase your commits on top of updated master branch
comment
created
sdogruyol
username_0
@username_1 i strongly agree on that :+1: For now i'm still getting the grasp of how this all works.
comment
created
waterlink
username_1
Yeah, postgres adapter actually already does some overriding there: https://github.com/username_1/postgres_adapter.cr/blob/master/src/postgres_adapter.cr#L10-L15

The same overload for `Array(T)` is required I assume.
comment
created
sdogruyol
username_0
@username_1 actually the problem is that `in([1,2,3])` is turned into `Criteria` and then later the params is not being merged into the query. 

Example.

```SELECT * FROM item_settings WHERE item_id IN (:1, :2, :3, :4)```

This gives SQL error since the syntax is not correct.
comment
created
waterlink
username_1
And maybe it makes sense to parametrize the code by the parameter name transformator and extract it to class of its own and re-use it from all implementations. Something like that:

```crystal
# in active_record
class ArrayQueryHandler
  def initialize(@param_name_transformer : Int32 -> String)
  end

  def handle(query : Array(T), param_count = 0)
    params = {} of String => T
    query = query.map do |value|
      param_count += 1
      params[param_count.to_s] = value
      @param_name_transformer.call(param_count)
    end.join(", ")
    {Query.new("(#{query})", params), param_count}
  end
end

# and usage in active_record:
def _generate(query : Array(T), param_count = 0)
  result, param_count = ArrayQueryHandler.new { |name| ":#{name}" }.handle(query)
  result
end

# and usage in prosgres_adapter
def _generate(query : Array(T), param_count = 0)
  result, param_count = ArrayQueryHandler.new { |name| "$#{name}" }.handle(query)
  result
end

# see closely at the difference in the block.
```
comment
created
waterlink
username_1
That sounds weird, it should not be converted to criteria. Could you update the branch (rebase against upstream master) and push it to github? So I can fetch it and play around to see what is happening.
comment
created
sdogruyol
username_0
@username_1 updated
comment
created
sdogruyol
username_0
I've add `ArrayQueryHandler` like you said :+1:
comment
created
sdogruyol
username_0
So just like you said adding below to `postgres_adapter` does it :tada: 

```crystal
def _generate(query : Array(T), param_count = 0)
    result, param_count = ::ActiveRecord::Sql::ArrayQueryHandler.new { |name| "$#{name}" }.handle(query)
    result
end
```
comment
created
waterlink
username_1
Great! I am going to merge this one then.
comment
created
sdogruyol
username_0
Thanks a lot :heart_eyes:
comment