Don't flag AVP as missing as a consequence of decode error

The decode of an incoming Diameter message uses the record
representation to determine whether or not an AVP has been received with
the expected arity, the number of AVPs in each field following decode
being compared with the arity specified in the message grammar. The
problem with this is that decode failure isn't reflected in the record
representation, so that an AVP can be appended to the errors field of a
diameter_packet record despite an entry for the same AVP already
existing. This isn't a fault as much as a misleading error indication,
but now only append AVPs that aren't already represented.

1 files changed, 15 insertions, 6 deletions

diff --git a/lib/diameter/include/diameter_gen.hrl b/lib/diameter/include/diameter_gen.hrl
index e8ffe7f92c..0b36389088 100644
--- a/lib/diameter/include/diameter_gen.hrl
+++ b/lib/diameter/include/diameter_gen.hrl
@@ -185,9 +185,10 @@ decode_avps(Name, Recs) ->
         = lists:foldl(fun(T,A) -> decode(Name, T, A) end,
                       {[], {newrec(Name), []}},
                       Recs),
-    {Rec, Avps, Failed ++ missing(Rec, Name)}.
-%% Append 5005 errors so that a 5014 for the same AVP will take
-%% precedence in a Result-Code/Failed-AVP setting.
+    {Rec, Avps, Failed ++ missing(Rec, Name, Failed)}.
+%% Append 5005 errors so that errors are reported in the order
+%% encountered. Failed-AVP should typically contain the first
+%% encountered error accordg to the RFC.
 
 newrec(Name) ->
     '#new-'(name2rec(Name)).
@@ -202,10 +203,18 @@ newrec(Name) ->
 %%      Vendor-Id if applicable.  The value field of the missing AVP
 %%      should be of correct minimum length and contain zeroes.
 
+missing(Rec, Name, Failed) ->
+    [{5005, A} || #diameter_avp{code = MC, vendor_id = MV} = A
+                      <- missing(Rec, Name),
+                  lists:all(fun({_, #diameter_avp{code = C, vendor_id = V}}) ->
+                                        MC /= C orelse MV /= V
+                            end,
+                            Failed)].
+
 missing(Rec, Name) ->
-    [{5005, empty_avp(F)} || F <- '#info-'(element(1, Rec), fields),
-                             A <- [avp_arity(Name, F)],
-                             false <- [have_arity(A, '#get-'(F, Rec))]].
+    [empty_avp(F) || F <- '#info-'(element(1, Rec), fields),
+                     A <- [avp_arity(Name, F)],
+                     not have_arity(A, '#get-'(F, Rec))].
 
 %% Maximum arities have already been checked in building the record.