Add support for find and findAll methods

[ignacioblit]

Hello,

Myself and I believe many more users of this library would benefit of having the possibility of transparently caching more sophisticated queries, such as the result of a find, findOne and findAll methods.

I do understand that some of the core logic within this library relies in using cache keys composed of the model name and an id. This may be the reason why supporting this methods could be kinda cumbersome.

Yet I wanna propose something that may make this work without having to change this core logic on how caching keys are composed.

What I propose is that we use a hash composed of the options argument of the find methods as the id argument and keep the model name as is.

It would be something like this:

find (options) {
    const id = hash(options)
    return cache.get(client, model, id)
        .then(instance => {
          if (instance) {
            return instance
          }
          return model.find.apply(model, arguments)
              .then(instance => cache.save(client, instance, id))
        })
    }

cache.save could check if id is set, and override the default behaviour when this happens.

For the hash function we could use something like this:

https://www.npmjs.com/package/object-hash

Or simply stringify the JSON.

@DanielHreben let me know if you would approve such change and functionality. I don't mind tackling this issue myself as I would put it to good use, yet I wanna be sure you will merge it before staring.

[DanielHreben]

Yes, this is very useful functionality and I have a working implementation in one of my previous projects, but currently I'm not using it, so that's why it's not here yet.
If you interested in implement this - you are very welcome, but my vision is a bit different.
First of all, this is very different type of cache, it could not be automatically invalidated. Let's make it clear for end users, how about something like that:

Model.cache('manual').findAll({...})

or

Model.manualCache().findAll({...})

This way we will explicitly separate automatically invalidated cache and manual one.
Also, I like to support as more sequelize methods as we could (findAll, raw, findAndCount, ...), keeping their signatures unchanged, instead of providing one special find method.
Also, I imagine invalidation api like this:

Model.cache('manual').clear().findAll({...})

Please don't hesitate to ask anything if I described it unclear!

[ignacioblit]

Thanks a lot for the feedback @DanielHreben!

I completely agree with you, it should be made obvious that this type of cache needs more attention and that it wont be automatically invalidated. This is actually what worries me more about this implementation; the best way to clear such cache.

Model.cache('manual').clear().findAll({...})

Having to call the actual Sequelize method to clear the cache looks odd to me. In real use cases you are likely to clear it without the direct need of making such query. Yet your feedback gave me a nice idea, which is to give the id/hash responsibility to the user when the cache does not have the option of transparent invalidation. This would give the library the ability to support most Sequelize methods, without the need of really having to worry about the underlying process or key management, while making obvious to the developer managing such cache is on their plate.

I'm thinking something like

// To cache
Model.cache(myCustomHash).findAll({...})
// To invalidate
Model.cache(myCustomHash).clear()

This way if the cache() function receives a hash, it may cache whatever Sequelize method was called using a key combining the model name and the received hash. What do you think?

I'm mainly proposing this way of doing it, because I'm having a hard time thinking of the best way of clearing this cache without having to rewrite the query. If you have better ideas or suggestions please say so :)

BTW the find method is actually part of Sequelize, it's the same as findOne. But I see your point, it is better to provide more specific method signatures.

[DanielHreben]

@ignacioblit got it, totally agree with custom cache ids idea!
However, still not 100% sure about approach to separate custom and manual cache methods.

The most explicit one is to use different methods to access them, like autoCache().findById() + manualCache(hash).findAll(), but this way is not so compact in comparison with relying on arguments passed to cache().
Anyway, this detail should be easy to change so feel free to bring some pr if you keen!

[DaveStein]

Let's say you have two models, User and Photo.

You might want to save off a cache keys as user:photos and photos:featured. This would imply you'd want to either delete or update cache keys in at least four situations:

1. User deletes account - remove from featured list
2. Photo updated - updated title in cache for both keys
3. Photo deleted - remove from both keys
4. Photo no longer featured - remove from featured list

It would be unfortunate if any place you want to update the photo you need to remember to update each of those keys individually. I'd recommend something like this:

When updating photos, photoinstance.cache().update would look to see if Photo.getCacheLists exists and then call it. It would return an array methods to run to save over the keys. In this case, it would reference a method in YourFeaturesObject that does the findAll and saves it to photos:featured, followed by another method in User that does its findAll for user:photos

