Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API] Missing Enclosure #1059

Closed
jangernert opened this issue Feb 28, 2021 · 4 comments · Fixed by #2109
Closed

[API] Missing Enclosure #1059

jangernert opened this issue Feb 28, 2021 · 4 comments · Fixed by #2109
Labels

Comments

@jangernert
Copy link

Running official docker image version 2.0.28

Some feeds offer an "Attachment" / "Enclosure"

Screenshot from 2021-02-28 15-03-36

But the Json of the same article states it doesn't have any enclosures:

{
    "id": 206138,
    "user_id": 1,
    "feed_id": 39,
    "status": "read",
    "hash": "4da9fe4f0283984033725b53e374871ba07007161f7a968751122cd9bb5e20a1",
    "title": "Schalke 04 stellt Trainer Christian Gross und Sportvorstand Jochen Schneider frei",
    "url": "https://www.spiegel.de/sport/fussball/schalke-04-stellt-trainer-christian-gross-und-sportvorstand-jochen-schneider-frei-a-c6a7b8d0-42ed-4e33-8aa9-6f6ce014f6b4#ref=rss",
    "comments_url": "",
    "published_at": "2021-02-28T09:30:00+01:00",
    "created_at": "2021-02-28T09:51:08.116349+01:00",
    "content": "Bundesligist Schalke 04 hat bestätigt, dass Trainer Christian Gross und Sportvorstand Jochen Schneider ihrer Aufgaben entbunden werden. Auch Teammanager Riether und Athletiktrainer Leuthard müssen gehen.",
    "author": "",
    "share_code": "",
    "starred": false,
    "reading_time": 1,
    "enclosures": null,
    "feed": {
        "id": 39,
        "user_id": 1,
        "feed_url": "https://www.spiegel.de/schlagzeilen/tops/index.rss",
        "site_url": "http://www.spiegel.de",
        "title": "SPIEGEL",
        "checked_at": "2021-02-28T14:51:07.494495+01:00",
        "next_check_at": "0001-01-01T00:00:00Z",
        "etag_header": "",
        "last_modified_header": "",
        "parsing_error_message": "",
        "parsing_error_count": 0,
        "scraper_rules": "",
        "rewrite_rules": "",
        "crawler": false,
        "blocklist_rules": "",
        "keeplist_rules": "",
        "user_agent": "",
        "username": "",
        "password": "",
        "disabled": false,
        "ignore_http_cache": false,
        "fetch_via_proxy": false,
        "category": {
            "id": 3,
            "title": "News",
            "user_id": 1
        },
        "icon": {
            "feed_id": 39,
            "icon_id": 35
        }
    }
},
@jangernert jangernert added the bug label Feb 28, 2021
@jangernert
Copy link
Author

Using Get Entry instead of Get Entries works fine for me:

{
    "id": 206138,
    "user_id": 1,
    "feed_id": 39,
    "status": "read",
    "hash": "4da9fe4f0283984033725b53e374871ba07007161f7a968751122cd9bb5e20a1",
    "title": "Schalke 04 stellt Trainer Christian Gross und Sportvorstand Jochen Schneider frei",
    "url": "https://www.spiegel.de/sport/fussball/schalke-04-stellt-trainer-christian-gross-und-sportvorstand-jochen-schneider-frei-a-c6a7b8d0-42ed-4e33-8aa9-6f6ce014f6b4#ref=rss",
    "comments_url": "",
    "published_at": "2021-02-28T09:30:00+01:00",
    "created_at": "2021-02-28T09:51:08.116349+01:00",
    "content": "Bundesligist Schalke 04 hat bestätigt, dass Trainer Christian Gross und Sportvorstand Jochen Schneider ihrer Aufgaben entbunden werden. Auch Teammanager Riether und Athletiktrainer Leuthard müssen gehen.",
    "author": "",
    "share_code": "",
    "starred": false,
    "reading_time": 1,
    "enclosures": [
        {
            "id": 189282,
            "user_id": 1,
            "entry_id": 206138,
            "url": "https://cdn.prod.www.spiegel.de/images/81313d36-1956-4c94-bc61-67277803f119_w520_r2.08_fpx38.67_fpy50.jpg",
            "mime_type": "image/jpeg",
            "size": 0
        }
    ],
    "feed": {
        "id": 39,
        "user_id": 1,
        "feed_url": "https://www.spiegel.de/schlagzeilen/tops/index.rss",
        "site_url": "http://www.spiegel.de",
        "title": "SPIEGEL",
        "checked_at": "2021-02-28T14:51:07.494495+01:00",
        "next_check_at": "0001-01-01T00:00:00Z",
        "etag_header": "",
        "last_modified_header": "",
        "parsing_error_message": "",
        "parsing_error_count": 0,
        "scraper_rules": "",
        "rewrite_rules": "",
        "crawler": false,
        "blocklist_rules": "",
        "keeplist_rules": "",
        "user_agent": "",
        "username": "",
        "password": "",
        "disabled": false,
        "ignore_http_cache": false,
        "fetch_via_proxy": false,
        "category": {
            "id": 3,
            "title": "News",
            "user_id": 1
        },
        "icon": {
            "feed_id": 39,
            "icon_id": 35
        }
    }
}

