From e650308e3d0d6400581da070f0bbfe771ff0ac2e Mon Sep 17 00:00:00 2001 From: Athanasius Date: Thu, 20 May 2021 15:28:52 +0100 Subject: [PATCH 1/4] EngineerProgress: Extra validation paranoia This is an example of just how icky it gets validating an event by hand. Technically we need a function, many would probably be lengthier and more complex than this one, for **every single journal event type**. --- monitor.py | 75 +++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 63 insertions(+), 12 deletions(-) diff --git a/monitor.py b/monitor.py index 49028521..a32a6a61 100644 --- a/monitor.py +++ b/monitor.py @@ -762,20 +762,24 @@ class EDLogs(FileSystemEventHandler): # type: ignore # See below self.state[event_type] = payload elif event_type == 'EngineerProgress': - engineers = self.state['Engineers'] - if 'Engineers' in entry: # Startup summary - self.state['Engineers'] = { - e['Engineer']: ((e['Rank'], e.get('RankProgress', 0)) if 'Rank' in e else e['Progress']) - for e in entry['Engineers'] - } + # Sanity check - at least once the 'Engineer' (name) was missing from this in early + # Odyssey 4.0.0.100. Might only have been a server issue causing incomplete data. - else: # Promotion - engineer = entry['Engineer'] - if 'Rank' in entry: - engineers[engineer] = (entry['Rank'], entry.get('RankProgress', 0)) + if self.event_valid_engineerprogress(entry): + engineers = self.state['Engineers'] + if 'Engineers' in entry: # Startup summary + self.state['Engineers'] = { + e['Engineer']: ((e['Rank'], e.get('RankProgress', 0)) if 'Rank' in e else e['Progress']) + for e in entry['Engineers'] + } - else: - engineers[engineer] = entry['Progress'] + else: # Promotion + engineer = entry['Engineer'] + if 'Rank' in entry: + engineers[engineer] = (entry['Rank'], entry.get('RankProgress', 0)) + + else: + engineers[engineer] = entry['Progress'] elif event_type == 'Cargo' and entry.get('Vessel') == 'Ship': self.state['Cargo'] = defaultdict(int) @@ -1629,6 +1633,53 @@ class EDLogs(FileSystemEventHandler): # type: ignore # See below logger.debug(f'Invalid journal entry:\n{line!r}\n', exc_info=ex) return {'event': None} + # TODO: *This* will need refactoring and a proper validation infrastructure + # designed for this in the future. This is a bandaid for a known issue. + def event_valid_engineerprogress(self, entry) -> bool: # noqa: CCR001 + """ + Check an `EngineerProgress` Journal event for validity. + + :param entry: Journal event dict + :return: True if passes validation, else False. + """ + # The event should have at least one of thes + if 'Engineers' not in entry and 'Progress' not in entry: + logger.warning(f"EngineerProgress has neither 'Engineers' nor 'Progress': {entry=}") + return False + + # But not both of them + if 'Engineers' in entry and 'Progress' in entry: + logger.warning(f"EngineerProgress has BOTH 'Engineers' and 'Progress': {entry=}") + return False + + if 'Engineers' in entry: + # 'Engineers' version should have a list as value + if not isinstance(entry['Engineers'], list): + logger.warning(f"EngineerProgress 'Engineers' is not a list: {entry=}") + return False + + # It should have at least one entry? This might still be valid ? + if len(entry['Engineers']) < 1: + logger.warning(f"EngineerProgress 'Engineers' list is empty ?: {entry=}") + # TODO: As this might be valid, we might want to only log + return False + + # And that list should have all of these keys + for e in entry['Engineers']: + for f in ('Engineer', 'EngineerID', 'Rank', 'Progress', 'RankProgress'): + if f not in e: + logger.warning(f"Engineer entry without '{f=}' key: {e=}") + return False + + if 'Progress' in entry: + for e in entry['Engineers']: + for f in ('Engineer', 'EngineerID', 'Rank', 'Progress', 'RankProgress'): + if f not in e: + logger.warning(f"Engineer entry without '{f=}' key: {e=}") + return False + + return True + def suit_loadout_id_from_loadoutid(self, journal_loadoutid: int) -> int: """ Determine the CAPI-oriented numeric slot id for a Suit Loadout. From a82099fe531dbd289e58ff3c6de8823281790e4f Mon Sep 17 00:00:00 2001 From: Athanasius Date: Thu, 20 May 2021 15:38:06 +0100 Subject: [PATCH 2/4] EngineerProgress: Don't need `f=`, just `f` here. --- monitor.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/monitor.py b/monitor.py index a32a6a61..2b6b1f9e 100644 --- a/monitor.py +++ b/monitor.py @@ -1668,14 +1668,14 @@ class EDLogs(FileSystemEventHandler): # type: ignore # See below for e in entry['Engineers']: for f in ('Engineer', 'EngineerID', 'Rank', 'Progress', 'RankProgress'): if f not in e: - logger.warning(f"Engineer entry without '{f=}' key: {e=}") + logger.warning(f"Engineer entry without '{f}' key: {e=}") return False if 'Progress' in entry: for e in entry['Engineers']: for f in ('Engineer', 'EngineerID', 'Rank', 'Progress', 'RankProgress'): if f not in e: - logger.warning(f"Engineer entry without '{f=}' key: {e=}") + logger.warning(f"Engineer entry without '{f}' key: {e=}") return False return True From 2184afba9b1b27d0eb2a656279fcf7c616dd3256 Mon Sep 17 00:00:00 2001 From: Athanasius Date: Thu, 20 May 2021 15:51:28 +0100 Subject: [PATCH 3/4] EngineerProgress: Rank/RankProgress is complicated. And, yes, flake8 checks, validating these events *is* complex. --- monitor.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/monitor.py b/monitor.py index 2b6b1f9e..4c6bc957 100644 --- a/monitor.py +++ b/monitor.py @@ -1635,7 +1635,7 @@ class EDLogs(FileSystemEventHandler): # type: ignore # See below # TODO: *This* will need refactoring and a proper validation infrastructure # designed for this in the future. This is a bandaid for a known issue. - def event_valid_engineerprogress(self, entry) -> bool: # noqa: CCR001 + def event_valid_engineerprogress(self, entry) -> bool: # noqa: CCR001 C901 """ Check an `EngineerProgress` Journal event for validity. @@ -1668,14 +1668,20 @@ class EDLogs(FileSystemEventHandler): # type: ignore # See below for e in entry['Engineers']: for f in ('Engineer', 'EngineerID', 'Rank', 'Progress', 'RankProgress'): if f not in e: - logger.warning(f"Engineer entry without '{f}' key: {e=}") + # For some Progress there's no Rank/RankProgress yet + if f in ('Rank', 'RankProgress'): + if (progress := e.get('Progress', None)) is not None: + if progress in ('Invited', 'Known'): + continue + + logger.warning(f"Engineer entry without '{f}' key: {e=} in {entry=}") return False if 'Progress' in entry: for e in entry['Engineers']: for f in ('Engineer', 'EngineerID', 'Rank', 'Progress', 'RankProgress'): if f not in e: - logger.warning(f"Engineer entry without '{f}' key: {e=}") + logger.warning(f"Engineer entry without '{f}' key: {e=} in {entry=}") return False return True From 278a78c09a1a0b27b7f55450c1a0b075482941ff Mon Sep 17 00:00:00 2001 From: Athanasius Date: Thu, 20 May 2021 17:34:09 +0100 Subject: [PATCH 4/4] plugins/inara: Don't try to send None-system location I'm not sure normal use can cause this, but it's an easy obvious check. --- plugins/inara.py | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/plugins/inara.py b/plugins/inara.py index e3fe6a4f..5e08cf74 100644 --- a/plugins/inara.py +++ b/plugins/inara.py @@ -449,14 +449,17 @@ def journal_entry( # noqa: C901, CCR001 ) # Update location - new_add_event( - 'setCommanderTravelLocation', - entry['timestamp'], - OrderedDict([ - ('starsystemName', system), - ('stationName', station), # Can be None - ]) - ) + # Might not be available if this event is a 'StartUp' and we're replaying + # a log. + if system: + new_add_event( + 'setCommanderTravelLocation', + entry['timestamp'], + OrderedDict([ + ('starsystemName', system), + ('stationName', station), # Can be None + ]) + ) # Update ship if state['ShipID']: # Unknown if started in Fighter or SRV