Problem with JSON encoding in lua

[kev82]

I only found out miniredis existed this morning and just wanted to say I think it is fantastic! However, I have hit a problem with json encoding in lua scripts. I have narrowed it down to the following example:

return cjson.encode({[1]=1, [2]=4, [3]=9, [-1]=1})

In redis 6.2.1 this ends up being encoded as a dictionary with the keys as strings, ie

Original 8 bit image from Plaguemon by hikikomori. Redis ver. 6.2.1
redis /shared/redis.sock> eval "return cjson.encode({[1]=1, [2]=4, [3]=9, [-1]=1})" 0
"{\"1\":1,\"2\":4,\"-1\":1,\"3\":9}"
redis /shared/redis.sock>

However, it seems to result in nil using miniredis v2.15.1.

I think I have managed to trace this to here: https://github.com/alicebob/gopher-json/blob/master/json.go#L111 where the code looks to be assuming that the presence of a number being returned as a table index means it is an array.

Hopefully I can fathom how to get the replace directive working in my go.mod and should be able to share an improvement to the existing code to at least fix this case if not fix it properly

[kev82]

May not actually fix it now, just realised it was easier to monkey patch my lua code as follows:

local cjson, cmsgpack = rawget(_G, [[cjson]]), rawget(_G, [[cmsgpack]])
if cmsgpack == nil then
  local deepcopystr
  function deepcopystr(x)
    if type(x) ~= [[table]] then return x end
    local numelems, transform = 0, function(z) return z end
    for k in pairs(x) do numelems = numelems + 1 end
    if numelems ~= #x then transform = tostring end
    local rv = {}
    for k, v in pairs(x) do
      rv[transform(k)] = deepcopystr(v)
    end
    return rv
  end
  local ojse = cjson.encode
  cjson.encode = function(x) return ojse(deepcopystr(x)) end
  -- ********** WARNING, DO NOT USE BELOW UNLESS SURE **********
  cmsgpack = {
    pack = cjson.encode,
    unpack = cjson.decode
  }
end

[alicebob]

Thank you for the issue and the potential workaround! I'll have a look soonish, but it might take a bit thanks to summer and such.

[alicebob]

I had a look, and you're right. But I don't see an easy enough way to fix this, without rewriting the whole encoding in gopher-json.

JSON can only have strings as keys, so another workaround would be not to ask it to encode ints in the first place:

127.0.0.1:6379> eval "return cjson.encode({[\"1\"]=1, [\"2\"]=4, [\"3\"]=9, [\"-1\"]=1})" 0
"{\"1\":1,\"-1\":1,\"3\":9,\"2\":4}"

[kev82]

Thanks for having a look, performance aside, my monkey patching of the cjson.encode function in the lua scripts themselves is working ok, so I'm good.

However, I have tried to study the code a bit, and it looks as though the implementation of the LTNumber case (https://github.com/alicebob/gopher-json/blob/master/json.go#L111) assumes that LTable.Next() will iterate an array in order. It looks very broken if this is not the case - so rather than switching on the key type, maybe we can switch on the key being the number 1, eg (not tested)

type TableClassification int

const (
  EmptyArray TableClassification = iota
  Array
  NotArray
)

func ClassifyTable(t *lua.LTable) TableClassification {
  if t == nil { return NotArray }
  k, _ := t.Next(lua.LNil)
  if k.Type() == lua.LTNil { return EmptyArray }
  if k.Type() != lua.LTNumber { return NotArray }
  if k.(LTNumber) != 1.0 { return NotArray }
  return Array
}

The the switch is done on the result of ClassifyTable(converted), and the case for NotArray (currently LTString) is extended to convert any numeric keys it finds to strings.

[alicebob]

Thanks for the extra effort. I don't think I'll dive into this. I'm happy to merge any PRs, esp if they are against the repo I forked (it had to be a fork to deal with some minor redis specific behavior).