-
Notifications
You must be signed in to change notification settings - Fork 35
Requires remote web requests even with full cache usage #11
Comments
Hi @zrisher Bit of a brain dump: You're right - caching isn't done right. My original plan was that by caching the time stamp you could derive a key (and therefore a cached view partial) without an API hit, but as you have found that's not how it works. :-( The correct solution is to be able to derive a Contentful model completely from the cache but unfortunately it's harder than it seems to generate one from a webhook in the first place, because the Contentful library (used by ContentfulModel and ContentfulRails) can't instantiate an entry (or asset or whatever) from JSON - only from a call to the API. Basically this is an unsolved problem. My suggested / planned solution is probably to subclass ContentfulModel::Base and include some way of marshalling the whole thing into the cache. Then you could hook into the instrumentation call raised when a web hook arrives, hit the api, and update it. At the moment there is a recursion issue in the underlying Contentful library which needs working around before the marshalling will work. Please feel free to suggest a PR - happy to provide commit access to this repo if you want to get involved. Ed
|
Hey Ed, Thanks for getting back to me so quickly. I'm at the tail end of the project I'm using Contentful for (thus the caching), but it's very likely we'll be using it again. Happy to submit PRs and work with you then, and yeah if you're busy commit access could help me move faster at that point. I can see there is a ton of complexity going on behind the scenes, so I'm hoping if we come back to this in 3 months some of it might be solved. 😄 I definitely think Contentful::Resource should be instantiable from JSON, and it seems like that would make things a lot simpler here. I also noticed a ton of I had assumed this and ContentfulModel were created by the Contentful team as well - if you're not affiliated (or really even if you are), huge props for getting this together and sharing it. |
Glad you're finding it useful. We aren't affiliated - we built it for some integrations we were doing, and we are Contentful partners off the back of it. We haven't got any Contentful projects at the moment, so you might get to the amends before us. Let me know if/when you want commit access. Ed
|
Hey @zrisher, Sorry for taking so long to check this out, but I find your feature request super interesting, and I'll definitely be looking into ways of providing support for this, as one of the concerns @edtjones raised is now solved (the Contentful SDK now properly marshals objects and can also build objects from raw JSON), which would allow this feature to be implemented. Cheers |
I'm a little confused as to how to best make use of the existing caching support.
I see in
lib/contentful_rails/caching/timestamps.rb
that this maintains atimestamp_cache_key
forContentfulModel
s that gets invalidated automatically if you tell Contentful to post back to your webhook. This also setsupdated_at
to cache based on this key. So, from my understanding, this allows us to do things like:where a
ContentfulModel
namedProduct
can be provided to Rails's cache method to provide the key. Very cool.But getting the full list of
Product
s is expensive too - it will take at least several hundred milliseconds and at worst many seconds.With a database, the Rails examples deal with this by using simpler, faster queries to determine a cache key, and then checking against that to see if they need to query the whole set:
But we can't do that, because no matter how simple the call to Contentful, it's still a remote web request and it's going to take forever in relation to other initial page load blockers.
So it seems like we need to maintain cache keys for each
ContentfulModel
class, which it then checks before refreshing an internally cached result set. These keys should be invalidated by a webhook POST for that Content Type.These seems like it might but probably doesn't fit under this item listed on the Readme's todos:
Can you confirm my thinking here? Is a solution like that planned? How have other people solved this problem?
The text was updated successfully, but these errors were encountered: