1
\$\begingroup\$

In my cowboy (Erlang) application, I have a handler called projects_from_json. This takes a JSON POST request as input and should create a database entry from this. Then, it should respond with a success message or the correct error message to the user.

I started with two exceptions that could occur (simplified):

projects_from_json(Req=#{method := <<"POST">>}, State) ->
    {ok, ReqBody, Req2} = cowboy_req:read_body(Req),
    try
        Name = parse_json(ReqBody),
        % Continuation of happy path
        db:create_project(Name)
    catch
        error:{badkey, Key} ->
            % return error message about invalid JSON
            % [<<"Key \"">>, Key, <<"\" does not exist in passed data">>]
        throw:duplicate ->
            % return error message about duplicate name
            % [<<"A project with name \"">>, Name, <<"\" already exists">>]
    end

Already in this case, this did not work, because Name is not safe in the second catch clause (since it comes from the try clause and could potentially be unset). I also could not move the call of db:create_project(Name) outside of the try, because then again Name would not be safe to be used.

Since there were some more exceptions and errors to be handled, I ended up with a very deeply nested situation (follows below).

My first idea would be to move stuff into own functions, but most of the code is error handling and as far as I know I can only handle the error by returning a tuple from the projects_from_json handler.

So here follows my nested mess. In total, I currently want to handle three error situations:

  • InvalidValue: The project name sent by the user contains invalid characters
  • DuplicateValue: A project with the same name already exists
  • MissingKey: User did not supply all required keys

Code:

projects_from_json(Req=#{method := <<"POST">>}, State) ->
    {ok, ReqBody, Req2} = cowboy_req:read_body(Req),
    try
        Name = project_name(ReqBody),

        case validate_project_name(Name) of
            invalid ->
                molehill_respond:respond_error(<<"InvalidValue">>,
                    <<"The project name can only consist of ASCII characters and numbers, and dash in the middle of the word.">>,
                    400, Req2, State);
            ok ->
                {ok, Conn} = moledb:connect_from_config(),

                try
                    moledb:create_project(Conn, Name),
                    Data = prepare_project_json(Name),
                    molehill_respond:respond_status(Data, 201, Req2, State)
                catch
                    throw:duplicate_element ->
                        molehill_respond:respond_error(<<"DuplicateValue">>,
                            erlang:iolist_to_binary(
                                [<<"A project with name \"">>, Name, <<"\" already exists">>]),
                            409, Req2, State)
                end
        end
    catch
        error:{badkey, Key} ->
            molehill_respond:respond_error(
                <<"MissingKey">>,
                erlang:iolist_to_binary(
                    [<<"Key \"">>, Key, <<"\" does not exist in passed data">>]),
                400, Req2, State)
    end.

Is it possible to restructure this to a clean form where all possible errors are on a similar indentation level at the end of the function?

molehill_respond is a helper module I wrote to simplify the creation of my JSON return messages, moledb is a helper module that executes all the SQL queries.

\$\endgroup\$
1
\$\begingroup\$

My first attempt (without changing any of your functions nor the semantics of your handler) would be:

projects_from_json(Req=#{method := <<"POST">>}, State) ->
    try
        {ok, ReqBody, Req2} = cowboy_req:read_body(Req),
        Name = get_project_name(ReqBody),
        {ok, Conn} = moledb:connect_from_config(),
        Data = get_project_json(Conn, Name),
        molehill_respond:respond_status(Data, 201, Req2, State)
    catch
        error:{badkey, Key} ->
            molehill_respond:respond_error(
                <<"MissingKey">>,
                erlang:iolist_to_binary(
                    [<<"Key \"">>, Key, <<"\" does not exist in passed data">>]),
                400, Req2, State);
        error:{error, Label, Message, Status} ->
            molehill_respond:respond_error(Label, Message, Status, Req2, State)
    end.

get_project_name(ReqBody) ->
    Name = project_name(ReqBody),
    case validate_project_name(Name) of
        invalid ->
            erlang:error({
                error,
                <<"InvalidValue">>,
                <<"The project name can only consist of ASCII characters and numbers, "
                    "and dash in the middle of the word.">>,
                400
            });
        ok ->
            Name
    end.

get_project_json(Conn, Name) ->
    try
        moledb:create_project(Conn, Name),
        prepare_project_json(Name)
    catch
        error:duplicate_element ->
            erlang:error({
                error,
                <<"DuplicateValue">>,
                erlang:iolist_to_binary(
                    [<<"A project with name \"">>, Name, <<"\" already exists">>]),
                409
            })
    end.

I basically moved pieces of your code from projects_to_json/2 into its own functions so you can keep your main one tidier.

But you might have noticed that my new functions still have a lot of code for encapsulating the errors. If you can get validate_project_name/1 and prepare_project_json/1 to raise the errors in the proper format themselves instead of returning invalid | ok, this gets reduced to…

projects_from_json(Req=#{method := <<"POST">>}, State) ->
    try
        {ok, ReqBody, Req2} = cowboy_req:read_body(Req),
        Name = project_name(ReqBody),
        validate_project_name(Name),
        {ok, Conn} = moledb:connect_from_config(),
        moledb:create_project(Conn, Name),
        Data = prepare_project_json(Name)
        molehill_respond:respond_status(Data, 201, Req2, State)
    catch
        error:{badkey, Key} ->
            molehill_respond:respond_error(
                <<"MissingKey">>,
                erlang:iolist_to_binary(
                    [<<"Key \"">>, Key, <<"\" does not exist in passed data">>]),
                400, Req2, State);
        error:{error, Label, Message, Status} ->
            molehill_respond:respond_error(Label, Message, Status, Req2, State)
    end.

%% @doc this function may raise an error like {error, binary(), binary(), 400..499}
validate_project_name(Name) ->
    …

%% @doc this function may raise an error like {error, binary(), binary(), 400..499}
prepare_project_json(Name) ->
    …

Basically, the idea is to include as much information as you need on the error itself, so… instead of failing with duplicate_element, you fail with {error, …, …} so that the outer loop can catch that. If you can’t decide on how your functions will fail, just wrap them up in other functions that will translate the errors/results into the expected form.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.