clugg / sm-json

A pure SourcePawn JSON encoder/decoder.
GNU General Public License v3.0
82 stars 8 forks source link

Support copying objects with `null` keys + support deep array concat + fix wrong hidden key index when concat'ting arrays #33

Closed nickdnk closed 1 year ago

nickdnk commented 1 year ago

Hello again

It would seem the library does not like it when you attempt to copy objects with null keys in them. This happens:

JSON_Object json = new JSON_Object();
json.SetObject("null_key", null);
json.SetString("non_null_key", "test_value");
JSON_Object copy = json.DeepCopy();
L 03/21/2023 - 02:35:53: [SM] Exception reported: Invalid Handle 0 (error 4)
L 03/21/2023 - 02:35:53: [SM] Blaming: theplugin.smx
L 03/21/2023 - 02:35:53: [SM] Call stack trace:
L 03/21/2023 - 02:35:53: [SM]   [0] StringMap.GetValue
L 03/21/2023 - 02:35:53: [SM]   [1] Line 98, addons/sourcemod/scripting/include/json/helpers/typedstringmap.inc::TypedStringMap.GetOptionalValue
L 03/21/2023 - 02:35:53: [SM]   [2] Line 136, addons/sourcemod/scripting/include/json/helpers/typedstringmap.inc::TypedStringMap.GetBool
L 03/21/2023 - 02:35:53: [SM]   [3] Line 67, addons/sourcemod/scripting/include/json/object.inc::JSON_Object.IsArray.get
L 03/21/2023 - 02:35:53: [SM]   [4] Line 728, addons/sourcemod/scripting/include/json.inc::json_copy_shallow
L 03/21/2023 - 02:35:53: [SM]   [5] Line 754, addons/sourcemod/scripting/include/json.inc::json_copy_deep
L 03/21/2023 - 02:35:53: [SM]   [6] Line 770, addons/sourcemod/scripting/include/json.inc::json_copy_deep
L 03/21/2023 - 02:35:53: [SM]   [7] Line 758, addons/sourcemod/scripting/include/json/object.inc::JSON_Object.DeepCopy

This PR should fix that, as far as I can tell. Let me know if I misunderstood anything here. I'm not 100% sure this is the best approach for it, but at least it works.

I wrote out a test for it "by hand", but I didn't run/compile it, because I don't have the tools handy for it (I only ever compile Get5 with Docker), so you better give it a go.

nickdnk commented 1 year ago

This now also fixes the problem mentioned in https://github.com/clugg/sm-json/pull/35 about hidden key index being wrong on array concat and the minor doc error.

I also added support for (and tests) for deep concat'ing arrays. All tests in this PR (101 total) pass as of now and it is ready to be merged. This also makes https://github.com/clugg/sm-json/pull/35 easier to move forward with. I would recommend you don't release a new version until both PRs have been merged.

clugg commented 1 year ago

Hey mate, thanks for spotting these and thanks for your hard work. I'm not a fan of adding deep-copy support to Concat, I feel that it blurs the intention a little bit - you could just do Concat(target.DeepCopy()) for the same outcome. What do you think?

As for the other fixes, they are now on the develop branch as per https://github.com/clugg/sm-json/issues/34#issuecomment-1483774832.

clugg commented 1 year ago

Fixes included in v4.1.3, thanks again!