Add support for optional arrays (#471)

* Add support for optional arrays * Update spec to verify adding and removing nilable array
luckyframework · Sep 25, 2020 · 4c5002c · 4c5002c
1 parent 60ebb71
commit 4c5002c
Show file tree

Hide file tree

Showing 7 changed files with 44 additions and 38 deletions.
diff --git a/db/migrations/20190712145900_create_buckets_with_array_types.cr b/db/migrations/20190712145900_create_buckets_with_array_types.cr
@@ -5,7 +5,7 @@ class CreateBucketsWithArrayTypes::V20190712145900 < Avram::Migrator::Migration:
       add_timestamps
       add bools : Array(Bool)
       add small_numbers : Array(Int16)
-      add numbers : Array(Int32)
+      add numbers : Array(Int32)?
       add big_numbers : Array(Int64)
       add names : Array(String)
     end

diff --git a/spec/array_column_spec.cr b/spec/array_column_spec.cr
@@ -1,5 +1,9 @@
 require "./spec_helper"
 
+private class SaveBucket < Bucket::SaveOperation
+  permit_columns numbers
+end
+
 describe "Array Columns" do
   it "fails when passing a single value to an array query" do
     BucketBox.new.numbers([1, 2, 3]).create
@@ -18,4 +22,14 @@ describe "Array Columns" do
     bucket = BucketQuery.new.last
     bucket.floaty_numbers.should eq([1.1, 2.2, 3.3, 4.4])
   end
+
+  it "handles optional Array" do
+    BucketBox.create &.numbers(nil)
+    bucket = BucketQuery.new.last
+    bucket.numbers.should be_nil
+    bucket = SaveBucket.update!(bucket, numbers: [1, 2, 3])
+    bucket.numbers.should eq([1, 2, 3])
+    bucket = SaveBucket.update!(bucket, numbers: nil)
+    bucket.numbers.should be_nil
+  end
 end
diff --git a/spec/migrator/alter_table_statement_spec.cr b/spec/migrator/alter_table_statement_spec.cr
@@ -41,7 +41,7 @@ describe Avram::Migrator::AlterTableStatement do
       ADD updated_at timestamptz,
       ADD future_time timestamptz NOT NULL DEFAULT '#{Time.local.to_utc}',
       ADD new_id uuid NOT NULL DEFAULT '46d9b2f0-0718-4d4c-a5a1-5af81d5b11e0',
-      ADD numbers int[] NOT NULL,
+      ADD numbers int[],
       DROP old_column,
       DROP employee_id;
     SQL

diff --git a/spec/support/bucket.cr b/spec/support/bucket.cr
@@ -3,7 +3,7 @@ class Bucket < BaseModel
   table do
     column bools : Array(Bool)
     column small_numbers : Array(Int16)
-    column numbers : Array(Int32)
+    column numbers : Array(Int32)?
     column big_numbers : Array(Int64)
     column names : Array(String)
     column floaty_numbers : Array(Float64)

diff --git a/src/avram/migrator/alter_table_statement.cr b/src/avram/migrator/alter_table_statement.cr
@@ -112,18 +112,17 @@ class Avram::Migrator::AlterTableStatement
   end
 
   macro add(type_declaration, index = false, using = :btree, unique = false, default = nil, fill_existing_with = nil, **type_options)
-    {% if type_declaration.type.is_a?(Union) %}
-      {% type = type_declaration.type.types.first %}
+    {% type = type_declaration.type %}
+    {% nilable = false %}
+    {% array = false %}
+    {% should_fill_existing = fill_existing_with && (fill_existing_with != :nothing) %}
+    {% if type.is_a?(Union) %}
+      {% type = type.types.first %}
       {% nilable = true %}
-      {% array = false %}
-    {% elsif type_declaration.type.is_a?(Generic) %}
-      {% type = type_declaration.type.type_vars.first %}
-      {% nilable = false %}
+    {% end %}
+    {% if type.is_a?(Generic) %}
+      {% type = type.type_vars.first %}
       {% array = true %}
-    {% else %}
-      {% type = type_declaration.type %}
-      {% nilable = (fill_existing_with != nil) && (fill_existing_with != :nothing) %}
-      {% array = false %}
     {% end %}
 
     {% if !nilable && default == nil && fill_existing_with == nil %}
@@ -133,9 +132,9 @@ class Avram::Migrator::AlterTableStatement
 
         Try one of these...
 
-          ▸ add #{type_declaration.var} : #{type}, default: "Something"
-          ▸ add #{type_declaration.var} : #{type}, fill_existing_with: "Something"
-          ▸ add #{type_declaration.var} : #{type}, fill_existing_with: :nothing
+          ▸ add #{type_declaration}, default: "Something"
+          ▸ add #{type_declaration}, fill_existing_with: "Something"
+          ▸ add #{type_declaration}, fill_existing_with: :nothing
         ERROR
       %}
     {% end %}
