davydovanton / shallow_attributes

Simple and lightweight Virtus analog.
MIT License
101 stars 18 forks source link

`present: true` not working as expected on embedded values #25

Open jits opened 5 years ago

jits commented 5 years ago

Hi – firstly, thank you for this library!

I've noticed that using present: true on embedded values throws an error (seemingly every time).

To reproduce: if you add , present: true to https://github.com/davydovanton/shallow_attributes/blob/9a9c55eacf25343489c456a9d979d91ac54bc573/test/custom_types_test.rb#L6 the test will fail with error messages like:

ShallowAttributes::MissingAttributeError: Mandatory attribute "name" was not provided
    /Users/jits/Src/shallow_attributes/lib/shallow_attributes/instance_methods.rb:214:in `block in define_mandatory_attributes'
    /Users/jits/Src/shallow_attributes/lib/shallow_attributes/instance_methods.rb:212:in `each'
    /Users/jits/Src/shallow_attributes/lib/shallow_attributes/instance_methods.rb:212:in `define_mandatory_attributes'
    /Users/jits/Src/shallow_attributes/lib/shallow_attributes/instance_methods.rb:40:in `initialize'
    /Users/jits/Src/shallow_attributes/lib/shallow_attributes/type.rb:80:in `new'
    /Users/jits/Src/shallow_attributes/lib/shallow_attributes/type.rb:80:in `type_instance'
    /Users/jits/Src/shallow_attributes/lib/shallow_attributes/type.rb:57:in `coerce'
    /Users/jits/Src/shallow_attributes/lib/shallow_attributes/class_methods.rb:117:in `city='
    /Users/jits/Src/shallow_attributes/lib/shallow_attributes/instance_methods.rb:227:in `block in define_attributes'
    /Users/jits/Src/shallow_attributes/lib/shallow_attributes/instance_methods.rb:226:in `each'
    /Users/jits/Src/shallow_attributes/lib/shallow_attributes/instance_methods.rb:226:in `define_attributes'
    /Users/jits/Src/shallow_attributes/lib/shallow_attributes/instance_methods.rb:93:in `attributes='
    /Users/jits/Src/shallow_attributes/lib/shallow_attributes/instance_methods.rb:142:in `coerce'
    /Users/jits/Src/shallow_attributes/lib/shallow_attributes/type.rb:57:in `coerce'
    /Users/jits/Src/shallow_attributes/lib/shallow_attributes/type/array.rb:31:in `block in coerce'
    /Users/jits/Src/shallow_attributes/lib/shallow_attributes/type/array.rb:30:in `map!'
    /Users/jits/Src/shallow_attributes/lib/shallow_attributes/type/array.rb:30:in `coerce'
    /Users/jits/Src/shallow_attributes/lib/shallow_attributes/type.rb:57:in `coerce'
    /Users/jits/Src/shallow_attributes/lib/shallow_attributes/class_methods.rb:117:in `addresses='
    /Users/jits/Src/shallow_attributes/lib/shallow_attributes/instance_methods.rb:227:in `block in define_attributes'
    /Users/jits/Src/shallow_attributes/lib/shallow_attributes/instance_methods.rb:226:in `each'
    /Users/jits/Src/shallow_attributes/lib/shallow_attributes/instance_methods.rb:226:in `define_attributes'
    /Users/jits/Src/shallow_attributes/lib/shallow_attributes/instance_methods.rb:38:in `initialize'
    /Users/jits/Src/shallow_attributes/test/custom_types_test.rb:62:in `new'
    /Users/jits/Src/shallow_attributes/test/custom_types_test.rb:62:in `block (4 levels) in <top (required)>'

I may be able to take a closer look and work on a PR when I have more time; any pointers are gladly welcome.

davydovanton commented 5 years ago

Hey, thanks for report. Will check it when will have time (hope on new years holidays) 👍

X1ting commented 5 years ago

So, I found the root cause of the issue. We are defining all mandatory attributes and only after it we are creating nested classes. So, in this step -> https://github.com/davydovanton/shallow_attributes/blob/master/lib/shallow_attributes/type.rb#L80 we have mandatory attributes for User, but we call .new without args and it threw an exception. I haven't ideas how to fix it properly, just thoughts. We have two ways:

  1. Define nested mandatory attributes later
  2. Call nested Class.new with params @jits @davydovanton WDYT ?
jits commented 5 years ago

@X1ting – nice find. To be honest, I'm not sure I fully understand the impact, but your option (2) sounds like it makes the most sense and is in line with how shallow_attributes expects objects to be initialised (i.e. all required data must be provided at object initialisation). Having the nested objects initialised in the same way makes a lot of sense.

Let me know if there's something I can help further with.