Date filtered queries shouldn't be cached

[Scott Gray-3]

In the current project I'm working on I see a lot of developers making the
mistake of caching date filtered queries.

Why shouldn't you cache date filtered queries?
1. If you're filtering by the current moment in time then the results are
irrelevant within a few moments of them being retrieved.  Some records may
be due to expire and others (that were filtered out) may be about to become
active.
2. Despite what I say in #1, there's a bug in the equals method of
EntityDateFilterCondition where all instances appear equal so the cached
result is never replaced until it is expired from the cache. So expired
records remain and future dated records never appear until the TTL is
reached.

The correct approach is to not have a date filter in the query, cache the
result and then filter the result in-memory using
EntityUtil.filterByDate(). An alternative approach is to only filter by
thruDate in the query since expired records will never become active, and
then filter by fromDate after you have the result.  We have no utility
tools in place for the latter approach though.

I'm writing all this because I've written a checker for the entity list and
object caches that inspect the query conditions for fromDate conditions and
if found, log a warning and refuse to cache.  I'd like to add this safety
feature to the OFBiz trunk but I'm wondering if anyone can think of a valid
reason to cache fromDate filtered records? I haven't been able to think of
any but that doesn't necessarily mean there aren't any use cases.

Thanks
Scott

[Swapnil Mane]

Thank you so much Scott for the detailed notes.
Indeed, will be helpful for writing quality code.


- Best Regards,
Swapnil M Mane

On Thu, Apr 6, 2017 at 4:40 AM, Scott Gray <[hidden email]>
wrote:

> In the current project I'm working on I see a lot of developers making the
> mistake of caching date filtered queries.
>
> Why shouldn't you cache date filtered queries?
> 1. If you're filtering by the current moment in time then the results are
> irrelevant within a few moments of them being retrieved.  Some records may
> be due to expire and others (that were filtered out) may be about to become
> active.
> 2. Despite what I say in #1, there's a bug in the equals method of
> EntityDateFilterCondition where all instances appear equal so the cached
> result is never replaced until it is expired from the cache. So expired
> records remain and future dated records never appear until the TTL is
> reached.
>
> The correct approach is to not have a date filter in the query, cache the
> result and then filter the result in-memory using
> EntityUtil.filterByDate(). An alternative approach is to only filter by
> thruDate in the query since expired records will never become active, and
> then filter by fromDate after you have the result.  We have no utility
> tools in place for the latter approach though.
>
> I'm writing all this because I've written a checker for the entity list and
> object caches that inspect the query conditions for fromDate conditions and
> if found, log a warning and refuse to cache.  I'd like to add this safety
> feature to the OFBiz trunk but I'm wondering if anyone can think of a valid
> reason to cache fromDate filtered records? I haven't been able to think of
> any but that doesn't necessarily mean there aren't any use cases.
>
> Thanks
> Scott
>

[taher]

Hi Scott,

Maybe I am not understanding your solution correctly, so I summarize what I
understood:

- Monitor all EntityCondition creations from
EntityDateFilterCondition,makeCondition
or EntityUtil.filterByDate
- If the query has a field called "fromDate" then log a warning and disable
caching

If my above understanding is correct, then isn't that a bit strange? I mean
it seems to be quite a specific workaround as opposed to a generic
solution. Shouldn't we instead have something that works for all dates
instead of just specific dates with the name "fromDate" when compared with
current date / time?

On Thu, Apr 6, 2017 at 2:10 AM, Scott Gray <[hidden email]>
wrote:

