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 plugin location path decoding from Url #2190

Merged
merged 4 commits into from
Mar 1, 2023

Conversation

tarkah
Copy link
Contributor

@tarkah tarkah commented Feb 25, 2023

Using URL file schema for plugin location is currently broken in a couple different ways:

  • Naively converting URL path to a PathBuf doesn't decode the percent encoded URL. This makes any path with "spaces" or percent encoded characters break.
  • Using path or now w/ to_file_path always returns an "absolute" PathBuf. The root prefix is now stripped when adding it to the config directory for relative lookup.

I've tested both relative lookup file:plugin.wasm as well as absolute path w/ spaces and both now work.

@tarkah tarkah changed the title Decode uri file as path & account for relative Fix plugin location path decoding from Url Feb 25, 2023
@tarkah tarkah temporarily deployed to cachix February 25, 2023 07:52 — with GitHub Actions Inactive
@imsnif
Copy link
Member

imsnif commented Feb 25, 2023

Hey @tarkah - thanks for looking into this!

Could you please add some tests (even if just unit tests for the changed functions) to clarify what's going on here? Barring that (if it's too complex to add a test suite here) maybe add very detailed comments with examples?

@tarkah
Copy link
Contributor Author

tarkah commented Feb 25, 2023

Hey @tarkah - thanks for looking into this!

Could you please add some tests (even if just unit tests for the changed functions) to clarify what's going on here? Barring that (if it's too complex to add a test suite here) maybe add very detailed comments with examples?

No problem! I did a bit more digging and there is definitely an issue w/ the url dep and parsing file schema with relative data. It always adds an absolute forward slash. So when using path or to_file_path, it always returns an absolute path. This caused Path::join to just return the absolute path.

  • I've added a new initial commit dcca532 includes a new unit test that fails.

  • e182597 Instead of trying to extract a valid file path from a Url, it checks if the url is using relative data after the schema and instead uses the raw string to build the path from. Until the url dep is fixed upstream, this is the best way to handle this. This also ensures to decode the percent encoded path instead of using it directly when absolute path is passed or builtin tag is used.

  • 939fd42 updates the unit test to check for paths with spaces. This would have previously failed as well.

@tarkah tarkah temporarily deployed to cachix March 1, 2023 14:19 — with GitHub Actions Inactive
Copy link
Member

@imsnif imsnif left a comment

Choose a reason for hiding this comment

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

Thanks for this, the test looks great!

@imsnif imsnif merged commit 6e6e269 into zellij-org:main Mar 1, 2023
@tarkah tarkah deleted the fix/plugin-location branch March 1, 2023 15:12
naosense pushed a commit to naosense/zellij that referenced this pull request Mar 2, 2023
* Add unit test for plugin run location parsing

* Fix file plugin parsing for relative paths

* Update test to check for path with spaces

* Add a couple more tests
joshheyse pushed a commit to joshheyse/zellij that referenced this pull request Mar 11, 2023
* Add unit test for plugin run location parsing

* Fix file plugin parsing for relative paths

* Update test to check for path with spaces

* Add a couple more tests
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