Help with idiomatic Elixir style when dealing with logic flow

Questions / Help
,
1

Hello,

I am somewhat new to Elixir, and finding that I am having difficulty grasping how I should handle logic for a series of sequences in an “operation”. For example, I have a phoenix post controller that I am using to onboard an organisation and user. So there are a few steps.

  1. Verify where the request came from (I know this is not perfect)
  2. Create an organisation if one does not already exist
  3. Create a user for this organisation if one does not already exist

In this example I am relying on “halt” to essentially return early. But I think I might be approaching the problem in Elixir like an imperative language. But in a regular function where such a mechanism does (no return statement in the language) I get a bit lost on how I should manage control flow. This seems it should be broken up somehow.

I also have ended up with a rather “nested” outcome, which perhaps could (or should?) be avoided with that cool pipeline operator, but I don’t know how I would handle the unhappy path nicely.

I would really appreciate some feedback, and any learning resources that help people like me who have been working in imperative languages so long that we have trouble breaking the habit! Thanks for reading!

def onboard(conn, params) do
    case verify_zendesk_origin(conn) do
      {:ok, _origin} ->
        user_params = params["user"]

        organisation_attrs = %{
          subdomain: params["subdomain"],
          name: params["name"],
          public_key: params["public_key"]
        }

        case Organisations.upsert(organisation_attrs) do
          {:ok, organisation} ->
            user_attrs = %{
              external_id: to_string(user_params["id"]),
              name: user_params["name"],
              role: user_params["role"],
              avatar_url: user_params["avatarUrl"],
              organisation_id: organisation.id
            }

            case ExternalAccounts.upsert_user(user_attrs) do
              {:ok, user} ->
                conn
                |> put_status(:ok)
                |> json(%{user_id: user.id})
                |> halt()

              {:error, changeset} ->
                IO.inspect(changeset.errors, label: "user upsert (onboard) changeset errors")

                conn
                |> put_status(:unprocessable_entity)
                |> json(%{
                  error: "invalid user data",
                  details: changeset_error_to_string(changeset)
                })
                |> halt()
            end

          {:error, changeset} ->
            IO.inspect(changeset.errors, label: "organisation upsert (onboard) changeset errors")

            conn
            |> put_status(:unprocessable_entity)
            |> json(%{
              error: "invalid organisation data",
              details: changeset_error_to_string(changeset)
            })
            |> halt()
        end

      {:error, _reason} ->
        conn
        |> put_status(:forbidden)
        |> json(%{error: "Invalid origin"})
        |> halt()
    end
  end
2

I would definitely go for a with in this case. Take a look at this anti-pattern, and use it as an example. Create functions that return either an :ok-tuple or a specific :error-tuple (or triplet) for that case.

3 Likes
3

The logic could be extracted to an use-case module.
The origin verification could also be a plug.

At a very high and very verbose level :


plug YourAppWeb.Plugs, :verify_zendesk_origin when action in [:onboard, ...]
alias YourAppWeb.UserFlows.OnboardUser

def onboard(conn, params) do
    with {_, {:ok, organisation}} <- {:upsert_organisation, OnboardUser.upsert_organisation(conn.assigns.organisation_attrs)},
            {_, {:ok, user_attrs}} <- {:user_attrs, OnboardUser.user_attrs_from_params_and_org(params, organisation)}
            {_, {:ok, user}} <- {:upsert_external_user, OnboardUser.upsert_external_user(user_attrs)} do
         conn |> put_status(:ok) |> json(%{user_id: user.id})
   else
     {:upsert_organisation, {:error, e}} -> # handle this case
     {:user_attrs, {:error, e}} -> #handle this one
     {:upsert_external_user, {:error, e}} # handle this one
     _ -> # etc
    end
end

I’ve put this fictional OnboardUser module in YourAppWeb because the helper function user_attrs_from_params_and_org depends on the params arg, so it is linked to the transport.
You can of course refine that and have a pure non-web logic OnboardUser use-case while having other extracted utilities to properly construct the arguments it consumes from the request.

There also are a few different ways to tag error tuples or triples with with.
I like to do it at the call site to keep the logic free of this tagging.

The more non-web your logic is, the more testable it becomes :slight_smile:

Edit : use-case based modules are an opinionated choice and not the idiomatic choice.

1 Like
4

I do not like the tagged approach.
I would go for a function that returns a {:ok, organisation} or {:organisation_error, error_information}.
But I do like moving the initial verification to a plug.

2 Likes
5

I think your way is cleaner overall, I don’t like putting the tags in the logic module, but have to admit it makes sense if we go for thin controllers.

6

Why not? It gets the job done.

7

Tags at the call site make the real logic less noisy and aware of callers, and tags in the logic makes the caller leaner but aware of the tags… with use-case based organisation, both solutions shouldn’t really be problems.

In the end it depends of the style of the particular codebase you’re working on, the best solution might be to go with the flow of the rest of the code.

2 Likes
8

I would probably do something like this.

Usually, there are few types of errors that your web layer will handle.

Something like:

  • {:error, :not_found}
  • {:error, changeset}
  • {:error, "Some error message as string"}

If your context functions always return these, you can have your error handling in the fallback controller and then just do those nice with {:ok, something} <- YourContext.some_fun(params) calls. And if there’s some exception to that, you can handle it in the else clause of the with statement.

3 Likes
9

Wow, really great options by everybody, thanks so much!

I wondered about making a plug for verify_zendesk_origin there are a total of two actions in this controller that care about it, so I will definitely do that. Definately seems more testable and “out of the way” of the controller action itself.

