EXPIRE/TTL and primaryKey issues

[richdunajewski]

Like several others have posted before me, it looks like EXPIRE isn't properly supported by this adapter, which is a major factor in selecting Redis over other data stores. I'm using Redis for caching, and need the keys to expire automatically.

My initial workaround which used the native connection to set the EXPIRE value on the created keys worked great, until the key expired and was flushed from Redis, however the keys created by Waterline which contain the primary keys of the data are not cleaned up. Thus, when attempting to locate something in Redis, it does not exist and I would try to create the key. This results in a ERROR Error (E_UNKNOWN) :: Encountered an unexpected error AdapterError: Record does not satisfy unique constraints error because the primary key is already in the Waterline keys waterline:cache:_indicies and waterline:cache:id (cache being my model and id being my primaryKey.)

Simply setting EXPIRE on the Waterline keys at the beginning of the program won't work, because Waterline will recreate these keys if they are not present (already expired), and the EXPIRE timeout will not be set, and you'll be back at square one.

The typical user may not notice this if they are using autoPk: true, because the IDs will be incrementing and a conflict should not happen (although you'll see the Waterline keys will contain references to expired keys anyway, and keep growing after the data is long gone). Since I'm using a custom primaryKey, there is always the chance for conflict.

I think we need to look into this and come up with a solution that allows setting the TTL on keys and also manages the indices Waterline creates. I'm relatively new to Redis, so I don't pretend to have the answers, but I think I identified a problem many Redis users will run into.

[jonathanwiesel]

+1.

@richdunajewski what are your thoughts on the functionality implemented on commit f6730ad034bc63cc0b5443552c07bab5e1fc72bd on the 0.9.x branch but never published to npm?

[timestep]

+1 for this. Expiry is pretty integral for me to use Redis.

[jnorberg]

+1.

[lukasredev]

+1

[Ryanc1256-zz]

Since Expire isn't really a valid waterline value it's kinda hard to keep the consistencies between all the waterline adapters and keep the api's the same saying that, i do believe the ttl/expires value would be super awesome to add, and it's one of the coolest features in Redis.

@jonathanwiesel Looking at the other guys commit it shouldn't be too hard to add into the current branch with a pull request to bring up to the master branch.

Lets see if i can do that sometime tomorrow huh, since i have fixed all the other problems haha

[Ryanc1256-zz]

So had a talk with the waterline guy's it look's like we are just going to put it in the waterline and then these adapters will get updated to support ttl

[timestep]

:+1:

[dmarcelino]

:+1:

1 - I believe there is room for a TTL interface in waterline given several datastores support this.

2 - Even for datastores/adapters that don't support it I think waterline could provide a shim as it's not incredible difficult to do. Simple example: https://github.com/dmarcelino/connect-waterline/blob/master/connect-waterline.js#L120

cc: @devinivy

[camsjams]

+1

any update on this?

[Ryanc1256-zz]

@camsjams not really haha, we have talked about but apart from that not really.

It's a feature we want to add, but it would need to be added to waterline so it's kinda up to the waterline team to put it in, so then i can update the adapter to put it in :smile:

[sailsbot]

Thanks for posting, @richdunajewski. I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message. On the other hand, if you are still waiting on a patch, please:

• review our contribution guide to make sure this submission meets our criteria (only verified bugs with documented features, please; no questions, commentary, or bug reports about undocumented features or unofficial plugins)
• create a new issue with the latest information, including updated version details with error messages, failing tests, and a link back to [the original issue](). This allows GitHub to automatically create a back-reference for future visitors arriving from search engines.

Thanks so much for your help!

[dmarcelino]

@sailsbot, I would say this one of the definitive features that sails-redis should support, so I vote for reopening this one.

[Ryanc1256-zz]

Yeah agreed re-opened

[sailsbot]

Thanks for posting, @richdunajewski. I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message. On the other hand, if you are still waiting on a patch, please:

• review our contribution guide to make sure this submission meets our criteria (only verified bugs with documented features, please; no questions, commentary, or bug reports about undocumented features or unofficial plugins)
• create a new issue with the latest information, including updated version details with error messages, failing tests, and a link back to [the original issue](). This allows GitHub to automatically create a back-reference for future visitors arriving from search engines.

Thanks so much for your help!

[tcharlat]

This behaviour just gave me a good headache.

1 we can set expire ttl ourselves 2 but the primaryKey is stored and won't disappear, leading to a sails-thrown uniqueness error.

The problem is that in most use cases involving a redis DB, we don't want sails to manage uniqueness for us, AT ALL. Uniqueness is already managed by the primaryKey ! We write there, or should be anyway.

I am currently reading index.js, and it's clear that the uniqueness and primaryKey checks should be optionnal, instead of throwing. The key string, ie 'waterline:collectionname:pkname:pkvalue' should also be under our control instead of imposed by the adapter. Redis is meant to be shared between instances, and not all of them are sails. Even if we had only sails server, it would force us to use the same tablename and pkname between our instances !

Redis is a key value database and repeatedly finding an item with a constant 'pseudo-key' (like uid, or email) wich is hidden inside the object, and an index beeing used as the key instead is a major anti-pattern. We ask redis to FIND{where} instead of asking redis to GET key.

It's sad, because to use Redis at least half correctly we have to shortcut waterline and sails-redis almost completely and lose a lot of useful standardization, model definition, type checking, and lifecycle callbacks.

[Ryanc1256-zz]

@Kallikrein agreed....

i've been looking into this problem for a bit now.... I think i have a good way of doing it but it requires quite a bit of change not only on the redis adapter side...

I'm hopefully going to share it with everyone this weekend, i might get super busy, but I'll try....

[tcharlat]

@Ryanc1256 Thanks.

Is there a communication channel like slack, discord or irc where we might chat together about participating and some todo's ?

For the time beeing I identified a set of points that contradict my vision of an ergonomic redis orm.

A lot of tests define a valid waterline adapter. I don't know wich tests concern waterline compliance or only redis behaviour. The problem I have with waterline is that I don't know if I do agree with the opinionated default adapter behaviour, I would appreciate insight on what is possible and what is not before trying to fit square features in a round loader...

in ./test/unit/adapter.create.js

it('should return error on non-auto incrementing primary key'

It should not.

it('should not create record with non-unique attributes'

It should.

Almost all of ./lib/database/sequence.js should be optionnal

  // Build a NoSQL Key name for this sequence
  this.keyName = 'waterline:' + collectionName.toLowerCase() + ':_sequences:' + this.name;

./lib/database/schema.js

Schema.prototype.indexKey = function(collectionName, index) {
  var name = collectionName.toLowerCase();
  return 'waterline:' + name + ':' + index;
};

./lib/database/indice.js

  // Build a NoSQL Key name for this sequence
  this.keyName = 'waterline:' + collectionName.toLowerCase() + ':_indicies:' + this.name;

We need control on these strings

As far as my opinion goes, I think the expected behaviour is the following for most use cases :

• It should use primaryKey as a redis GET key
• If defined in attributes, it should execute a keyGen() instance method function as a lifecycle callback to generate the key. keyGen : function() { return ['myKeyPrefixIWroteMyself', this.primaryKey].join(':')}
• If defined as a class method, it should execute a keyGen class method function during adapter loading and store the return value as a prefix for future key generation.
• It should remove the primaryKey property before outputing JSON object to store (redundant with key)
• It should accept an expire value in the model definition and set expire property
• If defined in attributes, it should execute a expire() instance method function as a lifecycle callback at each creation
• If defined as a class method, executing Model.expire(int) should set the expire value for the model.
• Create, save or update should be able to have ttl as an argument

I fear a lot of these features would contradict waterline dogmas.

The problem is that waterline wants us to be able to swap db from one to another, but we don't want to do that. It's looking for the smallest common denominator, alas clearly opinionated from a sql point of view. It is a very good thing when dealing with sql database, but it's poorly hacking how mongo or redis have a different perspective on storing data.

Maybe a better solution would be to create a concurrent adapter, focused on servicing a redis database ?

I would also gladly give up almost all of aggregate.js, since waterline grouping syntax is as complicated as native query - but way slower because of the agnostic layer. Plus, the key:value is what we want redis for, we don't want complex queries. Even if we do, we won't use waterline standard api since it's focused on doing good queries for sql and will most likely not expose the correct redis native functionality, since it's not conforming to sql standards.

I wonder, before I start doing another adapter, how waterline will let me in control of my custom adapter. I would like to be sure I won't mistake brick walls for doors - since I open doors with the head :)

I hope those interested in maintaining or forking redis support would gather and talk together to share knowledge and experience, and eventually plan for future dev...

See ya

[particlebanana]

@Kallikrein I'll let @Ryanc1256 chime in but I have always seen Redis as a very different adapter than all the others.

Just curious though at what point do you actually need Waterline for the redis stuff you mentioned? Why not just require("redis") and set and get the keys you defined?

[Ryanc1256-zz]

@particlebanana i find that waterline just makes everything nicer, yes you can do everything with your normal get a set, but if you want to do some relationship stuff, thats when it gets hard I must admit...

Also I do find that Redis and Mongo has always been a Very different adapter just because of the nature that it's not a query db, it's a key storage one... well mongo is a document storage... but very similar...

Which sometimes makes it hard when waterline kinda of feels like it was more made in for sql based db's in mind.... but of course you either have one of the other side i guess or have massive adapters with massive libraries of code... Just my opinion anyway.

@Kallikrein well, you can email me about it, since im guess its very different time zone where you are to me :p and i think my email is on my profile...

If you have a google hangouts account, i can im you when im at work ( for the next 8 hours ish )

I'm going to write up a changelog, on my redis or in a PR idk fork that i propose to change in this adapter to bring the speed and bring it back to normal redis usage, but backed by how waterline works...

So people can comment on it... I would love to have people's input on it... but i'll let everyone know when it's up there.

Before you do something/rewrite something, just message me... i've been thinking about this quite a bit and could you any help really :P

[tcharlat]

@particlebanana

A very good question !

I like the model / attribute definition, type checking, and standardized lifecycle callbacks.

I give you my use case :

A user signs up with an email.

Validation :

• beforeValidate I check this email is not already used (in postgres) and I generate a random token
• I check that it is a valid email
• I check the password is 8 char long

Creation :

• beforeCreate I hash the password
• I create a temporary key:value entry (KEY email {password: password, token: token}) expires 2 hrs
• afterCreate I set TTL, then send an email to my user.

In another route I receive the token/email through url contained in the email.

I query REDIS for the email as the key, check token validity. If it's fine, I create the user in my postrgres.

I could use my own factory/decorator logic but that would mean getting it out of alpha limbos :), and I'm kinda in a rush, trying to lift an app as the only dev and with no salary until we launch.

I finally opted to keep using waterline for my model definition. It keeps things standards and tidy, it's much more developed, there is a community around it and I like it.

I just needed to set ttl, once I scaned my redis I checked for the key syntax, I then log'd the model instance, then wrote my own expire() method that works pefectly fine. Since I use the id returned by create(), the access is O1. This solution is stable and will scale pretty well, since this process is not frequent.

BUT it is totally suboptimal, as far as I understand clearly.

• I let vanilla sails-redis manage the primary key. Since the autoincrement is up, I will never have a uniqueness thrown at my face.
• I let vanilla sails-redis manage the find process. It's a pity, since the find({email: email}) is much slower than a get(key). I can't get the key, because I don't know it, it's an autoincremented int :'(
• There is a lot of uneeded data generated : Indices and unique tables, index key (instead of the primaryKey). I don't know how find() is working yet in detail, but I'm afraid it's not O1 and most likely On with a lot of potential ram and proc waste.

For the time beeing I can stick to sails-redis. If I need more ooomph I will have to adress sails-redis or making a key-focused custom adapter (which will not be waterline canon).

PS : I wish to thank everyone involved in sails / waterline / adpater dev and support. I greatly appreciate the time saved and the robustness of this framework. Let's notice the irony that you question using waterline and adapters :)

@Ryanc1256 It's the evening here. (France) I sent you a hangout request (whatever that means :D)

[Ryanc1256-zz]

@Kallikrein I totally agree with everything you just put down! :P

and Its early morning here (NZ) and I think i added your to hangouts, you just gotta accept it :P

[tcharlat]

Edit : I can send the id returned by sails-redis create() in the email instead of ... the email. I indeed have the key. (thanks for rubberducking my backend sir)

So except for a little (but growing) waste of ram in the unique key collection, I don't have any good reason not to use waterline. Since I don't think millions of user are about to saturate my redis of wasted id storage... Safe for work.

I will most probably track down and kick uniqueness tables creation in my own fork when I have the time. The ideal micro-fix would be to create sails-redis specific class methods (keyGen(), expire, get(), set()), as proposed above.

I need to understand how waterline factory is producing ModelClass to properly access my custom model options. It might be shorter than we think, as long as it's considered valid to customize an adapter at such length.

[Ryanc1256-zz]

@Kallikrein here ya go buddy! @particlebanana could use your input too... see what you think.. :) https://github.com/balderdashy/sails-redis/pull/90

[camsjams]

As a related note to anyone using Sails, waterline and Redis, I've released a library that caches Waterline responses from a DB etc into Redis as a caching layer: https://www.npmjs.com/package/treasury

[particlebanana]

@camsjams awesome! I always thought Redis would work great for this.

[tyronedougherty]

Hi guys, any update on the current status of this, or any potential work arounds? Or is this feature more of a long term goal?

Many thanks guys, appreciate all your work on this awesome adapter :)

[Ryanc1256-zz]

@tyronedougherty it's defiantly a long term thing due to the fact waterline needs to support it before we can implement it...

[johnabrams7]

@Ryanc1256 @tyronedougherty @particlebanana @camsjams @Kallikrein @dmarcelino @richdunajewski @jonathanwiesel @timestep @jnorberg @lukasreichart Hey everyone, we're moving all the redis issues to the main Sails repo to gather more exposure to the community. Thanks for all the input so far!

[raqem]

Hi all @Ryanc1256 @tyronedougherty @particlebanana @camsjams @Kallikrein @dmarcelino @richdunajewski @jonathanwiesel @timestep @jnorberg @lukasreichart we are further cleaning up the issues by closing old issues. Please let us know if you think this issue should be reopened.

[mikermcneil]

sails-redis no longer implements the "semantic" interface