feat(rewrite): move referer mapping to json file #3931
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for loading referer override mappings from an external JSON file, allowing users to extend the built-in referer mappings without rebuilding the project. The hardcoded mappings are converted to a map-based structure and external mappings can override defaults.
Key changes:
- Refactored hardcoded switch statements to a map-based lookup structure
- Added
LoadRefererOverrides()function to load mappings from external JSON files - Added
REFERER_OVERRIDE_FILEconfiguration option with corresponding tests and documentation
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
internal/reader/rewrite/referer_override.go |
Refactored referer override logic from switch statements to map-based lookups, added function to load external JSON mappings |
internal/config/options.go |
Added REFERER_OVERRIDE_FILE configuration option and getter method |
internal/config/options_parsing_test.go |
Added test case for parsing the new configuration option |
internal/cli/cli.go |
Integrated referer override file loading during application startup |
miniflux.1 |
Added documentation for the new REFERER_OVERRIDE_FILE configuration option |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if filePath := config.Opts.RefererOverrideFile(); filePath != "" { | ||
| if err := rewrite.LoadRefererOverrides(filePath); err != nil { | ||
| printErrorAndExit(fmt.Errorf("unable to load referer overrides: %v", err)) | ||
| } |
There was a problem hiding this comment.
The code successfully loads the referer override file but doesn't log this operation, unlike similar initialization steps (e.g., proxy rotation on line 246). Consider adding a log statement to confirm successful loading of the file, which would be helpful for debugging and operational visibility.
| } | |
| } | |
| slog.Info("Loaded referer overrides", slog.String("file", filePath)) |
| for suffix, referer := range refererMappings { | ||
| if strings.HasSuffix(hostname, suffix) { | ||
| return referer | ||
| } | ||
| } |
There was a problem hiding this comment.
The refactored logic has a subtle bug. The function now checks all keys in the map using strings.HasSuffix, including exact hostname keys that don't start with a dot. This means an exact hostname match like "appinn.com" would also match "someprefix-appinn.com" when it shouldn't.
The original code had two separate switch statements: one for exact matches and one for suffix matches. The new code treats all keys uniformly as potential suffixes. Keys like "appinn.com" should only match the exact hostname "appinn.com", not hostnames ending with "appinn.com" like "fake-appinn.com".
Consider either:
- Separating exact matches from suffix matches in the map structure, or
- Checking if the suffix key starts with a dot before doing suffix matching, or
- For non-dot-prefixed keys, ensure the suffix match also checks for a preceding dot in the hostname
There was a problem hiding this comment.
The AI bot is not wrong in this case. I added few more test cases in PR #3942 to cover this scenario.
Running the additional tests with your PR is now failing:
--- FAIL: TestGetRefererForURL (0.00s)
--- FAIL: TestGetRefererForURL/Only_hostnames_starting_with_a_dot_should_match_as_suffix (0.00s)
/home/fred/repos/miniflux/v2/internal/reader/rewrite/referer_override_test.go:77: GetRefererForURL(https://fake-appinn.com/some/path): expected , got https://appinn.com
There was a problem hiding this comment.
I added handling dot prefixes hostnames to be treated as prefixes, this should fix the issue
mtsz-pl
force-pushed
the
referer-json
branch
2 times, most recently
from
9f76acf to
463126b
Compare
|
What's the usecase here? Is adding referers really a common operation? Also, why an external json file? Everything is either stored in the database, or in the configuration file. |
@jvoisin I needed to add this for some of the feeds I subscribe, and after fifth time I thought it would be better to have it configurable somehow instead of creating PRs/rebuilding project. Also IMO it's a code smell to have it hardcoded.
@jvoisin I agree, this part I didn't think thru. Will try to find better solution. |
I don't think it's a code smell, no. A handful of things are hardcoded, like rewriting rules (but more can be added via the web interface, true) and the content sanitizer. As stated on the website, "Miniflux is a minimalist and opinionated feed reader." Having everything configurable goes against this. Also, to be honest, having to override the referer is a bug that should be fixed by the feeds owners. Overriding it is a gross hack :/ But I guess that having a per-feed setting in the webui to specify a referer is acceptable. |
I tried this approach, but the issue is it's not always true one referer applies to single feed. Feed items can contain media from multiple servers. I simplified my solution to just passing comma separated doman=referer pairs. What do you think? |
mtsz-pl
force-pushed
the
referer-json
branch
2 times, most recently
from
4384b27 to
3cf5c67
Compare
|
Something like a bunch of key→value inputs? |
3 participants
Currently, referer overrides are hardcoded, and there's no easy way to extend them without rebuilding the project. This PR allows using JSON file with custom mappings. Hardcoded mappings are preserved. A config value is added to pass JSON file path.