feat: another attempt at cjson.as_array metatable

[thibaultcha]

Hey there!

I would really like to see this being pushed forward, so here is my attempt at convincing of the use of such a patch :) Criticism welcome of course. I mean that I will gladly make the necessary changes if something is not appropriate.

First, I would like to point out that the lack of empty table distinction from mpx/lua-cjson has lead to several (if not many) patches trying to solve this. And that I am glad to see this fork already includes the encode_empty_table_as_object, but I would like to emphasize on the fact that this feature is not enough even in fairly common use cases.

As an example, let's consider someone using the log_by_lua_* phase of ngx_lua (maybe along with lua-resty-logger-socket) to serialize the request/response data and send it somewhere to be stored and analyzed later. Such data could have empty objects representing either arrays or objects in the same Lua table (request headers vs form parameters). This is a simple use case where a higher level of granularity is necessary.

I had to deal with this a few month ago, and the aggregating tool enforced some schema validation, which made it impossible to work around without implementing some very ugly hacks, for which I still do not sleep well at night :)

---

I read the discussion at #1, and while I too believe a lightuserdata would be more efficient and consistent with cjson.null, I also think it would offload the actual work to the user.

While the metatable approach is fairly straightforward:

-- arr and obj might be empty, but never nil
function serialize(arr, obj)
  setmetatable(arr, cjson.as_array)

  local data = {
    foo = arr,
    bar = obj
  }

  return cjson.encode(data)
end

The lightuserdata is less:

function serialize(arr, obj)
  if #arr < 1 then
    arr = cjson.empty_array
  end

  local data = {
    foo = arr,
    bar = obj
  }

  return cjson.encode(data)
end

Note: if you see a better way of dealing with the userdata approach for users, I would be glad to hear about it and update the patch eventually!

---

Hence, this patch proposes a slightly improved version of #1, taking the given suggestions into account (since the patch was not updated since then):

• more specific name for the metatable key in the Lua registry.
• tests cases moved to t/agentzh.t, where cases for
  'encode_empty_table_as_object' are already written. It seems like a
  better place for tests specific to the OpenResty fork's additions.
• a more complex test case and slightly more concise changes.

Given the number of times such a feature was requested, I believe it would be really great if it could finally be included, if not in mpx/lua-cjson, at least in OpenResty :)

[agentzh]

Shall we avoid using luaL_getmetatable because it requires (attempts of) creating a new Lua string object?

[agentzh]

@thibaultcha Maybe we could just store the metatable in our own cjson module table with a lightuserdata key to avoid polluting the global Lua registry.

[thibaultcha]

I am sorry but I really don't know what you mean here.

Maybe we could just store the metatable in our own cjson module table

I can see this. But then how does one retrieve it for comparison in json_append_data? Do you mean to store it as an upvalue like the configuration? Either as an upvalue or in the registry is the only way I see this doable.

with a lightuserdata key

I can see some syntactic sugar here and I can implement a cjson.empty_array lightuserdata just like cjson.null, but I don't see how storing the metatable with a lightuserdata key allows one to retrieve the said metatable for comparison in json_append_data either. Except as an upvalue/in registry again, but then the lightuserdata key seems irrelevant?

to avoid polluting the global Lua registry

One could argue the Lua registry is slow and propitious to naming conflicts, but isn't it made for that exact purpose of storing Lua values needed by C modules? In practice, naming conflicts are not so much of a thing (and the name is now prefixed), and however slow the registry is, using it will still be way more efficient than performing the hacks currently necessary to encore empty arrays anyways.

Am I missing something here?

[agentzh]

@thibaultcha Now you are using a string key, "CJSON_AS_ARRAY_MT", to index the metatable in the registry table, which is slow since you need to intern that C string upon every invocation of luaL_getmetatable. I am suggesting the following sequence:

lua_pushlightuserdata(L, &some_global_c_stub_variable_in_cjson);
lua_rawget(L, LUA_REGISTRYINDEX);

By using the cjson table, I mean just store the metatable into the cjson module table returned by require "cjson". It might be tricky in a traditional Lua C module as far as I can see. Thus I'm also fine with using the registry table but with a lightuserdata key as demonstrated above.

[thibaultcha]

Now that makes sense to me indeed, my understanding of your comment was to store the metatable in the cjson module with a lightuserdata key, hence the confusion, because as you are saying, that would be quite tricky to retrieve for the comparison (except as an upvalue).

[thibaultcha]

I have updated the patch taking your comment in consideration, thanks for clarifying! I also attached the lightuserdata cjson.empty_array to cjson, which allows for an alternative to the metatable approach, in case one finds it more convenient in some cases.

Let me know! :)

[agentzh]

We can avoid the array value "json_empty_array" here to save some memory. All we need now is just an address.

[thibaultcha]

Er yeah, it's how I first implemented it, should have left it as is.

[agentzh]

@thibaultcha One last thing: will you please drop a quick note to README.md (I'm aware that this file does not even exist atm)? This way people can know how to use things without checking out source or PRs. Thanks for your time!

[thibaultcha]

Absolutely, I actually intended to ask you if we wanted to document this and where once the patch confirmed!

[thibaultcha]

I just added a README following more or less the OpenResty layout, let me know what you think of it.

[agentzh]

@thibaultcha Wow. The README looks awesome. Thanks for your contribution! I'll try testing your patches on my side and merging the stuff tonight.

[thibaultcha]

Glad you like it! I just added a test case to ensure non-empty Lua tables with the empty_array_mt metatable are not encoded as empty arrays.

[thibaultcha]

Hey @agentzh, did you get any chance to give this a try? I just feel like a release is coming and I think it'd be great to include those new features! I could be wrong though :)

[agentzh]

@thibaultcha Been busy preparing tomorrow's openresty meetup. Sorry for the delay on my side. Will look into this as soon as I can manage.

[thibaultcha]

Yeah I bet. Excited! Ok :)

[doujiang24]

@thibaultcha Very Nice:)

[agentzh]

@thibaultcha Merged with a minor file name extension change. Thanks!

[thibaultcha]

Great day for OpenResty! 😁 Thank you!