search

5apps / liquor-cabinet

remoteStorage HTTP API, based on Sinatra
MIT License
40 stars 6 forks source link

WIP: Fix 500 when last-modified missing in Redis #136

Closed raucao closed 4 years ago

raucao commented 4 years ago

I have trouble running the tests on my machine, so this is just an untested idea for a quick fix for now.

fixes #135

gregkare commented 4 years ago

The change looks good to me, I'm going to write a spec to go with it, because right now the specs are still green with this change

gregkare commented 4 years ago

I have added specs in 21a5700

raucao commented 4 years ago

So you think it should be a normal case that last-modified is missing? I"m not so sure about that.

Maybe it's for old files, before we stored it?

gregkare commented 4 years ago

It's definitely not a normal case, I have checked the code. In the new spec I have manually deleted the m key from Redis after a successful request.

You're right, it must be for files that were created before we started storing the date from the S3 response as Last-Modified on PUTs

gregkare commented 4 years ago

Actually, I can't find a point in time where liquor-cabinet did not store a Last-Modified date. Before d05cd4a it would do a head request on the newly created file, and since that it's using the Date header from the PUT request's response

Almost two years ago we added the Last-Modified to the listings in 60c508f, but the values were always stored in Redis

raucao commented 4 years ago

So then it's actually not expected that it would ever be empty. Meaning at least the staging data is corrupt. In which case specs for it wouldn't really be justified in my opinion, as they describe expected behavior.

I think it should at least log a warning, but also keep logging an exception to Sentry, even if it would deliver listings without last-modified properties on some objects.

gregkare commented 4 years ago

I found a file on staging that has a Last-Modified stored in Redis that looks like this: "2018-04-11 15:09:02 +0000". When turned into an integer by the tonumber lua function it turns into nil. I'm now figuring out how this has been saved as a string instead of a timestamp.

raucao commented 4 years ago

Closing this one, because it is a valid exception that should raise in the future in case someone's data is inconsistent. We fixed the wrong entry in Redis.