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.

Sign up for GitHub

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

[#182] Fixed sumo_store_riak to support date/datetime fields. #183

Merged
merged 3 commits into from
Aug 28, 2015
Merged

[#182] Fixed sumo_store_riak to support date/datetime fields. #183

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c15a1b2
[#182] Fixed sumo_store_riak to support date/datetime fields.
cabol Aug 24, 2015
725f4ee
[#182] Fixed deps in Makefile.
cabol Aug 24, 2015
9f25ef9
[#182] Fixed bug in internal sleep/wakeup functions within sumo_store…
cabol Aug 24, 2015
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
5 changes: 3 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ PROJECT = sumo_db

CONFIG ?= test/test.config

DEPS = lager emysql emongo tirerl epgsql worker_pool riakc uuid
DEPS = lager emysql emongo tirerl epgsql worker_pool riakc uuid iso8601

dep_lager = git https://github.com/basho/lager.git 2.1.1
dep_emysql = git https://github.com/Eonblast/Emysql.git v0.4.1
Expand All @@ -11,7 +11,8 @@ dep_tirerl = git https://github.com/inaka/tirerl 0.1.7
dep_epgsql = git https://github.com/epgsql/epgsql 2.0.0
dep_worker_pool = git https://github.com/inaka/worker_pool.git 1.0.2
dep_riakc = git https://github.com/inaka/riak-erlang-client.git 2.1.1-dialyzed
dep_uuid = git git://github.com/okeuday/uuid.git v1.4.0
dep_uuid = git https://github.com/okeuday/uuid.git v1.4.0
dep_iso8601 = git https://github.com/kivra/erlang_iso8601.git 1.1.2

TEST_DEPS = mixer
dep_mixer = git git://github.com/inaka/mixer.git 0.1.2
Expand Down
19 changes: 10 additions & 9 deletions rebar.config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,16 @@
warn_untyped_record, debug_info
]}.
{deps, [
{lager, ".*", {git, "git://github.com/basho/lager.git", "2.1.1"}},
{emysql, ".*", {git, "git://github.com/Eonblast/Emysql.git", "v0.4.1"}},
{emongo, ".*", {git, "git://github.com/inaka/emongo.git", "v0.2.1"}},
{tirerl, ".*", {git, "git://github.com/inaka/tirerl", "0.1.7"}},
{epgsql, ".*", {git, "git://github.com/epgsql/epgsql", "2.0.0"}},
{worker_pool, ".*", {git, "git://github.com/inaka/worker_pool.git", "1.0.2"}},
{riakc, ".*", {git, "git://github.com/inaka/riak-erlang-client", "2.1.1-dialyzed"}},
{uuid, ".*", {git, "git://github.com/okeuday/uuid.git", "v1.4.0"}},
{mixer, ".*", {git, "git://github.com/inaka/mixer", "0.1.2"}}
{lager, ".*", {git, "https://github.com/basho/lager.git", "2.1.1"}},
{emysql, ".*", {git, "https://github.com/Eonblast/Emysql.git", "v0.4.1"}},
{emongo, ".*", {git, "https://github.com/inaka/emongo.git", "v0.2.1"}},
{tirerl, ".*", {git, "https://github.com/inaka/tirerl", "0.1.7"}},
{epgsql, ".*", {git, "https://github.com/epgsql/epgsql", "2.0.0"}},
{worker_pool, ".*", {git, "https://github.com/inaka/worker_pool.git", "1.0.2"}},
{riakc, ".*", {git, "https://github.com/inaka/riak-erlang-client", "2.1.1-dialyzed"}},
{uuid, ".*", {git, "https://github.com/okeuday/uuid.git", "v1.4.0"}},
{mixer, ".*", {git, "https://github.com/inaka/mixer", "0.1.2"}},
{iso8601, ".*", {git, "https://github.com/kivra/erlang_iso8601.git", "1.1.2"}}
]}.
{xref_warnings, true}.
{xref_checks, [undefined_function_calls, undefined_functions, locals_not_used, deprecated_function_calls, deprecated_functions]}.
Expand Down
73 changes: 70 additions & 3 deletions src/sumo_store_riak.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ init(Opts) ->
) -> sumo_store:result(sumo_internal:doc(), state()).
persist(Doc,
#state{conn = Conn, bucket = Bucket, put_opts = Opts} = State) ->
{Id, NewDoc} = new_doc(Doc, State),
{Id, NewDoc} = new_doc(sleep(Doc), State),
case update_map(Conn, Bucket, Id, doc_to_rmap(NewDoc), Opts) of
{error, Error} ->
{error, Error, State};
Expand Down Expand Up @@ -223,6 +223,7 @@ find_by(DocName, Conditions, Limit, Offset, State) when is_list(Conditions) ->
find_by(DocName, Conditions, Limit, Offset, State) ->
find_by_query(DocName, Conditions, Limit, Offset, State).

%% @private
find_by_id_field(DocName, Key, State) ->
#state{conn = Conn, bucket = Bucket, get_opts = Opts} = State,
case fetch_map(Conn, Bucket, to_bin(Key), Opts) of
Expand All @@ -235,11 +236,13 @@ find_by_id_field(DocName, Key, State) ->
{error, Error, State}
end.

%% @private
find_by_query(DocName, Conditions, undefined, undefined, State) ->
#state{conn = Conn, index = Index} = State,
Query = build_query(Conditions),
find_all_by_query(DocName, Conn, Index, Query, State);

%% @private
find_by_query(DocName, Conditions, Limit, Offset, State) ->
#state{conn = Conn, index = Index} = State,
Query = build_query(Conditions),
Expand All @@ -248,6 +251,7 @@ find_by_query(DocName, Conditions, Limit, Offset, State) ->
{error, Error} -> {error, Error, State}
end.

%% @private
find_all_by_query(DocName, Conn, Index, Query, State) ->
FirstQuery =
case search_docs_by(DocName, Conn, Index, Query, 0, 0) of
Expand Down Expand Up @@ -306,7 +310,7 @@ map_to_rmap(Map) ->
sumo:schema_name(), riakc_map:crdt_map()
) -> sumo_internal:doc().
rmap_to_doc(DocName, RMap) ->
sumo_internal:new_doc(DocName, rmap_to_map(RMap)).
wakeup(sumo_internal:new_doc(DocName, rmap_to_map(RMap))).

-spec rmap_to_map(riakc_map:crdt_map()) -> map().
rmap_to_map(RMap) ->
Expand Down Expand Up @@ -365,6 +369,69 @@ build_query(Conditions) ->
%% Private API.
%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%

%% @private
sleep(Doc) ->
DTFields = datetime_fields(Doc),
Encode =
fun({FieldName, FieldValue}, Acc) ->
case {FieldName, is_datetime(FieldValue)} of
{datetime, true} ->
sumo_internal:set_field(FieldName, iso8601:format(FieldValue), Acc);
{date, true} ->
DateTime = {FieldValue, {0, 0, 0}},
sumo_internal:set_field(FieldName, iso8601:format(DateTime), Acc);
_ ->
Acc
end
end,
lists:foldl(Encode, Doc, DTFields).

%% @private
wakeup(Doc) ->
DTFields = datetime_fields(Doc),
Decode =
fun({FieldName, FieldValue}, Acc) ->
case FieldName of
datetime when FieldValue /= <<"undefined">> ->
sumo_internal:set_field(FieldName, iso8601:parse(FieldValue), Acc);
date when FieldValue /= <<"undefined">> ->
{Date, _} = iso8601:parse(FieldValue),
sumo_internal:set_field(FieldName, Date, Acc);
_ ->
Acc
end
end,
lists:foldl(Decode, Doc, DTFields).

%% @private
datetime_fields(Doc) ->
DocName = sumo_internal:doc_name(Doc),
Schema = sumo_internal:get_schema(DocName),
SchemaFields = sumo_internal:schema_fields(Schema),
Filter =
fun(Field, Acc) ->
case sumo_internal:field_type(Field) of
T when T =:= datetime; T =:= date ->
FieldName = sumo_internal:field_name(Field),
FieldValue = sumo_internal:get_field(FieldName, Doc),
[{FieldName, FieldValue} | Acc];
_ ->
Acc
end
end,
lists:foldl(Filter, [], SchemaFields).

%% @private
is_datetime(DT) ->
Copy link
Member

Choose a reason for hiding this comment

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

This can be solved better as function clauses instead of a case expression.

case DT of
{{_, _, _} = Date, {_, _, _}} ->
calendar:valid_date(Date);
{_, _, _} ->
calendar:valid_date(DT);
_ ->
false
end.

%% @private
doc_id(Doc) ->
DocName = sumo_internal:doc_name(Doc),
Expand Down Expand Up @@ -423,7 +490,7 @@ kv_to_doc(DocName, KV) ->
NK = normalize_doc_fields(K),
sumo_internal:set_field(to_atom(NK), V, Acc)
end,
lists:foldl(F, sumo_internal:new_doc(DocName), KV).
wakeup(lists:foldl(F, sumo_internal:new_doc(DocName), KV)).

%% @private
normalize_doc_fields(Src) ->
Expand Down
22 changes: 21 additions & 1 deletion test/sumo_basic_SUITE.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@
find_all/1,
find_by/1,
delete_all/1,
delete/1
delete/1,
check_datetime_fields/1
]).

-define(EXCLUDED_FUNS,
Expand Down Expand Up @@ -70,6 +71,11 @@ delete_all(_Config) ->
delete(_Config) ->
run_all_stores(fun delete_module/1).

check_datetime_fields(_Config) ->
lists:foreach(
fun do_check_datetime_fields/1,
[sumo_test_people_riak]).
Copy link
Member

Choose a reason for hiding this comment

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

We successfully managed to remove particular stores from the test suites. You should not have sumo_test_people_riak here. If you really need this and if this can only be tested in the riak store, then the list [sumo_test_people_riak] should come from sumo_test_utils:people_with_datetime() or something similar.
But I wonder why would sumo_test_people_riak have a function called datetime and other modules would not.
From my perspective, the list should be sumo_test_utils:all_people() and you should implement the datetime/1 function in all sumo_test_people's implementations.


%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
%%% Internal functions
%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
Expand Down Expand Up @@ -139,6 +145,20 @@ delete_module(Module) ->

1 = length(All) - length(NewAll).

do_check_datetime_fields(Module) ->
[P0] = sumo:find_by(Module, [{name, <<"A">>}]),
P1 = sumo:find(Module, Module:id(P0)),
[P2 | _] = sumo:find_all(Module),

{Date, _} = calendar:universal_time(),

Date = Module:date(P0),
{Date, {_, _, _}} = Module:datetime(P0),
Date = Module:date(P1),
{Date, {_, _, _}} = Module:datetime(P1),
Date = Module:date(P2),
{Date, {_, _, _}} = Module:datetime(P2).

Copy link
Member

Choose a reason for hiding this comment

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

You should also test that setting a value (e.g. Module:datetime(P2, {datetime, …})) works.

Copy link
Member

Choose a reason for hiding this comment

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

You should also test that using find_by with a datetime works.

%%% Helper

-spec run_all_stores(fun()) -> ok.
Expand Down
106 changes: 88 additions & 18 deletions test/sumo_test_people_riak.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,34 @@
-module(sumo_test_people_riak).

-behavior(sumo_doc).

-include_lib("mixer/include/mixer.hrl").
-mixin([{sumo_test_people,
[sumo_wakeup/1,
sumo_sleep/1,
new/2,
new/3,
new/4,
name/1,
id/1,
age/1
]
}
]).

-export([sumo_schema/0]).
%%% sumo_db callbacks
-export([ sumo_schema/0
, sumo_wakeup/1
, sumo_sleep/1
]).

-export([ new/2
, new/3
, new/4
, name/1
, id/1
, age/1
, date/1
, datetime/1
]).

-record(person, {id :: integer(),
name :: string(),
last_name :: string(),
age :: integer(),
address :: string(),
date :: calendar:date(),
Copy link
Member

Choose a reason for hiding this comment

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

Try not to call the fields in such a generic way (even for tests). Call them birthdate and last_login or created_at, if you will.

datetime :: calendar:datetime()}).

-type person() :: #person{}.

%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
%%% sumo_doc callbacks
%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%

-spec sumo_schema() -> sumo:schema().
sumo_schema() ->
Expand All @@ -25,6 +37,64 @@ sumo_schema() ->
sumo:new_field(name, string, [{length, 255}, not_null]),
sumo:new_field(last_name, string, [{length, 255}, not_null]),
sumo:new_field(age, integer),
sumo:new_field(address, string, [{length, 255}])
sumo:new_field(address, string, [{length, 255}]),
sumo:new_field(date, date),
sumo:new_field(datetime, datetime)
],
sumo:new_schema(?MODULE, Fields).

-spec sumo_sleep(person()) -> sumo:doc().
sumo_sleep(Person) ->
#{id => Person#person.id,
name => Person#person.name,
last_name => Person#person.last_name,
age => Person#person.age,
address => Person#person.address,
date => Person#person.date,
datetime => Person#person.datetime}.

-spec sumo_wakeup(sumo:doc()) -> person().
sumo_wakeup(Person) ->
#person{
id = maps:get(id, Person),
name = maps:get(name, Person),
last_name = maps:get(last_name, Person),
age = maps:get(age, Person),
address = maps:get(address, Person),
date = maps:get(date, Person),
datetime = maps:get(datetime, Person)
}.

%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
%%% Exported
%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%

new(Name, LastName) ->
new(Name, LastName, undefined).

new(Name, LastName, Age) ->
new(Name, LastName, Age, undefined).

new(Name, LastName, Age, Address) ->
Datetime = {Date, _} = calendar:universal_time(),
#person{name = Name,
last_name = LastName,
age = Age,
address = Address,
date = Date,
datetime = Datetime}.

name(Person) ->
Person#person.name.

id(Person) ->
Person#person.id.

age(Person) ->
Person#person.age.

date(Person) ->
Person#person.date.

datetime(Person) ->
Person#person.datetime.