Nested errors

[kirs]

My form:

class RegistrationForm < ActiveForm::Base
  self.main_model = :user

  attributes :email, :password

  association :company do
    attributes :name, :email, :phone
  end
end

When I submit it with such params:

{
  email: "", password: "", company_attributes: { name: "foo", email: ""}
}

Emails errors are:

@form.errors[:email]
=> ["Can't be blank", "Can't be blank"]

I find it weird that email errors are duplicated even if the fields have similar name.
Should we return a hash of errors to avoid it?

[m-Peter]

Probably we should try to correlate the attribute with its model. That would be more descriptive.

[kirs]

What do you mean?

[kirs]

Oh, sorry. At this moment it's fixed in #11

[sobrinho]

Hi guys, please consider a look at rails/rails#8638.

TL;DR

The errors output must be something like this:

class ProductForm < ActiveForm::Base
  attributes :name

  association :versions do
    attributes :name
  end
end

{
  errors: {
    name: ["can't be blank"],
    versions: {
      relation_errors: ["can't have more than 5"],
      record_errors: {
        0: { name: "can't be blank" },
        3: { name: "can't be blank" }
      }
    }
  }
}

Explanation

When you have the errors output being consumed by an API (i.e.: Backbone), you definitely must have the nested object index to identify which record has the problem.

Using this object you would have access to association errors at errors.association.relation_errors and records errors at errors.association.record_errors.index.

You may ask why relation_errors and record_errors and you can think at this example:

class Product < ActiveRecord::Base
  has_many :versions

  validate :must_have_at_least_one_version

  def must_have_at_least_one_version
    errors.add(:versions, :at_least_one) if versions.reject(&:marked_for_destruction?).empty?
  end
end

With that in mind, you have errors at the relation and errors at the records of the relation.

The latter is a painful point on rails today because you can't detect which record of the sent records has the issue, example:

PUT /products/1?product[name]=Example&product[versions_attributes][0][name]=White&product[versions_attributes][1][name]=Black
{
  errors: {
    "versions.name": "already taken"
  }
}

You can't answer if the white color or the black color was taken, or both.

What we've been doing today is using a custom presenter on rails applications to get the complete error output, like this (draft, wip):

class BackboneErrorsPresenter
  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]
        errors[association.name] = {}

        object.send(association.name).each_with_index do |child, index|
          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)
        errors[attribute] ||= {}
        errors[attribute][:relation_errors] = error
      else
        errors[attribute] = error
      end
    end

    { :errors => errors }
  end
end

Using this object, we have the desired output:

def create
  @product = Product.new(params[:product])

  if @product.save
    render :json => @product
  else
    render :json => BackboneErrorsPresenter.new(@product).to_h, :status => :unprocessable_entity
  end
end

It's working on production right now with backbone + backbone-nested-attributes + our plugin (draft, wip):

Application.Model = Backbone.NestedAttributesModel.extend({
  initialize: function () {
    this.computeHasOneRelations();
    this.computeHasManyRelations();

    this.clearErrors();

    this.on("request", this.clearErrors);
    this.on("error", this.parseErrors);
  },

  computeHasOneRelations: function () {
    this.hasOneRelations = _.where(this.relations, { type: "one" });
  },

  computeHasManyRelations: function () {
    this.hasManyRelations = _.where(this.relations, { type: "many" });
  },

  clearErrors: function () {
    this.clearMyErrors();
    this.clearHasOneErrors();
    this.clearHasManyErrors();
  },

  clearMyErrors: function () {
    this.errors = {};
  },

  clearHasOneErrors: function () {
    _.chain(this.hasOneRelations)
      .map(function (relation) { return this.get(relation.key) }, this)
      .invoke("clearErrors");
  },

  clearHasManyErrors: function () {
    _.chain(this.hasManyRelations)
      .map(function (relation) { return this.get(relation.key) }, this)
      .pluck("models")
      .flatten()
      .invoke("clearErrors");
  },

  validate: function () {
    return _.isEmpty(this.errors) && this.validateChildren();
  },

  validateChildren: function () {
    return this.validateHasManyChildren() && this.validateHasOneChildren();
  },

  validateHasManyChildren: function () {
    return _.chain(this.hasManyRelations)
      .map(function (relation) { return this.get(relation.key) }, this)
      .map(function (collection) { return collection.models })
      .flatten()
      .any(function (model) { return !model.isValid() })
      .value();
  },

  validateHasOneChildren: function () {
    return _.chain(this.hasOneRelations)
      .map(function (relation) { return this.get(relation.key) }, this)
      .any(function (model) { return !model.isValid() })
      .value();
  },

  parseErrors: function (model, jqXHR) {
    if (!jqXHR.responseJSON || !jqXHR.responseJSON.errors) {
      return;
    }

    this.attachErrors(jqXHR.responseJSON.errors);
  },

  attachErrors: function (errors) {
    this.attachMyErrors(errors);
    this.attachRelationsErrors(errors);

    this.trigger("validated", this);
  },

  attachMyErrors: function (errors) {
    _.each(errors, function (error, attribute) {
      if (_.isObject(error)) {
        error = error.relation_errors;
      }

      if (error) {
        this.errors[attribute] = error;
      }
    }, this);
  },

  attachRelationsErrors: function (errors) {
    this.attachHasOneRelationsErrors(errors);
    this.attachHasManyRelationsErrors(errors);
  },

  attachHasOneRelationsErrors: function (errors) {
    _.each(this.hasOneRelations, function (relation) {
      var relationErrors = errors[relation.key];

      if (relationErrors) {
        this.attachErrorsInRelation(relation, relationErrors);
      }
    }, this);
  },

  attachHasManyRelationsErrors: function (errors) {
    _.each(this.hasManyRelations, function (relation) {
      var relationErrors = errors[relation.key];

      if (relationErrors) {
        this.attachErrorsInRelations(relation, relationErrors);
      }
    }, this);
  },

  attachErrorInRelation: function (relation, errors) {
    var model = this.get(relation.key);

    if (model.attachErrors) {
      model.attachErrors(errors.records_errors);
    }
  },

  attachErrorsInRelations: function (relation, errors) {
    var collection = this.get(relation.key);

    collection.forEach(function (model, index) {
      if (errors.records_errors[index] && model.attachErrors) {
        model.attachErrors(errors.records_errors[index]);
      }
    });
  }
});

And the model:

Product = Application.Model.extend({
  urlRoot: "/products",

  relations: [
    {
      type: "many",
      key: "versions",
      relatedModel: function () { return Application.Model }
    }
  ]
});

We are going to finish all these objects and package it all on a ruby gem (there is 2 more objects which integrates all of this on marionette with a behaviour and a POJO to deal with the DOM).

My point is: every nested endpoint needs this error output or another one which provides the relation errors and the record errors with the index.

[kirs]

It makes sense, but as far as Active Form is just an extraction from Active Record with few additions, we can't break the stable AR::Base API.

[sobrinho]

Are we going to replace active record nested attributes?

I think it's a good time make this change since it's backwards incompatible, rails 5 is on the way.

If compatibility is a concern, we can make it opt-in.