WillB97 / srcomp-pystream

SRComp HTTP EventStream and WebSocket module
MIT License
0 stars 1 forks source link

Should remove empty values from state cache #7

Open PeterJCLaw opened 1 year ago

PeterJCLaw commented 1 year ago

Currently the update_THING handlers here have a general pattern of ignoring new values if they are None. This will miss cases where some value was previously present but is not present any more. The main case I can think of that this might happen is the tiebreaker, which could be in the schedule and then removed, though I don't think the stream should make assumptions about whether any of the values can be empty.

Specifically, the pattern is along the lines of:

        async with self.checked_response('/tiebreaker', silent_404=True) as data:
            if data is None or (new_tiebreaker := data.get('tiebreaker')) is None:
                return []

            if self.tiebreaker != new_tiebreaker:
                self.tiebreaker = new_tiebreaker
                return [{'event': 'tiebreaker', 'data': new_tiebreaker}]

where it should probably be:

        async with self.checked_response('/tiebreaker', silent_404=True) as data:
            new_tiebreaker = data.get('tiebreaker') if data else None

            if self.tiebreaker != new_tiebreaker:
                self.tiebreaker = new_tiebreaker
                if new_tiebreaker:
                    return [{'event': 'tiebreaker', 'data': new_tiebreaker}]

            return []

I'm not sufficiently familiar with the error handling semantics to know if this is quite right either, though I think general semantics we want are:

I realise that the tiebreaker case is complicated slightly by returning a 404 for the empty data case, though I'm guessing that's what the silent_404 parameter is aimed at helping.

PeterJCLaw commented 1 year ago

Looking now at the way that https://github.com/PeterJCLaw/srcomp-stream/ handles the tiebreaker I'm not sure that it gets it complete right either (nor even what the best thing to do for events is for the case of something being removed, see also https://github.com/PeterJCLaw/srcomp-screens/issues/5), however the general pattern described here is used throughout and so could impact other event types too.

PeterJCLaw commented 1 year ago

Another confounding factor here I realise is that the code is very defensive -- trying to make little assumption about what the SRComp HTTP API returns, so this might be more about clarifying cases where that's being done for general defensiveness rather than because the keys might reasonably be not present.

If there are cases where sometimes keys are missing and others they're null that's probably worth addressing by making the HTTP API more consistent. (I thiink it tries to aim to omit keys rather than let them be null, but I'm not completely sure)

WillB97 commented 1 year ago

In #10 I've added this logic for /matches, /knockout, /tiebreaker, /state. I've left /teams with the current logic because a null value there breaks the rest of the function's logic. Similarly the keys under /current return empty lists instead of None.