search

watermarkchurch / wcc-contentful

An alternative to Contentful's contentful.rb ruby client, contentful_model, and contentful_rails gems all in one.
MIT License
2 stars 1 forks source link

Fix Delayed Sync Job when an JSON parse error is thrown when fetching the sync:token #63

Closed reidcooper closed 6 years ago

reidcooper commented 6 years ago

Issue: https://github.com/watermarkchurch/wcc-contentful/issues/62

reidcooper commented 6 years ago

@gburgett Yeah, it was happening with the postgres store. It couldn't parse the JSON from Postgres. However, it should still work for a cache store, since it will retrieve a hash object.

gburgett commented 6 years ago

I think this is revealing that the postgres store doesn't quite conform to the same interface. So I guess we gotta decide if it's a part of the store interface to allow non-json objects, or not.

I'm divided on this. If the store can handle non-json objects, that would allow more flexibility in these kind of cases. However the primary purpose of the store is to store actual Contentful objects and scan over them, and it could make things easier if we ensured everything was JSON.

I think if we go the 'everything has to be JSON' route, then we need to test that inserted values are hashes, and throw an argument error in #index and #set when the value is not a hash

reidcooper commented 6 years ago

@gburgett But if all stores conform to storing their info as json, wouldn't the sync token as well need to be stored as JSON?

gburgett commented 6 years ago

Correct. And that's exactly what you're doing here by passing the value in as a hash.

reidcooper commented 6 years ago

@gburgett I started a WIP of a conversion to JSON enforcement, take a look. Recent commit.

gburgett commented 6 years ago

@reidcooper Thanks for the work! That looks like a good direction to take. Would you also update the DelayedSyncJob to store the sync token as a hash? And we ought to add a spec to the store examples to ensure that the sync token format is handled correctly.

reidcooper commented 6 years ago

@gburgett I thought I addressed the job storing the value as a hash? I think I also added that to the stores, but I think I am missing specs for the stores.

reidcooper commented 6 years ago

@gburgett Updated PR, please take a look

gburgett commented 6 years ago

yep you did, thanks!