json_is_meta_key does not exist on v4

[nickdnk]

Hello

This is not really a bug per se, but I don't know where else to ask. I upgraded Get5 to run on v4, and it won't compile because json_is_meta_key no longer exists. The changelog does not explain what this feature was replaced with, nor do I know what it actually did.

Can you perhaps explain if we can just ignore this all going forward?

[nickdnk]

Specifically, I'm asking if this is okay to just remove. I would assume this is somehow handled automatically now since the method is removed?

https://github.com/splewis/get5/pull/736/commits/37be4e9a351912299b9e562853511f76e9eaa864#diff-39ac21dd98f1be4861b5599e9534f3b42559f76bf60f9bf76b2159654b82de85L130-L134

Edit: Removing it breaks get5, so that's definitely not the way to go. Any help would be appreciated.

[clugg]

Hey @nickdnk, Looking back over the v4 release notes I don't think I was clear enough about this, sorry!

Prior to v4 we used a single StringMap to store both the data itself and related metadata. For example, not only did we store key = value but also other information, such as the type stored at key - to do this, we simply place a second value using key:type = string. This is what a "meta key" is - keys in the StringMap that are not directly set by the user but contain supporting information (specifically, all keys have type, strings have length as well, and hidden for any keys marked hidden). Since the snapshot would give us these keys back alongside our 'normal' keys, we needed a method to skip over them (json_is_meta_key). This implementation was not foolproof - for example, what happens if we actually want to store something ourselves under a key called key:type? There would be conflicts between what is actual data and what is metadata.

With v4 I created a nested StringMap which contains actual data while the parent StringMap contains all metadata. This prevents any key clashes, etc, and also makes iterating easier - now we only need to get a snapshot of the child data StringMap and naturally it will only contain/give us the keys we actually care about (no metadata keys). Alongside this I made some improvements to the API for iteration in general.

Explanation aside, you can remove uses of json_is_meta_key but you also need to use the new method for iterating over objects. If you check out the README diff from v3.x to v4 you will notice that we no longer create our own snapshot - we simply call Iterate on the object (which gets a fresh snapshot behind the scenes and returns its length) and then, within the for loop, call GetKey directly on the object.

Looking at the get5 example in question, this code would become something similar to:

int length = data.Iterate();
int key_length = 0;
char name[MAX_NAME_LENGTH];
for (int i = 0; i < length; i++) {
  key_length = data.GetKeySize(i);
  char[] key = new char[key_length];
  data.GetKey(i, key, key_length); 

  data.GetString(key, name, sizeof(name));
  char steam64[AUTH_LENGTH];
  if (ConvertAuthToSteam64(key, steam64)) {
    Get5_SetPlayerName(steam64, name);
    list.PushString(steam64);
    count++;
  }
}

Hopefully this all makes sense. Let me know if you have any further questions!

[nickdnk]

Alright. I will try and implement this and get back to you if I can't make it work. I would suggest that you add parts of this explanation to the changelog so people don't get confused about this part. Iterate() nor json_is_meta_key are anywhere to be found in there.

[nickdnk]

@clugg Okay, so I think there's a problem with the cleanup function on 4.1.1.

I get this stack trace now:

L 07/11/2022 - 00:55:02: [SM] Exception reported: Invalid Handle 30a002e0 (error 3)
L 07/11/2022 - 00:55:02: [SM] Blaming: get5.smx
L 07/11/2022 - 00:55:02: [SM] Call stack trace:
L 07/11/2022 - 00:55:02: [SM]   [0] StringMap.GetValue
L 07/11/2022 - 00:55:02: [SM]   [1] Line 98, /get5/addons/sourcemod/scripting/include/json/helpers/typedstringmap.inc::TypedStringMap.GetOptionalValue
L 07/11/2022 - 00:55:02: [SM]   [2] Line 136, /get5/addons/sourcemod/scripting/include/json/helpers/typedstringmap.inc::TypedStringMap.GetBool
L 07/11/2022 - 00:55:02: [SM]   [3] Line 77, /get5/addons/sourcemod/scripting/include/json/object.inc::JSON_Object.OrderedKeys.get
L 07/11/2022 - 00:55:02: [SM]   [4] Line 109, /get5/addons/sourcemod/scripting/include/json/object.inc::JSON_Object.Iterate
L 07/11/2022 - 00:55:02: [SM]   [5] Line 783, /get5/addons/sourcemod/scripting/include/json.inc::json_cleanup
L 07/11/2022 - 00:55:02: [SM]   [6] Line 798, /get5/addons/sourcemod/scripting/include/json.inc::json_cleanup
L 07/11/2022 - 00:55:02: [SM]   [7] Line 813, /get5/addons/sourcemod/scripting/include/json.inc::json_cleanup_and_delete
L 07/11/2022 - 00:55:02: [SM]   [8] Line 181, ./scripting/get5/matchconfig.sp::LoadMatchFile
L 07/11/2022 - 00:55:02: [SM]   [9] Line 57, ./scripting/get5/matchconfig.sp::LoadMatchConfig
L 07/11/2022 - 00:55:02: [SM]   [10] Line 895, ./scripting/get5.sp::CheckAutoLoadConfig
L 07/11/2022 - 00:55:02: [SM]   [11] Line 773, ./scripting/get5.sp::OnConfigsExecuted

