hapijs / catbox-redis

Redis adapter for catbox
Other
69 stars 63 forks source link

unable to cache `false` via redis #90

Closed AndreasChristianson closed 6 years ago

AndreasChristianson commented 6 years ago

When caching a boolean in redis, the cached value cannot be retrieved. Below is a test from the perspective of a hapi server method showing that false cannot be cached then retrieved.

This test assumes that redis is running locally on its default port.

import Hapi from 'hapi';
import catboxRedis from 'catbox-redis';

describe('catbox-redis', () => {
    test('should cache bool results', async () => {
        const server = Hapi.server({
            cache: [{
                host: '127.0.0.1',
                engine: catboxRedis
            }]
        });

        server.method({
            name: 'returnsBool',
            method: () => false,
            options: {
                cache: {
                    expiresIn: 60000,
                    generateTimeout: 1000,
                    getDecoratedValue: true
                },
            }
        });

        await server.initialize();

        console.log(await server.methods.returnsBool());
        console.log(await server.methods.returnsBool());

        expect(server.methods.returnsBool.cache.stats).toEqual({
            sets: 1,
            gets: 2,
            hits: 1,
            stales: 0,
            generates: 1,
            errors: 0
        });
    });
});

console output:

console.log test/catbox-handles-bools.test.js:27
    { value: false,
      cached: null,
      report: { msec: 1.658251017332077 } }

  console.log test/catbox-handles-bools.test.js:28
    { value: false,
      cached: null,
      report:
       { error:
          Error: Incorrect envelope structure
              at Object.<anonymous>.module.exports.internals.Connection.Object.<anonymous>.internals.Connection.get (/Users/achristianson/sandbox/scarcity-deployment/node_modules/catbox-redis/lib/index.js:145:15),
         msec: 1.3352600038051605 } }

My naive searches turned up #59, perhaps this is related.

devinivy commented 6 years ago

Yes, #59 was originally handled as a part of #77. I think this line (below) ought to be adjusted to account for '' and false. Perhaps it should just ensure the property exists and ignore the actual value/contents.

https://github.com/hapijs/catbox-redis/blob/042750d3672b35f2eb6e5be7036d91c7085c6e6b/lib/index.js#L143

AndreasChristianson commented 6 years ago

If no one has fixed this, I'll get a pr open in a couple days.

marcuspoehls commented 6 years ago

@AndreasChristianson Hey Andreas, good catch! Would be great if you create a PR for this. Thank you!

marcuspoehls commented 6 years ago

@AndreasChristianson catbox-redis 4.2.1 is available allowing you to cache false

lock[bot] commented 4 years ago

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.