Default array value is double-quoted/escaped

[manuelmeurer]

When I set the default value for an array text column via the Attributes API, the annotate output is double-quoted (or quoted and escaped?).

attribute :notifications, default: %w(newsletter)

generates

#  notifications :text             default(["\"newsletter\""]), not null, is an Array

Commands

$ bundle exec annotaterb models

Version

• annotaterb version: 4.4.0
• rails version: 6.1.7.6
• ruby version: 3.2.2

[drwl]

Hi @manuelmeurer, thanks for taking the time to report an issue! I'll take a further look into this soon. For reference, what database/adapter are you using?

[manuelmeurer]

Postgres 15. Thanks for taking a look!

It has something to do with

annotaterb/lib/annotate_rb/model_annotator/column_annotation/column_wrapper.rb

Line 98 in dc95dff

when Array then value.map { |v| quote(v) }

but I couldn't figure it out...

[drwl]

Could you share what the line used to described that field in schema.rb?

This is what I'm using while debugging and wanted to double check it matches:

# schema.rb
create_table "test_cases", force: :cascade do |t|
    t.datetime "created_at", null: false
    t.datetime "updated_at", null: false
    t.string "default_string_array", default: ["something"], array: true
  end

# test_case.rb
class TestCase < ApplicationRecord
  attribute :default_string_array, default: %w(newsletter)
end

[manuelmeurer]

