fakeredis doesn't seem to be working

[doapp-ryanp]

This is a continuation of https://github.com/arjunmehta/node-geo-proximity/issues/19

First off thanks for the re-write and the sorting!

I'm using nearby() with fakeredis and getting the following error:

Error: fakeredis: COMMAND is not implemented in fakeredis. Let me know if you need it.

From what I can tell, it looks like it is using georadius. Any idea if this error is coming from your code or fake redis? My hunch is the latter - and if so you know any workaround?

As I mentioned in the previous issue, I have to use fakeredis because I am using AWS lambda which does not currently have access to a redis server (via elasticache). Also my data set is pretty small so would like to pay the potential performance penalty to remove another thing I have to keep Highly Available.

Any ideas?

Here is my code (roughly):

...
var defaultNumResults = 10,
    defaultRadius     = 10,
    redisClient       = null,
    placeProximity    = null,
    nearbyOptions     = {
      withCoordinates: false, // Will provide coordinates with locations, default false
      withHashes: false, // Will provide a 52bit Geohash Integer, default false
      withDistances: true, // Will provide distance from query, default false
      order: 'ASC', // or 'DESC' or true (same as 'ASC'), default false
      units: 'mi', // or 'km', 'mi', 'ft', default 'm'
      count: 10, // Number of results to return, default undefined
      accurate: true, // Useful if in emulated mode and accuracy is important, default false
    };

exports.closestPlaces = function(lat, lng, numResults, radius) {
  if (!placeProximity) {
    redisClient    = require('fakeredis').createClient(0, '', {
      fast: true,
    });
    placeProximity = Promise.promisifyAll(require('georedis')).initialize(redisClient);
  }

  //Fastest: http://stackoverflow.com/questions/5349425/whats-the-fastest-way-to-loop-through-an-array-in-javascript
  for (var i = 0, len = channelsList.length; i < len; i++) {
    var entry = channelsList[i];

    try {
      placeProximity.addLocation(JSON.stringify(entry), {
        latitude: entry.lat,
        longitude: entry.lng,
      }, function(err, reply) {
        if (err) {
          console.error('error adding', entry, err);
          return Promise.reject(new MlnError(
              'Error adding' + JSON.stringify(entry, null, 2),
              MlnError.errorCodes.UNKNOWN
          ));
        }
      });
    } catch (e) {
      console.error('Error adding location', entry, e);
      return Promise.resolve([]);
    }
  }

  nearbyOptions.count = numResults || defaultNumResults;

  return placeProximity.nearbyAsync({latitude: lat, longitude: lng}, radius || defaultRadius, options)
      .then(function(locations) {
        return locations;
      })
      .catch(function(e) {
        return [];  //any error return cant find
      });
};

[doapp-ryanp]

https://github.com/hdachev/fakeredis/blob/369ba2df36df5888cdaf23b55aacea75be7dec37/lib/connection.js#L421 is where fakeredis is throwing the error - I cant seem to tell where in your code the literal command COMMAND is being specified. Sorry I'm not much help here, redis is a bit out of my area of expertice

[arjunmehta]

Agh, how annoying. An asynchronous method using throw. Okay, this is an easy fix I think...

Updated on the master branch if you'd like to check? I've tried testing it on my end, and it seems to work for the main commands. Though it seems to bug out with large sets, which I'm pretty sure is a limitation of fakeredis.

[doapp-ryanp]

I REALLY appreciate you getting on this so fast. This fixed the reported bug, however now:

• I started with 'mi' and I was getting incorrect results, when I switched back to 'm' it is working better but still not always accurate (more on this below)
• I get exactly one location returned from nearby() even if I remove the count option or set it to 100.
• As you can see above I use withDistances: true and units: "mi". The objects returned by nearby() return it in meters correct? I'm cool with this, and honestly don't want to spend CPU cycles doing the conversion.

So regarding the accuracy - I'm hoping that the count bugfix will resolve this issue however, here are some of the test points that are not working correctly that should help you re-produce.

• {latitude: 44.01, longitude: -92.47} correctly returns Rochester, MN: {"name":"Rochester, MN","lat":44.0150757,"lng":-92.4775256}
• {latitude: 42.7086815, longitude: -84.559032} INCORRECTLY returns Dimondale, MI: {"name":"Dimondale, MI","lat":42.6455915,"lng":-84.6488707} with a distance of 10166 (which is outside of my 8046.7 radius range...). As you will see in my dataset there is a

"name": "Lansing, MI",
    "lat": 42.7090686,
    "lng": -84.5584646,

I have emailed you my data set, as well as my full code (its simple) so you can copy/paste reproduce.

[arjunmehta]

@doapp-ryanp Hey Ryan!

Glad that fixed that issue. I'll need to take some time to dig into this other issue though. I'm swamped for the next couple days, unfortunately.

If you do your initial query with miles mi, everything will be in miles. Including the radius for search, the distances returned, etc.

Will dig into it more soon!

[doapp-ryanp]

So I think I figured it out - you can see above that I am using accurate: true, if I switch it to false things work. I figured that fakeredis did not have native geo commands, so that is why I initially set it to true. I will let you determine if this is a bug or not, if not feel free to close issue. Thanks!

[doapp-ryanp]

FWIW count does not seem to be working, i have count set to 10 and sometimes I get more than 10 results. Not a big deal to me though, as I just slice(). And returned distance is always in meters even if I specify mi

[arjunmehta]

Wow, some weird things happening overall. I'll see if this is all because of fakeredis or if there's something else going on!

Thanks for this.

[arjunmehta]

Hey @doapp-ryanp Could you give it a try now (from master)? Your sample code was helpful! I found some pretty horrible bugs with count and when using the accurate option and sort together.

Thanks!

[doapp-ryanp]

That fixed it! Count and 'mi' now works as expected. I'm gonna be testing this quite a bit over the next month or so, I will report back how it goes. Thanks!

[arjunmehta]

Awesome! I will publish this to npm so others can benefit. :)

Thanks for pointing out all the issues!

[doapp-ryanp]

np, will be on the lookout for the publish

[arjunmehta]

Published!

By the way, if you plan on using this in production, and the geo stuff is a big part of your app, you may want to consider using this module with real Redis 3.2 when it comes out. The native geo commands are really fast and the sorting and units stuff will save a lot of CPU cycles. It's almost 20 times faster when done by Redis, but I understand that adds a dependency, and makes the app less portable for you.

In any case, definitely let me know if you encounter any other issues moving forward.

[doapp-ryanp]

Hmm https://www.npmjs.com/package/georedis is still listing 3.0.1 for me

[arjunmehta]

That's the latest one! Is it not working as expected?

[doapp-ryanp]

nope sorry we are good. for some reason I coulda swore i was 3.0.1 before, but I didnt notice you version bumped package.json days before publish. we are good

[arjunmehta]

Yeah I held off publishing 3.0.1 till all those issues were solved.