Hey sorry for the delayed response, your patch fixed the issue in our test
case, but I wanted to make sure it worked in our full application as well
which took a while.
I think the reasoning makes sense (if the cache was set for a previously
nil foreign key then assume it was set correctly), unfortunately I think
that same reasoning possibly introduces a bug, which I've duplicated in the
end of the original gist w/ the patch applied.
If you happen to explicitly set the association to nil on a new object it
will be cached as having been set to nil. Then if you set the foreign key
later you will get the cached value of nil when calling the association
method rather than loading the association. The test case I added is
contrived but we actually ran into it in our application. This seems like
it could be surprising behavior.
I believe it could be fixed by changing the logic to "if the cache was set
to something other than nil for a previously nil foreign key then assume it
was set correctly", which I tried w/:
return super if c.nil? && !assocs.all?{|a| associations[a].nil?}
I think there is still a caveat though where someone could do:
Track.new \
release: Release.new(...),
release_id: 1
The association cache will be set to the new release, then when being set
to 1 the previous value will have been nil and the association cache will
be left alone referencing the previous release. The expectation would
probably be that setting release_id = 1 would clear the cached association
and track.release would return Release[1].
On Monday, November 5, 2018 at 3:04:21 PM UTC-8, Jeremy Evans wrote:
>
> On Monday, November 5, 2018 at 2:14:16 PM UTC-8, Ben Alavi wrote:
>>
>> Hey Jeremy,
>>
>> Thanks for the fast and thorough response! That makes sense and explains
>> why it is being validated multiple times.
>>
>> We actually ran into this issue in separate places: both in validate and
>> before_save hooks on the associated objects, but I was trying to keep my
>> example from becoming too convoluted and thought they were related.
>>
>> Since we've been digging some more it seems like there are actually two
>> separate things happening in our case: Not being able to skip the 2nd
>> validation as you mentioned, which validating once would solve, and then
>> the release_id on the track changing from nil to the actual value on save,
>> automatically clearing the reciprocal association due
>> to association_autoreloading (which it looks like is now baked in), which
>> then causes the association to be reloaded by the time any _save hooks are
>> run on the associated objects.
>>
>> We haven't figured out yet if the 2nd case is just how it needs to work
>> or if there is somewhere we can add a special case for nested_attributes
>> when creating the reciprocal object at the same time as the related objects
>> -- in which case I *think* we know that the cached reciprocal reference is
>> still valid. You bring up a good point though: it seems like if it does
>> actually make sense, we should try to contain it to the nested_attributes
>> plugin.
>>
>
> I've added a couple commits to the master branch that should fix the
> double validation issue.
>
> In terms of the association autoreloading issue, can you try the following
> patch and let me know whether it works for you?:
>
> diff --git a/lib/sequel/model/associations.rb
> b/lib/sequel/model/associations.rb
> index bd86f5520..ae01e73cf 100644
> --- a/lib/sequel/model/associations.rb
> +++ b/lib/sequel/model/associations.rb
> @@ -2412,6 +2412,12 @@ module Sequel
> # a higher level for existing objects.
> vals = @values
> return super unless !vals.include?(column) || value != (c =
> vals[column]) || value.class != c.class
> +
> + # If this is a new instance, and the current value is nil,
> + # but the association is already present in the cache, it
> was
> + # probably added to the cache for a reason, and we do not
> + # want to delete it in that case.
> + return super if c.nil?
> end
>
> assocs.each{|a| associations.delete(a)}
>
>
> This isn't specific to the nested_attributes plugin, but I think if you
> have a new object and a nil value for the foreign key, but the associations
> cache is present, it's probably better to skip the cache removal in that
> case. If that works for you I can probably commit that (it doesn't break
> any tests). If that doesn't work for some reason, please post back with an
> example of the autoreloading issue and I'll take a deeper look.
>
> Thanks,
> Jeremy
>
--
You received this message because you are subscribed to the Google Groups
"sequel-talk" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at https://groups.google.com/group/sequel-talk.
For more options, visit https://groups.google.com/d/optout.
