Duplicate keys in *.app env entry causes -config option to be ignored

[jesperes]

Describe the bug
If an OTP application has duplicate keys in the env sections of it's *.app file, any file specified using the -config option to erl is ignored, and instead value of the last duplicate key is used.

To Reproduce

1. Create a minimal OTP application, e.g. using rebar3 new testapp
2. Add a duplicate key to *.app.src:

  {env,[ {key1, '00_this_value_comes_from_app_env'}
       , {key1, '00_this_is_a_duplicate_value_from_app_env'}
       ]},

3. Create a sys.config file with an override:

[{testapp,
  [{key1, '10_this_value_comes_from_sysconfig'}]}
].

4. Run erl and print the app env:

rebar3 compile
export ERL_LIBS=_build/default/lib
erl -noshell -config sys.config -eval 'application:start(testapp), io:format("~p~n", [application:get_all_env(testapp)]).' -s erlang halt

5. The value of key1 is the duplicate value in testapp.app.src, not the override in the sys.config file.

12:51 $ ./run.sh 
===> Verifying dependencies...
===> Analyzing applications...
===> Compiling testapp
[{key1,'00_this_is_a_duplicate_value_from_app_env'}]

6. If you comment out the duplicate value in testapp.app.src:

  {env,[ {key1, '00_this_value_comes_from_app_env'}
       %% , {key1, '00_this_is_a_duplicate_value_from_app_env'}
       ]},

Then the override takes precedence as expected:

12:51 $ ./run.sh 
===> Verifying dependencies...
===> Analyzing applications...
===> Compiling testapp
[{key1,'10_this_value_comes_from_sysconfig'}]

Expected behavior
According to the docs:

Configuration parameter values in a configuration file or file descriptor override the values in the application resource files (see app(4)). The values in the configuration file are always overridden by command-line flags (see erts:erl(1)).

So, I would expect the value key1 to always be 10_this_value_comes_from_sysconfig, overriding any value in testapp.app.src.

Affected versions
I've been able to reproduce this in OTP-22.3.4.20, OTP-23.3.4.2, OTP-24.1.3, OTP-24.2.1, OTP-25.0-rc1

[jesperes]

Admittedly, having duplicate keys in *.app files can be considered a bug the application itself, and I would be fine with OTP just picking any of the duplicate keys, but ignoring any -config option just because there is a duplicate key is a (very) blatant violation of "the principle of least astonishment".

[garazdawi]

So what happens is this:

> Keys = application_controller:merge_app_env([{key1,app1},{key1,app2}],[{key1,config1}]).
[{key1,config1},{key1,app2}].
> [ets:insert(ac_tab, Tuple) || Tuple <- Keys].
[ok, ok].

That is, the config value gets merged into the first key and then the first key gets overwritten by the second key when the environment is inserted in the application_controller table.

This is obviously a bug, but I'm unsure what to do about it. Either reject any .app file with duplicate keys defined, or normalize the .app env values before merging with the sys.config value.

[jesperes]

Reject, I would say. The -config option already does this:

11:09 $ erl -config dups.config
{"could not start kernel pid",application_controller,"invalid config data: application: kdb; duplicate parameter: key1"}
could not start kernel pid (application_controller) (invalid config data: application: kdb; duplicate parameter: key1)

Crash dump is being written to: erl_crash.dump...done

[jesperes]

Although rejecting might have some backwards compatibility issues which hits existing applications.

[garazdawi]

I did not realize that -config already rejected duplicate keys. Seems reasonable that .app files should follow the same logic.

[garazdawi]

A potential fix is available in #5878. I'm not sure if I want to make this part of Erlang/OTP 25, or leave it for 26. It has just missed the window for the last rc and we don't like to add additional potentially breaking changes after the final rc.

[jesperes]

I'm fine with leaving it for OTP 26.