Unable to use custom converter

Question

Unable to use custom converter

dukenguyenxyz opened this issue 4 years ago · 3 comments

I have the following example which implements custom converter.

class House
  include Clear::Model
  column id : Int64, primary: true, presence: false
  column rooms : Hash(String, Int32), converter: "Hash(String, Int32)"
  column available : Bool?
end

module Clear::Model::Converter::CountsConverter
  def self.to_column(x) : Hash(String, Int32)?
    case x
    when Nil
      nil
    when ::JSON::Any
      x.as_h?.try do |hash|
        counts = hash.transform_values &.as_i?
        counts.has_value?(nil) ? nil : counts.compact
      end
    when Hash(String, Int32)
      x
    else
      raise "Cannot convert from #{x.class} to Hash(String, Int32)"
    end
  end

  def self.to_db(x : Hash(String, Int32)?) : Clear::SQL::Any
    x.to_json
  end
end

Clear::Model::Converter.add_converter("Hash(String, Int32)", Clear::Model::Converter::CountsConverter)
Clear::Model::Converter.add_converter("Hash(String, Int32) | Nil", Clear::Model::Converter::CountsConverter)

describe "Clear::Model::Converter", focus: true do
  it "should create a new model with a field from a new converter with a new type" do
    body = {
      "rooms" => {
        "bed"     => 4,
        "bath"    => 3,
        "kitchen" => 1,
      },
      "available" => true,
    }
    house = House.new(body)
    house.rooms.should eq(body["rooms"])
    house.available.should eq(true)
  end
end

However the following error is thrown (here is the full stack trace)

error in line 25
Error: while requiring "./spec/model/converters/custom_converter_spec.cr"


In spec/model/converters/custom_converter_spec.cr:43:19

 43 | house = House.new(body)
                    ^--
Error: instantiating 'House.class#new(Hash(String, Bool | Hash(String, Int32)))'


In spec/model/converters/custom_converter_spec.cr:2:3

 2 | include Clear::Model
     ^
Error: expanding macro


There was a problem expanding macro 'included'

Called macro defined in src/clear/model/model.cr:45:3

 45 | macro included

Which expanded to:

 >  1 |         # <~ Models are mutable objects;
 >  2 | 
 >  3 |     extend Clear::Model::HasHooks::ClassMethods
 >  4 | 
 >  5 |     getter cache : Clear::Model::QueryCache?
 >  6 | 
 >  7 |     def initialize
 >  8 |       @persisted = false
 >  9 |     end
 > 10 | 
 > 11 |     def initialize(h : Hash(String, _), @cache : Clear::Model::QueryCache? = nil, @persisted = false, fetch_columns = false )
 > 12 |       @attributes.merge!(h) if fetch_columns
 > 13 | 
 > 14 |       reset(h)
 > 15 |     end
 > 16 | 
 > 17 |     def initialize(json : ::JSON::Any, @cache : Clear::Model::QueryCache? = nil, @persisted = false )
 > 18 |       reset(json.as_h)
 > 19 |     end
 > 20 | 
 > 21 |     def initialize(t : NamedTuple, @persisted = false)
 > 22 |       reset(t)
 > 23 |     end
 > 24 | 
 > 25 |     # Invalidate local-to-relation cache and eager-loading cache.
 > 26 |     # Useful to forcefully query again when calling relation defined method
 > 27 |     def invalidate_caches : self
 > 28 |       @cache = nil
 > 29 |       self
 > 30 |     end
 > 31 | 
 > 32 |     # :nodoc:
 > 33 |     # This is a tricky method which is overriden by inherited models.
 > 34 |     #
 > 35 |     # The problem is usage of static array initialisation under `finalize` macro; they are initialized
 > 36 |     # AFTER the main code is executed, preventing it to work properly.
 > 37 |     #
 > 38 |     # The strategy here is to execute the static array initialization under a method and execute this method
 > 39 |     # before main.
 > 40 |     #
 > 41 |     # Then to redefine this method in the finalize block. The current behavior seems to be a crystal compiler bug
 > 42 |     # and should be fixed in near future.
 > 43 |     def self.__main_init__
 > 44 |     end
 > 45 |   
