Simple fix to handle trailing slashs by parsing the server address.

[lchausmann]

Fixes the trailing slash reported in #389.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
atuin	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 31, 2023 5:44pm

[conradludgate]

Nice, yeah I like the idea of a proper URL type. Looking at the docs though I was a bit confused at what join is supposed to do.

I looked at some work code and it seems we use path_segments_mut instead and extend it. I've put together a demo of what that looks like. join seems to remove all path items before it, not sure what the point of that is

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=6fc1dc05f7abd3fcfb8f79b820f15ae3

[conradludgate]

Another thought, don't want to put too much burden on you, but maybe we can parse the URL in the settings and pass that into these functions, rather than parse it every time. Not a huge concern, but it would be a little neater

[lchausmann]

No, thats quite alright. I dont mind.

So you are thinking that the the functions, should take the URL type as argument, and do the parsing once, when the config parameter is parsed?

[fernandocanizo]

Nice, yeah I like the idea of a proper URL type. Looking at the docs though I was a bit confused at what join is supposed to do.

I looked at some work code and it seems we use path_segments_mut instead and extend it. I've put together a demo of what that looks like. join seems to remove all path items before it, not sure what the point of that is

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=6fc1dc05f7abd3fcfb8f79b820f15ae3

According to url docs .join() is meant to join into a base url, hence the "path" clearing on your example in the playground. However using extend doesn't properly fix the issue with multiple slashes. On your example try: (notice the double slash at the end of the URL)

dbg!(extend("https://example.com//").to_string());

You'll get: (triple slash!)

extend("https://example.com//").to_string() = "https://example.com///bar/baz"

I'm not a Rust programmer and don't know if there may be another library which does proper handling of this issue, however I think the sanest option would be to build up all end points from a base URL and not try to join partial paths.

Anyway, atuin server configuration just receives a port, why would you have the server running on a more complicated path than "http://some-server:some-port/" ?

Sorry if I sound like I don't know what I'm talking about (I don't :P ) , but got bitten by this bug today and just wanted to point out that .path_segments_mut().unwrap().extend() won't solve the issue of users putting an extra slash on their configs or a programmer also making the mistake of adding an extra slash somewhere in a default string.

[fernandocanizo]

I perused all Url functions, played in the Rust playground, followed an old Url package discussion, and asked in forum and came to the conclusion that the best approach is to use Url functions as they were intended, as the idea of joining a base url containing a starting path with second part of the path, is not to be found anywhere in the package.

Also the idea that an url in a string could come from a foreign source, and thus not be correct, is something not to be found in Url.

I thought I could fix the issue as this is low hanging fruit, but as I stated, I'm not a Rust programmer, and couldn't come with a solution (got too many complaints from the compiler).

But I'd like to share the approach I tried, which only failed because my lack of Rust knowledge.

1. read configuration and decide what to use:
   a. sync_address from config.toml or
   b. environment variable (not sure if there's one though)

2. clean address via RegExp: s/\/+$// (remove any and all last slashes)

3. parse it into an Url object and save it in a config object available for other parts of the code, let's call it base_sync_url or base_sync_address

4. create a helper function to join base_sync_address with a path:

fn build_endpoint(path: &str) -> Url {...}

Inside it:
a. clean path: s/\/+/\//g (substitute all multiple slashes by just one)
b. use Url::join() to combine config::base_sync_url with path

Use build_endpoint() to build all the endpoints, not just the one for registering.

I didn't search the whole code base, but from what I get from api_client.rs:

$ grep format api_client.rs
    let url = format!("{address}/user/{username}");
    let url = format!("{address}/register");
    let url = format!("{address}/login");
        headers.insert(AUTHORIZATION, format!("Token {session_token}").parse()?);
        let url = format!("{}/sync/count", self.sync_addr);
        let url = format!("{}/sync/status", self.sync_addr);
            hash_str(&format!(
        let url = format!(
        let url = format!("{}/history", self.sync_addr);
        let url = format!("{}/history", self.sync_addr);
        let url = format!("{}/record", self.sync_addr);
        let url = format!(
                format!(
        let url = format!("{}/record", self.sync_addr);
        let url = format!("{}/account", self.sync_addr);

All constructed end-points are just a base + path, so no need to consider a base/path + 'second_path` for this.

Anyway, sorry for the noise and I hope to have contributed in some way.

[ellie]

Hey! I'm not really too sure on the state of this one right now (it's been a while, sorry!), but would it be possible to do the same for the Client methods too?

[ellie]

I'll merge as-is for now

[ellie]

Thank you!