@@ -145,10 +144,10 @@ class Avram::Migrator::AlterTableStatement
     {% end %}
 
     rows << Avram::Migrator::Columns::{{ type }}Column(
-    {% if array %}Array({% end %}{{ type }}{% if array %}){% end %}
+    {% if array %}Array({{ type }}){% else %}{{ type }}{% end %}
     ).new(
       name: {{ type_declaration.var.stringify }},
-      nilable: {{ nilable }},
+      nilable: {{ nilable || should_fill_existing }},
       default: {{ default }},
       {{ **type_options }}
     )
@@ -157,12 +156,12 @@ class Avram::Migrator::AlterTableStatement
     {% end %}
     .build_add_statement_for_alter
 
-    {% if fill_existing_with && fill_existing_with != :nothing %}
+    {% if should_fill_existing %}
       add_fill_existing_with_statements(
         column: {{ type_declaration.var.stringify }},
         type: {{ type }},
         value: Avram::Migrator::Columns::{{ type }}Column.prepare_value_for_database({{ fill_existing_with }}),
-        nilable: {{ type_declaration.type.is_a?(Union) }}
+        nilable: {{ nilable }}
       )
     {% end %}
 

diff --git a/src/avram/migrator/create_table_statement.cr b/src/avram/migrator/create_table_statement.cr
@@ -77,22 +77,20 @@ class Avram::Migrator::CreateTableStatement
   end
 
   macro add(type_declaration, default = nil, index = false, unique = false, using = :btree, **type_options)
-    {% if type_declaration.type.is_a?(Union) %}
-      {% type = type_declaration.type.types.first %}
+    {% type = type_declaration.type %}
+    {% nilable = false %}
+    {% array = false %}
+    {% if type.is_a?(Union) %}
+      {% type = type.types.first %}
       {% nilable = true %}
-      {% array = false %}
-    {% elsif type_declaration.type.is_a?(Generic) %}
-      {% type = type_declaration.type.type_vars.first %}
-      {% nilable = false %}
+    {% end %}
+    {% if type.is_a?(Generic) %}
+      {% type = type.type_vars.first %}
       {% array = true %}
-    {% else %}
-      {% type = type_declaration.type %}
-      {% nilable = false %}
-      {% array = false %}
     {% end %}
 
     rows << Avram::Migrator::Columns::{{ type }}Column(
-    {% if array %}Array({% end %}{{ type }}{% if array %}){% end %}
+    {% if array %}Array({{ type }}){% else %}{{ type }}{% end %}
     ).new(
       name: {{ type_declaration.var.stringify }},
       nilable: {{ nilable }},

diff --git a/src/avram/model.cr b/src/avram/model.cr
@@ -247,13 +247,8 @@ abstract class Avram::Model
   macro setup_getters(columns, *args, **named_args)
     {% for column in columns %}
       {% db_type = column[:type].is_a?(Generic) ? column[:type].type_vars.first : column[:type] %}
-      def {{column[:name]}} : {% if column[:nilable] %}::Union({{db_type}}, ::Nil){% else %}{{column[:type]}}{% end %}
-        %from_db = {{ db_type }}::Lucky.from_db!(@{{column[:name]}})
-        {% if column[:nilable] %}
-          %from_db.as?({{db_type}})
-        {% else %}
-          %from_db.as({{column[:type]}})
-        {% end %}
+      def {{column[:name]}} : {{column[:type]}}{{(column[:nilable] ? "?" : "").id}}
+        {{ db_type }}::Lucky.from_db!(@{{column[:name]}})
       end
       {% if column[:type].id == Bool.id %}
       def {{column[:name]}}? : Bool