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

Add support for optional arrays #471

Merged
merged 2 commits into from
Sep 25, 2020

Conversation

matthewmcgarvey
Copy link
Member

Fixes #470

There are a couple of bugs that are fixed by these changes:

  • You could not add an optional array column in a migration
  • Optional array fields on models were not converted from database queries correctly
  • Adding an array column to a table was adding "NOT NULL" to the add column sql statement even if it was not intended

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call on this! I think the implementation is definitely much cleaner this way. I'd like to see just 1 more spec for a little extra safety, and then I think we're good 👍

%from_db.as({{column[:type]}})
{% end %}
def {{column[:name]}} : {{column[:type]}}{{(column[:nilable] ? "?" : "").id}}
{{ db_type }}::Lucky.from_db!(@{{column[:name]}})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized not all types have a from_db! method. bool and uuid. I wonder if these are bugs that just haven't been caught yet 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they just use the default from_db! coming from Avram::Type https://github.com/luckyframework/avram/blob/master/src/avram/type.cr#L6

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohh... sneaky 😂

@@ -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 %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is much cleaner!

it "handles optional Array" do
BucketBox.create &.numbers(nil)
bucket = BucketQuery.new.last
bucket.numbers.should be_nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure it works, but just for peace of mind, can you add to this spec where we take this bucket and update it to have an empty array? Or maybe add another spec in here that just tests a SaveOperation can have a nilable array but be updated to an empty array?

@jwoertink jwoertink merged commit 4c5002c into luckyframework:master Sep 25, 2020
@matthewmcgarvey matthewmcgarvey deleted the optional-arrays branch September 25, 2020 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compilation errors when trying to make/add/use an optional array column
2 participants