Preserve default options when merge with other custom options

[allenwq]

class Article  < ActiveRecord::Base
  schema_validations except: [:name]
end

When I specify an except option in my mode, I get the error: ActiveRecord::RecordInvalid: Validation failed: Created at can't be blank, Updated at can't be blank when I trying to save the model.

It is because that the default except hash is replaced by the custom options, which I think doesn't make sense. The attributes like updated/created_at should always be whitelisted.

[ronen]

@allenwq I see what you're saying, and I agree it's surprising. Unfortunately the README should state more clearly what the default value is for :except, and and that passing something else overrides the default value. (The README intends to refer to the rdoc for details, but doesn't actually. Sigh.)

The PR has two problems, though, as I see it:

• I don't think that there should be any attributes that are hardwired in the code to be whitelisted. That should always be configurable or overrideable, through some appropriate calls or options.
• This would technically be a breaking change, so would need a major version bump. So the README would need a section to describe the changes.

If you want to address both of those, I'd probably be OK with accepting it.

[allenwq]

@ronen I'm pretty agree with your first point, just haven't found an elegant way to do it. How about add one more option called :except_only ? If user declares except_only: :name, then default except will be totally replaced.

[ronen]

@allenwq I think adding that option would just add confusion, and highlight the inelegance -- who could remember the difference between :except and :except_only? Also it'd be a breaking change. Could do except_also: [:name] to add :name to the list of exceptions, which wouldn't be a breaking change, but still

The current version is at least consistent: there's a default value for :except, and if you specify anything else, that overwrites the default value. If it were documented better it wouldn't be all that bad IMHO.

Or how about something like this: Add a configuration value config.whitelist which would list columns for which validations are never emitted, regardless of what options are given per-model. (Coupled with an option whitelist: that could be supplied per-model to override the whitelist!) That would also be a breaking change, and so version bump, but maybe worth it.

In any case the README should be improved.

[allenwq]

@ronen Sorry, was very busy during the past days.

Thanks for the suggestions ! I would prefer the last solution.

So in the global configuration we will have:

SchemaValidations.setup do |config|
  config.whitelist = [:created_at, :updated_at]
end

And in each model, we will have the option whitelist, user can omit the global whitelist in that model by setting whitelist option to false:

class Article  < ActiveRecord::Base
  schema_validations except: [:name], whitelist: false # default is true
end

Do I understand correctly ?

[ronen]

And in each model, we will have the option whitelist, user can omit the global whitelist in that model by setting whitelist option to false: ... Do I understand correctly ?

Yes. I think setting whitelist: false would just be a shortcut for whitelist: []

Though the default value of the whitelist to be consistent with the current behavior would be:

SchemaValidations.setup do |config|
  config.whitelist = [:created_at, :updated_at, :created_on, :updated_on]  # no need to config, this is the default
end

And to be clear:

   schema_validations except: [:name]  # no validations for :name, :created_at, :updated_at etc. as per your desired behavior
   schema_validations whitelist: [] # validations for everything including :created_at etc.
   schema_validations except: [:name], whitelist: [] # validations for everything including :created_at etc. but not :name

The first above is a breaking change so would cause a version bump. Though in practice probably not many people would be affected and so most could upgrade without needing to make any changes.

[allenwq]

@ronen Got some updates here.

[allenwq]

@ronen Yeah, that make sense, and it's compatible with existing configurations.

[allenwq]

@ronen Updated, including the readme.

[ronen]

@allenq sorry for the long delay -- real life has been busy.

I've merged this, further cleaned up the README, and released it as v2.0.0

Thanks again for your work and flexibility with this!

[allenwq]

@ronen It's OK. Thanks a lot !