Improve a way to handle errors for collection nested attributes #8638
Conversation
Example:
Employment.validates :company, :presence => true
Developer.has_many :employments
d = Developer.new
2.times { d.employments.build }
d.valid?
d.errors.messages
Old result:
{
:"employments.company"=>["can't be blank"],
:email=>["can't be blank"],
:password=>["can't be blank"]
}
New result:
{
:"employments[0].company"=>["can't be blank"],
:"employments[1].company"=>["can't be blank"],
:email=>["can't be blank"],
:password=>["can't be blank"]
}
|
The error messages should be available on the individual |
|
The benefit is that we can provide errors as the result of JSON api call and send them to a different server that doesn't know about objects we have. When you can render a form using active record models and form_for helper it is not required. But when the generation context is different and you can only pass a data in between this is problem. |
|
I see but I guess you could write the logic to aggregate the errors and then convert them to JSON. I'm not sure we should duplicate errors, that are already accessible through the object graph. To completely implement this feature (aggregating all child errors on the parent) you would also need to modify other validators. For example @rafaelfranca @carlosantoniodasilva what do you think? |
|
@senny Errors aggregation code is only related to autosafe associations not specific validators. If you are using any validator including Follow though the source code: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/autosave_association.rb#L286 |
|
I'm not sure. I understand the reason to add those errors messages, but I don't like the idea to show more errors messages for the users. @josevalim thoughts? |
|
@rafaelfranca As you can see Rails already adds error messages from associated models and doing it wrongly: :"employments.company"=>["can't be blank"]And this is wrong. |
|
I have been waiting for a solution for this quite a while now. But I can't see how this commit will improve the situation. After getting the hash with indexed error messages I still can't relate them to the records listed in my form, because the order of the records might differ with every request. I see that using the id of the object can't be the solution either, because it does not exist for new records. |
|
@kmerz according to html spec fields with same name are passed to backend with the same order they appear in DOM. I know this for sure because made a lot of features depending on this behavior that is compatible with every browser. Rails itself already relies on this behavior, example of html generated by rails form builder: .education
.field
%label{:for => "..."} University
%input{:name => "developer[educations_attributes][][university]"}
.field
%label{:for => "..."} Grade
%input{:name => "developer[educations_attributes][][grade]"}
.education
.field
%label{:for => "..."} University
%input{:name => "developer[educations_attributes][][university]"}
.field
%label{:for => "..."} Grade
%input{:name => "developer[educations_attributes][][grade]"}See that nested attributes fields here has no index, but rails always know how to group them by models without mixing them up. |
|
I agree with @bogdan here. It seems broken to me to merge errors together and make it impossible to know which instance caused the validation to fail. The only thing I'm not sure about is the square bracket notation in the key for the hash, but I'm can't think of another way. And I guess the square bracket notation matches the way the parameters are passed from the browser to the app. |
|
@jalada yeah, this is another way of solving this. I believe you mean empty hash instead of empty array here: I would prefer flat hash without nesting as it is in current PR. Maybe because of a way how I am using it. |
|
Spent some time to bring up a live demo of what this patch allows to do: |
|
+1, this is awesome and a much needed fix. Would love for this to be merged! |
|
This is great patch, thanks. Interesting side issue is that, for an existing parent record, validate_collection_association only checks (via its call to associated_records_to_validate_or_save) records that are deemed changed_for_autosave (or new records if autosave is false on the has_many). This means that if I add a new validation to the association model that causes existing records to not be valid, those records are not checked. Granted, this is a general issue in rails not in this patch, but it is of a similar flavor (issues with nested errors). As a workaround I changed: to |
|
Hi I'm a beginner, in which version of rails is this pull available? |
|
@wiryasastra: this PR hasn't been merged yet. It's not in any version of rails. |
|
This is a VERY IMPORTANT pull and should come to any version of rails, at least to 3.2!! |
|
There is a release process. The rails team tries to follow semver. |
|
+1 on this, i think it should be merged |
|
Random thought: Is better than: The former is more easy to parse. We could argue that |
|
latter is also a "JSON Path" notation. But all implementations of JSON Path I've seen also parse "items.0.product" without any problems. We use "items.0.product" notation in our project already. |
|
@codesnik that's even better, I was thinking it was invalid syntax! :) Another random thought... Instead of dumping keys like that, we could dump a more complete object: So instead of parsing the keys, we just iterate over it. |
|
This is a "json <=> keyvalue" conversion pseudo standard, one direction implemented in The need of it comes from a fact that form inputs can only be represented as keyvalue where In case of demo here: http://rails-ajax-validation.herokuapp.com But I understand the need of it. I like a json format idea but it will cause more serious incompatibilities with current implementation. There is 3rd party parsers available in case you need them: |
|
@bogdan have you heard anything from the core team on whether they're going to pull this in? I'm currently just passing my models into a validator that effectively builds the same thing... but it doesn't rely on the existing nested validation framework |
|
@danteoh no other discussion than here. |
|
+1 to this feature. It is so much needed. |
|
+1. We're facing the same problems with how to return JSON API error responses on dependent has_many relations. |
|
+1 |
|
👍 definitely needed. |
|
👍 |
|
@rafaelfranca please take a look this comment: railsgsoc/actionform#10 (comment) |
Added an option in accepts_nested_attributes_for
Example:
```ruby
class Bird < ActiveRecord::Base
belongs_to :pirate
validates_presence_of :name
accepts_nested_attributes_for :pirate, index_errors: true
end
```
This allows the format of errors printed through nested fields to be:
```json
{
"pirates[0].name"=>["Pirates name cannot be blank"]
}
```
|
Anyone is going to work on it? I agree that the errors should follow an object convention, not flat hashmap that should be additionally parsed afterwards. I've implemented 3 years ago almost the same functionality in Rails 3.2: https://gist.github.com/dmitry/3238745 (ugly, but works perfectly) I would like to participate in development of this feature if it's possible. |
|
Sum up discussion above: There is currently open question regarding migration plan, here are the options:
Also there is an open discussion regarding errors format:
{"educations[1].company" =≥ "can not be blank"} #1
{ "educations" => [{}, {:company => "can not be blank"}]} #2My personal choice is: @dmitry you can help by putting us your arguments on choices. |
|
@bogdan I've actually got something working for this, but I've been trying to figure out creating models and test cases for it (new to contributing). Here's what I have (work in progress): tsun1215@68bb7e7; With the manual testing I did, it should be working. (Also, the puts statement will be removed when I actually submit a request). I have yet to do the global configuration option for this. |
|
@tsun1215 yes I saw your work, that is why I've mansion this variant here. Thanks for showing me it. |
|
In our side we ended making a errors presenter: class NestedErrorsPresenter
attr_reader :object, :klass
def initialize(object)
@object = object
@klass = object.class
end
def to_h
errors = HashWithIndifferentAccess.new
klass.reflect_on_all_associations.each do |association|
if association.macro == :has_many && association.options[:autosave]
object.send(association.name).each_with_index do |child, index|
next if child.valid?
index = child.respond_to?(:front_end_index) ? child.front_end_index : index
errors[association.name] = {}
errors[association.name][:records_errors] ||= {}
errors[association.name][:records_errors][index] = child.errors.to_h
end
end
end
object.errors.each do |attribute, error|
next if attribute =~ /\./
if klass.reflect_on_association(attribute)
if association = klass.reflect_on_association(attribute)
errors[attribute] ||= {}
errors[attribute][:relation_errors] = error
if association.macro == :has_one
errors[attribute][:record_errors] = object.send(attribute).errors.to_h
end
else
errors[attribute] = error
end
else
errors[attribute] = error
end
end
{ :errors => errors }
end
endAnd using a custom reponder: class NestedErrorResponder < ApplicationResponder
def to_json
if resource.valid?
render :json => resource
else
render :json => NestedErrorsPresenter.new(resource).to_h, :status => :unprocessable_entity
end
end
endIt's working perfectly fine on production but we are still expecting this feature to be on core since it's a common need. If not, we are going to pack this as gem and publish :) |
|
There are actually 3 types of error formats possible:
I prefer nested with hash index (3). It's more universal and can be used in any languages, sometimes can be faster. Flat
Nested with array index
Nested with hash index
|
|
Please, be aware that we must support this structure: {
"name": "can't be blank",
"partners": {
"record_errors": {
0: { "percent": "can't be greater than 100" },
3: { "percent": "can't be greater than 100" }
},
"relation_errors": {
"base": "the total of percentages can't be greater than 100"
}
}
}First the index is not always sequential that is there is an error on first record and third record, so, it must be always a hash instead of an array. Second we must provide the subitems And honestly I think that the plain hash will be always worse since we will need to parse the keys everywhere and in every language (we don't always use this return on browser, it may be a mobile app or another program in Go/Python/Scala/etc) and returning nested hashes will avoid that. |
|
@sobrinho 👍 with small exception: subitems, because we can follow convention to use indexes of the records as numbers, and general errors with index as strings (normally it's just |
|
Surely all errors can be arrays of strings, as there can be multiple errors on any field |
|
I sent a wrong hash, sorry. The correct one would be: {
"name": ["can't be blank"],
"partners": {
"record_errors": {
0: { "percent": ["can't be greater than 100"] },
3: { "percent": ["can't be greater than 100"] }
},
"relation_errors": ["the total of percentages can't be greater than 100"]
}
}Note that in a case of a has one association, the hash would be different: {
"name": ["can't be blank"],
"address": {
"record_errors": {
"street": ["can't be blank"]
},
"relation_errors": ["is outside the country"]
}
}But still we must have Note that I'm also proposing the messages to be always an array now, it caused some trouble to us to have sometimes a string and sometimes an array, keeping it always as array make the front-end logic simpler. |
|
I am not sure what kind of architecture changes will be under all these relation errors. If the nested errors should be supported, it should be done at a different layer for example custom Allow |
|
class NestedErrorsPresenter
attr_reader :object, :klass
def initialize(object)
@object = object
@klass = object.class
end
def to_h
errors = HashWithIndifferentAccess.new
klass.reflect_on_all_associations.each do |association|
if association.macro == :has_many && association.options[:autosave]
object.send(association.name).each_with_index do |child, index|
next if child.valid?
index = child.respond_to?(:front_end_index) ? child.front_end_index : index
errors[association.name] = {}
errors[association.name][:records_errors] ||= {}
errors[association.name][:records_errors][index] = child.errors.to_h
end
end
end
object.errors.each do |attribute, error|
next if attribute =~ /\./
if klass.reflect_on_association(attribute)
if association = klass.reflect_on_association(attribute)
errors[attribute] ||= {}
errors[attribute][:relation_errors] = error
if association.macro == :has_one
errors[attribute][:record_errors] = object.send(attribute).errors.to_h
end
else
errors[attribute] = error
end
else
errors[attribute] = error
end
end
{ :errors => errors }
end
endSo, the Errors object wouldn't change, only the presentation of it. |
|
@sobrinho probably you didn't but others(including myself) made me think that this is proposed. I don't feel like it is related with this PR as it only cares about how errors are added to Presenters like yours can live beside this PR in Rails Core or outside of it. |
|
@bogdan not sure if I'm following you but the suggestion above is one solution for the problem of this pull request. People seems to agree, everyone on this pull request at least, that we must have a way to deal with it on active record natively. A presenter may or may not be the best solution but thinking in backwards compatibility it seems to be the way. Are we saying the same thing or I did not understood you? My suggestion is:
I'm suspect to say that but the hash which I proposed is working perfectly on at least 5 large applications here. Our front-end architecture is made by 4 javascript packages (private but will be open sourced in a couple of weeks): backbone.relation: Handles the concept of associations in backbone models (similar to Backbone.Relational) backbone.errors: Handle the errors hash on failure and distributes the errors through associations (using a small reflection implemented on backbone.relation) nested-errors-presenter: Handle the errors receiving the attribute name and the error and render it on DOM (assuming that you use bootstrap markup but it accepts any strategy) backbone.nested-errors-presenter: Basically knows how to receive the errors from a backbone.relation model and iterate through them and send it to nested-errors-presenter. Basically using these 4 packages the back-ends validations are totally painless to integrate on backbone and render them to DOM. We started working with React.JS right now and we are going to create React components/libraries which will knows how to deal with this hash too. |
* Using an instance variable allows JS populate operations to have
access to the @order from within the rendered .js.erb files
* Assign all line_item errors to the order's base
* Allow to respond to JS requests for `#populate` and `#update` actions.
It is left for the developer to actually implement the `js.erb` views
To consider / TODO:
- to avoid the whole rescue operation, and manually assigning line item
errors to the order, we'd need to do to be doing `order.save`
instead of `line_item.save!` in [`OrderContents`](https://github.com/solidusio/solidus/blob/master/core/app/models/spree/order_contents.rb#L151)
-> That would have the effect to "automatically" assign the line item
errors on the order in the form: `line_items.quantity` in Rails 4.x
and "line_items[0].quantity" in Rails 5
More info:
- rails/rails#8638
- rails/rails#19686
Example:
Old result:
See that it is impossible to detect which
Employmentobject is error related to.In order to fix that propose to use an index(as Employment could be new - we can not use an id).
Only applied to collection associations.
New result
Live demo:
http://rails-ajax-validation.herokuapp.com