refactor(rust): display more detailed error messages to the user when create_outlet fails

[DontPanicO]

Current behavior

Currently, when NodeManager.create_outlet returns an error, tcp_outlet_create_impl (the caller) does not parse it and returns a generic error message instead and thus does not give the user enough information about what has caused the failure. Also, the error message use the term outlet which is considered to not be consistent with the UI, that instead refers to it as service.

Proposed changes

Issue #5850

Including the source error message from NodeManager.create_outlet in tcp_outlet_create_impl and replacing the term outlet with service in those error messages.

Checks

• All commits in this Pull Request are signed and Verified by Github.
• All commits in this Pull Request follow the Ockam commit message convention.
• I accept the Ockam Community Code of Conduct.
• I have accepted the Ockam Contributor License Agreement by adding my Git/Github details in a row at the end of the CONTRIBUTORS.csv file in a separate pull request to the build-trust/ockam-contributors repository. The easiest way to do this is to edit the CONTRIBUTORS.csv file in the github web UI and create a PR, this will mark the commit as verified.

[davide-baldo]

Thanks for contributing again! We really appreciate it!

Inside the Ockam App we chose to use the term services rather than outlet since it easier to understand for the user and better reflect the definition the user is giving. On the other side, the ockam_api is lower level and reasons with building blocks like outlet and doesn't share the concept of service with the ockam app.

If we were to change the ockam_api error it would also reflect in ockam command, for example, if I create the same outlet twice it currently gives this error:

$ ockam tcp-outlet create --to 127.0.0.1:1080 --from my_outlet_address
       Creating TCP Outlet to 127.0.0.1:1080...

OCK500

  × An error occurred while processing the request. Status code: 400 BadRequest. Message: Failed to create outlet: operation failed for address 0#my_outlet_address (origin: Node, kind:
  │ AlreadyExists, source location: implementations/rust/ockam/ockam_node/src/error.rs:36:9)
  help: Please report this issue, with a copy of your logs, to https://github.com/build-trust/ockam/issues

  Version 0.90.0, hash: 8a77aa9dec024b05d9da00f4e75acd188ff6e785

Have you tried keeping the original error in ockam_api and see how they render in the app?

[DontPanicO]

Thanks @davide-baldo. Currently, the app shows a generic error message: "failed to create outlet". This is because the tcp_outlet_create_impl matches the result of NodeManager.create_outlet and if it's an error, it just returns a new one with the above message.
To preserve the internal terminology we could keep the term outlet in the api module, and only switch to the term service in tcp_outlet_create_impl.
However, since I wanted to avoid matching the error kind, I simply included the source error and if I don't edit this error message, the app would end up displaying something like failed to create service: failed to create outlet: ....
So, the best solution that comes to my mind is to keep the original error message from ockam_api (thus nothing but the app would be affected) and use String::replace, or better String::replacen if we want to change only the first occurence:

// ockam/implementations/rust/ockam/ockam_app/src/shared_service/tcp_outlet/create.rs
match node_manager
        .create_outlet(
        ...
        .await
    {
        Ok(status) => {
            info!(socket_addr = socket_addr.to_string(), "Outlet created");
            app_state.model_mut(|m| m.add_tcp_outlet(status)).await?;
            system_tray_on_update(&app);
            Ok(())
        }
        Err(e) => Err(Error::App(format!("{}", e).replacen("outlet", "service", 1))),
    }?;

Sounds reasonable?

[adrianbenavides]

To complement @davide-baldo's review, here are some comments on how to explicitly handle the errors we want to bubble up to the app user.

[adrianbenavides]

The service concept is only relevant in the ockam_app context. In the ockam_api, an outlet is still an outlet, so we don't want to use the term service here.

[adrianbenavides]

Note that the error message here can be literally anything, probably something very verbose and not user-friendly, so we shouldn't rely on it to show an error message to the user.

I'd revert this in favor of handling errors explicitly, as I mentioned in the previous comment.

[adrianbenavides]

As you can see here the alias is None so, we are currently allowing the user to create different outlets with the same worker_addr and/or same socket_addr. Those are the two cases we want to handle explicitly, similarly to what we currently do checking for duplicated alias.

As for the error messages, instead of using the `"A TCP outlet with name {name} already exists", perhaps you could write something like:

warn!("A TCP outlet with name {name} already exists");
let message = format!("name {name} already exists");
return Err(ockam_core::Error::new(...));

so that, in the ockam_app you can add the prefix Failed to create service: {e}.