The code that crashes looks like this, and as far as I can tell, LoadMatchFromJson does not delete any of the handles passed to it. It just reads the data. If I take out the json_cleanup_and_delete(json); line, the plugin works fine and everything loads as it should, but obviously the JSON_Object then leaks.

JSON_Object json = json_read_from_file(configFile);
if (json != null && LoadMatchFromJson(json)) {
  json_cleanup_and_delete(json); // <- This crashes
  Get5_MessageToAll("%t", "MatchConfigLoadedInfoMessage");
} else {
  MatchConfigFail("invalid match json");
  return false;
}

The JSON we read looks like this (sanitized for privacy, but so you can see the structure):

{
  "num_maps": 1,
  "players_per_team": 5,
  "min_players_to_ready": 1,
  "min_spectators_to_ready": 0,
  "skip_veto": true,
  "side_type": "always_knife",
  "maplist": [
    "de_train"
  ],
  "team1": {
    "name": "Team A",
    "tag": "",
    "flag": "DK",
    "players": {
      "76561197996426751": "", // These are steamid => name, where empty string means the player names are not locked
      "76561197996426752": "",
      "76561197996426753": "",
      "76561197996426754": "",
      "76561197996426755": ""
    }
  },
  "team2": {
    "name": "Team B",
    "tag": "",
    "flag": "DK",
    "players": {
      "76561197996426761": "",
      "76561197996426762": "",
      "76561197996426763": "",
      "76561197996426764": "",
      "76561197996426765": ""
    }
  },
  "cvars": {
    "hostname": "Scrim | sub.domain.tld"
  }
}

The two blocks of code I migrated from json_is_meta_key to Iterate() look like this:

JSON_Object cvars = json.GetObject("cvars"); // <- This is the cvar part of the above JSON.
if (cvars != null) {
  char cvarValue[MAX_CVAR_LENGTH];

  int length = cvars.Iterate();
  int key_length = 0;
  for (int i = 0; i < length; i++) {
    key_length = cvars.GetKeySize(i);
    char[] cvarName = new char[key_length];
    cvars.GetKey(i, cvarName, key_length);

    cvars.GetString(cvarName, cvarValue, sizeof(cvarValue));
    g_CvarNames.PushString(cvarName);
    g_CvarValues.PushString(cvarValue);
  }
}

and

JSON_Object data = json.GetObject(key); // <- Returns a `players` object from above JSON.
int length = data.Iterate();
int key_length = 0;
char name[MAX_NAME_LENGTH];
for (int i = 0; i < length; i++) {
  key_length = data.GetKeySize(i);
  char[] k = new char[key_length];
  data.GetKey(i, k, key_length);

  data.GetString(k, name, sizeof(name));
  char steam64[AUTH_LENGTH];
  if (ConvertAuthToSteam64(k, steam64)) {
    Get5_SetPlayerName(steam64, name);
    list.PushString(steam64);
    count++;
  }
}

Before I start debugging more on this I'd like your input, as I'm not sure this is Get5's fault, as I didn't really change anything except what you told me change.

Help much appreciated. Let me know if you need more info.

Edit: I should mention that in other places, where I don't call Iterate() on any objects, they seem to clean up fine, so maybe it has something to do with that.

[nickdnk]

I figured it out. There was a function hidden in there that called Cleanup on an array fetched from the JSON object: https://github.com/splewis/get5/commit/aa45fc6eb4e30b87679a327e362f0636a628e2aa#diff-39ac21dd98f1be4861b5599e9534f3b42559f76bf60f9bf76b2159654b82de85L90

After I removed that, everything is great. Why this didn't break the previous version I don't know, but either way this was wrong.

Thanks for the help anyway!

[clugg]

I've updated the v4 release notes to better detail the new iteration methods/the removal of json_is_meta_key, thanks for the suggestion! Glad to hear you've worked it out (but certainly strange that this behaviour didn't cause issues using v3), feel free to reach out if you have any other questions. Thanks again! :)

[nickdnk]

Cool.

I think it might be because of this change in the v4 logs:

• json_cleanup now also deletes the object passed in

-- as it was Cleanup() that was called in a nested array.