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

feat: Add API route /v1/enclosures/{enclosureId} #2784

Merged
merged 4 commits into from
Aug 18, 2024

Conversation

wolfhechel
Copy link
Contributor

The GET method exposes individual enclosures by their ID. The PUT method allows external clients to update media_progression, similar to the internal API used by the UI.

The media proxy code was refactored to a method on the Enclosure and EnclosureList structures to avoid duplicate code and risking diverging behavior between googlereader and the API.

Do you follow the guidelines?

The GET method exposes individual enclosures by their ID.
The PUT method allows external clients to update media_progression, similiar to the internal API used by the UI.

The media proxy code was refactored to a method on the Enclosure and EnclosureList structures
to avoid duplicate code and risking diverging behavior between googlereader and the API.
@wolfhechel
Copy link
Contributor Author

This PR does not include any integration tests as the test feed does not include any enclosures and I was uncertain if it's good practice to create mock objects to interact with.

Copy link
Member

@fguillot fguillot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if you can update the Go client package at the same time: https://github.com/miniflux/v2/tree/main/client

func (h *handler) getEnclosureById(w http.ResponseWriter, r *http.Request) {
enclosureID := request.RouteInt64Param(r, "enclosureID")

enclosure, err := h.store.GetEnclosure(enclosureID)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GetEnclosure() function needs some enhancements to handle inexistent enclosures. It should return nil in that case.

This API endpoint will return a 500 and show the message below instead of a 404 if the enclosure ID doesn't exist in the database.

{
  "error_message": "store: unable to fetch enclosure row: sql: no rows in result set"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetEnclosure is fixed now, but I would like to add integration tests if I'm to extend the client as well because I'm not using that codebase myself. Any chance I could get an enclosure somewhere in the Miniflux RSS feed that's being used for integration testing @fguillot?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetEnclosure is fixed now

I don't see any update in the pull-request

Any chance I could get an enclosure somewhere in the Miniflux RSS feed that's being used for integration testing

I made some improvements to the feed: https://miniflux.app/feed.xml

There are enclosures now:

<entry>
<title>Miniflux 2.1.4</title>
<link rel="alternate" type="text/html" href="https://miniflux.app/releases/2.1.4.html"/>
<id>https://miniflux.app/releases/2.1.4.html</id>
<updated>2024-07-09T00:00:00Z</updated>
<content type="html">
<![CDATA[ <ul> <li>test: add unit tests for <code>IsModified()</code> behaviour</li> <li>refactor: improve YouTube page feed detection</li> <li>fix(ui): settings form is not populated correctly after validation errors</li> <li>fix(ui): playback speed indicator precision</li> <li>fix(ui): playback speed indicator on shared entries</li> <li>fix(integration): preserve existing Pinboard bookmarks</li> <li>fix(googlereader): set <code>CrawlTimeMsec</code> to the correct precision</li> <li>fix(build): failed to solve container image <code>arm64v8/golang:1.22-bookworm</code></li> <li>fix(build): add <code>distroless</code> suffix on <code>latest</code> tag in GitHub workflow</li> <li>fix: use <code>ETag</code> as a stronger validator than <code>Last-Modified</code></li> <li>fix: update <code>theverge.com</code> rewrite rule to avoid duplicate image</li> <li>fix: incorrect Go package comment <code>reader/readingtime</code></li> <li>fix: error out for improper rewrite regexp when processing feed entries</li> <li>fix: ensures that session cookies are not expiring before the session is cleaned up from the database as per <code>CLEANUP_REMOVE_SESSIONS_DAYS</code></li> <li>fix: <code>&lt;img&gt;</code> aspect ratio with <code>height: auto</code></li> <li>feat(ui): add <code>viewport-fit=cover</code></li> <li>feat(sanitizer): add support for HTML hidden attribute</li> <li>feat(locale): update French translations</li> <li>feat(integration): add Raindrop integration</li> <li>feat(integration): add feed name to Telegram message</li> <li>feat(integration): add Betula integration</li> <li>feat: use of insecure TLS ciphers when &ldquo;Allow self-signed or invalid certificates&rdquo; is enabled to workaround some broken websites</li> <li>feat: discover feeds from a Youtube playlist pages</li> <li>feat: add navigation to last/first page</li> <li>feat: add global block and keep filters</li> <li>feat: add description field to feed settings</li> <li>feat: add <code>pitchfork.com</code> scraping rule</li> <li>feat: add <code>FETCH_NEBULA_WATCH_TIME</code> config option</li> <li>Bump <code>github.com/PuerkitoBio/goquery</code> from<code> 1.9.1</code> to<code> 1.9.2</code></li> <li>Bump <code>github.com/prometheus/client_golang</code> from <code>1.19.0</code> to <code>1.19.1</code></li> <li>build(deps): bump <code>library/alpine</code> in <code>/packaging/docker/alpine</code></li> <li>build(deps): bump <code>golangci/golangci-lint-action</code> from <code>4</code> to <code>6</code></li> <li>build(deps): bump <code>golang.org/x/term</code> from <code>0.19.0</code> to <code>0.22.0</code></li> <li>build(deps): bump <code>golang.org/x/oauth2</code> from <code>0.19.0</code> to <code>0.21.0</code></li> <li>build(deps): bump <code>golang.org/x/net</code> from <code>0.22.0</code> to <code>0.27.0</code></li> <li>build(deps): bump <code>golang.org/x/crypto</code> from <code>0.24.0</code> to <code>0.25.0</code></li> <li>build(deps): bump <code>github.com/yuin/goldmark</code> from <code>1.7.1</code> to <code>1.7.4</code></li> <li>build(deps): bump <code>github.com/tdewolff/minify/v2</code> from <code>2.20.20</code> to <code>2.20.36</code></li> <li>build(deps): bump <code>github.com/coreos/go-oidc/v3</code> from <code>3.10.0</code> to <code>3.11.0</code></li> <li>build(deps): bump <code>docker/build-push-action</code> from <code>5</code> to <code>6</code></li> </ul> ]]>
</content>
<link rel="enclosure" href="https://github.com/miniflux/v2/releases/download/2.1.4/miniflux-darwin-amd64" type="application/octet-stream"/>
<link rel="enclosure" href="https://github.com/miniflux/v2/releases/download/2.1.4/miniflux-darwin-arm64" type="application/octet-stream"/>
<link rel="enclosure" href="https://github.com/miniflux/v2/releases/download/2.1.4/miniflux-freebsd-amd64" type="application/octet-stream"/>
<link rel="enclosure" href="https://github.com/miniflux/v2/releases/download/2.1.4/miniflux-linux-amd64" type="application/octet-stream"/>
<link rel="enclosure" href="https://github.com/miniflux/v2/releases/download/2.1.4/miniflux-linux-arm64" type="application/octet-stream"/>
<link rel="enclosure" href="https://github.com/miniflux/v2/releases/download/2.1.4/miniflux-linux-armv5" type="application/octet-stream"/>
<link rel="enclosure" href="https://github.com/miniflux/v2/releases/download/2.1.4/miniflux-linux-armv6" type="application/octet-stream"/>
<link rel="enclosure" href="https://github.com/miniflux/v2/releases/download/2.1.4/miniflux-linux-armv7" type="application/octet-stream"/>
<link rel="enclosure" href="https://github.com/miniflux/v2/releases/download/2.1.4/miniflux-openbsd-amd64" type="application/octet-stream"/>
<link rel="enclosure" href="https://github.com/miniflux/v2/releases/download/2.1.4/miniflux-windows-amd64.exe" type="application/octet-stream"/>
<link rel="enclosure" href="https://github.com/miniflux/v2/releases/download/2.1.4/miniflux-2.1.4-1.0.x86_64.rpm" type="application/octet-stream"/>
<link rel="enclosure" href="https://github.com/miniflux/v2/releases/download/2.1.4/miniflux_2.1.4_amd64.deb" type="application/octet-stream"/>
<link rel="enclosure" href="https://github.com/miniflux/v2/releases/download/2.1.4/miniflux_2.1.4_arm64.deb" type="application/octet-stream"/>
<link rel="enclosure" href="https://github.com/miniflux/v2/releases/download/2.1.4/miniflux_2.1.4_armhf.deb" type="application/octet-stream"/>
</entry>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry forgot to push the mentioned fix. It's pushed now though, and the client has been updated with Enclosure and UpdateEnclosure. Both methods have integration tests which skips if no enclosures are found in the test feed.

Two new methods are introduced, Enclosure for fetching and
UpdateEnclosure for updating.

Both methods are integration tested. Tests are skipped if no enclosures
are found in the test feed.
@fguillot
Copy link
Member

fguillot commented Aug 16, 2024

For some reasons, GitHub prevents me to merge this PR because there are conflicts detected. Perhaps, your fork is not up to date?

Conflicting files
internal/api/api_integration_test.go

* main:
  fix(fever): correct sorting direction when using max_id argument
  build: add sha256 checksum file for published binaries
  build: bump Alpine Linux build image to v3.20
  chore: avoid using legacy key/value format in Dockerfile
  build: update GitHub Actions to Go 1.23
  feat: validate OAUTH2_PROVIDER value
  feat: change log level to info when running migrations
  build(deps): bump github.com/prometheus/client_golang
  feat: allow customizing the display name of the OpenID Connect provider
  feat: add license info to js, for LibreJS compatibility
  fix: Honor hide_globally when creating a new feed through the api
  feat: API: Allow filtering entries on globally_hidden
  feat: Add option to disable local auth form
  build(deps): bump golang.org/x/net from 0.27.0 to 0.28.0
  build(deps): bump golang.org/x/oauth2 from 0.21.0 to 0.22.0
  build(deps): bump golang.org/x/crypto from 0.25.0 to 0.26.0
@wolfhechel
Copy link
Contributor Author

Sorted out the merge conflict. I think it's quite common to get these really diffuse conflicts when the codebase between functions are very similar, including prefixed functions. Maybe the integration tests could use some splitting perhaps to avoid these issues.

@fguillot fguillot merged commit 810b351 into miniflux:main Aug 18, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants