From 444972968f3f1f7e5980dc3dcb3b817690cc3013 Mon Sep 17 00:00:00 2001
From: Feng Lee <feng@emqx.io>
Date: Mon, 12 Aug 2019 11:57:07 +0800
Subject: [PATCH] Improve emqx_mqtt_props module and add test cases

---
 src/emqx_mqtt_props.erl        | 103 ++++++++++++++++++++-------------
 test/emqx_mqtt_props_SUITE.erl |  54 ++++++++++++++---
 2 files changed, 110 insertions(+), 47 deletions(-)

diff --git a/src/emqx_mqtt_props.erl b/src/emqx_mqtt_props.erl
index 47a368714..1b75425a7 100644
--- a/src/emqx_mqtt_props.erl
+++ b/src/emqx_mqtt_props.erl
@@ -25,6 +25,12 @@
         , validate/1
         ]).
 
+%% For tests
+-export([all/0]).
+
+-type(prop_name() :: atom()).
+-type(prop_id() :: pos_integer()).
+
 -define(PROPS_TABLE,
         #{16#01 => {'Payload-Format-Indicator', 'Byte', [?PUBLISH]},
           16#02 => {'Message-Expiry-Interval', 'Four-Byte-Integer', [?PUBLISH]},
@@ -52,36 +58,10 @@
           16#27 => {'Maximum-Packet-Size', 'Four-Byte-Integer', [?CONNECT, ?CONNACK]},
           16#28 => {'Wildcard-Subscription-Available', 'Byte', [?CONNACK]},
           16#29 => {'Subscription-Identifier-Available', 'Byte', [?CONNACK]},
-          16#2A => {'Shared-Subscription-Available', 'Byte', [?CONNACK]}}).
-
-name(16#01) -> 'Payload-Format-Indicator';
-name(16#02) -> 'Message-Expiry-Interval';
-name(16#03) -> 'Content-Type';
-name(16#08) -> 'Response-Topic';
-name(16#09) -> 'Correlation-Data';
-name(16#0B) -> 'Subscription-Identifier';
-name(16#11) -> 'Session-Expiry-Interval';
-name(16#12) -> 'Assigned-Client-Identifier';
-name(16#13) -> 'Server-Keep-Alive';
-name(16#15) -> 'Authentication-Method';
-name(16#16) -> 'Authentication-Data';
-name(16#17) -> 'Request-Problem-Information';
-name(16#18) -> 'Will-Delay-Interval';
-name(16#19) -> 'Request-Response-Information';
-name(16#1A) -> 'Response-Information';
-name(16#1C) -> 'Server-Reference';
-name(16#1F) -> 'Reason-String';
-name(16#21) -> 'Receive-Maximum';
-name(16#22) -> 'Topic-Alias-Maximum';
-name(16#23) -> 'Topic-Alias';
-name(16#24) -> 'Maximum-QoS';
-name(16#25) -> 'Retain-Available';
-name(16#26) -> 'User-Property';
-name(16#27) -> 'Maximum-Packet-Size';
-name(16#28) -> 'Wildcard-Subscription-Available';
-name(16#29) -> 'Subscription-Identifier-Available';
-name(16#2A) -> 'Shared-Subscription-Available'.
+          16#2A => {'Shared-Subscription-Available', 'Byte', [?CONNACK]}
+         }).
 
+-spec(id(prop_name()) -> prop_id()).
 id('Payload-Format-Indicator')          -> 16#01;
 id('Message-Expiry-Interval')           -> 16#02;
 id('Content-Type')                      -> 16#03;
@@ -108,12 +88,47 @@ id('User-Property')                     -> 16#26;
 id('Maximum-Packet-Size')               -> 16#27;
 id('Wildcard-Subscription-Available')   -> 16#28;
 id('Subscription-Identifier-Available') -> 16#29;
-id('Shared-Subscription-Available')     -> 16#2A.
+id('Shared-Subscription-Available')     -> 16#2A;
+id(Name)                                -> error({bad_property, Name}).
 
+-spec(name(prop_id()) -> prop_name()).
+name(16#01) -> 'Payload-Format-Indicator';
+name(16#02) -> 'Message-Expiry-Interval';
+name(16#03) -> 'Content-Type';
+name(16#08) -> 'Response-Topic';
+name(16#09) -> 'Correlation-Data';
+name(16#0B) -> 'Subscription-Identifier';
+name(16#11) -> 'Session-Expiry-Interval';
+name(16#12) -> 'Assigned-Client-Identifier';
+name(16#13) -> 'Server-Keep-Alive';
+name(16#15) -> 'Authentication-Method';
+name(16#16) -> 'Authentication-Data';
+name(16#17) -> 'Request-Problem-Information';
+name(16#18) -> 'Will-Delay-Interval';
+name(16#19) -> 'Request-Response-Information';
+name(16#1A) -> 'Response-Information';
+name(16#1C) -> 'Server-Reference';
+name(16#1F) -> 'Reason-String';
+name(16#21) -> 'Receive-Maximum';
+name(16#22) -> 'Topic-Alias-Maximum';
+name(16#23) -> 'Topic-Alias';
+name(16#24) -> 'Maximum-QoS';
+name(16#25) -> 'Retain-Available';
+name(16#26) -> 'User-Property';
+name(16#27) -> 'Maximum-Packet-Size';
+name(16#28) -> 'Wildcard-Subscription-Available';
+name(16#29) -> 'Subscription-Identifier-Available';
+name(16#2A) -> 'Shared-Subscription-Available';
+name(Id)    -> error({unsupported_property, Id}).
+
+-spec(filter(emqx_types:packet_type(), emqx_types:properties()|list())
+      -> emqx_types:properties()).
 filter(PacketType, Props) when is_map(Props) ->
     maps:from_list(filter(PacketType, maps:to_list(Props)));
 
-filter(PacketType, Props) when ?CONNECT =< PacketType, PacketType =< ?AUTH, is_list(Props) ->
+filter(PacketType, Props) when ?CONNECT =< PacketType,
+                               PacketType =< ?AUTH,
+                               is_list(Props) ->
     Filter = fun(Name) ->
                  case maps:find(id(Name), ?PROPS_TABLE) of
                      {ok, {Name, _Type, 'ALL'}} ->
@@ -125,6 +140,7 @@ filter(PacketType, Props) when ?CONNECT =< PacketType, PacketType =< ?AUTH, is_l
              end,
     [Prop || Prop = {Name, _} <- Props, Filter(Name)].
 
+-spec(validate(emqx_types:properties()) -> ok).
 validate(Props) when is_map(Props) ->
     lists:foreach(fun validate_prop/1, maps:to_list(Props)).
 
@@ -132,23 +148,32 @@ validate_prop(Prop = {Name, Val}) ->
     case maps:find(id(Name), ?PROPS_TABLE) of
         {ok, {Name, Type, _}} ->
             validate_value(Type, Val)
-              orelse error(bad_property, Prop);
+                orelse error({bad_property_value, Prop});
         error ->
-            error({bad_property, Prop})
+            error({bad_property, Name})
     end.
 
 validate_value('Byte', Val) ->
-    is_integer(Val);
+    is_integer(Val) andalso Val =< 16#FF;
 validate_value('Two-Byte-Integer', Val) ->
-    is_integer(Val);
+    is_integer(Val) andalso 0 =< Val andalso Val =< 16#FFFF;
 validate_value('Four-Byte-Integer', Val) ->
-    is_integer(Val);
+    is_integer(Val) andalso 0 =< Val andalso Val =< 16#FFFFFFFF;
 validate_value('Variable-Byte-Integer', Val) ->
-    is_integer(Val);
+    is_integer(Val) andalso 0 =< Val andalso Val =< 16#7FFFFFFF;
+validate_value('UTF8-String-Pair', {Name, Val}) ->
+    validate_value('UTF8-Encoded-String', Name)
+        andalso validate_value('UTF8-Encoded-String', Val);
+validate_value('UTF8-String-Pair', Pairs) when is_list(Pairs) ->
+    lists:foldl(fun(Pair, OK) ->
+                        OK andalso validate_value('UTF8-String-Pair', Pair)
+                end, true, Pairs);
 validate_value('UTF8-Encoded-String', Val)  ->
     is_binary(Val);
 validate_value('Binary-Data', Val) ->
     is_binary(Val);
-validate_value('UTF8-String-Pair', Val) ->
-    is_tuple(Val) orelse is_list(Val).
+validate_value(_Type, _Val) -> false.
+
+-spec(all() -> map()).
+all() -> ?PROPS_TABLE.
 
diff --git a/test/emqx_mqtt_props_SUITE.erl b/test/emqx_mqtt_props_SUITE.erl
index b1a1b04b4..17ad975b6 100644
--- a/test/emqx_mqtt_props_SUITE.erl
+++ b/test/emqx_mqtt_props_SUITE.erl
@@ -20,23 +20,61 @@
 -compile(nowarn_export_all).
 
 -include("emqx_mqtt.hrl").
+-include_lib("eunit/include/eunit.hrl").
+-include_lib("emqx_ct_helpers/include/emqx_ct.hrl").
 
 all() -> emqx_ct:all(?MODULE).
 
 t_id(_) ->
-    'TODO'.
+    foreach_prop(
+      fun({Id, Prop}) ->
+              ?assertEqual(Id, emqx_mqtt_props:id(element(1, Prop)))
+      end),
+    ?catch_error({bad_property, 'Bad-Property'}, emqx_mqtt_props:id('Bad-Property')).
 
 t_name(_) ->
-    'TODO'.
+    foreach_prop(
+      fun({Id, Prop}) ->
+              ?assertEqual(emqx_mqtt_props:name(Id), element(1, Prop))
+      end),
+    ?catch_error({unsupported_property, 16#FF}, emqx_mqtt_props:name(16#FF)).
 
 t_filter(_) ->
-    'TODO'.
+    ConnProps = #{'Session-Expiry-Interval' => 1,
+                  'Maximum-Packet-Size' => 255
+                 },
+    ?assertEqual(ConnProps,
+                 emqx_mqtt_props:filter(?CONNECT, ConnProps)),
+    PubProps = #{'Payload-Format-Indicator' => 6,
+                 'Message-Expiry-Interval' => 300,
+                 'Session-Expiry-Interval' => 300
+                },
+    ?assertEqual(#{'Payload-Format-Indicator' => 6,
+                   'Message-Expiry-Interval' => 300
+                  },
+                 emqx_mqtt_props:filter(?PUBLISH, PubProps)).
 
 t_validate(_) ->
-    'TODO'.
+    ConnProps = #{'Session-Expiry-Interval' => 1,
+                  'Maximum-Packet-Size' => 255
+                 },
+    ok = emqx_mqtt_props:validate(ConnProps),
+    BadProps = #{'Unknown-Property' => 10},
+    ?catch_error({bad_property,'Unknown-Property'},
+                 emqx_mqtt_props:validate(BadProps)).
 
-deprecated_mqtt_properties_all(_) ->
-    Props = emqx_mqtt_props:filter(?CONNECT, #{'Session-Expiry-Interval' => 1, 'Maximum-Packet-Size' => 255}),
-    ok = emqx_mqtt_props:validate(Props),
-    #{} = emqx_mqtt_props:filter(?CONNECT, #{'Maximum-QoS' => ?QOS_2}).
+t_validate_value(_) ->
+    ok = emqx_mqtt_props:validate(#{'Correlation-Data' => <<"correlation-id">>}),
+    ok = emqx_mqtt_props:validate(#{'Reason-String' => <<"Unknown Reason">>}),
+    ok = emqx_mqtt_props:validate(#{'User-Property' => {<<"Prop">>, <<"Val">>}}),
+    ok = emqx_mqtt_props:validate(#{'User-Property' => [{<<"Prop">>, <<"Val">>}]}),
+    ?catch_error({bad_property_value, {'Payload-Format-Indicator', 16#FFFF}},
+                 emqx_mqtt_props:validate(#{'Payload-Format-Indicator' => 16#FFFF})),
+    ?catch_error({bad_property_value, {'Server-Keep-Alive', 16#FFFFFF}},
+                 emqx_mqtt_props:validate(#{'Server-Keep-Alive' => 16#FFFFFF})),
+    ?catch_error({bad_property_value, {'Will-Delay-Interval', -16#FF}},
+                 emqx_mqtt_props:validate(#{'Will-Delay-Interval' => -16#FF})).
+
+foreach_prop(Fun) ->
+    lists:foreach(Fun, maps:to_list(emqx_mqtt_props:all())).