[JacobNWolf]

Fixes #245.

Collections containing nil now report :invalid on the parent attribute instead of raising NoMethodError. Other elements are still validated, and merged errors preserve the original array indexes and hash keys without adding duplicate invalid messages.

Additionally adds settings for the Zed IDE so that Rubocop fixes autofixable linting/formatting errors on save and RSpec tests can be ran directly within the IDE.

Adds 23 regression examples covering arrays, hashes, polymorphic arrays, and merged errors.

Validation:

• bundle exec rspec — 572 examples, 0 failures
• Updated spec passes RuboCop
• All 23 new examples reproduce the failure against the original implementation

[DmitryTsepelev]

Thanks for the fix! The direction is right, but nil elements can still crash on two paths, and the nil element's own error often gets lost. Details are in the inline comments. Two points are about code outside the diff, so they are here.

Polymorphic arrays still crash before validation runs. ManyPolymorphic#ensure_model_class (many_polymorphic.rb) and ManyBase#decode_and_initialize (many_base.rb) pass nil to the user's wrapper block. A wrapper that reads its input, like StoreModel.one_of { |json| json["kind"] == "x" ? A : B }, raises NoMethodError: undefined method '[]' for nil on [nil], and also on "[null]" loaded from the database. StoreModel.union(...) returns nil from its wrapper and raises the extract-wrapper error. The hash types already skip nil with next object if object.nil? (hash_polymorphic.rb), so the array types could do the same.

The docs don't say how nil is handled. The docs say a custom array strategy receives store_models (validations.md). After this PR that list can contain nil, and neither validations.md nor hash_of_stored_models.md mentions it. People write custom strategies from these docs.

A suggestion for the overall shape. It would help to handle nil in one place. One option is for the validator to pass strategies only real models, keeping their indexes, and for the merge strategies to report [i] is invalid for each nil slot. Another option is to skip nil when casting, the way the hash types do. Either way, the custom-strategy crash, the lost errors and the duplicate errors go away together.

[DmitryTsepelev]

Custom array strategies still receive the nil elements here, because value is passed through unfiltered. Only the built-in MergeArrayErrorStrategy got a guard. A custom strategy written the way the docs describe still crashes:

validates :configurations, store_model: {
  merge_array_errors: ->(attr, errors, models) {
    models.each_with_index { |m, i| m.errors.full_messages.each { |msg| errors.add(attr, "[#{i}] #{msg}") } }
  }
}

With configurations: [nil, Configuration.new] this raises NoMethodError: undefined method 'errors' for nil. That is the same crash as #245.

The default strategy has a milder version of the problem. MarkInvalidErrorStrategy stores store_models in the error details, so errors.details[:configurations].first[:errors] becomes [nil, #<Configuration>]. Code that walks those details, such as a serializer doing .flat_map { |m| m.errors.to_a }, crashes on the nil.

[DmitryTsepelev]

This error is added outside the configured strategy, so it doesn't follow the strategy's format.

• With merge_array_errors: true and [nil], the message is Configurations is invalid, with no [0] index.
• With a custom strategy like FhtagnArrayErrorStrategy from the docs and [nil, Configuration.new], the errors are ["cthulhu fhtagn", "is invalid"]. The dedup on line 66 only knows about :invalid, so the user gets both.
• With [nil] alone, the custom strategy is never called, and the user gets is invalid instead of their own message.

[DmitryTsepelev]

of_kind?(attribute, :invalid) matches any :invalid error on the attribute, no matter what added it. So the nil element goes unreported whenever something else has already added :invalid.

• With merge_array_errors: true and [nil, Configuration.new], the only message is [1] Color can't be blank. Nothing says that index 0 is nil.
• With merge_hash_errors: true and { "a" => nil, "b" => Configuration.new }, key a is not reported.
• A custom validation that runs first, like validate { errors.add(:configurations, :invalid, message: "too many") }, hides the nil error too.

Also, the error added here has no :errors key in its details, unlike every other :invalid error this validator adds.

[DmitryTsepelev]

Minor: each collection is walked twice. select { ... }.present? builds an intermediate array, and then include?(nil) on line 44 walks the whole collection again, calling == on every model. One pass that notes whether it saw a nil would do both jobs.

[DmitryTsepelev]

This guard is one of three places that now deal with nil. The other two are the check after the strategy call in the validator and the of_kind? dedup. Custom strategies don't get this guard, which is why they still crash. If the validator handled nil in one place, strategies wouldn't need to know about nil at all. The review summary has two ways to do that.

[DmitryTsepelev]

This expectation, and the one on line 903 ("skips the nil one"), make dropping the nil element's error part of the contract. With merge_array_errors: true I'd expect something like ["[0] is invalid", "[1] Color can't be blank"] here, so the user can tell which slot is nil. As written, fixing that later would break this test.

[DmitryTsepelev]

This wrapper ignores its argument, so nil never reaches real discriminator logic. The test passes only because of that. A wrapper that reads its input, like { |json| json["kind"] == "x" ? A : Configuration }, fails on [nil] while casting, before validation runs. The review summary has more on this: the polymorphic array types need the same next object if object.nil? that the hash types have.

[DmitryTsepelev]

A few paths that are likely to carry nil aren't covered:

• A value loaded from the database as a string, such as "[null]" or '{"a":null}'. That goes through decode_and_initialize.
• A custom strategy, either a lambda or a class, that receives the nils.
• The polymorphic hash type with nil values.

A test with a custom strategy would have caught the crash on line 43. A polymorphic test with a wrapper that reads its input would have caught the crash while casting.

[DmitryTsepelev]

The entry is missing the author credit that the other entries have, like ([@JacobNWolf]) at the end. Without it, the link definition added on line 317 isn't used.

[DmitryTsepelev]

Could you drop the .zed/ files from this PR? The repo doesn't track editor settings. These would turn on format-on-save with RuboCop autofix for everyone who opens the repo in Zed. If you want to keep them locally, a global gitignore entry keeps them out of commits.