Improve migration to eosIDs #354

Ulibos · 2024-04-15T00:07:54Z

Summary

This PR completes migration to eosIDs for the tool (except for a couple plugins, more on that below).

Dependencies

This PR depends on #352 and lays foundation for #347

Changes

There are 5 commits:

Cleanup player connection log parsing to ease transition and get rid of clutter.
Rework and extend admin-related methods to make them "omnivore" to different IDs. *
Migrate core code to eosIDs.
Migrate plugins to eosIDs. *
Fix minor issue with uninitialized oldPlayerInfo on player list updates (was annoying me a bit during testing).

* - caveats

Testing

All of this was tested several times with event capture to make sure data is consistent. In summary (but not in 1 last go) I have tested:

connecting, disconnecting, possess, unpossess, create squad, kill (self inflicted), warn, force swap, kick, ban.

Only thing I didn't test for sure is a teamkill. I also am not aware if I had at least 1 run where I didn't have a steamID, most likely not. But so far generalized parsers work correctly, I haven't seen any errors in logs or missing data. I have also tested: getAdminPermsByAnyID(), getAdminsWithPermission() (both were tested for server.admins with steamID or eosID), getPlayerByAnyID() for both types of IDs, rcon methods (warn(), switchTeam(), kick(), ban()).

Caveats

Testing Plugins

I didn't test plugins. We don't use any default ones and we don't have the necessary setup, plus the ones I've edited have mostly trivial fixes. The one I've fixed only partially is discord-killfeed, I didn't touch it's CBL integration. The ones I didn't touch at all are: db-log, awn-api, cbl-info. Some of these are completely unknown to me, some are too dependent on steamID to fix them in this PR. For now they will work as they were before as long as players have steamIDs.

Admins IDs

For now, there is a potential issue with an admin having 2 sets of permissions, one for eosID and another for steamID. Current code won't be able to merge it (at the time of merging it doesn't have persistent mapping of player's IDs). Easily avoidable though by not intermixing two ID formats.
Another side-effect is that getAdminsWithPermission() will always return only those admins that have the permission AND are online (for at least 10 seconds). This happens because this method matches all existing IDs against all existing admin entries to convert them all to eosIDs.

Since last update all data necessary to identify a player comes in 2 logs within the same chain, so many previous loggers are deprecated. `JOIN_SUCCEEDED` is now used as an emit trigger because this is where we obtain the last piece of data for the log - `playerSuffix`.

