Ensure API params have binary keys

[wojtekmach]

In some cases we manually use binary keys:

https://github.com/hexpm/hex_core/blob/v0.6.1/src/hex_api_key.erl#L20:

Params = #{<<"name">> => Name, <<"permissions">> => Permissions},

but in some we allow user to pass the map:

https://github.com/hexpm/hex_core/blob/v0.6.1/src/hex_api_release.erl#L45

hex_api:post(Config, Path, Params).

And so the user may pass atom keys like we used to do here:

wojtekmach/mini_repo@f6c8f39#diff-5008c7016636a9b7531bd02fa4abb5ffL52

We should ensure the params are always a map of binary keys.

[essen]

Ran into this, in particular https://github.com/hexpm/hex_core/blob/master/src/hex_api_release.erl#L17 has retire params accept atoms keys and values when it should all be binaries.

[wojtekmach]

@essen is it a blocker or were you able to publish after all? we're happy to help in the meantime while we work on the proper fix.

[essen]

It's just the type it doesn't really matter it just confused me for a few minutes. I can publish, delete, retire etc. just fine (on local dev environment, for now anyway).

[starbelly]

I think there's two ways to solve this :

1. change the type specs to binary() for domain and resource.
2. doc the valid values
3. validate the parameters such that if any values are found in maps that are not binaries, return an error tuple with some generic message. Quickest way to fix it up basically (i.e., lists:all(fun(T) -> T end, lists:map(fun(P) -> lists:any(fun(V) -> not is_binary(V) end, maps:values(P)) end, [#{this => that}, #{this => <<"that">>}])).).

Or

1. same as the above
2. same as the above
3. validate the parameters but return specifics on which keys had non binary values, and return an error tuple with those in them and perhaps a message that tells the user what valid values are for the keys that had non-valid values.

I'm happy to do a PR for this one, but yeah need to suss that out. Quick glance shows this is limited to just a few modules, no? Namely, the ones you pointed out in this issue.

[wojtekmach]

Or option 3, raise :) I’d start with that as it’s easier to implement but also I think more desirable.

[starbelly]

@wojtekmach makes sense to me, I'll get up a PR 🔜 and we can hash out what the error message should be exactly 🚀

Edit:

@wojtekmach are you thinking raise early (i.e., as soon as an invalid value is found, whether it's a non-binary or a non-valid value) or are you thinking thinking iterate all the k/vs then raise? I suppose if we're going to raise with a generic message or raise with an error about a specific k/v, then an eager raise makes sense to me, but would like to get on the same page.

[wojtekmach]

yeah let's raise early with an error about a specific k/v. if people complain that this creates a game of whack-a-mole, we could change the error to include the whole params map. I'd rather avoid the latter given it can end up including secrets like passwords in the error message which to be fair is still possible with the approach we're taking, just rather unlikely given we'd fail on the first non-binary key.

[starbelly]

creates a game of whack-a-mole,

🤣 Love it

[starbelly]

@wojtekmach It looks like the following modules and their type specs and docs need to be updated :

• src/hex_api_release.erl
• src/hex_api_organization_member.erl
• src/hex_pb_package.erl (this one may be best left alone for a bit).

Can you confirm?

[wojtekmach]

Let's hold off on those changes. I thought about it a little bit last night and my conclusion is we'll be doing changes like these:

-hex_api_package:search(Config, <<"hex_core">>, [{page, 1}]).
+hex_api_package:search(Config, <<"hex_core">>, [{<<"page">>, 1}]).

but notice that we most likely won't turn 1 to <<"1">>. So why change the key but not the value? Well, that would be to the detriment of the interface.

The server accepts data in 3 ways:

• in URI query string, it's all strings
• as JSON, it's mostly strings but we also have numbers and booleans
• in ETF, it can be any term

The root cause of this issue was in the minirepo I used the atom keys and that was the bug. it means the server wasn't fully interoperable, it wouldn't work with clients that can only send JSON. Another way to solve such issue would be to make sure hex_core just before dumping to encodes keys appropriately so when we decode from ETF we always have binary keys, just like we would when decoding from JSON. So yeah, I'm now thinking we should move forward with this instead of what this issues says. Thoughts?

[starbelly]

The root cause of this issue was in the minirepo I used the atom keys and that was the bug. it means the server wasn't fully interoperable, it wouldn't work with clients that can only send JSON. Another way to solve such issue would be to make sure hex_core just before dumping to encodes keys appropriately so when we decode from ETF we always have binary keys, just like we would when decoding from JSON. So yeah, I'm now thinking we should move forward with this instead of what this issues says. Thoughts?

This approach makes more sense to me as you get to keep/restore the specificity that comes with atoms (the downside of the last PR is that is now gone).

[wojtekmach]

@ericmj do you have a preference whether we should allow atom or binary keys in params? I believe the advantage of atom keys is we can type spec them. What I think is pretty clear is if we allow atom keys we should dump them to strings before sending to the server, so that we have parity between ETF and JSON. (In JSON they are always dumped.)

[paulo-ferraz-oliveira]

I prefer atom, for the -spec(_). reason, and wouldn't mind (at least for the bit I changed in hex_core) updating the code so only atoms are accepted.

[starbelly]

Atoms still make the most sense to me, so long as they are stringified before they make it from hex_core to hexpm ;)

[ericmj]

do you have a preference whether we should allow atom or binary keys in params? I believe the advantage of atom keys is we can type spec them. What I think is pretty clear is if we allow atom keys we should dump them to strings before sending to the server, so that we have parity between ETF and JSON. (In JSON they are always dumped.)

I think we should use binary keys, otherwise we have to update the library every time the API changes. Having a stricter spec would be nice but then I think it needs to be generated so we don't have to manually update it.

[wojtekmach]

Good point. Let’s go with binary keys as originally stated. Thanks @ericmj!

[paulo-ferraz-oliveira]

Cool. Then it's the -spec(_). that's wrong. I'll update it in my PR. Edit: done in #150.