fanout / django-eventstream

Server-Sent Events for Django
MIT License
650 stars 85 forks source link

Fixed getting event #1 #8

Closed noamkush closed 6 years ago

noamkush commented 6 years ago

While writing a storage backend for a document database, I tested DjangoModelStorage to understand the expected behavior and I noticed a bug with get_events when trying to get event with id 1. After appending the first event, it's impossible to get it as calling get_events(channel, 0) raises an exception while get_events(channel, 1) returns an empty list. See example code:

from django_eventstream.storage import DjangoModelStorage
storage = DjangoModelStorage()
channel = 'my_channel'

data = {'a': 'b'}
s.append_event(channel, 'message', data)
s.get_current_id(channel) # returns 1
s.get_events(channel, 0) # raises an exception
s.get_events(channel, 1) # returns []

I added basic tests for the DjangoModelStorage class and implemented the fix that's required according to my understanding of the expected behavior.

jkarneges commented 6 years ago

Hi! Indeed you are correct that the expected behavior of calling get_events(channel, 0) is to return events with IDs > 0.

However, errors may go undetected in the proposed fix. If rows are trimmed (via e.g. trim_event_log), then get_events may return items that don't directly follow last_id. This is why the original code looks up the previous event and then ensures it matches the first event returned in the filter call. I'll think about this some more...

jkarneges commented 6 years ago

Fixed this in f70108b4f42381dc775badf96d9b569d04c6cd72. This should ensure queries against ID 0 will work, while ensuring events aren't silently missed due to time trimming. I confirmed with your tests. Can you update this PR to include only the tests, and then I'll merge?

noamkush commented 6 years ago

Okay, you can now merge

jkarneges commented 6 years ago

Great. Thank you. :)