Fix interpretation of vendor id in @grouped

A value is required to be the same as any specified with
@avp_vendor_id but otherwise the two locations are equivalent.
Both possibilities are allowed since @avp_vendor_id is required
for AVPs of types other than Grouped (modulo it not really needing
to exist at all: see commit 943266c9) and since the grammar parsed
in @grouped (from RFC 3588) allows it.

3 files changed, 25 insertions, 10 deletions

diff --git a/lib/diameter/src/compiler/diameter_codegen.erl b/lib/diameter/src/compiler/diameter_codegen.erl
index 3105310c5b..6763e06140 100644
--- a/lib/diameter/src/compiler/diameter_codegen.erl
+++ b/lib/diameter/src/compiler/diameter_codegen.erl
@@ -376,9 +376,7 @@ avp_name(Spec) ->
     Avps = get_value(avp_types, Spec),
     Imported = get_value(import_avps, Spec),
     Vid = orddict:find(vendor, Spec),
-
-    Vs = lists:flatmap(fun({V,Ns}) -> [{N,V} || N <- Ns] end,
-                       get_value(avp_vendor_id, Spec)),
+    Vs = vendor_id_map(Spec),
 
     lists:map(fun(T) -> c_avp_name(T, Vs, Vid) end, Avps)
         ++ lists:flatmap(fun(T) -> c_imported_avp_name(T, Vs) end, Imported)
@@ -393,7 +391,9 @@ c_avp_name({Name, Code, Type, Flags}, Vs, Vid) ->
 %% avp_vendor_id in the inheriting module and vendor in the inherited
 %% module. In particular, avp_vendor_id in the inherited module is
 %% ignored so can't just call Mod:avp_header/1 to retrieve the vendor
-%% id.
+%% id. A vendor id specified in @grouped is equivalent to one
+%% specified as avp_vendor_id.
+
 c_imported_avp_name({Mod, Avps}, Vs) ->
     lists:map(fun(A) -> c_avp_name(A, Vs, {module, Mod}) end, Avps).
 
@@ -407,6 +407,14 @@ c_avp_name_(T, Code, Vid) ->
      [],
      [T]}.
 
+vendor_id_map(Spec) ->
+    lists:flatmap(fun({V,Ns}) -> [{N,V} || N <- Ns] end,
+                  get_value(avp_vendor_id, Spec))
+        ++ lists:flatmap(fun({_,_,[],_}) -> [];
+                            ({N,_,[V],_}) -> [{N,V}]
+                         end,
+                         get_value(grouped, Spec)).
+
 %%% ------------------------------------------------------------------------
 %%% # avp_arity/2
 %%% ------------------------------------------------------------------------
@@ -615,9 +623,7 @@ avp_header(Spec) ->
     Native = get_value(avp_types, Spec),
     Imported = get_value(import_avps, Spec),
     Vid = orddict:find(vendor, Spec),
-
-    Vs = lists:flatmap(fun({V,Ns}) -> [{N,V} || N <- Ns] end,
-                       get_value(avp_vendor_id, Spec)),
+    Vs = vendor_id_map(Spec),
 
     lists:flatmap(fun(A) -> c_avp_header(A, Vs, Vid) end,
                   Native ++ Imported).
@@ -633,6 +639,7 @@ c_avp_header({Mod, Avps}, Vs, _Vid) ->
 %% Note that avp_vendor_id in the inherited dictionary is ignored. The
 %% value must be changed in the inheriting dictionary. This is
 %% consistent with the semantics of avp_name/2.
+
 c_imported_avp_header({Name, _Code, _Type, _Flags}, Mod, Vs) ->
     Apply = ?APPLY(Mod, avp_header, [?Atom(Name)]),
     {?clause, [?Atom(Name)],
diff --git a/lib/diameter/src/compiler/diameter_dict_util.erl b/lib/diameter/src/compiler/diameter_dict_util.erl
index 09aab4eff4..2207925e49 100644
--- a/lib/diameter/src/compiler/diameter_dict_util.erl
+++ b/lib/diameter/src/compiler/diameter_dict_util.erl
@@ -907,6 +907,11 @@ avp_type_known(Type, Name, Line) ->
         orelse ?RETURN(avp_has_unknown_type, [Name, Line, Type]).
 
 %% vendor_id_mismatch/6
+%%
+%% Require a vendor id specified on a group to match any specified
+%% in @avp_vendor_id. Note that both locations for the value are
+%% equivalent, both in the value being attributed to a locally
+%% defined AVP and ignored when imported from another dictionary.
 
 vendor_id_mismatch({_,_,_}, false, Name, _, Line, DefLine) ->
     ?RETURN(grouped_vendor_id_without_flag, [Name, Line, DefLine]);
diff --git a/lib/diameter/test/diameter_compiler_SUITE.erl b/lib/diameter/test/diameter_compiler_SUITE.erl
index a224a4f12e..cc4b1ddac5 100644
--- a/lib/diameter/test/diameter_compiler_SUITE.erl
+++ b/lib/diameter/test/diameter_compiler_SUITE.erl
@@ -136,6 +136,9 @@
           [{"(Failed-AVP .*)>", "\\1 17>"},
            {"^ *Failed-AVP .*$", "&V"},
            {"@avp_types", "@avp_vendor_id 18 Failed-AVP\n&"}]},
+         {ok,
+          [{"(Failed-AVP .*)>", "\\1 17>"},
+           {"^ *Failed-AVP .*$", "&V"}]},
          {message_name_already_defined,
           "CEA ::= .*:",
           "& 257 > {Result-Code}\n&"},
@@ -244,10 +247,10 @@ replace({E, RE, Repl}, Bin) ->
 
 replace({E, Mods}, Bin) ->
     B = iolist_to_binary(lists:foldl(fun re/2, Bin, Mods)),
-    case diameter_dict_util:parse(B, [{include, here()}]) of
-        {ok, Dict} when E == ok ->
+    case {E, diameter_dict_util:parse(B, [{include, here()}]), Mods} of
+        {ok, {ok, Dict}, _} ->
             Dict;
-        {error, {E,_} = T} ->
+        {_, {error, {E,_} = T}, _} ->
             S = diameter_dict_util:format_error(T),
             true = nochar($", S, E),
             true = nochar($', S, E),