Error: instantiating 'Hash(String, Array(JSON::Any) | Array(PG::BoolArray) | Array(PG::CharArray) | Array(PG::Float32Array) | Array(PG::Float64Array) | Array(PG::Int16Array) | Array(PG::Int32Array) | Array(PG::Int64Array) | Array(PG::NumericArray) | Array(PG::StringArray) | Array(PG::TimeArray) | BigDecimal | Bool | Char | Clear::Expression::UnsafeSql | Float32 | Float64 | Hash(String, JSON::Any) | Int16 | Int32 | Int64 | Int8 | JSON::Any | PG::Geo::Box | PG::Geo::Circle | PG::Geo::Line | PG::Geo::LineSegment | PG::Geo::Path | PG::Geo::Point | PG::Geo::Polygon | PG::Interval | PG::Numeric | Slice(UInt8) | String | Time | UInt16 | UInt32 | UInt64 | UInt8 | UUID | Nil)#merge!(Hash(String, Bool | Hash(String, Int32)))'


In /usr/local/Cellar/crystal/0.35.1_1/src/hash.cr:1417:11

 1417 | other.each do |k, v|
              ^---
Error: instantiating 'Hash(String, Bool | Hash(String, Int32))#each()'


In /usr/local/Cellar/crystal/0.35.1_1/src/hash.cr:1259:5

 1259 | each_entry_with_index do |entry, i|
        ^--------------------
Error: instantiating 'each_entry_with_index()'


In /usr/local/Cellar/crystal/0.35.1_1/src/hash.cr:1259:5

 1259 | each_entry_with_index do |entry, i|
        ^--------------------
Error: instantiating 'each_entry_with_index()'


In /usr/local/Cellar/crystal/0.35.1_1/src/hash.cr:1417:11

 1417 | other.each do |k, v|
              ^---
Error: instantiating 'Hash(String, Bool | Hash(String, Int32))#each()'


In /usr/local/Cellar/crystal/0.35.1_1/src/hash.cr:1418:11

 1418 | self[k] = v
            ^
Error: no overload matches 'Hash(String, Array(JSON::Any) | Array(PG::BoolArray) | Array(PG::CharArray) | Array(PG::Float32Array) | Array(PG::Float64Array) | Array(PG::Int16Array) | Array(PG::Int32Array) | Array(PG::Int64Array) | Array(PG::NumericArray) | Array(PG::StringArray) | Array(PG::TimeArray) | BigDecimal | Bool | Char | Clear::Expression::UnsafeSql | Float32 | Float64 | Hash(String, JSON::Any) | Int16 | Int32 | Int64 | Int8 | JSON::Any | PG::Geo::Box | PG::Geo::Circle | PG::Geo::Line | PG::Geo::LineSegment | PG::Geo::Path | PG::Geo::Point | PG::Geo::Polygon | PG::Interval | PG::Numeric | Slice(UInt8) | String | Time | UInt16 | UInt32 | UInt64 | UInt8 | UUID | Nil)#[]=' with types String, (Bool | Hash(String, Int32))

Overloads are:
 - Hash(K, V)#[]=(key : K, value : V)
