Get undeleted state deltas

[AndrewFerr]

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Code style is correct
  (run the linters)

[MadLittleMods]

Suggested change

	) -> Tuple[int, List]:
	) -> Tuple[int, Union[List[StateDelta], List[StateDeltaWithEventId]]]:

[AndrewFerr]

This is unfortunately not as simple as it seems. Mypy considers that type as incorrect because it treats this function's actual return type not as a union of two different list types, but as a list of unions (List[Union[StateDelta, StateDeltaWithEventId]]). Using that as the return type satisfies that error, but triggers a new error on mismatched types between the helper function & the containing function.

I attempted using generics to bypass this, but to no avail. For now the best I could think of was to use an unspecified List, but I'm open to using something more specific than that.

[MadLittleMods]

It's good to have this context for this decision to look back on in any case ⏩

[MadLittleMods]

What's the context for these changes? Why were they made? What was it affecting? How did we notice? What scenarios does this happen in? Is this prerequisite work for something else?

Do we need to apply this to other functions that read from current_state_delta_stream?

(update the PR description as well)

[AndrewFerr]

The motivation was from another PR that wanted only eventID-associated deltas, and a recommendation was made to move that filtering into the getter instead: #17810 (comment)

[AndrewFerr]

Do we need to apply this to other functions that read from current_state_delta_stream?

No, because this doesn't change the existing usage of the getters that don't filter out deltas based on eventID.

[MadLittleMods]

(update the PR description with all of this context)

My question was more like if we need it it here, why don't we only take into account eventID-associated deltas in the other places. But it's actually just a very specific use case that doesn't care about deltas where state gets deleted. At the very least, this needs some more context to the docstring on why/when you would use the exclude_deleted option.

I'm worried that we might be introducing easy foot-gun if we add this option. exclude_deleted kinda sounds like it's excluding data that doesn't matter anymore. And it gives a "better" looking type where you don't have to worry about an optional event_id.

And if you make these assumptions and try to use it; for example, if I grab the deltas between two tokens and then resolve it against a StateMap, If I accidentally use the exclude_deleted option, I would be missing the deltas where the state became unset and should be unset in the StateMap.

The use case might be better served with how it's being addressed now 🤷

[AndrewFerr]

In an out-of-band discussion about this PR, it was seen as risky to have this option, as it would suggest that ignoring deltas for deleted state is a generally-applicable thing to do -- but there are many times when callers should be aware of what state was deleted. Thus, if a caller really doesn't care about deleted state deltas, it should just filter them out itself.