I don't know golang but looking at the code I noticed this line in GetEntry() which queries the enclosure belonging to the entry and attaching it to the returned entry.

The GetEntries() function right below doesn't seem to do this at all.

@fguillot fguillot added improvements and removed bug labels Mar 23, 2021
@bubelov
Copy link

bubelov commented Sep 12, 2021

Just stumbled upon the same issue. It's probably a perf optimization in order to avoid joins, but it would be really nice to have a way to include enclosures, probably as an optional arg or something.

It's quite the opposite when it comes to feeds though. Here is how most offline-first clients with full-scale cache operate:

  1. Fetch feeds
  2. Fetch entries in batches while output size < max batch size
  3. For subsequent syncs, just query the "delta" (everything with changed_at > max(cached_entries_table.changed_at))

This approach allows two-way sync with reasonable number of HTTP requests and it also avoids querying the same data multiple times.

Based on my experience with Miniflux so far, here is the current state of the API when it comes to this use case:

  1. Flawless, nothing to add
  2. Two issues:
  • No attachments. It would force the client to make N requests in order to fetch attachments for N entries. That obviously breaks the flow, any workaround would be insanely inefficient
  • Every entry includes a feed, which is a lot of JSON, but the client already has all feeds because of step 1. It's not a big deal, but getting rid of feeds in this endpoint would cut the amount of data transferred quite dramatically
  1. I don't really see a way to enable two-way sync. My current approach is to use after_entry_id, but it only fetches new entries. Filtering by changed_at would also include any changes to existing entries made by Miniflux web interface or other clients (changes in read/starred toggles, possibly something else)

PS. I'm also not familiar with Go, but I'm ready to try if maintainers are busy of focused on something else

@Tokariew
Copy link

Tokariew commented Oct 9, 2021

I tried to add enclosures with something like this patch

diff --git a/storage/entry_query_builder.go b/storage/entry_query_builder.go
index 621d15e..6eeae71 100644
--- a/storage/entry_query_builder.go
+++ b/storage/entry_query_builder.go
@@ -337,6 +337,7 @@ func (e *EntryQueryBuilder) GetEntries() (model.Entries, error) {
 		entry.Date = timezone.Convert(tz, entry.Date)
 		entry.CreatedAt = timezone.Convert(tz, entry.CreatedAt)
 		entry.ChangedAt = timezone.Convert(tz, entry.ChangedAt)
+		entry.Enclosures, err = e.store.GetEnclosures(entry.ID)
 		entry.Feed.CheckedAt = timezone.Convert(tz, entry.Feed.CheckedAt)

 		entry.Feed.ID = entry.FeedID

and with python api it look fine, it started to return enclosures which look the same to ones returned by get_feed_entry
on webpage I still get attachments

edit:
not sure how errors should be handled. I don't program with Go…

@Tokariew
Copy link

Tokariew commented Jun 3, 2022

@fguillot Sorry for pinging you… Is solution of mine acceptable to get enclosures when using Get Entries?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

4 participants