Couldn't find overloads for these types:
 - Hash(String, Array(JSON::Any) | Array(PG::BoolArray) | Array(PG::CharArray) | Array(PG::Float32Array) | Array(PG::Float64Array) | Array(PG::Int16Array) | Array(PG::Int32Array) | Array(PG::Int64Array) | Array(PG::NumericArray) | Array(PG::StringArray) | Array(PG::TimeArray) | BigDecimal | Bool | Char | Clear::Expression::UnsafeSql | Float32 | Float64 | Hash(String, JSON::Any) | Int16 | Int32 | Int64 | Int8 | JSON::Any | PG::Geo::Box | PG::Geo::Circle | PG::Geo::Line | PG::Geo::LineSegment | PG::Geo::Path | PG::Geo::Point | PG::Geo::Polygon | PG::Interval | PG::Numeric | Slice(UInt8) | String | Time | UInt16 | UInt32 | UInt64 | UInt8 | UUID | Nil)#[]=(key : String, value : Hash(String, Int32))

I think this error is due to Clear::SQL::Any not including the custom converter type.

Answer 1 · 2020-11-25T07:47:57.000Z

Some more explication of this issue:

The custom converter as given in the example in the manual (https://clear.gitbook.io/project/model/column-types/converters) works perfectly fine (see #199)
The error above could be resolved if Hash(String, Int32) is added to the alias/constant Clear::SQL::Any, i.e. as in

    alias Any = Array(PG::BoolArray) | Array(PG::CharArray) | Array(PG::Float32Array) |
                Array(PG::Float64Array) | Array(PG::Int16Array) | Array(PG::Int32Array) |
                Array(PG::Int64Array) | Array(PG::StringArray) | Array(PG::TimeArray) |
                Array(PG::NumericArray) |
                Bool | Char | Float32 | Float64 | Int8 | Int16 | Int32 | Int64 | BigDecimal | JSON::Any | JSON::Any::Type | PG::Geo::Box | PG::Geo::Circle |
                PG::Geo::Line | PG::Geo::LineSegment | PG::Geo::Path | PG::Geo::Point |
                PG::Geo::Polygon | PG::Numeric | PG::Interval | Slice(UInt8) | String | Time |
                UInt8 | UInt16 | UInt32 | UInt64 | Clear::Expression::UnsafeSql | UUID |
                Nil | Hash(String, Int32)

Answer 2 · 2020-11-26T01:17:07.000Z

Bug investigation update:

This seems to be more of a problem with

src/clear/model/model.cr:45:3

 > 11 |     def initialize(h : Hash(String, _), @cache : Clear::Model::QueryCache? = nil, @persisted = false, fetch_columns = false )
 > 12 |       @attributes.merge!(h) if fetch_columns
 > 13 | 
 > 14 |       reset(h)
 > 15 |     end

specifically

@attributes.merge!(h) if fetch_columns

than it is with the Clear::SQL::Any or the Converter module.

This is because when House.new(body) body param is a NamedTuple instead of a Hash then the above code works without any need for modification of Clear::SQL::Any (the following code passes spec)

  it "should create a new model with a field from a new converter from NamedTuple", focus: true do
    body = {
      rooms: {
        "bed"     => 4,
        "bath"    => 3,
        "kitchen" => 1,
      },
      available: true,
    }
    house = House.new(body)
    house.rooms.should eq(body["rooms"])
    house.available.should eq(true)
  end

Answer 3 · 2020-11-26T09:56:04.000Z

Constructor by hash should not be used; it is used internally with recordset but should not be used outside to feed the model like you do.

The problem lay in the fact we don't have as much information about the value for each key in comparison to a NamedTuple.

For NamedTuple I can get each column type easily, as NamedTuple are defined like NamedTuple(col1: String, col2: OtherClasss, col3: ...) so you know which column is expecting which kind of value.
In hash, you will get Hash(String, String|OtherClass|...) where the value is a union of each type encountered when you defined your hash.

I'm not sure there's anything I can do, as I think this is a limit to Crystal language itself. I would say: don't use Hash in this case, but use from/to_json serialization for your model.

You could also create a static method from_hash which take the hash. You will then notice that you need to cast the parameters of you hash like this:

  def self.from_hash(hash)
     mdl = new(rooms: hash[:rooms].as(Hash(String, Int32)) # < this is the casting Clear is not aware of when you pass a hash.
  end