> In the current project I'm working on I see a lot of developers making the
> mistake of caching date filtered queries.
>
> Why shouldn't you cache date filtered queries?
> 1. If you're filtering by the current moment in time then the results are
> irrelevant within a few moments of them being retrieved.  Some records may
> be due to expire and others (that were filtered out) may be about to become
> active.
> 2. Despite what I say in #1, there's a bug in the equals method of
> EntityDateFilterCondition where all instances appear equal so the cached
> result is never replaced until it is expired from the cache. So expired
> records remain and future dated records never appear until the TTL is
> reached.
>
> The correct approach is to not have a date filter in the query, cache the
> result and then filter the result in-memory using
> EntityUtil.filterByDate(). An alternative approach is to only filter by
> thruDate in the query since expired records will never become active, and
> then filter by fromDate after you have the result.  We have no utility
> tools in place for the latter approach though.
>
> I'm writing all this because I've written a checker for the entity list and
> object caches that inspect the query conditions for fromDate conditions and
> if found, log a warning and refuse to cache.  I'd like to add this safety
> feature to the OFBiz trunk but I'm wondering if anyone can think of a valid
> reason to cache fromDate filtered records? I haven't been able to think of
> any but that doesn't necessarily mean there aren't any use cases.
>
> Thanks
> Scott
>

[Scott Gray-3]

Hi Taher,

Thanks for taking a look.

It's not about monitoring EntityCondition creations, but about inspecting
the EntityCondition supplied with the cache request e.g.
LocalCache.put(...) to determine if it contains an "uncachable" condition.

Regarding looking for "fromDate" vs. something more generic, I'm not sure I
follow.  from/thruDates in OFBiz are an established pattern for determining
whether a record is active or not.  Other timestamp fields serve other
purposes and the issues I've described don't apply to them.  Yes, I would
be relying upon the convention of naming a fromDate as "fromDate" (actually
more like fieldName.toLowerCase().contains("fromdate")) but that's a pretty
safe assumption to make isn't it?

Back to my original question, it occurs to me that it might be possible a
developer would want to retrieve all rows that would be valid on a specific
date other than "right now".  I can't think of a concrete scenario in which
that might be useful but it does give me pause and perhaps it might be
safer to err on the side of caution and only look for
EntityDateFilterConditions which are 100% guaranteed to fit the scenario of
results we don't want in the cache.

Regards
Scott

On 7 April 2017 at 00:34, Taher Alkhateeb <[hidden email]>
wrote:

> Hi Scott,
>
> Maybe I am not understanding your solution correctly, so I summarize what I
> understood:
>
> - Monitor all EntityCondition creations from
> EntityDateFilterCondition,makeCondition
> or EntityUtil.filterByDate
> - If the query has a field called "fromDate" then log a warning and disable
> caching
>
> If my above understanding is correct, then isn't that a bit strange? I mean
> it seems to be quite a specific workaround as opposed to a generic
> solution. Shouldn't we instead have something that works for all dates
> instead of just specific dates with the name "fromDate" when compared with
> current date / time?
>
> On Thu, Apr 6, 2017 at 2:10 AM, Scott Gray <[hidden email]>
> wrote:
>
> > In the current project I'm working on I see a lot of developers making
> the
> > mistake of caching date filtered queries.
> >
> > Why shouldn't you cache date filtered queries?
> > 1. If you're filtering by the current moment in time then the results are
> > irrelevant within a few moments of them being retrieved.  Some records
> may
> > be due to expire and others (that were filtered out) may be about to
> become
> > active.
> > 2. Despite what I say in #1, there's a bug in the equals method of
> > EntityDateFilterCondition where all instances appear equal so the cached
> > result is never replaced until it is expired from the cache. So expired
> > records remain and future dated records never appear until the TTL is
> > reached.
> >
> > The correct approach is to not have a date filter in the query, cache the
> > result and then filter the result in-memory using
> > EntityUtil.filterByDate(). An alternative approach is to only filter by
> > thruDate in the query since expired records will never become active, and
> > then filter by fromDate after you have the result.  We have no utility
> > tools in place for the latter approach though.
> >
> > I'm writing all this because I've written a checker for the entity list
> and
> > object caches that inspect the query conditions for fromDate conditions
> and
> > if found, log a warning and refuse to cache.  I'd like to add this safety
> > feature to the OFBiz trunk but I'm wondering if anyone can think of a
> valid
> > reason to cache fromDate filtered records? I haven't been able to think
> of
> > any but that doesn't necessarily mean there aren't any use cases.
> >
> > Thanks
> > Scott
> >
>

[Jacques Le Roux]

Hi Scott,

+1 on both your propositions below

Jacques


