On Monday, November 5, 2018 at 6:27:44 PM UTC-8, Ben Alavi wrote:
>
> 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?}
>
Thank you for the additional testing and analysis. I agree with you that
clearing the cache for nil values is something that should still be done to
avoid false negatives. We may want to only clear the nil associations in
that case, instead of clearing all associations if any association contains
nil. There is usually only a single association per key, but there can be
multiple.
I was also thinking it may make sense to do this not just for new objects,
but also for existing objects. If the foreign key value is nil, then the
only time there should be a non-nil value in the associations cache is if
it was put there manually, and the same logic should apply for both new
objects and existing objects. I think that would make things more
consistent.
I'll try to make code modifications and add more tests for this behavior
tomorrow.
> 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].
>
I don't think this would cause a problem, because Release.new wouldn't have
a primary key, and then calling Track#release= would raise an exception in
set_associated_object. If you did use Release#new with a primary key, then
it would set a non-nil foreign key, so the normal cache-clearing logic
would work. I didn't test either situation, that's just my guess from a
quick reading of the code.
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.
