Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to use custom converter #197

Open
dukeraphaelng opened this issue Nov 25, 2020 · 3 comments
Open

Unable to use custom converter #197

dukeraphaelng opened this issue Nov 25, 2020 · 3 comments
Labels

Comments

@dukeraphaelng
Copy link
Contributor

dukeraphaelng commented Nov 25, 2020

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.

@anykeyh anykeyh added the bug label Nov 25, 2020
@dukeraphaelng
Copy link
Contributor Author

dukeraphaelng commented Nov 25, 2020

Some more explication of this issue:

    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)

@dukeraphaelng
Copy link
Contributor Author

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

@anykeyh
Copy link
Owner

anykeyh commented Nov 26, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants