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

Fix: Windows drive paths misidentified as URLs #1460

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

toadslop
Copy link
Contributor

@toadslop toadslop commented Jun 29, 2024

Attempts to fix this issue.

This has resulted in some failing tests on Linux. Will revise.

Edit:

Fixed Linux tests. Let me know if you have any suggestions for improvement.

lychee-lib/src/types/input.rs Outdated Show resolved Hide resolved
@mre
Copy link
Member

mre commented Jun 29, 2024

Good job! Just a minor ask before we can merge this.

@toadslop
Copy link
Contributor Author

I made an adjustment to be more explicit. Relying on the behavior of RequestBuilder hides the intent a bit, so instead I explicitly say that I'm rejecting Urls that don't have an http (or https) scheme.

I also adjusted the last part of the function to handle Windows behavior better. The function originally assumed that missing a scheme could have been a mistake and adds https, but on Windows this can never happen because the separators in Windows filepaths are not valid for Urls.

Finally, I added some Windows-only tests to validate that the behavior is correct.

@toadslop
Copy link
Contributor Author

After the changes, though, some tests are failing that don't fail on Windows. I will need to investigate to understand the errors. They're related to something called "wayback", which I have never heard of and so I have no idea what the behavior is supposed to be.

@mre
Copy link
Member

mre commented Jun 29, 2024

Oh, the wayback you're referring to is short for "wayback machine", which we use to suggest archived versions in case of broken links. Unfortunately, their API is pretty flaky and lookups randomly fail, which makes these tests unreliable as well.

I suggest to ignore these particular errors for the moment. Perhaps we can even disable them entirely.

@toadslop
Copy link
Contributor Author

Yup, looks like you're right. I forced a test rerun and it passed.

lychee-lib/src/types/input.rs Outdated Show resolved Hide resolved
lychee-lib/src/types/input.rs Outdated Show resolved Hide resolved
@mre mre force-pushed the issue-1459-fix branch 2 times, most recently from 9a9b45a to 5762b18 Compare August 6, 2024 15:59
@mre
Copy link
Member

mre commented Aug 6, 2024

I cleaned up the PR a bit. I think it's ready to be shipped.
Thanks for your contribution @toadslop. 😊

@mre mre changed the title use RequestBuilder to identify invalid urls Fix: Windows drive paths misidentified as URLs Aug 6, 2024
@mre mre merged commit 8df3b99 into lycheeverse:master Aug 6, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants