Use Jason instead of Poison

[tlvenn]

This commit replace Poison with Jason and also wrap args so that there no side effect with them on the LUA side with cJson

Fixes: #165

[coveralls]

Coverage increased (+0.2%) to 87.942% when pulling 81121bc on tlvenn:jason into d8a2d05 on edgurgel:master.

[coveralls]

Coverage increased (+0.5%) to 88.29% when pulling 3648e9a on tlvenn:jason into d8a2d05 on edgurgel:master.

[edgurgel]

Do we need to convert to atoms because of the struct needs atoms to be reconstructed?

[tlvenn]

Yes exactly, from the doc:

Keys in the Enumerable that don’t exist in the struct are automatically discarded.
Note that keys must be atoms, as only atoms are allowed when defining a struct.

[sorentwo]

There is an option in Jason.decode/2 that handles this automatically, keys: :atoms. You can also use existing_atoms to be safer, IIRC.

[tlvenn]

Ha good to known @sorentwo, I will leverage this then

[edgurgel]

Nice! I completely forgot we had to do this 👍

[tlvenn]

One thing to realise is by leveraging Jason to do the conversion, it happens to all keys present in the payload. So if a external system enqueue a job with a map in the args which uses string keys, the worker will receive the map with atom keys.

Jobs enqueued by Verk are not affected by this change because we wrap the args and decode them separately but in does present the reverse scenario. If a job is enqueued by Verk and has a map with atom keys which is expected in Elixir, the worker will receive a map with string keys.

We should get our story straight and adopt one strategy, atom keys or string keys and document it.
Being in Elixir land, I would favor the atom keys stategy.

@edgurgel what do you think ?

[tlvenn]

I pushed the change toward adopting this strategy which imho remove another user surprise but this is definitely a breaking change for whoever was pushing maps to job args and had adapted their workers to expect map with strings keys already.

The migration if any should be trivial though.

[edgurgel]

Ok so the changes are:

If you had maps inside your arguments they will come with atoms?

The only reason why I don't like this change is that we will create atoms for any keys and possibly user input data. So we should avoid creating atoms as they are not garbage collected.

We should probably use the previous code you wrote (but with String.to_existing_atom as mentioned by @sorentwo) and just for the first level of the keys (not including the keys that may exist inside args).

This way we avoid new atoms being created indefinitely and we won't have breaking changes?

What do you think?

[tlvenn]

Fair enough but we should definitely document that if you pass map with atom keys as argument to Verk, the atom keys will not be preserved and your worker will have to deal with string keys.

Personally I would prefer to deal with atom keys from end to end and I know that in my case I dont open an attack vector as I never push a map where keys are given by the users.

I am wondering if we should not make this a config option and by default be on the safe side of things.

What do you think ?

[tlvenn]

Something like: config :verk, args_keys: :atoms which would default to :strings if not defined.

[tlvenn]

Pushed the changes so that:

• Only the first level keys are converted to known atoms when we decode the job
• By default, args are decoded using string keys but this behaviour can be changed using config :verk, args_keys: :atoms

[edgurgel]

@tlvenn, perfect. We got no breaking changes + a new feature and a bug fix. That's a win :)

[edgurgel]

The only thing that I'm missing are tests for encode!/1.

[tlvenn]

Yep I will add those and document the new option as well shortly

[tlvenn]

Should be good to go now @edgurgel !

[edgurgel]

@tlvenn, that's awesome! I will review and merge later today 👌 We should get a new release with this fix soon.

Thank you so much for fixing this issue once and for all 👍