On 14 August 2017 at 10:58, Daniel Gruno <[email protected]> wrote:
> On 08/14/2017 11:50 AM, sebb wrote:
>> On 14 August 2017 at 10:43, <[email protected]> wrote:
>>> Repository: incubator-ponymail
>>> Updated Branches:
>>> refs/heads/master a5c98e735 -> f09765481
>>>
>>>
>>> Address null entries in favorites list
>>>
>>> This fixes #392 by both using the proper call to remove
>>> favorite entries when needed, and also pruning existing
>>> corrupted entries when an account is loaded.
>>>
>>>
>>> Project: http://git-wip-us.apache.org/repos/asf/incubator-ponymail/repo
>>> Commit:
>>> http://git-wip-us.apache.org/repos/asf/incubator-ponymail/commit/f0976548
>>> Tree:
>>> http://git-wip-us.apache.org/repos/asf/incubator-ponymail/tree/f0976548
>>> Diff:
>>> http://git-wip-us.apache.org/repos/asf/incubator-ponymail/diff/f0976548
>>>
>>> Branch: refs/heads/master
>>> Commit: f09765481bb35c94b005c42f70a28894462a9430
>>> Parents: a5c98e7
>>> Author: Daniel Gruno <[email protected]>
>>> Authored: Mon Aug 14 11:43:10 2017 +0200
>>> Committer: Daniel Gruno <[email protected]>
>>> Committed: Mon Aug 14 11:43:10 2017 +0200
>>>
>>> ----------------------------------------------------------------------
>>> CHANGELOG.md | 1 +
>>> site/api/lib/user.lua | 13 +++++++++++++
>>> site/api/preferences.lua | 2 +-
>>> 3 files changed, 15 insertions(+), 1 deletion(-)
>>> ----------------------------------------------------------------------
>>>
>>>
>>> http://git-wip-us.apache.org/repos/asf/incubator-ponymail/blob/f0976548/CHANGELOG.md
>>> ----------------------------------------------------------------------
>>> diff --git a/CHANGELOG.md b/CHANGELOG.md
>>> index e3a6b10..05fab5d 100644
>>> --- a/CHANGELOG.md
>>> +++ b/CHANGELOG.md
>>> @@ -1,4 +1,5 @@
>>> ## CHANGES in 0.10:
>>> +- Fixed an issue where favorites could contain null entries due to missing
>>> GC (#392)
>>> - Added sample configs for Pony Mail (#374)
>>> - Renamed ll.py to list-lists.py (#378)
>>> - ID generators have now been split into a separate library (generators.py)
>>>
>>> http://git-wip-us.apache.org/repos/asf/incubator-ponymail/blob/f0976548/site/api/lib/user.lua
>>> ----------------------------------------------------------------------
>>> diff --git a/site/api/lib/user.lua b/site/api/lib/user.lua
>>> index 57c65ac..e441476 100644
>>> --- a/site/api/lib/user.lua
>>> +++ b/site/api/lib/user.lua
>>> @@ -17,6 +17,7 @@
>>>
>>> local elastic = require 'lib/elastic'
>>> local config = require 'lib/config'
>>> +local JSON = require 'cjson'
>>>
>>> -- allow local override of secure cookie attribute
>>> -- Note: the config item is named to make it more obvious that enabling it
>>> is not recommended
>>> @@ -32,6 +33,18 @@ local function getUser(r, override)
>>> if override or (cookie and #cookie >= 40 and cid) then
>>> local js = elastic.get('account', r:sha1(override or cid),
>>> true)
>>> if js and js.credentials and (override or (cookie ==
>>> js.internal.cookie)) then
>>> +
>>> + -- Issue #392: favorites may contain null entries, cull
>>> them.
>>> + if type(js.favorites) == "table" then
>>> + local jsfav = {}
>>> + for k, v in pairs(js.favorites) do
>>> + if v and v ~= JSON.null then
>>> + table.insert(jsfav, v)
>>> + end
>>> + end
>>> + js.favorites = jsfav
>>> + end
>>> +
>>
>> The above is a temporary fix and should be documented as such.
>
> Temporary for how long though? We don't control when people upgrade, so
> the issue could be present tomorrow, a year from now, whenever. I'd hold
> off on calling it temporary till there's something that can actually
> replace this. I don't currently know what a more permanent solution
> would look like, save maybe a python script to clean up the database,
> which admins would then have to run manually on the box.
It's a temporary fix because:
- it does not affect new installations
- it only affects a small percentage of existing users in current installations
- it only requires a once-off admin job to tidy up any existing entries
Yes, it's a bit more work to create such a script.
But that's sometimes necessary to tidy up buggy data.
If it were really difficult to write a script to fix the database
entries, then I agree that a work-round such as this might be worth
considering.
But that is not the case here.
So I think I am actually now -1 on the work-round.
> We can document in the code that it's not the _optimal_ fix, perhaps -
> something for people to look at if they wanna try out contributing to PM...
>> It's not needed for new installations, nor ones where the erroneous
>> null entries have been manually removed from the database.
>>
>>> login = {
>>> credentials = {
>>> email = js.credentials.email,
>>>
>>> http://git-wip-us.apache.org/repos/asf/incubator-ponymail/blob/f0976548/site/api/preferences.lua
>>> ----------------------------------------------------------------------
>>> diff --git a/site/api/preferences.lua b/site/api/preferences.lua
>>> index 384cf1e..7f20f65 100644
>>> --- a/site/api/preferences.lua
>>> +++ b/site/api/preferences.lua
>>> @@ -253,7 +253,7 @@ Pony Mail - Email for Ponies and People.
>>> -- ensure it's here....
>>> for k, v in pairs(favs) do
>>> if v == rem then
>>> - favs[k] = nil
>>> + table.remove(favs, k)
>>> break
>>> end
>>> end
>>>
>
