silviucpp / erlkaf

Erlang kafka driver based on librdkafka
MIT License
83 stars 41 forks source link

Dialyzer shows an error for usages of `create_consumer_group` #44

Closed ramonpin closed 2 years ago

ramonpin commented 2 years ago

The contract for create_consumer_group function says:

-spec create_consumer_group(client_id(), binary(), [binary()], [client_option()], [topic_option()]) -> ok | {error, reason()}.

create_consumer_group(ClientId, GroupId, Topics, ClientConfig0, DefaultTopicsConfig)

It seems that we expect Topics to be a [binary()] but later in the code there is the following validation:

valid_consumer_topics([H|T]) ->
    case H of
        {K, V} when is_binary(K) and is_list(V) ->
            Mod = erlkaf_utils:lookup(callback_module, V),
            case Mod =/= undefined andalso is_atom(Mod) of
                true ->
                    valid_consumer_topics(T);
                _ ->
                    {error, {invalid_topic, H}}
            end;
        _ ->
            {error, {invalid_topic, H}}
    end;

That seems to force Topics to be a list of pairs {binary(), [{callback_module, atom()}]} or similar.

Dialyzer says the contract is broken for valid Topics parameter.

lib/broadway_erlkaf/v1/api/topic.ex:0: The call erlkaf:create_consumer_group
         (_worker_name@1 :: atom(),
          _group_name@1 :: any(),
          [{_,
            [{'callback_args', [any(), ...]} |
             {'callback_module', 'Elixir.BroadwayErlkaf.V1.Api.Topic'} |
             {'dispatch_mode', {'batch', _}},
             ...]},
           ...],
          _client@1 :: any(),
          _topics_opts@1 :: [{'auto_offset_reset', 'smallest'}, ...]) breaks the contract 
          (client_id(),
          binary(),
          [binary()],
          [client_option()],
          [topic_option()]) ->
             'ok' | {'error', reason()}

I believe we only need to properly define the spec for the Topics parameter at create_consumer_group but don't really know which is the proper type for it.

silviucpp commented 2 years ago

Hello,

While ago I changed the interface for consumers but I forgot to update the specs. You can read the motivation here:

https://github.com/silviucpp/erlkaf/blob/master/CHANGELOG.md#v200

In erlkaf 1.x the topics were under : [<<"topic1">>, <<"topic2">>] format.

Now in 2.x:

[
        {<<"topic1">>, [
            {callback_module, module_topic1},
            {callback_args, []}
        ]},
        {<<"topic2">>, [
            {callback_module, module_topic2},
            {callback_args, []}
        ]}

So it's a tuple, where the first element is the topic name (a binary) and the second is a list of callback settings.

I think the correct signature is now to add in the erlkaf.hrl a new type:

-type callback_options() :: callback_module|callback_args.

And then to define it as :

[{binary(), [callback_options()]}]

What do you think ? Also if you agree I apreciate if you can send a PR for this.

ramonpin commented 2 years ago

I will prepare a PR. My hesitation was about me not knowing how to do It properly. I am just a beginner with spec declarations on erlang.

I believe your proposed type will work.

silviucpp commented 2 years ago

Thanks for PR !