As an added side effect, methods like `server.getAdminPermsBySteamID()` will only return data for admins that are currently in-game (because to map anyID onto eosID it needs player's data which is not persistent and is available only when a player is online).

Plugins that were not updated: 1. db-log SteamID is basically it's backbone. Migrading to EOS would require obtaining all steam -> EOS pairs, migrating databasesm etc. Maybe we could make a db-logV2 with new data and migration assisting tools so that anyone willing to migrate can do it in an assisted mode. 2. awn-api Not something that I use or know, left as is. 3. cbl-info Again, steamID seems to be it's backbone, I don't want to mess with a project that has maintainers that know ins and outs way better than me. Plugins that were partially updated: 1. discord-killfeed CBL integration is left untouched, other things are updated.

fantinodavide

Looking good overall, but some changes are required in my opinion.

fantinodavide · 2024-04-15T00:27:14Z

squad-server/index.js

@@ -353,16 +336,26 @@ export default class SquadServer extends EventEmitter {
    await this.logParser.watch();
  }

+  // DEPRECATED FUNCTION


can be replaced with a more suitable jsdoc

/** * @deprecated <optional message here> */

fantinodavide · 2024-04-15T00:44:12Z

core/id-parser.js

I personally don't like the approach of having a "unified" regex to match the IDs because it depends only on the hope of OWI not breaking things with typos or non-standard formats, which has already happened, an example is a log line which has Contoller instead of Controller.

All suggestions are implemented apart from one: changing getAdminsWithPermission(perm) to return players instead of IDs is not yet implemented. I'm all for implementing it, just want to make sure it is greenlit. Or, as a compromise, we could extend it to getAdminsWithPermission(perm, returnPlayers=false), that would allow for the new feature without potentially breaking custom plugins.
As for unified regex, I kindly disagree. master implementation is prone to: changing ordering, extra spacing, lack of steamID (some regexes in master were fixed to have steam as an optional but not all), renaming a key. This implementation will break only if delimiters are broken. Swapping, cloning, removing arbitrary k:v pairs, introducing new IDs, surrounding colons with spaces won't break it. Renaming a key will break things down the line but not the parser itself. And when OWI eventually breaks some output again - we can patch that specific item within it's parser, while everything else follows the same logic and is easy to remember.

I'm good with the new changes after my first review, and with keeping your implementation of the ID parser since you proved to have went through an analysis of all the side effects of both your implementation and the one currently on the master branch.

fantinodavide · 2024-04-15T00:48:46Z

squad-server/index.js

@@ -218,7 +218,7 @@ export default class SquadServer extends EventEmitter {
      data.player = await this.getPlayerByEOSID(data.eosID);
      if (data.player) data.player.suffix = data.playerSuffix;

-      delete data.steamID;
+      for (const k in data) if (k.endsWith('ID')) delete data[k];


this should be more specific:

for (const k in data) if (["steamID", "eosID"].includes(k)) delete data[k];

fantinodavide · 2024-04-15T00:49:13Z

squad-server/index.js

@@ -227,7 +227,7 @@ export default class SquadServer extends EventEmitter {
    this.logParser.on('PLAYER_DISCONNECTED', async (data) => {
      data.player = await this.getPlayerByEOSID(data.eosID);

-      delete data.steamID;
+      for (const k in data) if (k.endsWith('ID')) delete data[k];


this should be more specific:

for (const k in data) if (["steamID", "eosID"].includes(k)) delete data[k];

fantinodavide · 2024-04-15T00:56:10Z

squad-server/index.js

    }
-    return ret;
+    return anyIDsToPlayers(ret, this.players).map(player => player.eosID);


considering that this is a breaking change, and it also limits the functionality from getting ALL the admins to ONLY THE ONLINE admins, I would prefer this to be converted to returning an array of objects with both eosID and steamID optional parameters.
this should be further discussed.

As discussed on Discord with @Ulibos, the best choice appears to be a method that accepts an extra parameter to tune the output based on the needs.

A jsdoc would be a good fit for defining all the possible values for the extra parameter that I will just refer to as "output format parameter".

The output format parameter could be defined with the following jsdoc: @param {'steamID'|'eosID'|'any'|'player'} and will return the output formatted as showed in the following example:

steamID: The output stays the same as the current implementation. Example: [ '01234567891234567', '01234567891234567' ]

eosID: Example: [ 'a1b2c3d4e5f6a7b8c9d1e2f3a4b5c6d7e8f9', 'a1b2c3d4e5f6a7b8c9d1e2f3a4b5c6d7e8f9' ]

any: The eosID should be the preferred ID but include a fallback to steamID in case of missing eosID. Example [ 'a1b2c3d4e5f6a7b8c9d1e2f3a4b5c6d7e8f9', '01234567891234567' ]

player: [ { name: 'example_name', eosID: 'a1b2c3d4e5f6a7b8c9d1e2f3a4b5c6d7e8f9', steamID: '01234567891234567', ... }, { name: 'example_name', eosID: 'a1b2c3d4e5f6a7b8c9d1e2f3a4b5c6d7e8f9', steamID: null|undefined, ... } ]

fantinodavide · 2024-04-15T00:59:09Z

squad-server/index.js

@@ -591,6 +585,7 @@ export default class SquadServer extends EventEmitter {
    );
  }

+  // DEPRECATED


can be replaced with a more suitable jsdoc

/** * @deprecated <optional message here> */

fantinodavide · 2024-04-15T01:10:42Z

core/id-parser.js

+
+// main function intended for parsing `Online IDs:` body. Returns an
+// iterator that yields `{platform: id}` pairs.
+export const iterate = (idsStr) => {


this should be renamed to a more descriptive name such as iterateIDs

fantinodavide · 2024-04-15T01:26:41Z

Additionally, the "Fix/" prefix is not appropriate in my opinion, and I would change it to "refactor!: " (see https://www.conventionalcommits.org/en/v1.0.0/)

werewolfboy13 · 2024-04-15T02:30:44Z

Flags added as needed. Let me know when good to push.

Renamed main function, rewrote ID iterations, fixed a bug in `anyIDsToPlayers()` (wasn't eliminating duplicats in returned list), minor comments cleanup.

Ulibos · 2024-04-15T21:33:41Z

Status update here: #354 (comment)
Sorry for bad placement, my github kung-fu is still weak.

fantinodavide · 2024-05-15T19:08:00Z

core/id-parser.js

I'm good with the new changes after my first review, and with keeping your implementation of the ID parser since you proved to have went through an analysis of all the side effects of both your implementation and the one currently on the master branch.

1. `getAdminsWithPermission` now supports both legacy and new call conventions. 2. any-id.js is also refactored a bit (added `isPlayerID` function). 3. Minor JSDoc fixes.

fantinodavide · 2024-05-27T16:13:25Z

New changes look good overall. The error handling of the new getAdminsWithPermission is very clear and easy to understand.

fantinodavide

#354 (comment)

Ulibos · 2024-05-29T13:10:01Z

I have closed #352 because it is already integrated into this PR so no unsatisfied dependencies left. #347 can be merged in after this PR.

Parsers will skip lines with `INVALID` Online IDs.

Ulibos added 6 commits March 26, 2024 16:49

ID parser implemented.

f68f8f3

Migrate core code to eosID and anyID.

8a2849e

Fix attemts to access non-existent oldPlayerInfo.

b506205

Ulibos mentioned this pull request Apr 15, 2024

Robust ID parser. #352

Closed

fantinodavide suggested changes Apr 15, 2024

View reviewed changes

werewolfboy13 added core bug Bug related to the core SquadJS API minor Minor Change Testing Required This requires testing before approval. More Information Needed labels Apr 15, 2024

Refactor id-parser.

7b298a7

Renamed main function, rewrote ID iterations, fixed a bug in `anyIDsToPlayers()` (wasn't eliminating duplicats in returned list), minor comments cleanup.

fantinodavide approved these changes May 15, 2024

View reviewed changes

refactor: getAdminsWithPermission extended.

a78036b

1. `getAdminsWithPermission` now supports both legacy and new call conventions. 2. any-id.js is also refactored a bit (added `isPlayerID` function). 3. Minor JSDoc fixes.

fantinodavide approved these changes May 27, 2024

View reviewed changes

Ulibos added 2 commits June 19, 2024 01:57

Merge branch 'master' into fix/migrate-to-eosID

cbe29e5

Add safeguerds for INVALID IDs.

07be06f

Parsers will skip lines with `INVALID` Online IDs.

werewolfboy13 merged commit 078f722 into Team-Silver-Sphere:master Jul 3, 2024

werewolfboy13 removed More Information Needed Testing Required This requires testing before approval. labels Jul 3, 2024

lbzepoqo mentioned this pull request Jul 11, 2024

Remove duplicate "victim" friend from DiscordKillFeed plugin #364

Merged

Ulibos deleted the fix/migrate-to-eosID branch July 11, 2024 16:31

Thomas-Smyth changed the title ~~Fix/migrate to eos~~ Improve migration to eosIDs Aug 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve migration to eosIDs #354

Improve migration to eosIDs #354

Ulibos commented Apr 15, 2024

fantinodavide left a comment

fantinodavide Apr 15, 2024

fantinodavide Apr 15, 2024

Ulibos Apr 15, 2024

fantinodavide May 15, 2024

fantinodavide Apr 15, 2024

fantinodavide Apr 15, 2024

fantinodavide Apr 15, 2024

fantinodavide May 15, 2024

fantinodavide Apr 15, 2024

fantinodavide Apr 15, 2024

fantinodavide commented Apr 15, 2024

werewolfboy13 commented Apr 15, 2024

Ulibos commented Apr 15, 2024

fantinodavide May 15, 2024

fantinodavide commented May 27, 2024

fantinodavide left a comment

Ulibos commented May 29, 2024

Improve migration to eosIDs #354

Improve migration to eosIDs #354

Conversation

Ulibos commented Apr 15, 2024

Summary

Dependencies

Changes

Testing

Caveats

Testing Plugins

Admins IDs

fantinodavide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fantinodavide commented Apr 15, 2024

werewolfboy13 commented Apr 15, 2024

Ulibos commented Apr 15, 2024

Choose a reason for hiding this comment

fantinodavide commented May 27, 2024

fantinodavide left a comment

Choose a reason for hiding this comment

Ulibos commented May 29, 2024