On Tuesday, November 6, 2018 at 8:52:03 AM UTC-8, Jeremy Evans wrote:
>
> 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
>
Sorry was a busy day!
Had a chance to run tests w/ this version against our app, everything
worked and in our case it results in about a 5x performance improvement
(turns out we were querying a lot of data in save hooks so it's probably an
extreme example).
The logic looks good to me! I tested that last case I posted as well and
you were right, set_associated_object raises because of the missing pk so
it wouldn't cause an issue.
On a side note while updating our app to 5+ in order to test out these
patches I believe I found a bug, will post as soon as I can reproduce.
Thanks again!,
Ben
--
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.
