Writing private attributes from the constructor about virtus HOT 16 CLOSED

It sounds like we may be collapsing multiple responsibilities into the public/private syntax. I wonder if we need to split things out and have a way to specify if an attribute is "mass assignable".

We could have something like this:

attribute :person, Person, accessor: :private, accessible: true

Would we make all attributes accessible by default or not accessible? I'm thinking we make the default true, but provide a way to change that globally (which I think we already do in Virtus for other options).

from virtus.

BTW I only choose the term "accessible" based on it being in common use in other popular ruby libs. Maybe there's a better name for this, I know a few times I have been confused by it's use in AR.

from virtus.

I think having the default behaviour to allow setting from the constructor is a good idea. That's what I would want, and would guess it is the majority use case.

I think it could do with a better name though. What about just calling it mass_assignable?

from virtus.

I usually prefer single, short, descriptive words for option names if I can help it. Sometimes it doesn't work out, but I usually like to try.

from virtus.

I almost wonder if we should punt on it and just make everything accessible, and then tell people to write their own #attributes= method if they want specific filtering, or to "slice" the Hash in the client code.

My thinking is that it might be a bit of a slippery slope to try to encode all of the cases in the DSL where an attribute should be mass assignable or not. It's likely it will be context specific fairly often, and the object won't have enough context to make the right choice.

We've discussed this before in the context of DataMapper and MVC frameworks, why we don't have an attr_accessible equivalent. It was our thinking that the model was not the right place to specify these things. The controller's responsibility is to properly filter and route the parameters to the object and they can't have enough context to properly filter things on it's own. It would probably be an SRP violation to give domain objects the responsibility to filter data, when that should probably be done earlier in the process.

@solnic wdyt?

from virtus.

Yeah I think it's a good idea to remove writer visibility check from #attributes=. I just realized we will need that anyway in the mapper for DM2! Otherwise we won't be able to initialize objects with all their attributes set. Alternatively we could add a new method that would behave like #attributes= minus the writer visibility check. #load maybe? I'm not sure yet.

Regarding attr_accessibile - I really don't want it in virtus. Input data sanitization shouldn't be a concern here.

from virtus.

@solnic I think we're in agreement on this.

@mattwynne does this sound ok to you? Do you want to submit a pull request that changes #allowed_methods to return the full list of methods, not just the public ones?

Now, if someone wants to restrict the methods, they can always just do:

def allowed_methods
  super - private_methods.map(&:to_s)
end

from virtus.

Also keep in mind that there are two #allowed_methods methods, one for handling inclusion and one for object extension.

from virtus.

I will do this, just need space to focus on it.

I've had another thought about it though. What if we use a totally different method to define these things, rather than adding lots of options to the attribute method? I was thinking we could call these things 'fields'.

So you'd have, for example:

class PersonPage
  include Virtus

  attribute :name, String
  field :person, Person

  def age
    person.age
  end
end

It seems better to me to give this thing a name rather than just making it a special kind of attribute. WDYT?

from virtus.

So what would be the difference between attribute and field? Just visibility?

from virtus.

Yes, a field would be private, but assignable from the constructor.

from virtus.

What happened with this in the end guys? Did you change the default behaviour of attributes? Should I close this ticket?

from virtus.

@mattwynne I think the similarity between field and attribute is probably what killed the momentum on the comments to this ticket. I don't think it would be particularly easy for us to articulate the differences between the two. I think it would confuse people.

I also think that we should only be adding the most common case things to the DSL, and while still allowing the uncommon cases through overriding methods we provide. So in this case I would probably hold off on trying to extend the DSL with something field-like and wait and see how people use virtus.

I do believe #allowed_methods was changed to return all public methods though. That means the approach I outlined above where you could define your own #allowed_methods to restrict which can be mass assignable would work great.

from virtus.

Cool, let me try that out.

from virtus.

I had a similar confusion with how private attributes work but the allowed_methods solution did not work for me (with and without 'self') :

def self.allowed_methods
  super - (private_methods + [:age=]).map(&:to_s)
end

My common use case for private attributes is for attributes that I do not want the user to write like person.age = 23 but I do want that internal state to be persisted and restored from a database (loaded as a keyword in the constructor).

My naive initial implementation was like this:

class PersonPage
  include Virtus

  attribute :name, String
  attribute :age, Integer
  private :age=
end

But that private statement will break Virtus constructor, ignoring also :age values

I solved this issue by writing an initialize function and set manually the value. Is that the standard way to solve this problem?

from virtus.

A perhaps slightly more hackish way of having attributes assignable only by mass-assignment:

class Model
  include Virtus::Model

  def initialize(args = {})
    super(args)

    # Handle "immutable" attributes
    attribute_set.each do |attribute|
      if attribute.options.fetch(:immutable, false)
        name = attribute.options.fetch(:name)
        define_singleton_method(:"#{name}=") { |_| raise "Attribute #{name} is immutable" }
      end
    end
  end
end

class Post < Model
  attribute :id, Integer, default: nil, immutable: true
end

post = Post.new(id: 1)
# => #<Post:0x00000000024af450 @id=1>
post.id = 2
# => RuntimeError: Attribute id is immutable

from virtus.