On Monday, November 5, 2018 at 9:49:50 PM UTC-8, Jeremy Evans wrote:
>
> 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.
>
OK, I committed a patch that I hope will fix this
issue:
https://github.com/jeremyevans/sequel/commit/b6e19b9811db51e8d7891522f175e2a47ebc00f1
Please give that a shot and let me know whether you think further
modifications are needed.
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.
