Lazy default values

[solnic]

This PR adds new feature where you can set an attribute to be lazy so that its default value will be evaluated when reader is called for the first time (just like it used to be for all attributes).

Another change is that default values are now set in the constructor instead of being lazy all the time. This speeds up reading significantly and adds more flexibility.

[mbj]

Does freezing still expand defaults before object itself gets frozen?

[solnic]

All non-lazy attributes are set in the constructor. Freezing shouldn't expand lazy attributes. At least that's my current thinking.

[mbj]

How about lazy attributes?

[solnic]

As I wrote - those are not evaluated in freeze unless you've read them prior freezing. I think it's a good and logical contract.

[mbj]

Sorry for not getting your point. Will this open https://github.com/solnic/virtus/pull/105 again? Especially the problem described in https://github.com/mbj/virtus/commit/6592ba29b71e0f269ed144418629ff8cecf4d908

[solnic]

Yes because I want to make clear distinction between defaults that are set in the constructor and lazy ones. I'm open for discussion obviously, as always.

Please also note that lazy evaluation is actually required to support various cases. For example when a default value of one attribute relies on another attribute (we have specs for that).

[mbj]

@solnic Sorry man. I was to lazy with communication. Are you still expanding lazy defaults before freezing, or will freezing an object with lazy defaults blow up if you access this default again? The commits I referred to fixed exactly this problem. (We had only lazy defaults before your refactoring).

[solnic]

Yes it will blow up

[mbj]

Can we fix this with expanding lazy defaults prior freezing again?

[solnic]

Would it be an appropriate thing to do? Since you are explicitly setting sth as lazy then it's your job to take care of freezing in correct moment and don't mutate the state after an object was frozen.

[mbj]

I expect the following to work:

class Foo
  include Virtus
  attribute :bar, Bar :default => proc { Bar.find_me_some }
end

foo = MyBusinessLogic.give_me_a_foo
foo.bar # Instance of Bar, regardless if frozen or not. It is my contract Foo#bar returns instance of Bar

If I need to make sure I do not get random frozen related exceptions, all places where an instance of Foo can be frozen need to take care of expanding lazy defaults. This would defeat using virtus with adamantium together a lot. Imagine a case where a query method that returns instances of Foo is memoized.

[solnic]

This will work but it'll be evaluated in the constructor. It will not work if you use :lazy => true and don't read the value prior freezing.

[mbj]

IMHO we need to change this. Why we expanded defaults on freeze prior to your refactoring?

We had:

• Lazy defaults that where expanded prior to freeze

We now have:

• Constructor defaults
• Lazy defaults that where not expanded prior freezing and accessing them will cause exceptions far away

Why that change? I like it to have constructor defaults, but why drop a sane behavior that followed POLS?

[solnic]

Why do you think POLS would apply here? If you mark something as lazy then you are aware that this value will not be set until you access an attribute for the first time.

Unless we want to establish a contract that ALL default values are set automatically when freezing an object. Keep in mind that this may introduce issues when one default value depends on a value of some other attribute.

[mbj]

If I access an attribute I expect to receive the attributes value. I do not expect to mutate an object via accessing an attribute. And I do not expect I should track this fact while programming against the attributes.

So it would be a big surprise when I access an attribute of an frozen object and get an exception related to a forbidden mutation.

I do not want to take into account how the attribute is implemented internally. The contract is:

Send a message to the object with the name and receive the attributes value.

Virtus should take care to implement this contract at all cost. If virtus has an attribute that does state mutation when accessing it the first time, and future state mutations are disallowed by calling #freeze, there should be precaution the contract "send a message and receive a value" still holds. For this reason I think we should expand lazy defaults before freeze.

Call it "Liskov substitution principle", you can change the attributes implementation ( required, constructor-default, lazy-default ) but they still behave the same, also when frozen.

[solnic]

OK let's leave the previous behavior of #freeze. I'm still not sure if it's a good idea but since that's how it used to work I'll keep it that way.

[mbj]

Yeah, thx. I know it can cause problems with circular attribute default dependencies.

Lazy attribute A needs lazy attribute B, B needs A ;)

But this is not related to expansion prior to #freeze, will happen in an unfrozen environment also.

[solnic]

@mbj I restored freeze like it used to be :) I think I'm gonna merge it in if there are no more comments /cc @dkubb

[mbj]

@solnic Thx!

[solnic]

@mbj @dkubb one more idea I have is to reduce number of instance methods that virtus adds. I'm wondering if it'd be a good idea to introduce an object that's responsible for most of the things that are defined in InstanceMethods module. Maybe it could allow to remove some of the methods.

[mbj]

@solnic I have a similar object with anima on the singleton.

class Foo
  include Anima.new(:bar, :baz)
end

Foo.anima.attributes
Foo.anima.attribute_names

I like this pattern, and I think virtus would benefit!

I'll extend anima so it supports

class Bar
  include Foo.anima.extend(:new_attribute)
end

This way anima supports "always frozen" state objects.

[solnic]

@mbj yeah I suppose we could just use attribute set

[solnic]

I moved get_attributes and set_attributes from InstanceMethods to AttributeSet and removed get_attribute and set_attribute since both felt redundant. Since it's our internal API I think it's better to keep it there and avoid adding private methods to objects extended by Virtus. Those methods mutate received object but I don't think it's bad since, as I said, it's our internal API.

I also noticed that []= used __send__ which basically allowed using private writer methods. I think it was a bug so I changed it to use public_send send.

I'm going to clean this up too since the code is not as clean as I would like it to be so stay tuned :smile:

[blambeau]

I also noticed that []= used send which basically allowed using private writer methods. I think it was a bug so I changed it to use public_send send.

@solnic []= using __send__ is an important vector for recent PoCs demonstrating YAML vulnerability. Since then, I would be tempted to say that we should all avoid that, especially on public interfaces. I don't know whether it is important here; I just wanted to draw your attention though.

EDIT: __send__ and public_send are probably much safer than eval but still...

[solnic]

I can add a guard statement checking if attribute with given name actually exists. Then use public_send.

Thanks for pointing this out!

On Saturday, 9 February 2013 at 16:43, Bernard Lambeau wrote:

I also noticed that []= used send which basically allowed using private writer methods. I think it was a bug so I changed it to use public_send send.

@solnic (https://github.com/solnic) []= using send is an important vector for recent PoCs demonstrating YAML vulnerability. Since then, I would be tempted to say that we should all avoid that, especially on public interfaces. I don't know whether this is important here; I just wanted to draw your attention though.

— Reply to this email directly or view it on GitHub (https://github.com/solnic/virtus/pull/149#issuecomment-13332800)..

[mbj]

@blambeau I'm developing conversion algebra that can be used to whitelist specific keys. I plan to use this as the world fencing interface convergin params-hash to attributes-hash. Feel free to take a look at https://github.com/mbj/ducktrap.

[blambeau]

@solnic yeah that might be a good idea. I bet that people will start whitelisting their classes for YAML deserialization and I can imagine that virtus objects will be candidates for that. They should only do that if their class is safe but will not necessarily check it themselves. I'd take all possible precautions here therefore.

[blambeau]

@mbj I'm not sure to understand how ducktrap works (just checked spec very quickly) and will prevent what I point here. It would be nice if the README could add some user-oriented examples.

[mbj]

@bambeau I'll add this after the weekend. Sorry for referencing a lib that is not well documented.