funbox/smppex

submit_sm/deliver_sm data_coding is mandatory, but the factory never includes it

archseer opened this issue · 3 comments

archseer commented 
iex(2)> SMPPEX.Pdu.Factory.submit_sm({"123", 1, 1}, {"456", 1, 1}, "Hi!")
%SMPPEX.Pdu{command_id: 4, command_status: 0,
 mandatory: %{dest_addr_npi: 1, dest_addr_ton: 1, destination_addr: "456",
   registered_delivery: 0, short_message: "Hi!", source_addr: "123",
   source_addr_npi: 1, source_addr_ton: 1}, optional: %{},
 ref: #Reference<0.0.5.466>, sequence_number: 0}

I've gotten around it by doing:

defp set_mandatory_field(%Pdu{} = pdu, field, val) do
  %{pdu | mandatory: pdu.mandatory |> Map.put(field, val)}
end

pdu |> set_mandatory_field(:data_coding, 3)

If we extend the submit_sm/deliver_sm factories though, we break backwards compatibility -- the spec states "There is no default setting for the data_coding parameter." so we'd have to include it as an extra argument with no default value (function arity would change)

savonarola commented

Hello!

But there is a built in function with the same name :)

iex(1)> SMPPEX.Pdu.Factory.submit_sm({"123", 1, 1}, {"456", 1, 1}, "Hi!") |> SMPPEX.Pdu.set_mandatory_field(:data_coding, 3) 
%SMPPEX.Pdu{command_id: 4, command_status: 0,
 mandatory: %{data_coding: 3, dest_addr_npi: 1, dest_addr_ton: 1,
   destination_addr: "456", registered_delivery: 0, short_message: "Hi!",
   source_addr: "123", source_addr_npi: 1, source_addr_ton: 1}, optional: %{},
 ref: #Reference<0.0.3.452>, sequence_number: 0}

data_coding is an integer single byte mandatory field, we can't put any kind of "nil" value in a binary packet, so when it is not specified, we should choose one of the following:

  • fail to generate binary packet;
  • set some default value.

Since there are many mandatory fields which are generally not used (usually filled with nil/empty strings or zeroes) failing is not a good option: there would be a lot of boilerplate to be done to send a single pdu. So SMPPEX chooses to treat nil or missing mandatory integer fields as zeroes:

iex(1)> {:ok, data} = SMPPEX.Pdu.Factory.submit_sm({"123", 1, 1}, {"456", 1, 1}, "Hi!") |> SMPPEX.Protocol.build
{:ok,
 <<0, 0, 0, 42, 0, 0, 0, 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 49, 50, 51, 0, 1,
   1, 52, 53, 54, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 72, 105, 33>>}
iex(2)> {:ok, {:pdu, pdu}, _} = data |> SMPPEX.Protocol.parse
{:ok,
 {:pdu,
  %SMPPEX.Pdu{command_id: 4, command_status: 0,
   mandatory: %{data_coding: 0, dest_addr_npi: 1, dest_addr_ton: 1,
     destination_addr: "456", esm_class: 0, priority_flag: 0, protocol_id: 0,
     registered_delivery: 0, replace_if_present_flag: 0,
     schedule_delivery_time: "", service_type: "", short_message: "Hi!",
     sm_default_msg_id: 0, sm_length: 3, source_addr: "123", source_addr_npi: 1,
     source_addr_ton: 1, validity_period: ""}, optional: %{},
   ref: #Reference<0.0.3.415>, sequence_number: 0}}, ""}
iex(3)> pdu |> SMPPEX.Pdu.mandatory_field(:data_coding)
0
savonarola commented

My idea about extending factory is the following: we may extend factory to accept as message argument one of the following:

  • a binary message
  • a tuple {data_coding, message}

so that we would be able to call factory in the following way:

pdu = SMPPEX.Pdu.Factory.submit_sm({"123", 1, 1}, {"456", 1, 1}, {3, "Hi!"})
archseer commented

Sorry for the late reply! Yeah I figured that most mandatory fields could default to null, but data_coding seemed necessary for us, since most upstreams treat it as 7bit encoding.

I somehow missed the builtin function! Swapped our code to use that from now on.

As always, thanks for the swift reply!