354345 - Set isStoredWithId for RSS 2.0 feed items

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Description

[Wladimir Palant]

For some reason item.isStoredWithId is only set for IETF Atom feeds at the moment, for all other feeds the items are stored by their URL. This sometimes causes duplicated entries, e.g. blogs.msdn.com (most prominently the IEBlog) changed the URLs regularly in the past - they alternated between the server's name and its IP. The GUID stayed the same of course.

Comment 1

[Wladimir Palant]

Attachment: Proposed patch

Comment 2

[David :Bienvenu]

Comment on attachment 240170 [details] [diff] [review]
Proposed patch

Scott, should Robert review this? I really have no idea if this is the right thing to do or not...

Comment 3

[Scott MacGregor]

Comment on attachment 240170 [details] [diff] [review]
Proposed patch

Let's get Robert to review this change and David can sr.

Comment 4

[Robert Sayre]

I'll take a look at this today. I didn't set this for RSS2 because I was concerned about existing items. We were starting fresh with the Atom stuff, so dupes weren't a concern.

Comment 5

[Rimas Kudelis [use needinfo to ping]]

Note: in bug 258465, the duplicates appear even if the URL hasn't changed.

Comment 6

[Wladimir Palant]

Ok, that's some explanation at least. We could use the URL as fallback if we don't find the GUID. Would add something like this to the isStored method:

if (!downloaded && itemURI != this.mURL)
{
  itemResource = rdf.GetResource(this.mURL);
  downloaded = ds.GetTarget(itemResource, FZ_STORED, true);
}

Would this be acceptable? This code could them be removed in some later version of Thunderbird.

Comment 7

[Wladimir Palant]

Rimas, this bug is only about solving one part of bug 258465. There are probably more issues causing duplicated entries but they are not subject of this bug.

Comment 8

[Robert Sayre]

(In reply to comment #6)
> Ok, that's some explanation at least. 

Well, IIRC, the Atom code went in pretty late before TB1.5. Changing RSS2 wouldn't have been an option at that point.

> Would this be acceptable? This code could them be removed in some later version
> of Thunderbird.
> 

Yeah, this works. We'll need a new patch with a comment explaining why this check exists. Thanks for digging into this.

Comment 9

[Wladimir Palant]

http://adblockplus.org/trash/test_feed.pl is testcase - this feed will change URL randomly generating new items on average every second time you download it.

Comment 10

[Wladimir Palant]

Attachment: Patch with fallback

Added fallback code to the patch - if we can't find an item by its ID we'll try the URL now. Tested: testcase stops generating duplicate items, and IEBlog items aren't re-added after the update.

Comment 11

[Robert Sayre]

Comment on attachment 240183 [details] [diff] [review]
Patch with fallback

r=sayrer

Comment 12

[alta88]

(In reply to comment #9)
> http://adblockplus.org/trash/test_feed.pl is testcase - this feed will change
> URL randomly generating new items on average every second time you download it.
> 

@wp - thanks...

testcase caught bug [url=https://bugzilla.mozilla.org/show_bug.cgi?id=297569]297569[/url] when subscribing.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20060925 Thunderbird/2.0b1pre ID:2006092503

Comment 13

[Wladimir Palant]

Something is wrong with that patch. Restored my feeditems.rdf from before applying this patch - the first download of an RSS 2.0 feed is fine but the second re-adds all the items. feeditems.rdf doesn't contain the items stored by URL any more then, trying to find out why they were removed...

Comment 14

[Wladimir Palant]

Attachment: Patch with fallback 2

Found the issue - we need to add the same fallback check to item.markValid(). The way it was markValid() would mark the RDF resource built from ID instead of URL for old items - as a result we get one RDF node that is "valid" but not stored and another that is stored but not valid. The latter gets removed then.

Requesting review again, just in case. This time everything really seems to work.

Comment 15

[Robert Sayre]

Comment on attachment 241361 [details] [diff] [review]
Patch with fallback 2

good catch, sorry I missed that.

Comment 16

[Wladimir Palant]

Comment on attachment 241361 [details] [diff] [review]
Patch with fallback 2

With the revised Thunderbird 2.0 roadmap - any chance for this to get in?

Comment 17

[Scott MacGregor]

Comment on attachment 241361 [details] [diff] [review]
Patch with fallback 2

This can land on the branch and the trunk. Thanks for the patch!

Comment 18

[Robert Sayre]

landed on trunk.

Checking in feed-parser.js;
/cvsroot/mozilla/mail/extensions/newsblog/content/feed-parser.js,v  <--  feed-parser.js
new revision: 1.19; previous revision: 1.18
done
Checking in FeedItem.js;
/cvsroot/mozilla/mail/extensions/newsblog/content/FeedItem.js,v  <--  FeedItem.js
new revision: 1.39; previous revision: 1.38
done

Comment 19

[Scott MacGregor]

and I just landed it for thunderbird 2. thanks for the patch!

Comment 20

[Scott MacGregor]

Wladimir, could this patch have caused Bug 363154 ?