Validate package metadata

[ericmj]

Validate package metadata against spec errors, do not include superfluous fields and do not allow type errors.

We can also add separate, optional functions to validate package metadata for warnings, we can pass it a list of policies that can be used, like (hexpm/public/private). We could handle the tarball size validation the same way to not be locked into hexpm specifics.

NOTE: Do not include maintainers in the package metadata.

[jadeallenx]

What are superfluous fields? Like if I add <<"dog">> => <<"cat">> to the map in the .app.src file, you're saying prune that field out?

[ericmj]

Correct.

[jadeallenx]

Do you mean modify the underlying file as it's stored in hex or just ignore it for purposes of hex?

[ericmj]

I am talking about the package metadata. The package metadata is built from app.src, but we don't care about the actual contents of app.src inside the package files. When we build the package metadata from the configuration in either mix.exs or app.src we should include relevant fields for the metadata.

[jadeallenx]

ОК, thank you for clarifying!

[wojtekmach]

When we have basic validations (required fields, type checks etc) we could add more validations on top:

• check that links are valid URLs
• check that version follows semver.org
• check that included license is a valid spdx.org license identifier (proof of concept)

etc. We should work on these separately, just mentioning these as examples of things we may want to consider.

[wojtekmach]

Another validation:

• ensure all decoded strings are utf8 (Verify Binary Strings are Printable  elixir-ecto/ecto#161 (comment))

[starbelly]

Aye, so the validation code that just made it into rebar3_hex (erlef/rebar3_hex#85) really should probably be in hex_core. If agreed, I'll put something together in a few days.

[ericmj]

It would be great if you could contribute this to hex_core as well. This is what hex validates [1], I haven't looked at the rebar3_hex PR yet.

[1] https://github.com/hexpm/hex/blob/master/lib/mix/tasks/hex.build.ex#L165

[wojtekmach]

Note, these are two aspects of validating package metadata:

• when creating the tarball
• when unpacking the tarball

Let's focus on validations on creation in this issue for now.

[starbelly]

I just released 0.1.0 of https://github.com/starbelly/verl . For the moment, it's just semver parsing, but will be adding the rest later. Whether hex_core wants to vendor this in or use as dep or has other plans is the question. After that decision is made I can do a PR that validates the version.

[starbelly]

semver aside... what should we do when validation errors have accumulated prior to creating a tarball? I see @ericmj was saying deprecated metadata should be ignored, as it is server side. So it sounds like we don't want to end in error if the maintainers field exists and everything else is ok? If so, I'm thinking we should return

 #{
    errors => [{type, MetaData}, {type, MetaData}], 
    warnings => [{type, MetaData}, {type, MetaData}]
 }

... when they exist and the caller can choose to treat the warnings as errors or not, etc. Then the caller can possibly pass the whole result back to hex_core for formatting errors and warnings using a format_error/1 or could iterate the lists themselves and call format_error/2 per error/warning, possibly selectively.

[ericmj]

That sounds like a good idea. That way we can also better control the deprecation process.

[wojtekmach]

I think we have two options:

1. a separate validate functions that returns the map above. This means that the caller would first have to validate metadata prior to calling hex_tarball:create

2. run validations on create. This means that on success we return the tarball and warnings (but not errors) and on failure we return warnings and errors.

   Maybe something like this?

   -spec create(metadata(), files()) ->
     {ok, {tarball(), checksum(), Warnings :: [{atom(), term()}]} |
     {error, #{errors => [{binary(), term()}], warnings => [{binary(), term()}]

   Examples:

   > hex_tarball:create(Metadata, Files).
   {ok, {Tarball, Checksum, [{<<"maintainers">>, deprecated}]}}
   
   > hex_tarball:create(#{<<"foo">> => <<"bar">>}, Files).
   {ok, {Tarball, Checksum, [{<<"foo">>, unknown_field}]}}
   
   > hex_tarball:create(#{<<"version">> => <<"1.0.bad">>}}, Files).
   {error, #{errors => [{<<"version">>, invalid}], warnings => []}}
   
   > hex_tarball:create(#{<<"licenses">> => <<"">>}}, Files).
   {error, #{errors => [{<<"licenses">>, invalid}], warnings => []}}

I lean towards option 2. What do you think?

Something to keep in mind is that later we'd want to handle hex_tarball:unpack metadata errors in the same way. There are also non-metadata errors that can be returned, e.g. we call: hex_tarball:create(Metadata, [{'foo.erl', '/non_existing'}]) - should it return a different error tuple? (Currently it crashes.)

[ericmj]

Yeah, 2. sounds even better.