search

feathersjs-ecosystem / feathers-redis

A Feathers redis service adapter
MIT License
4 stars 0 forks source link

Discussion PR. (Not really for pull/merge.) #3

Closed appurist closed 7 years ago

appurist commented 7 years ago

This is just a discussion PR. This is not ready for merge into the feathersjs, but it may provide a basis for discussion about further work.

These files provide a possible initial (partial) feathers-redis implementation. Much of the code was lifted from feathers-mongo and/or feathers-nedb and/or feathers-memory in order to leverage some of the techniques in place there, and to try to maintain consistent semantics across feathers data services. The create and get methods are provided. The create method supports both objects and arrays and will use any id provided including custom id names. I will continue work on it to provide the other standard service methods (which really means make them work with the tests) over the next couple of days.

At this point get is returning the records from create and the tests are creating records but many of the tests are failing.

Other Information

The bigger issue is that the common feathers service API seems very thin when it comes to Redis data storage. I thought I was going to be able to at least store an array create as a Redis list, but I don't think that is possible since the feathers service API doesn't allow an id to be specified on a create. This means each array element must either have it's own id (which means it can't be stored as a Redis list), or I could assume if ids were not included in the data array, that it would be okay to provide a single id for all entries in the array, later retrievable on a single get, as an array. But I don't think the feathers service API actually really supports saving an array as a (single) resource/record, but rather only as a convenience feature for saving more than one resource. In other words, we could really use a setArray vs setMultiple here. Or perhaps a set that accepted a (single) id for the data argument.

To further expand this point, Redis supports some very cool data types, like logical sets, and hash tables, etc and the service interface here doesn't really allow any of that to be taken advantage of. So my approach in this initial attempt is to store the objects as JSON string resources, and store arrays as multiple JSON string resources, each with their own ID, either provided or auto-generated by this service.

Service-specific

There are some good things we can provide for Redis users though. One is data namespaces; it is common for redis IDs to represent a namespace hierarchy, e.g. 'users:123:posts:42'. I've (temporarily at least) added an option to specify an id namespace for a route, e.g.:

app.use('/messages', service({
  Model: client,
  paginate: {
    default: 2,
    max: 4
  },
  monitor: true,
  idPrefix: 'messages:',
  redisOptions: redisOptions
}));

Keys generated are currently GUIDs/UUIDs. The idPrefix will also be applied to any auto-generated keys. I'd also like to change do another update such that if an idPrefix was provided and a specified key did not start with it prefix, that it would also be auto-prefixed with that (Redis namespace).

Redis also supports a monitor event for logging. The monitor: true option above enables that to go to the console.

This is still very much a work in progress so it is too soon to even really document the example app, which is likely to change in significant ways still. So I don't think it's a good idea to merge this PR and I will continue to work in my fork for now while we discuss this.

appurist commented 7 years ago

To be honest, I'd be just as happy if someone else wanted to do this project, and feathers didn't use any of this. I'm doing it both as a learning project, and also to be useful: to fill the gap in feathers services. I am happy to provide it though if you feel it will eventually be good enough.

appurist commented 7 years ago

I've changed my tests to specify options.id = 'id'; and the internal check if it's not specified to use 'id' and that does change the id field to match, but it has no effect on the tests failing though. Am I correct in assuming that https://github.com/feathersjs/feathers-service-tests/blob/master/src/common-tests.js#L20 is the one failing? This even though with monitor=true, I can see (blue arrows) that the IDs seem to match what is expected? First two tests that fail

appurist commented 7 years ago

Okay, it seems this is the remove code. And it's doing a throw so that is why the tests are failing (and why the db is left with many people records). ;) I hadn't gotten to remove yet, so I shouldn't be running the tests yet really.

I'll try to finish off the other methods, testing each with the sample app before running the full test set.

ekryski commented 7 years ago

@appurist did you have more to add here or should we merge this and see if we can patch it up?

I'm still very interested in this adapter but we've been busy trying to get Auk released first. What we've started to do is modules that aren't ready we make them private and release them when they are ready. That way people don't report bugs on WIP repos.

If you aren't able to contribute more here, that's totally ok and we'll merge this and make the repo private. Let me know...

appurist commented 7 years ago

Yikes, in preparation for answering, I committed my current uncommitted mess. I forgot there was an outstanding PR from my fork.

Anyhow, the comment on the last commit says it all: The last commit was an attempt to map redis results in to Feathers, but I found that very complex to map the very different redis semantics into feathers.

The last couple of commits are questionable; they were part of a work-in-progress that was interrupted due to other priorities, and still has serious problems with promises. scanFrom.then should not be getting passed to try, etc. Unfortunately I was learning promises as I was going there and ran out of time for side projects.

Might be better reverting this and addressing the previous state, or someone who has mapped different db semantics to feathers ignoring this fork entirely and starting from scratch.

On the other hand, except for the promises problems, I had mostly worked out how to map redis to feathers for about the first half of the service methods, find being a bitch to map in. None of the feathers parameters are actually supported. You can't even ask for a specific number of results, or a specific range.

But my work load went exponential just before Christmas, and I won't be able to continue this effort, even a little, for quite some time, so I would encourage anyone who wants to do it to just go ahead and take over completely, and I wouldn't be even a little disappointed if this repo was just trashed completely and replaced with a new one. Whatever the developer working on it wants to do.

I'm disappointed that I wasn't able to do this in any reasonable time frame but I definitely underestimated it. Which probably means I made it too complex and someone should start from scratch, and only pull in whatever (if anything) they feel is useful from this.

appurist commented 7 years ago

On further thought, this would probably be a huge mess for anyone to figure out and use.

I was playing with calls in app.js rather than test cases, and running and debugging that instead of the tests. Even there, the change to the scan calls to match redis semantics broke almost everything.

Pretty sure someone should just continue without the PR, even completely ignoring it (it's my mess), although the code here may provide some redis-specific hints and might be useful as a reference (ignoring the promises handling).

daffl commented 7 years ago

As per the discussion in #4 I'll also close this PR but keep the branch so that now work is being lost.