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

Create sumo_single_entity_handler #4

Merged
merged 1 commit into from
Nov 23, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,12 @@ DEPS = mixer cowboy iso8601 jiffy katana trails swagger sumo_db
SHELL_DEPS = sync
TEST_DEPS = elvis xref_runner shotgun

dep_katana = git https://github.com/inaka/erlang-katana.git 0.2.14
dep_katana = git https://github.com/inaka/erlang-katana.git 07efe94
dep_cowboy = git https://github.com/extend/cowboy.git 1.0.4
dep_iso8601 = git https://github.com/zerotao/erlang_iso8601.git 0d14540
dep_jiffy = git https://github.com/davisp/jiffy.git 0.14.4
dep_mixer = git https://github.com/inaka/mixer.git 0.1.4
dep_sync = git https://github.com/inaka/sync.git 0.1.3
dep_sync = git https://github.com/rustyio/sync.git 9c78e7b
dep_shotgun = git https://github.com/inaka/shotgun.git 0.1.12
dep_sumo_db = git https://github.com/inaka/sumo_db.git 0.3.13
dep_swagger = git https://github.com/inaka/cowboy-swagger e9b1062
Expand Down
52 changes: 27 additions & 25 deletions src/sr_entities_handler.erl
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@
, handle_get/2
, handle_post/2
]).
-export([ announce_req/2
, handle_exception/3
]).

-type options() :: #{ path => string()
, model => module()
Expand Down Expand Up @@ -95,6 +98,30 @@ handle_post(Req, State) ->
_:Exception -> handle_exception(Exception, Req, State)
end.

-spec announce_req(cowboy_req:req(), options()) -> cowboy_req:req().
announce_req(Req, #{verbose := true}) ->
{Method, Req1} = cowboy_req:method(Req),
{Path, Req2} = cowboy_req:path(Req1),
_ = error_logger:info_msg("~s ~s", [Method, Path]),
Req2;
announce_req(Req, _Opts) -> Req.

-spec handle_exception(term(), cowboy_req:req(), state()) ->
{halt, cowboy_req:req(), state()}.
handle_exception(Reason, Req, State) ->
_ =
error_logger:error_msg(
"~p. Stack Trace: ~s", [Reason, ktn_debug:ppst()]),
{ok, Req1} =
try cowboy_req:reply(500, Req)
catch
_:Error ->
Msg = "~p trying to report error through cowboy. Stack Trace: ~s",
error_logger:error_msg(Msg, [Error, ktn_debug:ppst()]),
{ok, Req}
end,
{halt, Req1, State}.

%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
%%% Auxiliary Functions
%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
Expand All @@ -118,33 +145,8 @@ handle_post(Entity, Req1, State) ->
Location = iolist_to_binary([Path, Model:uri_path(PersistedEntity)]),
{{true, Location}, Req2, State}.


-spec announce_req(cowboy_req:req(), options()) -> cowboy_req:req().
announce_req(Req, #{verbose := true}) ->
{Method, Req1} = cowboy_req:method(Req),
{Path, Req2} = cowboy_req:path(Req1),
_ = error_logger:info_msg("~s ~s", [Method, Path]),
Req2;
announce_req(Req, _Opts) -> Req.

-spec atom_to_method(get|put|post|delete) -> binary().
atom_to_method(get) -> <<"GET">>;
atom_to_method(put) -> <<"PUT">>;
atom_to_method(post) -> <<"POST">>;
atom_to_method(delete) -> <<"DELETE">>.

-spec handle_exception(term(), cowboy_req:req(), state()) ->
{halt, cowboy_req:req(), state()}.
handle_exception(Reason, Req, State) ->
_ =
error_logger:error_msg(
"~p. Stack Trace: ~s", [Reason, ktn_debug:ppst()]),
{ok, Req1} =
try cowboy_req:reply(500, Req)
catch
_:Error ->
Msg = "~p trying to report error through cowboy. Stack Trace: ~s",
error_logger:error_msg(Msg, [Error, ktn_debug:ppst()]),
{ok, Req}
end,
{halt, Req1, State}.
121 changes: 121 additions & 0 deletions src/sr_single_entity_handler.erl
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
%%% @doc Base GET|PUT|DELETE /[entity]s/:id implementation
-module(sr_single_entity_handler).

-include_lib("mixer/include/mixer.hrl").
-mixin([{ sr_entities_handler
, [ init/3
, allowed_methods/2
, content_types_provided/2
, announce_req/2
, handle_exception/3
]
}]).

-export([ rest_init/2
, resource_exists/2
, content_types_accepted/2
, handle_get/2
, handle_put/2
, delete_resource/2
]).

-type options() :: #{ path => string()
, model => module()
, verbose => boolean()
}.
-type state() :: #{ opts => options()
, id => binary()
, entity => sumo:user_doc()
}.
-export_type([state/0, options/0]).