Le 07/04/2017 à 06:04, Scott Gray a écrit :

> Hi Taher,
>
> Thanks for taking a look.
>
> It's not about monitoring EntityCondition creations, but about inspecting
> the EntityCondition supplied with the cache request e.g.
> LocalCache.put(...) to determine if it contains an "uncachable" condition.
>
> Regarding looking for "fromDate" vs. something more generic, I'm not sure I
> follow.  from/thruDates in OFBiz are an established pattern for determining
> whether a record is active or not.  Other timestamp fields serve other
> purposes and the issues I've described don't apply to them.  Yes, I would
> be relying upon the convention of naming a fromDate as "fromDate" (actually
> more like fieldName.toLowerCase().contains("fromdate")) but that's a pretty
> safe assumption to make isn't it?
>
> Back to my original question, it occurs to me that it might be possible a
> developer would want to retrieve all rows that would be valid on a specific
> date other than "right now".  I can't think of a concrete scenario in which
> that might be useful but it does give me pause and perhaps it might be
> safer to err on the side of caution and only look for
> EntityDateFilterConditions which are 100% guaranteed to fit the scenario of
> results we don't want in the cache.
>
> Regards
> Scott
>
> On 7 April 2017 at 00:34, Taher Alkhateeb <[hidden email]>
> wrote:
>
>> Hi Scott,
>>
>> Maybe I am not understanding your solution correctly, so I summarize what I
>> understood:
>>
>> - Monitor all EntityCondition creations from
>> EntityDateFilterCondition,makeCondition
>> or EntityUtil.filterByDate
>> - If the query has a field called "fromDate" then log a warning and disable
>> caching
>>
>> If my above understanding is correct, then isn't that a bit strange? I mean
>> it seems to be quite a specific workaround as opposed to a generic
>> solution. Shouldn't we instead have something that works for all dates
>> instead of just specific dates with the name "fromDate" when compared with
>> current date / time?
>>
>> On Thu, Apr 6, 2017 at 2:10 AM, Scott Gray <[hidden email]>
>> wrote:
>>
>>> In the current project I'm working on I see a lot of developers making
>> the
>>> mistake of caching date filtered queries.
>>>
>>> Why shouldn't you cache date filtered queries?
>>> 1. If you're filtering by the current moment in time then the results are
>>> irrelevant within a few moments of them being retrieved.  Some records
>> may
>>> be due to expire and others (that were filtered out) may be about to
>> become
>>> active.
>>> 2. Despite what I say in #1, there's a bug in the equals method of
>>> EntityDateFilterCondition where all instances appear equal so the cached
>>> result is never replaced until it is expired from the cache. So expired
>>> records remain and future dated records never appear until the TTL is
>>> reached.
>>>
>>> The correct approach is to not have a date filter in the query, cache the
>>> result and then filter the result in-memory using
>>> EntityUtil.filterByDate(). An alternative approach is to only filter by
>>> thruDate in the query since expired records will never become active, and
>>> then filter by fromDate after you have the result.  We have no utility
>>> tools in place for the latter approach though.
>>>
>>> I'm writing all this because I've written a checker for the entity list
>> and
>>> object caches that inspect the query conditions for fromDate conditions
>> and
>>> if found, log a warning and refuse to cache.  I'd like to add this safety
>>> feature to the OFBiz trunk but I'm wondering if anyone can think of a
>> valid
>>> reason to cache fromDate filtered records? I haven't been able to think
>> of
>>> any but that doesn't necessarily mean there aren't any use cases.
>>>
>>> Thanks
>>> Scott
>>>

[Suraj Khurana]

+1

I have created a Jira <https://issues.apache.org/jira/browse/OFBIZ-10579>
for this as per suggestion from Scott.

--
Thanks and Regards,
Suraj Khurana
Omnichannel OMS Technical Expert
HotWax Systems





On Tue, Apr 11, 2017 at 1:31 PM, Jacques Le Roux <
[hidden email]> wrote:

