Skip to content

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

Closed
nickdnk wants to merge 2 commits intoclugg:masterfrom
nickdnk:master
Closed

Support copying objects with null keys + support deep array concat + fix wrong hidden key index when concat'ting arrays#33
nickdnk wants to merge 2 commits intoclugg:masterfrom
nickdnk:master

Conversation

@nickdnk
Copy link
Contributor

@nickdnk nickdnk commented Mar 21, 2023 •
edited
Loading

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 added 2 commits March 21, 2023 04:19
@nickdnk
Support copying objects with null keys
d33ffd1
@nickdnk
Fix error in array concat hidden keys
3f12285 
Support deep concat arrays
Fix doc error
Fix test for null key copy
@nickdnk
Copy link
Contributor Author

nickdnk commented Mar 24, 2023 •
edited
Loading

This now also fixes the problem mentioned in #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 #35 easier to move forward with. I would recommend you don't release a new version until both PRs have been merged.

@nickdnk nickdnk mentioned this pull request Mar 24, 2023
Closed
@nickdnk nickdnk changed the title Support copying objects with null keys Support copying objects with null keys + support deep array concat + fix wrong hidden key index when concat'ting arrays Mar 24, 2023
nickdnk
nickdnk commented Mar 24, 2023
View reviewed changes
addons/sourcemod/scripting/json_test.sp
Test_AssertEqual("merged 3 type", arr1.GetType(3), JSON_Type_Bool);
Test_AssertEqual("merged 3", arr1.GetBool(3), false);
Test_AssertTrue("merged 1 hidden", arr1.GetHidden(1));
Test_AssertTrue("merged 3 hidden", arr1.GetHidden(3));
Copy link
Contributor Author

@nickdnk nickdnk Mar 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the arrays are now not of equal length, this assertion would fail if I hadn't also fixed the hidden key index bug.

@clugg
Copy link
Owner

clugg commented Mar 25, 2023

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 #34 (comment).

@clugg
Copy link
Owner

clugg commented Mar 27, 2023

Fixes included in v4.1.3, thanks again!

@clugg clugg closed this Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nickdnk @clugg