Is `accept_nested_attributes_for` considered safe when used with Delegated Types?

Full disclosure: I don’t consider myself any close to an expert on Delegated Types and accepts_nested_attributes. Please point out any misconceptions if any.

(for reference: Add accepts_nested_attributes_for support for delegated_type by xtr3me · Pull Request #41717 · rails/rails · GitHub)

Say I have my model:

class Foo
  delegated_type :detail, %w[Bar Baz]
  accepts_nested_attributes_for :detail
end

Now, AFAIU I’m supposed to use it this way:

Foo.new(detail_type: "Bar", detail_attribues: {attr1: ""})

So far so good, but if I do this Foo.new(detail_type: "User", detail_attribues: {}) I get an error that User does not have attr1 attribute.

This feels to me that I’m exposing too much to the users (by manipulating “detail_type” value they can make my app to instantiate and attempt to assign an attribute to any model, maybe even any objects).

If that’s the case – I can imagine I can just not accept detail_type in my controller, but make up some bogus type (e.g., detail_alias) and map it to actual types:

Foo.new(
  detail_type: {"bar" => "Bar", "Baz" => "baz"}.fetch(params[:detail_alias])
  ...

But would that be an intended way to use delegated types with nested attributes? Maybe we need some sort of richer config, like:

  delegated_type :detail, {bar: Bar, baz: Baz} # the key is an alias

I’m very interested in your experience/opinions :bowing_man:

Edit: Since writing the original post, I found about the reject_if: for nested attributes:


    accepts_nested_attributes_for :detail,
                                  reject_if: :illegal_detail_type?

    def illegal_detail_type?(_attributes)
      !self.class.detail_types.include?(detail_type)
    end

Would that be the intended way to make nested attributes safer?

I suppose you could use ANAF, but I think this is going against the grain with delegated_type.

Foo.create!(detail: Bar.new(params.require(:bar).permit(...)))

This is how I generally handle it, and all of the real world examples I’ve seen go this route.

Remember the delegated type is just a normal belongs to under the hood. And I think it’s generally a bad idea to be in a situation where you’re manually setting the type, just pass it an instance of the actual object.

Thanks for taking the time, @chris.covington

Ok, but passing a type is how the docs are showcasing one way to use delegated types, so I wonder if those docs should be improved?

In those docs it’s Message so it looks innocent, but in my case I have those models namespaced so it was FooBar::Message and it got me thinking.

I think it’s the intended usage. This is an extraction from Basecamp where they have a couple of really complex uses of it, and all of them pass the delegated type as an argument.

ANAF works, because under the hood it’s just a polymorphic belongs to. But if I’m being honest, ANAF has always backfired on me, and ended up reverting it and decoupling the models.

If anything I would only use ANAF in a way that was a TypeA has_many TypeB with delegated_type (A,B,C,D) kind of setup. But I think that’s pretty niche.

Without more info I’m not sure I can help, but I would take a step back and ask is ANAF really helping or is it making things too complex?

I have a model with two kinds of notes. With delegated types and ANAF I managed to refactor from two sets of controllers (and views coming with it) to one. Here I just have a render partial where I pass the form and call form.fields_for).

This refactor allowed me to speed up changes to the “parent” model, and add features that are related to it without worrying about details coming from the delegated types.

It’s not causing me any trouble (yet) but for security’s sake I do use reject_if on the ANAF, and in my controller I map attributes coming from the form into actual class names.