Make sense? Or was this incoherent? :)

[ignacioblit]

Hey @DaveStein this totally makes sense, yet I don't think solving his automatically in with the library would be viable or even possible.

Imagine you have

UserModel.cache(myCustomKey).findAll({include: [{
    model: PhotoModel,
    where: {rating: 10}
   }]})

This would get all users which have photos of rating 10 and these same photos. Trying to automatically update or delete myCustomKey when you update one of these N users or photos would be a pain, and prone to error, specially if you start joining tables together.

I believe that managing this type of custom/manual cache should be on the developers end, as what you propose seems 'too much magic', hard to implement and really prone to errors if not enough considerations are made.

I have to give to you that the idea sounds nice and does makes sense, but when i start to think about all the possible queries and cases and the underlying implementation to make this work properly i simply do not see it viable.

I may be wrong, though I would simply start with the possibility of manually caching some queries and leaving such cache responsibility on the developers end.

My suggestion would be to leave your implementation for some another issue/pull request. Yet if you believe i'm wrong and see this implementation easier than what I describe I'm open to trying to tackle this together.

[DaveStein]

Here is code sample. The passing of original vs updated entities is TBD cause I'm not inside the code but....

class UserModel {
  // called on update or destroy of user
  static getCacheLists() {
    return [this.setRatedCacheKeys]
  }

  static setRatedCacheKeys(originalUser, updatedUser) {
    const data = UserModel.findAll({include: [{
      model: PhotoModel,
      where: {rating: 10}
    }]})

    // Ignore this implementation, but the gist is to set after having selected outside of cache
    UserModel.cache().set('mykey', data);
  }
}

class PhotoModel {
  // called on update or destroy of photo
  static getCacheLists() {
    return [this.setUserRatedCacheKeys]
  }

  static setUserRatedCacheKeys(originalPhoto, updatedPhoto) {
    // No need to change cache
    if (originalPhoto.rating === updatedPhoto.rating) {
      return;
    }

    // a change was made that would impact that specific list
    if (originalPhoto.rating === 10 || updatedPhoto.rating === 10) {
      User.setRatedCacheKeys();
    }
  }
}

So there is still responsibility on the user's end but no updates to user will go by without checking. I can add updates to rating via 20 different methods, and I know that cache key won't accidentally get stale.

[ignacioblit]

Hi! Sorry for the delay, this was not on the top list of my priorities, though I managed to get some work done:

#32

I believe it is enough to get this one going.

@DaveStein I do see what you mean on your example, but that looks way to specific to a particular case. If you want you can check the actual implementation on my Pull request, and see if your idea would fit. I'm open for suggestions.

Cheers!

[DaveStein]

I'll try and sneak a look next week, although I wouldn't be implementing anything on my end for a real try for a bit. A few more higher priority things came in at work before I get to "choose final solution for caching" lol. Right now I just got some minor bits up from what I posted before to make sure my app is properly communicating with Redos :P

[DanielHreben]

Hi @ignacioblit and @DaveStein ! I just published version 2.0.1, with .manualCache() support. The only one missing thing is docs, I will update them eventually, please give it a shoot btw.

[ignacioblit]

Thanks a lot @DanielHreben! I do see the merge and that you also went ahead with the requested changes.

Im appreciate that :) I will certainly give it a shoot.

BTW: In Sequelize the find method maps to findAll, why remove it in this case?

[DanielHreben]

@ignacioblit I was now able to call this method on latest sequelize, that's why I was not sure if that is part of sequelize api, but I don't mind to add this alias if you can confirm that was part of sequelize api at some poit.

[ignacioblit]

Hi @DanielHreben. I'm currently using the find call in a couple of places with "sequelize": "^4.42.0"

Yet I just googled and browsed for a while and do not see it very documented. So it would seem the find method is not really encouraged. Having said this I will probably remove it from my codebase and I do not see adding support all that important as it is not very present on the sequelize documentation.

[modestfake]

@ignacioblit Method find was marked as deprecated in v4.41.0 (#9933) with a couple of other methods and removed in v5. You might see a warning when using it in 4.42.0.

[DanielHreben]

Ok, I finally updated documentation, so we can close this now! However, there is no more manualCache() method in new version, so for all early testers, make sure you updated your code.