Support custom field types

[Blacksmoke16]

Initial pass at supporting custom DB types. Resolves #193 and resolves #152.

• Allows fields to have converters to convert from their actual type, to a type that could be used by the DB.
  • Serializing a class to json on save, but deserializing it from json on read.
• Adds converters for UUID, Enum, JSON, and PG::Numeric.
  • Refactors the current auto UUID implementation to use the new converter
• This DOESNT address the issue that Support Adapter-specific types #152 mentions in that the union of supported types is limited and fixed. But it would help alleviate the more pressing limitations with the current implementation. I.e. would allow for easily handling the conversion of PG::Numeric into Float64, or a String into an actual UUID object when saving/reading from the DB.
• Allows the user to specify the actual data type of eac field. Only useful ATM for the migrator for specs.

TODO:

• Update docs

[drujensen]

lgtm 💯

[drujensen]

Ok. I’m fine with it the way it is.

…

On Jun 16, 2019, at 10:22 AM, Blacksmoke16 ***@***.***> wrote: @Blacksmoke16 commented on this pull request. In src/granite/converters.cr: > @@ -0,0 +1,11 @@ +module Granite::Converters Hmm, I dunno. That seems like a lot more work than just adding a , converter: Granite::Converters::UuidConverter to the field. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

[Blacksmoke16]

Currently planning on implementing the following converters:

• UUID - Both binary and string formats.
• Enum - Both integer and string formats.
• JSON - Both binary and string formats
• PG::Numeric - Allows using types like DECIMAL in PG.

Any others you think that would be helpful?

[drujensen]

@Blacksmoke16 Sounds like a good plan. We can always add more converters later.

[Blacksmoke16]

@drujensen What are your thoughts on how we should handle the different return values from each DB driver? I.e. PG returns JSON::Any for its JSON(B) types, but SQLite just returns a String for its TEXT type, which messes up the reads.

I'm thinking we can:

1. Have global converters to handle each case, i.e. BinaryJson, StringJson, Json, for Bytes, String, and JSON::Any.
2. Handle each case but namespace the converters by driver to handle the slight differences, which gets included when including that adapter. i.e. Granite::Converters::PG::Json.
3. Similar to 2, but only support driver specific types, i.e. for PG, only support JSON(B) types, and not String or Bytes column types; for SQLite support String/Blob.

Option 1 would have the most flexibility, but also the most converters.
Option 2 would mostly be 3 near identical sets based on option 1.
Option 3 would have the least amount of duplication, but also the least flexible.

[drujensen]

I’m leaning toward option 1. It seems the most flexible.

…

On Jun 18, 2019, at 6:18 PM, Blacksmoke16 ***@***.***> wrote: @drujensen What are your thoughts on how we should handle the different return values from each DB driver? I.e. PG returns JSON::Any for its JSON(B) types, but SQLite just returns a string, which messes up the read. I'm thinking we can either: Have global converters to handle each case, i.e. BinaryJson, StringJson, Json, for Bytes, String, and JSON::Any. Namespace the converters by driver, which gets included when including that adapter. i.e. Granite::Converters::PG::Json. Similar to 2, but only support driver specific types, i.e. for PG, only support JSON(B) types, and not String or BYTEA types; for SQLite support String/Blob. Option 1 would have the most flexibility, but also the most converters. Option 2 would mostly be 3 near identical sets based on option 1. Option 3 would have the least amount of duplication, but also the least flexible. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[drujensen]

Is there a way to get rid of the large case statement to perform type conversions using a set of converters?

…

On Jun 18, 2019, at 6:27 PM, Dru Jensen ***@***.***> wrote: I’m leaning toward option 1. It seems the most flexible. > On Jun 18, 2019, at 6:18 PM, Blacksmoke16 ***@***.***> wrote: > > @drujensen What are your thoughts on how we should handle the different return values from each DB driver? I.e. PG returns JSON::Any for its JSON(B) types, but SQLite just returns a string, which messes up the read. > > I'm thinking we can either: > > Have global converters to handle each case, i.e. BinaryJson, StringJson, Json, for Bytes, String, and JSON::Any. > Namespace the converters by driver, which gets included when including that adapter. i.e. Granite::Converters::PG::Json. > Similar to 2, but only support driver specific types, i.e. for PG, only support JSON(B) types, and not String or BYTEA types; for SQLite support String/Blob. > Option 1 would have the most flexibility, but also the most converters. > Option 2 would mostly be 3 near identical sets based on option 1. > Option 3 would have the least amount of duplication, but also the least flexible. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub, or mute the thread.

[Blacksmoke16]

Probably yea. But prob best for another PR.

I'm thinking you would lose some flexibility though, but I'd be ok with that. I.e. you couldn't pass "yes" to a Bool field and expect it to be converted correctly.

EDIT: Well maybe you could, have think about it more.

[Blacksmoke16]

@drujensen One thing I thought of that would help cut down on the amount of duplication, would be to have the user provide the type that should be read from the column.

Granite::Converters::Json(String) or Granite::Converters::Json(Bytes)

Then you would have a singular Json converter module that just defines the to/from methods based on the type of that generic.

module Json(T)
  {% if T == String %}
    # Add from_rs/to_db methods that go to/from a String
  {% elsif T == JSON::Any %}
    # Add from_rs/to_db methods that go to/from JSON::Any
  {% elsif T == Bytes %}
    # Add from_rs/to_db methods that go to/from Bytes
  {% else %}
    {% raise "#{{{@type.name}}} does not currently support #{T}" %}
  {% end %}
end

Is still going to be some duplication, but this would at least limit the total number of converters and keep the code centralized.

[drujensen]

@Blacksmoke16 Yes, good idea. Another option is having just one converter that supports a Union of (String | Bytes | JSON::Any). then add logic to check the type before converting.

[Blacksmoke16]

Hmm, I'll have to try that how to see how it works with rs.read.

[Blacksmoke16]

@drujensen I tested it out, it worked pretty good for the reading from the db, however you wouldn't really have a way to know what to do with the value on save. I.e. for an enum, should I call .to_i or .to_s?

But as it seems, I don't have access to the generic type in macro land either :/

EDIT:

module Enum(E, T)
  extend self

  def self.to_db(value : E?) : Granite::Fields::Type
    return nil if value.nil?
    {% if T <= Number %}
      value.to_i
    {% elsif T == String %}
      value.to_s
    {% else %}
      {% raise "#{@type.name}#to_db does not support #{T} yet." %}
    {% end %}
  end

  def self.from_rs(result : ::DB::ResultSet) : E?
    value = result.read(T?)
    return nil if value.nil?
    {% if T <= Number %}
      E.from_value? value
    {% elsif T == String %}
      E.parse? value
    {% else %}
      {% raise "#{@type.name}#from_rs does not support #{T} yet." %}
    {% end %}
  end
end

This will work pretty good tho.

[Blacksmoke16]

@drujensen @robacarp This should be good for final review.

EDIT: Can review but I need to update docs first before merging if everything looks good.

EDIT2: Docs are updated.

[drujensen]

Nice! 💯

[drujensen]

@amberframework/contributors Any other feedback? If not, I will merge.