> Hi Scott,
>
> +1 on both your propositions below
>
> Jacques
>
>
>
> Le 07/04/2017 à 06:04, Scott Gray a écrit :
>
>> Hi Taher,
>>
>> Thanks for taking a look.
>>
>> It's not about monitoring EntityCondition creations, but about inspecting
>> the EntityCondition supplied with the cache request e.g.
>> LocalCache.put(...) to determine if it contains an "uncachable" condition.
>>
>> Regarding looking for "fromDate" vs. something more generic, I'm not sure
>> I
>> follow.  from/thruDates in OFBiz are an established pattern for
>> determining
>> whether a record is active or not.  Other timestamp fields serve other
>> purposes and the issues I've described don't apply to them.  Yes, I would
>> be relying upon the convention of naming a fromDate as "fromDate"
>> (actually
>> more like fieldName.toLowerCase().contains("fromdate")) but that's a
>> pretty
>> safe assumption to make isn't it?
>>
>> Back to my original question, it occurs to me that it might be possible a
>> developer would want to retrieve all rows that would be valid on a
>> specific
>> date other than "right now".  I can't think of a concrete scenario in
>> which
>> that might be useful but it does give me pause and perhaps it might be
>> safer to err on the side of caution and only look for
>> EntityDateFilterConditions which are 100% guaranteed to fit the scenario
>> of
>> results we don't want in the cache.
>>
>> Regards
>> Scott
>>
>> On 7 April 2017 at 00:34, Taher Alkhateeb <[hidden email]>
>> wrote:
>>
>> Hi Scott,
>>>
>>> Maybe I am not understanding your solution correctly, so I summarize
>>> what I
>>> understood:
>>>
>>> - Monitor all EntityCondition creations from
>>> EntityDateFilterCondition,makeCondition
>>> or EntityUtil.filterByDate
>>> - If the query has a field called "fromDate" then log a warning and
>>> disable
>>> caching
>>>
>>> If my above understanding is correct, then isn't that a bit strange? I
>>> mean
>>> it seems to be quite a specific workaround as opposed to a generic
>>> solution. Shouldn't we instead have something that works for all dates
>>> instead of just specific dates with the name "fromDate" when compared
>>> with
>>> current date / time?
>>>
>>> On Thu, Apr 6, 2017 at 2:10 AM, Scott Gray <[hidden email]
>>> >
>>> wrote:
>>>
>>> In the current project I'm working on I see a lot of developers making
>>>>
>>> the
>>>
>>>> mistake of caching date filtered queries.
>>>>
>>>> Why shouldn't you cache date filtered queries?
>>>> 1. If you're filtering by the current moment in time then the results
>>>> are
>>>> irrelevant within a few moments of them being retrieved.  Some records
>>>>
>>> may
>>>
>>>> be due to expire and others (that were filtered out) may be about to
>>>>
>>> become
>>>
>>>> active.
>>>> 2. Despite what I say in #1, there's a bug in the equals method of
>>>> EntityDateFilterCondition where all instances appear equal so the cached
>>>> result is never replaced until it is expired from the cache. So expired
>>>> records remain and future dated records never appear until the TTL is
>>>> reached.
>>>>
>>>> The correct approach is to not have a date filter in the query, cache
>>>> the
>>>> result and then filter the result in-memory using
>>>> EntityUtil.filterByDate(). An alternative approach is to only filter by
>>>> thruDate in the query since expired records will never become active,
>>>> and
>>>> then filter by fromDate after you have the result.  We have no utility
>>>> tools in place for the latter approach though.
>>>>
>>>> I'm writing all this because I've written a checker for the entity list
>>>>
>>> and
>>>
>>>> object caches that inspect the query conditions for fromDate conditions
>>>>
>>> and
>>>
>>>> if found, log a warning and refuse to cache.  I'd like to add this
>>>> safety
>>>> feature to the OFBiz trunk but I'm wondering if anyone can think of a
>>>>
>>> valid
>>>
>>>> reason to cache fromDate filtered records? I haven't been able to think
>>>>
>>> of
>>>
>>>> any but that doesn't necessarily mean there aren't any use cases.
>>>>
>>>> Thanks
>>>> Scott
>>>>
>>>>
>