The default is set to [] in the migration and schema (I'm not a fan of setting defaults in the DB), only via the Attributes API to %w(newsletter)

# migration
create_table :mytable do |t|
  t.text :notifications, array: true, default: [], null: false
end

# schema
create_table "mytable", force: :cascade do |t|
  t.text "notifications", default: [], null: false, array: true
end

# model
attribute :notifications, default: %w(newsletter)

Another questions is whether annotate should show the default as ["newsletter"] in this case at all... now that I think about it, I think it should be [] since that is the migration/schema default.

[drwl]

Not sure if you saw the tag in the PR but if you have the time and are able to test the branch, I'd be happy to merge it in after.

Another questions is whether annotate should show the default as ["newsletter"] in this case at all... now that I think about it, I think it should be [] since that is the migration/schema default.

This is a good question. The way it's currently implemented is that it reads the value for a specific column from the Model.column_defaults which returns a hash. Rails populates the hash with defaults, first reading from the database (and not schema.rb) [1] and then any overrides, which attribute does.

This poses an interesting scenario, where if someone changes the default option in the attribute line, then the annotation goes stale. I think this might be out of the scope of the gem as I've seen various CI steps check for stale annotations and fail if they exist. But I am curious what you think about this.

[1] I did a quick test in a dummy Rails postgres app.

# schema.rb
create_table "test_cases", force: :cascade do |t|
    t.string "default_string_array", default: ['foo'], array: true
  end

# bin/rails c
[1] pry(main)> cols = TestCase.columns; col = cols.last
=> #<ActiveRecord::ConnectionAdapters::PostgreSQL::Column:0x000000010e67dc48
 @collation=nil,
 @comment=nil,
 @default="{}",
 @default_function=nil,
 @generated="",
 @name="default_string_array",
 @null=true,
 @serial=nil,
 @sql_type_metadata=
  #<ActiveRecord::ConnectionAdapters::SqlTypeMetadata:0x000000010e67df40
   @limit=nil,
   @precision=nil,
   @scale=nil,
   @sql_type="character varying[]",
   @type=:string>>
[2] pry(main)> col.default
=> "{}"

[manuelmeurer]

Thanks for the explanation!

...if someone changes the default option in the attribute line, then the annotation goes stale.

What do you mean by this?

I think the annotations should be on a DB level only, i.e. read from schema.rb (or structure.sql).
But this is of course an individual preference, so I totally understand if you don't want to change the logic.

I'll try to test the changes in the branch soon!

[drwl]

What do you mean by this?

Currently, annotations capture the default(s) from the database and application. What I mean by stale is that, a non-db change (such as changing the attribute ... line can make the annotations wrong if annotaterb is not re-ran.

For example:

# I have a model with a column that looks like this:
# In schema.rb
create_table "test_cases", force: :cascade do |t|
  t.string "default_string_array", default: ["something"], array: true
end

# In the model file
# == Schema Information
#
# Table name: test_cases
#
#  notifications :string default(["newsletter"]), is an Array
# ...
class TestCase < ApplicationRecord
  attribute :default_string_array, default: %w(newsletter)
end

There's a separate discussion to be had for "annotations should be on a DB level only, i.e. read from schema.rb (or structure.sql)". Notice the annotation uses the default from the attribute line. If someone were to change the model file to look like:

# In the model file
# == Schema Information
#
# Table name: test_cases
#
#  notifications :string default(["newsletter"]), is an Array
# ...
class TestCase < ApplicationRecord
  attribute :default_string_array, default: %w(another_value) # <= the default was changed
end

Without re-running annotaterb, notice the annotation will show "newsletter" instead of "another_value".

[drwl]

I think the annotations should be on a DB level only, i.e. read from schema.rb (or structure.sql).

This is interesting and worth discussing. Historically, I think the old annotate gem and annotaterb rely on Rails/ActiveRecord for default values. How the logic works:

1. If a ActiveRecord model column has a #default value
2. Then it will read ModelName.column_defaults[column.name]

This happens in AnnotateRb::ModelAnnotator::ColumnAnnotation::AttributesBuilder#build.

I can do a spike and see if it's possible to see what are the database defaults, and if they differ, to have the option to annotate them instead of what I'll call "functional" defaults. Thoughts?

[manuelmeurer]

Hmm, my personal expectation would be that only the DB defaults are taken into account.
Since the annotations are in my model file, the same place where I would set a default value with the Attributes API, I would assume they are on the "same level", so the annotations don't take into account those overrides.. makes sense? 😄
That's just my personal preference though, and I'm sure many people would argue that defaults defined in the model should also be taken into account. 🤷‍♂️

[drwl]

Since the annotations are in my model file, the same place where I would set a default value with the Attributes API, I would assume they are on the "same level", so the annotations don't take into account those overrides.. makes sense?

This makes sense. Two thoughts,

1. I spiked on seeing how this might work and it doesn't look straight forward without reaching into Rails private internals. As far as I can tell, Rails internals doesn't differentiate between defaults from the database vs from Attribute API. I'm looking at ActiveModel::AttributeSet, ActiveRecord::ModelSchema, and ActiveModel::Attributes in Rails 7.0. I may be wrong though.
   Also, if this is something you would like to take on and create a PR for, I could see adding another flag like --schema-only for models that only uses default values from the database.

2. I don't have much experience with the Attributes API, but I'm curious why default values are being set with that API rather than at the schema level? It seems like default array values in schema.rb are supported with the postgres adapter.

For example, a schema looks like it could provide the same default behavior you're getting with the attributes api:

  # schema.rb
  create_table "test_cases", force: :cascade do |t|
    ...
    t.string "default_string_array", default: ['hello'], array: true
    ...
  end

[manuelmeurer]

1. Yeah, I had a look as well, it doesn't seem straightforward, so let's leave it as it is. Like I said, maybe the current behavior makes the most sense for most people. 😄
2. I never liked setting default values on the DB level (apart from defaulting an array column to [] instead of NULL), I rather have them in the model. Just a personal preference I guess.

[drwl]

Got it, that's all understandable. I think the goal of this gem should be to improve the developer experience working in Rails codebases so I'd be happy to support "db level" annotations, but doesn't seem feasible at the moment. Thanks for your help testing and I'll publish the new gem version with the fix in the next few days.

[drwl]

Closing this as it should be resolved v4.4.1, which I just pushed to Rubygems. Feel free to open if the issue still exists.

[manuelmeurer]

Awesome, thanks for your work on this, @drwl! 🙏