These tags are rather interesting, I find this function you have written to be very clear and compact, I had not seen tags before. I’m reading up on with which seems to be the common suggestion. That confuses me a little, mainly because I use that in Python all the time, but it’s very different in Elixir!

10

This is also really cool, and shows some concepts that are also new to me. More reading need at my end I think.

1 Like
11

To my eyes it is too noisy.

1 Like
12

I agree that it’s noisy, it just became my default because very often I had to consume APIs where I couldn’t influence the return values of the functions (hence your {:organisation_error, error_information} was often not possible to have). To me with plus tagging seemed like the better cope.

We had lengthy discussions on the forum in the past and I could see the perceived benefits of making smaller functions that call the 3rd party API and slightly reshape their returns (very often you would want to convert :error to {:error, :cannot_parse_url} for example) but I could not see the objectively demonstrable value of (1) adding extra functions, (2) reshaping the return values of the wrapped API just so we emulate an extra atom in a tuple in a with chain. :person_shrugging: It was just a preference that many people stated that they have but I was still left unconvinced because, again, it was just a preference.

I have to be honest here, the older I get the more I want LESS freedom for anyone in programming to just do stuff as they prefer – myself included, I made quite some impressive messes many times and really should have been slapped back to other techniques.

4 Likes
13

Same here!

14

So I’ve tried a few things as well and this is what I came up with.

First of all, the steps for onboarding a user should probably be combined in a context/core function, not in the web facing layer. I agree with the origin check being done in a plug, so after doing that, this is what the controller function would look like:

def onboard(conn, params) do
  organization_attrs = %{
    ...
  }
  user_attrs = %{
    ...
  }

  case Users.onboard(organization_attrs, user_attrs) do
    {:ok, user} ->
      ...

    {:error, :invalid_organization_data, changeset} ->
      ...

    {:error, :invalid_user_data, changeset} ->
      ...
  end

This would require the Users.onboard function to combine the functions from other modules and tag the errors. If we want to avoid the with and tagging approach, we end up with small single-use functions like this:

defmodule Users do
  def onboard(organization_attrs, user_attrs) do
    with {:ok, organization} <- upsert_organization(organization_attrs) do
      user_attrs
      |> Map.put(:organization_id, organization.id)
      |> upsert_user()
    end
  end

  def upsert_organization(attrs) do
    case Organizations.upsert(attrs) do
      {:ok, organization} -> {:ok, organization}
      {:error, changeset} -> {:error, :invalid_organization_data, changeset}
    end
  end

  def upsert_user(attrs) do
    case ExternalAccounts.upsert_user(attrs) do
      {:ok, user} -> {:ok, user}
      {:error, changeset} -> {:error, :invalid_user_data, changeset}
    end
  end
end

When we extract the parts of these tiny functions that remain the same, we can see that what we actually need is to simply tag the errors. This could lead to the following code:

defmodule Users do
  def onboard(organization_attrs, user_attrs) do
    with {:ok, organization} <-
           Organizations.upsert(organization_attrs) |> tag_error(:invalid_organization_data) do
      user_attrs
      |> Map.put(:organization_id, organization.id)
      |> ExternalAccounts.upsert_user(attrs)
      |> tag_error(:invalid_user_data)
    end
  end

  defp tag_error({:ok, _} = result, _tag), do: result
  defp tag_error({:error, info}, tag), do: {:error, tag, info}
end

My conclusion is that this is reason number fifteen million and forty seven why Elixir should have built-in functions for working with result values. What do others think about this?

1 Like
15

I thought this too at some point. A few years later I felt that pattern matching itself is an highly expressive and flexible tool and that using it is better than more stdlib surface…

1 Like
16

I still think that pattern matching on results is the simplest way, but inevitably I see people making all kinds of utility functions like my tag_error here. If these were a part of the standard library, we would have a unified way to do things, instead of everyone rolling their own (which can be a good enough reason not to do this at all).

1 Like
17

I’m not sure about that. From all the projects I worked on, I’ve seen or used this kind of tag strategy exactly zero times, I can understand why you would want to do that sometimes.

What you achieved is a solution, not the solution, hence why I don’t see the need of additional functionality supporting this in the stdlib. If you find yourself using these in all of your projects, you can always publish a library and use it in those places.

2 Likes
18

I think this is exactly the reason why there should be an official way to do things. The problem is handling multiple steps which may fail and returning information about which of them failed. That situation exists in literally every program beyond hello world.

Solutions have been discussed many times in many threads and people can never agree on what to actually do, which results in many approaches and libraries, even though the problem is conceptually pretty simple.

1 Like
19

Is it though? I think this is a simplistic view and the problem itself is much more complex. Each presented solution has its own drawbacks and they should be chosen accordingly depending on how the system should behave.

1 Like
20

I think the three solutions presented in this thread differ only in syntax. The least magical solution is simply nesting case expressions. That’s clearly ugly and I’m not surprised that most people would prefer something else.

Then with comes to the rescue, so the steps are now clearly in order, but now we don’t know which of them failed. So we end up adding a tag somewhere. Either to the whole step, which adds noise and requires else to convert {step, {:error, info}} into something the caller will understand like {:error, step, info} or {:error, {step, info}}.

Or the tag can only be added to the error, which removes the need for the else, but requires either those tiny single-use functions (like the docs suggest), or a generalization that only adds a tag to the error.

But in the end, it’s still just syntax and I could even be persuaded into going back to nesting case expressions, because at least everybody knows what is going on with them.

2 Likes