SMPP 3.4 compatibility for (un)pack with tests (#115)

* compatibility PDU tests
* #114 test cases re-ordered
* pack - ignoring body if the status is not ok
* not parsing body on non OK status for pack and unpack
* removed unused macro M_MSG_ID
* fixed indentation 💄
* tester de-factoring

Author: shamis

src/smpp.erl

@@ -823,28 +823,26 @@ err(undefined)                      -> err(?ESME_RUNKNOWNERR).
                      command_status => statusstr(?ESME_ROK),
                      sequence_number => 0}).
 -define(M_SYS_ID(_Id), ?BASE(_Id)#{system_id => <<>>}).
--define(M_MSG_ID(_Id), ?BASE(_Id)#{message_id => <<>>}).
 -define(M_DST_ADDR(_Id), ?BASE(_Id)#{destination_addr => <<>>}).
 
 info() ->
     #{templates =>
         #{unbind                    => ?BASE(cmd(unbind)),
-          query_sm                  => ?M_MSG_ID(cmd(query_sm)),
-          replace_sm                => ?M_MSG_ID(cmd(replace_sm)),
+          query_sm                  => ?BASE(cmd(query_sm)),
+          replace_sm                => ?BASE(cmd(replace_sm)),
           outbind                   => ?M_SYS_ID(cmd(outbind)),
           enquire_link              => ?BASE(cmd(enquire_link)),
           data_sm                   => ?M_DST_ADDR(cmd(data_sm)),
-          cancel_sm                 => ?M_DST_ADDR(cmd(cancel_sm))
-                                            #{message_id => <<>>},
+          cancel_sm                 => ?M_DST_ADDR(cmd(cancel_sm)),
           submit_sm                 => ?M_DST_ADDR(cmd(submit_sm)),
           deliver_sm                => ?M_DST_ADDR(cmd(deliver_sm)),
           bind_receiver             => ?M_SYS_ID(cmd(bind_receiver)),
           alert_notification        => ?BASE(cmd(alert_notification)),
-          query_broadcast_sm        => ?M_MSG_ID(cmd(query_broadcast_sm)),
-          cancel_broadcast_sm       => ?M_MSG_ID(cmd(cancel_broadcast_sm)),
+          query_broadcast_sm        => ?BASE(cmd(query_broadcast_sm)),
+          cancel_broadcast_sm       => ?BASE(cmd(cancel_broadcast_sm)),
           bind_transmitter          => ?M_SYS_ID(cmd(bind_transmitter)),
           bind_transceiver          => ?M_SYS_ID(cmd(bind_transceiver)),
-          broadcast_sm              => ?M_MSG_ID(cmd(broadcast_sm))
+          broadcast_sm              => ?BASE(cmd(broadcast_sm))
                                             #{broadcast_area_identifier =>
                                                 [#{details => <<>>, format => 0}],
                                               broadcast_content_type =>
@@ -856,22 +854,22 @@ info() ->
                                                     #{dest_address => <<>>},
 
           unbind_resp               => ?BASE(cmd(unbind_resp)),
-          data_sm_resp              => ?M_MSG_ID(cmd(data_sm_resp)),
+          data_sm_resp              => ?BASE(cmd(data_sm_resp)),
           generic_nack              => ?BASE(cmd(generic_nack)),
-          query_sm_resp             => ?M_MSG_ID(cmd(query_sm_resp)),
-          submit_sm_resp            => ?M_MSG_ID(cmd(submit_sm_resp)),
+          query_sm_resp             => ?BASE(cmd(query_sm_resp)),
+          submit_sm_resp            => ?BASE(cmd(submit_sm_resp)),
           cancel_sm_resp            => ?BASE(cmd(cancel_sm_resp)),
-          deliver_sm_resp           => ?M_MSG_ID(cmd(deliver_sm_resp)),
+          deliver_sm_resp           => ?BASE(cmd(deliver_sm_resp)),
           replace_sm_resp           => ?BASE(cmd(replace_sm_resp)),
-          broadcast_sm_resp         => ?M_MSG_ID(cmd(broadcast_sm_resp)),
+          broadcast_sm_resp         => ?BASE(cmd(broadcast_sm_resp)),
           enquire_link_resp         => ?BASE(cmd(enquire_link_resp)),
           bind_receiver_resp        => ?M_SYS_ID(cmd(bind_receiver_resp)),
           cancel_broadcast_sm_resp  => ?BASE(cmd(cancel_broadcast_sm_resp)),
           bind_transceiver_resp     => ?M_SYS_ID(cmd(bind_transceiver_resp)),
           bind_transmitter_resp     => ?M_SYS_ID(cmd(bind_transmitter_resp)),
-          submit_multi_resp         => ?M_MSG_ID(cmd(submit_multi_resp))
+          submit_multi_resp         => ?BASE(cmd(submit_multi_resp))
                                                        #{unsuccess_sme => <<>>},
-          query_broadcast_sm_resp   => ?M_MSG_ID(cmd(query_broadcast_sm_resp))
+          query_broadcast_sm_resp   => ?BASE(cmd(query_broadcast_sm_resp))
                                                     #{broadcast_area_identifier =>
                                                         [#{details => <<>>, format => 0}],
                                                       broadcast_area_success => [0],

src/smpp_param_syntax.erl

@@ -173,11 +173,6 @@ encode_std(undefined, #standard{default = undefined, error = undefined}) ->
 	{error, ?ESME_RUNKNOWNERR};
 encode_std(undefined, #standard{default = undefined, error = Error}) ->
 	{error, Error};
-% SMPP Protocol Specification v3.4 compatibility
-% Sections : 4.1.2, 4.1.4 and 4.4.2
-%     Empty/null message_id (00) is not added if not found in body
-encode_std(undefined, #standard{name = message_id}) ->
-    {ok, <<>>};
 encode_std(undefined, #standard{domain = Domain, default = Default}) ->
     % Assumes default values should always be OK...if not, it'll be easier to
     % review the default definitions rather than touching this clause.

src/smpp_pdu_syntax.erl

@@ -64,30 +64,35 @@ sequence_number({_CmdId, _Status, SeqNum, _Body}) ->
 new_pdu(CmdId, Status, SeqNum, Body) ->
     {CmdId, Status, SeqNum, Body}.
 
-pack({CmdId, Status, SeqNum, Body}, PduType) ->
+pack({CmdId, ?ESME_ROK, SeqNum, Body}, PduType) ->
     case pack_body(Body, PduType) of
         {ok, BodyL} ->
             BodyBin = list_to_binary(BodyL),
             Len = size(BodyBin) + 16,
-            {ok, <<Len:32, CmdId:32, Status:32, SeqNum:32, BodyBin/binary>>};
+            {ok, <<Len:32, CmdId:32, ?ESME_ROK:32, SeqNum:32, BodyBin/binary>>};
         {error, Error} ->
             {error, CmdId, Error, SeqNum}
-    end.
+    end;
+% SMPP Protocol Specification v3.4 compatibility
+% Sections : 4.1.2, 4.1.4 and 4.4.2
+%     Note: The PDU Body is not returned if the command_status field contains a non-zero value
+pack({CmdId, Status, SeqNum, _Body}, _PduType) ->
+    {ok, <<16:32, CmdId:32, Status:32, SeqNum:32>>}.
 
-unpack(<<Len:32, CmdId:32, Status:32, SeqNum:32, Body/binary>>, PduType)
+unpack(<<Len:32, CmdId:32, ?ESME_ROK:32, SeqNum:32, Body/binary>>, PduType)
   when Len == size(Body) + 16 ->
     case unpack_body(Body, PduType) of
         {ok, BodyParams} ->
-            {ok, new_pdu(CmdId, Status, SeqNum, BodyParams)};
+            {ok, new_pdu(CmdId, ?ESME_ROK, SeqNum, BodyParams)};
         {error, Error} ->
-            % SMPP Protocol Specification v3.4 compatibility
-            % Sections : 4.1.2, 4.1.4 and 4.4.2
-            %     Note: The PDU Body is not returned if the command_status field contains a non-zero value
-            case Status of
-                ?ESME_ROK -> {error, CmdId, Error, SeqNum};
-                Status -> {ok, new_pdu(CmdId, Status, SeqNum, [])}
-            end
+            {error, CmdId, Error, SeqNum}
     end;
+% SMPP Protocol Specification v3.4 compatibility
+% Sections : 4.1.2, 4.1.4 and 4.4.2
+%     Note: The PDU Body is not returned if the command_status field contains a non-zero value
+unpack(<<Len:32, CmdId:32, Status:32, SeqNum:32, Body/binary>>, _PduType)
+  when Len == size(Body) + 16 ->
+    {ok, new_pdu(CmdId, Status, SeqNum, [])};
 unpack(<<_Len:32, CmdId:32, _Status:32, SeqNum:32, _Body/binary>>, _PduType) ->
     {error, CmdId, ?ESME_RINVCMDLEN, SeqNum}.
 

test/smpp_3_4.erl

@@ -0,0 +1,37 @@
+-module(smpp_3_4).
+
+-include_lib("eunit/include/eunit.hrl").
+-include("../src/smpp_globals.hrl").
+
+-define(TESTS, [
+	{"submit_sm_resp(ESME_RINVMSGLEN)",
+		"00000010800000040000000100000001",
+		{?COMMAND_ID_SUBMIT_SM_RESP, ?ESME_RINVMSGLEN, 1, []}},
+	{"submit_sm_resp(ESME_RTHROTTLED)",
+		"00000010800000040000005800000001",
+		{?COMMAND_ID_SUBMIT_SM_RESP, ?ESME_RTHROTTLED, 1, []}},
+	{"submit_sm_resp(ESME_RMSGQFUL)",
+		"00000010800000040000001400000001",
+		{?COMMAND_ID_SUBMIT_SM_RESP, ?ESME_RMSGQFUL, 1, []}},
+	{"bind_transceiver_resp(ESME_RALYBND)",
+		"00000010800000090000000500000001",
+		{?COMMAND_ID_BIND_TRANSCEIVER_RESP, ?ESME_RALYBND, 1, []}},
+	{"bind_transceiver_resp(ESME_RBINDFAIL)",
+		"00000010800000090000000D00000001",
+		{?COMMAND_ID_BIND_TRANSCEIVER_RESP, ?ESME_RBINDFAIL, 1, []}}
+]).
+
+packunpack_test_() ->
+    {inparallel,
+     [{T,
+	  	fun() ->
+		  	Bin = hex2bin(Pdu),
+			?assertEqual({ok, Parsed}, smpp_operation:unpack(Bin)),
+			?assertEqual({ok, Bin}, smpp_operation:pack(Parsed))
+		end}
+      || {T, Pdu, Parsed} <- ?TESTS]
+    }.
+
+hex2bin([]) -> <<>>;
+hex2bin([A,B|Rest]) ->
+	<<(list_to_integer([A,B], 16)), (hex2bin(Rest))/binary>>.

test/tests.hrl

@@ -7,8 +7,6 @@
 -define(PDU(_Id), ?PDU(_Id, "")).
 -define(PDU_SYSID(_Id), ?PDU(_Id, ",\"system_id\":\"\",\"sc_interface_version\":80")).
 -define(PDU_DSTADDR(_Id), ?PDU(_Id, ",\"destination_addr\":\"\"")).
--define(PDU_MSGID(_Id), ?PDU(_Id, ",\"message_id\":\"\"")).
--define(PDU_DSTADDR_MSGID(_Id), ?PDU(_Id, ",\"destination_addr\":\"\",\"message_id\":\"\"")).
 
 -define(TESTS,
 [{"bind_transceiver",
@@ -950,32 +948,31 @@
      message_id => <<"this_could_be_a_message_id">>,
      sequence_number => 1}},
   {"deliver_sm_resp_error_status_code_#107",
-   "00 00 00 17 80 00 01 03 00 00 00 05 00 00 00 00 31 32 33 34 35 36 00",
-   #{command_id => <<"data_sm_resp">>, command_length => 23,
-     command_status => <<"ESME_RALYBND">>, message_id => <<"123456">>,
-     sequence_number => 0}}
+   "00 00 00 10 80 00 01 03 00 00 00 05 00 00 00 01",
+   #{command_id => <<"data_sm_resp">>, command_length => 16,
+     command_status => <<"ESME_RALYBND">>, sequence_number => 1}}
 ]).
 
 -define(TESTS2,
 [% requests
 {"bind_receiver",          16#00000001,  ?PDU_SYSID(16#00000001)},
 {"bind_transmitter",       16#00000002,  ?PDU_SYSID(16#00000002)},
-{"query_sm",               16#00000003,  ?PDU_DSTADDR_MSGID(16#00000003)},
+{"query_sm",               16#00000003,  ?PDU_DSTADDR(16#00000003)},
 {"submit_sm",              16#00000004,  ?PDU_DSTADDR(16#00000004)},
 {"deliver_sm",             16#00000005,  ?PDU_DSTADDR(16#00000005)},
 {"unbind",                 16#00000006,  ?PDU_DSTADDR(16#00000006)},
-{"replace_sm",             16#00000007,  ?PDU_DSTADDR_MSGID(16#00000007)},
-{"cancel_sm",              16#00000008,  ?PDU_DSTADDR_MSGID(16#00000008)},
+{"replace_sm",             16#00000007,  ?PDU_DSTADDR(16#00000007)},
+{"cancel_sm",              16#00000008,  ?PDU_DSTADDR(16#00000008)},
 {"bind_transceiver",       16#00000009,  ?PDU_SYSID(16#00000009)},
 {"outbind",                16#0000000B,  ?PDU_SYSID(16#0000000B)},
 {"enquire_link",           16#00000015,  ?PDU(16#000000015)},
 
 % responses
 {"bind_receiver_resp",     16#80000001,  ?PDU_SYSID(16#80000001)},
 {"bind_transmitter_resp",  16#80000002,  ?PDU_SYSID(16#80000002)},
-{"query_sm_resp",          16#80000003,  ?PDU_MSGID(16#80000003)},
-{"submit_sm_resp",         16#80000004,  ?PDU_MSGID(16#80000004)},
-{"deliver_sm_resp",        16#80000005,  ?PDU_MSGID(16#80000005)},
+{"query_sm_resp",          16#80000003,  ?PDU(16#80000003)},
+{"submit_sm_resp",         16#80000004,  ?PDU(16#80000004)},
+{"deliver_sm_resp",        16#80000005,  ?PDU(16#80000005)},
 {"unbind_resp",            16#80000006,  ?PDU(16#80000006)},
 {"replace_sm_resp",        16#80000007,  ?PDU(16#80000007)},
 {"cancel_sm_resp",         16#80000008,  ?PDU(16#80000008)},