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

Replace regex in rewriter #3000

Merged
merged 1 commit into from
Dec 13, 2024
Merged

Replace regex in rewriter #3000

merged 1 commit into from
Dec 13, 2024

Conversation

jvoisin
Copy link
Collaborator

@jvoisin jvoisin commented Dec 11, 2024

No need for brittle regex when matching plain strings or domain names. This should save some negligible amount of heap memory as well as tremendously speeding up the matching.

@fguillot
Copy link
Member

In PR #3009, I added a unit test for GetRefererForURL(). Your refactoring is not backward compatible:

--- FAIL: TestGetRefererForURL (0.00s)
    --- FAIL: TestGetRefererForURL/Pixiv_Image_URL (0.00s)
        referer_override_test.go:62: GetRefererForURL(https://i.pximg.net/img-master/example.jpg): expected https://www.pixiv.net, got
    --- FAIL: TestGetRefererForURL/SSPai_CDN_URL (0.00s)
        referer_override_test.go:62: GetRefererForURL(https://cdnfile.sspai.com/example.png): expected https://sspai.com, got
    --- FAIL: TestGetRefererForURL/Piokok_URL (0.00s)
        referer_override_test.go:62: GetRefererForURL(https://sp1.piokok.com/example.jpg): expected https://sp1.piokok.com, got
    --- FAIL: TestGetRefererForURL/Weibo_Video_URL (0.00s)
        referer_override_test.go:62: GetRefererForURL(https://f.video.weibocdn.com/example.mp4): expected https://weibo.com, got
    --- FAIL: TestGetRefererForURL/HelloGithub_Image_URL (0.00s)
        referer_override_test.go:62: GetRefererForURL(https://img.hellogithub.com/example.png): expected https://hellogithub.com, got

No need for brittle regex when matching plain strings or domain names.
This should save some negligible amount of heap memory as well as
tremendously speeding up the matching.
@jvoisin
Copy link
Collaborator Author

jvoisin commented Dec 13, 2024

You're right, I should have added tests in the first place :/

@fguillot fguillot merged commit 945d436 into miniflux:main Dec 13, 2024
16 checks passed
@fguillot
Copy link
Member

I'm wondering if it's worth to have benchmark unit tests in your refactoring. It's hard to tell how the new implementation compare to the previous one in term of performance.

@jvoisin jvoisin deleted the noreproxy branch December 13, 2024 22:55
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