%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
%%% Cowboy Callbacks
%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%

-spec rest_init(cowboy_req:req(), options()) ->
{ok, cowboy_req:req(), state()}.
rest_init(Req, Opts) ->
Req1 = announce_req(Req, Opts),
{Id, Req2} = cowboy_req:binding(id, Req1),
{ok, Req2, #{opts => Opts, id => Id}}.

-spec resource_exists(cowboy_req:req(), state()) ->
{boolean(), cowboy_req:req(), state()}.
resource_exists(Req, State) ->
#{opts := #{model := Model}, id := Id} = State,
case sumo:find(Model, Id) of
notfound -> {false, Req, State};
Entity -> {true, Req, State#{entity => Entity}}
end.

%% @todo Use swagger's 'consumes' to auto-generate this if possible
%% @see https://github.com/inaka/sumo_rest/issues/7
-spec content_types_accepted(cowboy_req:req(), state()) ->
{[{{binary(), binary(), '*'}, atom()}], cowboy_req:req(), state()}.
content_types_accepted(Req, State) ->
{[{{<<"application">>, <<"json">>, '*'}, handle_put}], Req, State}.

-spec handle_get(cowboy_req:req(), state()) ->
{iodata(), cowboy_req:req(), state()}.
handle_get(Req, State) ->
#{opts := #{model := Model}, entity := Entity} = State,
Copy link
Member

Choose a reason for hiding this comment

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

Although it's true that at this point not much can go wrong, the other handle functions are wrapped in a try catch for unforseen exceptions and to return a proper 500 error, should this one do the same or is it overengineering?

Copy link
Member Author

Choose a reason for hiding this comment

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

Our experience dictates that it's impossible to get this function to throw an exception. The main difference is the lack of access to databases or repositories here.

ResBody = sr_json:encode(Model:to_json(Entity)),
{ResBody, Req, State}.

-spec handle_put(cowboy_req:req(), state()) ->
{{true, binary()} | false | halt, cowboy_req:req(), state()}.
handle_put(Req, #{entity := Entity} = State) ->
#{opts := #{model := Model}} = State,
try
{ok, Body, Req1} = cowboy_req:body(Req),
Json = sr_json:decode(Body),
handle_put(Model:update(Entity, Json), Req1, State)
catch
_:badjson ->
Req3 = cowboy_req:set_resp_body(<<"Malformed JSON request">>, Req),
{false, Req3, State};
_:Exception -> handle_exception(Exception, Req, State)
end;
handle_put(Req, #{id := Id} = State) ->
#{opts := #{model := Model}} = State,
try
{ok, Body, Req1} = cowboy_req:body(Req),
Json = sr_json:decode(Body),
handle_put(from_json(Model, Id, Json), Req1, State)
catch
_:badjson ->
Req3 = cowboy_req:set_resp_body(<<"Malformed JSON request">>, Req),
{false, Req3, State};
_:Exception -> handle_exception(Exception, Req, State)
end.

-spec delete_resource(cowboy_req:req(), state()) ->
{boolean() | halt, cowboy_req:req(), state()}.
delete_resource(Req, State) ->
#{opts := #{model := Model}, id := Id} = State,
try
Result = sumo:delete(Model, Id),
{Result, Req, State}
catch
_:Exception -> handle_exception(Exception, Req, State)
end.

%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
%%% Auxiliary Functions
%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
from_json(Model, Id, Json) ->
case erlang:function_exported(Model, from_json, 2) of
Copy link
Member

Choose a reason for hiding this comment

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

The "problem" with incredibly useful and generic code that does things automagically is that it is often hard to debug when things go wrong. E.g. if the user programmer fails to export the proper functions, all they will get is a stacktrace. If exporting a certain function is a requirement and a source of a possibly foreseeable error, should it not be handled?

Copy link
Member

Choose a reason for hiding this comment

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

Specifically, should it not check it from_json/1 is exported and if not, emit a readable error?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's checked at compile time, since it's a callback in the sumo_rest_doc behaviour

true -> Model:from_json(Id, Json);
false -> Model:from_json(Json)
end.

handle_put({error, Reason}, Req, State) ->
Req1 = cowboy_req:set_resp_body(Reason, Req),
{false, Req1, State};
handle_put({ok, Entity}, Req1, State) ->
#{opts := #{model := Model}} = State,
PersistedEntity = sumo:persist(Model, Entity),
ResBody = sr_json:encode(Model:to_json(PersistedEntity)),
Req2 = cowboy_req:set_resp_body(ResBody, Req1),
{true, Req2, State}.
3 changes: 3 additions & 0 deletions src/sumo_rest_doc.erl
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@

-callback to_json(entity()) -> json().
-callback from_json(json()) -> {ok, entity()} | {error, reason()}.
-callback update(entity(), json()) -> {ok, entity()} | {error, reason()}.
-callback uri_path(entity()) -> iodata().
%% @todo optional callback (it's only needed if dups should raise 409 conflict)
-callback id(entity()) -> term().
%% @todo optional callback (it's only needed if ids are not coming in PUT jsons)
-callback from_json(binary(), json()) -> {ok, entity()} | {error, reason()}.
1 change: 1 addition & 0 deletions test/cover.spec
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
{
incl_mods,
[ sr_entities_handler
, sr_single_entity_handler
, sr_json
, sumo_rest_doc
]
Expand Down
85 changes: 82 additions & 3 deletions test/sr_elements_SUITE.erl
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
, duplicated_key/1
, invalid_headers/1
, invalid_parameters/1
, not_found/1
]).

-spec all() -> [atom()].
Expand Down Expand Up @@ -58,6 +59,61 @@ success_scenario(_Config) ->
sr_test_utils:api_call(get, "/elements"),
[Element1] = sr_json:decode(Body2),

ct:comment("And we can fetch it"),
#{status_code := 200, body := Body21} =
sr_test_utils:api_call(get, "/elements/element1"),
Element1 = sr_json:decode(Body21),

ct:comment("The element value can be changed"),
#{status_code := 200, body := Body3} =
sr_test_utils:api_call(
put, "/elements/element1", Headers,
#{ value => <<"newval1">>
}),
#{ <<"key">> := <<"element1">>
, <<"created_at">> := CreatedAt
, <<"updated_at">> := UpdatedAt
} = Element3 = sr_json:decode(Body3),
true = UpdatedAt >= CreatedAt,

ct:comment("Still just one element"),
#{status_code := 200, body := Body4} =
sr_test_utils:api_call(get, "/elements"),
[Element3] = sr_json:decode(Body4),

ct:comment("Elements can be created by PUT"),
#{status_code := 201, body := Body5} =
sr_test_utils:api_call(
put, "/elements/element2", Headers,
#{value => <<"val2">>}),
#{ <<"key">> := <<"element2">>
, <<"created_at">> := CreatedAt5
, <<"updated_at">> := CreatedAt5
} = Element5 = sr_json:decode(Body5),
true = CreatedAt5 >= CreatedAt,

ct:comment("There are two elements now"),
#{status_code := 200, body := Body6} =
sr_test_utils:api_call(get, "/elements"),
[Element5] = sr_json:decode(Body6) -- [Element3],

ct:comment("Element1 is deleted"),
#{status_code := 204} = sr_test_utils:api_call(delete, "/elements/element1"),

ct:comment("One element again"),
#{status_code := 200, body := Body7} =
sr_test_utils:api_call(get, "/elements"),
[Element5] = sr_json:decode(Body7),

ct:comment("DELETE is not idempotent"),
#{status_code := 204} = sr_test_utils:api_call(delete, "/elements/element2"),
#{status_code := 404} = sr_test_utils:api_call(delete, "/elements/element2"),

ct:comment("There are no elements"),
#{status_code := 200, body := Body8} =
sr_test_utils:api_call(get, "/elements"),
[] = sr_json:decode(Body8),

{comment, ""}.

-spec duplicated_key(sr_test_utils:config()) -> {comment, string()}.
Expand Down Expand Up @@ -86,19 +142,27 @@ invalid_headers(_Config) ->
, <<"accept">> => <<"text/html">>
},

ct:comment("content-type must be provided for POST"),
ct:comment("content-type must be provided for POST and PUT"),
#{status_code := 415} =
sr_test_utils:api_call(post, "/elements", NoHeaders, <<>>),
#{status_code := 415} =
sr_test_utils:api_call(put, "/elements/noheaders", NoHeaders, <<>>),

ct:comment("content-type must be JSON for POST"),
ct:comment("content-type must be JSON for POST and PUT"),
#{status_code := 415} =
sr_test_utils:api_call(post, "/elements", InvalidHeaders, <<>>),
#{status_code := 415} =
sr_test_utils:api_call(put, "/elements/badtype", InvalidHeaders, <<>>),

ct:comment("Agent must accept json for POST, GET"),
ct:comment("Agent must accept json for POST, GET and PUT"),
#{status_code := 406} =
sr_test_utils:api_call(post, "/elements", InvalidAccept, <<>>),
#{status_code := 406} =
sr_test_utils:api_call(get, "/elements", InvalidAccept, <<>>),
#{status_code := 406} =
sr_test_utils:api_call(put, "/elements/badaccept", InvalidAccept, <<>>),
#{status_code := 406} =
sr_test_utils:api_call(get, "/elements/badaccept", InvalidAccept, <<>>),

{comment, ""}.

Expand All @@ -109,16 +173,31 @@ invalid_parameters(_Config) ->
ct:comment("Empty or broken parameters are reported"),
#{status_code := 400} =
sr_test_utils:api_call(post, "/elements", Headers, <<>>),
#{status_code := 400} =
sr_test_utils:api_call(put, "/elements/nobody", Headers, <<>>),
#{status_code := 400} =
sr_test_utils:api_call(post, "/elements", Headers, <<"{">>),
#{status_code := 400} =
sr_test_utils:api_call(put, "/elements/broken", Headers, <<"{">>),

ct:comment("Missing parameters are reported"),
None = #{},
#{status_code := 400} =
sr_test_utils:api_call(post, "/elements", Headers, None),
#{status_code := 400} =
sr_test_utils:api_call(put, "/elements/none", Headers, None),

NoVal = #{key => <<"noval">>},
#{status_code := 400} =
sr_test_utils:api_call(post, "/elements", Headers, NoVal),
#{status_code := 400} =
sr_test_utils:api_call(put, "/elements/noval", Headers, NoVal),

{comment, ""}.

-spec not_found(sr_test_utils:config()) -> {comment, string()}.
not_found(_Config) ->
ct:comment("Not existing element is not found"),
#{status_code := 404} = sr_test_utils:api_call(get, "/elements/notfound"),
#{status_code := 404} = sr_test_utils:api_call(delete, "/elements/notfound"),
{comment, ""}.
16 changes: 16 additions & 0 deletions test/sr_test/sr_elements.erl
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
, from_json/1
, uri_path/1
, id/1
, from_json/2
, update/2
]).

%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
Expand Down Expand Up @@ -65,6 +67,10 @@ to_json(Element) ->
, updated_at => sr_json:encode_date(maps:get(updated_at, Element))
}.

-spec from_json(key(), sumo_rest_doc:json()) ->
{ok, element()} | {error, iodata()}.
from_json(Key, Json) -> from_json(Json#{<<"key">> => Key}).

-spec from_json(sumo_rest_doc:json()) -> {ok, element()} | {error, iodata()}.
from_json(Json) ->
Now = sr_json:encode_date(calendar:universal_time()),
Expand All @@ -81,6 +87,16 @@ from_json(Json) ->
{error, <<"missing field: ", Key/binary>>}
end.

-spec update(element(), sumo_rest_doc:json()) ->
{ok, element()} | {error, iodata()}.
update(Element, Json) ->
case from_json(id(Element), Json) of
{error, Reason} -> {error, Reason};
{ok, Updates} ->
UpdatedElement = maps:merge(Element, Updates),
{ok, UpdatedElement#{updated_at => calendar:universal_time()}}
end.

-spec uri_path(element()) -> binary().
uri_path(Element) -> key(Element).

Expand Down
Loading